diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/IfChainToSwitch.java b/core/src/main/java/com/google/errorprone/bugpatterns/IfChainToSwitch.java index eda11c1a1fd..9d45b7e1e6b 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/IfChainToSwitch.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/IfChainToSwitch.java @@ -31,6 +31,7 @@ import static com.google.errorprone.util.ASTHelpers.isConsideredFinal; import static com.google.errorprone.util.ASTHelpers.isSubtype; import static com.google.errorprone.util.ASTHelpers.sameVariable; +import static com.google.errorprone.util.ASTHelpers.stripParentheses; import static com.sun.source.tree.Tree.Kind.EXPRESSION_STATEMENT; import static com.sun.source.tree.Tree.Kind.THROW; import static java.lang.Math.max; @@ -418,6 +419,28 @@ private static boolean switchOnNullWouldImplicitlyThrow( && cases.stream().noneMatch(CaseIr::hasCaseNull); } + /** + * Determines whether the given case IR has an "unguarded" pattern. As defined in the JLS, + * "unguarded" means that either there is no guard or the guard is the boolean literal `true`. + */ + private static boolean isUnguarded(CaseIr caseIr) { + // Not a pattern + if (caseIr.instanceOfOptional().isEmpty()) { + return false; + } + + if (caseIr.guardOptional().isEmpty()) { + return true; + } + + // Guard is present and is `true` + ExpressionTree guard = stripParentheses(caseIr.guardOptional().get()); + return isBooleanLiteral(guard) + && guard instanceof LiteralTree literalTree + && literalTree.getValue() instanceof Boolean b + && b; + } + /** * Analyzes the supplied case IRs for a switch statement for issues related default/unconditional * cases. If deemed necessary, this method injects a `default` and/or `case null` into the @@ -452,7 +475,7 @@ private Optional> maybeFixDefaultNullAndUnconditional( .filter( caseIr -> caseIr.instanceOfOptional().isPresent() - && caseIr.guardOptional().isEmpty() + && isUnguarded(caseIr) && isSubtype( getType(subject), getType(caseIr.instanceOfOptional().get().type()), @@ -1673,7 +1696,7 @@ public static boolean isDominatedBy( boolean isPrimitive = getType(constantExpression).isPrimitive(); if (isPrimitive) { // Guarded patterns cannot dominate primitives - if (lhs.guardOptional().isPresent()) { + if (!isUnguarded(lhs)) { continue; } if (lhs.instanceOfOptional().isPresent()) { @@ -1705,7 +1728,7 @@ public static boolean isDominatedBy( } boolean isEnum = isEnumValue(constantExpression, state); if (isEnum) { - if (lhs.guardOptional().isPresent()) { + if (!isUnguarded(lhs)) { // Guarded patterns cannot dominate enum values continue; } @@ -1726,7 +1749,7 @@ public static boolean isDominatedBy( // RHS must be a reference // The rhs-reference code would be needed to support e.g. String literals. It is included // for completeness. - if (lhs.guardOptional().isPresent()) { + if (!isUnguarded(lhs)) { // Guarded patterns cannot dominate references continue; } @@ -1757,7 +1780,7 @@ public static boolean isDominatedBy( } // RHS must be a pattern - if (lhs.guardOptional().isPresent()) { + if (!isUnguarded(lhs)) { // LHS has a guard, so cannot dominate RHS return false; } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/IfChainToSwitchTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/IfChainToSwitchTest.java index d93434d7b40..b74c5296665 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/IfChainToSwitchTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/IfChainToSwitchTest.java @@ -3036,7 +3036,8 @@ public void foo(Suit s) { } @Test - public void ifChain_domination3_error() { + public void ifChain_conflictingUnguardedTypePatterns_noError() { + // Integer and Number are conflicting; both unconditional for Integer helper .addSourceLines( "Test.java", @@ -3067,7 +3068,7 @@ public void foo(Suit s) { } @Test - public void ifChain_domination4_noError() { + public void ifChain_conflictingUnguardedTypePatternsShort_noError() { // Number and Integer are conflicting (both unconditional for Integer) helper .addSourceLines( @@ -3093,7 +3094,7 @@ public void foo(Suit s) { } @Test - public void ifChain_domination5_noError() { + public void ifChain_dominationDefaultAndUnconditionalConflict_noError() { // Both a default and unconditional conflict helper .addSourceLines( @@ -3120,6 +3121,55 @@ public void foo(Suit s) { .doTest(); } + @Test + public void ifChain_dominationWithTrueGuard_error() { + // Guard of `true` must be treated as unguarded, causing case to be pulled up + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Integer i = s == null ? 0 : 1; + if (i == 0) { + System.out.println("It's 0!"); + } else if (i instanceof Integer o && o.hashCode() == 17) { + System.out.println("Its hashcode is 17"); + } else if (i instanceof Integer in && (true)) { + System.out.println("It's unguarded"); + } else if (i == 23) { + System.out.println("It's a 23"); + } else if (i instanceof Integer in && i > 348) { + System.out.println("It's a big integer!"); + } + } + } + """) + .addOutputLines( + "Test.java", +""" +import java.lang.Number; + +class Test { + public void foo(Suit s) { + Integer i = s == null ? 0 : 1; + switch (i) { + case 0 -> System.out.println("It's 0!"); + case Integer o when o.hashCode() == 17 -> System.out.println("Its hashcode is 17"); + case 23 -> System.out.println("It's a 23"); + case Integer in when i > 348 -> System.out.println("It's a big integer!"); + case Integer in when (true) -> System.out.println("It's unguarded"); + } + } +} +""") + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + @Test public void ifChain_javadocEnum_error() { refactoringHelper