Skip to content

Commit ba4ecbf

Browse files
jjohnstnjjohnstn
authored
Fix extract local to not create tmp variables for certain assignments (#2220)
* Fix extract local to not create tmp variables for certain assignments - fix ChangedValueChecker to accept an Expression to ignore if assigned to - fix ChangedValueChecker.UpdateVisitor to ignore the specified Expression on left side of Assignment, if specified - add new test to ExtractTempTests and fix test cases which behave different with new logic - fix ExtractTempRefactoring to pass the selected expression to ignore if assigned to and if assigned to, replace with a double assignment (x = y = zzzz) - also fix ExtractTempRefactoring name logic to bump up the end number rather than concatenating a new digit - fixes #1901
1 parent 94c3550 commit ba4ecbf

File tree

6 files changed

+101
-28
lines changed

6 files changed

+101
-28
lines changed

‎org.eclipse.jdt.core.manipulation/refactoring/org/eclipse/jdt/internal/corext/refactoring/code/ExtractTempRefactoring.java‎

Lines changed: 34 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*******************************************************************************
2-
* Copyright (c) 2000, 2024 IBM Corporation and others.
2+
* Copyright (c) 2000, 2025 IBM Corporation and others.
33
*
44
* This program and the accompanying materials
55
* are made available under the terms of the Eclipse Public License 2.0
@@ -20,6 +20,7 @@
2020
* Taiming Wang <[email protected]> - [extract local] Automated Name Recommendation For The Extract Local Variable Refactoring. - https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/601
2121
* Taiming Wang <[email protected]> - [extract local] Context-based Automated Name Recommendation For The Extract Local Variable Refactoring. - https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/655
2222
* Taiming Wang <[email protected]> - [extract local] Extract Similar Expression in All Methods If End-Users Want. - https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/785
23+
* Red Hat Inc. - add support to replace assignments with nested assignments (Issue 1901)
2324
*******************************************************************************/
2425
package org.eclipse.jdt.internal.corext.refactoring.code;
2526

@@ -368,11 +369,6 @@ private static ASTNode[] getParents(ASTNode node) {
368369

369370
private static boolean isLeftValue(ASTNode node) {
370371
ASTNode parent= node.getParent();
371-
if (parent instanceof Assignment) {
372-
Assignment assignment= (Assignment) parent;
373-
if (assignment.getLeftHandSide() == node)
374-
return true;
375-
}
376372
if (parent instanceof PostfixExpression)
377373
return true;
378374
if (parent instanceof PrefixExpression) {
@@ -647,12 +643,21 @@ private void addReplaceExpressionWithTemp() throws JavaModelException {
647643
if (!fSeen.add(fragment))
648644
continue;
649645

650-
SimpleName tempName= fCURewrite.getAST().newSimpleName(fTempName);
646+
ASTNode replacement= null;
647+
ASTNode fragmentNode= fragment.getAssociatedNode();
648+
if (fragmentNode.getLocationInParent() == Assignment.LEFT_HAND_SIDE_PROPERTY) {
649+
Assignment newAssignment= fCURewrite.getAST().newAssignment();
650+
newAssignment.setLeftHandSide(fCURewrite.getAST().newSimpleName(fTempName));
651+
newAssignment.setRightHandSide((Expression) rewrite.createCopyTarget(fragmentNode));
652+
replacement= newAssignment;
653+
} else {
654+
replacement= fCURewrite.getAST().newSimpleName(fTempName);
655+
}
651656
TextEditGroup description= fCURewrite.createGroupDescription(RefactoringCoreMessages.ExtractTempRefactoring_replace);
652657

653-
fragment.replace(rewrite, tempName, description);
658+
fragment.replace(rewrite, replacement, description);
654659
if (fLinkedProposalModel != null)
655-
fLinkedProposalModel.getPositionGroup(KEY_NAME, true).addPosition(rewrite.track(tempName), false);
660+
fLinkedProposalModel.getPositionGroup(KEY_NAME, true).addPosition(rewrite.track(replacement), false);
656661
}
657662
}
658663

@@ -859,11 +864,12 @@ private void processSelectedExpression() throws CoreException {
859864
|| retainOnlyReplacableMatches.length == 0) {
860865
createTempDeclaration();
861866
addReplaceExpressionWithTemp();
862-
fTempName= newName + ++cnt;
863-
while (usedNames.contains(fTempName)) {
864-
fTempName= newName + ++cnt;
867+
String oldTempName= fTempName;
868+
String baseName= getBaseName(fTempName);
869+
fTempName= baseName + ++cnt;
870+
while (usedNames.contains(fTempName) || fTempName.equals(oldTempName)) {
871+
fTempName= baseName + ++cnt;
865872
}
866-
867873
}
868874
while (replaceAllOccurrences() &&
869875
retainOnlyReplacableMatches.length > fSeen.size()) {
@@ -885,9 +891,11 @@ private void processSelectedExpression() throws CoreException {
885891
createTempDeclaration();
886892
if (fStartPoint != -1 && fEndPoint != -1) {
887893
addReplaceExpressionWithTemp();
888-
fTempName= newName + ++cnt;
889-
while (usedNames.contains(fTempName)) {
890-
fTempName= newName + ++cnt;
894+
String oldTempName= fTempName;
895+
String baseName= getBaseName(fTempName);
896+
fTempName= baseName + ++cnt;
897+
while (usedNames.contains(fTempName) || fTempName.equals(oldTempName)) {
898+
fTempName= baseName + ++cnt;
891899
}
892900
}
893901
}
@@ -898,6 +906,15 @@ private void processSelectedExpression() throws CoreException {
898906

899907
}
900908

909+
private String getBaseName(String tempName) {
910+
int i= tempName.length() - 1;
911+
while (i > 0 && Character.isDigit(tempName.charAt(i))) {
912+
--i;
913+
}
914+
return tempName.substring(0, i + 1);
915+
}
916+
917+
901918
private void processOtherIdenticalExpressions() throws CoreException {
902919
ASTNode associatedNode= getSelectedExpression().getAssociatedNode();
903920
CompilationUnit cuNode= ASTNodes.getFirstAncestorOrNull(associatedNode, CompilationUnit.class);
@@ -1240,7 +1257,7 @@ private ASTNode evalStartAndEnd(IASTFragment[] retainOnlyReplacableMatches, int
12401257
int end= selectNumber;
12411258
int expandFlag= 2; // 2:backward 1:forward 0:break
12421259

1243-
ChangedValueChecker cvc= new ChangedValueChecker(getSelectedExpression().getAssociatedNode(), fEnclosingKey);
1260+
ChangedValueChecker cvc= new ChangedValueChecker(getSelectedExpression().getAssociatedNode(), fEnclosingKey, true);
12441261

12451262
while (expandFlag > 0 && start <= end) {
12461263
IASTFragment iASTFragment= retainOnlyReplacableMatches[start];

‎org.eclipse.jdt.core.manipulation/refactoring/org/eclipse/jdt/internal/corext/refactoring/util/ChangedValueChecker.java‎

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*******************************************************************************
2-
* Copyright (c) 2000, 2024 IBM Corporation and others.
2+
* Copyright (c) 2000, 2025 IBM Corporation and others.
33
*
44
* This program and the accompanying materials
55
* are made available under the terms of the Eclipse Public License 2.0
@@ -36,6 +36,7 @@
3636
import org.eclipse.jdt.core.IType;
3737
import org.eclipse.jdt.core.JavaModelException;
3838
import org.eclipse.jdt.core.dom.AST;
39+
import org.eclipse.jdt.core.dom.ASTMatcher;
3940
import org.eclipse.jdt.core.dom.ASTNode;
4041
import org.eclipse.jdt.core.dom.ASTParser;
4142
import org.eclipse.jdt.core.dom.ASTVisitor;
@@ -44,6 +45,7 @@
4445
import org.eclipse.jdt.core.dom.CompilationUnit;
4546
import org.eclipse.jdt.core.dom.DoStatement;
4647
import org.eclipse.jdt.core.dom.EnhancedForStatement;
48+
import org.eclipse.jdt.core.dom.Expression;
4749
import org.eclipse.jdt.core.dom.FieldAccess;
4850
import org.eclipse.jdt.core.dom.ForStatement;
4951
import org.eclipse.jdt.core.dom.IBinding;
@@ -95,11 +97,19 @@ public class ChangedValueChecker extends AbstractChecker {
9597

9698
private String fEnclosingMethodSignature;
9799

100+
private ASTNode fAssignmentExpressionToIgnore;
101+
98102
public ChangedValueChecker(ASTNode selectedExpression, String enclosingMethodSignature) {
99103
this.fEnclosingMethodSignature= enclosingMethodSignature;
100104
analyzeSelectedExpression(selectedExpression);
101105
}
102106

107+
public ChangedValueChecker(ASTNode selectedExpression, String enclosingMethodSignature, boolean ignoreAssignmentUpdates) {
108+
this.fEnclosingMethodSignature= enclosingMethodSignature;
109+
this.fAssignmentExpressionToIgnore= ignoreAssignmentUpdates ? selectedExpression : null;
110+
analyzeSelectedExpression(selectedExpression);
111+
}
112+
103113
public void detectConflict(int startOffset, int endOffset, ASTNode node,
104114
ASTNode bodyNode, ArrayList<IASTFragment> candidateList) {
105115
fNode2= node;
@@ -134,7 +144,8 @@ public boolean hasConflict() {
134144
Position pos= new Position(node.getStartPosition(), node.getLength());
135145
if (fPosSet.add(pos)) {
136146
threadPool.execute(() -> {
137-
UpdateVisitor uv= new UpdateVisitor(fDependSet, true);
147+
UpdateVisitor uv= new UpdateVisitor(fDependSet, true, (Expression) fAssignmentExpressionToIgnore);
148+
System.out.println(node);
138149
node.accept(uv);
139150
if (uv.hasConflict()) {
140151
fConflict= true;
@@ -555,9 +566,12 @@ class UpdateVisitor extends ASTVisitor {
555566

556567
private boolean visitMethodCall;
557568

558-
public UpdateVisitor(Set<Elem> dependSet, boolean visitMethodCall) {
569+
private Expression ignoreAssignmentToExpression;
570+
571+
public UpdateVisitor(Set<Elem> dependSet, boolean visitMethodCall, Expression ignoreAssignmentToExpression) {
559572
this.updateSet= new HashSet<>();
560573
this.visitMethodCall= visitMethodCall;
574+
this.ignoreAssignmentToExpression= ignoreAssignmentToExpression;
561575
this.dependSet= dependSet;
562576
}
563577

@@ -579,6 +593,11 @@ private boolean hasConflict() {
579593

580594
@Override
581595
public boolean visit(Assignment assignment) {
596+
if (ignoreAssignmentToExpression != null) {
597+
if (assignment.getLeftHandSide().subtreeMatch(new ASTMatcher(), ignoreAssignmentToExpression)) {
598+
return super.visit(assignment);
599+
}
600+
}
582601
ReadVisitor v= new ReadVisitor(visitMethodCall);
583602
assignment.getLeftHandSide().accept(v);
584603
for (Elem e : v.readSet) {
@@ -622,7 +641,7 @@ public boolean visit(MethodInvocation methodInvocation) {
622641
}
623642
MethodDeclaration md= findFunctionDefinition(resolveMethodBinding.getDeclaringClass(), resolveMethodBinding);
624643
if (md != null && md.getLength() < THRESHOLD) {
625-
UpdateVisitor uv= new UpdateVisitor(dependSet, false);
644+
UpdateVisitor uv= new UpdateVisitor(dependSet, false, ignoreAssignmentToExpression);
626645
md.accept(uv);
627646
for (Elem e : uv.updateSet) {
628647
addToList(new Elem(methodInvocation, visitMethodCall, e));

‎org.eclipse.jdt.ui.tests.refactoring/resources/ExtractTemp/canExtract/A_test127_out.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,8 @@ void method(B b) {
77
System.out.println(v2);
88
}
99
if (b instanceof C) {
10-
int v22= ((C) b).getV2();
11-
System.out.println(v22);
10+
int v3= ((C) b).getV2();
11+
System.out.println(v3);
1212
}
1313

1414
}

‎org.eclipse.jdt.ui.tests.refactoring/resources/ExtractTemp/canExtract/A_test162_in.java‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
package p; // 9, 13, 9, 17
2+
3+
import java.util.ArrayList;
4+
5+
public class A {
6+
private ArrayList list = null;
7+
8+
public void updateBug() {
9+
if (list == null) {
10+
list = new ArrayList<>();
11+
}
12+
int index = 0;
13+
while (index < list.size()) {
14+
Object it = list.get(index++);
15+
}
16+
}
17+
}

‎org.eclipse.jdt.ui.tests.refactoring/resources/ExtractTemp/canExtract/A_test162_out.java‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
package p; // 9, 13, 9, 17
2+
3+
import java.util.ArrayList;
4+
5+
public class A {
6+
private ArrayList list = null;
7+
8+
public void updateBug() {
9+
ArrayList list2= list;
10+
if (list2 == null) {
11+
list2= list = new ArrayList<>();
12+
}
13+
int index = 0;
14+
while (index < list2.size()) {
15+
Object it = list2.get(index++);
16+
}
17+
}
18+
}

‎org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/ExtractTempTests.java‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1113,6 +1113,12 @@ public void test161() throws Exception {
11131113
helper1(7, 26, 7, 50, true, false, "serverPanel", "serverPanel");
11141114
}
11151115

1116+
@Test
1117+
public void test162() throws Exception {
1118+
//test for https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/1901
1119+
helper1(9, 13, 9, 17, true, false, "list2", "list2");
1120+
}
1121+
11161122
@Test
11171123
public void testZeroLengthSelection0() throws Exception {
11181124
// printTestDisabledMessage("test for bug 30146");
@@ -1272,11 +1278,7 @@ public void testFail26() throws Exception {
12721278
failHelper1(4, 15, 4, 20, false, false, "temp", RefactoringStatus.FATAL);
12731279
}
12741280

1275-
@Test
1276-
public void testFail27() throws Exception {
1277-
// printTestDisabledMessage("test for bug 29513");
1278-
failHelper1(7, 13, 7, 24, true, false, "temp", RefactoringStatus.WARNING);
1279-
}
1281+
// testFail27() available as it is no longer a failure case
12801282

12811283
@Test
12821284
public void testFail28() throws Exception {

0 commit comments

Comments
 (0)