Convert assigning Switch statements to Switch expressions

[pdelagrave]

What's changed?

A new recipe to convert switch statements for which all cases are doing an assignment to the same variable into a switch expression that is assigned to that variable:

From:

       String formatted = "initialValue";
       switch (obj) {
            case Integer i -> formatted = String.format("int %d", i);
            case Long l -> formatted = String.format("long %d", l);
            default -> formatted = "unknown";
       }

To:

        String formatted = switch (obj) {
            case Integer i -> String.format("int %d", i);
            case Long l -> String.format("long %d", l);
            default -> "unknown";
        }

What's your motivation?

• Closes Convert assigning Switch statements to Switch expressions #718

Anything in particular you'd like reviewers to focus on?

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

Any additional context

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've used the IntelliJ IDEA auto-formatter on affected files

[github-actions]

Some suggestions could not be made:

• src/main/java/org/openrewrite/java/migrate/lang/NullCheckAsSwitchCase.java
  • lines 27-27

[github-actions]

Some suggestions could not be made:

• src/main/java/org/openrewrite/java/migrate/lang/NullCheckAsSwitchCase.java
  • lines 27-28

[Laurens-W]

Appreciate the draft with extensive tests here!
Perhaps good to disable some of the more extensive tests for now and start work on converting the simple colon/arrow into a single statement/expression cases!

[github-actions]

Some suggestions could not be made:

• src/main/java/org/openrewrite/java/migrate/lang/NullCheckAsSwitchCase.java
  • lines 27-27

[timtebeek]

Ran it against some 600 local LSTs; only saw results for a single repository: apache/maven

