diff --git a/java/java-analysis-impl/src/com/intellij/refactoring/util/duplicates/ReturnValue.java b/java/java-analysis-impl/src/com/intellij/refactoring/util/duplicates/ReturnValue.java index 612ef9b61eea..73a88e5cc46e 100644 --- a/java/java-analysis-impl/src/com/intellij/refactoring/util/duplicates/ReturnValue.java +++ b/java/java-analysis-impl/src/com/intellij/refactoring/util/duplicates/ReturnValue.java @@ -31,4 +31,9 @@ public interface ReturnValue { @Nullable PsiStatement createReplacement(@NotNull PsiMethod extractedMethod, @NotNull PsiMethodCallExpression methodCallExpression, @Nullable PsiType returnType) throws IncorrectOperationException; + + static boolean areEquivalent(@Nullable ReturnValue value1, @Nullable ReturnValue value2) { + return value1 == null && value2 == null || + value1 != null && value1.isEquivalent(value2); + } } diff --git a/java/java-impl/src/META-INF/JavaPlugin.xml b/java/java-impl/src/META-INF/JavaPlugin.xml index 666c698f0044..3a916d35a282 100644 --- a/java/java-impl/src/META-INF/JavaPlugin.xml +++ b/java/java-impl/src/META-INF/JavaPlugin.xml @@ -909,6 +909,11 @@ groupKey="group.names.verbose.or.redundant.code.constructs" groupBundle="messages.InspectionsBundle" enabledByDefault="true" level="WEAK WARNING" implementationClass="com.intellij.codeInspection.duplicateExpressions.DuplicateExpressionsInspection" /> + branches = collectBranches(switchStatement); + int size = branches.size(); + if (size > 1) { + boolean[] isDuplicate = new boolean[size]; + for (int i = 0; i < size - 1; i++) { + if (isDuplicate[i]) continue; + + for (int j = i + 1; j < size; j++) { + if (areDuplicates(branches, i, j)) { + isDuplicate[j] = true; + registerProblem(branches.get(j).myStatements); + + if (!isDuplicate[i]) { + isDuplicate[i] = true; + registerProblem(branches.get(i).myStatements); + } + } + } + } + } + } + + private void registerProblem(@NotNull PsiStatement[] statements) { + ProblemDescriptor descriptor = InspectionManager.getInstance(myHolder.getProject()) + .createProblemDescriptor(statements[0], statements[statements.length - 1], + InspectionsBundle.message("inspection.duplicate.branches.in.switch.message"), + ProblemHighlightType.GENERIC_ERROR_OR_WARNING, myHolder.isOnTheFly()); + myHolder.registerProblem(descriptor); + } + } + + @NotNull + static List collectBranches(@NotNull PsiSwitchStatement switchStatement) { + PsiCodeBlock body = switchStatement.getBody(); + if (body == null) return Collections.emptyList(); + + List branches = new ArrayList<>(); + List statementList = null; + for (PsiStatement statement : body.getStatements()) { + if (statement instanceof PsiSwitchLabelStatement) { + if (statementList != null) { + branches.add(new Branch(statementList, hasImplicitBreak(statement))); + statementList = null; + } + continue; + } + if (statementList == null) { + if (isIgnoredSingleStatement(statement)) continue; // trivial duplicate branches are probably OK + statementList = new ArrayList<>(); + } + statementList.add(statement); + } + if (statementList != null) { + branches.add(new Branch(statementList, true)); + } + return branches; + } + + static boolean areDuplicates(List branches, int index, int otherIndex) { + Branch branch = branches.get(index); + Branch otherBranch = branches.get(otherIndex); + if (branch.canFallThrough() || otherBranch.canFallThrough()) { + return false; + } + + Match match = branch.match(otherBranch); + if (match != null) { + Match otherMatch = otherBranch.match(branch); + if (otherMatch != null) { + return ReturnValue.areEquivalent(match.getReturnValue(), otherMatch.getReturnValue()); + } + } + return false; + } + + private static boolean hasImplicitBreak(@NotNull PsiStatement statement) { + while (statement instanceof PsiSwitchLabelStatement) { + statement = PsiTreeUtil.getNextSiblingOfType(statement, PsiStatement.class); + } + return statement == null || isBreakWithoutLabel(statement); + } + + private static boolean isBreakWithoutLabel(@Nullable PsiStatement statement) { + return statement instanceof PsiBreakStatement && ((PsiBreakStatement)statement).getLabelIdentifier() == null; + } + + private static boolean isIgnoredSingleStatement(@NotNull PsiStatement statement) { + if (statement instanceof PsiBreakStatement) { + return true; + } + if (statement instanceof PsiReturnStatement) { + PsiExpression value = ((PsiReturnStatement)statement).getReturnValue(); + return value == null || ExpressionUtils.isNullLiteral(value); + } + return false; + } + + private static class Branch { + private final PsiStatement[] myStatements; + + private DuplicatesFinder myFinder; + private Boolean myCanFallThrough; + + Branch(@NotNull List statementList, boolean hasImplicitBreak) { + int lastIndex = statementList.size() - 1; + PsiStatement lastStatement = statementList.get(lastIndex); + if (hasImplicitBreak || + lastStatement instanceof PsiBreakStatement || + lastStatement instanceof PsiReturnStatement || + lastStatement instanceof PsiContinueStatement || + lastStatement instanceof PsiThrowStatement) { + myCanFallThrough = false; // in more complex cases it will be computed lazily + } + if (lastIndex > 0 && isBreakWithoutLabel(lastStatement)) { + statementList = statementList.subList(0, lastIndex); // trailing 'break' is already taken into account in myCanFallThrough + } + myStatements = statementList.toArray(PsiStatement.EMPTY_ARRAY); + } + + @Nullable + Match match(Branch other) { + return getFinder().isDuplicate(other.myStatements[0], true); + } + + boolean canFallThrough() { + if (myCanFallThrough == null) { + myCanFallThrough = calculateCanFallThrough(myStatements); + } + return myCanFallThrough; + } + + @NotNull + private DuplicatesFinder getFinder() { + if (myFinder == null) { + myFinder = createFinder(myStatements); + } + return myFinder; + } + + @NotNull + private static DuplicatesFinder createFinder(@NotNull PsiStatement[] statements) { + Project project = statements[0].getProject(); + InputVariables noVariables = new InputVariables(Collections.emptyList(), project, new LocalSearchScope(statements), false); + return new DuplicatesFinder(statements, noVariables, null, Collections.emptyList()); + } + + private static boolean calculateCanFallThrough(@NotNull PsiStatement[] statements) { + PsiSwitchStatement switchStatement = PsiTreeUtil.getParentOfType(statements[0], PsiSwitchStatement.class); + if (switchStatement != null) { + PsiElement switchBody = switchStatement.getBody(); + if (switchBody != null) { + try { + ControlFlow flow = HighlightControlFlowUtil.getControlFlowNoConstantEvaluate(switchBody); + int branchStart = flow.getStartOffset(statements[0]); + int branchEnd = flow.getEndOffset(statements[statements.length - 1]); + if (branchStart >= 0 && branchEnd >= 0) { + return ControlFlowUtil.isInstructionReachable(flow, branchEnd, branchStart); + } + } + catch (AnalysisCanceledException ignore) { + } + } + } + return true; + } + } +} diff --git a/java/java-impl/src/inspectionDescriptions/DuplicateBranchesInSwitch.html b/java/java-impl/src/inspectionDescriptions/DuplicateBranchesInSwitch.html new file mode 100644 index 000000000000..6874ea6cff7c --- /dev/null +++ b/java/java-impl/src/inspectionDescriptions/DuplicateBranchesInSwitch.html @@ -0,0 +1,6 @@ + + +Reports switch statements containing the same code in different branches. + + + \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateBranchesInSwitch/AllFallThrough.java b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/AllFallThrough.java new file mode 100644 index 000000000000..3eeb8803153b --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/AllFallThrough.java @@ -0,0 +1,13 @@ +class C { + void foo(int n) { + switch (n) { + case 1: + bar("A"); + case 2: + bar("A"); + case 3: + bar("A"); + } + } + void bar(String s){} +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateBranchesInSwitch/BreakAndReturnUnderIf.java b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/BreakAndReturnUnderIf.java new file mode 100644 index 000000000000..cfe476ca502a --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/BreakAndReturnUnderIf.java @@ -0,0 +1,26 @@ +class C { + int foo(int n, boolean b) { + switch (n) { + case 1: + if(b) { + return bar("A"); + } else { + break; + } + case 2: + if(b) { + return bar("B"); + } else { + break; + } + case 3: + if(b) { + return bar("A"); + } else { + break; + } + } + return 0; + } + int bar(String s){return s.charAt(0);} +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateBranchesInSwitch/BreakWithLabel.java b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/BreakWithLabel.java new file mode 100644 index 000000000000..9bf62b598538 --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/BreakWithLabel.java @@ -0,0 +1,20 @@ +class C { + void foo(int n) { + OuterLabel: + if (n > 0) { + switch (n) { + case 1: + bar("A"); + break; + case 2: + bar("B"); + break; + case 3: + bar("A"); + break OuterLabel; + } + bar("Z"); + } + } + void bar(String s){} +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateBranchesInSwitch/ComplexBranches.java b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/ComplexBranches.java new file mode 100644 index 000000000000..747300476ff8 --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/ComplexBranches.java @@ -0,0 +1,31 @@ +class C { + void foo(int n, boolean b) { + switch (n) { + case 1: + if(b) { + bar("A"); + } else { + bar("z"); + } + bar("o"); + break; + case 2: + if(b) { + bar("B"); + } else { + bar("z"); + } + bar("o"); + break; + case 3: + if(b) { + bar("A"); + } else { + bar("z"); + } + bar("o"); + break; + } + } + void bar(String s){} +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateBranchesInSwitch/Continue.java b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/Continue.java new file mode 100644 index 000000000000..535202a893aa --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/Continue.java @@ -0,0 +1,21 @@ +class C { + int foo(int n) { + int s = 0; + for (int i = 0; i < n; i++) { + switch (i % 4) { + case 1: + s += i; + continue; + case 2: + continue; + case 3: + s += i; + continue; + default: + s += i; + } + s /= 2; + } + return s; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateBranchesInSwitch/FallThrough.java b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/FallThrough.java new file mode 100644 index 000000000000..cc926c234848 --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/FallThrough.java @@ -0,0 +1,15 @@ +class C { + void foo(int n) { + switch (n) { + case 1: + bar("A"); + case 2: + bar("B"); + break; + case 3: + bar("A"); + break; + } + } + void bar(String s){} +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateBranchesInSwitch/FallThroughToBreak.java b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/FallThroughToBreak.java new file mode 100644 index 000000000000..383d6d2bc5f1 --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/FallThroughToBreak.java @@ -0,0 +1,17 @@ +class C { + void foo(int n) { + switch (n) { + case 1: + bar("A"); + case 2: + break; + case 3: + bar("A"); + break; + case 4: + bar("A"); + case 5: + } + } + void bar(String s){} +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateBranchesInSwitch/NoLastBreak.java b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/NoLastBreak.java new file mode 100644 index 000000000000..7ddbeae2f69e --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/NoLastBreak.java @@ -0,0 +1,15 @@ +class C { + void foo(int n) { + switch (n) { + case 1: + bar("A"); + break; + case 2: + bar("B"); + break; + case 3: + bar("A"); + } + } + void bar(String s){} +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateBranchesInSwitch/Return.java b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/Return.java new file mode 100644 index 000000000000..6bbf73fb68d1 --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/Return.java @@ -0,0 +1,13 @@ +class C { + String foo(int n) { + switch (n) { + case 1: + return "A"; + case 2: + return "B"; + case 3: + return "A"; + } + return ""; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateBranchesInSwitch/Simple.java b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/Simple.java new file mode 100644 index 000000000000..862d7397ce08 --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/Simple.java @@ -0,0 +1,16 @@ +class C { + void foo(int n) { + switch (n) { + case 1: + bar("A"); + break; + case 2: + bar("B"); + break; + case 3: + bar("A"); + break; + } + } + void bar(String s){} +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateBranchesInSwitch/ThreeDuplicates.java b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/ThreeDuplicates.java new file mode 100644 index 000000000000..a1234a017856 --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/ThreeDuplicates.java @@ -0,0 +1,19 @@ +class C { + void foo(int n) { + switch (n) { + case 1: + bar("A"); + break; + case 2: + bar("B"); + break; + case 3: + bar("A"); + break; + default: + bar("A"); + break; + } + } + void bar(String s){} +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateBranchesInSwitch/Throw.java b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/Throw.java new file mode 100644 index 000000000000..96ac2f59385b --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/Throw.java @@ -0,0 +1,13 @@ +class C { + String foo(int n) { + switch (n) { + case 1: + throw new IllegalArgumentException("A"); + case 2: + throw new IllegalStateException("A"); + case 3: + throw new IllegalArgumentException("A"); + } + return ""; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateBranchesInSwitch/TwoCaseLabels.java b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/TwoCaseLabels.java new file mode 100644 index 000000000000..0593989140fc --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/TwoCaseLabels.java @@ -0,0 +1,17 @@ +class C { + void foo(int n) { + switch (n) { + case 1: + case 2: + bar("A"); + break; + case 3: + bar("A"); + break; + case 4: + bar("B"); + break; + } + } + void bar(String s){} +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/DuplicateBranchesInSwitchTest.kt b/java/java-tests/testSrc/com/intellij/java/codeInspection/DuplicateBranchesInSwitchTest.kt new file mode 100644 index 000000000000..ae7cc33ba1a9 --- /dev/null +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DuplicateBranchesInSwitchTest.kt @@ -0,0 +1,38 @@ +// 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.codeInspection.DuplicateBranchesInSwitchInspection +import com.intellij.testFramework.fixtures.LightCodeInsightFixtureTestCase + +/** + * @author Pavel.Dolgov + */ +class DuplicateBranchesInSwitchTest : LightCodeInsightFixtureTestCase() { + val inspection = DuplicateBranchesInSwitchInspection() + + override fun setUp() { + super.setUp() + myFixture.enableInspections(inspection) + } + + override fun getBasePath() = JavaTestUtil.getRelativeJavaTestDataPath() + "/inspection/duplicateBranchesInSwitch" + + fun testSimple() = doTest() + fun testReturn() = doTest() + fun testThrow() = doTest() + fun testContinue() = doTest() + fun testFallThrough() = doTest() + fun testAllFallThrough() = doTest() + fun testNoLastBreak() = doTest() + fun testFallThroughToBreak() = doTest() + fun testThreeDuplicates() = doTest() + fun testTwoCaseLabels() = doTest() + fun testComplexBranches() = doTest() + fun testBreakWithLabel() = doTest() + fun testBreakAndReturnUnderIf() = doTest() + + private fun doTest() { + myFixture.testHighlighting("${getTestName(false)}.java") + } +} \ No newline at end of file diff --git a/platform/platform-resources-en/src/messages/InspectionsBundle.properties b/platform/platform-resources-en/src/messages/InspectionsBundle.properties index e2b99ad2a223..6216fbd2f726 100644 --- a/platform/platform-resources-en/src/messages/InspectionsBundle.properties +++ b/platform/platform-resources-en/src/messages/InspectionsBundle.properties @@ -1021,4 +1021,7 @@ inspection.join.declaration.and.assignment.message=Assignment can be joined with inspection.join.declaration.and.assignment.fix.family.name=Join declaration and assignment inspection.overflowing.loop.index.inspection.name=Loop executes zero or billions times -inspection.overflowing.loop.index.inspection.description=Loop executes zero or billions times \ No newline at end of file +inspection.overflowing.loop.index.inspection.description=Loop executes zero or billions times + +inspection.duplicate.branches.in.switch.display.name=Duplicate branches in 'switch' statement +inspection.duplicate.branches.in.switch.message=Duplicate branch in 'switch' statement \ No newline at end of file diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/migration/TryWithIdenticalCatchesInspection.java b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/migration/TryWithIdenticalCatchesInspection.java index 91d52abb9701..06fe61d395fa 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/migration/TryWithIdenticalCatchesInspection.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/migration/TryWithIdenticalCatchesInspection.java @@ -281,12 +281,7 @@ public class TryWithIdenticalCatchesInspection extends BaseInspection { if (match2 == null) { return false; } - final ReturnValue returnValue1 = match1.getReturnValue(); - final ReturnValue returnValue2 = match2.getReturnValue(); - if (returnValue1 == null) { - return returnValue2 == null; - } - return returnValue1.isEquivalent(returnValue2); + return ReturnValue.areEquivalent(match1.getReturnValue(), match2.getReturnValue()); } private Match findDuplicate(@NotNull CatchSectionWrapper section) {