Java: Reworked introducing the parameters in duplicate fragments to support repeated argument values (IDEA-201024)

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

@@ -62,22 +62,16 @@ public class ExtractableExpressionPart {
     return new ExtractableExpressionPart(myUsage, myVariable, myValue, myType);
   }
 
-  @NotNull
-  ExtractableExpressionPart deepCopy() {
-    PsiElementFactory factory = JavaPsiFacade.getElementFactory(myUsage.getProject());
-    PsiExpression usageCopy = factory.createExpressionFromText(myUsage.getText(), myUsage);
-    return new ExtractableExpressionPart(usageCopy, myVariable, myValue, myType);
-  }
-
-  boolean isEquivalent(@NotNull ExtractableExpressionPart part) {
+  public boolean isEquivalent(@NotNull ExtractableExpressionPart part) {
     if (myVariable != null && myVariable.equals(part.myVariable)) {
       return true;
     }
     if (myValue != null && myValue.equals(part.myValue)) {
       return true;
     }
-    return JavaPsiEquivalenceUtil.areExpressionsEquivalent(PsiUtil.skipParenthesizedExprDown(myUsage),
-                                                           PsiUtil.skipParenthesizedExprDown(part.myUsage));
+    PsiExpression usage1 = PsiUtil.skipParenthesizedExprDown(myUsage);
+    PsiExpression usage2 = PsiUtil.skipParenthesizedExprDown(part.myUsage);
+    return usage1 != null && usage2 != null && JavaPsiEquivalenceUtil.areExpressionsEquivalent(usage1, usage2);
   }
 
   @Nullable
@@ -173,4 +167,9 @@ public class ExtractableExpressionPart {
       : "expected " + type.getCanonicalText() + ", got " + usageType.getCanonicalText();
     return new ExtractableExpressionPart(usage, null, null, type);
   }
+
+  @Override
+  public String toString() {
+    return myUsage.getText();
+  }
 }

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

@@ -50,17 +50,6 @@ public class ExtractedParameter {
     if (type == null) {
       return false;
     }
-    for (ExtractedParameter parameter : parameters) {
-      boolean samePattern = parameter.samePattern(patternPart);
-      boolean sameCandidate = parameter.sameCandidate(candidatePart);
-      if (samePattern && sameCandidate) {
-        parameter.addUsages(patternPart);
-        return true;
-      }
-      if (samePattern || sameCandidate) {
-        return false;
-      }
-    }
     parameters.add(new ExtractedParameter(patternPart, candidatePart, type));
     return true;
   }
@@ -78,21 +67,13 @@ public class ExtractedParameter {
     return type.getCanonicalText();
   }
 
