Record Patterns Implementation Proposal

[johannescoetzee]

This is related to #4361, but is a more specific proposal related only to RecordPatterns.

As mentioned in #4217, I would like to add support for Record Patterns introduced in https://openjdk.org/jeps/440 to javaparser.

Syntax Changes (for reference)

Old Syntax

Pattern:
    TypePattern

TypePattern:
    LocalVariableDeclaration

New Syntax

Pattern:
    TypePattern
    RecordPattern

TypePattern:
    LocalVariableDeclaration

RecordPattern:
    ReferenceType ( [PatternList] )

PatternList:
    Pattern {, Pattern }

Implementation Proposal

In JP, PatternExpr is currently used to represent a TypePattern. I propose we split this up into 2 classes: TypePatternExpr representing a TypePattern and RecordPatternExpr representing a RecordPattern. These 2 XPatternExpr classes will share a common Pattern parent class, so that we end up with something like the following:

abstract class Pattern extends Expression {
    // TODO: Do the different classes actually share anything worth sharing in the base class?
}

class TypePatternExpr extends Pattern {
    private NodeList<Modifier> modifiers;

    private SimpleName name;

    private ReferenceType type;
}

class RecordPatternExpr extends Pattern {
    private NodeList<Modifier> modifiers;

    private ReferenceType type;

    private NodeList<Pattern> patterns;
}

Base Class and PatternExpr Considerations

There's a choice to be made about what to use what to do with the existing PatternExpr.

Option 1: Use PatternExpr as the new base class

In the examples above, that would mean replacing every instance of Pattern with PatternExpr.
This approach comes with 2 notable advantages:

1. It's the "cleanest" representation of patterns (in my opinion, at least) and is similar to what JP is already doing with AnnotationExpr.
2. We don't need to change parameters and fields where PatternExpr is expected.

The downside to this option is that we would be changing the behaviour of PatternExpr without changing the name. This
could be confusing for users doing JP upgrades who might not understand why PatternExpr suddenly doensn't work the
same anymore.

Option 2: Introduce a new class to use as the base class and have PatternExpr extend that as well

In the examples above, this new base class would be Pattern and we'd change PatternExpr to be:

class PatternExpr extends Pattern {
...
}

With this case, we have 2 further options:
(2.1) Don't introduce a new TypePatternExpr and just keep using PatternExpr for that purpose as it is currently.
(2.2) Introduce a new TypePatternExpr to replace PatternExpr and deprecate PatternExpr

The benefit of option 2.1 is that it could be easier for users to upgrade to the new version, while the benefit of
2.2 is that the naming matches the JLS naming, so it may be easier for future users to understand.

Symbol Solving

A change similar to that for switch pattern matching in #4375 is
necessary. For example, resolving bValue in a case like

case RecordA ( RecordB ( String bValue ), RecordC cRecord ) -> ...

we can traverse the pattern tree RecordA -> RecordB -> String bValue to find the bValue declaration. This is
a separate topic from actually resolving record types (for example resolving cRecord) which is outside of the scope of this task.

My thoughts on this

In my opinion, Option 1 is the best option. The confusion regarding the change of the meaning/purpose of PatternExpr
is a real downside, but I think the impact of that could be mitigated with an upgrade guide telling users how to
fix the use of PatternExpr in their code and explaining the rationale behind it. Some change to user code during
upgrade is inevitable and I believe the benefit of having a clearer naming convention that more closely matches the
JLS outweighs a slightly more confusing upgrade process in the long term. There may well be other factors I did not
consider, however, so I'd be happy to hear any thoughts or suggestions on this.

[johannescoetzee]

@jlerbsc If this seems a good enough representation to you in general, then I'm particularly interested in hearing your thoughts on the matter of what to do with PatternExpr, what to call the pattern base class and whether it makes sense to repurpose PatternExpr to fill the base class role.

Any suggestions or alternative ideas are of course welcome too :)

[jlerbsc]

