`EmptyBlock` removes `} else {` when the `if` block was empty, breaking the logic

[valters]

What version of OpenRewrite are you using?

I am using

• Maven plugin 5.35.0
• rewrite-static-analysis 1.11.0

How are you running OpenRewrite?

Maven plugin. Minimal project to repro the issue: https://github.com/valters/test-openrewrite Please see steps in readme.

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

Trying to rewrite code with moderately tricky if logic:

public class WithEmptyBlock {

  public static class Base {
    public boolean isExtended() {
      return false;
    }
  }

  public static class Extender extends Base {

    @Override
    public boolean isExtended() {
      return true;
    }

    public boolean isExtendedClass() {
      return true;
    }

    public boolean isSomeOtherClass() {
      return false;
    }
  }

  public void shouldNotFlipCondition(Base arg) {

    if (arg.isExtended()) {
      if (((Extender) arg).isExtendedClass()
          || ((Extender) arg).isSomeOtherClass()) {
      } else {
        System.out.println("This line should not be executed!");
        throw new RuntimeException("code should not reach here! if you see this, rewrite has messed up.");
      }
    }
  }
}

What did you expect to see?

Either if condition should be inverted, or the else keyword can't be removed.

What did you see instead?

Broken diff after rewrite:

diff --git a/source-problems/src/main/java/ch/vingolds/testopenrewrite/WithEmptyBlock.java b/source-problems/src/main/java/ch/vingolds/testopenrewrite/WithEmptyBlock.java
index 5b4c050..c1a9f75 100644
--- a/source-problems/src/main/java/ch/vingolds/testopenrewrite/WithEmptyBlock.java
+++ b/source-problems/src/main/java/ch/vingolds/testopenrewrite/WithEmptyBlock.java
@@ -29,7 +29,6 @@ public class WithEmptyBlock {
     if (arg.isExtended()) {
       if (((Extender) arg).isExtendedClass()
           || ((Extender) arg).isSomeOtherClass()) {
-      } else {
         System.out.println("This line should not be executed!");
         throw new RuntimeException("code should not reach here! if you see this, rewrite has messed up.");
       }

What is the full stack trace of any errors you encountered?

No error. Just incorrect output.

[timtebeek]

Thanks a lot for the detailed report and reproducer! I think it would be best to turn this into a runnable unit test as seen here, and then work from there on a draft PR.

https://github.com/openrewrite/rewrite-static-analysis/blob/48c94b5f4ebf27f3ab589096018c15dd78b61fb7/src/test/java/org/openrewrite/staticanalysis/EmptyBlockTest.java#L391-L417

[greg-at-moderne]

Thank you again for reporting. I've managed to simplify the test case to:

    @Test
    void empty2() {
        rewriteRun(
          java(
            """
            public class WithEmptyBlock {
              public void shouldNotFlipCondition() {
                  if (1 > 0 || 0 > 1) {
                  } else {
                    System.out.println("wrong");
                  }
              }
            }
            """
          )
        );
    }

From what I can see the fact that the if statements uses an or operator (||) is crucial to the problem.