From e1a6a54c652b9558bcc8b74a865422ecb49fa9c5 Mon Sep 17 00:00:00 2001 From: Pavel Dolgov Date: Fri, 6 Oct 2017 17:28:24 +0300 Subject: [PATCH] Java: Fixed handling of complex constant expressions in parametrized duplicates matching (IDEA-179924) --- .../util/duplicates/DuplicatesFinder.java | 9 +++++++- .../duplicates/ExtractableExpressionPart.java | 18 ++++++++-------- ...tChangeSignatureEqualConstExprFolding.java | 16 ++++++++++++++ ...eSignatureEqualConstExprFolding_after.java | 19 +++++++++++++++++ ...stChangeSignatureLongConstExprFolding.java | 18 ++++++++++++++++ ...geSignatureLongConstExprFolding_after.java | 21 +++++++++++++++++++ .../java/refactoring/ExtractMethodTest.java | 8 +++++++ 7 files changed, 99 insertions(+), 10 deletions(-) create mode 100644 java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureEqualConstExprFolding.java create mode 100644 java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureEqualConstExprFolding_after.java create mode 100644 java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureLongConstExprFolding.java create mode 100644 java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureLongConstExprFolding_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 22bdfdccee3a..449e1f4e754c 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 @@ -313,7 +313,14 @@ public class DuplicatesFinder { final ASTNode node1 = pattern.getNode(); final ASTNode node2 = candidate.getNode(); if (node1 == null || node2 == null) return false; - return node1.getElementType() == node2.getElementType(); + if (node1.getElementType() != node2.getElementType()) return false; + if (pattern instanceof PsiUnaryExpression) { + return ((PsiUnaryExpression)pattern).getOperationTokenType() == ((PsiUnaryExpression)candidate).getOperationTokenType(); + } + if (pattern instanceof PsiPolyadicExpression) { + return ((PsiPolyadicExpression)pattern).getOperationTokenType() == ((PsiPolyadicExpression)candidate).getOperationTokenType(); + } + return true; } private boolean matchPattern(PsiElement pattern, diff --git a/java/java-analysis-impl/src/com/intellij/refactoring/util/duplicates/ExtractableExpressionPart.java b/java/java-analysis-impl/src/com/intellij/refactoring/util/duplicates/ExtractableExpressionPart.java index ed2dfe3d2438..957283d95f9a 100644 --- a/java/java-analysis-impl/src/com/intellij/refactoring/util/duplicates/ExtractableExpressionPart.java +++ b/java/java-analysis-impl/src/com/intellij/refactoring/util/duplicates/ExtractableExpressionPart.java @@ -72,18 +72,18 @@ public class ExtractableExpressionPart { static ExtractableExpressionPart match(@NotNull PsiExpression expression, @NotNull List scope, @Nullable ComplexityHolder complexityHolder) { - if (PsiUtil.isConstantExpression(expression)) { - if (PsiTreeUtil.findChildOfType(expression, PsiJavaCodeReferenceElement.class) != null) { - // Avoid coincidental replacement of equal expressions containing different constant fields - // E.g. don't count as equal (Foo.A + 1) and (Bar.B - 2) because most likely it's not what the users expect - return null; - } - return matchConstant(expression); - } if (expression instanceof PsiReferenceExpression) { return matchVariable((PsiReferenceExpression)expression, scope); } - if (complexityHolder != null && complexityHolder.isAcceptableExpression(expression)) { + boolean isConstant = PsiUtil.isConstantExpression(expression); + if (isConstant) { + // Avoid replacement of coincidentally equal expressions containing different constant fields + // E.g. don't count as equal values expressions like (Foo.A + 1) and (Bar.B - 2) + if (PsiTreeUtil.findChildOfType(expression, PsiJavaCodeReferenceElement.class) == null) { + return matchConstant(expression); + } + } + if (complexityHolder != null && (isConstant || complexityHolder.isAcceptableExpression(expression))) { PsiType type = expression.getType(); if (type != null && !PsiType.VOID.equals(type)) { return new ExtractableExpressionPart(expression, null, null, type); diff --git a/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureEqualConstExprFolding.java b/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureEqualConstExprFolding.java new file mode 100644 index 000000000000..9e3586b51a9f --- /dev/null +++ b/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureEqualConstExprFolding.java @@ -0,0 +1,16 @@ +class C { + public static final int A = 4 - 1; + public static final int B = 4 + 2; + + void foo(int x) { + + if(x == A + 1) + bar(A + 1); + + + if(x == B - 2) + bar(B - 2); + } + + void bar(int n) {} +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureEqualConstExprFolding_after.java b/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureEqualConstExprFolding_after.java new file mode 100644 index 000000000000..b6a068f68dcc --- /dev/null +++ b/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureEqualConstExprFolding_after.java @@ -0,0 +1,19 @@ +class C { + public static final int A = 4 - 1; + public static final int B = 4 + 2; + + void foo(int x) { + + newMethod(x, A + 1); + + + newMethod(x, B - 2); + } + + private void newMethod(int x, int i) { + if (x == i) + bar(i); + } + + void bar(int n) {} +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureLongConstExprFolding.java b/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureLongConstExprFolding.java new file mode 100644 index 000000000000..1786233ab6e7 --- /dev/null +++ b/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureLongConstExprFolding.java @@ -0,0 +1,18 @@ +class C { + public static final int A = 1, B = 2, C = 3, D = 4, E = 5; + + void foo(int x) { + + if (x >= A + B + C + D + E + 1) + bar("A" + "B" + "C" + "D" + "E" + 1); + + + if (x >= A + B + C + D + E + 2) + bar("A" + "B" + "C" + "D" + "E" + 2); + + if (x <= A + B + C + D + E + 2) + bar("A" + "B" + "C" + "D" + "E" + 2); + } + + void bar(String s) {} +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureLongConstExprFolding_after.java b/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureLongConstExprFolding_after.java new file mode 100644 index 000000000000..74c86954b034 --- /dev/null +++ b/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureLongConstExprFolding_after.java @@ -0,0 +1,21 @@ +class C { + public static final int A = 1, B = 2, C = 3, D = 4, E = 5; + + void foo(int x) { + + newMethod(x, 1, "A" + "B" + "C" + "D" + "E" + 1); + + + newMethod(x, 2, "A" + "B" + "C" + "D" + "E" + 2); + + if (x <= A + B + C + D + E + 2) + bar("A" + "B" + "C" + "D" + "E" + 2); + } + + private void newMethod(int x, int i, String s) { + if (x >= A + B + C + D + E + i) + bar(s); + } + + void bar(String s) {} +} \ 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 9ff2953bac29..d7224c42aaed 100644 --- a/java/java-tests/testSrc/com/intellij/java/refactoring/ExtractMethodTest.java +++ b/java/java-tests/testSrc/com/intellij/java/refactoring/ExtractMethodTest.java @@ -815,6 +815,14 @@ public class ExtractMethodTest extends LightCodeInsightTestCase { doDuplicatesTest(); } + public void testSuggestChangeSignatureEqualConstExprFolding() throws Exception { + doDuplicatesTest(); + } + + public void testSuggestChangeSignatureLongConstExprFolding() 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");