I agree with "option 1: Use PatternExpr as the new base class" with an upgrade guide telling users how to
fix the use of PatternExpr in their code and explaining the rationale behind it. But you have to go for progressive and homogeneous commits.

[johannescoetzee]

I'm glad to hear we agree! I'm not sure what you mean with progressive and homogeneous commits, however. Could you please clarify that just to avoid a situation where I might have to do some more complex rebasing/commit splitting or anything like that?

[jlerbsc]

I'm glad to hear we agree! I'm not sure what you mean with progressive and homogeneous commits, however. Could you please clarify that just to avoid a situation where I might have to do some more complex rebasing/commit splitting or anything like that?

I simply don't want to have commits that modify other commits. I want to be able to follow the progression of ideas. For example, separating what comes under LPP and the parser or symbol solver. Separate what concerns JavaCC from what concerns the core of JP. Not to make changes that are not relevant to the subject in hand...

[jlerbsc]

I agree with "option 1: Use PatternExpr as the new base class" with an upgrade guide telling users how to fix the use of PatternExpr in their code and explaining the rationale behind it. But you have to go for progressive and homogeneous commits.

I need to think about these ideas for a while. I'm divided between limiting the impact of this development and designing it in line with the state of the art.

[jlerbsc]

@jlerbsc If this seems a good enough representation to you in general, then I'm particularly interested in hearing your thoughts on the matter of what to do with PatternExpr, what to call the pattern base class and whether it makes sense to repurpose PatternExpr to fill the base class role.

Any suggestions or alternative ideas are of course welcome too :)

Indeed, calling the PatternExpr base class is a good idea. The next version to include these changes will be 3.26.0 and it will indicate the break in the PatternExpr API. The mistake would probably be to try to limit the impact of the change by keeping what already exists. In general, I'm more in favour of limiting the impact of changes, but in this case it seems to me that we need to be closer to the JLS to make the code easier to understand.

[johannescoetzee]

That sounds good. I'll proceed with option 1 then :)

[johannescoetzee]

I'm working on the PatternExpr refactor now and the diff is getting fairly large with a decent amount of work still to be done, in all likelihood. I think I'm going to split this issue up into 2 separate PRs: one for the refactor and one for the new record pattern support.

For the refactor PR, many of the commits won't be very clean. Since the code needs to compile to run the code generators and since I want to run unit tests very frequently while doing the refactor to avoid accidental regressions, this has essentially turned into a process of making small, incremental changes, running the code generators, running tests and repeating. This means a lot of the commits touch a lot of files to keep things compiling, but I hope the commit messages capture enough of the motivation behind each change to make it possible to follow my train of thought. I did consider splitting each of my "actions" into self-contained commits, but that would result in a lot of commits where any single commit doesn't really explain what is happening.

[jlerbsc]

I think I'm going to split this issue up into 2 separate PRs: one for the refactor and one for the new record pattern support.

OK. That seems a good idea to me.

[jlerbsc]

For the refactor PR, many of the commits won't be very clean. Since the code needs to compile to run the code generators and since I want to run unit tests very frequently while doing the refactor to avoid accidental regressions

I don't understand why this iterative work has an impact on the number of commits. Can you confirm that you only commit when your work is stable and tested locally on your machine.

In my mind, for the refactoring, which would consist of renaming the PatternExpr class and then adding the base class, you only need 2 commits.

[johannescoetzee]

I can definitely squash my commits into those 2, but I've tried to separate commits with generated code from code I wrote to keep that clearer. That means not every commit is stable code (or even code that compiles), but I do make sure that everything works after each commit "group" (so adding changes by hand, codegen, then any required fixes by hand).

The refactor isn't quite as simple as a renaming, however. Where an old-style PatternExpr was previously expected in the code, we still expect a new PatternExpr (since a record pattern will also work there in the future), but this requires some logic changes as well.

I'm on vacation for the rest of the week, but I'll open a WIP PR in the meantime if you'd like to have a look at what I've done so far.

[jlerbsc]

since a record pattern will also work there in the future

Perhaps it would be easier to make these adaptations at a later date. Unless you've already anticipated the future.