apache / netbeans

Apache NetBeans
https://netbeans.apache.org/
Apache License 2.0
2.63k stars 841 forks source link

Fixing rename refactoring for record components and compact constructors #7670

Closed lahodaj closed 2 weeks ago

lahodaj commented 1 month ago

This is an attempt to fix rename refactoring for record components. Will still need some cleanup/more testing. Does not alter the AST shapes in any way, but uses them to perform the work. Will also rename "linked" elements for the component, like the accessor and the parameter for the (compact) constructor.

@mbien - not sure if you are looking at something similar? I apologize if I stepped on something you're looking at.

mbien commented 1 month ago
  • instant-renaming the record itself will not rename the compact constructor

The second point is caused due to InstntRenamePerformer#computeChangePoints looping through ElementFilter.constructorsIn(el.getEnclosedElements())), however, this list does not include the compact constructor. In fact, it isn't even in getEnclosedElements()- so I don't know how to get it unfortunately.

ah I get it. The compact constructor is represented by the canonical constructor. Utilities#getToken can't convert it to a token though and returns null. https://github.com/apache/netbeans/blob/36171a7e45f592c11be357fbef9a1dcf8d0e08ec/java/java.editor/src/org/netbeans/modules/java/editor/rename/InstantRenamePerformer.java#L377-L385

mbien commented 3 weeks ago

spoke a bit too soon, using the example https://github.com/apache/netbeans/pull/7670#pullrequestreview-2243889285 and renaming a record component will rename it with instant-rename now which forgets about accessor methods and also about the compact constructor param link. I believe last iteration used non-instant rename which works fine now.

        System.out.println(vec.x);
        //              rename ^  (uses now instant-rename which doesn't rename accessor,
        //                         using the right-click rename action works)

it might be best to use non-instant rename for record components for now.

lahodaj commented 3 weeks ago

I have squashed and fixed on more issue (the rename didn't work when the rename started from the constructor's parameter, because the TreePathHandle didn't use the ElementHandle for it, which then failed the refactorings in all other files expect the one that contains the parameter). If the tests are green, and there are no objections, I would merge.

lahodaj commented 2 weeks ago

I've found another problem yesterday: ElementHandle.create was happily creating ElementHandle for lambda parameters, but could not (quite obviously) resolve them back. In some cases, this then broke the refactoring. I've attempted to tweak that in this commit: https://github.com/apache/netbeans/pull/7670/commits/56461a268907b628093ddf840cb875b329b02b79

mbien commented 2 weeks ago

still +1 from me - great fixes!

lahodaj commented 2 weeks ago

If the tests are green, noone objects, and noone will find a new problem, I'll merge tomorrow. Thanks for the reviews!