Java: Highlight identical branches in 'switch' statement (IDEA-181304)

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);
+  }
 }

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" />
+    <localInspection groupPath="Java" language="JAVA" shortName="DuplicateBranchesInSwitch"
+                     key="inspection.duplicate.branches.in.switch.display.name" bundle="messages.InspectionsBundle"
+                     groupKey="group.names.verbose.or.redundant.code.constructs" groupBundle="messages.InspectionsBundle"
+                     enabledByDefault="true" level="WEAK WARNING"
+                     implementationClass="com.intellij.codeInspection.DuplicateBranchesInSwitchInspection" />
     <localInspection groupPath="Java" language="JAVA" shortName="SortedCollectionWithNonComparableKeys"
                      key="inspection.sorted.collection.with.non.comparable.keys.display.name" bundle="messages.InspectionsBundle"
                      groupKey="group.names.probable.bugs" groupBundle="messages.InspectionsBundle"

java/java-impl/src/com/intellij/codeInspection/DuplicateBranchesInSwitchInspection.java

@@ -0,0 +1,210 @@
+// 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;
+
+import com.intellij.codeInsight.daemon.impl.analysis.HighlightControlFlowUtil;
+import com.intellij.openapi.project.Project;
+import com.intellij.psi.*;
+import com.intellij.psi.controlFlow.AnalysisCanceledException;
+import com.intellij.psi.controlFlow.ControlFlow;
+import com.intellij.psi.controlFlow.ControlFlowUtil;
+import com.intellij.psi.search.LocalSearchScope;
+import com.intellij.psi.util.PsiTreeUtil;
+import com.intellij.refactoring.extractMethod.InputVariables;
+import com.intellij.refactoring.util.duplicates.DuplicatesFinder;
+import com.intellij.refactoring.util.duplicates.Match;
+import com.intellij.refactoring.util.duplicates.ReturnValue;
+import com.siyeh.ig.psiutils.ExpressionUtils;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+/**
+ * @author Pavel.Dolgov
+ */
+public class DuplicateBranchesInSwitchInspection extends LocalInspectionTool {
+
+  @NotNull
+  @Override
+  public PsiElementVisitor buildVisitor(@NotNull ProblemsHolder holder, boolean isOnTheFly) {
+    return new DuplicateBranchesVisitor(holder);
+  }
+
+  private static class DuplicateBranchesVisitor extends JavaElementVisitor {
+    private final ProblemsHolder myHolder;
+
+    DuplicateBranchesVisitor(ProblemsHolder holder) {myHolder = holder;}
+
+    @Override
+    public void visitSwitchStatement(PsiSwitchStatement switchStatement) {
+      super.visitSwitchStatement(switchStatement);
+
+      List<Branch> 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<Branch> collectBranches(@NotNull PsiSwitchStatement switchStatement) {
+    PsiCodeBlock body = switchStatement.getBody();
+    if (body == null) return Collections.emptyList();
+
+    List<Branch> branches = new ArrayList<>();
+    List<PsiStatement> 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<Branch> 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<PsiStatement> 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;
+    }
+  }
+}

java/java-impl/src/inspectionDescriptions/DuplicateBranchesInSwitch.html

@@ -0,0 +1,6 @@
+<html>
+<body>
+Reports <code>switch</code> statements containing the same code in different branches.
+<!-- tooltip end -->
+</body>
+</html>

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){}
+}

java/java-tests/testData/inspection/duplicateBranchesInSwitch/BreakAndReturnUnderIf.java

@@ -0,0 +1,26 @@
+class C {
+  int foo(int n, boolean b) {
+    switch (n) {
+      case 1:
+        <weak_warning descr="Duplicate branch in 'switch' statement">if(b) {
+          return bar("A");
+        } else {
+          break;
+        }</weak_warning>
+      case 2:
+        if(b) {
+          return bar("B");
+        } else {
+          break;
+        }
+      case 3:
+        <weak_warning descr="Duplicate branch in 'switch' statement">if(b) {
+          return bar("A");
+        } else {
+          break;
+        }</weak_warning>
+    }
+    return 0;
+  }
+  int bar(String s){return s.charAt(0);}
+}

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){}
+}

java/java-tests/testData/inspection/duplicateBranchesInSwitch/ComplexBranches.java

