Fix JDK compatibility issue in LombokHandler and introduce AstHelpersBackports#795
Fix JDK compatibility issue in LombokHandler and introduce AstHelpersBackports#795
Conversation
Pull Request Test Coverage Report for Build #1150
💛 - Coveralls |
msridhar
changed the title
lazaroclapp
left a comment
lazaroclapp
left a comment
There was a problem hiding this comment.
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.
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.
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().
3 participants
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:testJdk8task 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 anAstHelpersBackportsclass to contain any backported logic fromAstHelpersto enable the fixes.