From 256050bb385e9c5ef6be37b32629c28d32c7f982 Mon Sep 17 00:00:00 2001 From: Pavel Dolgov Date: Wed, 19 Sep 2018 14:20:04 +0300 Subject: [PATCH] Java: Tests for fixes for duplicate expressions inspection (IDEA-136761) --- .../ComplexityCalculator.java | 33 ++ .../DuplicateExpressionsContext.java | 8 +- .../DuplicateExpressionsInspection.java | 288 +++++++++++++++++- ...ideEffectExpressionEquivalenceChecker.java | 26 +- .../SideEffectCalculator.java | 9 +- .../CompositeQualifier.java | 8 + .../MethodCallWithSideEffect.java | 11 + .../VariableModified.java | 17 ++ .../VariableNotModified.java | 16 + .../IntroduceVariable.java | 15 + ...roduceVariableOtherVariableNotInScope.java | 17 ++ ...VariableOtherVariableNotInScope_after.java | 18 ++ .../IntroduceVariable_after.java | 16 + .../ReplaceOthers.java | 16 + .../ReplaceOthers_after.java | 16 + .../ReuseVariable.java | 16 + .../ReuseVariable_after.java | 16 + .../VariableNotInScopeCantReplaceOthers.java | 18 ++ .../DuplicateExpressionsFixTest.kt | 62 ++++ .../DuplicateExpressionsTest.kt | 8 +- .../src/messages/InspectionsBundle.properties | 8 +- 21 files changed, 596 insertions(+), 46 deletions(-) create mode 100644 java/java-tests/testData/inspection/duplicateExpressions/CompositeQualifier.java create mode 100644 java/java-tests/testData/inspection/duplicateExpressions/MethodCallWithSideEffect.java create mode 100644 java/java-tests/testData/inspection/duplicateExpressions/VariableModified.java create mode 100644 java/java-tests/testData/inspection/duplicateExpressions/VariableNotModified.java create mode 100644 java/java-tests/testData/inspection/duplicateExpressionsFix/IntroduceVariable.java create mode 100644 java/java-tests/testData/inspection/duplicateExpressionsFix/IntroduceVariableOtherVariableNotInScope.java create mode 100644 java/java-tests/testData/inspection/duplicateExpressionsFix/IntroduceVariableOtherVariableNotInScope_after.java create mode 100644 java/java-tests/testData/inspection/duplicateExpressionsFix/IntroduceVariable_after.java create mode 100644 java/java-tests/testData/inspection/duplicateExpressionsFix/ReplaceOthers.java create mode 100644 java/java-tests/testData/inspection/duplicateExpressionsFix/ReplaceOthers_after.java create mode 100644 java/java-tests/testData/inspection/duplicateExpressionsFix/ReuseVariable.java create mode 100644 java/java-tests/testData/inspection/duplicateExpressionsFix/ReuseVariable_after.java create mode 100644 java/java-tests/testData/inspection/duplicateExpressionsFix/VariableNotInScopeCantReplaceOthers.java create mode 100644 java/java-tests/testSrc/com/intellij/java/codeInspection/DuplicateExpressionsFixTest.kt diff --git a/java/java-impl/src/com/intellij/codeInspection/duplicateExpressions/ComplexityCalculator.java b/java/java-impl/src/com/intellij/codeInspection/duplicateExpressions/ComplexityCalculator.java index 0fe05e54d5f1..6c6b767a6d6c 100644 --- a/java/java-impl/src/com/intellij/codeInspection/duplicateExpressions/ComplexityCalculator.java +++ b/java/java-impl/src/com/intellij/codeInspection/duplicateExpressions/ComplexityCalculator.java @@ -2,7 +2,9 @@ package com.intellij.codeInspection.duplicateExpressions; import com.intellij.psi.*; +import com.intellij.psi.tree.IElementType; import com.intellij.psi.tree.TokenSet; +import com.intellij.psi.util.PsiUtil; import com.intellij.util.containers.ObjectIntHashMap; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -69,6 +71,19 @@ class ComplexityCalculator { int c = NEGATIONS.contains(unary.getOperationTokenType()) ? SIMPLE_OPERATOR : OPERATOR; return getComplexity(unary.getOperand()) + c; } + if (e instanceof PsiBinaryExpression) { + PsiBinaryExpression binary = (PsiBinaryExpression)e; + IElementType token = binary.getOperationTokenType(); + PsiExpression left = binary.getLOperand(); + PsiExpression right = binary.getROperand(); + int c = BOOLEAN_OPERATION_TOKENS.contains(token) || + JavaTokenType.PLUS.equals(token) && (isLiteral(left, "1") || isLiteral(right, "1")) || + JavaTokenType.MINUS.equals(token) && isLiteral(right, "1") || + JavaTokenType.ASTERISK.equals(token) && (isLiteral(left, "2") || isLiteral(right, "2")) || + JavaTokenType.DIV.equals(token) && isLiteral(right, "2") + ? SIMPLE_OPERATOR : OPERATOR; + return c + getComplexity(left) + getComplexity(right); + } if (e instanceof PsiPolyadicExpression) { PsiPolyadicExpression polyadic = (PsiPolyadicExpression)e; int c = BOOLEAN_OPERATION_TOKENS.contains(polyadic.getOperationTokenType()) ? SIMPLE_OPERATOR : OPERATOR; @@ -148,4 +163,22 @@ class ComplexityCalculator { } return c; } + + private static boolean isLiteral(@Nullable PsiExpression expression, @NotNull String withText) { + expression = PsiUtil.skipParenthesizedExprDown(expression); + return expression instanceof PsiLiteral && expression.textMatches(withText); + } + + /** + * Quick check to filter out the obvious things early + */ + static boolean isDefinitelySimple(@Nullable PsiExpression expression, int threshold) { + if (expression instanceof PsiLiteral) { + return CONSTANT < threshold; + } + if (expression instanceof PsiReferenceExpression && ((PsiReferenceExpression)expression).getQualifierExpression() == null) { + return REFERENCE < threshold; + } + return false; + } } diff --git a/java/java-impl/src/com/intellij/codeInspection/duplicateExpressions/DuplicateExpressionsContext.java b/java/java-impl/src/com/intellij/codeInspection/duplicateExpressions/DuplicateExpressionsContext.java index 8f51c18bac42..e8389aeaafb6 100644 --- a/java/java-impl/src/com/intellij/codeInspection/duplicateExpressions/DuplicateExpressionsContext.java +++ b/java/java-impl/src/com/intellij/codeInspection/duplicateExpressions/DuplicateExpressionsContext.java @@ -44,8 +44,8 @@ class DuplicateExpressionsContext { } @Nullable - static DuplicateExpressionsContext getOrCreateContext(PsiExpression expression, @NotNull UserDataHolder session) { - PsiCodeBlock nearestBody = ObjectUtils.tryCast(ControlFlowUtil.findCodeFragment(expression), PsiCodeBlock.class); + static DuplicateExpressionsContext getOrCreateContext(@NotNull PsiExpression expression, @NotNull UserDataHolder session) { + PsiCodeBlock nearestBody = findNearestBody(expression); if (nearestBody != null) { Map contexts = session.getUserData(CONTEXTS_KEY); if (contexts == null) { @@ -61,4 +61,8 @@ class DuplicateExpressionsContext { Map contexts = session.getUserData(CONTEXTS_KEY); return contexts != null ? contexts.get(body) : null; } + + static PsiCodeBlock findNearestBody(@NotNull PsiExpression expression) { + return ObjectUtils.tryCast(ControlFlowUtil.findCodeFragment(expression), PsiCodeBlock.class); + } } diff --git a/java/java-impl/src/com/intellij/codeInspection/duplicateExpressions/DuplicateExpressionsInspection.java b/java/java-impl/src/com/intellij/codeInspection/duplicateExpressions/DuplicateExpressionsInspection.java index 96736a3cb694..8a1e1fe25bda 100644 --- a/java/java-impl/src/com/intellij/codeInspection/duplicateExpressions/DuplicateExpressionsInspection.java +++ b/java/java-impl/src/com/intellij/codeInspection/duplicateExpressions/DuplicateExpressionsInspection.java @@ -1,25 +1,35 @@ // 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.codeInspection.duplicateExpressions; -import com.intellij.codeInspection.InspectionsBundle; -import com.intellij.codeInspection.LocalInspectionTool; -import com.intellij.codeInspection.LocalInspectionToolSession; -import com.intellij.codeInspection.ProblemsHolder; +import com.intellij.codeInspection.*; import com.intellij.codeInspection.ui.SingleIntegerFieldOptionsPanel; +import com.intellij.openapi.application.ApplicationManager; +import com.intellij.openapi.editor.Editor; +import com.intellij.openapi.fileEditor.FileEditorManager; +import com.intellij.openapi.fileEditor.OpenFileDescriptor; import com.intellij.openapi.project.Project; import com.intellij.openapi.util.Ref; import com.intellij.openapi.util.UserDataHolder; +import com.intellij.openapi.vfs.VirtualFile; import com.intellij.psi.*; import com.intellij.psi.controlFlow.*; import com.intellij.psi.util.PsiTreeUtil; +import com.intellij.psi.util.PsiUtil; +import com.intellij.refactoring.introduceVariable.InputValidator; +import com.intellij.refactoring.introduceVariable.IntroduceVariableHandler; +import com.intellij.refactoring.introduceVariable.IntroduceVariableSettings; +import com.intellij.refactoring.ui.TypeSelectorManagerImpl; import com.intellij.refactoring.util.RefactoringUtil; +import com.intellij.util.IncorrectOperationException; +import com.siyeh.ig.psiutils.CommentTracker; +import gnu.trove.THashMap; import gnu.trove.THashSet; +import org.jetbrains.annotations.Nls; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import javax.swing.*; -import java.util.List; -import java.util.Set; +import java.util.*; /** * @author Pavel.Dolgov @@ -40,10 +50,32 @@ public class DuplicateExpressionsInspection extends LocalInspectionTool { if (expression instanceof PsiParenthesizedExpression) { return; } + visitExpressionImpl(expression); + } + + @Override + public void visitReferenceExpression(PsiReferenceExpression expression) { + super.visitReferenceExpression(expression); + + if (expression.getParent() instanceof PsiCallExpression) { + return; + } + visitExpressionImpl(expression); + } + + public void visitExpressionImpl(PsiExpression expression) { + if (ComplexityCalculator.isDefinitelySimple(expression, complexityThreshold) || + SideEffectCalculator.isDefinitelyWithSideEffect(expression)) { + return; + } DuplicateExpressionsContext context = DuplicateExpressionsContext.getOrCreateContext(expression, session); if (context == null || context.mayHaveSideEffect(expression)) { return; } + PsiType type = RefactoringUtil.getTypeByExpressionWithExpectedType(expression); + if (type == null || PsiType.VOID.equals(type)) { + return; + } if (context.getComplexity(expression) > complexityThreshold) { context.addOccurrence(expression); } @@ -55,7 +87,7 @@ public class DuplicateExpressionsInspection extends LocalInspectionTool { PsiCodeBlock body = method.getBody(); if (body != null) { - filterExpressions(body, session); + registerProblemsForExpressions(body, session); } } @@ -63,7 +95,7 @@ public class DuplicateExpressionsInspection extends LocalInspectionTool { public void visitClassInitializer(PsiClassInitializer initializer) { super.visitClassInitializer(initializer); - filterExpressions(initializer.getBody(), session); + registerProblemsForExpressions(initializer.getBody(), session); } @Override @@ -72,11 +104,11 @@ public class DuplicateExpressionsInspection extends LocalInspectionTool { PsiElement body = expression.getBody(); if (body instanceof PsiCodeBlock) { - filterExpressions((PsiCodeBlock)body, session); + registerProblemsForExpressions((PsiCodeBlock)body, session); } } - public void filterExpressions(@NotNull PsiCodeBlock body, @NotNull UserDataHolder session) { + public void registerProblemsForExpressions(@NotNull PsiCodeBlock body, @NotNull UserDataHolder session) { DuplicateExpressionsContext context = DuplicateExpressionsContext.getContext(body, session); if (context == null) return; @@ -84,14 +116,34 @@ public class DuplicateExpressionsInspection extends LocalInspectionTool { context.forEach((pattern, occurrences) -> { if (!processed.contains(pattern)) { processed.addAll(occurrences); - if (occurrences.size() > 1 && areSafeToExtract(occurrences, body)) { - for (PsiExpression occurrence : occurrences) { - holder.registerProblem(occurrence, InspectionsBundle.message("inspection.duplicate.expressions.message")); - } - } + registerProblems(occurrences, body); } }); } + + public void registerProblems(@NotNull List occurrences, @NotNull PsiCodeBlock body) { + if (occurrences.size() > 1 && areSafeToExtract(occurrences, body)) { + Map> reusableVariables = collectReusableVariables(occurrences); + for (PsiExpression occurrence : occurrences) { + List fixes = new ArrayList<>(); + List variables = reusableVariables.get(occurrence); + if (variables != null) { + for (PsiVariable variable : variables) { + fixes.add(new ReuseVariableFix(occurrence, variable)); + } + } + PsiVariable variable = findVariableByInitializer(occurrence); + if (variable != null && canReplaceOtherOccurrences(occurrence, occurrences, variable)) { + fixes.add(new ReplaceOtherOccurrencesFix(occurrence, variable)); + } + else if (isOnTheFly) { + fixes.add(new IntroduceVariableFix(occurrence)); + } + holder.registerProblem(occurrence, InspectionsBundle.message("inspection.duplicate.expressions.message"), + fixes.toArray(LocalQuickFix.EMPTY_ARRAY)); + } + } + } }; } @@ -154,10 +206,214 @@ public class DuplicateExpressionsInspection extends LocalInspectionTool { return variables; } + @NotNull + private static Map> collectReusableVariables(@NotNull List occurrences) { + if (occurrences.size() <= 1) { + return Collections.emptyMap(); + } + Map initializers = new THashMap<>(); + for (PsiExpression occurrence : occurrences) { + PsiVariable variable = findVariableByInitializer(occurrence); + if (variable != null) { + initializers.put(variable, occurrence); + } + } + if (initializers.isEmpty()) { + return Collections.emptyMap(); + } + + Map> result = new THashMap<>(); + initializers.forEach((variable, initializer) -> { + for (PsiExpression occurrence : occurrences) { + if (occurrence != initializer && canReplaceWith(occurrence, variable)) { + result.computeIfAbsent(occurrence, unused -> new ArrayList<>()).add(variable); + } + } + }); + return result; + } + + private static boolean canReplaceOtherOccurrences(@NotNull PsiExpression originalOccurrence, + @NotNull List occurrences, + @NotNull PsiVariable variable) { + return occurrences.stream().anyMatch(occurrence -> occurrence != originalOccurrence && canReplaceWith(occurrence, variable)); + } + + private static boolean canReplaceWith(@NotNull PsiExpression occurrence, @NotNull PsiVariable variable) { + String variableName = variable.getName(); + if (variableName == null) { + return false; + } + PsiElementFactory factory = JavaPsiFacade.getInstance(variable.getProject()).getElementFactory(); + PsiExpression refExpr; + try { + refExpr = factory.createExpressionFromText(variableName, occurrence); + } + catch (IncorrectOperationException e) { + return false; + } + return refExpr instanceof PsiReferenceExpression && ((PsiReferenceExpression)refExpr).resolve() == variable; + } + + @Nullable + private static PsiVariable findVariableByInitializer(@NotNull PsiExpression expression) { + PsiElement parent = PsiUtil.skipParenthesizedExprUp(expression.getParent()); + if (parent instanceof PsiVariable) { + PsiVariable variable = (PsiVariable)parent; + if (PsiTreeUtil.isAncestor(variable.getInitializer(), expression, false)) { + return variable; + } + } + return null; + } + @Nullable @Override public JComponent createOptionsPanel() { return new SingleIntegerFieldOptionsPanel( - InspectionsBundle.message("inspection.duplicate.complexity.threshold"), this, "complexityThreshold", 3); + InspectionsBundle.message("inspection.duplicate.expressions.complexity.threshold"), this, "complexityThreshold", 3); + } + + private static class IntroduceVariableFix implements LocalQuickFix { + private final String myExpressionText; + + private IntroduceVariableFix(@NotNull PsiExpression expression) {myExpressionText = expression.getText();} + + @Nls(capitalization = Nls.Capitalization.Sentence) + @NotNull + @Override + public String getFamilyName() { + return InspectionsBundle.message("inspection.duplicate.expressions.introduce.variable.fix.family.name"); + } + + @Nls(capitalization = Nls.Capitalization.Sentence) + @NotNull + @Override + public String getName() { + return InspectionsBundle.message("inspection.duplicate.expressions.introduce.variable.fix.name", myExpressionText); + } + + @Override + public boolean startInWriteAction() { + return false; + } + + @Override + public void applyFix(@NotNull Project project, @NotNull ProblemDescriptor descriptor) { + PsiElement element = descriptor.getPsiElement(); + if (element instanceof PsiExpression) { + VirtualFile virtualFile = element.getContainingFile().getVirtualFile(); + Editor editor = FileEditorManager.getInstance(project).openTextEditor(new OpenFileDescriptor(project, virtualFile), true); + if (editor != null) { + new MyIntroduceVariableHandler().invoke(project, editor, (PsiExpression)element); + } + } + } + + private static class MyIntroduceVariableHandler extends IntroduceVariableHandler { + @Override + public IntroduceVariableSettings getSettings(Project project, Editor editor, PsiExpression expr, + PsiExpression[] occurrences, TypeSelectorManagerImpl typeSelectorManager, + boolean declareFinalIfAll, boolean anyAssignmentLHS, InputValidator validator, + PsiElement anchor, JavaReplaceChoice replaceChoice) { + if (replaceChoice == null && ApplicationManager.getApplication().isUnitTestMode()) { + replaceChoice = JavaReplaceChoice.ALL; + } + return super.getSettings(project, editor, expr, occurrences, typeSelectorManager, + declareFinalIfAll, anyAssignmentLHS, validator, anchor, replaceChoice); + } + } + } + + private static class ReuseVariableFix implements LocalQuickFix { + private final String myExpressionText; + private final String myVariableName; + + private ReuseVariableFix(@NotNull PsiExpression expression, @NotNull PsiVariable variable) { + myExpressionText = expression.getText(); + myVariableName = variable.getName(); + } + + @Nls(capitalization = Nls.Capitalization.Sentence) + @NotNull + @Override + public String getFamilyName() { + return InspectionsBundle.message("inspection.duplicate.expressions.reuse.variable.fix.family.name"); + } + + @Nls(capitalization = Nls.Capitalization.Sentence) + @NotNull + @Override + public String getName() { + return InspectionsBundle.message("inspection.duplicate.expressions.reuse.variable.fix.name", myVariableName, myExpressionText); + } + + @Override + public void applyFix(@NotNull Project project, @NotNull ProblemDescriptor descriptor) { + PsiElement element = descriptor.getPsiElement(); + if (element instanceof PsiExpression) { + new CommentTracker().replaceAndRestoreComments(element, myVariableName); + } + } + } + + private static class ReplaceOtherOccurrencesFix implements LocalQuickFix { + private final String myExpressionText; + private final String myVariableName; + + private ReplaceOtherOccurrencesFix(@NotNull PsiExpression expression, @NotNull PsiVariable variable) { + myExpressionText = expression.getText(); + myVariableName = variable.getName(); + } + + @Nls(capitalization = Nls.Capitalization.Sentence) + @NotNull + @Override + public String getFamilyName() { + return InspectionsBundle.message("inspection.duplicate.expressions.replace.other.occurrences.fix.family.name"); + } + + @Nls(capitalization = Nls.Capitalization.Sentence) + @NotNull + @Override + public String getName() { + return InspectionsBundle.message("inspection.duplicate.expressions.replace.other.occurrences.fix.name", myVariableName, myExpressionText); + } + + @Override + public void applyFix(@NotNull Project project, @NotNull ProblemDescriptor descriptor) { + PsiElement element = descriptor.getPsiElement(); + if (element instanceof PsiExpression) { + List occurrences = collectReplaceableOccurrences((PsiExpression)element); + for (PsiExpression occurrence : occurrences) { + new CommentTracker().replaceAndRestoreComments(occurrence, myVariableName); + } + } + } + + @NotNull + private static List collectReplaceableOccurrences(@NotNull PsiExpression originalExpr) { + PsiVariable variable = findVariableByInitializer(originalExpr); + PsiCodeBlock nearestBody = DuplicateExpressionsContext.findNearestBody(originalExpr); + if (variable != null && nearestBody != null) { + List replaceableOccurrences = new ArrayList<>(); + nearestBody.accept(new JavaRecursiveElementWalkingVisitor() { + final ExpressionHashingStrategy hashingStrategy = new ExpressionHashingStrategy(); + + @Override + public void visitExpression(PsiExpression occurrence) { + super.visitExpression(occurrence); + + if (occurrence != originalExpr && + hashingStrategy.equals(occurrence, originalExpr) && + canReplaceWith(occurrence, variable)) { + replaceableOccurrences.add(occurrence); + } + } + }); + return replaceableOccurrences; + } + return Collections.emptyList(); + } } } diff --git a/java/java-impl/src/com/intellij/codeInspection/duplicateExpressions/NoSideEffectExpressionEquivalenceChecker.java b/java/java-impl/src/com/intellij/codeInspection/duplicateExpressions/NoSideEffectExpressionEquivalenceChecker.java index fbcc62d13464..385079f97f2d 100644 --- a/java/java-impl/src/com/intellij/codeInspection/duplicateExpressions/NoSideEffectExpressionEquivalenceChecker.java +++ b/java/java-impl/src/com/intellij/codeInspection/duplicateExpressions/NoSideEffectExpressionEquivalenceChecker.java @@ -1,44 +1,22 @@ // 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.codeInspection.duplicateExpressions; -import com.intellij.psi.*; -import com.intellij.psi.tree.IElementType; +import com.intellij.psi.PsiAssignmentExpression; +import com.intellij.psi.PsiUnaryExpression; import com.intellij.psi.util.PsiUtil; import com.siyeh.ig.psiutils.EquivalenceChecker; -import com.siyeh.ig.psiutils.SideEffectChecker; import org.jetbrains.annotations.NotNull; /** * @author Pavel.Dolgov */ class NoSideEffectExpressionEquivalenceChecker extends EquivalenceChecker { - @Override - protected Match newExpressionsMatch(@NotNull PsiNewExpression newExpression1, - @NotNull PsiNewExpression newExpression2) { - return EXACT_MISMATCH; - } - - @Override - protected Match methodCallExpressionsMatch(@NotNull PsiMethodCallExpression methodCallExpression1, - @NotNull PsiMethodCallExpression methodCallExpression2) { - if (SideEffectChecker.mayHaveSideEffects(methodCallExpression1)) { - return EXACT_MISMATCH; - } - return super.methodCallExpressionsMatch(methodCallExpression1, methodCallExpression2); - } - @Override protected Match assignmentExpressionsMatch(@NotNull PsiAssignmentExpression assignmentExpression1, @NotNull PsiAssignmentExpression assignmentExpression2) { return EXACT_MISMATCH; } - @Override - protected Match arrayInitializerExpressionsMatch(@NotNull PsiArrayInitializerExpression arrayInitializerExpression1, - @NotNull PsiArrayInitializerExpression arrayInitializerExpression2) { - return EXACT_MISMATCH; - } - @Override protected Match unaryExpressionsMatch(@NotNull PsiUnaryExpression unaryExpression1, @NotNull PsiUnaryExpression unaryExpression2) { if (PsiUtil.isIncrementDecrementOperation(unaryExpression1)) { diff --git a/java/java-impl/src/com/intellij/codeInspection/duplicateExpressions/SideEffectCalculator.java b/java/java-impl/src/com/intellij/codeInspection/duplicateExpressions/SideEffectCalculator.java index c970b726e41d..c7160ede92b0 100644 --- a/java/java-impl/src/com/intellij/codeInspection/duplicateExpressions/SideEffectCalculator.java +++ b/java/java-impl/src/com/intellij/codeInspection/duplicateExpressions/SideEffectCalculator.java @@ -5,7 +5,6 @@ import com.intellij.codeInspection.dataFlow.CommonDataflow; import com.intellij.codeInspection.dataFlow.DfaFactType; import com.intellij.psi.*; import com.intellij.psi.util.PsiUtil; -import com.intellij.util.ObjectUtils; import com.intellij.util.containers.ObjectIntHashMap; import com.siyeh.ig.psiutils.ClassUtils; import com.siyeh.ig.psiutils.MethodUtils; @@ -165,4 +164,12 @@ class SideEffectCalculator { } return true; } + + /** + * Quick check to filter out the obvious things early + */ + static boolean isDefinitelyWithSideEffect(@Nullable PsiExpression expression) { + return expression instanceof PsiAssignmentExpression || + PsiUtil.isIncrementDecrementOperation(expression); + } } diff --git a/java/java-tests/testData/inspection/duplicateExpressions/CompositeQualifier.java b/java/java-tests/testData/inspection/duplicateExpressions/CompositeQualifier.java new file mode 100644 index 000000000000..1f61667ddace --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateExpressions/CompositeQualifier.java @@ -0,0 +1,8 @@ +class C { + int foo(String s) { + if (s.split(",").length > 2) { + return (s.split(",").length); + } + return 0; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateExpressions/MethodCallWithSideEffect.java b/java/java-tests/testData/inspection/duplicateExpressions/MethodCallWithSideEffect.java new file mode 100644 index 000000000000..164a8566ee50 --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateExpressions/MethodCallWithSideEffect.java @@ -0,0 +1,11 @@ +class C { + String foo(String a, String b, StringBuilder s) { + if (b.equals(a.substring(2) + b.substring(a.length()))) { + return a.substring(2) + b.substring(a.length()); + } + if (s.append(a).append(b.substring(a.length())) != null) { + return s.append(a).append(b.substring(a.length())).toString(); + } + return a; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateExpressions/VariableModified.java b/java/java-tests/testData/inspection/duplicateExpressions/VariableModified.java new file mode 100644 index 000000000000..62b59fce5caa --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateExpressions/VariableModified.java @@ -0,0 +1,17 @@ +class C { + String foo(String s) { + String t = s; + int len = s.length(); + if (len >= 2 && (s.charAt(0) == '(' && s.charAt(len - 1) == ')')) { + t = s.substring(1, len); + } + len--; + if (len >= 2 && (s.charAt(0) == '{' && s.charAt(len - 1) == '}')) { + t = s.substring(1, len); + } + if (len >= 2 && (s.charAt(0) == '[' && s.charAt(len - 1) == ']')) { + t = s.substring(1, len); + } + return t; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateExpressions/VariableNotModified.java b/java/java-tests/testData/inspection/duplicateExpressions/VariableNotModified.java new file mode 100644 index 000000000000..0f52b0b7b9b5 --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateExpressions/VariableNotModified.java @@ -0,0 +1,16 @@ +class C { + String foo(String s) { + String t = s; + int len = s.length(); + if (len >= 3 && (s.charAt(0) == '(' && s.charAt(len - 2) == ')')) { + t = s.substring(1, len); + } + if (len >= 3 && (s.charAt(0) == '{' && s.charAt(len - 2) == '}')) { + t = s.substring(1, len); + } + if (len >= 3 && (s.charAt(0) == '[' && s.charAt(len - 2) == ']')) { + t = s.substring(1, len); + } + return t; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateExpressionsFix/IntroduceVariable.java b/java/java-tests/testData/inspection/duplicateExpressionsFix/IntroduceVariable.java new file mode 100644 index 000000000000..1c48decf10b9 --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateExpressionsFix/IntroduceVariable.java @@ -0,0 +1,15 @@ +class C { + private static String toString(float... x) { + String ret = null; + for (float v : x) { + String s = String.format("%f", v); + if (ret == null) { + ret = s.substring(s.length() - 9); + } + else { + ret += "," + s.substring(s.length() - 9); + } + } + return ret; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateExpressionsFix/IntroduceVariableOtherVariableNotInScope.java b/java/java-tests/testData/inspection/duplicateExpressionsFix/IntroduceVariableOtherVariableNotInScope.java new file mode 100644 index 000000000000..3d0a774e94bb --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateExpressionsFix/IntroduceVariableOtherVariableNotInScope.java @@ -0,0 +1,17 @@ +class C { + private static String toString(float... x) { + String ret = null; + for (float v : x) { + String s = String.format("%f", v); + String substr = s.substring(s.length() - 9); + if (ret == null) { + ret = s.substring(s.length() - 9); + } + else { + String substr2 = s.substring(s.length() - 9); + ret += "," + substr2; + } + } + return ret; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateExpressionsFix/IntroduceVariableOtherVariableNotInScope_after.java b/java/java-tests/testData/inspection/duplicateExpressionsFix/IntroduceVariableOtherVariableNotInScope_after.java new file mode 100644 index 000000000000..c01fa3cfd55a --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateExpressionsFix/IntroduceVariableOtherVariableNotInScope_after.java @@ -0,0 +1,18 @@ +class C { + private static String toString(float... x) { + String ret = null; + for (float v : x) { + String s = String.format("%f", v); + String substring = s.substring(s.length() - 9); + String substr = substring; + if (ret == null) { + ret = substring; + } + else { + String substr2 = substring; + ret += "," + substr2; + } + } + return ret; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateExpressionsFix/IntroduceVariable_after.java b/java/java-tests/testData/inspection/duplicateExpressionsFix/IntroduceVariable_after.java new file mode 100644 index 000000000000..398102d9f783 --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateExpressionsFix/IntroduceVariable_after.java @@ -0,0 +1,16 @@ +class C { + private static String toString(float... x) { + String ret = null; + for (float v : x) { + String s = String.format("%f", v); + String substring = s.substring(s.length() - 9); + if (ret == null) { + ret = substring; + } + else { + ret += "," + substring; + } + } + return ret; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateExpressionsFix/ReplaceOthers.java b/java/java-tests/testData/inspection/duplicateExpressionsFix/ReplaceOthers.java new file mode 100644 index 000000000000..84a2d5db3658 --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateExpressionsFix/ReplaceOthers.java @@ -0,0 +1,16 @@ +class C { + private static String toString(float... x) { + String ret = null; + for (float v : x) { + String s = String.format("%f", v); + String substr = s.substring(s.length() - 9); + if (ret == null) { + ret = s.substring(s.length() - 9); + } + else { + ret += "," + s.substring(s.length() - 9); + } + } + return ret; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateExpressionsFix/ReplaceOthers_after.java b/java/java-tests/testData/inspection/duplicateExpressionsFix/ReplaceOthers_after.java new file mode 100644 index 000000000000..d6716b7a1e4e --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateExpressionsFix/ReplaceOthers_after.java @@ -0,0 +1,16 @@ +class C { + private static String toString(float... x) { + String ret = null; + for (float v : x) { + String s = String.format("%f", v); + String substr = s.substring(s.length() - 9); + if (ret == null) { + ret = substr; + } + else { + ret += "," + substr; + } + } + return ret; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateExpressionsFix/ReuseVariable.java b/java/java-tests/testData/inspection/duplicateExpressionsFix/ReuseVariable.java new file mode 100644 index 000000000000..c7dbca10e685 --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateExpressionsFix/ReuseVariable.java @@ -0,0 +1,16 @@ +class C { + private static String toString(float... x) { + String ret = null; + for (float v : x) { + String s = String.format("%f", v); + String substr = s.substring(s.length() - 9); + if (ret == null) { + ret = s.substring(s.length() - 9); + } + else { + ret += "," + s.substring(s.length() - 9); + } + } + return ret; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateExpressionsFix/ReuseVariable_after.java b/java/java-tests/testData/inspection/duplicateExpressionsFix/ReuseVariable_after.java new file mode 100644 index 000000000000..a020dff6f211 --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateExpressionsFix/ReuseVariable_after.java @@ -0,0 +1,16 @@ +class C { + private static String toString(float... x) { + String ret = null; + for (float v : x) { + String s = String.format("%f", v); + String substr = s.substring(s.length() - 9); + if (ret == null) { + ret = substr; + } + else { + ret += "," + s.substring(s.length() - 9); + } + } + return ret; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateExpressionsFix/VariableNotInScopeCantReplaceOthers.java b/java/java-tests/testData/inspection/duplicateExpressionsFix/VariableNotInScopeCantReplaceOthers.java new file mode 100644 index 000000000000..880dbd39da9d --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateExpressionsFix/VariableNotInScopeCantReplaceOthers.java @@ -0,0 +1,18 @@ +class C { + private static String toString(float... x) { + String ret = null; + for (float v : x) { + String s = String.format("%f", v); + { + String substr = s.substring(s.length() - 9); + } + if (ret == null) { + ret = s.substring(s.length() - 9); + } + else { + ret += "," + s.substring(s.length() - 9); + } + } + return ret; + } +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/DuplicateExpressionsFixTest.kt b/java/java-tests/testSrc/com/intellij/java/codeInspection/DuplicateExpressionsFixTest.kt new file mode 100644 index 000000000000..f613feba5722 --- /dev/null +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DuplicateExpressionsFixTest.kt @@ -0,0 +1,62 @@ +// 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.java.codeInspection + +import com.intellij.JavaTestUtil +import com.intellij.codeInsight.intention.IntentionAction +import com.intellij.codeInspection.InspectionsBundle +import com.intellij.codeInspection.duplicateExpressions.DuplicateExpressionsInspection +import com.intellij.testFramework.fixtures.LightCodeInsightFixtureTestCase + +/** + * @author Pavel.Dolgov + */ +class DuplicateExpressionsFixTest : LightCodeInsightFixtureTestCase() { + val inspection = DuplicateExpressionsInspection() + + override fun setUp() { + super.setUp() + myFixture.enableInspections(inspection) + } + + override fun getBasePath() = JavaTestUtil.getRelativeJavaTestDataPath() + "/inspection/duplicateExpressionsFix" + + fun testIntroduceVariable() = doTest(introduce("s.substring(s.length() - 9)")) + fun testReuseVariable() = doTest(reuse("substr", "s.substring(s.length() - 9)")) + fun testReplaceOthers() = doTest(replace("substr", "s.substring(s.length() - 9)")) + fun testIntroduceVariableOtherVariableNotInScope() = doTest(introduce("s.substring(s.length() - 9)")) + fun testVariableNotInScopeCantReplaceOthers() = doNegativeTest(replace("substr", "s.substring(s.length() - 9)")) + + private fun doTest(message: String, threshold: Int = 50) = + withThreshold(threshold) { + myFixture.configureByFile("${getTestName(false)}.java") + myFixture.launchAction(myFixture.findSingleIntention(message)) + myFixture.checkResultByFile("${getTestName(false)}_after.java") + } + + private fun doNegativeTest(message: String, threshold: Int = 50) = + withThreshold(threshold) { + myFixture.configureByFile("${getTestName(false)}.java") + val intentions = myFixture.filterAvailableIntentions(message) + assertEquals(emptyList(), intentions) + } + + private fun withThreshold(threshold: Int, block: () -> Unit) { + val oldThreshold = inspection.complexityThreshold + try { + inspection.complexityThreshold = threshold + block() + } + finally { + inspection.complexityThreshold = oldThreshold + } + } + + private fun introduce(expr: String) = + InspectionsBundle.message("inspection.duplicate.expressions.introduce.variable.fix.name", expr)!! + + private fun reuse(name: String, expr: String) = + InspectionsBundle.message("inspection.duplicate.expressions.reuse.variable.fix.name", name, expr)!! + + private fun replace(name: String, expr: String) = + InspectionsBundle.message("inspection.duplicate.expressions.replace.other.occurrences.fix.name", name, expr)!! +} diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/DuplicateExpressionsTest.kt b/java/java-tests/testSrc/com/intellij/java/codeInspection/DuplicateExpressionsTest.kt index 127a1693fe59..1889af0b2af0 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/DuplicateExpressionsTest.kt +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DuplicateExpressionsTest.kt @@ -19,12 +19,16 @@ class DuplicateExpressionsTest : LightCodeInsightFixtureTestCase() { override fun getBasePath() = JavaTestUtil.getRelativeJavaTestDataPath() + "/inspection/duplicateExpressions" fun testUpdateInWhileLoop() = doTest(70) - fun testIntFloatMixedMath() = doTest(60) + fun testIntFloatMixedMath() = doTest(50) fun testTernaryIf() = doTest(60) fun testStringStartsWith() = doTest(60) fun testFinalFieldsOfParameter() = doTest(100) - fun testStringCharAt() = doTest(70) + fun testStringCharAt() = doTest(60) fun testEquals() = doTest(70) + fun testVariableModified() = doTest(50) + fun testVariableNotModified() = doTest(50) + fun testCompositeQualifier() = doTest(40) + fun testMethodCallWithSideEffect() = doTest(70) private fun doTest(threshold: Int = 50) { val oldThreshold = inspection.complexityThreshold diff --git a/platform/platform-resources-en/src/messages/InspectionsBundle.properties b/platform/platform-resources-en/src/messages/InspectionsBundle.properties index a545205dcafa..1c2e3ac74348 100644 --- a/platform/platform-resources-en/src/messages/InspectionsBundle.properties +++ b/platform/platform-resources-en/src/messages/InspectionsBundle.properties @@ -1006,4 +1006,10 @@ inspection.objects.equals.can.be.simplified.fix.name=Replace 'Objects.equals()' inspection.duplicate.expressions.display.name=Multiple occurrences of the same expression inspection.duplicate.expressions.message=Multiple occurrences of #ref #loc -inspection.duplicate.complexity.threshold=Expression complexity threshold \ No newline at end of file +inspection.duplicate.expressions.complexity.threshold=Expression complexity threshold +inspection.duplicate.expressions.introduce.variable.fix.family.name=Introduce variable +inspection.duplicate.expressions.introduce.variable.fix.name=Introduce variable for ''{0}'' +inspection.duplicate.expressions.reuse.variable.fix.family.name=Reuse variable +inspection.duplicate.expressions.reuse.variable.fix.name=Reuse variable ''{0}'' for ''{1}'' +inspection.duplicate.expressions.replace.other.occurrences.fix.family.name=Replace with variable other occurrences of expression +inspection.duplicate.expressions.replace.other.occurrences.fix.name=Replace with ''{0}'' other occurrences of ''{1}'' \ No newline at end of file