RemoveObjectsIsNull doesn't remove unnecessary parentheses, if it

[Bananeweizen]

How are you running OpenRewrite?

I am using the Maven plugin. The important configuration is that only a single recipe is enabled.

  <configuration>
    <activeRecipes>
	<recipe>org.openrewrite.java.RemoveObjectsIsNull</recipe>
    </activeRecipes>
  </configuration>

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

class A {
    void foo(String bar) {
        if (nonNull(foo)) {
          ...
        }
    }
}

What did you expect to see?

class A {
    void foo(String bar) {
        if (foo != null) {
          ...
        }
    }
}

What did you see instead?

class A {
    void foo(String bar) {
        if ((foo != null)) {
          ...
        }
    }
}

Note that there are superfluous braces around the replaced expression. This is surprising, because the recipe invokes the visitor for removing parentheses unconditionally directly in this visitor: https://github.com/openrewrite/rewrite/blob/295b0b25cf266167832408dbb2d915a35d134cf0/rewrite-java/src/main/java/org/openrewrite/java/RemoveObjectsIsNull.java#L70

Now I'm wondering if directly calling that second visitor may eventually not work if the necessary recipe for the second visitor is not enabled, and if that rather needs to be moved into doAfterVisit (because I've seen "cleanup" recipes mostly used there).

Are you interested in contributing a fix to OpenRewrite?

If someone can advice whether that doAfterVisit() is the right way, sure.

[timtebeek]

Thanks for the report! Curious indeed. The difference between directly calling a visitor with an argument and doAfterVisit is that the first approach only applies to the argument, whereas doAfterVisit applies to the whole file. There might be cases where that ends up removing parentheses that are more of a personal preference, unrelated to the RemoveObjectsIsNull change. That's why the first case is preferred where possible.

Perhaps a good first step is a failing test using the above, and then we can try to see how we can debug and fix this case. I'm suspicious of the getCursor()passed into the visitNonNull, given that there's also a replaced argument. Elsewhere we update the cursor in such cases, but I'd have to dive in to this particular case to know if that's a fix here. Would appreciate the help!