SimplifyBooleanExpressionVisitor: do not drop operands with side effects

[martinfrancois]

Suggested review order: 6 of 52 (Score: 8)
Review first: openrewrite/rewrite-static-analysis#969

What's changed?

SimplifyBooleanExpressionVisitor now applies a boolean identity only when the operand it deletes can be dropped unobserved.

Rewriting x && false to false, or x && x to x, preserves the value of an expression but deletes the evaluation of an operand. On main that happens by tree shape alone, so evaluation a program can observe disappears:

Input	Output on main
effect() && false	false - effect() never runs
next().value && next().value	next().value - one call instead of two
arr[5] && false	false - no ArrayIndexOutOfBoundsException
if (false && o instanceof String s) { s.length(); }	if (false) { s.length(); } - does not compile
if (o is String && false) { o.length } (Kotlin)	drops the smart cast - does not compile

With this change the visitor rewrites none of them.

A new predicate, isEvaluationFreeOfObservableEffects, gates the five places a simplification drops an evaluation or folds two into one: x && false, x || true, x && x, x || x and x.equals(x). It is a recursive allow list - literals, identifiers, parentheses, field access, non-modifying unaries, instanceof without a pattern, ternaries, and binary operators other than +, / and % (string concatenation can call a user defined toString(); / and % can throw ArithmeticException). Method calls, array access, casts, new, assignments and reads of volatile variables are rejected. String#isEmpty() and String#equals(Object) are the only invocations accepted, because the visitor already folds both and String is final, so the only effect either can have is a NullPointerException on a null receiver.

Field access and instanceof are accepted in Java sources only: in the other languages that inherit this visitor a.b is a property read that may run a getter, and a type test such as Kotlin's o is String narrows the operand for the code it guards.

A short circuited operand is never evaluated, but it can still declare a pattern variable the surrounding code reads, so declaresPatternVariable guards that case separately.

What's your motivation?

Recipe: SimplifyBooleanExpressionVisitor through its consuming recipes.

Before

case 0: yield (x = 1) == 1 || true;

Actual after the recipe

case 0: yield true;

Expected after the recipe

(unchanged)

This visitor has no recipe id of its own. InvertCondition and RemoveObjectsIsNull here, and SimplifyBooleanExpression, SimplifyConstantIfBranchExecution and SystemGetSecurityManagerToNull in rewrite-static-analysis and rewrite-migrate-java, all inherit its behaviour, in every language that extends the Java LST. Compiled and run with Java 21, the inputs above and main's output for them behave differently; the last two do not compile at all.

Confirmed real-world executions

Both executions used org.openrewrite.recipe:rewrite-static-analysis:2.41.0.

Project	Stars on 2026-08-16	Location
OpenJDK	23,233	DefiniteAssignment1.java at b545bc4e
OpenJDK 8 Updates	304	PackerImpl.java at aa3f9dea

In current OpenJDK, the released recipe changed 11 occurrences and removed assignments that the compiler regression test reads later. The generated source fails definite-assignment checks. In OpenJDK 8 Updates, it deletes the side-effecting retainAll(...) || true assertion expression and emits invalid Java.

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

No existing expectation changed: both touched test files gain lines only. Two gaps remain, and main behaves the same way in both, so neither is a regression:

• s.isEmpty() && false on a null s still folds to false, losing the NullPointerException, because those two String calls are on the allow list.
• Outside Java a bare identifier, as in flag && false, is still dropped. Rejecting unattributed identifiers as well would stop almost all simplification in the languages that do not attribute them.

Have you considered any alternatives or workarounds?

A deny list mirroring SideEffects.mayHaveSideEffects in rewrite-static-analysis would give both repositories one rule. I tried it and did not keep it: it treats a.isEmpty() as impure, so the visitor stops folding it and the existing simplifyLiteralNull rows fail, and it treats array access, casts and volatile reads as pure, which are exactly the cases this change is about. Switching later means replacing the body of isEvaluationFreeOfObservableEffects and nothing else, since every call site goes through it.

Any additional context

Pre-existing tests changed: None.

Adds 10 tests across rewrite-java-test, rewrite-groovy and rewrite-kotlin. Seven cover evaluation that must now be kept and fail without the code change; the other three pin simplifications this change keeps performing.

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