fix(java): correctly parse named attribute assignment in annotations (#8723) - #8724
Open
SamBarker wants to merge 5 commits into
Open
fix(java): correctly parse named attribute assignment in annotations (#8723)#8724SamBarker wants to merge 5 commits into
SamBarker wants to merge 5 commits into
Conversation
Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
Contributor
|
The compact constructor isn't required: Record-component annotation trees carry no end position on the argument, so Deciding :139 from the source after |
Record-component annotations get attributed to multiple copies of the same annotation tree (field, accessor, constructor parameter). Only the original copy is registered in javac's position table; the others report no end position at all, even when the attribute name was genuinely written in source. The parser used that end-position lookup to decide whether an attribute was explicitly named (@A(value = "x")) or compiler shorthand (@A("x")). Since the lookup fails for these copies regardless of which case it actually is, it treated a real explicit assignment as shorthand, converted only the value, and left its cursor stuck mid-attribute-name. Check the source text directly for the attribute name instead of relying on the position lookup. Fixes openrewrite#8723 Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
Same fix as 5901e22, applied to the Java 17 parser: check the source text directly for the attribute name instead of relying on an end-position lookup that's unset for record-component copies of the annotation tree. Fixes openrewrite#8723 Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
Same fix as 5901e22, applied to the Java 25 parser: check the source text directly for the attribute name instead of relying on an end-position lookup that's unset for record-component copies of the annotation tree. Fixes openrewrite#8723 Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
Author
|
Thanks @renechoi I've added fixes to the PR so the tests now pass locally 😀 |
This file contains hidden or 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
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.
What's changed?
attribute (e.g.
@A(value = "x")), for the Java 17, 21, and 25 parsers.where the annotated component is followed by additional record
components (to catch parser-cursor corruption, not just a crash).
What's your motivation?
Record-component annotations get attributed to multiple copies of the same
annotation tree (field, accessor, constructor parameter). Only the original
copy is registered in javac's end-position table; the other copies report
no end position at all, even when the attribute was genuinely named in
source. The parser was using that end-position lookup to decide whether an
attribute was explicitly named (
@A(value = "x")) or compiler shorthand(
@A("x")) — since the lookup fails for these copies either way, itmisread real explicit assignments as shorthand, converted only the value,
and left its cursor stuck mid-attribute-name, corrupting the parsed source
range for anything that followed.
The fix checks the source text directly for the attribute name instead of
relying on the unreliable end-position lookup. Applied identically to the
three JDK-version parser modules that support records (17, 21, 25) — Java
8 and 11 don't need it, since records didn't exist as a standard feature
until JDK 16.
This started as a minimal reproducer for an issue encountered running
OpenRewrite on
https://github.com/kroxylicious/kroxylicious/blob/main/kroxylicious-api/src/main/java/io/kroxylicious/proxy/config/tls/TlsCredentialSupplierConfig.java
Anything in particular you'd like reviewers to focus on?
Whether checking the source text for the attribute name (rather than
relying on
endPos()/the end-position table) is the right generalapproach here, or if there's a more idiomatic way to detect explicit vs.
compiler-synthesized annotation arguments in this codebase.
Anyone you would like to review specifically?
Have you considered any alternatives or workarounds?
Tried comparing
assign.lhs.getStartPosition()toassign.rhs.getStartPosition()to distinguish explicit vs. synthesized assigns without touching source
text — this broke on other shorthand cases (e.g.
@Retention(RetentionPolicy.RUNTIME),which is also represented as a
JCAssigninternally with non-equalpositions), so it wasn't reliable enough. The source-text check is simpler.
Any additional context
Checklist
./gradlew buildlocally, and committed any resulting changes torecipes.csv