Remove null checks before literal equality only for side-effect-free expressions

[martinfrancois]

Suggested review order: 35 of 52 (Score: 2.5)
Review first: openrewrite/rewrite-migrate-java#1191

What's changed?

RemoveRedundantNullCheckBeforeLiteralEquals rewrites expr != null && "literal".equals(expr) into "literal".equals(expr). With this change it makes no change at all when evaluating expr may have side effects.

isRedundantNullCheck, which decides whether a != null test may be dropped, now returns false, meaning "keep the null check, change nothing", when either of two new checks fires on the argument of equals. Checking that one node is enough: the recipe removes nothing unless SemanticallyEqual.areEqual reports it to be the same expression as the null-checked one.

• SideEffects.mayHaveSideEffects, the package-private helper that pull request Do not delete expressions that may have side effects #959 added to this package; this change does not modify it.
• readsVolatileField, a new private method in this recipe's visitor. A volatile read is not a side effect, so SideEffects does not report it, but it is a synchronization action: JLS 17.4.2 lists a read of a volatile variable among them, so dropping one of two reads can change what another thread sees.

visitBinary matches a direct shape and a chained shape, enabled && expr != null && "literal".equals(expr), and calls isRedundantNullCheck on both, so the guard covers both.

What's your motivation?

Recipe: org.openrewrite.staticanalysis.RemoveRedundantNullCheckBeforeLiteralEquals.

Before

return next() != null && "ok".equals(next());

Actual after the recipe

return "ok".equals(next());

Expected after the recipe

(unchanged)

The input evaluates expr twice, the output evaluates it once. SemanticallyEqual.areEqual proves that the two occurrences of expr mean the same thing, not that evaluating it once is the same as evaluating it twice.

The output that main produces still compiles, but it can produce a different value at runtime. With a next() that returns "no" on its first call and "ok" on its second, the input returns true: the null check consumes the first call and equals compares "ok" with the "ok" of the second. The output returns false, because equals now sees the first call. The same applies to values[i++], to an assignment such as (s = t) and to a volatile read.

A recipe must not stop evaluating an expression that may have side effects. Issue #953 describes this class of problem, and pull request #959 (merged as commit 70da659) applied that rule to four other recipes and moved the side-effect check that SimplifyRedundantLogicalExpression already carried into the new shared SideEffects helper. Neither the issue nor that pull request mentions this recipe. Reproduced on v2.40.0 and on 2.41.0-SNAPSHOT from main 5785534.

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

One existing test is deleted, because this change reverses an expectation that held until now: removeRedundantNullCheckWithMethodInvocation asserted that getValue() != null && "expected".equals(getValue()) becomes "expected".equals(getValue()). The new doNotChangeWhenNullCheckedExpressionIsMethodInvocation covers the same shape and asserts that it is now left unchanged.

Three limits, all with this change applied:

• A method call anywhere in the null-checked expression now stops the removal, including a simple getter such as list.get(0): SideEffects cannot tell a read-only getter from one that changes state, so it reports every method call as possibly effectful.
• In Groovy only an explicit call is caught. A property read backed by a getter is written without parentheses and parses as a property access rather than a method invocation, so SideEffects sees no call and the null check in front of it is still removed. Groovy is partly fixed, not fully fixed.
• readsVolatileField asks every identifier in the argument of equals whether its JavaType.Variable field type carries Flag.Volatile. An LST parsed without type attribution has no such field type, so the check misses that case.

An array access such as values[0] and a cast such as (String) o are still simplified: neither evaluates anything that may have side effects.

Have you considered any alternatives or workarounds?

One alternative is to put the volatile check in SideEffects instead of in this recipe. Four other recipes call SideEffects.mayHaveSideEffects: AllBranchesIdentical, RemoveDuplicateConditions, RemoveUnconditionalValueOverwrite and SimplifyRedundantLogicalExpression. Moving the check into the shared helper would therefore change their runtime behaviour too, not only this recipe's.

The opposite option is to drop the volatile check entirely, leaving this change the same shape as the fixes in #959: a call to mayHaveSideEffects and nothing else.

Either option is about ten lines: the private readsVolatileField method and its call inside isRedundantNullCheck, plus the test doNotChangeWhenNullCheckedFieldIsVolatile. That test would then belong with the shared helper, which has no test class of its own today, or be deleted along with the check.

Any additional context

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

This change adds 8 tests to RemoveRedundantNullCheckBeforeLiteralEqualsTest and deletes the test named above, taking that class from 16 tests to 23. Without the code change in this pull request, these 4 new tests fail:

• doNotChangeWhenArrayIndexHasSideEffect
• doNotChangeWhenNullCheckedExpressionIsAssignment
• doNotChangeWhenNullCheckedExpressionIsMethodInvocation
• doNotChangeWhenNullCheckedFieldIsVolatile

The other 4 new tests assert that expressions without side effects are still simplified.

It also adds a new Groovy test class of the same name, in org.openrewrite.staticanalysis.groovy, with 2 tests. Without the code change in this pull request, 1 of them fails: doNotChangeWhenNullCheckedExpressionIsMethodInvocation. The other, removeRedundantNullCheck, asserts that the null check in front of a Groovy parameter is still removed.

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

I ran the formatter with the repository's .editorconfig. It also wanted to re-indent lines that this change does not touch, so I left those alone and kept the diff limited to this change.

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