-
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 JDK compatibility issue in LombokHandler and introduce AstHelpersBackports #795
Conversation
Pull Request Test Coverage Report for Build #1150
💛 - Coveralls |
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.
One minor question which applies to the code both before and after, but otherwise, this LGTM!
* <p>Same as this ASTHelpers method in Error Prone: | ||
* https://github.com/google/error-prone/blame/a1318e4b0da4347dff7508108835d77c470a7198/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java#L1148 | ||
* TODO: delete this method and switch to ASTHelpers once we can require Error Prone 2.20.0 | ||
*/ |
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.
Not sure I get, from these docs, what's the difference or problem with calling Symbol.getEnclosedElements()
directly...
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.
See here. Sometime after JDK 11, some subclasses of Symbol
were changed to override getEnclosedElements()
with a covariant return type (returning com.sun.tools.javac.util.List
instead of java.util.List
). So, e.g., if your code directly invokes getEnclosedElements()
on a variable of type ClassSymbol
, and you compile on JDK 17, the bytecode signature at the call site will have a return type of com.sun.tools.javac.util.List
. Then when you try to run this code on JDK 11, it fails, since a method with that exact return type cannot be found. This issue occurs even if you set your source and target bytecode level to 11 (or 8). Using this wrapper method works around the problem, since the declared target of the call will always be Symbol.getEnclosedElements()
.
Fixes #794. Since the JDK interface change was made for JDK 17, I think even if NullAway was built on and targeted JDK 11, we would still have this problem. I added some hacky unit test coverage for this code so we'd have a way to test for regressions in the future. (The new unit test fails the
:nullaway:testJdk8
task without this change.)We missed this problem earlier since we suppressed a warning from
AstHelpersSuggestions
. Even though we cannot accept some of the suggestions from that check (since we may not require a recent enough version of Error Prone), the issues it flags can be serious. So, this PR removes all other suppressions of that checker and fixes the warnings. We introduce anAstHelpersBackports
class to contain any backported logic fromAstHelpers
to enable the fixes.