From af32276ccf5585a4c51e9f470117ef6bbbeb5d05 Mon Sep 17 00:00:00 2001 From: Pavel Dolgov Date: Wed, 31 Oct 2018 15:25:13 +0300 Subject: [PATCH] Java: Highlight identical branches in 'switch' statement, comment handling added (IDEA-181304) --- .../DuplicateBranchesInSwitchInspection.java | 113 ++++++++++++++---- .../MethodCallInReturn.java | 19 +++ .../ReturnWithComments.java | 37 ++++++ .../UnaryMinusInReturn.java | 19 +++ .../DuplicateBranchesInSwitchTest.kt | 3 + .../TryWithIdenticalCatchesInspection.java | 59 ++++++--- 6 files changed, 213 insertions(+), 37 deletions(-) create mode 100644 java/java-tests/testData/inspection/duplicateBranchesInSwitch/MethodCallInReturn.java create mode 100644 java/java-tests/testData/inspection/duplicateBranchesInSwitch/ReturnWithComments.java create mode 100644 java/java-tests/testData/inspection/duplicateBranchesInSwitch/UnaryMinusInReturn.java diff --git a/java/java-impl/src/com/intellij/codeInspection/DuplicateBranchesInSwitchInspection.java b/java/java-impl/src/com/intellij/codeInspection/DuplicateBranchesInSwitchInspection.java index 792c094b6248..cb802872f6d4 100644 --- a/java/java-impl/src/com/intellij/codeInspection/DuplicateBranchesInSwitchInspection.java +++ b/java/java-impl/src/com/intellij/codeInspection/DuplicateBranchesInSwitchInspection.java @@ -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 branches = new ArrayList<>(); List 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 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 statementList, boolean hasImplicitBreak) { + Branch(@NotNull List 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 myTexts = new ArrayList<>(); + private final List 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); + } } } diff --git a/java/java-tests/testData/inspection/duplicateBranchesInSwitch/MethodCallInReturn.java b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/MethodCallInReturn.java new file mode 100644 index 000000000000..750a0039a93b --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/MethodCallInReturn.java @@ -0,0 +1,19 @@ +enum T { + A, B, C; + + int foo(T t) { + switch (t) { + case A: + return t.ordinal(); // comment 1 + + case B: + return t.ordinal(); + + case C: + return t.ordinal(); // comment 2 + + default: + return 0; + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateBranchesInSwitch/ReturnWithComments.java b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/ReturnWithComments.java new file mode 100644 index 000000000000..3a3c7b7a4b31 --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/ReturnWithComments.java @@ -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 */ + return "A"; + case THE_SAME_CODE_WITH_DIFFERENT_COMMENT: + /* comment 2 */ + return "A"; + case LINE_COMMENT: + // comment 1 + return "A"; + case COMMENT_INSIDE_THE_CODE: + return /* comment 1 */"A"; + case JAVADOC_COMMENT: + /** comment 1 */ + return "A"; + case COMMENT_WITH_NEW_LINES: + /* + comment 1 + */ + return "A"; + case EMPTY_COMMENTS_ARE_IGNORED: + /* comment 1 */ + // + return "A"; + // comment 1 + case COMMENT_RIGHT_BEFORE_A_CASE_IS_ATTACHED_TO_THAT_CASE: + return "A"; + } + return ""; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/duplicateBranchesInSwitch/UnaryMinusInReturn.java b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/UnaryMinusInReturn.java new file mode 100644 index 000000000000..070529b62c88 --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateBranchesInSwitch/UnaryMinusInReturn.java @@ -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; + } + } +} \ 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 index ae7cc33ba1a9..c7be0dee182a 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/DuplicateBranchesInSwitchTest.kt +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DuplicateBranchesInSwitchTest.kt @@ -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") 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 06fe61d395fa..694e7bd6d84c 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/migration/TryWithIdenticalCatchesInspection.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/migration/TryWithIdenticalCatchesInspection.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 collectCommentTexts(@NotNull PsiElement element) { - final List result = new ArrayList<>(); + public static void collectCommentTexts(@NotNull PsiElement element, @NotNull Collection 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 result, PsiComment child) { + String text = getCommentText(child); + if (!text.isEmpty()) { + result.add(text); + } + } + + @NotNull + private static List collectCommentTexts(@NotNull PsiElement element) { + final List 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