diff --git a/impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/LogbackConfiguration.java b/impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/LogbackConfiguration.java
index 4481486..da4519e 100644
--- a/impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/LogbackConfiguration.java
+++ b/impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/LogbackConfiguration.java
@@ -30,20 +30,13 @@ org.openrewrite.java.migrate.lang.SwitchCaseAssigningToSwitchExpression
 public class LogbackConfiguration extends BaseSlf4jConfiguration {
     @Override
     public void setRootLoggerLevel(Level level) {
-        ch.qos.logback.classic.Level value;
-        switch (level) {
-            case DEBUG:
-                value = ch.qos.logback.classic.Level.DEBUG;
-                break;
+        ch.qos.logback.classic.Level value = switch (level) {
+            case DEBUG: yield ch.qos.logback.classic.Level.DEBUG;
 
-            case INFO:
-                value = ch.qos.logback.classic.Level.INFO;
-                break;
+            case INFO: yield ch.qos.logback.classic.Level.INFO;
 
-            default:
-                value = ch.qos.logback.classic.Level.ERROR;
-                break;
-        }
+            default: yield ch.qos.logback.classic.Level.ERROR;
+        };
         ((ch.qos.logback.classic.Logger) LoggerFactory.getLogger(Logger.ROOT_LOGGER_NAME)).setLevel(value);
     }
 
diff --git a/impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/Log4j2Configuration.java b/impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/Log4j2Configuration.java
index 1f02e57..07e84c8 100644
--- a/impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/Log4j2Configuration.java
+++ b/impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/Log4j2Configuration.java
@@ -28,20 +28,13 @@ org.openrewrite.java.migrate.lang.SwitchCaseAssigningToSwitchExpression
 public class Log4j2Configuration extends BaseSlf4jConfiguration {
     @Override
     public void setRootLoggerLevel(Level level) {
-        String value;
-        switch (level) {
-            case DEBUG:
-                value = "debug";
-                break;
+        String value = switch (level) {
+            case DEBUG: yield "debug";
 
-            case INFO:
-                value = "info";
-                break;
+            case INFO: yield "info";
 
-            default:
-                value = "error";
-                break;
-        }
+            default: yield "error";
+        };
         System.setProperty("maven.logging.root.level", value);
     }
 
diff --git a/impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/MavenSimpleConfiguration.java b/impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/MavenSimpleConfiguration.java
index 5147cd8..0c5f4e2 100644
--- a/impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/MavenSimpleConfiguration.java
+++ b/impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/MavenSimpleConfiguration.java
@@ -31,20 +31,13 @@ org.openrewrite.java.migrate.lang.SwitchCaseAssigningToSwitchExpression
 public class MavenSimpleConfiguration extends BaseSlf4jConfiguration {
     @Override
     public void setRootLoggerLevel(Level level) {
-        String value;
-        switch (level) {
-            case DEBUG:
-                value = "debug";
-                break;
+        String value = switch (level) {
+            case DEBUG: yield "debug";
 
-            case INFO:
-                value = "info";
-                break;
+            case INFO: yield "info";
 
-            default:
-                value = "error";
-                break;
-        }
+            default: yield "error";
+        };
         System.setProperty("org.slf4j.simpleLogger.defaultLogLevel", value);
     }

Does that align with what you were seeing there?

[pdelagrave]

@timtebeek
While the switch is already exhaustive, wouldn't still adding default: yield "initialValue"; at the end be safe and preserve the logic in any situation, given the last case is empty?
This would require a fix in the code but that new test made me wonder why not.

[pdelagrave]

Ran it against some 600 local LSTs; only saw results for a single repository: apache/maven

diff --git a/impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/LogbackConfiguration.java b/impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/LogbackConfiguration.java
index 4481486..da4519e 100644
--- a/impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/LogbackConfiguration.java
+++ b/impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/LogbackConfiguration.java
@@ -30,20 +30,13 @@ org.openrewrite.java.migrate.lang.SwitchCaseAssigningToSwitchExpression
 public class LogbackConfiguration extends BaseSlf4jConfiguration {
     @Override
     public void setRootLoggerLevel(Level level) {
-        ch.qos.logback.classic.Level value;
-        switch (level) {
-            case DEBUG:
-                value = ch.qos.logback.classic.Level.DEBUG;
-                break;
+        ch.qos.logback.classic.Level value = switch (level) {
+            case DEBUG: yield ch.qos.logback.classic.Level.DEBUG;
 
-            case INFO:
-                value = ch.qos.logback.classic.Level.INFO;
-                break;
+            case INFO: yield ch.qos.logback.classic.Level.INFO;
 
-            default:
-                value = ch.qos.logback.classic.Level.ERROR;
-                break;
-        }
+            default: yield ch.qos.logback.classic.Level.ERROR;
+        };
         ((ch.qos.logback.classic.Logger) LoggerFactory.getLogger(Logger.ROOT_LOGGER_NAME)).setLevel(value);
     }
 
diff --git a/impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/Log4j2Configuration.java b/impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/Log4j2Configuration.java
index 1f02e57..07e84c8 100644
--- a/impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/Log4j2Configuration.java
+++ b/impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/Log4j2Configuration.java
@@ -28,20 +28,13 @@ org.openrewrite.java.migrate.lang.SwitchCaseAssigningToSwitchExpression
 public class Log4j2Configuration extends BaseSlf4jConfiguration {
     @Override
     public void setRootLoggerLevel(Level level) {
-        String value;
-        switch (level) {
-            case DEBUG:
-                value = "debug";
-                break;
+        String value = switch (level) {
+            case DEBUG: yield "debug";
 
-            case INFO:
-                value = "info";
-                break;
+            case INFO: yield "info";
 
-            default:
-                value = "error";
-                break;
-        }
+            default: yield "error";
+        };
         System.setProperty("maven.logging.root.level", value);
     }
 
diff --git a/impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/MavenSimpleConfiguration.java b/impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/MavenSimpleConfiguration.java
index 5147cd8..0c5f4e2 100644
--- a/impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/MavenSimpleConfiguration.java
+++ b/impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/MavenSimpleConfiguration.java
@@ -31,20 +31,13 @@ org.openrewrite.java.migrate.lang.SwitchCaseAssigningToSwitchExpression
 public class MavenSimpleConfiguration extends BaseSlf4jConfiguration {
     @Override
     public void setRootLoggerLevel(Level level) {
-        String value;
-        switch (level) {
-            case DEBUG:
-                value = "debug";
-                break;
+        String value = switch (level) {
+            case DEBUG: yield "debug";
 
-            case INFO:
-                value = "info";
-                break;
+            case INFO: yield "info";
 
-            default:
-                value = "error";
-                break;
-        }
+            default: yield "error";
+        };
         System.setProperty("org.slf4j.simpleLogger.defaultLogLevel", value);
     }

Does that align with what you were seeing there?

I've ran it on apache repos and got a few repos responding, but by far apache/fineract had the most changes (~34)
There were more initially (48) but we're seeing less of them as the recipe is now stricter to be safe, for example:

    EnumOptionData optionData = new EnumOptionData(AccountType.INVALID.getValue().longValue(), AccountType.INVALID.getCode(), "Invalid");
        switch (type) {
            case INVALID:
                optionData = new EnumOptionData(AccountType.INVALID.getValue().longValue(), AccountType.INVALID.getCode(), "Invalid");
...

doesn't get migrated anymore because the optionData initializer code could have a side effect and we don't want to remove it, even though in many circumstances (like this one) it would be fine.

[Bhadboiab]

Great job 👏