Java: Don't introduce parameter for potentially matching fields if the fields are written (IDEA-182284)

java/java-analysis-impl/src/com/intellij/refactoring/util/duplicates/ExtractableExpressionPart.java

@@ -17,6 +17,7 @@ package com.intellij.refactoring.util.duplicates;
 
 import com.intellij.codeInsight.JavaPsiEquivalenceUtil;
 import com.intellij.psi.*;
+import com.intellij.psi.tree.IElementType;
 import com.intellij.psi.util.PsiTreeUtil;
 import com.intellij.psi.util.PsiUtil;
 import org.jetbrains.annotations.NotNull;
@@ -121,6 +122,9 @@ public class ExtractableExpressionPart {
   @Nullable
   static ExtractableExpressionPart matchVariable(@NotNull PsiReferenceExpression expression, @Nullable List<PsiElement> scope) {
     PsiElement resolved = expression.resolve();
+    if (resolved instanceof PsiField && isUnqualifiedModification(expression)) {
+      return null;
+    }
     if (resolved instanceof PsiVariable && (scope == null || !DuplicatesFinder.isUnder(resolved, scope))) {
       PsiVariable variable = (PsiVariable)resolved;
       return new ExtractableExpressionPart(expression, variable, null, variable.getType());
@@ -128,6 +132,28 @@ public class ExtractableExpressionPart {
     return null;
   }
 
+  private static boolean isUnqualifiedModification(@NotNull PsiReferenceExpression expression) {
+    PsiExpression qualifier = expression.getQualifierExpression();
+    if (qualifier == null || qualifier instanceof PsiQualifiedExpression) { // 'this.' and 'super.' with fields are just like unqualified
+      PsiElement parent = PsiUtil.skipParenthesizedExprUp(expression.getParent());
+      if (parent instanceof PsiAssignmentExpression) {
+        PsiAssignmentExpression assignment = (PsiAssignmentExpression)parent;
+        if (PsiTreeUtil.isAncestor(assignment.getLExpression(), expression, false)) {
+          return true;
+        }
+      }
+      else if (parent instanceof PsiUnaryExpression) {
+        PsiUnaryExpression unary = (PsiUnaryExpression)parent;
+        IElementType tokenType = unary.getOperationTokenType();
+        if ((tokenType.equals(JavaTokenType.PLUSPLUS) || tokenType.equals(JavaTokenType.MINUSMINUS)) &&
+            PsiTreeUtil.isAncestor(unary.getOperand(), expression, false)) {
+          return true;
+        }
+      }
+    }
+    return false;
+  }
+
   @Nullable
   private static ExtractableExpressionPart matchExpression(@NotNull PsiExpression expression) {
     PsiType type = expression.getType();

java/java-analysis-impl/src/com/intellij/refactoring/util/duplicates/ExtractedParameter.java

@@ -50,12 +50,6 @@ public class ExtractedParameter {
     if (type == null) {
       return false;
     }
-    if (patternPart.myVariable != null && !isStaticOrLocal(patternPart.myVariable)) {
-      return false;
-    }
-    if (candidatePart.myVariable != null && !isStaticOrLocal(candidatePart.myVariable)) {
-      return false;
-    }
     for (ExtractedParameter parameter : parameters) {
       boolean samePattern = parameter.samePattern(patternPart);
       boolean sameCandidate = parameter.sameCandidate(candidatePart);
@@ -141,13 +135,6 @@ public class ExtractedParameter {
     return false;
   }
 
-  static boolean isStaticOrLocal(@NotNull PsiVariable variable) {
-    if (variable instanceof PsiField && variable.hasModifierProperty(PsiModifier.STATIC)) {
-      return true;
-    }
-    return variable instanceof PsiLocalVariable || variable instanceof PsiParameter;
-  }
-
   private static class FieldModificationVisitor extends JavaRecursiveElementWalkingVisitor {
     private final Set<PsiField> myFields;
     private boolean myModified;

java/java-tests/testData/refactoring/extractMethod/ArgumentFoldingMethodCall.java

@@ -0,0 +1,24 @@
+import java.util.List;
+
+class C {
+    List<String> x;
+    List<String> y;
+
+    private void foo() {
+        <selection>
+        if (x.isEmpty()) return;
+        x.remove(0);
+        y.add(str());
+        baz();</selection>
+    }
+
+    private void bar() {
+        if (y.isEmpty()) return;
+        y.remove(0);
+        x.add(str());
+        baz();
+    }
+
+    private void baz() { }
+    private String str() { return null; }
+}

java/java-tests/testData/refactoring/extractMethod/ArgumentFoldingMethodCall_after.java

@@ -0,0 +1,25 @@
+import java.util.List;
+
+class C {
+    List<String> x;
+    List<String> y;
+
+    private void foo() {
+
+        newMethod(x, y);
+    }
+
+    private void newMethod(List<String> x, List<String> y) {
+        if (x.isEmpty()) return;
+        x.remove(0);
+        y.add(str());
+        baz();
+    }
+
+    private void bar() {
+        newMethod(y, x);
+    }
+
+    private void baz() { }
+    private String str() { return null; }
+}

java/java-tests/testData/refactoring/extractMethod/ArgumentFoldingWholeStatementForUpdateList_after.java

@@ -6,19 +6,17 @@ class ArgumentFoldingWholeStatement {
 
     private void foo() {
 
-        newMethod();
+        newMethod(x);
     }
 
-    private void newMethod() {
+    private void newMethod(List<String> x) {
         for (int i = 0; i < 5; i++, x.indexOf(str())) {
             baz();
         }
     }
 
     private void bar() {
-        for (int i = 0; i < 5; i++, y.indexOf(str())) {
-            baz();
-        }
+        newMethod(y);
     }
 
     private String str() { return null; }

java/java-tests/testData/refactoring/extractMethod/ArgumentFoldingWholeStatementForUpdate_after.java

@@ -6,19 +6,17 @@ class ArgumentFoldingWholeStatement {
 
     private void foo() {
 
-        newMethod();
+        newMethod(x);
     }
 
-    private void newMethod() {
+    private void newMethod(List<String> x) {
         for (int i = 0; ++i < 5; x.indexOf(str())) {
             baz();
         }
     }
 
     private void bar() {
-        for (int i = 0; ++i < 5; y.indexOf(str())) {
-            baz();
-        }
+        newMethod(y);
     }
 
     private String str() { return null; }

java/java-tests/testData/refactoring/extractMethod/ArgumentFoldingWholeStatement_after.java

@@ -5,17 +5,16 @@ class ArgumentFoldingWholeStatement {
     List<String> y;
 
     private void foo() {
-        newMethod();
+        newMethod(x);
     }
 
-    private void newMethod() {
+    private void newMethod(List<String> x) {
         x.add(str());
         baz();
     }
 
     private void bar() {
-        y.add(str());
-        baz();
+        newMethod(y);
     }
 
     private String str() { return null; }

java/java-tests/testData/refactoring/extractMethod/DecrementDifferentFieldsDuplicate.java

@@ -0,0 +1,19 @@
+class DecrementDifferentFieldsDuplicate {
+    int x;
+    int y;
+
+    private void foo() {
+        <selection>
+        if (x > 0) {
+            bar(x);
+            x--;
+        }</selection>
+
+        if (y > 0) {
+            bar(y);
+            y--;
+        }
+    }
+
+    private void bar(int i) { }
+}

java/java-tests/testData/refactoring/extractMethod/DecrementDifferentFieldsDuplicate_after.java

@@ -0,0 +1,23 @@
+class DecrementDifferentFieldsDuplicate {
+    int x;
+    int y;
+
+    private void foo() {
+
+        newMethod();
+
+        if (y > 0) {
+            bar(y);
+            y--;
+        }
+    }
+
+    private void newMethod() {
+        if (x > 0) {
+            bar(x);
+            x--;
+        }
+    }
+
+    private void bar(int i) { }
+}

java/java-tests/testSrc/com/intellij/java/refactoring/ExtractMethodTest.java

@@ -1181,6 +1181,10 @@ public class ExtractMethodTest extends LightCodeInsightTestCase {
     doDuplicatesTest();
   }
 
+  public void testDecrementDifferentFieldsDuplicate() throws Exception {
+    doDuplicatesTest();
+  }
+
   public void testArgumentFoldingWholeStatement() throws Exception {
     doDuplicatesTest();
   }
@@ -1193,6 +1197,10 @@ public class ExtractMethodTest extends LightCodeInsightTestCase {
     doDuplicatesTest();
   }
 
+  public void testArgumentFoldingMethodCall() throws Exception {
+    doDuplicatesTest();
+  }
+
   private void doTestDisabledParam() throws PrepareFailedException {
     final CodeStyleSettings settings = CodeStyleSettingsManager.getSettings(getProject());
     settings.ELSE_ON_NEW_LINE = true;