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 short-circuit || in non-conditional flows #227

Merged
merged 2 commits into from
Apr 3, 2024

Conversation

sonalmahajan15
Copy link
Contributor

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 ]

Copy link

github-actions bot commented Apr 1, 2024

Golden Test

Note

✅ NilAway errors reported on standard libraries are identical.

3261 errors on base branch (main, 29def5e)
3261 errors on test branch (1312f57)

Copy link

codecov bot commented Apr 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.25%. Comparing base (29def5e) to head (bc65217).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #227   +/-   ##
=======================================
  Coverage   87.25%   87.25%           
=======================================
  Files          60       60           
  Lines        7727     7731    +4     
=======================================
+ Hits         6742     6746    +4     
  Misses        807      807           
  Partials      178      178           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@yuxincs yuxincs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comprehensive test suite!

Let's keep this in mind for the potential false negatives and think of more general ways to handle this in the future :)

@sonalmahajan15 sonalmahajan15 merged commit 755a685 into main Apr 3, 2024
8 checks passed
@sonalmahajan15 sonalmahajan15 deleted the sonalmahajan15/support-short-circuit-or branch April 3, 2024 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix false positive for a corner case of short-circuiting expressions
2 participants