From ab72ea0a3fff985d75d80c775d29884df3225847 Mon Sep 17 00:00:00 2001 From: Pavel Dolgov Date: Mon, 2 Jul 2018 17:22:10 +0300 Subject: [PATCH] Java: Even if changing the signature is not accepted by the user, it's OK to replace exact duplicates (IDEA-194709) --- .../util/duplicates/DuplicatesFinder.java | 5 +-- .../extractMethod/ExtractMethodProcessor.java | 32 +++++++++++++++---- .../extractMethod/ParametrizedDuplicates.java | 9 ++++-- .../ParametrizedDuplicateKeepSignature.java | 7 ++++ ...ametrizedDuplicateKeepSignature_after.java | 11 +++++++ .../java/refactoring/ExtractMethodTest.java | 10 ++++++ 6 files changed, 63 insertions(+), 11 deletions(-) create mode 100644 java/java-tests/testData/refactoring/extractMethod/ParametrizedDuplicateKeepSignature.java create mode 100644 java/java-tests/testData/refactoring/extractMethod/ParametrizedDuplicateKeepSignature_after.java diff --git a/java/java-analysis-impl/src/com/intellij/refactoring/util/duplicates/DuplicatesFinder.java b/java/java-analysis-impl/src/com/intellij/refactoring/util/duplicates/DuplicatesFinder.java index ed8c30628261..9b3903b251a7 100644 --- a/java/java-analysis-impl/src/com/intellij/refactoring/util/duplicates/DuplicatesFinder.java +++ b/java/java-analysis-impl/src/com/intellij/refactoring/util/duplicates/DuplicatesFinder.java @@ -63,7 +63,6 @@ public class DuplicatesFinder { LOG.assertTrue(pattern.length > 0); myPattern = pattern; myPatternAsList = Arrays.asList(myPattern); - myParameters = parameters; myOutputParameters = outputParameters; myMatchType = matchType; myEffectivelyLocal = effectivelyLocal != null ? effectivelyLocal : Collections.emptySet(); @@ -90,11 +89,13 @@ public class DuplicatesFinder { myMultipleExitPoints = exitPoints.size() > 1; if (myMultipleExitPoints) { - myParameters.removeParametersUsedInExitsOnly(codeFragment, exitStatements, controlFlow, startOffset, endOffset); + parameters = parameters.copy(); + parameters.removeParametersUsedInExitsOnly(codeFragment, exitStatements, controlFlow, startOffset, endOffset); } } catch (AnalysisCanceledException ignored) { } + myParameters = parameters; } public DuplicatesFinder(@NotNull PsiElement[] pattern, diff --git a/java/java-impl/src/com/intellij/refactoring/extractMethod/ExtractMethodProcessor.java b/java/java-impl/src/com/intellij/refactoring/extractMethod/ExtractMethodProcessor.java index c33eb9d5e057..fe70a5762a20 100644 --- a/java/java-impl/src/com/intellij/refactoring/extractMethod/ExtractMethodProcessor.java +++ b/java/java-impl/src/com/intellij/refactoring/extractMethod/ExtractMethodProcessor.java @@ -66,9 +66,11 @@ import org.jetbrains.annotations.*; import java.util.*; import static com.intellij.codeInsight.AnnotationUtil.CHECK_TYPE; +import static com.intellij.refactoring.util.duplicates.DuplicatesFinder.MatchType; public class ExtractMethodProcessor implements MatchProvider { private static final Logger LOG = Logger.getInstance("#com.intellij.refactoring.extractMethod.ExtractMethodProcessor"); + public static final Key SIGNATURE_CHANGE_ALLOWED = Key.create("SignatureChangeAllowed"); protected final Project myProject; private final Editor myEditor; @@ -117,6 +119,7 @@ public class ExtractMethodProcessor implements MatchProvider { protected boolean myIsChainedConstructor; protected List myDuplicates; private ParametrizedDuplicates myParametrizedDuplicates; + private ParametrizedDuplicates myExactDuplicates; @PsiModifier.ModifierConstant protected String myMethodVisibility = PsiModifier.PRIVATE; protected boolean myGenerateConditionalExit; protected PsiStatement myFirstExitStatementCopy; @@ -891,7 +894,10 @@ public class ExtractMethodProcessor implements MatchProvider { } protected void initDuplicates() { - myParametrizedDuplicates = ParametrizedDuplicates.findDuplicates(this); + myParametrizedDuplicates = ParametrizedDuplicates.findDuplicates(this, MatchType.PARAMETRIZED); + if (myParametrizedDuplicates != null && !myParametrizedDuplicates.isEmpty()) { + myExactDuplicates = ParametrizedDuplicates.findDuplicates(this, MatchType.EXACT); + } myDuplicates = new ArrayList<>(); } @@ -914,7 +920,7 @@ public class ExtractMethodProcessor implements MatchProvider { DuplicatesFinder finder = new DuplicatesFinder(elements, myInputVariables.copy(), value, parameters); List myDuplicates = finder.findDuplicates(myTargetClass); if (!ContainerUtil.isEmpty(myDuplicates)) return myDuplicates.size(); - ParametrizedDuplicates parametrizedDuplicates = ParametrizedDuplicates.findDuplicates(this); + ParametrizedDuplicates parametrizedDuplicates = ParametrizedDuplicates.findDuplicates(this, MatchType.PARAMETRIZED); if (parametrizedDuplicates != null) { List duplicates = parametrizedDuplicates.getDuplicates(); return duplicates != null ? duplicates.size() : 0; @@ -2164,10 +2170,7 @@ public class ExtractMethodProcessor implements MatchProvider { .allMatch(List::isEmpty); boolean isFoldable = myInputVariables.isFoldable(); if (!showDialog || isSignatureUnchanged || isFoldable || - ApplicationManager.getApplication().isUnitTestMode() || - new SignatureSuggesterPreviewDialog(myExtractedMethod, myParametrizedDuplicates.getParametrizedMethod(), - myMethodCall, myParametrizedDuplicates.getParametrizedCall(), - myParametrizedDuplicates.getSize()).showAndGet()) { + shouldChangeSignature()) { myDuplicates = myParametrizedDuplicates.getDuplicates(); if (myExtractedMethod.isPhysical()) { @@ -2183,7 +2186,22 @@ public class ExtractMethodProcessor implements MatchProvider { return true; } } - return false; + if (myExactDuplicates != null) { + myDuplicates = myExactDuplicates.getDuplicates(); + } + return !myDuplicates.isEmpty(); + } + + private boolean shouldChangeSignature() { + if (ApplicationManager.getApplication().isUnitTestMode()) { + return Optional.of(myExtractedMethod) + .map(PsiElement::getContainingFile) + .map(psiFile -> psiFile.getUserData(SIGNATURE_CHANGE_ALLOWED)) + .orElse(true); + } + return new SignatureSuggesterPreviewDialog(myExtractedMethod, myParametrizedDuplicates.getParametrizedMethod(), + myMethodCall, myParametrizedDuplicates.getParametrizedCall(), + myParametrizedDuplicates.getSize()).showAndGet(); } private void replaceParametrizedMethod() { 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 c2deba3bf98e..153257c7b31d 100644 --- a/java/java-impl/src/com/intellij/refactoring/extractMethod/ParametrizedDuplicates.java +++ b/java/java-impl/src/com/intellij/refactoring/extractMethod/ParametrizedDuplicates.java @@ -79,8 +79,9 @@ public class ParametrizedDuplicates { } @Nullable - public static ParametrizedDuplicates findDuplicates(@NotNull ExtractMethodProcessor originalProcessor) { - DuplicatesFinder finder = createDuplicatesFinder(originalProcessor, DuplicatesFinder.MatchType.PARAMETRIZED); + public static ParametrizedDuplicates findDuplicates(@NotNull ExtractMethodProcessor originalProcessor, + @NotNull DuplicatesFinder.MatchType matchType) { + DuplicatesFinder finder = createDuplicatesFinder(originalProcessor, matchType); if (finder == null) { return null; } @@ -414,6 +415,10 @@ public class ParametrizedDuplicates { return myMatches; } + boolean isEmpty() { + return ContainerUtil.isEmpty(myMatches); + } + @NotNull private static PsiElement[] wrapWithCodeBlock(@NotNull PsiElement[] elements, @NotNull InputVariables inputVariables) { PsiElement fragmentStart = elements[0]; diff --git a/java/java-tests/testData/refactoring/extractMethod/ParametrizedDuplicateKeepSignature.java b/java/java-tests/testData/refactoring/extractMethod/ParametrizedDuplicateKeepSignature.java new file mode 100644 index 000000000000..8cb7bb03c7e8 --- /dev/null +++ b/java/java-tests/testData/refactoring/extractMethod/ParametrizedDuplicateKeepSignature.java @@ -0,0 +1,7 @@ +class KeepSignature { + void hello() { + System.out.println("Foo"); + System.out.println("Bar"); + System.out.println("Foo"); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/extractMethod/ParametrizedDuplicateKeepSignature_after.java b/java/java-tests/testData/refactoring/extractMethod/ParametrizedDuplicateKeepSignature_after.java new file mode 100644 index 000000000000..1a34af5fff42 --- /dev/null +++ b/java/java-tests/testData/refactoring/extractMethod/ParametrizedDuplicateKeepSignature_after.java @@ -0,0 +1,11 @@ +class KeepSignature { + void hello() { + newMethod(); + System.out.println("Bar"); + newMethod(); + } + + private void newMethod() { + System.out.println("Foo"); + } +} \ 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 db7327ea052e..e23854f8944d 100644 --- a/java/java-tests/testSrc/com/intellij/java/refactoring/ExtractMethodTest.java +++ b/java/java-tests/testSrc/com/intellij/java/refactoring/ExtractMethodTest.java @@ -42,6 +42,7 @@ import com.intellij.util.ArrayUtil; import com.intellij.util.IncorrectOperationException; import org.jetbrains.annotations.NonNls; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; import java.util.List; @@ -921,6 +922,10 @@ public class ExtractMethodTest extends LightCodeInsightTestCase { doDuplicatesTest(); } + public void testParametrizedDuplicateKeepSignature() throws Exception { + doTest(true, () -> getFile().putUserData(ExtractMethodProcessor.SIGNATURE_CHANGE_ALLOWED, Boolean.FALSE)); + } + public void testSuggestChangeSignatureWithChangedParameterName() throws Exception { configureByFile(BASE_PATH + getTestName(false) + ".java"); boolean success = performExtractMethod(true, true, getEditor(), getFile(), getProject(), false, null, false, "p"); @@ -1354,7 +1359,12 @@ public class ExtractMethodTest extends LightCodeInsightTestCase { } private void doTest(boolean duplicates) throws Exception { + doTest(duplicates, null); + } + + private void doTest(boolean duplicates, @Nullable Runnable prepare) throws Exception { configureByFile(BASE_PATH + getTestName(false) + ".java"); + if (prepare != null) prepare.run(); boolean success = performAction(true, duplicates); assertTrue(success); checkResultByFile(BASE_PATH + getTestName(false) + "_after.java");