-
Notifications
You must be signed in to change notification settings - Fork 299
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
Conversation
Codecov ReportAttention:
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. |
The test coverage failure is expected here as we do not expect the exceptional cases to actually occur |
@yuxincs @lazaroclapp could one of you review? |
&& ASTHelpers.isSubtype( | ||
ASTHelpers.getReceiverType(invocationTree), containingTypeSupplier.get(state), state); |
There was a problem hiding this comment.
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?
There was a problem hiding this 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 :)
Fixes #866
Before, we would check that an enhanced-for loop includes a call to
Set.iterator()
on the result of callingMap.keySet()
. However, it is possible and legal that statically, the target of this call is insteadCollection.iterator()
. So, we change our check to test the receiver type passed into the call (which must still be aSet
). Also, opportunistically switch a couple of places we were throwingRuntimeException
around this check to throw the more meaningfulVerifyException
. 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.