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 af779449a77..e0d54fea85a 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 @@ -765,6 +765,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 44bbaa49c37..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 @@ -19,6 +19,7 @@ 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.ParenthesizeVisitor; @@ -26,6 +27,8 @@ 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 { @@ -36,28 +39,38 @@ public J visitBinary(J.Binary binary, ExecutionContext ctx) { if (asBinary.getOperator() == J.Binary.Type.And) { if (isLiteralFalse(asBinary.getLeft())) { - j = asBinary.getLeft(); + 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(); + 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) { @@ -286,7 +299,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); } } @@ -432,6 +447,114 @@ private static J.Unary not(Expression sideRetained) { JavaType.Primitive.Boolean); } + /** + * 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}). + *

+ * 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. + *

+ * {@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) { + 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 isJava() && + 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; + } + + /** + * 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() { + @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, @@ -440,7 +563,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; @@ -462,7 +585,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; 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) + } + """ + ) + ); + } }