feat(record): add Java 16 record support

[MartinWitt]

fixes #4132

Records

This PR adds Records and Record Components to the spoon metamodel.
Records are a mix between simple tuples types and the black magic of compiler writers in 2021.
In its simplest form, a record is a tuple with fields (record components), accessor methods and equals/hashcode implementation.

record Point(int x, int y){};

Can be seen as

public final class Point {
  private final int x;
  private final int y;

  Point(int x, int y) {
    this.x = x;
    this.y = y;
}
//... getter, equals hashcode
}

TLDR Records

Important record features:

• Records are comparable to enums and classes
• No explicit instance fields
• explicit accessors are allowed
• missing accessors and fields are implicit added
• toString/hashCode/equals are magically created at runtime
• new compact constructor as constructor type

Interesting(broken) spec features

After showing how simple records could be, let's show how are some details of the spec implemented

In the following, I try to provide the spec sentence and how it is implemented:

Abstract Modifier

It is a compile-time error if a record declaration has the modifier abstract.

Simple Check in set/add modifiers, throwing a SpoonException if the rule is violated.

Implicit final

A record class is implicitly final. It is permitted for the declaration of a record class to redundantly specify the final modifier

Implemented in DefaultCoreFactory#createRecord()

Implicit static

A nested record class is implicitly static.

Implemented in CtRecord#setParent

Superclass

The direct superclass type of a record class is Record (§8.1.4).

Implemented in CtRecord#setParent

Record Components

Implicit field/accessor

A record component corresponds to two members of the record class: a private field declared implicitly, and a public accessor method declared explicitly or implicitly (§8.10.3)

Implemented in CtRecord#addRecordComponent. RecordComponents have a toField and toMethod, converting them to the corresponding accessor and field. This unfolding/folding is done for every remove and add call in CtRecord.

Clashing methods

It is a compile-time error for a record declaration to declare a record component with the name clone, finalize, getClass, hashCode, notify, notifyAll,
toString, or wait.

Implemented in CtRecordComponent#setSimpleName

TypeMembers

It is a compile-time error for the body of a record declaration to contain a non static field declaration (§8.3.1.1).

It is a compile-time error for the body of a record declaration to contain a method declaration that is abstract or native (§8.4.3.1, §8.4.3.4).

It is a compile-time error for the body of a record declaration to contain an instance initializer (§8.6).

Implemented in CtRecord#addTypeMemberAt

RecordMembers

Field Modifiers

A component field is private, final, and non-static.

Implemented in CtRecordComponent#toField

Annotations for fields and methods

A component field is annotated with the annotations, if any, that appear on the
Corresponding record component and whose annotation interfaces are applicable
in the field declaration context, or in type contexts, or both (§9.7.4).

Here we had to be creative. If the annotation is known for spoon and has the matching element type, it is added. If either the annotation is not known or the element type is not matching, the annotation is omitted. The first part means that this is only a conservative approximation, the result could be wrong but the result AST is correct java code.

Record Constructors

Records can have three different constructors, either an implicit, the "normal" or a compact constructor.
The first two are well known and have to initialize all record component fields. The third is new.
Compact Constructor:

Rational { 
 int gcd = gcd(num, denom); 
 num /= gcd; 
 denom /= gcd; 
 } 

Normal Constructor

Rational(int num, int demon) {
 int gcd = gcd(num, denom);
 num /= gcd;
 denom /= gcd;
 this.num = num;
 this.denom = denom;
}

Compact constructors have 2 important differences from normal constructors:

1. No Executable Parameters(small but ugly change in printer) and no ( )brackets
2. The fields get implicitly assigned after the written code.(solved in jdt)

Open Problems

• In the current version, we don't generate implicit toString/equals/hashcode methods, because the methods are generated at runtime by the jvm. This could be added, but the value of an implicit method that could be close or totally off to the spec behaviour seems low.
• The ReplaceParametrizedTest.testContract test tries to add an invalid element to records. As in only a few places consistency checks are implemented, this was never an issue. A solution could be creating a new exception for JLS invalid operations subtyping SpoonException and handling the new exception differently in the test case.
• Currently, the ElementPrinterHelper#writeElementList only skips implicit elements that are constructors.

if (element instanceof CtConstructor && element.isImplicit()) {
  continue;
}

This leads to the caller removing implicit fields instead of the print method itself. As the printer is kinda complex I'm unsure if the methods could be changed to ignore implicit elements, but this would improve the code in printer

• CtType uses the private field typeMembers directly and does not call the getter. Using the getter strictly could improve the code in CtRecord. But I wanted to keep the already big PR as small as possible, so this was not changed.
• Sorald and deadstores are simply a bug, or?

[raghav-deepsource]

I'm not entirely sure of this, but will this not fail if a predefined accessor exists with a different body than the one generated with toMethod?

[raghav-deepsource]

If this condition returns true when another accessor for the same field exists, there may be problems.

[MartinWitt]

