openrewrite / rewrite-static-analysis

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

`if` statements with literal conditions and jumps in their branches aren't simplified #192

Closed tkindy closed 11 months ago

tkindy commented 11 months ago

What version of OpenRewrite are you using?

I am using

How are you running OpenRewrite?

I am using the Maven plugin, and my project is a single module project.

<plugin>
  <groupId>org.openrewrite.maven</groupId>
  <artifactId>rewrite-maven-plugin</artifactId>
  <version>5.8.1</version>
  <configuration>
    <activeRecipes>
      <recipe>org.openrewrite.staticanalysis.SimplifyConstantIfBranchExecution</recipe>
    </activeRecipes>
  </configuration>
  <dependencies>
    <dependency>
      <groupId>org.openrewrite.recipe</groupId>
      <artifactId>rewrite-static-analysis</artifactId>
      <version>1.0.8</version>
    </dependency>
  </dependencies>
</plugin>

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

class A {
  public int foo() {
    if (true) {
      return 5;
    }
    return 6;
  }
}

What did you expect to see?

class A {
  public int foo() {
    return 5;
  }
}

What did you see instead?

class A {
  public int foo() {
    if (true) {
      return 5;
    }
    return 6;
  }
}

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

No errors.

Are you interested in contributing a fix to OpenRewrite?

Yes, I'm interested in contributing a fix.

tkindy commented 11 months ago

From some digging, this seems to be the recipe's intended behavior. ifs with jumps in the branches were ignored in https://github.com/openrewrite/rewrite-static-analysis/commit/5537be97bb56a5aafd21d360030906ef4dce2d27 as the fix for https://github.com/openrewrite/rewrite-java-security/issues/34 where the combination of SimplifyConstantIfBranchExecution and a different recipe led to unreachable code when the if had a return in the kept branch.

However, I think that fix is overly broad as it leaves some redundant if statements around. I think the right fix would be to eliminate the unreachable code introduced in that scenario. I could try to implement that if others agree on this approach.