Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for complex short-circuiting in non-conditional flows #226

Open
sonalmahajan15 opened this issue Apr 1, 2024 · 0 comments
Open
Labels
enhancement New feature or request false negative

Comments

@sonalmahajan15
Copy link
Contributor

NilAway can support prevalently found short-circuiting in non-conditional logical expressions. Examples of supported expressions are shown below (refer to nonconditional.go for detailed test cases).

return x != nil && x.f != nil && x.f.g != nil && x.f.g.h == 1
return x == nil || x.f == nil || x.f.g == nil || x.f.g.h == 1

Although not common, NilAway reports false negatives for complex short-circuit expressions, such as below.

return (a != nil || b == nil) && *a == 1
return !(a != nil && b != nil) && *a == 1

Following are the ideas that can be used to extend support for the above cases:

  1. Short-circuit expressions in conditional flows (e.g., if (a != nil || b == nil) && *a == 1 {}) are well-supported in NilAway. So, the idea here is to transform the non-conditional expression to a conditional expression in the pre-process phase. However, this would require the AST to be modified. We currently don't do this since drivers such as nogo create a shared AST between all linters. However, if we deeply copy the AST in NilAway, then this idea could be a viable approach.
  2. Emulate complier logic for evaluating short-circuit expressions.
@sonalmahajan15 sonalmahajan15 added enhancement New feature or request false negative labels Apr 1, 2024
sonalmahajan15 added a commit that referenced this issue Apr 3, 2024
This PR adds support for short-circuit `||` in non-conditional flows,
thereby reducing false positives. For example,
```
return x == nil || *x == 1
```
was reported as a false positive, since NilAway only analyzed `&&`.

We apply logic similar to the handling of `&&`. Because this analysis
resides in the recursion of the short-circuit expression, where we have
limited context visibility, it makes it difficult to accurately analyze
complex expressions. Therefore, with extending support for `||`, we had
to trade-off some of the precision we previously had with only the `&&`
support. However, we made this tough choice since empirically we
observed that complex cases that we had to trade-off did not occur
frequently enough, while simple `||` patterns were more prevalent. I
have created issue #226 to keep a track of the comprehensive support
that we plan to add in the future.

[closes #92 ]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request false negative
Projects
None yet
Development

No branches or pull requests

1 participant