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-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..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 @@ -592,6 +592,108 @@ 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 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/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 22092471649..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,6 +147,9 @@ 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) { + j = ParenthesizeVisitor.maybeParenthesize((Expression) j, getCursor()); + } } if (asUnary != j) { j = j.withPrefix(asUnary.getPrefix()); @@ -232,15 +236,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)) { @@ -416,9 +418,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(),