From 5c182bf05eaad2478eb188fbe987908ca79bda8d Mon Sep 17 00:00:00 2001 From: Mikhail Pyltsin Date: Tue, 16 Sep 2025 16:32:36 +0200 Subject: [PATCH] [java-inspection] IDEA-379192 Type pattern switch statements: IDEA code inspection fails to consider inheritance GitOrigin-RevId: fdb6c97e5b7501a3e582c9764a5eb62d2d2e1467 --- .../DuplicateBranchesInSwitchInspection.java | 76 ++++++++++++++----- .../DefaultWithDominated.java | 43 +++++++++++ .../DuplicateBranchesInEnhancedSwitchTest.kt | 1 + 3 files changed, 103 insertions(+), 17 deletions(-) create mode 100644 java/java-tests/testData/inspection/duplicateBranchesInEnhancedSwitch/DefaultWithDominated.java diff --git a/java/java-impl-inspections/src/com/intellij/codeInspection/DuplicateBranchesInSwitchInspection.java b/java/java-impl-inspections/src/com/intellij/codeInspection/DuplicateBranchesInSwitchInspection.java index 36113cf2fc1b..9e07521f4a3a 100644 --- a/java/java-impl-inspections/src/com/intellij/codeInspection/DuplicateBranchesInSwitchInspection.java +++ b/java/java-impl-inspections/src/com/intellij/codeInspection/DuplicateBranchesInSwitchInspection.java @@ -3,6 +3,7 @@ package com.intellij.codeInspection; import com.intellij.codeInspection.util.InspectionMessage; import com.intellij.java.JavaBundle; +import com.intellij.java.codeserver.core.JavaPsiSwitchUtil; import com.intellij.java.syntax.parser.JavaKeywords; import com.intellij.modcommand.ModPsiUpdater; import com.intellij.modcommand.PsiUpdateModCommandQuickFix; @@ -179,7 +180,7 @@ public final class DuplicateBranchesInSwitchInspection extends LocalInspectionTo if (body != null) { //noinspection AssignmentToForLoopParameter element = collectCommentsUntilNewLine(commentTexts, element); - Rule rule = new Rule(ruleStatement, body, ArrayUtilRt.toStringArray(commentTexts)); + Rule rule = new Rule(ruleStatement, ruleStatement, body, ArrayUtilRt.toStringArray(commentTexts)); commentTexts.clear(); rulesByHash.computeIfAbsent(rule.hash(), __ -> new ArrayList<>()).add(rule); } @@ -249,11 +250,12 @@ public final class DuplicateBranchesInSwitchInspection extends LocalInspectionTo if (statementList == null || statementList.isEmpty()) { return previousBranch; } - PsiSwitchLabelStatement[] labels = Branch.collectLabels(statementList.get(0)); + PsiStatement first = statementList.getFirst(); + PsiSwitchLabelStatement[] labels = Branch.collectLabels(first); if (labels.length == 0) { return previousBranch; // the code without a label is not allowed in 'switch', just ignore it } - Branch branch = new Branch(labels, statementList, hasImplicitBreak, comments.fetchTexts()); + Branch branch = new Branch(first, labels, statementList, hasImplicitBreak, comments.fetchTexts()); if (!branch.canFallThrough() && (previousBranch == null || !previousBranch.canFallThrough())) { int hash = branch.hash(); List branches = branchesByHash.get(hash); @@ -447,13 +449,13 @@ public final class DuplicateBranchesInSwitchInspection extends LocalInspectionTo myNextFromLabelToMergeWith = PsiTreeUtil.skipWhitespacesForward(myLabelToMergeWith); - myCommentsToMergeWith = Collections.unmodifiableSet(new HashSet<>(Arrays.asList(myBranchToMergeWith.myCommentTexts))); + myCommentsToMergeWith = Collections.unmodifiableSet(ContainerUtil.newHashSet(myBranchToMergeWith.myCommentTexts)); return true; } void moveBranchLabel() { - PsiElement firstElementToMove = myBranchPrefixToMove.get(0); - PsiElement lastElementToMove = myBranchPrefixToMove.get(myBranchPrefixToMove.size() - 1); + PsiElement firstElementToMove = myBranchPrefixToMove.getFirst(); + PsiElement lastElementToMove = myBranchPrefixToMove.getLast(); PsiElement moveTarget = myLabelToMergeWith; if (myLabelToMergeWith.isDefaultCase()) { @@ -598,11 +600,12 @@ public final class DuplicateBranchesInSwitchInspection extends LocalInspectionTo private final boolean myIsDefault; private DuplicatesFinder myFinder; protected final boolean myCanDeleteRedundantBranch; + private final PsiElement myContext; - BranchBase(T @NotNull [] labels, PsiStatement @NotNull [] statements, String @NotNull [] commentTexts) { + BranchBase(@NotNull PsiElement context, T @NotNull [] labels, PsiStatement @NotNull [] statements, String @NotNull [] commentTexts) { LOG.assertTrue(labels.length != 0, "labels.length"); LOG.assertTrue(statements.length != 0, "statements.length"); - + myContext = context; myFirstLabel = labels[0]; myStatements = statements; myCommentTexts = commentTexts; @@ -635,9 +638,8 @@ public final class DuplicateBranchesInSwitchInspection extends LocalInspectionTo abstract PsiSwitchLabelStatementBase[] getLabels(); private boolean canBeJoinedWith(@NotNull BranchBase other) { - //for default branch, it is always possible to delete another branch. - //see also BranchBase.mergeWithDefaultFix - if (isDefault() || other.isDefault()) return true; + if (isDefault() && !hasDominanceAfter(other, this)) return true; + if (other.isDefault() && !hasDominanceAfter(this, other)) return true; //fast exist if (!this.canMergeBranch()) return false; @@ -688,6 +690,40 @@ public final class DuplicateBranchesInSwitchInspection extends LocalInspectionTo PsiTreeUtil.findSiblingForward(thisLabels[thisLabels.length - 1], baseElementType, true, null) == otherLabels[0]; } + private static boolean hasDominanceAfter(@NotNull BranchBase branch, + @NotNull BranchBase other) { + PsiElement context = branch.myContext; + PsiSwitchLabelStatementBase overWhom = PsiTreeUtil.getParentOfType(branch.myContext, PsiSwitchLabelStatementBase.class, false); + if (overWhom == null) return false; + PsiCaseLabelElementList whomCaseLabelElementList = overWhom.getCaseLabelElementList(); + if (whomCaseLabelElementList == null) return false; + PsiSwitchLabelStatementBase until = PsiTreeUtil.getParentOfType(other.myContext, PsiSwitchLabelStatementBase.class, false); + PsiSwitchBlock switchBlock = PsiTreeUtil.getParentOfType(context, PsiSwitchBlock.class, false); + if (switchBlock == null) return false; + boolean start = false; + PsiExpression switchBlockExpression = switchBlock.getExpression(); + if (switchBlockExpression == null) return false; + PsiType expressionType = switchBlockExpression.getType(); + if (expressionType == null) return false; + for (PsiElement switchBranch : JavaPsiSwitchUtil.getSwitchBranches(switchBlock)) { + if (!start) { + if (PsiTreeUtil.isAncestor(context, switchBranch, false)) { + start = true; + } + continue; + } + if (PsiTreeUtil.isAncestor(until, switchBranch, false)) { + return false; + } + if (ContainerUtil.or(whomCaseLabelElementList.getElements(), + element -> + JavaPsiSwitchUtil.isDominated(element, switchBranch, expressionType))) { + return true; + } + } + return false; + } + int effectiveLength() { return myStatements.length == 1 && myStatements[0] instanceof PsiBlockStatement blockStatement ? blockStatement.getCodeBlock().getStatementCount() @@ -836,11 +872,12 @@ public final class DuplicateBranchesInSwitchInspection extends LocalInspectionTo private final boolean myCanCopyCaseValues; private final PsiSwitchLabelStatement @NotNull [] myLabels; - Branch(PsiSwitchLabelStatement @NotNull [] labels, + Branch(@NotNull PsiElement context, + PsiSwitchLabelStatement @NotNull [] labels, @NotNull List statementList, boolean hasImplicitBreak, String @NotNull [] comments) { - super(labels, statementsWithoutTrailingBreak(statementList), comments); + super(context, labels, statementsWithoutTrailingBreak(statementList), comments); int lastIndex = statementList.size() - 1; PsiStatement lastStatement = statementList.get(lastIndex); @@ -989,7 +1026,8 @@ public final class DuplicateBranchesInSwitchInspection extends LocalInspectionTo return false; } - private static PsiSwitchLabelStatement[] collectLabels(PsiStatement statement) { + private static PsiSwitchLabelStatement[] collectLabels(@Nullable PsiStatement statement) { + if (statement == null) return EMPTY_LABELS_ARRAY; List labels = new ArrayList<>(); for (PsiElement element = PsiTreeUtil.getPrevSiblingOfType(statement, PsiStatement.class); element instanceof PsiSwitchLabelStatement; @@ -1041,8 +1079,11 @@ public final class DuplicateBranchesInSwitchInspection extends LocalInspectionTo private final boolean myCanMergeBranches; private final boolean myCanMergeWithDefaultBranch; - Rule(@NotNull PsiSwitchLabeledRuleStatement rule, @NotNull PsiStatement body, String @NotNull [] commentTexts) { - super(new PsiSwitchLabeledRuleStatement[]{rule}, new PsiStatement[]{body}, commentTexts); + Rule(@NotNull PsiElement context, + @NotNull PsiSwitchLabeledRuleStatement rule, + @NotNull PsiStatement body, + String @NotNull [] commentTexts) { + super(context, new PsiSwitchLabeledRuleStatement[]{rule}, new PsiStatement[]{body}, commentTexts); myIsSimpleExit = body instanceof PsiExpressionStatement || body instanceof PsiThrowStatement; myCanMergeBranches = calculateCanMergeBranches(rule); myCanMergeWithDefaultBranch = SwitchUtils.isCaseNull(rule); @@ -1185,7 +1226,8 @@ public final class DuplicateBranchesInSwitchInspection extends LocalInspectionTo } } } - myCommentsToMergeWith = Collections.unmodifiableSet(new HashSet<>(Arrays.asList(myRuleToMergeWith.myCommentTexts))); + if(myRuleToMergeWith == null) return false; + myCommentsToMergeWith = Set.of(myRuleToMergeWith.myCommentTexts); return true; } diff --git a/java/java-tests/testData/inspection/duplicateBranchesInEnhancedSwitch/DefaultWithDominated.java b/java/java-tests/testData/inspection/duplicateBranchesInEnhancedSwitch/DefaultWithDominated.java new file mode 100644 index 000000000000..34525b60b9c9 --- /dev/null +++ b/java/java-tests/testData/inspection/duplicateBranchesInEnhancedSwitch/DefaultWithDominated.java @@ -0,0 +1,43 @@ +class C { + public Object convert(Object value) { + switch (value) { + case Integer ignored -> { + return value; + } + case Number v -> { + return v.toString(); + } + default -> { + return value; + } + } + } + + public Object convert2(Object value) { + switch (value) { + case Integer ignored -> { + return value; + } + case String v -> { + return v + "1"; + } + default -> { + return value; + } + } + } + + public Object convert3(Object value) { + switch (value) { + case String v -> { + return v + "1"; + } + case Integer ignored -> { + return value; + } + default -> { + return value; + } + } + } +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/DuplicateBranchesInEnhancedSwitchTest.kt b/java/java-tests/testSrc/com/intellij/java/codeInspection/DuplicateBranchesInEnhancedSwitchTest.kt index b42256ac2c97..d2657e2df412 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/DuplicateBranchesInEnhancedSwitchTest.kt +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DuplicateBranchesInEnhancedSwitchTest.kt @@ -39,6 +39,7 @@ class DuplicateBranchesInEnhancedSwitchTest : LightJavaCodeInsightFixtureTestCas fun testNullDuplicatesDefault() = IdeaTestUtil.withLevel(module, LanguageLevel.JDK_21) { doTest() } fun testMixedCases() = IdeaTestUtil.withLevel(module, LanguageLevel.JDK_21_PREVIEW) { doTest() } fun testDominatedUnnamedVariables() = IdeaTestUtil.withLevel(module, LanguageLevel.JDK_21_PREVIEW) { doTest() } + fun testDefaultWithDominated() = IdeaTestUtil.withLevel(module, LanguageLevel.JDK_25) { doTest() } fun testSkipUnnamedForPreviousVersions() = IdeaTestUtil.withLevel(module, LanguageLevel.JDK_21) { doTest() } private fun doTest() {