Java class and record generators and friends side-by-side

[protobufel2]

1. Java code generation via Java Records and
2. Wither implementation close to JEP 468
3. extra "generateRecords" option in Gradle JavaCodeGenTask
4. extra "--generate-records" option in pkl-codegen-java CLI
5. updated docs

see pkl-codegen-java/README.md

[bioball]

Thanks for this PR! This is a much needed feature.

I haven't looked at this in detail yet; worth discussing the implementation first.
Also, it's a little hard to review this because of the duplicated JavaRecordCodeGenerator class. Can we consolidate this?

[bioball]

This document feels like it belongs as a description in this PR, rather than as a README checked into source code.

Also: I'm not sure how I feel about this; it feels like it's adding a lot of abstraction for not much benefit.

I think it would be fine to just generate simple records; it's not too hard derive creation of new records with pattern matching (for Java 21 users):

var newPerson = switch (record) {
  case Person(var name, var age) -> new Person("Bob", 15)
}

Another option is to generate with methods, which is also lower abstraction.

record Person(String name, int age) {
  public Person withName(String name) {
    return new Person(name, age);
  }

  public Person withAge(int age) {
    return new Person(name, age);
  }
}

And, this feature also offers little benefit to those that are using Java 23 or higher with --enable-preview (and also in the further future once Java 25 is out and adopted).

By the way, some other comments about this impl:

• The name Memento feels off; I usually see this called Builder
• There's not much benefit that I can see from Wither being an interface

[protobufel2]

@bioball: thx much for such a thorough review!

1. Regarding withers:
   • JEP 468 has never made it into Java 23 preview and it's TBD when
   • JEP 468 makes a case for me against the current state of "normal" withers - they are inefficient and verbose
   • until JEP 468 or its deconstruction based successor released, the proposed implementation is closest one can get in form, spirit, and efficiency - the pattern matching is not good and too cumbersome as a means of simple record withering, for the lack of a better word
   • the proposed implementation should be easily transformed when JEP 468 and friends are in place as it closely resembles it, e.g. can be done with an OpenRewrite rule, yet can easily coexist with anything new delivered in Java vis-a-vis records - we can deprecate then without any disruption to the users
2. Memento is because it's a Memento Pattern and also doesn't look like builder - there is no validation, flow, or any kind of builder related helpers here, just raw object of the same shape as its record which is its mere copy, solely exposed for the sake of amendments (hi, Pkl :) ) within the wither method
3. Wither being an interface provides a uniform feature of all such records, while also providing for future additions across all such records via Wither's extra default methods, if needed.
4. Regarding the attached doc, yes!, it was meant to get the reviewer's attention and also to keep the design close to the code. What is the proper place to have this or a similar doc for the records, pun intended?
   Unlike Kotlin data classes, Java records can't extend any class and can only implement interfaces, so Pkl class inheritance is modeled like the following:
   • Pkl abstract class -> Java interface; if isModule -> same name, else prepend I
   • Pkl closed class -> Java record
   • Pkl open class -> Java record + interface named with prepended I; if isModule -> 2 separate files
   • in generic annotations, boundaries are Java interface based ones, i.e. I prepended
   • nullablity is implemented identically to the current Java class generation; will be revamped when switched to JSpecify by annotating @nullable cases instead of @nonnull ones, plus annotating records and their interfaces with @NullMarked (or the entire package at once instead, TBD)

[bioball]

Usually, design docs belong in a SPICE.

Do you want to write one? That might be a better forum for discussing the intricacies of this change.

Didn't know that JEP-468 didn't make it to Java 23! That's confusing; the JEP makes it seem like it's available under the --enable-preview flag. My mistake!

Memento is because it's a Memento Pattern and also doesn't look like builder - there is no validation, flow, or any kind of builder related helpers here, just raw object of the same shape as its record which is its mere copy, solely exposed for the sake of amendments (hi, Pkl :) ) within the wither method

I've heard of memento, but I haven't that name much in the wild. We use "builder" for this in our own codebase, e.g.

