Java: Highlight identical branches in 'switch' statement, comment handling added (IDEA-181304)

2026-09-27 10:03:11 +07:00 · 2018-11-01 14:49:26 +03:00
parent b0bd16ee01
commit af32276ccf
6 changed files with 213 additions and 37 deletions
@@ -9,18 +9,22 @@ 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.psi.util.PsiUtil;
 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 com.intellij.util.ArrayUtil;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;

 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;

+import static com.siyeh.ig.migration.TryWithIdenticalCatchesInspection.collectCommentTexts;
+
 /**
 * @author Pavel.Dolgov
 */
@@ -79,22 +83,32 @@ public class DuplicateBranchesInSwitchInspection extends LocalInspectionTool {

    List<Branch> branches = new ArrayList<>();
    List<PsiStatement> statementList = null;
-    for (PsiStatement statement : body.getStatements()) {
-      if (statement instanceof PsiSwitchLabelStatement) {
+    Comments comments = new Comments();
+
+    for (PsiElement child = body.getFirstChild(); child != null; child = child.getNextSibling()) {
+      if (child instanceof PsiSwitchLabelStatement) {
+        PsiSwitchLabelStatement switchLabel = (PsiSwitchLabelStatement)child;
        if (statementList != null) {
-          branches.add(new Branch(statementList, hasImplicitBreak(statement)));
+          branches.add(new Branch(statementList, hasImplicitBreak(switchLabel), comments.fetchTexts()));
          statementList = null;
        }
-        continue;
+        comments.addFrom(switchLabel);
      }
-      if (statementList == null) {
-        if (isIgnoredSingleStatement(statement)) continue; // trivial duplicate branches are probably OK
-        statementList = new ArrayList<>();
+      else if (child instanceof PsiStatement) {
+        PsiStatement statement = (PsiStatement)child;
+        if (statementList == null) {
+          statementList = new ArrayList<>();
+        }
+        statementList.add(statement);
+        comments.addFrom(statement);
+      }
+      else {
+        comments.addPending(child);
      }
-      statementList.add(statement);
    }
+
    if (statementList != null) {
-      branches.add(new Branch(statementList, true));
+      branches.add(new Branch(statementList, true, comments.fetchTexts()));
    }
    return branches;
  }
@@ -102,7 +116,8 @@ public class DuplicateBranchesInSwitchInspection extends LocalInspectionTool {
  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()) {
+    if (branch.canFallThrough() || otherBranch.canFallThrough() ||
+        branch.isSimpleExit() != otherBranch.isSimpleExit()) {
      return false;
    }

@@ -110,6 +125,9 @@ public class DuplicateBranchesInSwitchInspection extends LocalInspectionTool {
    if (match != null) {
      Match otherMatch = otherBranch.match(branch);
      if (otherMatch != null) {
+        if (branch.isSimpleExit() && otherBranch.isSimpleExit() && !Arrays.equals(branch.myCommentTexts, otherBranch.myCommentTexts)) {
+          return false;
+        }
        return ReturnValue.areEquivalent(match.getReturnValue(), otherMatch.getReturnValue());
      }
    }
@@ -127,24 +145,15 @@ public class DuplicateBranchesInSwitchInspection extends LocalInspectionTool {
    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 final String[] myCommentTexts;
+    private final boolean myIsSimpleExit;

    private DuplicatesFinder myFinder;
    private Boolean myCanFallThrough;

-    Branch(@NotNull List<PsiStatement> statementList, boolean hasImplicitBreak) {
+    Branch(@NotNull List<PsiStatement> statementList, boolean hasImplicitBreak, String[] commentTexts) {
      int lastIndex = statementList.size() - 1;
      PsiStatement lastStatement = statementList.get(lastIndex);
      if (hasImplicitBreak ||
@@ -154,10 +163,12 @@ public class DuplicateBranchesInSwitchInspection extends LocalInspectionTool {
          lastStatement instanceof PsiThrowStatement) {
        myCanFallThrough = false; // in more complex cases it will be computed lazily
      }
+      myIsSimpleExit = lastIndex == 0 && isSimpleExit(lastStatement);
      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);
+      myCommentTexts = commentTexts;
    }

    @Nullable
@@ -172,6 +183,10 @@ public class DuplicateBranchesInSwitchInspection extends LocalInspectionTool {
      return myCanFallThrough;
    }

+    boolean isSimpleExit() {
+      return myIsSimpleExit;
+    }
+
    @NotNull
    private DuplicatesFinder getFinder() {
      if (myFinder == null) {
@@ -206,5 +221,57 @@ public class DuplicateBranchesInSwitchInspection extends LocalInspectionTool {
      }
      return true;
    }
+
+    private static boolean isSimpleExit(@Nullable PsiStatement statement) {
+      if (statement instanceof PsiBreakStatement ||
+          statement instanceof PsiContinueStatement ||
+          statement instanceof PsiThrowStatement) {
+        return true;
+      }
+      if (statement instanceof PsiReturnStatement) {
+        return isSimpleExpression(((PsiReturnStatement)statement).getReturnValue());
+      }
+      return false;
+    }
+
+    private static boolean isSimpleExpression(@Nullable PsiExpression expression) {
+      expression = PsiUtil.deparenthesizeExpression(expression);
+      if (expression == null || expression instanceof PsiLiteralExpression) {
+        return true;
+      }
+      if (expression instanceof PsiReferenceExpression) {
+        PsiExpression qualifier = ((PsiReferenceExpression)expression).getQualifierExpression();
+        return qualifier == null || qualifier instanceof PsiQualifiedExpression;
+      }
+      if (expression instanceof PsiUnaryExpression) {
+        return isSimpleExpression(((PsiUnaryExpression)expression).getOperand());
+      }
+      return false;
+    }
+  }
+
+  private static class Comments {
+    private final List<String> myTexts = new ArrayList<>();
+    private final List<PsiElement> myPending = new ArrayList<>();
+
+    String[] fetchTexts() {
+      String[] result = ArrayUtil.toStringArray(myTexts);
+      myTexts.clear();
+      return result;
+    }
+
+    void addFrom(PsiStatement statement) {
+      // The comments followed by a switch label are attached to that switch label.
+      // They're pending until we know if the next statement is a label or not.
+      for (PsiElement pending : myPending) {
+        collectCommentTexts(pending, myTexts);
+      }
+      myPending.clear();
+      collectCommentTexts(statement, myTexts);
+    }
+
+    public void addPending(PsiElement element) {
+      myPending.add(element);
+    }
  }
 }
@@ -0,0 +1,19 @@
+enum T {
+  A, B, C;
+
+  int foo(T t) {
+    switch (t) {
+      case A:
+        <weak_warning descr="Duplicate branch in 'switch' statement">return t.ordinal(); // comment 1</weak_warning>
+
+      case B:
+        <weak_warning descr="Duplicate branch in 'switch' statement">return t.ordinal();</weak_warning>
+
+      case C:
+        <weak_warning descr="Duplicate branch in 'switch' statement">return t.ordinal(); // comment 2</weak_warning>
+
+      default:
+        return 0;
+    }
+  }
+}
@@ -0,0 +1,37 @@
+enum C {
+  ORIGINAL_CODE_WITH_COMMENT, THE_SAME_CODE_WITH_DIFFERENT_COMMENT,COMMENT_INSIDE_THE_CODE,LINE_COMMENT,
+  JAVADOC_COMMENT,COMMENT_WITH_NEW_LINES,EMPTY_COMMENTS_ARE_IGNORED,
+  COMMENT_RIGHT_BEFORE_A_CASE_IS_ATTACHED_TO_THAT_CASE;
+
+  String foo(C c) {
+    switch (c) {
+      case ORIGINAL_CODE_WITH_COMMENT:
+        /* comment 1 */
+        <weak_warning descr="Duplicate branch in 'switch' statement">return "A";</weak_warning>
+      case THE_SAME_CODE_WITH_DIFFERENT_COMMENT:
+        /* comment 2 */
+        return "A";
+      case LINE_COMMENT:
+        // comment 1
+        <weak_warning descr="Duplicate branch in 'switch' statement">return "A";</weak_warning>
+      case COMMENT_INSIDE_THE_CODE:
+        <weak_warning descr="Duplicate branch in 'switch' statement">return /* comment 1 */"A";</weak_warning>
+      case JAVADOC_COMMENT:
+        /** comment 1 */
+        <weak_warning descr="Duplicate branch in 'switch' statement">return "A";</weak_warning>
+      case COMMENT_WITH_NEW_LINES:
+        /*
+        comment 1
+        */
+        <weak_warning descr="Duplicate branch in 'switch' statement">return "A";</weak_warning>
+      case EMPTY_COMMENTS_ARE_IGNORED:
+        /* comment 1 */
+        //
+        <weak_warning descr="Duplicate branch in 'switch' statement">return "A";</weak_warning>
+      // comment 1
+      case COMMENT_RIGHT_BEFORE_A_CASE_IS_ATTACHED_TO_THAT_CASE:
+        <weak_warning descr="Duplicate branch in 'switch' statement">return "A";</weak_warning>
+    }
+    return "";
+  }
+}
@@ -0,0 +1,19 @@
+enum T {
+  A, B, C;
+
+  int foo(T t) {
+    switch (t) {
+      case A:
+        return -1; // comment 1
+
+      case B:
+        return 1;
+
+      case C:
+        return -1; // comment 2
+
+      default:
+        return 0;
+    }
+  }
+}
@@ -31,6 +31,9 @@ class DuplicateBranchesInSwitchTest : LightCodeInsightFixtureTestCase() {
  fun testComplexBranches() = doTest()
  fun testBreakWithLabel() = doTest()
  fun testBreakAndReturnUnderIf() = doTest()
+  fun testReturnWithComments() = doTest()
+  fun testUnaryMinusInReturn() = doTest()
+  fun testMethodCallInReturn() = doTest()

  private fun doTest() {
    myFixture.testHighlighting("${getTestName(false)}.java")
@@ -18,10 +18,11 @@ package com.siyeh.ig.migration;
 import com.intellij.codeInspection.ProblemDescriptor;
 import com.intellij.codeInspection.ProblemHighlightType;
 import com.intellij.openapi.project.Project;
-import com.intellij.openapi.util.text.StringUtil;
 import com.intellij.pom.java.JavaFeature;
 import com.intellij.psi.*;
 import com.intellij.psi.codeStyle.JavaCodeStyleManager;
+import com.intellij.psi.impl.source.tree.LeafPsiElement;
+import com.intellij.psi.javadoc.PsiDocComment;
 import com.intellij.psi.search.LocalSearchScope;
 import com.intellij.psi.tree.IElementType;
 import com.intellij.psi.util.PsiTreeUtil;
@@ -356,31 +357,61 @@ public class TryWithIdenticalCatchesInspection extends BaseInspection {
    }
  }

-  @NotNull
-  private static List<String> collectCommentTexts(@NotNull PsiElement element) {
-    final List<String> result = new ArrayList<>();
+  public static void collectCommentTexts(@NotNull PsiElement element, @NotNull Collection<String> result) {
+    if (element instanceof PsiComment) {
+      addCommentText(result, (PsiComment)element);
+      return;
+    }
+    if (element instanceof LeafPsiElement) {
+      return; // optimization
+    }
    PsiTreeUtil.processElements(element, child -> {
      if (child instanceof PsiComment) {
-        String text = getCommentText((PsiComment)child);
-        if (!text.isEmpty()) {
-          result.add(text);
-        }
+        addCommentText(result, (PsiComment)child);
      }
      return true;
    });
+  }
+
+  private static void addCommentText(@NotNull Collection<String> result, PsiComment child) {
+    String text = getCommentText(child);
+    if (!text.isEmpty()) {
+      result.add(text);
+    }
+  }
+
+  @NotNull
+  private static List<String> collectCommentTexts(@NotNull PsiElement element) {
+    final List<String> result = new ArrayList<>();
+    collectCommentTexts(element, result);
    return result;
  }

  @NotNull
-  private static String getCommentText(@NotNull PsiComment comment) {
+  public static String getCommentText(@NotNull PsiComment comment) {
    final IElementType type = comment.getTokenType();
-    if (type == JavaTokenType.END_OF_LINE_COMMENT) {
-      return StringUtil.trimStart(comment.getText(), "//").trim();
+    final String text = comment.getText();
+    int start = 0, end = text.length();
+
+    if (comment instanceof PsiDocComment) {
+      if (text.startsWith("/**")) start += "/**".length();
+      if (text.endsWith("*/")) end -= "*/".length();
    }
-    if (type == JavaTokenType.C_STYLE_COMMENT) {
-      return StringUtil.trimStart(StringUtil.trimEnd(comment.getText(), "*/"), "/*").trim();
+    else if (type == JavaTokenType.C_STYLE_COMMENT) {
+      if (text.startsWith("/*")) start += "/*".length();
+      if (text.endsWith("*/")) end -= "*/".length();
    }
-    return "";
+    else if (type == JavaTokenType.END_OF_LINE_COMMENT) {
+      if (text.startsWith("//")) start += "//".length();
+    }
+
+    while (start < end && Character.isWhitespace(text.charAt(start))) {
+      start++;
+    }
+    while (start < end - 1 && Character.isWhitespace(text.charAt(end - 1))) {
+      end--;
+    }
+    return start < end ? text.substring(start, end) : "";
  }

  @Override