fix(java): correctly parse named attribute assignment in annotations (#8723) - #8724
Conversation
Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
|
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>
|
Thanks @renechoi I've added fixes to the PR so the tests now pass locally 😀 |
javac's `Annotate#enterAnnotation` rewrites `@A(expr)` into `@A(value = expr)`
in the tree it hands us, so a single-element annotation always arrives as a
`JCAssign` whether or not the source wrote the name. The `endPosTable` used to
answer which it was, but it cannot for record components: javac copies their
annotations onto the generated field or constructor parameter, and the copies
carry no end positions. The name was dropped whenever the annotation could not
land on the field, so `@Target({RECORD_COMPONENT, PARAMETER})` reproduces it and
adding `FIELD` hides it.
Compare the attribute name's position with the value expression's instead.
`Annotate` builds the synthetic name with `make.at(rhs.pos)`, so the two
coincide only when the name was elided, and positions survive the copy. Probing
the source text at the cursor was the other candidate and gets `@A(value)`
wrong, where the argument is a constant that happens to be named after the
attribute.
Extend the record test with a component that spaces and comments its attribute,
and cover the constant-named-`value` case next to the other elided-argument
tests in `AnnotationTest`.
|
Polished, instead of a long discussion, as this should land rather sooner than later, I hope you are good with this. The class Holder {
static final String value = "a";
@A(value) // printed back as @A(value=value)
String name;
} The commit compares positions instead. The trigger isn't record components as such, it's whether the annotation can reach the generated field. I added and folded some test cases based on equivalence classes. Unrelated but nearby: |
`@OnMethod(value = "full_name")` in a record header only parses since #8724: `getOriginalAnnos()` hands back copies that are absent from the end position table, so `endPos(arg) < 0` holds for every annotation written there and a written attribute name was taken for an elided one.
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