-  public void addUsages(ExtractableExpressionPart patternPart) {
+  public void addUsages(@NotNull ExtractableExpressionPart patternPart) {
     myPatternUsages.add(patternPart.getUsage());
   }
 
-  private boolean sameCandidate(ExtractableExpressionPart part) {
-    return myCandidate.isEquivalent(part);
-  }
-
-  private boolean samePattern(ExtractableExpressionPart part) {
-    return myPattern.isEquivalent(part);
-  }
-
-  public static List<Match> getCompatibleMatches(List<Match> matches,
-                                                 PsiElement[] pattern,
-                                                 List<PsiElement[]> candidates) {
+  public static List<Match> getCompatibleMatches(@NotNull List<Match> matches,
+                                                 @NotNull PsiElement[] pattern,
+                                                 @NotNull List<PsiElement[]> candidates) {
     List<Match> result = new ArrayList<>();
     Set<PsiExpression> firstUsages = null;
     for (Match match : matches) {
@@ -119,7 +100,7 @@ public class ExtractedParameter {
     return result;
   }
 
-  private static boolean containsModifiedField(@NotNull PsiElement[] elements, Set<PsiVariable> variables) {
+  private static boolean containsModifiedField(@NotNull PsiElement[] elements, @NotNull Set<PsiVariable> variables) {
     Set<PsiField> fields = StreamEx.of(variables)
       .select(PsiField.class)
       .filter(field -> !field.hasModifierProperty(PsiModifier.FINAL))
@@ -183,4 +164,9 @@ public class ExtractedParameter {
       }
     }
   }
+
+  @Override
+  public String toString() {
+    return myPattern + " -> " + myCandidate + " [" + myPatternUsages.size() + "] : " + myType.getPresentableText();
+  }
 }

java/java-impl/src/com/intellij/refactoring/extractMethod/ParametrizedDuplicates.java

@@ -258,7 +258,7 @@ public class ParametrizedDuplicates {
     Map<Match, Map<PsiExpression, PsiExpression>> expressionsMapping = new HashMap<>();
     for (ClusterOfUsages usages : myUsagesList) {
       for (Match match : myMatches) {
-        ExtractedParameter parameter = usages.myParameters.get(match);
+        ExtractedParameter parameter = usages.getParameter(match);
         if (parameter == null) {
           Map<PsiExpression, PsiExpression> expressions =
             expressionsMapping.computeIfAbsent(match, unused -> {
@@ -277,10 +277,36 @@ public class ParametrizedDuplicates {
       }
     }
 
+    mergeDuplicateUsages(myUsagesList, myMatches);
     myUsagesList.sort(Comparator.comparing(usages -> usages.myFirstOffset));
     return true;
   }
 
+  private static void mergeDuplicateUsages(@NotNull List<ClusterOfUsages> usagesList, @NotNull List<Match> matches) {
+    Set<ClusterOfUsages> duplicateUsages = new THashSet<>();
+    for (int i = 0; i < usagesList.size(); i++) {
+      ClusterOfUsages usages = usagesList.get(i);
+      if (duplicateUsages.contains(usages)) continue;
+
+      for (int j = i + 1; j < usagesList.size(); j++) {
+        ClusterOfUsages otherUsages = usagesList.get(j);
+
+        if (usages.isEquivalent(otherUsages, matches)) {
+          for (Match match : matches) {
+            ExtractedParameter parameter = usages.getParameter(match);
+            ExtractedParameter otherParameter = otherUsages.getParameter(match);
+            if (parameter != null && otherParameter != null) {
+              parameter.addUsages(otherParameter.myPattern);
+              match.getExtractedParameters().remove(otherParameter);
+            }
+          }
+          duplicateUsages.add(otherUsages);
+        }
+      }
+    }
+    usagesList.removeAll(duplicateUsages);
+  }
+
   private static List<Match> filterNestedSubexpressions(List<Match> matches) {
     Map<PsiExpression, Set<Match>> patternUsages = new THashMap<>();
     for (Match match : matches) {
@@ -319,8 +345,8 @@ public class ParametrizedDuplicates {
     List<ExtractedParameter> parameters = match.getExtractedParameters();
     for (ExtractedParameter parameter : parameters) {
       ClusterOfUsages usages = usagesMap.get(parameter.myPattern.getUsage());
-      if (usages != null && !usages.isEquivalent(parameter) ||
-          usages == null && ClusterOfUsages.isPresent(usagesMap, parameter)) {
+      if (usages != null && !usages.arePatternsEquivalent(parameter) ||
+          usages == null && ClusterOfUsages.isPatternPresent(usagesMap, parameter)) {
         return null;
       }
       if (usages == null) {
@@ -670,16 +696,40 @@ public class ParametrizedDuplicates {
       myFirstOffset = myPatterns.stream().mapToInt(PsiElement::getTextOffset).min().orElse(0);
     }
 
-    public void putParameter(Match match, ExtractedParameter parameter) {
+    void putParameter(@NotNull Match match, @NotNull ExtractedParameter parameter) {
       myParameters.put(match, parameter);
     }
 
-    public boolean isEquivalent(ExtractedParameter parameter) {
+    @Nullable
+    ExtractedParameter getParameter(@NotNull Match match) {
+      return myParameters.get(match);
+    }
+
+    boolean arePatternsEquivalent(@NotNull ExtractedParameter parameter) {
       return myPatterns.equals(parameter.myPatternUsages);
     }
 
-    public static boolean isPresent(Map<PsiExpression, ClusterOfUsages> usagesMap, @NotNull ExtractedParameter parameter) {
+    boolean isEquivalent(@NotNull ClusterOfUsages usages, @NotNull Collection<Match> matches) {
+      if (!myParameter.myPattern.isEquivalent(usages.myParameter.myPattern)) {
+        return false;
+      }
+      for (Match match : matches) {
+        ExtractedParameter parameter = getParameter(match);
+        ExtractedParameter otherParameter = usages.getParameter(match);
+        if (parameter == null || otherParameter == null || !parameter.myCandidate.isEquivalent(otherParameter.myCandidate)) {
+          return false;
+        }
+      }
+      return true;
+    }
+
+    static boolean isPatternPresent(@NotNull Map<PsiExpression, ClusterOfUsages> usagesMap, @NotNull ExtractedParameter parameter) {
       return parameter.myPatternUsages.stream().anyMatch(usagesMap::containsKey);
     }
+
+    @Override
+    public String toString() {
+      return StreamEx.of(myParameters.values()).map(p -> p.myPattern + "->" + p.myCandidate).joining(", ");
+    }
   }
 }

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

@@ -0,0 +1,14 @@
+class C {
+  void foo(String s, StringBuilder b) {
+    b.append("repeated");
+    b.append("repeated");
+    b.append("repeated");
+
+    <selection>
+    b.append("a");
+    b.append("a");
+    b.append("a");
+    </selection>
+    b.append("d");
+  }
+}

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

@@ -0,0 +1,16 @@
+class C {
+  void foo(String s, StringBuilder b) {
+      newMethod(b, "repeated");
+
+
+      newMethod(b, "a");
+
+      b.append("d");
+  }
+
+    private void newMethod(StringBuilder b, String a) {
+        b.append(a);
+        b.append(a);
+        b.append(a);
+    }
+}

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

@@ -0,0 +1,14 @@
+class C {
+  void foo(String s, StringBuilder b) {
+    b.append("repeated");
+    b.append(s);
+    b.append("repeated");
+
+    <selection>
+    b.append("a");
+    b.append("b");
+    b.append("c");
+    </selection>
+    b.append("d");
+  }
+}

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

@@ -0,0 +1,16 @@
+class C {
+  void foo(String s, StringBuilder b) {
+      newMethod(b, "repeated", s, "repeated");
+
+
+      newMethod(b, "a", "b", "c");
+
+      b.append("d");
+  }
+
+    private void newMethod(StringBuilder b, String a, String b2, String c) {
+        b.append(a);
+        b.append(b2);
+        b.append(c);
+    }
+}

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

@@ -0,0 +1,16 @@
+class C {
+  void foo(String s, StringBuilder b) {
+    b.append("repeated");
+    b.append("repeated");
+    b.append(s);
+    b.append("repeated");
+
+    <selection>
+    b.append("a");
+    b.append("b");
+    b.append("b");
+    b.append("c");
+    </selection>
+    b.append("d");
+  }
+}

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

@@ -0,0 +1,17 @@
+class C {
+  void foo(String s, StringBuilder b) {
+      newMethod(b, "repeated", "repeated", s, "repeated");
+
+
+      newMethod(b, "a", "b", "b", "c");
+
+      b.append("d");
+  }
+
+    private void newMethod(StringBuilder b, String a, String b2, String b3, String c) {
+        b.append(a);
+        b.append(b2);
+        b.append(b3);
+        b.append(c);
+    }
+}

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

@@ -935,6 +935,18 @@ public class ExtractMethodTest extends LightCodeInsightTestCase {
     doExactDuplicatesTest();
   }
 
+  public void testParametrizedDuplicateRepeatedArguments() throws Exception {
+    doDuplicatesTest();
+  }
+
+  public void testParametrizedDuplicateTripleRepeatedArguments() throws Exception {
+    doDuplicatesTest();
+  }
+
+  public void testParametrizedDuplicateExactlyRepeatedArguments() throws Exception {
+    doDuplicatesTest();
+  }
+
   public void testSuggestChangeSignatureWithChangedParameterName() throws Exception {
     configureByFile(BASE_PATH + getTestName(false) + ".java");
     boolean success = performExtractMethod(true, true, getEditor(), getFile(), getProject(), false, null, false, "p");