Java: Fixed extracting method from duplicates containing reused variables (IDEA-188894)

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

@@ -28,10 +28,7 @@ import com.intellij.psi.util.PsiUtil;
 import com.intellij.refactoring.introduceField.ElementToWorkOn;
 import com.intellij.refactoring.introduceParameter.IntroduceParameterHandler;
 import com.intellij.refactoring.util.VariableData;
-import com.intellij.refactoring.util.duplicates.DuplicatesFinder;
-import com.intellij.refactoring.util.duplicates.ExtractedParameter;
-import com.intellij.refactoring.util.duplicates.Match;
-import com.intellij.refactoring.util.duplicates.VariableReturnValue;
+import com.intellij.refactoring.util.duplicates.*;
 import com.intellij.util.containers.ContainerUtil;
 import com.intellij.util.text.UniqueNameGenerator;
 import gnu.trove.THashMap;
@@ -326,16 +323,45 @@ public class ParametrizedDuplicates {
 
   @NotNull
   private static PsiElement[] wrapWithCodeBlock(@NotNull PsiElement[] elements) {
-    PsiElement parent = elements[0].getParent();
-    PsiElementFactory factory = JavaPsiFacade.getElementFactory(elements[0].getProject());
+    PsiElement fragmentStart = elements[0];
+    PsiElement fragmentEnd = elements[elements.length - 1];
+    List<ReusedLocalVariable> reusedLocalVariables =
+      ReusedLocalVariablesFinder.findReusedLocalVariables(fragmentStart, fragmentEnd, Collections.emptySet());
+
+    PsiElement parent = fragmentStart.getParent();
+    PsiElementFactory factory = JavaPsiFacade.getElementFactory(fragmentStart.getProject());
     PsiBlockStatement statement = (PsiBlockStatement)factory.createStatementFromText("{}", parent);
-    statement.getCodeBlock().addRange(elements[0], elements[elements.length - 1]);
-    statement = (PsiBlockStatement)parent.addBefore(statement, elements[0]);
-    parent.deleteChildRange(elements[0], elements[elements.length - 1]);
+    statement.getCodeBlock().addRange(fragmentStart, fragmentEnd);
+    statement = (PsiBlockStatement)parent.addBefore(statement, fragmentStart);
+    parent.deleteChildRange(fragmentStart, fragmentEnd);
+
     PsiCodeBlock codeBlock = statement.getCodeBlock();
-    PsiElement[] elementsInCopy = codeBlock.getChildren();
-    LOG.assertTrue(elementsInCopy.length >= elements.length + 2, "wrapper block length is too small");
-    return Arrays.copyOfRange(elementsInCopy, 1, elementsInCopy.length - 1);
+    PsiElement[] elementsInBlock = codeBlock.getChildren();
+    LOG.assertTrue(elementsInBlock.length >= elements.length + 2, "wrapper block length is too small");
+    elementsInBlock = Arrays.copyOfRange(elementsInBlock, 1, elementsInBlock.length - 1);
+
+    declareReusedLocalVariables(reusedLocalVariables, statement, factory);
+    return elementsInBlock;
+  }
+
+  private static void declareReusedLocalVariables(@NotNull List<ReusedLocalVariable> reusedLocalVariables,
+                                                  @NotNull PsiBlockStatement statement,
+                                                  @NotNull PsiElementFactory factory) {
+    PsiElement parent = statement.getParent();
+    PsiCodeBlock codeBlock = statement.getCodeBlock();
+    PsiStatement addAfter = statement;
+    for (ReusedLocalVariable variable : reusedLocalVariables) {
+      if (variable.reuseValue()) {
+        PsiStatement declarationBefore = factory.createStatementFromText(variable.getTempDeclarationText(), codeBlock.getRBrace());
+        parent.addBefore(declarationBefore, statement);
+
+        PsiStatement assignment = factory.createStatementFromText(variable.getAssignmentText(), codeBlock.getRBrace());
+        codeBlock.addBefore(assignment, codeBlock.getRBrace());
+      }
+      PsiStatement declarationAfter = factory.createStatementFromText(variable.getDeclarationText(), statement);
+      parent.addAfter(declarationAfter, addAfter);
+      addAfter = declarationAfter;
+    }
   }
 
   @Nullable

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

@@ -0,0 +1,41 @@
+// Copyright 2000-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file.
+package com.intellij.refactoring.extractMethod;
+
+import com.intellij.psi.PsiKeyword;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * @author Pavel.Dolgov
+ */
+public class ReusedLocalVariable {
+  @NotNull private final String myName;
+  @Nullable private final String myTempName;
+  @NotNull private final String myType;
+  private final boolean myReuseValue;
+
+  public ReusedLocalVariable(@NotNull String name, @Nullable String tempName, @NotNull String type, boolean reuseValue) {
+    assert reuseValue == (tempName != null);
+    myName = name;
+    myTempName = tempName;
+    myType = type;
+    myReuseValue = reuseValue;
+  }
+
+  public String getDeclarationText() {
+    String initText = myReuseValue ? " = " + myTempName : "";
+    return myType + " " + myName + initText + ";";
+  }
+
+  public String getAssignmentText() {
+    return myTempName + " = " + myName + ";";
+  }
+
+  public String getTempDeclarationText() {
+    return myType + " " + myTempName + ";";
+  }
+
+  public boolean reuseValue() {
+    return myReuseValue;
+  }
+}

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

@@ -0,0 +1,125 @@
+// Copyright 2000-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file.
+package com.intellij.refactoring.extractMethod;
+
+import com.intellij.psi.*;
+import com.intellij.psi.codeStyle.JavaCodeStyleManager;
+import com.intellij.psi.controlFlow.*;
+import com.intellij.psi.util.PsiTreeUtil;
+import com.intellij.util.SmartList;
+import com.intellij.util.containers.ContainerUtil;
+import com.intellij.util.text.UniqueNameGenerator;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+
+import java.util.*;
+
+/**
+ * Finds local variables declared inside a code fragment and then used outside of that code fragment
+ *
+ * @author Pavel.Dolgov
+ */
+public class ReusedLocalVariablesFinder {
+  private final ControlFlow myControlFlow;
+  private final PsiStatement myNextStatement;
+  private final int myOffset;
+  private final JavaCodeStyleManager myCodeStyleManager;
+
+  private ReusedLocalVariablesFinder(@NotNull ControlFlow controlFlow, @NotNull PsiStatement nextStatement, int offset) {
+    myControlFlow = controlFlow;
+    myNextStatement = nextStatement;
+    myOffset = offset;
+    myCodeStyleManager = JavaCodeStyleManager.getInstance(myNextStatement.getProject());
+  }
+
+  public static List<ReusedLocalVariable> findReusedLocalVariables(@NotNull PsiElement fragmentStart,
+                                                                   @NotNull PsiElement fragmentEnd,
+                                                                   @NotNull Set<PsiLocalVariable> ignoreVariables) {
+    List<PsiLocalVariable> declaredVariables = getDeclaredVariables(fragmentStart, fragmentEnd, ignoreVariables);
+    if (declaredVariables.isEmpty()) {
+      return Collections.emptyList();
+    }
+
+    ReusedLocalVariablesFinder finder = createFinder(fragmentEnd);
+    if (finder == null) {
+      return Collections.emptyList();
+    }
+
+    List<PsiLocalVariable> reusedVariables = ContainerUtil.filter(declaredVariables, finder::isVariableReused);
+    if (reusedVariables.isEmpty()) {
+      return Collections.emptyList();
+    }
+
+    List<ReusedLocalVariable> result = new ArrayList<>();
+    Set<String> tempNames = new HashSet<>();
+    for (PsiLocalVariable variable : reusedVariables) {
+      String name = variable.getName();
+      if (name == null) {
+        continue;
+      }
+      String typeText = variable.getType().getCanonicalText();
+      if (finder.isValueReused(variable)) {
+        String suggestedName = finder.suggestUniqueVariableName(name);
+        String tempName = UniqueNameGenerator.generateUniqueName(suggestedName, tempNames);
+        tempNames.add(tempName);
+        result.add(new ReusedLocalVariable(name, tempName, typeText, true));
+      }
+      else {
+        result.add(new ReusedLocalVariable(name, null, typeText, false));
+      }
+    }
+    return result;
+  }
+
+  private static List<PsiLocalVariable> getDeclaredVariables(@NotNull PsiElement start,
+                                                             @NotNull PsiElement end,
+                                                             @NotNull Set<PsiLocalVariable> ignoreVariables) {
+    // Only the variables declared at the current code block's level can be reused after the end of the fragment.
+    List<PsiLocalVariable> result = new SmartList<>();
+    for (PsiElement element = start; element != null; element = element != end ? element.getNextSibling() : null) {
+      if (element instanceof PsiDeclarationStatement) {
+        PsiElement[] declaredElements = ((PsiDeclarationStatement)element).getDeclaredElements();
+        for (PsiElement declaredElement : declaredElements) {
+          if (declaredElement instanceof PsiLocalVariable && !ignoreVariables.contains(declaredElement)) {
+            result.add((PsiLocalVariable)declaredElement);
+          }
+        }
+      }
+    }
+    return result;
+  }
+
+  @Nullable
+  private static ReusedLocalVariablesFinder createFinder(@NotNull PsiElement fragmentEnd) {
+    PsiStatement nextStatement = PsiTreeUtil.getNextSiblingOfType(fragmentEnd, PsiStatement.class);
+    if (nextStatement == null) {
+      return null;
+    }
+
+    PsiElement codeFragment = ControlFlowUtil.findCodeFragment(nextStatement);
+    ControlFlow controlFlow;
+    try {
+      controlFlow = ControlFlowFactory.getInstance(codeFragment.getProject()).getControlFlow(
+        codeFragment, new LocalsControlFlowPolicy(codeFragment), false, false);
+    }
+    catch (AnalysisCanceledException e) {
+      return null;
+    }
+    int offset = controlFlow.getStartOffset(nextStatement);
+    if (offset < 0) {
+      return null;
+    }
+    return new ReusedLocalVariablesFinder(controlFlow, nextStatement, offset);
+  }
+
+  private boolean isVariableReused(@NotNull PsiVariable variable) {
+    return ControlFlowUtil.isVariableUsed(myControlFlow, myOffset, myControlFlow.getSize(), variable);
+  }
+
+  private boolean isValueReused(@NotNull PsiVariable variable) {
+    return ControlFlowUtil.needVariableValueAt(variable, myControlFlow, myOffset);
+  }
+
+  private String suggestUniqueVariableName(String name) {
+    return myCodeStyleManager.suggestUniqueVariableName(name, myNextStatement, true);
+  }
+}

java/java-psi-impl/src/com/intellij/psi/controlFlow/ControlFlowFactory.java

@@ -120,6 +120,9 @@ public class ControlFlowFactory {
                                     @NotNull ControlFlowPolicy policy,
                                     boolean enableShortCircuit,
                                     boolean evaluateConstantIfCondition) throws AnalysisCanceledException {
+    if (!element.isPhysical()) {
+      return new ControlFlowAnalyzer(element, policy, enableShortCircuit, evaluateConstantIfCondition).buildControlFlow();
+    }
     final long modificationCount = element.getManager().getModificationTracker().getModificationCount();
     ConcurrentList<ControlFlowContext> cached = getOrCreateCachedFlowsForElement(element);
     for (ControlFlowContext context : cached) {

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

@@ -0,0 +1,19 @@
+import java.util.List;
+
+class DeclaredOutputVariable {
+    void foo(List<String> a) {
+        <selection>
+        String s = a.get(1);
+        if (s == null) return;
+        System.out.println(s.charAt(1));
+        </selection>
+        System.out.println(s.length());
+    }
+
+    void bar(List<String> a) {
+        String s = a.get(2);
+        if (s == null) return;
+        System.out.println(s.charAt(2));
+        System.out.println(s.length());
+    }
+}

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

@@ -0,0 +1,27 @@
+import org.jetbrains.annotations.Nullable;
+
+import java.util.List;
+
+class DeclaredOutputVariable {
+    void foo(List<String> a) {
+
+        String s = newMethod(a, 1);
+        if (s == null) return;
+
+        System.out.println(s.length());
+    }
+
+    @Nullable
+    private String newMethod(List<String> a, int i) {
+        String s = a.get(i);
+        if (s == null) return null;
+        System.out.println(s.charAt(i));
+        return s;
+    }
+
+    void bar(List<String> a) {
+        String s = newMethod(a, 2);
+        if (s == null) return;
+        System.out.println(s.length());
+    }
+}

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

@@ -873,6 +873,10 @@ public class ExtractMethodTest extends LightCodeInsightTestCase {
     doDuplicatesTest();
   }
 
+  public void testParametrizedDuplicateDeclaredOutputVariable() 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");