@@ -0,0 +1,31 @@
+class C {
+  void foo(int n, boolean b) {
+    switch (n) {
+      case 1:
+        <weak_warning descr="Duplicate branch in 'switch' statement">if(b) {
+          bar("A");
+        } else {
+          bar("z");
+        }
+        bar("o");</weak_warning>
+        break;
+      case 2:
+        if(b) {
+          bar("B");
+        } else {
+          bar("z");
+        }
+        bar("o");
+        break;
+      case 3:
+        <weak_warning descr="Duplicate branch in 'switch' statement">if(b) {
+          bar("A");
+        } else {
+          bar("z");
+        }
+        bar("o");</weak_warning>
+        break;
+    }
+  }
+  void bar(String s){}
+}

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:
+          <weak_warning descr="Duplicate branch in 'switch' statement">s += i;
+          continue;</weak_warning>
+        case 2:
+          continue;
+        case 3:
+          <weak_warning descr="Duplicate branch in 'switch' statement">s += i;
+          continue;</weak_warning>
+        default:
+          s += i;
+      }
+      s /= 2;
+    }
+    return s;
+  }
+}

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){}
+}

java/java-tests/testData/inspection/duplicateBranchesInSwitch/FallThroughToBreak.java

@@ -0,0 +1,17 @@
+class C {
+  void foo(int n) {
+    switch (n) {
+      case 1:
+        <weak_warning descr="Duplicate branch in 'switch' statement">bar("A");</weak_warning>
+      case 2:
+        break;
+      case 3:
+        <weak_warning descr="Duplicate branch in 'switch' statement">bar("A");</weak_warning>
+        break;
+      case 4:
+        <weak_warning descr="Duplicate branch in 'switch' statement">bar("A");</weak_warning>
+      case 5:
+    }
+  }
+  void bar(String s){}
+}

java/java-tests/testData/inspection/duplicateBranchesInSwitch/NoLastBreak.java

@@ -0,0 +1,15 @@
+class C {
+  void foo(int n) {
+    switch (n) {
+      case 1:
+        <weak_warning descr="Duplicate branch in 'switch' statement">bar("A");</weak_warning>
+        break;
+      case 2:
+        bar("B");
+        break;
+      case 3:
+        <weak_warning descr="Duplicate branch in 'switch' statement">bar("A");</weak_warning>
+    }
+  }
+  void bar(String s){}
+}

java/java-tests/testData/inspection/duplicateBranchesInSwitch/Return.java

@@ -0,0 +1,13 @@
+class C {
+  String foo(int n) {
+    switch (n) {
+      case 1:
+        <weak_warning descr="Duplicate branch in 'switch' statement">return "A";</weak_warning>
+      case 2:
+        return "B";
+      case 3:
+        <weak_warning descr="Duplicate branch in 'switch' statement">return "A";</weak_warning>
+    }
+    return "";
+  }
+}

java/java-tests/testData/inspection/duplicateBranchesInSwitch/Simple.java

@@ -0,0 +1,16 @@
+class C {
+  void foo(int n) {
+    switch (n) {
+      case 1:
+        <weak_warning descr="Duplicate branch in 'switch' statement">bar("A");</weak_warning>
+        break;
+      case 2:
+        bar("B");
+        break;
+      case 3:
+        <weak_warning descr="Duplicate branch in 'switch' statement">bar("A");</weak_warning>
+        break;
+    }
+  }
+  void bar(String s){}
+}

java/java-tests/testData/inspection/duplicateBranchesInSwitch/ThreeDuplicates.java

@@ -0,0 +1,19 @@
+class C {
+  void foo(int n) {
+    switch (n) {
+      case 1:
+        <weak_warning descr="Duplicate branch in 'switch' statement">bar("A");</weak_warning>
+        break;
+      case 2:
+        bar("B");
+        break;
+      case 3:
+        <weak_warning descr="Duplicate branch in 'switch' statement">bar("A");</weak_warning>
+        break;
+      default:
+        <weak_warning descr="Duplicate branch in 'switch' statement">bar("A");</weak_warning>
+        break;
+    }
+  }
+  void bar(String s){}
+}

java/java-tests/testData/inspection/duplicateBranchesInSwitch/Throw.java

@@ -0,0 +1,13 @@
+class C {
+  String foo(int n) {
+    switch (n) {
+      case 1:
+        <weak_warning descr="Duplicate branch in 'switch' statement">throw new IllegalArgumentException("A");</weak_warning>
+      case 2:
+        throw new IllegalStateException("A");
+      case 3:
+        <weak_warning descr="Duplicate branch in 'switch' statement">throw new IllegalArgumentException("A");</weak_warning>
+    }
+    return "";
+  }
+}

java/java-tests/testData/inspection/duplicateBranchesInSwitch/TwoCaseLabels.java

@@ -0,0 +1,17 @@
+class C {
+  void foo(int n) {
+    switch (n) {
+      case 1:
+      case 2:
+        <weak_warning descr="Duplicate branch in 'switch' statement">bar("A");</weak_warning>
+        break;
+      case 3:
+        <weak_warning descr="Duplicate branch in 'switch' statement">bar("A");</weak_warning>
+        break;
+      case 4:
+        bar("B");
+        break;
+    }
+  }
+  void bar(String s){}
+}

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")
+  }
+}

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
+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

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) {