pkl/pkl-core/src/main/java/org/pkl/core/runtime/VmSet.java

Line 351 in 9f22c56

private static final class Builder implements VmCollection.Builder<VmSet> {

JEP 468 makes a case for me against the current state of "normal" withers - they are inefficient and verbose

It makes the case that they introduce boilerplate, but your version here does too.

Any performance difference here seems negligible to me; here's a quick perf test (source here: https://github.com/bioball/pkl/blob/withers-bench/bench/src/jmh/java/org/pkl/core/Withers.java. ./gradlew bench:jmh to run)

Benchmark                  Mode  Cnt           Score           Error  Units
Withers.classicalWithers  thrpt    5  3974943373.183 ± 123215391.975  ops/s
Withers.withers           thrpt    5  3874586942.045 ±  36404946.224  ops/s

Do you have some prior art of other Java libraries that use this type of "withers" API?

Pkl open class -> Java record + interface named with prepended I; if isModule -> 2 separate files

Open classes should be probably be generated as regular classes (they can be instantiated, and can also be inherited from)

[protobufel2]

1. Would love to write a SPICE draft, and the rest, bandwidth permitting
2. JEP not making into Java despite clearly stating it'll be available as a preview is almost unheard of, you're not alone to be confused! :(
3. I still think the Memento is the right name for this memento. Will change if you insist, NP :)
4. Thinking about a large Pkl template generated into a large Java object model used by at least a mid sized code base, imaging how many places would there potentially be using withers like with<prop name>(...), and how difficult would be to change that, especially if some of the generated records would become a part of the code's public API... The proposed wither would be much easier and gentler choice for the API evolution, both for Pkl Java generation and for the user's codebase.
5. I don't know much about the withers' API usage - the JEP brakes the current builder tradition in this respect, not unlike Java has done with many other patterns, e.g. Visitor, so the prior art here might be not a good guide.
6. Open classes are generated as a record with its companion interface, so the "subclass" becomes a record implementing its (open) superclass's interface while retaining the superclass' properties as record components exactly as it's implemented today with the generated Java classes. Given we're going the Java Records all-in, it's a logical and sensible approach - as Java data classes representing the generated Java immutable object data model, to have only records and the related interfaces, wherein "inheritance" is only via shallow interface implementation (the approach forced upon us by Java, not that it's not welcome).

[bioball]

Open classes are generated as a record with its companion interface, so the "subclass" becomes a record implementing its (open) superclass's interface while retaining the superclass' properties as record components exactly as it's implemented today with the generated Java classes. Given we're going the Java Records all-in, it's a logical and sensible approach - as Java data classes representing the generated Java immutable object data model, to have only records and the related interfaces, wherein "inheritance" is only via shallow interface implementation (the approach forced upon us by Java, not that it's not welcome).

That could work; would mean that any usages of the open class as a type would need to be the interface, rather than the record.

[protobufel2]

Yes!

[bioball]

I think we should just flip this around and have a flag called generatePojos which defaults to false.

By default, the code generator should be generating records whenever it can (all Pkl users are on Java 17).
This flag would just be for folks that are migrating and don't want to suffer a breaking change.

[protobufel2]

Regarding flipping generateRecords to the default true, it'd be very distractive, incompatible change by breaking all regenerated Java object models' usage, necessitating the corresponding boolean flip in Gradle extension and any such Java generation via using pkl-core or pkl-config-java by the affected users

[bioball]

I think that's okay; for users that want minimal breakage, they can add generatePojos = true to retain the current code generator output. For most users, this should be a matter of adding a line to their build.gradle.

[protobufel2]

Can we follow the industry practice of deprecation, keep things as compatible as possible for one, two releases before make it the breaking change? I guess, until Pkl goes 1.0.0 we could've followed Kotlin's model of making such breaking changes after a couple of minor releases, especially when it doesn't change the Pkl itself?

[bioball]

The downside of that is: it is harder for new users that are adopting Pkl (the code generator has less sensible defaults).

