openrewrite / rewrite-static-analysis

OpenRewrite recipes for identifying and fixing static analysis issues.
Apache License 2.0
31 stars 51 forks source link

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

Open valters opened 3 months ago

valters commented 3 months ago

What version of OpenRewrite are you using?

I am using

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 commented 3 months ago

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