-
Notifications
You must be signed in to change notification settings - Fork 8
SPICE-0017: Java Records Code Generation #19
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
base: main
Are you sure you want to change the base?
Conversation
|
@bioball Please review and approve! Thanks much in advance! |
| .. for a Pkl `open` class: | ||
| ... generate its Java interface as in the `abstract` Pkl case, only always name it with `I` prepended | ||
| ... generate its Java record as in the non-abstract Pkl case | ||
| ... the generated Java record should, in addition, implement the above generated Java interface |
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 think that we should keep abstract and open classes how they're currently represented (as regular classes).
This is a closer way to model the relationship between Pkl and Java (abstract maps to abstract, open maps to non-final).
This is what we're doing in Kotlin, also. Classes that are neither open nor abstract turn into data classes, and otherwise become regular classes.
Creating a companion interface here would be a breaking change, and would also open us to naming conflicts. For example:
open class Person { // okay, let's generate interface `IPerson` because this is open.
name: String
}
class IPerson { // uh oh, conflict
name: String
}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.
Yes, this type of naming conflict unfortunately is unavoidable due to Java not allowing multiple public classes/interfaces per one Java file (Kotlin's no problem with this).
The easiest is to warn users about this - don't make your classes start with I<capital cased identifier>.
In case of IPerson renamed to Iperson, it's be fine.
As an alternative, we could've used I_ instead of I prefix.
And In general, we could allow any arbitrary user defined prefix via --prefix-interfaces-with option, with I as default. (This would require minor code change/refactoring).
Ultimately, this whole Pkl code generation is for users to employ as their object/data models, so it's best left to their customization.
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 think avoiding the class inheritance wholesale is the best from both usage and implementation viewpoints, and the interface customization feature offers just 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.
@bioball I truly think that pure Records + Interfaces is the best possible solution for this use case both from usage and implementation viewpoints. With customizable interface prefixes, as a good improvement on the original doc, we should go for it with full force, IMHO. And it's ready to go now. :)
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 think avoiding the class inheritance wholesale is the best from both usage and implementation viewpoints, and the interface customization feature offers just that.
IMO: if users want to avoid class inheritance, users should avoid that on the Pkl side; the Java side simply reflects what's described in Pkl.
| * @param pigeon module property comment. | ||
| * *emphasized* `code`. |
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.
For readability sake, it would be good for successive lines to start on the same indentation level
| * @param pigeon module property comment. | |
| * *emphasized* `code`. | |
| * @param pigeon module property comment. | |
| * *emphasized* `code`. |
| } | ||
| } | ||
| } | ||
| ---- |
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 is a neat pattern! I'm fine with this being an optional add-on.
I think, in the default case, let's generate records that have with methods.
record R (String p1, String p2, String p3) {
public R withP1(String p1) { /* impl */ }
public R withP2(String p2) { /* impl */ }
public R withP2(String p2) { /* impl */ }
}This means that this is source-code compatible for any users migrating to the new Pkl java code generator.
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.
Sure! May we, please, implement this after the July release, during one of minor deliveries before 0.30?
Also, it's good to get some bandwidth to a bit refactor all these features internally - not a big deal, but requires some time and effort.
Due to the large number of tests, it take much more effort to adjust them then the production code itself.
|
|
||
| Note that the above `it` is properly typed allowing for Java syntax verification and IDE code completion. | ||
|
|
||
| * enable Lombok Builder(-s) for records via `--use-lombok-builders` in CLI or `useLombokBuilders` in Pkl Gradle Plugin, `false` by default |
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'd like to exclude this from the SPICE; we can add it if there's a popular enough request for it.
The wither interface and the with methods already allows you to transform these objects.
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'm not a Lombok Builder lover on records, so am not pushing back on this.
But for users to decide on this, can we just mark it is @experimental and most likely to be removed in the future, so if nobody cares, to removed by the next release?
Please advice! No issues for me to remove this completely.
PS:
The code itself, though, somewhat corresponds to the recently introduced @Generated Annotation.
We could've introduced an option like -- generate-annotations with the list of FQN annotations.
This is easily implementable.
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.
The code itself, though, somewhat corresponds to the recently introduced @generated Annotation.
We could've introduced an option like -- generate-annotations with the list of FQN annotations.
This is easily implementable.
Good thought, although, many annotations need arguments (for example, the javax.annotation.processing.Generated annotation has mandatory String value field).
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’d be hesitant to add Lombok support unless/until there is a compelling use case that cannot possibly be satisfied by withers.
| with the becoming de-facto https://jspecify.dev[JSpecify] as more and more prominent Java projects along with Kotlin adopting it as of this writing. | ||
|
|
||
| With `JSpecify` we would be able to adopt its nonnull by default style with `@NullMarked`, removing much of the noise in the generated code, | ||
| assuming that most of the Pkl source properties are nonnull. This is a flip of the current implementation. |
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.
See apple/pkl#811 for an ongoing conversation about this
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.
Totally agree! This is a special separate concern to address properly and say goodbye to JSR-305.
| If the records generation proves successful, the current Java (non-record, plain Java classes) Code Generation might be deprecated and completely removed with minimal impact to the codebase. | ||
|
|
||
| In addition, the newly introduced optional features, `useWithers` and `useLombokBuilders`, along with their self-contained code, | ||
| can also be easily deprecated/replaced/removed upon the official Java withers arrival, whatever it might be. |
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.
Instead of deprecating the current Java code generation, we should just go ahead and change the current Java code generator to produce records for normal (non-abstract, non-open) classes. This should be a minimally breaking change for users (it breaks for users on older Java, but Pkl already doesn't support anything less than Java 17).
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 propose to keep the new solution and the old one separate during this release lifecycle and sort these things out in the interim, otherwise things would be messy in terms of maintenance and refactoring.
I've code well tested and ready to go, in addition to being easily refactorable being side-by-side with the old solution. Any serious change would only delay the delivery.
| while having a number of drawbacks like: | ||
|
|
||
| * requires `hashCode/equals` implementation for each artifact | ||
| * weak, insecure serialization |
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.
Can you elaborate on what this means?
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.
Keeping in mind that we've delivering the immutable Java object model of the source Pkl templates, nothing more and nothing less, along with the ability to data bind the concrete Pkl configs to it, any unnecessary inheritance pitfalls, implementation wise, better to be avoided.
hashCode/equals: seecanEqualsand friends + the whole art generating good hashes...- see below among plenty of similar articles and papers by Oracle Java Team
The Java Records Code Generation SPICE describes the proposed Java Records code generation along with the optional Withers and Lombok Builders features.
This has been implemented in apple/pkl#970 PR.