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

Fix assertion check for structure of enhanced-for loop over a Map keySet #868

Merged
merged 4 commits into from
Nov 27, 2023

Conversation

msridhar
Copy link
Collaborator

@msridhar msridhar commented Nov 25, 2023

Fixes #866

Before, we would check that an enhanced-for loop includes a call to Set.iterator() on the result of calling Map.keySet(). However, it is possible and legal that statically, the target of this call is instead Collection.iterator(). So, we change our check to test the receiver type passed into the call (which must still be a Set). Also, opportunistically switch a couple of places we were throwing RuntimeException around this check to throw the more meaningful VerifyException. Unfortunately, we have not found a way to add a test in open-source to reproduce the failure from #866 but we have confirmed this change fixes the problem.

Copy link

codecov bot commented Nov 25, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (8f4f8a6) 87.02% compared to head (ff60b56) 87.04%.

Files Patch % Lines
...llaway/dataflow/AccessPathNullnessPropagation.java 50.00% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #868      +/-   ##
============================================
+ Coverage     87.02%   87.04%   +0.02%     
  Complexity     1922     1922              
============================================
  Files            77       77              
  Lines          6219     6221       +2     
  Branches       1210     1208       -2     
============================================
+ Hits           5412     5415       +3     
- Misses          403      404       +1     
+ Partials        404      402       -2     

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

@msridhar msridhar marked this pull request as ready for review November 25, 2023 23:08
@msridhar
Copy link
Collaborator Author

The test coverage failure is expected here as we do not expect the exceptional cases to actually occur

@msridhar
Copy link
Collaborator Author

@yuxincs @lazaroclapp could one of you review?

@msridhar msridhar changed the title Another possible fix for #866 Fix assertion check for structure of enhanced-for loop over a Map keySet Nov 26, 2023
Comment on lines 645 to 646
&& ASTHelpers.isSubtype(
ASTHelpers.getReceiverType(invocationTree), containingTypeSupplier.get(state), state);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is pretty subtle, do we want to add a comment here stating that symbol.owner.type won't work for certain compilers?

Copy link
Collaborator

@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.

Maybe we want to add a comment for future readers here?

Anyways, this fix LGTM :)

@msridhar msridhar merged commit bf74867 into uber:master Nov 27, 2023
9 of 10 checks passed
@msridhar msridhar deleted the try-fixing-866 branch November 27, 2023 23:20
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.

java.lang.RuntimeException: expected call to iterator(), instead saw null
2 participants