good catch should be fixed now

[slarse]

somehow i failed to add @I-Al-Istannen and @SirYwell as coauthors, this shouldn't get lost.

To make sure we don't forget about this, I suggest you add an empty commit with this commit message:

Add coauthor attribution

Co-authored-by: I-Al-Istannen <[email protected]>
Co-authored-by: Hannes Greule <[email protected]>

GitHub should then automatically add them as coauthors when we merge the PR.

[MartinWitt]

I have added the missing text to the initial post. The commit history is a bit convoluted, but as there was already feedback, we should avoid cleaning it for now.

[slarse]

Hi @MartinWitt, thanks for putting in all this work! I haven't had the time to review the source code in any particular detail, but I can answer your open questions so you can finish the PR.

In the current version, we don't generate implicit toString/equals/hashcode methods, because the methods are generated at runtime by the jvm

I agree, let's not generate those members unless we find a very compelling use case for doing so. I can think of none.

The ReplaceParametrizedTest.testContract test tries to add an invalid element to records. As in only a few places consistency checks are implemented, this was never an issue. A solution could be creating a new exception for JLS invalid operations subtyping SpoonException and handling the new exception differently in the test case.

Sounds like a good idea to me. In general, I don't really like the fact that SpoonException is thrown everywhere, we should have more specific exceptions (that are subtypes of SpoonException) to allow for more specific error handling. Care to implement it for this PR to make tests go green? Shouldn't be much code, right?

Currently, the ElementPrinterHelper#writeElementList only skips implicit elements that are constructors.

tl;dr: Remove the check for if the element is a constructor and see what happens.

Implicit elements should not be printed. If we ignore the inconsistent use of the DefaultJavaPrettyPrinter#ignoreImplicit field, that is (let's ignore it for now, that's really only used for type names afaik). In the context of the printer helper, the fact that only constructors are skipped is either a dirty hack for something, or a remnant of something that is no longer relevant.

Sorald and deadstores are simply a bug, or?

Yeah, the version of SonarJava used is not reliable on dead stores. Overall, we've had way too many false positives with that version of SonarJava, so I'm removing it for the time being (#4146).

[MartinWitt]

Sounds like a good idea to me. In general, I don't really like the fact that SpoonException is thrown everywhere, we should have more specific exceptions (that are subtypes of SpoonException) to allow for more specific error handling. Care to implement it for this PR to make tests go green? Shouldn't be much code, right?

After some thought process, I believe a good solution is:
Create a new Exception:

public class JLSCorrectnessException extends SpoonException()

and add a new method to CtElement.

protected void throwJLSCorrectnesException(String reason) {
if(!this.getFactory().getEnvironment().ignoreJLSCorrectness()) {
  throw new JLSCorrectnessException(reason);
}
else {
.... logging? 
}

This flag allows writing code transformations, which produce JLS invalid code, in intermediate steps or at the end. But if a user wants JLS correct code, he can set the flag to true.
An open point is, setting the flag to true or false as default.
False as default and therefore ignoring all JLS correctness is close to the current behavior and non-breaking.
Setting it to true as default could create some kind of JLS correctness awareness. And most code transformation normally don't want to create incorrect code. I'm open for both default settings. wdyt?

[MartinWitt]

I removed the review tag because we(@I-Al-Istannen, @SirYwell and I) implemented shadow records and it's not 100% done

[MartinWitt]

I changed it back to review, but #4148 is still a stopper. In the latest update, the JavaReflectionTreeBuilder supports best effort records if the JDK is version 16 or above.

[lgtm-com]

This pull request introduces 1 alert when merging 77fcfb0 into d9bcb11 - view on LGTM.com

new alerts:

• 1 for Container contents are never accessed

[slarse]

ping @monperrus, don't forget about this substantial contribution

ping @MartinWitt, some trivial conflicts to resolve

[monperrus]

yes, it's on my todo list, once I'm done with fixing the bulk of Java 11 build / CI errors.

Plus, I really look forward to the next major release with Java 16 support.

[monperrus]

Hi @MartinWitt,

Made a pass over this important PR. It's really good.

• Left one question about a test change
• There is a conflict to fix
• FYI I made a couple of formatting changes on the fly

After, I'm happy to merge.

Thanks!

[monperrus]

why do we need this?

[MartinWitt]

If I remember correctly, this tests tries to add some not valid children to record components. With this flag, we ignore JLSViolation, which is thrown by CtRecord

[monperrus]

ok, it would be better to handle as a special case below (see list of special cases as of line 90). WDYT?

[MartinWitt]

Then the list of special cases would grow, if we add more JLS checks. A lot of tests in spoon suffer from complex lists of edge cases, we ignore. Maybe we should shrink these lists instead of adding more to improve test readability?

[monperrus]

It's a tradeoff. By ignoring violations for all, we may silence some failures.

[monperrus]

Let's move on. I'll proceed with merge.

[monperrus]

Many many thanks @MartinWitt