You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@netbeans.apache.org by "lahodaj (via GitHub)" <gi...@apache.org> on 2024/08/13 07:48:03 UTC
[PR] Fixing rename refactoring for record components [netbeans]
lahodaj opened a new pull request, #7670:
URL: https://github.com/apache/netbeans/pull/7670
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixing rename refactoring for record components [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #7670:
URL: https://github.com/apache/netbeans/pull/7670#discussion_r1720570335
##########
java/java.editor/src/org/netbeans/modules/java/editor/rename/InstantRenamePerformer.java:
##########
@@ -450,6 +450,10 @@ private static boolean allowInstantRename(CompilationInfo info, Element e, Eleme
if(isProperty) {
return false;
}
+ boolean isRecordComponent = e.getEnclosingElement().getKind() == ElementKind.RECORD;
+ if (isRecordComponent) {
+ return false;
+ }
}
Review Comment:
this is for component accessor methods (L457):
```java
} else if (e.getKind() == ElementKind.METHOD) {
Element ee = e.getEnclosingElement();
if (ee.getKind() == ElementKind.RECORD) {
for (RecordComponentElement rc : ((TypeElement) ee).getRecordComponents()) {
if (e.getSimpleName().equals(rc.getSimpleName())) {
return false;
}
}
}
}
```
##########
java/refactoring.java/src/org/netbeans/modules/refactoring/java/plugins/RenameRefactoringPlugin.java:
##########
@@ -433,6 +438,27 @@ public void run(CompilationController info) throws Exception {
//add all references of overriding methods
allMethods.add(ElementHandle.create((ExecutableElement)el));
}
+ if (kind == ElementKind.FIELD && el.getEnclosingElement().getKind() == ElementKind.RECORD) {
+ for (ExecutableElement m : ElementFilter.methodsIn(el.getEnclosingElement().getEnclosedElements())) {
+ if (m.getSimpleName().equals(el.getSimpleName()) && m.getParameters().isEmpty()) {
+ //accessor:
+// addMethods(m, set, info, idx);
+ recordLinkedDeclarations.add(TreePathHandle.create(m, info));
+ addUsesOfMethod(m, set, info, idx);
+ }
+ }
+ for (ExecutableElement c : ElementFilter.constructorsIn(el.getEnclosingElement().getEnclosedElements())) {
+ if (!info.getElements().isCanonicalConstructor(c)) {
+ continue;
+ }
+ for (VariableElement param : c.getParameters()) {
+ if (param.getSimpleName().equals(el.getSimpleName())) {
+ //the parameter for the canonical constructor:
+ recordLinkedDeclarations.add(TreePathHandle.create(param, info));
+ }
+ }
+ }
+ }
Review Comment:
L467 below would need the reverse case for renaming a record accessor which would have to be linked to the comonents i believe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixing rename refactoring for record components and compact constructors [netbeans]
Posted by "lahodaj (via GitHub)" <gi...@apache.org>.
lahodaj commented on PR #7670:
URL: https://github.com/apache/netbeans/pull/7670#issuecomment-2318577851
If the tests are green, noone objects, and noone will find a new problem, I'll merge tomorrow. Thanks for the reviews!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixing rename refactoring for record components and compact constructors [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #7670:
URL: https://github.com/apache/netbeans/pull/7670#discussion_r1722578595
##########
java/java.editor/src/org/netbeans/modules/java/editor/rename/InstantRenamePerformer.java:
##########
@@ -450,14 +450,13 @@ private static boolean allowInstantRename(CompilationInfo info, Element e, Eleme
if(isProperty) {
return false;
}
- boolean isRecordComponent = e.getEnclosingElement().getKind() == ElementKind.RECORD;
- if (isRecordComponent) {
- return false;
- }
Review Comment:
might be still needed after all?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixing rename refactoring for record components and compact constructors [netbeans]
Posted by "lahodaj (via GitHub)" <gi...@apache.org>.
lahodaj commented on code in PR #7670:
URL: https://github.com/apache/netbeans/pull/7670#discussion_r1722735263
##########
java/java.editor/src/org/netbeans/modules/java/editor/rename/InstantRenamePerformer.java:
##########
@@ -450,14 +450,13 @@ private static boolean allowInstantRename(CompilationInfo info, Element e, Eleme
if(isProperty) {
return false;
}
- boolean isRecordComponent = e.getEnclosingElement().getKind() == ElementKind.RECORD;
- if (isRecordComponent) {
- return false;
- }
Review Comment:
I just placed the new check at a wrong place. Sorry for that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixing rename refactoring for record components [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on PR #7670:
URL: https://github.com/apache/netbeans/pull/7670#issuecomment-2294664524
> - 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixing rename refactoring for record components and compact constructors [netbeans]
Posted by "lahodaj (via GitHub)" <gi...@apache.org>.
lahodaj commented on PR #7670:
URL: https://github.com/apache/netbeans/pull/7670#issuecomment-2311975471
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixing rename refactoring for record components and compact constructors [netbeans]
Posted by "lahodaj (via GitHub)" <gi...@apache.org>.
lahodaj commented on PR #7670:
URL: https://github.com/apache/netbeans/pull/7670#issuecomment-2310323012
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixing rename refactoring for record components [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #7670:
URL: https://github.com/apache/netbeans/pull/7670#discussion_r1715695760
##########
java/java.source.base/apichanges.xml:
##########
@@ -25,6 +25,18 @@
<apidef name="javasource_base">Java Source API</apidef>
</apidefs>
<changes>
+ <change id="TreeMaker.RecordComponent">
+ <api name="javasource_base" />
+ <summary>Adding TreeMaker.StringTemplate</summary>
Review Comment:
-> "Adding TreeMaker.RecordComponent"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixing rename refactoring for record components and compact constructors [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #7670:
URL: https://github.com/apache/netbeans/pull/7670#discussion_r1734306804
##########
java/java.source.base/apichanges.xml:
##########
@@ -25,6 +25,18 @@
<apidef name="javasource_base">Java Source API</apidef>
</apidefs>
<changes>
+ <change id="TreeMaker.RecordComponent">
+ <api name="javasource_base" />
+ <summary>Adding TreeMaker.RecordComponent</summary>
+ <version major="1" minor="2.70.0"/>
+ <date day="20" month="8" year="2023"/>
Review Comment:
@lahodaj just wanting to make sure that 2023 was intended ;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixing rename refactoring for record components and compact constructors [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on PR #7670:
URL: https://github.com/apache/netbeans/pull/7670#issuecomment-2314779100
still +1 from me - great fixes!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixing rename refactoring for record components [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #7670:
URL: https://github.com/apache/netbeans/pull/7670#discussion_r1715695760
##########
java/java.source.base/apichanges.xml:
##########
@@ -25,6 +25,18 @@
<apidef name="javasource_base">Java Source API</apidef>
</apidefs>
<changes>
+ <change id="TreeMaker.RecordComponent">
+ <api name="javasource_base" />
+ <summary>Adding TreeMaker.StringTemplate</summary>
Review Comment:
-> "Adding TreeMaker.RecordComponent"
(I do miss STs too)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixing rename refactoring for record components [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #7670:
URL: https://github.com/apache/netbeans/pull/7670#discussion_r1722523433
##########
java/java.source.base/apichanges.xml:
##########
@@ -25,6 +25,18 @@
<apidef name="javasource_base">Java Source API</apidef>
</apidefs>
<changes>
+ <change id="TreeMaker.RecordComponent">
+ <api name="javasource_base" />
+ <summary>Adding TreeMaker.RecordComponent</summary>
+ <version major="1" minor="2.70.0"/>
+ <date day="20" month="8" year="2023"/>
Review Comment:
check date
##########
java/java.source.base/src/org/netbeans/api/java/source/ElementUtilities.java:
##########
@@ -928,7 +929,143 @@ public ExecutableElement getDescriptorElement(TypeElement origin) {
}
return null;
}
-
+
+ /**
+ * Find all elements that are linked record elements for the given input. Will
+ * return the record component element, field, accessor method, and canonical constructor
+ * parameters.
+ *
+ * This method can be called on any {@code Element}, and will return a collection
+ * with a single entry if the provided element is not a record element.
+ *
+ * @param forElement for which the linked elements should be found
+ * @return a collection containing the provided element, plus any additional elements
+ * that are linked to it by the Java record specification
+ * @since 2.70
+ */
+ public Collection<? extends Element> getLinkedRecordElements(Element forElement) {
Review Comment:
great method!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixing rename refactoring for record components and compact constructors [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on PR #7670:
URL: https://github.com/apache/netbeans/pull/7670#issuecomment-2297752737
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. I believe last iteration used non-instant rename which works fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixing rename refactoring for record components and compact constructors [netbeans]
Posted by "lahodaj (via GitHub)" <gi...@apache.org>.
lahodaj merged PR #7670:
URL: https://github.com/apache/netbeans/pull/7670
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists