From e4b477fc6e025f161bb1c0617a40aebbcc9fa1a2 Mon Sep 17 00:00:00 2001 From: Pavel Dolgov Date: Wed, 28 Mar 2018 12:23:37 +0300 Subject: [PATCH] Java: Fixed extracting method from duplicates containing reused variables (IDEA-188894) --- .../extractMethod/ParametrizedDuplicates.java | 50 +++++-- .../extractMethod/ReusedLocalVariable.java | 41 ++++++ .../ReusedLocalVariablesFinder.java | 125 ++++++++++++++++++ .../psi/controlFlow/ControlFlowFactory.java | 3 + ...trizedDuplicateDeclaredOutputVariable.java | 19 +++ ...DuplicateDeclaredOutputVariable_after.java | 27 ++++ .../java/refactoring/ExtractMethodTest.java | 4 + 7 files changed, 257 insertions(+), 12 deletions(-) create mode 100644 java/java-impl/src/com/intellij/refactoring/extractMethod/ReusedLocalVariable.java create mode 100644 java/java-impl/src/com/intellij/refactoring/extractMethod/ReusedLocalVariablesFinder.java create mode 100644 java/java-tests/testData/refactoring/extractMethod/ParametrizedDuplicateDeclaredOutputVariable.java create mode 100644 java/java-tests/testData/refactoring/extractMethod/ParametrizedDuplicateDeclaredOutputVariable_after.java diff --git a/java/java-impl/src/com/intellij/refactoring/extractMethod/ParametrizedDuplicates.java b/java/java-impl/src/com/intellij/refactoring/extractMethod/ParametrizedDuplicates.java index 8ce69a54f6b3..809d04a61c04 100644 --- a/java/java-impl/src/com/intellij/refactoring/extractMethod/ParametrizedDuplicates.java +++ b/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 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 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 diff --git a/java/java-impl/src/com/intellij/refactoring/extractMethod/ReusedLocalVariable.java b/java/java-impl/src/com/intellij/refactoring/extractMethod/ReusedLocalVariable.java new file mode 100644 index 000000000000..35a0f8d35cd7 --- /dev/null +++ b/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; + } +} diff --git a/java/java-impl/src/com/intellij/refactoring/extractMethod/ReusedLocalVariablesFinder.java b/java/java-impl/src/com/intellij/refactoring/extractMethod/ReusedLocalVariablesFinder.java new file mode 100644 index 000000000000..76ea63b5438a --- /dev/null +++ b/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 findReusedLocalVariables(@NotNull PsiElement fragmentStart, + @NotNull PsiElement fragmentEnd, + @NotNull Set ignoreVariables) { + List declaredVariables = getDeclaredVariables(fragmentStart, fragmentEnd, ignoreVariables); + if (declaredVariables.isEmpty()) { + return Collections.emptyList(); + } + + ReusedLocalVariablesFinder finder = createFinder(fragmentEnd); + if (finder == null) { + return Collections.emptyList(); + } + + List reusedVariables = ContainerUtil.filter(declaredVariables, finder::isVariableReused); + if (reusedVariables.isEmpty()) { + return Collections.emptyList(); + } + + List result = new ArrayList<>(); + Set 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 getDeclaredVariables(@NotNull PsiElement start, + @NotNull PsiElement end, + @NotNull Set ignoreVariables) { + // Only the variables declared at the current code block's level can be reused after the end of the fragment. + List 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); + } +} diff --git a/java/java-psi-impl/src/com/intellij/psi/controlFlow/ControlFlowFactory.java b/java/java-psi-impl/src/com/intellij/psi/controlFlow/ControlFlowFactory.java index 32615f15d74d..4dbe3b50f762 100644 --- a/java/java-psi-impl/src/com/intellij/psi/controlFlow/ControlFlowFactory.java +++ b/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 cached = getOrCreateCachedFlowsForElement(element); for (ControlFlowContext context : cached) { diff --git a/java/java-tests/testData/refactoring/extractMethod/ParametrizedDuplicateDeclaredOutputVariable.java b/java/java-tests/testData/refactoring/extractMethod/ParametrizedDuplicateDeclaredOutputVariable.java new file mode 100644 index 000000000000..926aa7658136 --- /dev/null +++ b/java/java-tests/testData/refactoring/extractMethod/ParametrizedDuplicateDeclaredOutputVariable.java @@ -0,0 +1,19 @@ +import java.util.List; + +class DeclaredOutputVariable { + void foo(List a) { + + String s = a.get(1); + if (s == null) return; + System.out.println(s.charAt(1)); + + System.out.println(s.length()); + } + + void bar(List a) { + String s = a.get(2); + if (s == null) return; + System.out.println(s.charAt(2)); + System.out.println(s.length()); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/extractMethod/ParametrizedDuplicateDeclaredOutputVariable_after.java b/java/java-tests/testData/refactoring/extractMethod/ParametrizedDuplicateDeclaredOutputVariable_after.java new file mode 100644 index 000000000000..8e7e3d9ab6f8 --- /dev/null +++ b/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 a) { + + String s = newMethod(a, 1); + if (s == null) return; + + System.out.println(s.length()); + } + + @Nullable + private String newMethod(List a, int i) { + String s = a.get(i); + if (s == null) return null; + System.out.println(s.charAt(i)); + return s; + } + + void bar(List a) { + String s = newMethod(a, 2); + if (s == null) return; + System.out.println(s.length()); + } +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/java/refactoring/ExtractMethodTest.java b/java/java-tests/testSrc/com/intellij/java/refactoring/ExtractMethodTest.java index 5ee1971e092a..e43abcf24d6c 100644 --- a/java/java-tests/testSrc/com/intellij/java/refactoring/ExtractMethodTest.java +++ b/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");