From 4f615011a05df9d99878388f19f4130c6bdfbd46 Mon Sep 17 00:00:00 2001
From: markbrady <markbrady@google.com>
Date: Thu, 16 Jan 2025 12:29:30 -0800
Subject: [PATCH] [StatementSwitchToExpressionSwitch] for direct conversion
 pattern, if the switch has a `default:` clause and is otherwise exhaustive
 (even without said `default:` clause), then propose an alternative fix which
 removes the `default:` clause and its code.

The first fix always retains the `default:` clause.

PiperOrigin-RevId: 716336071
---
 .../StatementSwitchToExpressionSwitch.java    |  30 +-
 ...StatementSwitchToExpressionSwitchTest.java | 344 +++++++++++++-----
 2 files changed, 286 insertions(+), 88 deletions(-)

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java b/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java
index db07e834596..3b182ddcac0 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java
@@ -193,7 +193,15 @@ public Description matchSwitch(SwitchTree switchTree, VisitorState state) {
       }
     }
     if (enableDirectConversion && analysisResult.canConvertDirectlyToExpressionSwitch()) {
-      suggestedFixes.add(convertDirectlyToExpressionSwitch(switchTree, state, analysisResult));
+      suggestedFixes.add(
+          convertDirectlyToExpressionSwitch(
+              switchTree, state, analysisResult, /* removeDefault= */ false));
+
+      if (analysisResult.canRemoveDefault()) {
+        suggestedFixes.add(
+            convertDirectlyToExpressionSwitch(
+                switchTree, state, analysisResult, /* removeDefault= */ true));
+      }
     }
 
     return suggestedFixes.isEmpty()
