Hi Jan,
I understand your modification suggestion of VerryPretty to print the
record parameters.
I had that in an earlier attempt, which made it even to PR status, but I
have closed that PR before acceptance, because of the
'implements after the {' so I started anew.Running my test on the code with your changes applied shows that the tests do not hit them. (I inserted an extra System.err.println()) I have added record-specific test with file. *.../org/netbeans/modules/refactoring/java/test/InnerOuterRecordTest.java* To make the output more intuitive, I have added a test helper class in harness/nbunit in file *.../org/netbeans/junit/AssertLinesEqualHelpers.java* and modified *.../org/netbeans/modules/refactoring/java/test/RefactoringTestbase.java* All can be found in https://github.com/homberghp/netbeans/tree/issue7044b. I have added the named files in a zip for your convenience. Of course, these files would have gone/will go into a successful PR. Inspect, and unpack (unzip -l) in the root of the NetBeans project/branch. Met vriendelijke groet, Pieter van den Hombergh. Kerboschstraat 12 5913 WH Venlo On Mon, Apr 14, 2025 at 7:59 PM Jan Lahoda <lah...@gmail.com> wrote: > While it is true that some refactorings like inner to outter is mostly > copying stuff, the point of the code generator is to abstract from that. So > that at the feature level (for each IDE feature) we don't need to think > about all the aspects of each language feature. > > When creating a tree that does not correspond to anything in the original > source, CasualDiff will, of course, use VeryPretty. And VeryPretty has some > capability to simply copy the original text to the new place (see > VeryPretty.handlePossibleOldTrees), although I don't think it currently > works across files. > > I've sketched some very simply changes to make at least some simple > inner-to-outter refactoring partially work: > https://github.com/lahodaj/netbeans/tree/NETBEANS-7044-attempt > > Although there's much more to do. (Among other things, the tests should > primarily be in java.source.base.) > > Jan > > > On Mon, Apr 14, 2025 at 5:24 PM Pieter van den Hombergh < > pieter.van.den.hombe...@gmail.com> wrote: > > > Jan, > > thank you for your time to read and extensively answers to my questions. > > > > In the case of inner-to-outer the "old" AST (oldT in the code) > > is rather useless imho, because it has no relation to the original code > but > > instead is generated from a template, with an empty parameter list and > > empty class body. So diffing is actually rebuilding the code from the > AST. > > Which is what the VerryPretty code printer does or should do. Also, in > case > > of enum and record, which have no link to the host class, the "new" code > is > > a verbatim copy of the outed inner class, with the small exception of > what > > the code template brings for license and class javadoc code, and of > course > > out-denting. > > > > Where could we benefit from those facts? > > met vriendelijke groet > > Pieter van den Hombergh > > > > Op ma 14 apr 2025, 15:10 schreef Jan Lahoda <jlah...@apache.org>: > > > > > 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 > > > > > > > > > >
<<attachment: jlahoda.zip>>
--------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@netbeans.apache.org For additional commands, e-mail: dev-h...@netbeans.apache.org For further information about the NetBeans mailing lists, visit: https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
