search

openrewrite / rewrite-static-analysis

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

Add test case for `== false` rewrite #266

Closed koppor closed 4 months ago

koppor commented 4 months ago

Context: https://rewriteoss.slack.com/archives/C01A843MWG5/p1709465857842709

Expected change:

- if (file.isOnlineLink() == false) {
+ if (!file.isOnlineLink()) {

According to https://rewriteoss.slack.com/archives/C01A843MWG5/p1709466186202159?thread_ts=1709465857.842709&cid=C01A843MWG5, it should go into SimplifyBooleanExpression. Therefore, I added a test

It also could be a refaster template, but IDK. Think, it is closer to the existing code in SimplifyBooleanExpression - or should be the complete class rewritten by refaster templates?

The visitor is there https://github.com/openrewrite/rewrite/blob/main/rewrite-java/src/main/java/org/openrewrite/java/cleanup/SimplifyBooleanExpressionVisitor.java. (https://rewriteoss.slack.com/archives/C01A843MWG5/p1709476587357369?thread_ts=1709465857.842709&cid=C01A843MWG5)

Note that SimplifyBooleanExpressionVisitor resides in a different project.

I could not do asBinary.getRight().negate() as negate() is not a method - and I did not find the appropriate method to do so.

timtebeek commented 4 months ago

Thanks for the example! Should be fixed upstream and trickle down here. :)

koppor commented 4 months ago

@timtebeek Shouldn't we keep the tests also here? Or should the tests be remove alltogether, because they are contained upstream somehohw? Or should there be a comment that the tests are incomplete on purpose?

timtebeek commented 4 months ago

@timtebeek Shouldn't we keep the tests also here? Or should the tests be remove alltogether, because they are contained upstream somehohw? Or should there be a comment that the tests are incomplete on purpose?

There's a bit of a split here between testing the visitor for Java, and testing this recipe and the overridden visitor here for Kotlin. That's at least in part because rewrite-kotlin is a separate repository not contained within openrewrite/rewrite. As such I'd only test anything here if it were related specifically to the method override we did for Kotlin. https://github.com/openrewrite/rewrite-static-analysis/blob/aaf0da8aca137d6fee220732447dffbbabae0929/src/main/java/org/openrewrite/staticanalysis/SimplifyBooleanExpression.java#L63-L77

I think the case you'd added here should be sufficiently covered already, but you're welcome to challenge me on that; I haven't looked into the specifics for Kotlin here.