Fix Java parser for annotated varargs by adding marker on J.ArrayType, as opposed to field on J.VariableDeclarations#6098
Fix Java parser for annotated varargs by adding marker on J.ArrayType, as opposed to field on J.VariableDeclarations#6098knutwannheden wants to merge 17 commits intomainfrom
J.ArrayType, as opposed to field on J.VariableDeclarations#6098Conversation
Using a new `Varargs` marker.
|
Let me know if you'd like help replicating this pattern to the Java 8, 11, 17 and 25 parsers. Would be good to see this resolved. |
timtebeek
left a comment
There was a problem hiding this comment.
Approved from my side; anything that had made you held off on merging?
I'd seen this fail in a number of projects, including when serializing the LST, so figured good to clear out.
| """ | ||
| class Test { | ||
| void method(String first, int... values) {} | ||
| void method(String first, int ... values) {} |
There was a problem hiding this comment.
Above we had two spaces; after normalization we are now left with one instead of zero. I think that's fine?
There was a problem hiding this comment.
Yep, intentional — SpacesVisitor doesn't have a rule that strips the space, just normalizes it. Going from two spaces to one is the expected behavior.
| @Deprecated | ||
| @ToBeRemoved(after = "2026-04-01") | ||
| Space varargs; |
There was a problem hiding this comment.
It's still used once with a non-null value in the Groovy parser, but figured we can revise that if/when we decide to drop the field. Most other cases it's null already, and then there will be the long tail of deserialization support.
There was a problem hiding this comment.
Updated based on the cross-language audit (see main thread comment): keeping @Deprecated (right signal for Java recipe authors — use the marker), but removed @ToBeRemoved since JS/TS, Python, and Go are all actively using the field for their own native varargs syntax. Removal needs the cross-language migration tracked above.
J.ArrayType, as opposed to field on J.VariableDeclarations
|
Tagging Jonathan for review since we're adding a marker to the LST model, and deprecating a field (not yet removed). |
| * Indicates that a {@link org.openrewrite.java.tree.J.ArrayType} represents a varargs parameter type. | ||
| * When present, the array type should be printed using {@code ...} syntax instead of {@code []}. | ||
| * <p> | ||
| * For example, {@code String...} instead of {@code String[]}. | ||
| */ | ||
| @Value | ||
| @With | ||
| public class Varargs implements Marker { | ||
| UUID id; |
There was a problem hiding this comment.
This marker could use a review before we merge, since we're adding a marker for what used to be a field.
There was a problem hiding this comment.
Sam reviewed on Apr 1 — marking resolved.
sambsnyd
left a comment
There was a problem hiding this comment.
Basically all languages have some concept of varargs. Can we standardize this across all of them? Assuming it's agreed this is the way to go.
Just to help with the discussion, I think below is a fair description of current state of affairs. TLDR: it's fairly consistent.
Comment:
|
JS/TS, Python, and Go all use this field for their own native varargs/star-args syntax (rest parameters, `*args`/`**kwargs`, variadic types). Removing the field requires coordinated migration across all five frontends, so a fixed removal date isn't realistic. The `@Deprecated` annotation remains.
|
Thanks @greg-at-moderne for the survey — couple of corrections after digging into each. Scala is implemented, but not using the The other languages share a more interesting pattern: JS/TS, Python, and Go all actively read/write this same field for their own native varargs syntax, not just relay it over RPC:
So this isn't a Java field that other languages happen to relay — it's a shared whitespace bucket every variable-declaration-having frontend is squatting on. Only C# is pure relay (it uses a That reshapes the plan. I think the right path is: land this PR as Java-only now, then track the cross-language work separately:
Once 1–5 land we can drop the field; until then it stays. I've also dropped the |
Using a new
Varargsmarker.TODO:
varargsfield