Remove null checks before instanceof only for side-effect-free expressions

[martinfrancois]

Suggested review order: 31 of 52 (Score: 3)
Review first: #996

What's changed?

RemoveRedundantNullCheckBeforeInstanceof now makes no change when the expression tested by instanceof may have side effects. It still removes the null check for variables, fields and other expressions that only read a value.

The fix adds one condition to the if at the top of the helper method isRedundantNullCheck, whose first conjunct is unchanged from main:

if (nullCheck.getOperator() == J.Binary.Type.NotEqual &&
        !mayHaveSideEffects(instanceOf.getExpression())) {

visitBinary calls isRedundantNullCheck for both of the shapes it handles, the direct expr != null && expr instanceof T and the chained a && expr != null && expr instanceof T, so the single condition covers both. mayHaveSideEffects is SideEffects.mayHaveSideEffects, a package-private helper added to main by #959 and reached here through a new static import; this change calls it and does not modify it. Four recipes on main already call it to avoid the same problem, dropping an expression whose evaluation may be observable: RemoveDuplicateConditions, AllBranchesIdentical, RemoveUnconditionalValueOverwrite and SimplifyRedundantLogicalExpression. Only the expression that instanceof tests is passed to it, not the null-checked one, which is enough: the recipe changes nothing unless those two are semantically equal, and mayHaveSideEffects answers the same for two semantically equal trees.

What's your motivation?

Recipe: org.openrewrite.staticanalysis.RemoveRedundantNullCheckBeforeInstanceof.

Before

return next() != null && next() instanceof String;

Actual after the recipe

return next() instanceof String;

Expected after the recipe

(unchanged)

expr != null && expr instanceof T evaluates expr twice. The expr instanceof T left behind evaluates it once. Main uses SemanticallyEqual.areEqual alone to decide that the two occurrences are interchangeable, which proves that they mean the same thing, not that evaluating the expression twice is the same as evaluating it once.

The output that main produces still compiles, but the method it has rewritten can return a different value. Compiled and run with Java 21, direct() above returns false from the input source and true from the source main produces, because there next() is called once instead of twice. The lost evaluation reaches other observable behaviour too: when the tested expression throws on its second invocation and not on its first, the caller no longer sees that exception at all, because after the rewrite the second invocation never happens. Reproduced on 2.40.0 and on 2.41.0-SNAPSHOT built from main 5785534a. The recipe is listed in common-static-analysis.yml, so it runs for everyone who applies CommonStaticAnalysis.

Anything in particular you'd like reviewers to focus on?

This change alters an expectation that used to hold. Main's removeRedundantNullCheckWithMethodInvocation expected getValue() != null && getValue() instanceof String to become getValue() instanceof String. That name is gone: the same before source is now one of the three no-change scenarios in doNotChangeWhenNullCheckedExpressionIsMethodInvocation, with the after block removed.

Limits worth knowing:

• The recipe no longer removes the null check when the tested expression contains a method call, even a side-effect-free one such as list.get(0). The Lossless Semantic Tree, OpenRewrite's representation of the parsed source, does not record whether a called method's body changes state, not even for a getter that only returns a field, so no invocation can be ruled harmless. The same applies to a constructor call (doNotChangeWhenConstructorCall) and to an assignment or increment inside the tested expression (doNotChangeWhenArrayIndexHasSideEffect).
• In Groovy, a property read such as other.flag parses as a field access, so the null check is still removed there. Same as on main.
• Two reads of a volatile field are two separate actions in the Java memory model (JLS 17.4.2) and can return different values, and the null check is still removed in that case. Also the same as on main.

Have you considered any alternatives or workarounds?

One alternative is to mention in the recipe's description field the new case in which the recipe declines to remove the null check. I left that text unchanged, because the existing description does not claim that the null check is always removed. If you would rather see that carve-out spelled out, it is a one-line edit.

Any additional context

Pre-existing tests changed: RemoveRedundantNullCheckBeforeInstanceofTest.java.removeRedundantNullCheckWithMethodInvocation (removed).

This change adds 4 net test methods to RemoveRedundantNullCheckBeforeInstanceofTest, taking the focused class from 19 to 23 executions. They cover 7 new scenarios. Without the code change in this pull request, these 4 methods fail:

• doNotChangeWhenNullCheckedExpressionIsMethodInvocation, covering direct, null-on-left, and chained calls
• doNotChangeWhenConstructorCall
• doNotChangeWhenArrayIndexHasSideEffect
• doNotChangeWhenOnlyTheNullCheckedOperandHasSideEffects

removeRedundantNullCheckInChainedCondition passes either way and keeps the chained && shape covered.

This change was prepared with AI assistance (Claude Code). I reviewed the code, the tests and this description.

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've run ./gradlew build locally, and committed any resulting changes to recipes.csv
• I've formatted the lines I changed, without reformatting code I didn't touch