search

openrewrite / rewrite

Automated mass refactoring of source code.
https://docs.openrewrite.org
Apache License 2.0
2.03k stars 300 forks source link

NPE in SimplifyMethodChain #3580

Closed Bananeweizen closed 8 months ago

Bananeweizen commented 10 months ago

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

org.openrewrite.java.SimplifyMethodChain without arguments.

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

 java.lang.NullPointerException
    at org.openrewrite.java.SimplifyMethodChain.lambda$validate$0(SimplifyMethodChain.java:64)
    at org.openrewrite.Validated.test(Validated.java:81)
    at org.openrewrite.java.SimplifyMethodChain.validate(SimplifyMethodChain.java:62)
    at org.openrewrite.Recipe.validate(Recipe.java:283)
    at org.openrewrite.Recipe.validate(Recipe.java:286)
    at org.openrewrite.Recipe.validate(Recipe.java:286)
    at org.openrewrite.Recipe.validateAll(Recipe.java:319)

Are you interested in contributing a fix to OpenRewrite?

Yes.

Root cause is verifying the list size without verifying there is actually a list: https://github.com/openrewrite/rewrite/blob/0d526a0dc6593d3c47e0d622f9b6f7676aee6615/rewrite-java/src/main/java/org/openrewrite/java/SimplifyMethodChain.java#L64 I would just add a null check on c before accessing its size.

timtebeek commented 10 months ago

Ah yes thanks! Not the first time we see a NPE in validation unfortunately. Thanks for indicating you're willing to fix that! If you don't get to it let me know and I'll do the same.

timtebeek commented 10 months ago

Similar in that sense to https://github.com/openrewrite/rewrite-gradle-plugin/issues/198 ; really helps to get reports like these, thanks!

Bananeweizen commented 10 months ago

I think I now have another suggestion that fixes both (and probably some other unknown ones). The real root cause is that developers expect super.validate().and(Validate.test(somePredicate)) to be evaluated lazily, and therefore somePredicate to only be checked if super.validate is true. But that's not the case, because Validate.test immediately evaluates the predicate. I'd rewrite that part to create an anonymous Validate subclass which implements isValid to evaluate the predicate lazily. I'm still fiddling with the build as such, but should have something soon.