From 8a308f5f90ec7619298b3012cef6c9e201f40e58 Mon Sep 17 00:00:00 2001 From: Pavel Dolgov Date: Tue, 6 Nov 2018 15:16:05 +0300 Subject: [PATCH] Java: Highlight identical branches in 'switch' statement, optimize duplicates matching (IDEA-181304) --- .../DuplicateBranchesInSwitchInspection.java | 120 ++++++++---------- .../DuplicateBranchesInSwitch.html | 1 + .../FallThroughBefore.java | 14 ++ .../DuplicateBranchesInSwitchTest.kt | 1 + 4 files changed, 71 insertions(+), 65 deletions(-) create mode 100644 java/java-tests/testData/inspection/duplicateBranchesInSwitch/FallThroughBefore.java diff --git a/java/java-impl/src/com/intellij/codeInspection/DuplicateBranchesInSwitchInspection.java b/java/java-impl/src/com/intellij/codeInspection/DuplicateBranchesInSwitchInspection.java index cb802872f6d4..20857d8958c7 100644 --- a/java/java-impl/src/com/intellij/codeInspection/DuplicateBranchesInSwitchInspection.java +++ b/java/java-impl/src/com/intellij/codeInspection/DuplicateBranchesInSwitchInspection.java @@ -1,12 +1,8 @@ // 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.psi.util.PsiUtil; @@ -15,13 +11,11 @@ import com.intellij.refactoring.util.duplicates.DuplicatesFinder; import com.intellij.refactoring.util.duplicates.Match; import com.intellij.refactoring.util.duplicates.ReturnValue; import com.intellij.util.ArrayUtil; +import com.siyeh.ig.psiutils.ControlFlowUtils; 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 java.util.*; import static com.siyeh.ig.migration.TryWithIdenticalCatchesInspection.collectCommentTexts; @@ -45,21 +39,29 @@ public class DuplicateBranchesInSwitchInspection extends LocalInspectionTool { public void visitSwitchStatement(PsiSwitchStatement switchStatement) { super.visitSwitchStatement(switchStatement); - List branches = collectBranches(switchStatement); + for (List branches : collectSameLengthBranches(switchStatement)) { + registerProblems(branches); + } + } + + void registerProblems(List branches) { 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 index = 0; index < size - 1; index++) { + if (isDuplicate[index]) continue; - for (int j = i + 1; j < size; j++) { - if (areDuplicates(branches, i, j)) { - isDuplicate[j] = true; - registerProblem(branches.get(j).myStatements); + for (int otherIndex = index + 1; otherIndex < size; otherIndex++) { + Branch branch = branches.get(index); + Branch otherBranch = branches.get(otherIndex); - if (!isDuplicate[i]) { - isDuplicate[i] = true; - registerProblem(branches.get(i).myStatements); + if (areDuplicates(branch, otherBranch)) { + isDuplicate[otherIndex] = true; + registerProblem(otherBranch.myStatements); + + if (!isDuplicate[index]) { + isDuplicate[index] = true; + registerProblem(branch.myStatements); } } } @@ -77,21 +79,21 @@ public class DuplicateBranchesInSwitchInspection extends LocalInspectionTool { } @NotNull - static List collectBranches(@NotNull PsiSwitchStatement switchStatement) { + static Collection> collectSameLengthBranches(@NotNull PsiSwitchStatement switchStatement) { PsiCodeBlock body = switchStatement.getBody(); if (body == null) return Collections.emptyList(); - List branches = new ArrayList<>(); List statementList = null; Comments comments = new Comments(); + Branch previousBranch = null; + Map> branchesByLength = new HashMap<>(); 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(switchLabel), comments.fetchTexts())); - statementList = null; - } + previousBranch = addBranchToMap(branchesByLength, statementList, hasImplicitBreak(switchLabel), comments, previousBranch); + + statementList = null; comments.addFrom(switchLabel); } else if (child instanceof PsiStatement) { @@ -107,17 +109,30 @@ public class DuplicateBranchesInSwitchInspection extends LocalInspectionTool { } } - if (statementList != null) { - branches.add(new Branch(statementList, true, comments.fetchTexts())); - } - return branches; + addBranchToMap(branchesByLength, statementList, true, comments, previousBranch); + return branchesByLength.values(); } - static boolean areDuplicates(List branches, int index, int otherIndex) { - Branch branch = branches.get(index); - Branch otherBranch = branches.get(otherIndex); - if (branch.canFallThrough() || otherBranch.canFallThrough() || - branch.isSimpleExit() != otherBranch.isSimpleExit()) { + @Nullable + private static Branch addBranchToMap(@NotNull Map> branchesByLength, + @Nullable List statementList, + boolean hasImplicitBreak, + @NotNull Comments comments, + @Nullable Branch previousBranch) { + if (statementList == null || statementList.isEmpty()) { + return previousBranch; + } + Branch branch = new Branch(statementList, hasImplicitBreak, comments.fetchTexts()); + if (previousBranch == null || !previousBranch.canFallThrough()) { + List branches = branchesByLength.computeIfAbsent(branch.length(), unused -> new ArrayList<>()); + branches.add(branch); + } + return branch; + } + + static boolean areDuplicates(Branch branch, Branch otherBranch) { + if (branch.isSimpleExit() != otherBranch.isSimpleExit() || + branch.canFallThrough() || otherBranch.canFallThrough()) { return false; } @@ -149,20 +164,14 @@ public class DuplicateBranchesInSwitchInspection extends LocalInspectionTool { private final PsiStatement[] myStatements; private final String[] myCommentTexts; private final boolean myIsSimpleExit; + private final boolean myCanFallThrough; private DuplicatesFinder myFinder; - private Boolean myCanFallThrough; - Branch(@NotNull List statementList, boolean hasImplicitBreak, String[] commentTexts) { + Branch(@NotNull List statementList, boolean hasImplicitBreak, @NotNull String[] commentTexts) { 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 - } + myCanFallThrough = !hasImplicitBreak && ControlFlowUtils.statementMayCompleteNormally(lastStatement); myIsSimpleExit = lastIndex == 0 && isSimpleExit(lastStatement); if (lastIndex > 0 && isBreakWithoutLabel(lastStatement)) { statementList = statementList.subList(0, lastIndex); // trailing 'break' is already taken into account in myCanFallThrough @@ -177,9 +186,6 @@ public class DuplicateBranchesInSwitchInspection extends LocalInspectionTool { } boolean canFallThrough() { - if (myCanFallThrough == null) { - myCanFallThrough = calculateCanFallThrough(myStatements); - } return myCanFallThrough; } @@ -187,6 +193,10 @@ public class DuplicateBranchesInSwitchInspection extends LocalInspectionTool { return myIsSimpleExit; } + int length() { + return myStatements.length; + } + @NotNull private DuplicatesFinder getFinder() { if (myFinder == null) { @@ -202,26 +212,6 @@ public class DuplicateBranchesInSwitchInspection extends LocalInspectionTool { 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; - } - private static boolean isSimpleExit(@Nullable PsiStatement statement) { if (statement instanceof PsiBreakStatement || statement instanceof PsiContinueStatement || diff --git a/java/java-impl/src/inspectionDescriptions/DuplicateBranchesInSwitch.html b/java/java-impl/src/inspectionDescriptions/DuplicateBranchesInSwitch.html index 6874ea6cff7c..43125f8619af 100644 --- a/java/java-impl/src/inspectionDescriptions/DuplicateBranchesInSwitch.html +++ b/java/java-impl/src/inspectionDescriptions/DuplicateBranchesInSwitch.html @@ -2,5 +2,6 @@ Reports switch statements containing the same code in different branches. +

New in 2019.1

\ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateBranchesInSwitch/FallThroughBefore.java b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/FallThroughBefore.java new file mode 100644 index 000000000000..63a179e88b05 --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/FallThroughBefore.java @@ -0,0 +1,14 @@ +class C { + void foo(String s) { + switch (s) { + case "A": + bar(1); + break; + case "B": + bar(2); + case "C": + bar(1); + } + } + void bar(int n) {} +} diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/DuplicateBranchesInSwitchTest.kt b/java/java-tests/testSrc/com/intellij/java/codeInspection/DuplicateBranchesInSwitchTest.kt index c7be0dee182a..047f00213bbe 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/DuplicateBranchesInSwitchTest.kt +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DuplicateBranchesInSwitchTest.kt @@ -23,6 +23,7 @@ class DuplicateBranchesInSwitchTest : LightCodeInsightFixtureTestCase() { fun testThrow() = doTest() fun testContinue() = doTest() fun testFallThrough() = doTest() + fun testFallThroughBefore() = doTest() fun testAllFallThrough() = doTest() fun testNoLastBreak() = doTest() fun testFallThroughToBreak() = doTest()