RemoveUnusedLocalVariables removes code with side effects

[protocol7]

I am using

• org.openrewrite:rewrite-core:8.62.4
• org.openrewrite.recipe:rewrite-static-analysis:2.18.0

What is the smallest, simplest way to reproduce the problem?

  @Test
  void removingCodeWithSideEffects() {
    rewriteRun(
        spec -> spec.recipe(new RemoveUnusedLocalVariables(null, null, true)),
        // language=java
        java(
            """
             public class Foo {
               private String baz() {
                 String foo;
                 try {
                    foo = String.valueOf(1);
                 } catch (RuntimeException e) {
                     return "error";
                 }
                 return "ok";
               }
             }
            """));
  }

The value of the withSideEffects doesn't matter for the results.

org.opentest4j.AssertionFailedError: [Expected recipe to complete in 0 cycles, but took at least one more cycle. Between the last two executed cycles there were changes to "Foo.java"]
expected:
  "public class Foo {
    private String baz() {
      String foo;
      try {
         foo = String.valueOf(1);
      } catch (RuntimeException e) {
          return "error";
      }
      return "ok";
    }
  }"
 but was:
  "public class Foo {
    private String baz() {
      try {
      } catch (RuntimeException e) {
          return "error";
      }
      return "ok";
    }
  }"
	at org.openrewrite.test.LargeSourceSetCheckingExpectedCycles.afterCycle(LargeSourceSetCheckingExpectedCycles.java:97)
	at org.openrewrite.RecipeScheduler.runRecipeCycles(RecipeScheduler.java:95)
	at org.openrewrite.RecipeScheduler.scheduleRun(RecipeScheduler.java:41)
	at org.openrewrite.Recipe.run(Recipe.java:442)
	at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:382)
	at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:130)

[timtebeek]

Thanks for the runnable report! Not quite pleased with the approach yet, but here's a quick attempt at a fix; let me know your thoughts:

• Fix RemoveUnusedLocalVariables to preserve assignments with side effects #741

[timtebeek]

Circled back on the above and came up with a much smaller but more impactful change to the recipe: do not remove any variables if their assignment might have side effects, which is the safer thing to do here. Hope you agree!

[protocol7]

Looks, thanks!