Skip to content

SimplifyBooleanExpressionVisitor: correctly negate a nested ternary - #8456

Merged
timtebeek merged 3 commits into
openrewrite:mainfrom
martinfrancois:fix/simplify-boolean-maybenegate-nested-ternary
Aug 11, 2026
Merged

SimplifyBooleanExpressionVisitor: correctly negate a nested ternary#8456
timtebeek merged 3 commits into
openrewrite:mainfrom
martinfrancois:fix/simplify-boolean-maybenegate-nested-ternary

Conversation

@martinfrancois

@martinfrancois martinfrancois commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

What's changed?

maybeNegate in SimplifyBooleanExpressionVisitor negated a ternary by inverting its condition and
swapping its two branches. Those two edits cancel each other out: c ? t : f and !c ? f : t are
the same value, not opposites. So where a real negation was needed, the visitor produced an
expression equal to the one it was asked to negate, and the negation disappeared from the output.

The method now negates the two branches instead and leaves the condition alone: the negation of
c ? t : f is c ? !t : !f. That is the same rule unpackExpression in this class already applies
when a ternary is negated directly, as in the existing ternayNegation test.

What's your motivation?

The broken path is reached when a ternary is nested inside another negated ternary. Input:

boolean m1(boolean a, boolean b, boolean x, boolean y, boolean z) {
    return !(a ? b ? x : y : z);
}

Output from current main, first cycle:

    return a ? !b ? y : x : !z;

The nested part !b ? y : x equals b ? x : y, so when a is true this returns x where the
input returns !x. The compiled result is the opposite of the input. The second cycle then rewrites
!b ? y : x back to b ? x : y, which both shows the two are the same expression and makes the
recipe fail the single-cycle convergence check in RewriteTest.

A ternary nested in the false position loses its negation the same way: !(a ? x : b ? y : z)
becomes a ? !x : b ? y : z after both cycles, so the b ? y : z part is returned unnegated.

The visitor is shared: SimplifyBooleanExpression, InvertCondition and RemoveObjectsIsNull
inherit this behavior wherever they negate an expression with a nested ternary in it.

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

No existing test expectation changed. SimplifyBooleanExpressionVisitorTest has 75 tests on main;
this change adds 1, nestedTernaryNegation, with four methods: a nested ternary in the true
position, one in the false position, one whose nested condition is itself negated
(!(a ? !b ? x : y : z) becomes a ? b ? !y : !x : !z, in a single cycle), and an unchanged
control without a negation. Without the code change, that test fails; the other 75 pass before and
after. I also ran the three consumers of the visitor that have their own tests: rewrite-groovy's
SimplifyBooleanExpressionVisitorTest (61 tests), InvertConditionTest (1) and
RemoveObjectsIsNullTest (7), all green with this change.

Two limits, both present on main today and out of scope here:

  • A nested ternary wrapped in its own parentheses, !(a ? (b ? x : y) : z), reaches the correct
    output but needs two cycles, because maybeNegate has no parentheses case and first wraps the
    inner ternary in a !(...). Main does the same.
  • Negating a comparison maps < to >=, which is not equivalent when an operand is NaN. That
    mapping is existing code and main already applies it to a comparison in an unnested ternary; this
    change lets it reach comparisons one nesting level deeper.

Have you considered any alternatives or workarounds?

Keeping the condition inversion and negating the branches as well would also be correct, but it
edits three nodes where one rule suffices, and it diverges from how unpackExpression negates a
directly negated ternary. Reusing the same rule keeps the two paths consistent.

Any additional context

Our open PR #8447 touches the same file for an unrelated concern (operands with side effects).
The two changes are independent and edit different methods, but whichever merges second may need a
trivial rebase of the shared test file.

This change was prepared with AI assistance (Claude Code). I reviewed the code, the test 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 used the IntelliJ IDEA auto-formatter on affected files

I ran the formatter calibrated to each file's existing style. It also wanted to re-indent lines this
change does not touch, so I left those alone and kept the diff limited to this change.

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.
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)`.
@knutwannheden

Copy link
Copy Markdown
Contributor

@timtebeek can we leverage ParenthesizeVisitor.maybeParenthesize(...)?

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.

@timtebeek timtebeek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! Approved from my end; but since I've added changes as well @knutwannheden might want to approve too.

@github-project-automation github-project-automation Bot moved this from In Progress to Ready to Review in OpenRewrite Aug 11, 2026
@timtebeek
timtebeek merged commit e9e6b9e into openrewrite:main Aug 11, 2026
1 check passed
@github-project-automation github-project-automation Bot moved this from Ready to Review to Done in OpenRewrite Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants