From 633a56a39b5aadcf99016c06bd85a5eb47bac172 Mon Sep 17 00:00:00 2001 From: martinfrancois Date: Mon, 10 Aug 2026 13:25:41 +0200 Subject: [PATCH 1/2] SimplifyBooleanExpressionVisitor: do not drop operands with side effects Boolean identities preserve the resulting value, not the evaluation that produced it. This visitor applied them by tree shape alone, so it dropped or de-duplicated evaluation that can mutate state, throw, block, synchronize, perform I/O or return a changing value: effect() && false folded to false, effect() || true to true, and next().value && next().value and nextString().equals(nextString()) each lost one of two evaluations. SimplifyBooleanExpression and SimplifyConstantIfBranchExecution reuse this visitor and inherited the same behavior. Gate every identity that drops or de-duplicates an operand on a single conservative purity and repeatability predicate. It accepts an allow list of node kinds only, rejects volatile reads, array access and casts, and among invocations allows only String#isEmpty() and String#equals(Object), both of which this visitor already constant folds. For review: a short-circuited operand is still dropped, except when it declares a pattern variable that the surrounding code reads, since dropping it would delete the declaration. The predicate also keeps eliding a few effects that were not preserved before it existed either, chiefly NullPointerException from a null receiver or from unboxing; its javadoc lists them. Outside Java only qualified field reads are guarded, because a property read there can run a getter, so a bare flag && false still simplifies exactly as it did before. --- .../SimplifyBooleanExpressionVisitorTest.java | 23 ++ .../SimplifyBooleanExpressionVisitorTest.java | 286 ++++++++++++++++++ .../SimplifyBooleanExpressionVisitor.java | 181 ++++++++++- 3 files changed, 481 insertions(+), 9 deletions(-) diff --git a/rewrite-groovy/src/test/java/org/openrewrite/groovy/cleanup/SimplifyBooleanExpressionVisitorTest.java b/rewrite-groovy/src/test/java/org/openrewrite/groovy/cleanup/SimplifyBooleanExpressionVisitorTest.java index 1d3c8484bcb..222c5d3f131 100644 --- a/rewrite-groovy/src/test/java/org/openrewrite/groovy/cleanup/SimplifyBooleanExpressionVisitorTest.java +++ b/rewrite-groovy/src/test/java/org/openrewrite/groovy/cleanup/SimplifyBooleanExpressionVisitorTest.java @@ -355,6 +355,29 @@ def doubleNegation(boolean g) { ); } + @Test + void retainPropertyReadThatMayCallAGetter() { + rewriteRun( + groovy( + """ + class A { + boolean getFlag() { + println("effect") + true + } + } + class B { + def m(A a) { + boolean b = a.flag && false + boolean c = a.flag || true + boolean d = a.flag && a.flag + } + } + """ + ) + ); + } + @Test void simplifyNotEqualsFalse() { rewriteRun( diff --git a/rewrite-java-test/src/test/java/org/openrewrite/java/cleanup/SimplifyBooleanExpressionVisitorTest.java b/rewrite-java-test/src/test/java/org/openrewrite/java/cleanup/SimplifyBooleanExpressionVisitorTest.java index 821285e3011..cc11c76513d 100644 --- a/rewrite-java-test/src/test/java/org/openrewrite/java/cleanup/SimplifyBooleanExpressionVisitorTest.java +++ b/rewrite-java-test/src/test/java/org/openrewrite/java/cleanup/SimplifyBooleanExpressionVisitorTest.java @@ -663,6 +663,292 @@ boolean booleanExpression() { ); } + @Test + void retainEvaluationDroppedByBooleanIdentities() { + rewriteRun( + java( + """ + class Test { + static class State { + boolean value; + } + + boolean effect() { + return true; + } + + State next() { + return new State(); + } + + String nextString() { + return ""; + } + + boolean andFalse() { + return effect() && false; + } + + boolean orTrue() { + return effect() || true; + } + + boolean repeatedField() { + return next().value && next().value; + } + + boolean repeatedEquals() { + return nextString().equals(nextString()); + } + } + """ + ) + ); + } + + @Test + void retainEvaluationNestedInsideDroppedOperand() { + rewriteRun( + java( + """ + class Test { + static class State { + boolean value; + } + + boolean[] flags = new boolean[1]; + + State next() { + return new State(); + } + + Boolean boxed() { + return Boolean.TRUE; + } + + int denominator() { + return 0; + } + + int index() { + return 0; + } + + boolean nestedInFieldAccess() { + return next().value && false; + } + + boolean nestedInArrayAccess() { + return flags[index()] || true; + } + + boolean nestedInCast() { + return (boolean) boxed() && false; + } + + boolean nestedInParentheses() { + return /*keep*/(next().value) || true; + } + + boolean nestedInDivision() { + return 1 / denominator() > 0 && false; + } + } + """ + ) + ); + } + + @Test + void retainEvaluationRejectedWithoutAnyMethodCall() { + rewriteRun( + java( + """ + class Test { + boolean[] flags = new boolean[1]; + Boolean boxed = Boolean.TRUE; + int zero = 0; + int counter; + + boolean arrayAccess() { + return flags[0] && false; + } + + boolean cast() { + return (boolean) boxed || true; + } + + boolean division() { + return 1 / zero > 0 && false; + } + + boolean modulo() { + return 1 % zero > 0 || true; + } + + boolean increment() { + return counter++ > 0 && false; + } + + boolean concatenation() { + return ("a" + boxed).isEmpty() && false; + } + } + """ + ) + ); + } + + @Test + void retainVolatileReads() { + rewriteRun( + java( + """ + class Test { + volatile boolean flag; + + boolean andFalse() { + return flag && false; + } + + boolean orTrue() { + return flag || true; + } + + boolean repeated() { + return flag && flag; + } + } + """ + ) + ); + } + + @Test + void removeShortCircuitedRightOperand() { + rewriteRun( + java( + """ + class Test { + boolean effect() { + return true; + } + + boolean andFalse() { + return false && effect(); + } + + boolean orTrue() { + return true || effect(); + } + } + """, + """ + class Test { + boolean effect() { + return true; + } + + boolean andFalse() { + return false; + } + + boolean orTrue() { + return true; + } + } + """ + ) + ); + } + + @Test + void retainPatternVariableDeclaredByShortCircuitedOperand() { + rewriteRun( + java( + """ + class Test { + int andFalse(Object o) { + if (false && o instanceof String s) { + return s.length(); + } + return 0; + } + + int orTrue(Object o) { + if (true || !(o instanceof String s)) { + return 0; + } + return s.length(); + } + } + """ + ) + ); + } + + @Test + void dropShortCircuitedOperandWhosePatternVariableCannotEscape() { + rewriteRun( + java( + """ + import java.util.List; + + class Test { + boolean orTrue(List l) { + return true || l.stream().anyMatch(o -> o instanceof String s && !s.isEmpty()); + } + } + """, + """ + import java.util.List; + + class Test { + boolean orTrue(List l) { + return true; + } + } + """ + ) + ); + } + + @Test + void stillSimplifyEvaluationFreeIdentities() { + rewriteRun( + java( + """ + class Test { + boolean field; + + void m(boolean a, String s) { + boolean b = a && false; + boolean c = a || true; + boolean d = a && a; + boolean e = a || a; + boolean f = this.field && false; + boolean g = s.equals(s); + } + } + """, + """ + class Test { + boolean field; + + void m(boolean a, String s) { + boolean b = false; + boolean c = true; + boolean d = a; + boolean e = a; + boolean f = false; + boolean g = true; + } + } + """ + ) + ); + } + @CsvSource(delimiterString = "//", textBlock = """ a == null || a.isEmpty() // a == null || a.isEmpty() a == null || !a.isEmpty() // a == null || !a.isEmpty() diff --git a/rewrite-java/src/main/java/org/openrewrite/java/cleanup/SimplifyBooleanExpressionVisitor.java b/rewrite-java/src/main/java/org/openrewrite/java/cleanup/SimplifyBooleanExpressionVisitor.java index 22092471649..50d6f2ac8e5 100644 --- a/rewrite-java/src/main/java/org/openrewrite/java/cleanup/SimplifyBooleanExpressionVisitor.java +++ b/rewrite-java/src/main/java/org/openrewrite/java/cleanup/SimplifyBooleanExpressionVisitor.java @@ -19,12 +19,15 @@ import org.openrewrite.ExecutionContext; import org.openrewrite.SourceFile; import org.openrewrite.Tree; +import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.JavaVisitor; import org.openrewrite.java.MethodMatcher; import org.openrewrite.java.search.SemanticallyEqual; import org.openrewrite.java.tree.*; import org.openrewrite.marker.Markers; +import java.util.concurrent.atomic.AtomicBoolean; + import static java.util.Collections.emptyList; public class SimplifyBooleanExpressionVisitor extends JavaVisitor { @@ -35,28 +38,42 @@ public J visitBinary(J.Binary binary, ExecutionContext ctx) { if (asBinary.getOperator() == J.Binary.Type.And) { if (isLiteralFalse(asBinary.getLeft())) { - j = asBinary.getLeft(); + // The right side is short-circuited, so it is never evaluated, but it may still + // declare a pattern variable that the surrounding code reads. + if (!declaresPatternVariable(asBinary.getRight())) { + j = asBinary.getLeft(); + } } else if (isLiteralFalse(asBinary.getRight())) { - j = asBinary.getRight().withPrefix(asBinary.getRight().getPrefix().withWhitespace("")); + if (isEvaluationFreeOfObservableEffects(asBinary.getLeft())) { + j = asBinary.getRight().withPrefix(asBinary.getRight().getPrefix().withWhitespace("")); + } } else if (isLiteralTrue(asBinary.getLeft())) { j = asBinary.getRight(); } else if (isLiteralTrue(asBinary.getRight())) { j = asBinary.getLeft().withPrefix(asBinary.getLeft().getPrefix().withWhitespace("")); } else if (!(asBinary.getLeft() instanceof MethodCall) && - SemanticallyEqual.areEqual(asBinary.getLeft(), asBinary.getRight())) { + SemanticallyEqual.areEqual(asBinary.getLeft(), asBinary.getRight()) && + isEvaluationFreeOfObservableEffects(asBinary.getLeft())) { j = asBinary.getLeft(); } } else if (asBinary.getOperator() == J.Binary.Type.Or) { if (isLiteralTrue(asBinary.getLeft())) { - j = asBinary.getLeft(); + // The right side is short-circuited, so it is never evaluated, but it may still + // declare a pattern variable that the surrounding code reads. + if (!declaresPatternVariable(asBinary.getRight())) { + j = asBinary.getLeft(); + } } else if (isLiteralTrue(asBinary.getRight())) { - j = asBinary.getRight().withPrefix(asBinary.getRight().getPrefix().withWhitespace("")); + if (isEvaluationFreeOfObservableEffects(asBinary.getLeft())) { + j = asBinary.getRight().withPrefix(asBinary.getRight().getPrefix().withWhitespace("")); + } } else if (isLiteralFalse(asBinary.getLeft())) { j = asBinary.getRight(); } else if (isLiteralFalse(asBinary.getRight())) { j = asBinary.getLeft().withPrefix(asBinary.getLeft().getPrefix().withWhitespace("")); } else if (!(asBinary.getLeft() instanceof MethodCall) && - SemanticallyEqual.areEqual(asBinary.getLeft(), asBinary.getRight())) { + SemanticallyEqual.areEqual(asBinary.getLeft(), asBinary.getRight()) && + isEvaluationFreeOfObservableEffects(asBinary.getLeft())) { j = asBinary.getLeft(); } } else if (asBinary.getOperator() == J.Binary.Type.Equal) { @@ -284,7 +301,9 @@ public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext execu Expression arg = asMethod.getArguments().get(0); if (arg instanceof J.Literal && select instanceof J.Literal) { return booleanLiteral(method, ((J.Literal) select).getValue().equals(((J.Literal) arg).getValue())); - } else if (SemanticallyEqual.areEqual(select, arg)) { + } else if (isEvaluationFreeOfObservableEffects(select) && + isEvaluationFreeOfObservableEffects(arg) && + SemanticallyEqual.areEqual(select, arg)) { return booleanLiteral(method, true); } } @@ -430,6 +449,150 @@ private static J.Unary not(Expression sideRetained) { JavaType.Primitive.Boolean); } + /** + * A single, deliberately conservative purity and repeatability predicate, used everywhere this + * visitor would otherwise delete an evaluation ({@code effect() && false}, {@code effect() || true}) + * or fold two evaluations into one ({@code x && x}, {@code x.equals(x)}). + *

