Record Patterns Implementation Proposal

[johannescoetzee]

Record Patterns Implementation Proposal

[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.

[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.

[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.