Hi Pieter,
In principle, the goal of CasualDiff is simple: having an original AST,
original source text and an updated AST, and produce (textual) diffs that
transform the original source text into a source text that (when parsed)
would result in the updated AST.
And the other code then can only take the original AST, and do re-writes to
it such that the new re-written/updated AST is what they want, and
CasualDiff should make sure the textual representation is (or can be)
updated.
In reality, this may get very tricky, esp. for AST shapes that don't very
well match what's in the source code (like enums, multi-variables, and now
records). But, ideally, as long as the updated AST corresponds to an AST
that would be produced for some source code, CasualDiff should be able to
handle it.
Also, sometimes, there need to be special methods or handling in TreeMaker
to create special AST nodes - like if we wanted clients to create compact
constructors from scratch, we probably need a new method there. But, that
may not be necessary for the case where the compact constructor is just an
existing AST node.
Let's see if I can look into this.
Jan
On Sun, Apr 13, 2025 at 11:29 AM Pieter van den Hombergh <
pieter.van.den.hombe...@gmail.com> wrote:
> Dear developers,
> @Jan.Lahoda I have noticed that you are the last person working on
> CasualDiff in relation with records.
>
> I am trying to resolve issue7044 but am dumbfounded by the interaction
> between the refactoring transformers and
> classes in java.source.base, in particular CasualDiff and friends.
>
> The code breaks on two, maybe three issues:
>
> 1. Varargs are not properly transferred to the new (new outer) record.
> 2. `implements` clauses are put in the wrong place, after the first
> brace '{' of the record.
> 3. How to detect that a constructor is a compact constructor and omit
> the parameter list.
>
> I have added some tests (and a test helper class)
> The failing test show the following: (the '|' shows where expected and
> actual differ)
> test test8Varargs PASSED with compare mode = IGNORE_WHITESPACE_DIFF
> t/A.java expected
> + actual
>
> t/A.java [ 0] package t;
> package t;
>
> t/A.java [ 1] record A(F f, String... params) {
> record A(F f, String... params) {
>
> t/A.java [ 2] public A {
> public A {
>
> t/A.java [ 3] assert f != null;
> assert f != null;
>
> t/A.java [ 4] }
> }
>
> t/A.java [ 5] }
> }
>
>
> test test8Varargs FAILED with compare mode = IGNORE_WHITESPACE_DIFF
> t/F.java expected
> + actual
>
> t/F.java [ 0] /*
> /*
>
> t/F.java [ 1] * Refactoring License
> * Refactoring License
>
> t/F.java [ 2] */
> */
>
> t/F.java [ 3] package t;
> package t;
>
> t/F.java [ 4] /**
> /**
>
> t/F.java [ 5] *
> *
>
> t/F.java [ 6] * @author junit
> * @author junit
>
> t/F.java [ 7] */
> */
>
> t/F.java [ 8] record F<P>(P first, String... second) {
> | record F<P>(P first, String[] second) {
>
> t/F.java [ 9] public F {
> | public F(P first, String... second) {
>
> t/F.java [ 10] assert null != first;
> assert null != first;
>
> t/F.java [ 11] assert null != second && second.length > 0;
> assert null != second && second.length > 0;
>
> t/F.java [ 12] }
> }
>
> t/F.java [ 13] }
> }
>
>
> Some issues might be resolved by detecting the actual record parameters.
> A suitable constructor is always in the AST, either generated by the
> compiler with name '<init>' of by the source code. This constructor shows
> the varargs as intended, and is
> in my opinion the correct candidate to pick up the varargs. I have given it
> a try with a class called RecordUtils, but the resulting parameter list
> somehow interferes with what CasualDiff expects
> Anyway, it messes things up.
>
> The implements clause causes another issue:
> test test8RecordImplements1 PASSED with compare mode =
> IGNORE_WHITESPACE_DIFF
> t/A.java expected
> + actual
>
> t/A.java [ 0] package t;
> package t;
>
> t/A.java [ 1] import java.time.LocalDate;
> import java.time.LocalDate;
>
> t/A.java [ 2] import java.io.Serializable;
> import java.io.Serializable;
>
> t/A.java [ 3] public class A implements Cloneable, Serializable {
> public class A implements Cloneable, Serializable {
>
> t/A.java [ 4] static F f;
> static F f;
>
> t/A.java [ 5] }
> }
>
>
> test test8RecordImplements1 FAILED with compare mode =
> IGNORE_WHITESPACE_DIFF
> t/F.java expected
> + actual
>
> t/F.java [ 0] /*
> /*
>
> t/F.java [ 1] * Refactoring License
> * Refactoring License
>
> t/F.java [ 2] */
> */
>
> t/F.java [ 3] package t;
> package t;
>
> t/F.java [ 4] import java.io.Serializable;
> import java.io.Serializable;
>
> t/F.java [ 5] /**
> /**
>
> t/F.java [ 6] *
> *
>
> t/F.java [ 7] * @author hom
> * @author hom
>
> t/F.java [ 8] */
> */
>
> t/F.java [ 9] public record F(int x, int y) implements Cloneable,
> Serializable { | public record F(int x, int y) { implements
> Cloneable, Serializable
> t/F.java [ 10] /** I should be back. */
> /** I should be back. */
>
> t/F.java [ 11] static String code = "nix";
> static String code = "nix";
>
> t/F.java [ 12] }
> }
>
>
> It is particularly annoying that the 'implements' in the original text for
> the class A is transferred properly. I assume that this
> is realised by the text copying part in CasualDif. However, the newly
> generated class 'F' is wrong, and it is not clear to me who is responsible
> for
> the generation here. I was not able to fix this in CasualDiff without
> breaking the formatting for either A of F in this case, and such changes
> in CasualDiff might affect the expectations of other transformers (and did,
> by my experience with the PullUp transformer).
>
> I have seen recent changes in CasualDiff, in this case about permits
> clauses (which are not relevant for records, as the can't be extended).
> So could someone please shed some light on the interaction of CasualDiff
> and the other collaborators.
>
> Anyone that is willing to help, I have a fork with a branch at
> https://github.com/homberghp/netbeans/tree/issue7044b
> This is synced with the main branch on sunday April 13.
>
> Thanks in advance.
>
> Met vriendelijke groet,
> Pieter van den Hombergh.
> Kerboschstraat 12
> 5913 WH Venlo
>
