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 be40384d69f..72ff7695c82 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/IfChainToSwitch.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/IfChainToSwitch.java @@ -101,11 +101,13 @@ enum Validity { } private final boolean enableMain; + private final boolean enableSafe; private final int maxChainLength; @Inject IfChainToSwitch(ErrorProneFlags flags) { enableMain = flags.getBoolean("IfChainToSwitch:EnableMain").orElse(false); + enableSafe = flags.getBoolean("IfChainToSwitch:EnableSafe").orElse(false); maxChainLength = flags.getInteger("IfChainToSwitch:MaxChainLength").orElse(50); } @@ -255,7 +257,9 @@ private static String prettyPrint( StringBuilder sb = new StringBuilder(); sb.append("switch (").append(state.getSourceForNode(subject)).append(") {\n"); for (CaseIr caseIr : cases) { - if (caseIr.hasCaseNull()) { + if (caseIr.hasCaseNull() && caseIr.hasDefault()) { + sb.append("case null, default"); + } else if (caseIr.hasCaseNull()) { sb.append("case null"); } else if (caseIr.hasDefault()) { sb.append("default"); @@ -399,13 +403,37 @@ private static String printRawTypesAsWildcards( return sb.toString(); } + /** + * Determines whether a {@code switch} having the given {@code subject} and {@code cases} would + * implicitly throw in the event that the {@code subject} is {@code null} at runtime. Here, + * implicitly throwing means that an exception would be thrown, and further that the {@code throw} + * would not be caused by logic in any of the supplied {@code cases}. (If the subject cannot be + * assigned {@code null}, returns {@code false}.) + */ + private static boolean switchOnNullWouldImplicitlyThrow( + ExpressionTree subject, List cases) { + Type type = getType(subject); + if (type.isPrimitive()) { + return false; + } + + // If there is an explicit `case null` already, then there can't be an implicit throw caused by + // null + boolean hasCaseNull = cases.stream().anyMatch(CaseIr::hasCaseNull); + if (hasCaseNull) { + return false; + } + + return true; + } + /** * Analyzes the supplied case IRs for a switch statement for issues related default/unconditional - * cases. If deemed necessary, this method injects a `default` case into the supplied case IRs. If - * the supplied case IRs cannot be used to form a syntactically valid switch statement, returns - * `Optional.empty()`. + * cases. If deemed necessary, this method injects a `default` and/or `case null` into the + * supplied case IRs. If the supplied case IRs cannot be used to form a syntactically valid switch + * statement, returns `Optional.empty()`. */ - private static Optional> maybeFixDefaultAndUnconditional( + private Optional> maybeFixDefaultNullAndUnconditional( List cases, ExpressionTree subject, StatementTree ifTree, @@ -417,6 +445,10 @@ private static Optional> maybeFixDefaultAndUnconditional( Range ifTreeSourceRange) { boolean hasDefault = cases.stream().anyMatch(CaseIr::hasDefault); + boolean hasCaseNull = cases.stream().anyMatch(CaseIr::hasCaseNull); + boolean recheckDominanceNeeded = false; + + boolean switchOnNullWouldImplicitlyThrow = switchOnNullWouldImplicitlyThrow(subject, cases); // Has an unconditional case, meaning that any non-null value of the subject will be matched long unconditionalCount = @@ -467,7 +499,7 @@ && isSubtype( .orElse(ifTreeSourceRange.lowerEndpoint()); casesCopy.add( new CaseIr( - /* hasCaseNull= */ false, + /* hasCaseNull= */ !hasCaseNull && (enableSafe && switchOnNullWouldImplicitlyThrow), /* hasDefault= */ true, /* instanceOfOptional= */ Optional.empty(), /* guardOptional= */ Optional.empty(), @@ -476,6 +508,45 @@ && isSubtype( /* caseSourceCodeRange= */ Range.closedOpen( previousCaseEndPosition, previousCaseEndPosition))); cases = casesCopy; + recheckDominanceNeeded = true; + } else if (enableSafe && !hasCaseNull && switchOnNullWouldImplicitlyThrow) { + List casesCopy = new ArrayList<>(cases); + if (hasDefault) { + // Upgrade existing `default` to `case null, default`. + casesCopy = + casesCopy.stream() + .map( + caseIr -> { + if (caseIr.hasDefault()) { + return new CaseIr( + /* hasCaseNull= */ true, + /* hasDefault= */ true, + /* instanceOfOptional= */ caseIr.instanceOfOptional(), + /* guardOptional= */ caseIr.guardOptional(), + /* expressionsOptional= */ caseIr.expressionsOptional(), + /* arrowRhsOptional= */ caseIr.arrowRhsOptional(), + /* caseSourceCodeRange= */ caseIr.caseSourceCodeRange()); + } + return caseIr; + }) + .collect(toImmutableList()); + recheckDominanceNeeded = true; + } else { + // Invariant: enableSafe=true + // Inject new empty `case null` + casesCopy.add( + new CaseIr( + /* hasCaseNull= */ true, + /* hasDefault= */ false, + /* instanceOfOptional= */ Optional.empty(), + /* guardOptional= */ Optional.empty(), + /* expressionsOptional= */ Optional.empty(), + /* arrowRhsOptional= */ Optional.empty(), + /* caseSourceCodeRange= */ Range.closedOpen( + ifTreeSourceRange.lowerEndpoint(), ifTreeSourceRange.lowerEndpoint()))); + recheckDominanceNeeded = true; + } + cases = casesCopy; } // Given any possible changes, is the code after the switch reachable? @@ -548,7 +619,7 @@ && isSubtype( } } - return Optional.of(cases); + return recheckDominanceNeeded ? maybeFixDominance(cases, state, subject) : Optional.of(cases); } /** @@ -558,7 +629,7 @@ && isSubtype( * original case IRs. Note that this method does not fully validate the resulting case IRs, but * rather only partially validates them with respect to pull-up. */ - private static Optional> maybePullUp( + private Optional> maybePullUp( List cases, VisitorState state, IfChainAnalysisState ifChainAnalysisState, @@ -570,6 +641,13 @@ private static Optional> maybePullUp( return Optional.empty(); } + boolean hasCaseNull = cases.stream().anyMatch(CaseIr::hasCaseNull); + if (hasCaseNull) { + // Don't support pulling up when there is already an explicit `case null`, since this + // could implicate duplication of the pulled-up statements into both `case null` and `default` + return Optional.empty(); + } + List casesCopy = new ArrayList<>(cases); if (ifChainAnalysisState.at().getElseStatement() == null @@ -584,7 +662,6 @@ private static Optional> maybePullUp( // Statements containing break or yield cannot be pulled up break; } - int startPos = casesCopy.isEmpty() ? ifTreeRange.lowerEndpoint() @@ -592,7 +669,9 @@ private static Optional> maybePullUp( int endPos = state.getEndPosition(statement); casesCopy.add( new CaseIr( - /* hasCaseNull= */ false, + /* hasCaseNull= */ enableSafe + && switchOnNullWouldImplicitlyThrow( + ifChainAnalysisState.subjectOptional().get(), cases), /* hasDefault= */ true, /* instanceOfOptional= */ Optional.empty(), /* guardOptional= */ Optional.empty(), @@ -1283,7 +1362,7 @@ private static Optional unboxed(Tree tree, VisitorState state) { } /** Performs a detailed analysis of the if-chain, generating suggested fixes as needed. */ - private static List deepAnalysisOfIfChain( + private List deepAnalysisOfIfChain( List cases, IfChainAnalysisState finalIfChainAnalysisState, IfTree ifTree, @@ -1322,7 +1401,7 @@ private static List deepAnalysisOfIfChain( .flatMap(caseList -> maybeFixDominance(caseList, state, subject)) .flatMap( x -> - maybeFixDefaultAndUnconditional( + maybeFixDefaultNullAndUnconditional( x, subject, ifTree, @@ -1344,7 +1423,7 @@ private static List deepAnalysisOfIfChain( .flatMap(caseList -> maybeFixDominance(caseList, state, subject)) .flatMap( caseList -> - maybeFixDefaultAndUnconditional( + maybeFixDefaultNullAndUnconditional( caseList, subject, ifTree, @@ -1353,7 +1432,9 @@ private static List deepAnalysisOfIfChain( /* numberPulledUp= */ 0, finalIfChainAnalysisState.handledEnumValues(), switchType, - ifTreeSourceRange)); + ifTreeSourceRange)) + // Changing default/null can affect dominance + .flatMap(caseList -> maybeFixDominance(caseList, state, subject)); } List suggestedFixes = new ArrayList<>(); 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 36c8aab0b8b..9ff8f59b319 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/IfChainToSwitchTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/IfChainToSwitchTest.java @@ -125,6 +125,8 @@ public void foo(Suit s) { return; } else if (suit instanceof Object) { throw new RuntimeException("It's an object!"); + } else if (suit == null) { + throw new RuntimeException("It's null!"); } // Farewell to this comment System.out.println("Delete me"); @@ -148,6 +150,7 @@ public void foo(Suit s) { return; } case Object unused -> throw new RuntimeException("It's an object!"); + case null -> throw new RuntimeException("It's null!"); } } } @@ -178,6 +181,8 @@ public void foo(Suit s) { return; } else if (suit instanceof Object) { throw new RuntimeException("It's an object!"); + } else if (suit == null) { + throw new RuntimeException("It's null!"); } // LINT.Something(...) @@ -202,6 +207,7 @@ public void foo(Suit s) { return; } case Object unused -> throw new RuntimeException("It's an object!"); + case null -> throw new RuntimeException("It's null!"); } // LINT.Something(...) @@ -365,6 +371,61 @@ public void foo(Suit s) { .doTest(TEXT_MATCH); } + @Test + public void ifChain_dontAlwaysPullUpSafe_error() { + // Pull up should not occur if it would result in a conflict + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + private Object suit; + + public void foo(Suit s) { + this.suit = null; + System.out.println("yo"); + if (this.suit instanceof String) { + System.out.println("It's a string!"); + } else if (suit instanceof Number) { + System.out.println("It's a number!"); + } else if (suit instanceof Suit) { + System.out.println("It's a Suit!"); + } else if (this.suit instanceof Object o) { + System.out.println("It's an object!"); + } + throw new AssertionError(); + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + private Object suit; + + public void foo(Suit s) { + this.suit = null; + System.out.println("yo"); + switch (suit) { + case String unused -> System.out.println("It's a string!"); + case Number unused -> System.out.println("It's a number!"); + case Suit unused -> System.out.println("It's a Suit!"); + case Object o -> System.out.println("It's an object!"); + case null -> {} + } + throw new AssertionError(); + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain", "-XepOpt:IfChainToSwitch:EnableSafe") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + @Test public void ifChain_nonexhausivePattern_noError() { // Pulling up the throw would change semantics @@ -416,6 +477,8 @@ public void foo(Suit s) { System.out.println("It's a number!"); } else if (suit instanceof Object) { System.out.println("It's an object!"); + } else if (suit == null) { + System.out.println("It's null!"); } System.out.println("Don't delete me!"); System.out.println("Don't delete me either!"); @@ -437,6 +500,7 @@ public void foo(Suit s) { case String unused -> System.out.println("It's a string!"); case Number unused -> System.out.println("It's a number!"); case Object unused -> System.out.println("It's an object!"); + case null -> System.out.println("It's null!"); } System.out.println("Don't delete me!"); System.out.println("Don't delete me either!"); @@ -501,6 +565,60 @@ public void foo(Suit s) { .doTest(TEXT_MATCH); } + @Test + public void ifChain_bindingPatternTreeSafe_error() { + // Note `instanceof Number n`, otherwise same as ifChain_twoLinesAfterDefault_error + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + private Object suit; + + public void foo(Suit s) { + this.suit = s; + System.out.println("yo"); + if (this.suit instanceof String) { + System.out.println("It's a string!"); + } else if (suit instanceof Number n) { + System.out.println("It's a number!"); + } else if (suit instanceof Object) { + System.out.println("It's an object!"); + } + System.out.println("Don't delete me!"); + System.out.println("Don't delete me either!"); + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + private Object suit; + + public void foo(Suit s) { + this.suit = s; + System.out.println("yo"); + switch (suit) { + case String unused -> System.out.println("It's a string!"); + case Number n -> System.out.println("It's a number!"); + case Object unused -> System.out.println("It's an object!"); + case null -> {} + } + System.out.println("Don't delete me!"); + System.out.println("Don't delete me either!"); + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain", "-XepOpt:IfChainToSwitch:EnableSafe") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + @Test public void ifChain_removesParens_error() { // Removes redundant parens around if condition @@ -547,6 +665,52 @@ public void foo(Suit s) { .doTest(TEXT_MATCH); } + @Test + public void ifChain_removesParensSafe_error() { + // Removes redundant parens around if condition + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + if (suit instanceof String) { + System.out.println("It's a string!"); + } else if ((suit instanceof Number)) { + System.out.println("It's a number!"); + } else if (suit instanceof Suit) { + System.out.println("It's a Suit!"); + } else throw new AssertionError(); + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + switch (suit) { + case String unused -> System.out.println("It's a string!"); + case Number unused -> System.out.println("It's a number!"); + case Suit unused -> System.out.println("It's a Suit!"); + case null, default -> throw new AssertionError(); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain", "-XepOpt:IfChainToSwitch:EnableSafe") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + @Test public void ifChain_equality_error() { // Enum equality @@ -596,6 +760,55 @@ public void foo(Suit s) { .doTest(TEXT_MATCH); } + @Test + public void ifChain_equalitySafe_error() { + // Enum equality + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + if (suit instanceof String) { + System.out.println("It's a string!"); + } else if (suit == Suit.DIAMOND) { + System.out.println("It's a diamond!"); + } else if ((suit instanceof Number)) { + System.out.println("It's a number!"); + } else if (suit instanceof Suit) { + System.out.println("It's a Suit!"); + } else throw new AssertionError(); + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + switch (suit) { + case String unused -> System.out.println("It's a string!"); + case Suit.DIAMOND -> System.out.println("It's a diamond!"); + case Number unused -> System.out.println("It's a number!"); + case Suit unused -> System.out.println("It's a Suit!"); + case null, default -> throw new AssertionError(); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain", "-XepOpt:IfChainToSwitch:EnableSafe") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + @Test public void ifChain_exhaustiveEnum_error() { // Enum equality @@ -640,6 +853,50 @@ public void foo(Suit suit) { .doTest(TEXT_MATCH); } + @Test + public void ifChain_exhaustiveEnumSafe_error() { + // Enum equality + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + public void foo(Suit suit) { + if (suit == Suit.CLUB) { + System.out.println("It's a club!"); + } else if (suit == Suit.DIAMOND) { + System.out.println("It's a diamond!"); + } else if ((suit == Suit.HEART)) { + System.out.println("It's a heart!"); + } else { // c1 + { + System.out.println("It's a diamond!"); + } + } + } + } + """) + .addOutputLines( + "Test.java", +""" +class Test { + public void foo(Suit suit) { + switch (suit) { + case Suit.CLUB -> System.out.println("It's a club!"); + case Suit.DIAMOND -> System.out.println("It's a diamond!"); + case Suit.HEART -> System.out.println("It's a heart!"); + case null, default -> + // c1 + System.out.println("It's a diamond!"); + } + } +} +""") + .setArgs("-XepOpt:IfChainToSwitch:EnableMain", "-XepOpt:IfChainToSwitch:EnableSafe") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + @Test public void ifChain_equalityNull_error() { // Equality with null @@ -954,10 +1211,11 @@ public void foo(Suit s) { } @Test - public void ifChain_dupeEnum_noError() { - // Duplicate enum - helper - .addSourceLines( + public void ifChain_legalDuplicateSafe_error() { + // Although the guard effectively duplicates the diamond constant case, this construction is + // legal + refactoringHelper + .addInputLines( "Test.java", """ import java.lang.Number; @@ -968,17 +1226,62 @@ public void foo(Suit s) { System.out.println("Diamond"); } else if (s instanceof Suit r) { System.out.println("It's some black suit"); - } else if (Suit.HEART == s) { - System.out.println("Hearts"); - } else if (Suit.DIAMOND == s) { - System.out.println("Uh oh, diamond again"); + } else if (s == Suit.HEART) { + System.out.println("Heart"); + } else if (s instanceof Suit ss && ss == Suit.DIAMOND) { + System.out.println("Technically allowed"); } } } """) - .setArgs("-XepOpt:IfChainToSwitch:EnableMain") - .doTest(); - } + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + switch (s) { + case Suit.DIAMOND -> System.out.println("Diamond"); + case Suit.HEART -> System.out.println("Heart"); + case Suit ss when ss == Suit.DIAMOND -> System.out.println("Technically allowed"); + case Suit r -> System.out.println("It's some black suit"); + case null -> {} + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain", "-XepOpt:IfChainToSwitch:EnableSafe") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_dupeEnum_noError() { + // Duplicate enum + helper + .addSourceLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + if (s == Suit.DIAMOND) { + System.out.println("Diamond"); + } else if (s instanceof Suit r) { + System.out.println("It's some black suit"); + } else if (Suit.HEART == s) { + System.out.println("Hearts"); + } else if (Suit.DIAMOND == s) { + System.out.println("Uh oh, diamond again"); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .doTest(); + } @Test public void ifChain_loopContext_noError() { @@ -1348,8 +1651,57 @@ public void foo(Suit s) { .doTest(TEXT_MATCH); } + @Test + public void ifChain_elseBecomesDefaultSafe_error() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + if (suit instanceof String) { + System.out.println("It's a string!"); + } else if (suit == Suit.DIAMOND) { + System.out.println("It's a diamond!"); + } else if ((suit instanceof Number)) { + System.out.println("It's a number!"); + } else if (suit instanceof Suit) { + System.out.println("It's a Suit!"); + } else { + throw new AssertionError(); + } + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + switch (suit) { + case String unused -> System.out.println("It's a string!"); + case Suit.DIAMOND -> System.out.println("It's a diamond!"); + case Number unused -> System.out.println("It's a number!"); + case Suit unused -> System.out.println("It's a Suit!"); + case null, default -> throw new AssertionError(); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain", "-XepOpt:IfChainToSwitch:EnableSafe") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + @Test public void ifChain_commentHandling_error() { + refactoringHelper .addInputLines( "Test.java", @@ -1436,6 +1788,94 @@ public void foo(Suit s) { .doTest(TEXT_MATCH); } + @Test + public void ifChain_commentHandlingSafe_error() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + // alpha + /* beta */ if /* gamma */ (suit /* delta */ instanceof String /* epsilon */) { + // zeta + return; + /* eta */ + } /* nu */ else /* theta */ if (suit == /* iota */ Suit.DIAMOND) { + /* kappa */ { + return; // lambda + } + } else if (((suit instanceof Number) /* tao */)) { + // Square + throw new NullPointerException(/* chi */ ); + } else if (suit /* omicron */ instanceof Suit /* pi */) { + /* mu */ + return; + /* nu */ + } + return; + /* xi */ + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + // alpha + /* beta */ switch (suit) { + case String unused -> + /* gamma */ + /* delta */ + /* epsilon */ + /* nu */ + /* theta */ + { + // zeta + return; + /* eta */ + } + case Suit.DIAMOND -> + /* iota */ + /* kappa */ + { + return; // lambda + } + case Number unused -> + /* tao */ + // Square + throw new NullPointerException(/* chi */ ); + case Suit unused -> + /* omicron */ + /* pi */ + { + /* mu */ + return; + /* nu */ + } + case null, default -> { + return; + } + } + + /* xi */ + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain", "-XepOpt:IfChainToSwitch:EnableSafe") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + @Test public void ifChain_pullUp_error() { @@ -1497,7 +1937,67 @@ public void foo(Suit s) { } @Test - public void ifChain_pullUpReachability_error() { + public void ifChain_pullUpSafe_error() { + + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + if (suit instanceof String) { + return; + } else if (suit == Suit.DIAMOND) { + return; + } else if ((suit instanceof Number)) { + return; + } else if (suit instanceof Suit) { + return; + } + return; + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + switch (suit) { + case String unused -> { + return; + } + case Suit.DIAMOND -> { + return; + } + case Number unused -> { + return; + } + case Suit unused -> { + return; + } + case null, default -> { + return; + } + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain", "-XepOpt:IfChainToSwitch:EnableSafe") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_pullUpReachabilityUnchanged_error() { refactoringHelper .addInputLines( "Test.java", @@ -1520,7 +2020,7 @@ public void foo(Suit s) { System.out.println("this will become unreachable"); System.out.println("this will too"); } - System.out.println("this will vanish"); + System.out.println("this will too"); } } """) @@ -1557,7 +2057,7 @@ public void foo(Suit s) { } @Test - public void ifChain_instanceOfWithGuard_error() { + public void ifChain_pullUpReachabilityChanges_error() { refactoringHelper .addInputLines( "Test.java", @@ -1566,26 +2066,145 @@ public void ifChain_instanceOfWithGuard_error() { class Test { public void foo(Suit s) { - Object suit = s; - System.out.println("yo"); - if (suit instanceof String) { - System.out.println("It's a string!"); - } else if (suit instanceof Suit && suit == Suit.SPADE) { - System.out.println("It's a diamond!"); - } else if ((suit instanceof Number)) { - { - { - /* stay silent about numbers */ - } - } - } else if (suit instanceof Suit) { - System.out.println("It's a Suit!"); - } else throw new AssertionError(); - } - } - """) - .addOutputLines( - "Test.java", + { + Suit suit = s; + if (Suit.SPADE == suit) { + return; + } else if (suit == Suit.DIAMOND) { + return; + } else if (suit == Suit.HEART) { + return; + } else if (suit == Suit.CLUB) { + return; + } else if (suit == null) { + return; + } + System.out.println("this will become unreachable"); + System.out.println("this will too"); + } + System.out.println("this will vanish"); + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + { + Suit suit = s; + switch (suit) { + case Suit.SPADE -> { + return; + } + case Suit.DIAMOND -> { + return; + } + case Suit.HEART -> { + return; + } + case Suit.CLUB -> { + return; + } + case null -> { + return; + } + } + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_instanceOfWithGuardSafe_error() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + if (suit instanceof String) { + System.out.println("It's a string!"); + } else if (suit instanceof Suit && suit == Suit.SPADE) { + System.out.println("It's a diamond!"); + } else if ((suit instanceof Number)) { + { + { + /* stay silent about numbers */ + } + } + } else if (suit instanceof Suit) { + System.out.println("It's a Suit!"); + } else throw new AssertionError(); + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + switch (suit) { + case String unused -> System.out.println("It's a string!"); + case Suit unused when suit == Suit.SPADE -> System.out.println("It's a diamond!"); + case Number unused -> { + /* stay silent about numbers */ + } + case Suit unused -> System.out.println("It's a Suit!"); + case null, default -> throw new AssertionError(); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain", "-XepOpt:IfChainToSwitch:EnableSafe") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + + @Test + public void ifChain_instanceOfWithGuard_error() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + if (suit instanceof String) { + System.out.println("It's a string!"); + } else if (suit instanceof Suit && suit == Suit.SPADE) { + System.out.println("It's a diamond!"); + } else if ((suit instanceof Number)) { + { + { + /* stay silent about numbers */ + } + } + } else if (suit instanceof Suit) { + System.out.println("It's a Suit!"); + } else throw new AssertionError(); + } + } + """) + .addOutputLines( + "Test.java", """ import java.lang.Number; @@ -1659,6 +2278,66 @@ public void foo(Suit s) { .doTest(); } + @Test + public void ifChain_parameterizedTypeSafe_error() { + // Raw types are converted to the wildcard type. + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + import java.util.List; + import java.util.ArrayList; + + class Test { + public void foo(Suit s) { + List list = new ArrayList<>(); + if (list instanceof ArrayList && list.size() == 0) { + System.out.println("int empty array list"); + } else if (list instanceof ArrayList && list.size() == 0) { + System.out.println("raw empty array list"); + } else if (list instanceof ArrayList && list.size() == 1) { + System.out.println("wildcard element array list"); + } else if (list instanceof ArrayList && list.size() == 1) { + System.out.println("number element array list"); + } else if (list instanceof List l && l.hashCode() == 17) { + System.out.println("hash 17 list"); + } else if (list instanceof List l) { + System.out.println("list"); + } + } + } + """) + .addOutputLines( + "Test.java", +""" +import java.lang.Number; +import java.util.List; +import java.util.ArrayList; + +class Test { + public void foo(Suit s) { + List list = new ArrayList<>(); + switch (list) { + case ArrayList unused when list.size() == 0 -> + System.out.println("int empty array list"); + case ArrayList unused when list.size() == 0 -> System.out.println("raw empty array list"); + case ArrayList unused when list.size() == 1 -> + System.out.println("wildcard element array list"); + case ArrayList unused when list.size() == 1 -> + System.out.println("number element array list"); + case List l when l.hashCode() == 17 -> System.out.println("hash 17 list"); + case List l -> System.out.println("list"); + case null -> {} + } + } +} +""") + .setArgs("-XepOpt:IfChainToSwitch:EnableMain", "-XepOpt:IfChainToSwitch:EnableSafe") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + @Test public void ifChain_parameterizedType_error() { // Raw types are converted to the wildcard type. @@ -1718,6 +2397,66 @@ public void foo(Suit s) { .doTest(TEXT_MATCH); } + @Test + public void ifChain_multiParameterizedTypeSafe_error() { + // Raw types are converted to the wildcard type. + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + import java.util.Map; + import java.util.HashMap; + + class Test { + public void foo(Suit s) { + Map map = new HashMap<>(); + if (map instanceof HashMap && map.size() == 0) { + System.out.println("empty hash map"); + } else if (map instanceof HashMap && map.size() == 0) { + System.out.println("empty hash map with type parameters"); + } else if (map instanceof HashMap && map.size() == 0) { + System.out.println("empty hash map with wildcard"); + } else if (map instanceof HashMap && map.size() == 1) { + System.out.println("one element hash map"); + } else if (map instanceof Map m && m.hashCode() == 17) { + System.out.println("hash 17"); + } else if (map instanceof Map m) { + System.out.println("map"); + } + } + } + """) + .addOutputLines( + "Test.java", +""" +import java.lang.Number; +import java.util.Map; +import java.util.HashMap; + +class Test { + public void foo(Suit s) { + Map map = new HashMap<>(); + switch (map) { + case HashMap unused when map.size() == 0 -> System.out.println("empty hash map"); + case HashMap unused when map.size() == 0 -> + System.out.println("empty hash map with type parameters"); + case HashMap unused when map.size() == 0 -> + System.out.println("empty hash map with wildcard"); + case HashMap unused when map.size() == 1 -> System.out.println("one element hash map"); + case Map m when m.hashCode() == 17 -> + System.out.println("hash 17"); + case Map m -> System.out.println("map"); + case null -> {} + } + } +} +""") + .setArgs("-XepOpt:IfChainToSwitch:EnableMain", "-XepOpt:IfChainToSwitch:EnableSafe") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + @Test public void ifChain_multiParameterizedType_error() { // Raw types are converted to the wildcard type. @@ -1803,6 +2542,57 @@ public void foo(Suit s) { .doTest(); } + @Test + public void ifChain_domination1Safe_error() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + if (suit instanceof String) { + System.out.println("It's a string!"); + } else if (suit instanceof Suit) { + System.out.println("It's a diamond!"); + } else if ((suit instanceof Number)) { + System.out.println("It's a number!"); + } else if (suit instanceof Suit && suit == Suit.DIAMOND) { + System.out.println("It's a Suit!"); + } else if (suit instanceof Suit suity && suit == Suit.SPADE) { + System.out.println("It's a Suity!"); + } else throw new AssertionError(); + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Suit s) { + Object suit = s; + System.out.println("yo"); + switch (suit) { + case String unused -> System.out.println("It's a string!"); + case Suit unused when suit == Suit.DIAMOND -> System.out.println("It's a Suit!"); + case Suit suity when suit == Suit.SPADE -> System.out.println("It's a Suity!"); + case Suit unused -> System.out.println("It's a diamond!"); + case Number unused -> System.out.println("It's a number!"); + case null, default -> throw new AssertionError(); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain", "-XepOpt:IfChainToSwitch:EnableSafe") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + @Test public void ifChain_domination1_error() { refactoringHelper @@ -1987,6 +2777,46 @@ public void foo(Suit s) { .doTest(); } + @Test + public void ifChain_javadocOrdering_error() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Object obj) { + if (obj instanceof Object) { + System.out.println("It's an object!"); + } else if (obj instanceof Number n) { + System.out.println("It's a number!"); + } else if (obj instanceof String) { + System.out.println("It's a string!"); + } + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.lang.Number; + + class Test { + public void foo(Object obj) { + switch (obj) { + case Number n -> System.out.println("It's a number!"); + case String unused -> System.out.println("It's a string!"); + case Object unused -> System.out.println("It's an object!"); + } + } + } + """) + .setArgs("-XepOpt:IfChainToSwitch:EnableMain") + .setFixChooser(IfChainToSwitchTest::assertOneFixAndChoose) + .doTest(TEXT_MATCH); + } + /** * Asserts that there is exactly one suggested fix and returns it. * diff --git a/docs/bugpattern/IfChainToSwitch.md b/docs/bugpattern/IfChainToSwitch.md index 8cd3be544d4..bba1dfff281 100644 --- a/docs/bugpattern/IfChainToSwitch.md +++ b/docs/bugpattern/IfChainToSwitch.md @@ -58,6 +58,24 @@ Note that with the new `switch` style (`->`), one gets exhaustiveness checking the `switch` would raise a compile-time error, whereas the original chain of `if` statements would need to be manually detected and edited. +If the flag `EnableSafe` is set, the output will include an empty `case null`; +this more closely matches the behavior of the original if-chain when `suit` is +`null`, although is more verbose and may not match the intent. + +``` {.good} +enum Suit {HEARTS, CLUBS, SPADES, DIAMONDS}; + +private void foo(Suit suit) { + switch (suit) { + case Suit.SPADE -> System.out.println("spade"); + case Suit.DIAMOND -> System.out.println("diamond"); + case Suit.HEART -> System.out.println("heart"); + case Suit.CLUB -> System.out.println("club"); + case null -> {} + } +} +``` + #### 2. Patterns This conversion works for `instanceof`s too: @@ -116,5 +134,19 @@ private void describeObject(Object obj) { When calling `describeObject("hello")`, one might expect to have `It's a string!` printed, but this is not what happens. Because the `Object` check happens first in code, it matches, resulting in `It's an object!`. This behavior -is most likely a bug, and can sometimes be hard to spot. (This check can be -suppressed if the behavior is intentional.) +is most likely a bug, and can sometimes be hard to spot. This checker will +automatically reorder `cases`s if needed to correct the issue, like this: + +``` {.good} +private void describeObject(Object obj) { + + switch(obj) { + case Number n -> System.out.println("It's a number!"); + case String unused -> System.out.println("It's a string!"); + case Object unused -> System.out.println("It's an object!"); + } +} +``` + +In this way, the behavior of the switch (`It's a string!`) and the original +if-chain (`It's an object!`) are different.