@@ -639,10 +647,14 @@ private static boolean areStatementsConvertibleToExpressionSwitch(
   /**
    * Transforms the supplied statement switch into an expression switch directly. In this
    * conversion, each nontrivial statement block is mapped one-to-one to a new {@code Expression} or
-   * {@code StatementBlock} on the right-hand side. Comments are preserved where possible.
+   * {@code StatementBlock} on the right-hand side (the `default:` case is removed if {@code
+   * removeDefault} is true). Comments are preserved where possible.
    */
   private static SuggestedFix convertDirectlyToExpressionSwitch(
-      SwitchTree switchTree, VisitorState state, AnalysisResult analysisResult) {
+      SwitchTree switchTree,
+      VisitorState state,
+      AnalysisResult analysisResult,
+      boolean removeDefault) {
 
     List<? extends CaseTree> cases = switchTree.getCases();
     ImmutableList<ErrorProneComment> allSwitchComments =
@@ -662,6 +674,11 @@ private static SuggestedFix convertDirectlyToExpressionSwitch(
       CaseTree caseTree = cases.get(caseIndex);
       boolean isDefaultCase = isSwitchDefault(caseTree);
 
+      if (removeDefault && isDefaultCase) {
+        // Skip default case
+        continue;
+      }
+
       // For readability, filter out trailing unlabelled break statement because these become a
       // No-Op when used inside expression switches
       ImmutableList<StatementTree> filteredStatements = filterOutRedundantBreak(caseTree);
@@ -756,7 +773,12 @@ private static SuggestedFix convertDirectlyToExpressionSwitch(
     // Close the switch statement
     replacementCodeBuilder.append("\n}");
 
-    return SuggestedFix.builder().replace(switchTree, replacementCodeBuilder.toString()).build();
+    SuggestedFix.Builder suggestedFixBuilder = SuggestedFix.builder();
+    if (removeDefault) {
+      suggestedFixBuilder.setShortDescription(REMOVE_DEFAULT_CASE_SHORT_DESCRIPTION);
+    }
+    suggestedFixBuilder.replace(switchTree, replacementCodeBuilder.toString());
+    return suggestedFixBuilder.build();
   }
 
   /**
diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java
index fe26e6a2f14..4adf726cde9 100644
--- a/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java
+++ b/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java
@@ -133,6 +133,7 @@ public void foo(Side side) {
             }
             """)
         .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")
+        .setFixChooser(StatementSwitchToExpressionSwitchTest::assertOneFixAndChoose)
         .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
   }
 
@@ -224,6 +225,168 @@ public void foo(Side side) {
             }
             """)
         .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")
+        .setFixChooser(StatementSwitchToExpressionSwitchTest::assertOneFixAndChoose)
+        .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
+  }
+
+  @Test
+  public void switchByEnumExhaustiveWithoutDefault_removesDefault_error() {
+    // The switch covers all enum values and also includes a default clause, so assert that a
+    // secondary fix is generated to remove the default clause.
+    helper
+        .addSourceLines(
+            "Test.java",
+            """
+            class Test {
+              enum Side {
+                OBVERSE,
+                REVERSE
+              };
+
+              public Test(int foo) {}
+
+              public void foo(Side side) {
+                // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]
+                switch (side) {
+                  // Comment before first case
+                  case OBVERSE:
+                    // Explanatory comment
+                    System.out.println("this block cannot complete normally");
+                    {
+                      throw new NullPointerException();
+                    }
+                  case REVERSE:
+                    System.out.println("reverse");
+                    break;
+                  default:
+                    System.out.println("default");
+                }
+              }
+            }
+            """)
+        .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")
+        .doTest();
+
+    // Check correct generated code
+    refactoringHelper
+        .addInputLines(
+            "Test.java",
+            """
+            class Test {
+              enum Side {
+                OBVERSE,
+                REVERSE
+              };
+
+              public Test(int foo) {}
+
+              public void foo(Side side) {
+                switch (side) {
+                  // Comment before first case
+                  case OBVERSE:
+                    // Explanatory comment
+                    System.out.println("this block cannot complete normally");
+                    {
+                      throw new NullPointerException();
+                    }
+                  case REVERSE:
+                    System.out.println("reverse");
+                    break;
+                  default:
+                    System.out.println("default");
+                }
+              }
+            }
+            """)
+        .addOutputLines(
+            "Test.java",
+            """
+            class Test {
+              enum Side {
+                OBVERSE,
+                REVERSE
+              };
+
+              public Test(int foo) {}
+
+              public void foo(Side side) {
+                switch (side) {
+                  case OBVERSE -> {
+                    // Comment before first case
+                    // Explanatory comment
+                    System.out.println("this block cannot complete normally");
+                    {
+                      throw new NullPointerException();
+                    }
+                  }
+                  case REVERSE -> System.out.println("reverse");
+                  default -> System.out.println("default");
+                }
+              }
+            }
+            """)
+        .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")
+        .setFixChooser(FixChoosers.FIRST)
+        .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
+
+    refactoringHelper2
+        .addInputLines(
+            "Test.java",
+            """
+            class Test {
+              enum Side {
+                OBVERSE,
+                REVERSE
+              };
+
+              public Test(int foo) {}
+
+              public void foo(Side side) {
+                switch (side) {
+                  // Comment before first case
+                  case OBVERSE:
+                    // Explanatory comment
+                    System.out.println("this block cannot complete normally");
+                    {
+                      throw new NullPointerException();
+                    }
+                  case REVERSE:
+                    System.out.println("reverse");
+                    break;
+                  default:
+                    System.out.println("default");
+                }
+              }
+            }
+            """)
+        .addOutputLines(
+            "Test.java",
+            """
+            class Test {
+              enum Side {
+                OBVERSE,
+                REVERSE
+              };
+
+              public Test(int foo) {}
+
+              public void foo(Side side) {
+                switch (side) {
+                  case OBVERSE -> {
+                    // Comment before first case
+                    // Explanatory comment
+                    System.out.println("this block cannot complete normally");
+                    {
+                      throw new NullPointerException();
+                    }
+                  }
+                  case REVERSE -> System.out.println("reverse");
+                }
+              }
+            }
+            """)
+        .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")
+        .setFixChooser(FixChoosers.SECOND)
         .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
   }
 
@@ -328,6 +491,7 @@ public void foo(Side side) {
             }
             """)
         .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")
+        .setFixChooser(StatementSwitchToExpressionSwitchTest::assertOneFixAndChoose)
         .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
   }
 
@@ -429,6 +593,7 @@ public void foo(Side side) {
             }
             """)
         .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")
+        .setFixChooser(StatementSwitchToExpressionSwitchTest::assertOneFixAndChoose)
         .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
   }
 
@@ -531,6 +696,7 @@ public void foo(Side side) {
             }
             """)
         .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")
+        .setFixChooser(StatementSwitchToExpressionSwitchTest::assertOneFixAndChoose)
         .doTest();
   }
 
@@ -706,6 +872,7 @@ public void foo(Side side) {
             }
             """)
         .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")
+        .setFixChooser(StatementSwitchToExpressionSwitchTest::assertOneFixAndChoose)
         .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
   }
 
@@ -809,6 +976,7 @@ public void foo(Side side) {
             }
             """)
         .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")
+        .setFixChooser(StatementSwitchToExpressionSwitchTest::assertOneFixAndChoose)
         .doTest();
   }
 
@@ -898,6 +1066,7 @@ public void foo(Side side) {
             }
             """)
         .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")
+        .setFixChooser(StatementSwitchToExpressionSwitchTest::assertOneFixAndChoose)
         .doTest();
   }
 
@@ -1071,6 +1240,7 @@ public void foo(Side side) {
             }
             """)
         .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")
+        .setFixChooser(StatementSwitchToExpressionSwitchTest::assertOneFixAndChoose)
         .doTest();
   }
 
@@ -1370,6 +1540,7 @@ private void bar() {}
             }
             """)
         .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")
+        .setFixChooser(StatementSwitchToExpressionSwitchTest::assertOneFixAndChoose)
         .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
   }
 
@@ -1480,6 +1651,7 @@ private void bar() {}
             }
             """)
         .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")
+        .setFixChooser(StatementSwitchToExpressionSwitchTest::assertOneFixAndChoose)
         .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
   }
 
@@ -1590,6 +1762,7 @@ private void bar() {}
             }
             """)
         .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")
+        .setFixChooser(StatementSwitchToExpressionSwitchTest::assertOneFixAndChoose)
         .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
   }
 
@@ -1680,6 +1853,7 @@ public void foo(Side side) {
             }
             """)
         .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")
+        .setFixChooser(StatementSwitchToExpressionSwitchTest::assertOneFixAndChoose)
         .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
   }
 
@@ -1769,6 +1943,7 @@ public void foo(Side side) {
             }
             """)
         .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")
+        .setFixChooser(StatementSwitchToExpressionSwitchTest::assertOneFixAndChoose)
         .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
   }
 
@@ -1865,6 +2040,91 @@ public int foo(Suit suit) {
             }
             """)
         .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion")
+        .setFixChooser(StatementSwitchToExpressionSwitchTest::assertOneFixAndChoose)
+        .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
+  }
+
+  @Test
+  public void i4222() {
+    refactoringHelper
+        .addInputLines(
+            "Test.java",
+            """
+            public class Test {
+              public static void main(String[] args) {
+                switch (args.length) {
+                  case 0:
+                    {
+                      System.out.println(0);
+                      break;
+                    }
+                  case 1:
+                    System.out.println(1);
+                    break;
+                  case 2:
+                    System.out.println(2);
+                    System.out.println(2);
+                    break;
+                }
+              }
+            }
+            """)
+        .addOutputLines(
+            "Test.java",
+            """
+            public class Test {
+              public static void main(String[] args) {
+                switch (args.length) {
+                  case 0 -> System.out.println(0);
+                  case 1 -> System.out.println(1);
+                  case 2 -> {
+                    System.out.println(2);
+                    System.out.println(2);
+                  }
+                }
+              }
+            }
+            """)
+        .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion=true")
+        .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
+  }
+
+  @Test
+  public void unnecessaryBreaks() {
+    refactoringHelper
+        .addInputLines(
+            "Test.java",
+            """
+            public class Test {
+              public static void main(String[] args) {
+                switch (args.length) {
+                  case 0:
+                    System.out.println(0);
+                    break;
+                  default:
+                    // hello
+                    // world
+                    break;
+                }
+              }
+            }
+            """)
+        .addOutputLines(
+            "Test.java",
+            """
+            public class Test {
+              public static void main(String[] args) {
+                switch (args.length) {
+                  case 0 -> System.out.println(0);
+                  default -> {
+                    // hello
+                    // world
+                  }
+                }
+              }
+            }
+            """)
+        .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion=true")
         .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
   }
 
@@ -5113,90 +5373,6 @@ public int foo(Side side) {
         .doTest();
   }
 
-  @Test
-  public void i4222() {
-    refactoringHelper
-        .addInputLines(
-            "Test.java",
-            """
-            public class Test {
-              public static void main(String[] args) {
-                switch (args.length) {
-                  case 0:
-                    {
-                      System.out.println(0);
-                      break;
-                    }
-                  case 1:
-                    System.out.println(1);
-                    break;
-                  case 2:
-                    System.out.println(2);
-                    System.out.println(2);
-                    break;
-                }
-              }
-            }
-            """)
-        .addOutputLines(
-            "Test.java",
-            """
-            public class Test {
-              public static void main(String[] args) {
-                switch (args.length) {
-                  case 0 -> System.out.println(0);
-                  case 1 -> System.out.println(1);
-                  case 2 -> {
-                    System.out.println(2);
-                    System.out.println(2);
-                  }
-                }
-              }
-            }
-            """)
-        .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion=true")
-        .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
-  }
-
-  @Test
-  public void unnecessaryBreaks() {
-    refactoringHelper
-        .addInputLines(
-            "Test.java",
-            """
-            public class Test {
-              public static void main(String[] args) {
-                switch (args.length) {
-                  case 0:
-                    System.out.println(0);
-                    break;
-                  default:
-                    // hello
-                    // world
-                    break;
-                }
-              }
-            }
-            """)
-        .addOutputLines(
-            "Test.java",
-            """
-            public class Test {
-              public static void main(String[] args) {
-                switch (args.length) {
-                  case 0 -> System.out.println(0);
-                  default -> {
-                    // hello
-                    // world
-                  }
-                }
-              }
-            }
-            """)
-        .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion=true")
-        .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
-  }
-
   @Test
   public void mixedExpressionsAndYields() {
     refactoringHelper