-
Notifications
You must be signed in to change notification settings - Fork 365
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support JEP-440: Pattern matching for Records #4925
Conversation
private @Nullable J getNodePattern(@Nullable PatternTree pattern, JavaType type) { | ||
if (pattern instanceof JCBindingPattern b) { | ||
return new J.Identifier(randomId(), sourceBefore(b.getVariable().getName().toString()), Markers.EMPTY, emptyList(), b.getVariable().getName().toString(), | ||
type, typeMapping.variableType(b.var.sym)); | ||
} else if (pattern instanceof DeconstructionPatternTree r) { | ||
return visitDeconstructionPattern(r, whitespace()); | ||
} else { | ||
if (pattern == null) { | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to keep the J.Unknown
code path below? I'd kind of expected that to be dropped, or are there still paths that hit that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's currently 3 types of patterns, binding which is mapped to variable, deconstruction which currently only implements records and any
which is a preview feature. So in theory we no longer need it for Java 21, but it could help us with Java 24/25 print idempotency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to check a few more things, but here is a first set of comments.
rewrite-java/src/main/java/org/openrewrite/java/JavaVisitor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a few small comments. After merging this it would further be cool if we could also look into updating the formatter to support adding spaces before or after parentheses as configured in the style.
@@ -61,6 +61,7 @@ public enum Location { | |||
IF_ELSE(Space.Location.IF_ELSE_SUFFIX), | |||
IF_THEN(Space.Location.IF_THEN_SUFFIX), | |||
IMPLEMENTS(Space.Location.IMPLEMENTS_SUFFIX), | |||
PATTERN(Space.Location.PATTERN), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should reference a different Space.Location
. These locations are frequently used by the formatting visitors, so it makes sense to bind unique locations to the various padding elements. A _SUFFIX
suffix is the pattern here. But we should actually consider renaming the PATTERN
locations to DECONSTRUCTION_PATTERN
as that would match how we typically name these things.
What's changed?
Add full blown support for record pattern matching
Anyone you would like to review specifically?
Any additional context
https://openjdk.org/jeps/440
Checklist