We usually try to minimize breaking changes, but this one feels quite tolerable to me. I'm also okay with already making the generatePojos option already deprecated, and eventually removed. Once we ship 1.x, we will be stricter about breaking changes.

[bioball]

There's quite a lot of duplication between this and JavaCodeGenerator. Can you refactor this?

Either combine this logic into one class, or introduce an abstract parent class that shares common methods.

[protobufel2]

1. Yes, the duplication is there intentionally - having the current Java class generation along with the records one in the same file would've added much more spaghetti, thus adding to the maintenance cost and comprehension issues.
2. Yes, we can try refactoring it as you've suggested, but the inner functions inside functions make it more than one might want to chew - JavaCodeGenerator, even before JavaRecordCodeGenerator, was due for some refactoring, one might argue.
   • Would it be better just to leave it as-is for now, release this out as a minor version as soon as possible to gather the feedback, and then deprecate the Java class generation altogether, keeping JavaCodeGenerator file as-is until its eventual retirement while evolving JavaRecordCodeGenerator as needed, including all the necessary refactoring?

[stackoverflow]

Thanks for the PR! Generating records is something we were already discussing since we switched to Java 17.
Just left some comments for the high level archtecture.

[stackoverflow]

As Dan already mentioned, we want to deprecate, and eventually remove the old pojo generation and just go with records, so this should be inverted to --generate-pojos for people who are still migrating.

[protobufel2]

Sure! Will do! Was trying to be as backwards compatible as possible :)

[stackoverflow]

I'm unsure of the cost/benefit of generating the Wither/Memento structures. It's quite verbose, which is one of the things we'd like to address with the current POJO approach.
I'd rather we start with just generating naked records.
This functionality could be added later or even become a separate option for people who want them.
It'll also simplify the codegen.

[protobufel2]

@stackoverflow thank you for your time and review!
A couple of points and pain spots to highlight the approach of this particular PR and records code generation:

1. Initially, I tried to combine the current and the proposed Java codegen, however it'll be a mess, so intentionally opted out to have 2 separate Kotlin files, so the older Java codegen can later be just discarded - exactly as you described - full agreement here :)
2. Unlike Kotlin which has data classes with normal inheritance, Java records present a challenge as the inheritance here is only via interfaces, so had to dance carefully how records "self-interfaces" appear in generics, where they placed in the file, whether they start with I, or for abstract modules named as-is, and so on. Happy to say, all is resolved with no temp crutches.
3. Unfortunately, Java records are kinda naked, yet, as of Java 24, in terms any wither or builder support, and until this arrives we need something in its place, hence the implemented Wither interface and friends.
   • the goal of Wither is to be a solid, and easily refactored by users if needed, solution until the arrival of JEP 468 or similar
   • the implemented Wither is extremely close to JEP 468
   • having just one with(r -> {r.p1 = value1; r.p2 = r.p2 + 1, ...}) method, syntactically close to JEP 468 makes the later refactoring, by users, into the Java finalized whatever it might be wither relatively easy and in minimal places
   • the Wither is very efficient, will work with any future Java Record enhancements, so might be used forever even upon the official Java solution, side by side or being gradually replaced at will
4. Being a pure implementation detail besides adhering to the generated target spec, the Java Records Codegen code can be safely refactored into pure records generation + extras later at the time of our convenience
5. Given 100s of tests involved and the additional time/bandwidth required to make any significant refactoring of this PR, I propose to fast track it to release, marked as experimental, and then quickly iterate on that.
6. Without any wither, the records generation would be effectively useless; the current with{PropName}(value)
   solution involves numerous different with methods presenting huge challenge for later refactoring by users, as soon as they start using them in their codebase extensively; the proposed implementation doesn't have these drawbacks

BTW,

1. my project has a very big Pkl template and the Java object model generated with this Java Records Codegen, so it's very well tested in the real life - no issues so far at all!
2. the whole point of generating code is to use it as an object model; and as such it becomes the central model API the project of any size would be relying on, meaning possibly hundreds, even thousands of places where the API used wherein withers might be employed. So, the stability of Pkl code generation own target API is crucial here!