Fix bugs in reading varargs annotations from bytecodes #1055
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1055 +/- ##
============================================
- Coverage 87.61% 87.60% -0.02%
- Complexity 2183 2203 +20
============================================
Files 85 85
Lines 7161 7187 +26
Branches 1404 1415 +11
============================================
+ Hits 6274 6296 +22
- Misses 453 456 +3
- Partials 434 435 +1 ☔ View full report in Codecov by Sentry. |
|
Found another bug, will try to fix it as part of this PR |
msridhar
changed the title
|
Now ready for review again 🙂 |
lazaroclapp
approved these changes
Oct 13, 2024
| "public class Test {", | ||
| " public void testDeclaration() {", | ||
| " String x = null;", | ||
| " Varargs s = new Varargs(x);", |
There was a problem hiding this comment.
Can we add the case passing new Varargs(x, y) where x is non-null but y is @Nullable?
| " String[] y = null;", | ||
| " // BUG: Diagnostic contains: passing @Nullable parameter 'x'", | ||
| " RestrictivelyAnnotatedVarargs.test(x);", | ||
| " RestrictivelyAnnotatedVarargs.test(y);", |
There was a problem hiding this comment.
Is there a way to annotate varargs as non-null array of non-null elements? Do we want a test case for that? (separate from RestrictivelyAnnotatedVarargs.test)
I'd even argue that it is exceedingly rare for the intention of a varargs argument to be "a potentially nullable array of non-null elements", but I guess we should follow JSpecify here either way.
Comment on lines
347
to
348
| switch (position.type) { | ||
| case METHOD_FORMAL_PARAMETER: |
There was a problem hiding this comment.
If we care only about one case, why is this a switch and not an if?
copybara-service bot
pushed a commit
to google/error-prone
that referenced
this pull request
Oct 17, 2024
I discovered these issues while using this code in NullAway; see uber/NullAway#1055. (We support older Error Prone versions so can't directly call this API and had to adapt the code instead.) The previous code didn't handle explicit annotations on lambda parameters (e.g., `(@nullable Object x) -> { ... }`) and annotations on enum constants. Not sure the best way to add tests for this but happy to add them if given a suggestion. Actually the enum constant case was weird; I was unable to repro with bytecodes output by `javac` and I only observed the case with an `ijar` from Bazel. FYI @cpovirk @cushon Fixes #4620 FUTURE_COPYBARA_INTEGRATE_REVIEW=#4620 from msridhar:more-annotations-tweaks fb3690b PiperOrigin-RevId: 686729409
copybara-service bot
pushed a commit
to google/error-prone
that referenced
this pull request
Oct 17, 2024
I discovered these issues while using this code in NullAway; see uber/NullAway#1055. (We support older Error Prone versions so can't directly call this API and had to adapt the code instead.) The previous code didn't handle explicit annotations on lambda parameters (e.g., `(@nullable Object x) -> { ... }`) and annotations on enum constants. Not sure the best way to add tests for this but happy to add them if given a suggestion. Actually the enum constant case was weird; I was unable to repro with bytecodes output by `javac` and I only observed the case with an `ijar` from Bazel. FYI @cpovirk @cushon Fixes #4620 COPYBARA_INTEGRATE_REVIEW=#4620 from msridhar:more-annotations-tweaks fb3690b PiperOrigin-RevId: 686916301
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For the case of declaration annotations, we were using
Flags.VARARGSto check if aSymbolrepresented a varargs parameter, but this only works if the method is defined in source code. We add new methods for varargs parameters that may be defined in bytecodes, and always check for declaration annotations in such cases.For type use annotations on parameters, for a method from bytecodes the annotation info is stored on the method's symbol, not the parameter's. We borrow some Error Prone code to handle this case.