[IfChainToSwitch] when safe mode is enabled, don't reorder cases because this can change program semantics. Update docs accordingly.

PiperOrigin-RevId: 865428565

Author: markhbrady

core/src/main/java/com/google/errorprone/bugpatterns/IfChainToSwitch.java

@@ -422,7 +422,8 @@ private static boolean switchOnNullWouldImplicitlyThrow(
    * 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
    * supplied case IRs. If the supplied case IRs cannot be used to form a syntactically valid switch
-   * statement, returns `Optional.empty()`.
+   * statement, returns `Optional.empty()`. Precondition: the list of supplied cases must not
+   * contain any dominance violations.
    */
   private Optional<List<CaseIr>> maybeFixDefaultNullAndUnconditional(
       List<CaseIr> cases,
@@ -608,7 +609,11 @@ && isSubtype(
       }
     }
 
-    return recheckDominanceNeeded ? maybeFixDominance(cases, state, subject) : Optional.of(cases);
+    return recheckDominanceNeeded
+        // If there is a dominance violation, then we must have caused it by adding a case, so
+        // fixing it merely puts the new case in the correct position
+        ? maybeFixDominance(cases, state, subject, /* canReorderCases= */ true)
+        : Optional.of(cases);
   }
 
   /**
@@ -705,12 +710,13 @@ private static Optional<List<CaseIr>> maybeDetectDuplicateConstants(List<CaseIr>
   }
 
   /**
-   * Analyzes the supplied case IRs for dominance violations. If a dominance violation is detected,
-   * attempts to fix the problem by reordering them so that the violation does not occur. If the
-   * dominance violation cannot be corrected by this algorithm, returns {@code Optional.empty()}.
+   * Analyzes the supplied case IRs for dominance violations. If a dominance violation is detected
+   * and {@code canReorderCases} is {@code true}, attempts to fix the problem by reordering cases so
+   * that no violation is present. If a dominance violation exists and cannot be corrected by this
+   * algorithm, returns {@code Optional.empty()}.
    */
   private static Optional<List<CaseIr>> maybeFixDominance(
-      List<CaseIr> cases, VisitorState state, ExpressionTree subject) {
+      List<CaseIr> cases, VisitorState state, ExpressionTree subject, boolean canReorderCases) {
 
     List<CaseIr> casesCopy = new ArrayList<>(cases);
     List<CaseIr> casesToInsert = new ArrayList<>();
@@ -720,8 +726,12 @@ private static Optional<List<CaseIr>> maybeFixDominance(
       // they were encountered in the if-chain)
       casesToInsert.add(casesCopy.get(insert));
 
+      // Violation found when trying to insert @insert?
       if (hasDominanceViolation(casesToInsert, state, subject)) {
-        // Violation found when trying to insert @insert
+        if (!canReorderCases) {
+          // Dominance violation exists and not allowed to fix
+          return Optional.empty();
+        }
         casesToInsert.remove(casesToInsert.size() - 1);
 
         // Slow path: try to clean up by moving insertion around.  The idea is a bit like an
@@ -1380,7 +1390,9 @@ private List<SuggestedFix> deepAnalysisOfIfChain(
     Optional<List<CaseIr>> fixedCasesOptional =
         casesWithPullupMaybeApplied
             .flatMap(caseList -> maybeDetectDuplicateConstants(caseList))
-            .flatMap(caseList -> maybeFixDominance(caseList, state, subject))
+            .flatMap(
+                caseList ->
+                    maybeFixDominance(caseList, state, subject, /* canReorderCases= */ !enableSafe))
             .flatMap(
                 x ->
                     maybeFixDefaultNullAndUnconditional(
@@ -1402,7 +1414,10 @@ private List<SuggestedFix> deepAnalysisOfIfChain(
       fixedCasesOptional =
           Optional.of(cases)
               .flatMap(caseList -> maybeDetectDuplicateConstants(caseList))
-              .flatMap(caseList -> maybeFixDominance(caseList, state, subject))
+              .flatMap(
+                  caseList ->
+                      maybeFixDominance(
+                          caseList, state, subject, /* canReorderCases= */ !enableSafe))
               .flatMap(
                   caseList ->
                       maybeFixDefaultNullAndUnconditional(
@@ -1416,7 +1431,10 @@ private List<SuggestedFix> deepAnalysisOfIfChain(
                           switchType,
                           ifTreeSourceRange))
               // Changing default/null can affect dominance
-              .flatMap(caseList -> maybeFixDominance(caseList, state, subject));
+              .flatMap(
+                  caseList ->
+                      maybeFixDominance(
+                          caseList, state, subject, /* canReorderCases= */ !enableSafe));
     }
 
     List<SuggestedFix> suggestedFixes = new ArrayList<>();

core/src/test/java/com/google/errorprone/bugpatterns/IfChainToSwitchTest.java

@@ -1211,11 +1211,10 @@ public void foo(Suit s) {
   }
 
   @Test
-  public void ifChain_legalDuplicateSafe_error() {
-    // Although the guard effectively duplicates the diamond constant case, this construction is
-    // legal
-    refactoringHelper
-        .addInputLines(
+  public void ifChain_legalDuplicateSafe_noError() {
+    // Cannot reorder cases to fix dominance when safe mode is enabled
+    helper
+        .addSourceLines(
             "Test.java",
             """
             import java.lang.Number;
@@ -1234,26 +1233,8 @@ public void foo(Suit s) {
               }
             }
             """)
-        .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);
+        .doTest();
   }
 
   @Test
@@ -2659,9 +2640,10 @@ public void foo(Suit s) {
   }
 
   @Test
-  public void ifChain_domination1Safe_error() {
-    refactoringHelper
-        .addInputLines(
+  public void ifChain_domination1Safe_noError() {
+    // Domination violation, but safe mode enabled, so no error
+    helper
+        .addSourceLines(
             "Test.java",
             """
             import java.lang.Number;
@@ -2684,29 +2666,8 @@ public void foo(Suit s) {
               }
             }
             """)
-        .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);
+        .doTest();
   }
 
   @Test

docs/bugpattern/IfChainToSwitch.md

@@ -58,9 +58,10 @@ 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.
+If the flag `-XepOpt:IfChainToSwitch:EnableSafe=true` 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};
@@ -149,4 +150,5 @@ private void describeObject(Object obj) {
 ```
 
 In this way, the behavior of the switch (`It's a string!`) and the original
-if-chain (`It's an object!`) are different.
+if-chain (`It's an object!`) are different. To prevent the checker from changing
+behavior in this way, set the flag `-XepOpt:IfChainToSwitch:EnableSafe=true`.