From a6d524fd97056ef2166945c2be63075527210432 Mon Sep 17 00:00:00 2001 From: martinfrancois Date: Tue, 11 Aug 2026 19:39:08 +0200 Subject: [PATCH 1/3] SimplifyBooleanExpressionVisitor: correctly negate a nested ternary maybeNegate negated a J.Ternary by flipping its condition and swapping its branches. Doing both preserves the ternary's value instead of negating it, so `!(a ? b ? x : y : z)` lost the negation of the nested branch: cycle one produced `a ? !b ? y : x : !z`, and the next cycle normalized that back to `a ? b ? x : y : !z`, which also broke single-cycle convergence. Negate the ternary's branches instead, the same way unpackExpression already negates the branches of a directly negated ternary. --- .../SimplifyBooleanExpressionVisitorTest.java | 40 +++++++++++++++++++ .../SimplifyBooleanExpressionVisitor.java | 14 +++---- 2 files changed, 46 insertions(+), 8 deletions(-) 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..0d7201ce7e8 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 @@ -592,6 +592,46 @@ boolean m4(boolean a, boolean b, boolean c) { ); } + @Test + void nestedTernaryNegation() { + rewriteRun( + java( + """ + public class A { + boolean m1(boolean a, boolean b, boolean x, boolean y, boolean z) { + return !(a ? b ? x : y : z); + } + boolean m2(boolean a, boolean b, boolean x, boolean y, boolean z) { + return !(a ? x : b ? y : z); + } + boolean m3(boolean a, boolean b, boolean x, boolean y, boolean z) { + return !(a ? !b ? x : y : z); + } + boolean m4(boolean a, boolean b, boolean x, boolean y, boolean z) { + return a ? b ? x : y : z; + } + } + """, + """ + public class A { + boolean m1(boolean a, boolean b, boolean x, boolean y, boolean z) { + return a ? b ? !x : !y : !z; + } + boolean m2(boolean a, boolean b, boolean x, boolean y, boolean z) { + return a ? !x : b ? !y : !z; + } + boolean m3(boolean a, boolean b, boolean x, boolean y, boolean z) { + return a ? b ? !y : !x : !z; + } + boolean m4(boolean a, boolean b, boolean x, boolean y, boolean z) { + return a ? b ? x : y : z; + } + } + """ + ) + ); + } + @Test void differentFieldAccesses() { rewriteRun( 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..420ff761db5 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 @@ -232,15 +232,13 @@ private Expression maybeNegate(Expression expr) { } else if (expr instanceof J.Unary && ((J.Unary) expr).getOperator() == J.Unary.Type.Not) { return ((J.Unary) expr).getExpression().withPrefix(expr.getPrefix()); } else if (expr instanceof J.Ternary) { + // The negation of `c ? t : f` is `c ? !t : !f`. Flipping the condition and swapping the + // branches instead would preserve the ternary's value rather than negate it. J.Ternary ternary = (J.Ternary) expr; - Expression negatedCondition = maybeNegate(ternary.getCondition()); - if (negatedCondition != ternary.getCondition()) { - return ternary - .withCondition(negatedCondition) - .withTruePart(ternary.getFalsePart()) - .withFalsePart(ternary.getTruePart()) - .withPrefix(expr.getPrefix()); - } + return ternary + .withTruePart(maybeNegate(ternary.getTruePart())) + .withFalsePart(maybeNegate(ternary.getFalsePart())) + .withPrefix(expr.getPrefix()); } else if (isLiteralTrue(expr)) { return ((J.Literal) expr).withValue(false).withValueSource("false"); } else if (isLiteralFalse(expr)) { From fcebfc16459346744eb019b30c11ff9d30de971e Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 11 Aug 2026 20:17:31 +0200 Subject: [PATCH 2/3] Keep parentheses when a negation becomes a ternary Replacing `!(...)` with a ternary drops the parentheses, so `!(a ? b : c) && d` printed as `a ? !b : !c && d`, which regroups as `a ? !b : (!c && d)`. Wrap the ternary again when the parent binds tighter than a ternary does. Also keep the prefix outside the parentheses that `not()` inserts, so it emits `!(o instanceof String)` rather than `!( o instanceof String)`. --- .../SimplifyBooleanExpressionVisitorTest.java | 62 +++++++++++++++++++ .../SimplifyBooleanExpressionVisitor.java | 24 ++++++- 2 files changed, 84 insertions(+), 2 deletions(-) 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 0d7201ce7e8..af779449a77 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 @@ -632,6 +632,68 @@ boolean m4(boolean a, boolean b, boolean x, boolean y, boolean z) { ); } + @Test + void negatedTernaryBranchWrappedInParentheses() { + rewriteRun( + java( + """ + public class A { + boolean m1(boolean a, Object o, boolean c) { + return !(a ? o instanceof String : c); + } + } + """, + """ + public class A { + boolean m1(boolean a, Object o, boolean c) { + return a ? !(o instanceof String) : !c; + } + } + """ + ) + ); + } + + @Test + void negatedTernaryAsOperandKeepsParentheses() { + rewriteRun( + java( + """ + public class A { + boolean m1(boolean a, boolean b, boolean c, boolean d) { + return !(a ? b : c) && d; + } + boolean m2(boolean a, boolean b, boolean c, boolean d) { + return !(a ? b : c) ? c : d; + } + boolean m3(boolean a, boolean b, boolean c, boolean d) { + return d ? !(a ? b : c) : d; + } + boolean m4(boolean a, boolean b, boolean c) { + return !!(a ? b : c); + } + } + """, + """ + public class A { + boolean m1(boolean a, boolean b, boolean c, boolean d) { + return (a ? !b : !c) && d; + } + boolean m2(boolean a, boolean b, boolean c, boolean d) { + return (a ? !b : !c) ? c : d; + } + boolean m3(boolean a, boolean b, boolean c, boolean d) { + return d ? a ? !b : !c : d; + } + boolean m4(boolean a, boolean b, boolean c) { + return a ? b : c; + } + } + """ + ) + ); + } + @Test void differentFieldAccesses() { rewriteRun( 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 420ff761db5..8b1fcbaec82 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 @@ -146,6 +146,12 @@ public J visitUnary(J.Unary unary, ExecutionContext ctx) { if (asUnary.getOperator() == J.Unary.Type.Not) { j = unpackExpression(asUnary.getExpression(), asUnary); + if (j instanceof J.Ternary && parentBindsTighterThanTernary(unary)) { + j = new J.Parentheses<>(Tree.randomId(), + Space.EMPTY, + Markers.EMPTY, + JRightPadded.build(((Expression) j).withPrefix(Space.EMPTY))); + } } if (asUnary != j) { j = j.withPrefix(asUnary.getPrefix()); @@ -154,6 +160,20 @@ public J visitUnary(J.Unary unary, ExecutionContext ctx) { return j; } + /** + * A ternary binds looser than every operator it can be an operand of, so replacing a negation + * with a ternary in such a position needs parentheses to preserve the original grouping, + * e.g. {@code !(a ? b : c) && d} must not become {@code a ? !b : !c && d}. + */ + private boolean parentBindsTighterThanTernary(J.Unary unary) { + Object parent = getCursor().getParentTreeCursor().getValue(); + if (parent instanceof J.Binary || parent instanceof J.Unary || + parent instanceof J.InstanceOf || parent instanceof J.TypeCast) { + return true; + } + return parent instanceof J.Ternary && ((J.Ternary) parent).getCondition() == unary; + } + @Override public J visitTernary(J.Ternary ternary, ExecutionContext executionContext) { J j = super.visitTernary(ternary, executionContext); @@ -414,9 +434,9 @@ private static J.Unary not(Expression sideRetained) { !(sideRetained instanceof J.Parentheses) && !(sideRetained instanceof J.Unary)) { sideRetained = new J.Parentheses<>(Tree.randomId(), - Space.EMPTY, + sideRetained.getPrefix(), Markers.EMPTY, - JRightPadded.build(sideRetained)); + JRightPadded.build(sideRetained.withPrefix(Space.EMPTY))); } return new J.Unary(Tree.randomId(), sideRetained.getPrefix(), From 241a5d633450604b5c9b2e3f828aaeec5c2ee26f Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 11 Aug 2026 21:04:47 +0200 Subject: [PATCH 3/3] Reuse ParenthesizeVisitor.maybeParenthesize for the negated ternary Replaces the local parent-precedence check, as suggested in review. ParenthesizeVisitor.visitTernary was missing two positions where Java requires a ternary to be parenthesized: as the condition of another ternary, since `a ? b : c ? d : e` groups as `a ? b : (c ? d : e)`, and as the operand of a cast, since `(Object) a ? b : c` casts the condition. --- .../java/ParenthesizeVisitorTest.java | 26 +++++++++++++++++++ .../openrewrite/java/ParenthesizeVisitor.java | 7 ++++- .../SimplifyBooleanExpressionVisitor.java | 22 +++------------- 3 files changed, 35 insertions(+), 20 deletions(-) diff --git a/rewrite-java-test/src/test/java/org/openrewrite/java/ParenthesizeVisitorTest.java b/rewrite-java-test/src/test/java/org/openrewrite/java/ParenthesizeVisitorTest.java index 66962628f44..a0eb67a2f79 100644 --- a/rewrite-java-test/src/test/java/org/openrewrite/java/ParenthesizeVisitorTest.java +++ b/rewrite-java-test/src/test/java/org/openrewrite/java/ParenthesizeVisitorTest.java @@ -174,6 +174,32 @@ void method() { ); } + @Test + void ternaryAsConditionOrCastOperand() { + rewriteRun( + java( + """ + class Test { + void method(boolean x, boolean y, boolean z) { + boolean a = (x ? y : z) ? y : z; + Object b = (Object) (x ? y : z); + boolean c = (x ? y : z); + } + } + """, + """ + class Test { + void method(boolean x, boolean y, boolean z) { + boolean a = (x ? y : z) ? y : z; + Object b = (Object) (x ? y : z); + boolean c = x ? y : z; + } + } + """ + ) + ); + } + @Test void instanceofExpressions() { rewriteRun( diff --git a/rewrite-java/src/main/java/org/openrewrite/java/ParenthesizeVisitor.java b/rewrite-java/src/main/java/org/openrewrite/java/ParenthesizeVisitor.java index 8bc5c40517c..a547b78b7f9 100644 --- a/rewrite-java/src/main/java/org/openrewrite/java/ParenthesizeVisitor.java +++ b/rewrite-java/src/main/java/org/openrewrite/java/ParenthesizeVisitor.java @@ -269,7 +269,12 @@ public J visitTernary(J.Ternary ternary, P p) { if (needsParentheses(t, parent.getValue())) { return parenthesize(t); } else if (parent.getValue() instanceof J.Binary || - parent.getValue() instanceof J.InstanceOf) { + parent.getValue() instanceof J.InstanceOf || + parent.getValue() instanceof J.TypeCast) { + return parenthesize(t); + } else if (parent.getValue() instanceof J.Ternary && + t.isScope(((J.Ternary) parent.getValue()).getCondition())) { + // `a ? b : c ? d : e` groups as `a ? b : (c ? d : e)`, so a ternary condition needs parentheses return parenthesize(t); } 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 8b1fcbaec82..44bbaa49c37 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 @@ -21,6 +21,7 @@ import org.openrewrite.Tree; import org.openrewrite.java.JavaVisitor; import org.openrewrite.java.MethodMatcher; +import org.openrewrite.java.ParenthesizeVisitor; import org.openrewrite.java.search.SemanticallyEqual; import org.openrewrite.java.tree.*; import org.openrewrite.marker.Markers; @@ -146,11 +147,8 @@ public J visitUnary(J.Unary unary, ExecutionContext ctx) { if (asUnary.getOperator() == J.Unary.Type.Not) { j = unpackExpression(asUnary.getExpression(), asUnary); - if (j instanceof J.Ternary && parentBindsTighterThanTernary(unary)) { - j = new J.Parentheses<>(Tree.randomId(), - Space.EMPTY, - Markers.EMPTY, - JRightPadded.build(((Expression) j).withPrefix(Space.EMPTY))); + if (j instanceof J.Ternary) { + j = ParenthesizeVisitor.maybeParenthesize((Expression) j, getCursor()); } } if (asUnary != j) { @@ -160,20 +158,6 @@ public J visitUnary(J.Unary unary, ExecutionContext ctx) { return j; } - /** - * A ternary binds looser than every operator it can be an operand of, so replacing a negation - * with a ternary in such a position needs parentheses to preserve the original grouping, - * e.g. {@code !(a ? b : c) && d} must not become {@code a ? !b : !c && d}. - */ - private boolean parentBindsTighterThanTernary(J.Unary unary) { - Object parent = getCursor().getParentTreeCursor().getValue(); - if (parent instanceof J.Binary || parent instanceof J.Unary || - parent instanceof J.InstanceOf || parent instanceof J.TypeCast) { - return true; - } - return parent instanceof J.Ternary && ((J.Ternary) parent).getCondition() == unary; - } - @Override public J visitTernary(J.Ternary ternary, ExecutionContext executionContext) { J j = super.visitTernary(ternary, executionContext);