+ * Boolean identities preserve the resulting value, not the evaluation that produced it, so they are + * only sound when re-ordering, dropping or de-duplicating that evaluation cannot be observed. Only an + * allow list of node kinds is accepted; anything else, in particular an arbitrary method invocation, + * constructor call, assignment or increment anywhere in the subtree, is assumed to mutate state, throw, + * block, synchronize, perform I/O or return a changing value. Array access and casts are rejected + * outright, which also covers method calls nested inside them, and reads of {@code volatile} variables + * are rejected because they are synchronization actions whose repetition is observable. + *

+ * The only invocations on the allow list are {@code String#isEmpty()} and {@code String#equals(Object)}, + * the two calls this visitor already constant folds. Both are declared on the {@code final} class + * {@code String}, so neither can be overridden, and both only read {@code String} state, so the sole + * effect they can have is a {@link NullPointerException} on a {@code null} receiver. + *

+ * These elisions are accepted deliberately, because rejecting them would stop this visitor from + * cleaning up the literal-heavy conditions that Refaster templates and constant folding produce, and + * none of them was preserved before this predicate existed either: + *

    + *
  • a {@link NullPointerException} from a {@code null} receiver, both for the two allowed + * {@code String} calls and for a field read such as {@code a.b};
    • + *
  • a {@link NullPointerException} from unboxing, as in {@code Boolean b; b && false};
    • + *
  • the static initializer that reading a non-constant static field of another class runs;
    • + *
  • a {@code volatile} read whose variable carries no type attribution, which the languages + * that do not attribute variables rely on.
    • + *
+ * The allow list describes Java semantics. Groovy, Kotlin and the other languages that inherit this + * visitor resolve more of it to user code: {@code a.b} is a property read that runs a getter, and + * {@code ==}, {@code <}, {@code !}, {@code &&} and {@code ?:} dispatch to a user-definable + * {@code equals}, {@code compareTo}, {@code not} or {@code asBoolean}. Only the qualified read is + * guarded here, because it is the one case that can be rejected without also rejecting every + * unattributed identifier and operator, which would leave those languages unable to simplify + * anything but literals. So {@code a.b && false} keeps its operand outside Java, while a bare + * {@code flag && false} or an operator such as {@code (a == b) && false} still drops it, exactly as + * both did before this predicate existed. Closing that remainder needs a language-aware operator + * model rather than a wider allow list. + * + * @param tree the subtree that a simplification would drop or evaluate fewer times + * @return true only when that is unobservable, modulo the elisions listed above + */ + protected boolean isEvaluationFreeOfObservableEffects(@Nullable J tree) { + if (tree instanceof J.Literal || tree instanceof J.Empty) { + return true; + } + if (tree instanceof J.Identifier) { + return isStableRead(((J.Identifier) tree).getFieldType()); + } + if (tree instanceof J.FieldAccess) { + J.FieldAccess fieldAccess = (J.FieldAccess) tree; + return isJava() && + isStableRead(fieldAccess.getName().getFieldType()) && + isEvaluationFreeOfObservableEffects(fieldAccess.getTarget()); + } + if (tree instanceof J.Parentheses) { + return isEvaluationFreeOfObservableEffects(((J.Parentheses) tree).getTree()); + } + if (tree instanceof J.ControlParentheses) { + return isEvaluationFreeOfObservableEffects(((J.ControlParentheses) tree).getTree()); + } + if (tree instanceof J.Unary) { + J.Unary unary = (J.Unary) tree; + return !unary.getOperator().isModifying() && isEvaluationFreeOfObservableEffects(unary.getExpression()); + } + if (tree instanceof J.InstanceOf) { + J.InstanceOf instanceOf = (J.InstanceOf) tree; + return instanceOf.getPattern() == null && isEvaluationFreeOfObservableEffects(instanceOf.getExpression()); + } + if (tree instanceof J.Ternary) { + J.Ternary ternary = (J.Ternary) tree; + return isEvaluationFreeOfObservableEffects(ternary.getCondition()) && + isEvaluationFreeOfObservableEffects(ternary.getTruePart()) && + isEvaluationFreeOfObservableEffects(ternary.getFalsePart()); + } + if (tree instanceof J.Binary) { + J.Binary binary = (J.Binary) tree; + J.Binary.Type operator = binary.getOperator(); + if (operator == J.Binary.Type.Addition || // String concatenation can call a user defined `toString()` + operator == J.Binary.Type.Division || operator == J.Binary.Type.Modulo) { // Can throw `ArithmeticException` + return false; + } + return isEvaluationFreeOfObservableEffects(binary.getLeft()) && + isEvaluationFreeOfObservableEffects(binary.getRight()); + } + if (tree instanceof J.MethodInvocation) { + J.MethodInvocation method = (J.MethodInvocation) tree; + if (!isEmpty.matches(method) && !equals.matches(method)) { + return false; + } + if (!isEvaluationFreeOfObservableEffects(method.getSelect())) { + return false; + } + for (Expression argument : method.getArguments()) { + if (!isEvaluationFreeOfObservableEffects(argument)) { + return false; + } + } + return true; + } + return false; + } + + private static boolean isStableRead(JavaType.@Nullable Variable variable) { + return variable == null || !variable.hasFlags(Flag.Volatile); + } + + private boolean isJava() { + return getCursor().firstEnclosing(SourceFile.class) instanceof J.CompilationUnit; + } + + /** + * A short-circuited operand is never evaluated, but it can still declare a pattern variable that the + * surrounding code reads: {@code if (false && o instanceof String s) { s.length(); }} compiles, because + * {@code s} is definitely matched wherever the condition is true. Dropping that operand deletes the + * declaration and produces source that no longer compiles. + *

+ * This over-approximates: a pattern variable only escapes the operand through {@code &&}, {@code ||}, + * {@code !} and parentheses, so some of the operands this rejects could in fact be dropped. The one + * exception made is a lambda body, whose pattern variables are scoped to the lambda and can never be + * read by the surrounding code. + * + * @param expression the short-circuited operand a simplification would drop + * @return true if dropping it would delete a pattern variable declaration + */ + private static boolean declaresPatternVariable(Expression expression) { + return new JavaIsoVisitor() { + @Override + public J.InstanceOf visitInstanceOf(J.InstanceOf instanceOf, AtomicBoolean found) { + if (instanceOf.getPattern() != null) { + found.set(true); + return instanceOf; + } + return super.visitInstanceOf(instanceOf, found); + } + + @Override + public J.Lambda visitLambda(J.Lambda lambda, AtomicBoolean found) { + return lambda; + } + }.reduce(expression, new AtomicBoolean()).get(); + } + /** * In Java, {@code !} only applies to boolean expressions, so {@code !!x} is always * equivalent to {@code x}. In other languages like JavaScript/TypeScript and Groovy, @@ -438,7 +601,7 @@ private static J.Unary not(Expression sideRetained) { * semantics when {@code x} is not boolean-typed. */ private boolean canSimplifyDoubleNegation(Expression innerExpression) { - if (getCursor().firstEnclosing(SourceFile.class) instanceof J.CompilationUnit) { + if (isJava()) { return true; } return innerExpression.getType() == JavaType.Primitive.Boolean; @@ -460,7 +623,7 @@ private boolean canSimplifyDoubleNegation(Expression innerExpression) { * @return true if the equals comparison can be safely simplified */ protected boolean shouldSimplifyEqualsOn(J j) { - if (getCursor().firstEnclosing(SourceFile.class) instanceof J.CompilationUnit) { + if (isJava()) { return true; } return j instanceof Expression && ((Expression) j).getType() == JavaType.Primitive.Boolean; From fd78de75921f8edf7af74aefceb922e40bfe478b Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 11 Aug 2026 22:38:39 +0200 Subject: [PATCH 2/2] Guard type tests outside Java and trim commentary A Kotlin `is` never sets a pattern but still narrows the operand for the guarded code, so dropping it produced source that no longer compiles. --- .../SimplifyBooleanExpressionVisitor.java | 72 +++++-------------- .../SimplifyBooleanExpressionVisitorTest.java | 19 +++++ 2 files changed, 35 insertions(+), 56 deletions(-) diff --git a/rewrite-java/src/main/java/org/openrewrite/java/cleanup/SimplifyBooleanExpressionVisitor.java b/rewrite-java/src/main/java/org/openrewrite/java/cleanup/SimplifyBooleanExpressionVisitor.java index ab31a1e6269..9879a0fa01b 100644 --- a/rewrite-java/src/main/java/org/openrewrite/java/cleanup/SimplifyBooleanExpressionVisitor.java +++ b/rewrite-java/src/main/java/org/openrewrite/java/cleanup/SimplifyBooleanExpressionVisitor.java @@ -39,8 +39,6 @@ public J visitBinary(J.Binary binary, ExecutionContext ctx) { if (asBinary.getOperator() == J.Binary.Type.And) { if (isLiteralFalse(asBinary.getLeft())) { - // The right side is short-circuited, so it is never evaluated, but it may still - // declare a pattern variable that the surrounding code reads. if (!declaresPatternVariable(asBinary.getRight())) { j = asBinary.getLeft(); } @@ -59,8 +57,6 @@ public J visitBinary(J.Binary binary, ExecutionContext ctx) { } } else if (asBinary.getOperator() == J.Binary.Type.Or) { if (isLiteralTrue(asBinary.getLeft())) { - // The right side is short-circuited, so it is never evaluated, but it may still - // declare a pattern variable that the surrounding code reads. if (!declaresPatternVariable(asBinary.getRight())) { j = asBinary.getLeft(); } @@ -452,47 +448,18 @@ private static J.Unary not(Expression sideRetained) { } /** - * A single, deliberately conservative purity and repeatability predicate, used everywhere this - * visitor would otherwise delete an evaluation ({@code effect() && false}, {@code effect() || true}) - * or fold two evaluations into one ({@code x && x}, {@code x.equals(x)}). + * Whether dropping or de-duplicating the evaluation of {@code tree} is unobservable, so that a boolean + * identity may rewrite away an operand ({@code effect() && false}) or fold two into one ({@code x && x}). *

- * Boolean identities preserve the resulting value, not the evaluation that produced it, so they are - * only sound when re-ordering, dropping or de-duplicating that evaluation cannot be observed. Only an - * allow list of node kinds is accepted; anything else, in particular an arbitrary method invocation, - * constructor call, assignment or increment anywhere in the subtree, is assumed to mutate state, throw, - * block, synchronize, perform I/O or return a changing value. Array access and casts are rejected - * outright, which also covers method calls nested inside them, and reads of {@code volatile} variables - * are rejected because they are synchronization actions whose repetition is observable. + * Only an allow list of node kinds is accepted; anything else is assumed to have an effect. + * {@code String#isEmpty()} and {@code String#equals(Object)} are on it because {@code String} is final + * and both only read its state. Deliberately not preserved, as they were not before either: a + * {@link NullPointerException} from a {@code null} receiver or from unboxing, the static initializer a + * field read runs, and a {@code volatile} read carrying no type attribution. *

- * The only invocations on the allow list are {@code String#isEmpty()} and {@code String#equals(Object)}, - * the two calls this visitor already constant folds. Both are declared on the {@code final} class - * {@code String}, so neither can be overridden, and both only read {@code String} state, so the sole - * effect they can have is a {@link NullPointerException} on a {@code null} receiver. - *

- * These elisions are accepted deliberately, because rejecting them would stop this visitor from - * cleaning up the literal-heavy conditions that Refaster templates and constant folding produce, and - * none of them was preserved before this predicate existed either: - *

    - *
  • a {@link NullPointerException} from a {@code null} receiver, both for the two allowed - * {@code String} calls and for a field read such as {@code a.b};
    • - *
  • a {@link NullPointerException} from unboxing, as in {@code Boolean b; b && false};
    • - *
  • the static initializer that reading a non-constant static field of another class runs;
    • - *
  • a {@code volatile} read whose variable carries no type attribution, which the languages - * that do not attribute variables rely on.
    • - *
- * The allow list describes Java semantics. Groovy, Kotlin and the other languages that inherit this - * visitor resolve more of it to user code: {@code a.b} is a property read that runs a getter, and - * {@code ==}, {@code <}, {@code !}, {@code &&} and {@code ?:} dispatch to a user-definable - * {@code equals}, {@code compareTo}, {@code not} or {@code asBoolean}. Only the qualified read is - * guarded here, because it is the one case that can be rejected without also rejecting every - * unattributed identifier and operator, which would leave those languages unable to simplify - * anything but literals. So {@code a.b && false} keeps its operand outside Java, while a bare - * {@code flag && false} or an operator such as {@code (a == b) && false} still drops it, exactly as - * both did before this predicate existed. Closing that remainder needs a language-aware operator - * model rather than a wider allow list. - * - * @param tree the subtree that a simplification would drop or evaluate fewer times - * @return true only when that is unobservable, modulo the elisions listed above + * {@code J.FieldAccess} and {@code J.InstanceOf} are accepted for Java only, where they cannot run a + * getter or narrow a type. Other operators still dispatch to user code in Groovy and Kotlin, but + * rejecting those too would leave both unable to simplify anything beyond literals. */ protected boolean isEvaluationFreeOfObservableEffects(@Nullable J tree) { if (tree instanceof J.Literal || tree instanceof J.Empty) { @@ -519,7 +486,9 @@ protected boolean isEvaluationFreeOfObservableEffects(@Nullable J tree) { } if (tree instanceof J.InstanceOf) { J.InstanceOf instanceOf = (J.InstanceOf) tree; - return instanceOf.getPattern() == null && isEvaluationFreeOfObservableEffects(instanceOf.getExpression()); + return isJava() && + instanceOf.getPattern() == null && + isEvaluationFreeOfObservableEffects(instanceOf.getExpression()); } if (tree instanceof J.Ternary) { J.Ternary ternary = (J.Ternary) tree; @@ -564,18 +533,9 @@ private boolean isJava() { } /** - * A short-circuited operand is never evaluated, but it can still declare a pattern variable that the - * surrounding code reads: {@code if (false && o instanceof String s) { s.length(); }} compiles, because - * {@code s} is definitely matched wherever the condition is true. Dropping that operand deletes the - * declaration and produces source that no longer compiles. - *

- * This over-approximates: a pattern variable only escapes the operand through {@code &&}, {@code ||}, - * {@code !} and parentheses, so some of the operands this rejects could in fact be dropped. The one - * exception made is a lambda body, whose pattern variables are scoped to the lambda and can never be - * read by the surrounding code. - * - * @param expression the short-circuited operand a simplification would drop - * @return true if dropping it would delete a pattern variable declaration + * Whether dropping the never evaluated {@code expression} would delete a pattern variable the + * surrounding code still reads, as in {@code if (false && o instanceof String s) { s.length(); }}. + * Lambda bodies are skipped, as their pattern variables cannot escape. */ private static boolean declaresPatternVariable(Expression expression) { return new JavaIsoVisitor() { diff --git a/rewrite-kotlin/src/test/java/org/openrewrite/kotlin/cleanup/SimplifyBooleanExpressionVisitorTest.java b/rewrite-kotlin/src/test/java/org/openrewrite/kotlin/cleanup/SimplifyBooleanExpressionVisitorTest.java index 102f8600c20..90546418ec3 100644 --- a/rewrite-kotlin/src/test/java/org/openrewrite/kotlin/cleanup/SimplifyBooleanExpressionVisitorTest.java +++ b/rewrite-kotlin/src/test/java/org/openrewrite/kotlin/cleanup/SimplifyBooleanExpressionVisitorTest.java @@ -108,4 +108,23 @@ fun check(b: Boolean) { ) ); } + + @Test + void retainTypeTestThatSmartCasts() { + rewriteRun( + kotlin( + """ + fun f(o: Any) { + if (o is String && false) { + println(o.length) + } + if (o !is String || true) { + return + } + println(o.length) + } + """ + ) + ); + } }