org.openrewrite.java.RemoveObjectsIsNull fails on negated expressions

[protocol7]

What version of OpenRewrite are you using?

• org.openrewrite:rewrite-core:jar:8.27.4
• org.openrewrite.recipe:rewrite-migrate-java:jar:2.17.0

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

import org.junit.jupiter.api.Test;
import org.openrewrite.java.JavaParser;
import org.openrewrite.java.RemoveObjectsIsNull;
import org.openrewrite.test.RecipeSpec;
import org.openrewrite.test.RewriteTest;

import static org.openrewrite.java.Assertions.java;

public class KnownBugsTest implements RewriteTest {

  @Override
  public void defaults(RecipeSpec spec) {
    spec.recipe(new RemoveObjectsIsNull()).parser(JavaParser.fromJavaVersion());
  }

  @Test
  public void negatedRemoveObjectsIsNull() {
    rewriteRun(
        java(
            """
            package com.helloworld;
            
            import java.util.Objects;

            class Hello {
              public boolean hello(String abc) {
                return !Objects.isNull(abc);
              }
            }
            """,
            // "!abc == null" is not a valid expression
            """
            package com.helloworld;

            class Hello {
              public boolean hello(String abc) {
                return abc != null;
              }
            }
            """));
  }
}

Are you interested in contributing a fix to OpenRewrite?

Yes, possibly. Reporting this in the meantime.

[timtebeek]

Thanks for the clear & runnable report @protocol7 ! Simplest way to fix this is probably to add parentheses around the JavaTemplate expression https://github.com/openrewrite/rewrite/blob/52751fd4112720a193c0b0b0dc34800e9d5e9eee/rewrite-java/src/main/java/org/openrewrite/java/RemoveObjectsIsNull.java#L48-L52

That way the inserted code is valid even if there's a negation, which we can simplify using SimplifyBooleanExpressionVisitor, same as we do for unnecessary parentheses. https://github.com/openrewrite/rewrite/blob/52751fd4112720a193c0b0b0dc34800e9d5e9eee/rewrite-java/src/main/java/org/openrewrite/java/RemoveObjectsIsNull.java#L70

[protocol7]

@timtebeek thanks for the pointers, I may look into pushing a PR for this, but likely only in a few days time.

[protocol7]

Not to expose too much of my near complete ignorance of the internals of OpenRewrite :) But I'm confused about how to use SimplifyBooleanExpressionVisitor in this case as the ! of !Objects.isNull(a) is outside the replaced method invokation and thus does not seem to be taken into consideration with a simple: Expression simplified = (Expression) new SimplifyBooleanExpressionVisitor().visitNonNull(replaced, ctx, getCursor());

I noted that InvertCondition uses getCursor().getParentOrThrow(), but I fail to get that to work as well.

[timtebeek]

No worries; we all start somewhere; there's roughly two options (I think):

1. doAfterVisit, which then applies to the whole file (and might thus have unrelated changes)
2. call visit with the parent cursor as argument

Feel free to push up a rough draft without that change just yet and then we can play around to make it work.

[knutwannheden]

getCursor().getParentOrThrow() won't work from a method where a modification is being made, as the parent won't be aware of this change yet. The solution would be to use cursor messaging to set a message on the parent cursor and then is postVisit() you can when this message is on the cursor apply the boolean simplification. Hope this makes sense.