Skip to content

fix(rewrite): short-circuit chained comparisons - #14822

Draft
RonnyPfannschmidt wants to merge 18 commits into
pytest-dev:mainfrom
RonnyPfannschmidt:ronny/fix-chained-compare-short-circuit
Draft

fix(rewrite): short-circuit chained comparisons#14822
RonnyPfannschmidt wants to merge 18 commits into
pytest-dev:mainfrom
RonnyPfannschmidt:ronny/fix-chained-compare-short-circuit

Conversation

@RonnyPfannschmidt

@RonnyPfannschmidt RonnyPfannschmidt commented Jul 31, 2026

Copy link
Copy Markdown
Member

Fixes #14819. Stacked on #14821#14817#14816#14815#14814#14447#14921#14813; its diff includes theirs.

Python evaluates a comparison chain lazily — in a < b < c, c is never evaluated when a < b is false. visit_Compare walked the comparators in a loop and only combined the results with and afterwards, by which time everything had already run:

assert 1 < 0 < 1 / 0     # ZeroDivisionError, not AssertionError
assert 1 < 0 < boom()    # boom() runs

Each link past the first now goes inside an if on the link before it — the same shape visit_BoolOp has used since #57 was fixed for and/or. Comparison chains never got the same treatment.

@py_assert1 = a < b
@py_assert2 = None
if @py_assert1:
    @py_assert2 = b < c
if not (@py_assert1 and @py_assert2):
    ...

The failure path builds a tuple of every link's result and every operand, so the temporaries belonging to links that never ran are set to None ahead of the chain rather than left unbound. None is falsey, which is also what _call_reprcompare wants: it stops at the first falsey result, and that is the link that actually failed. Messages are unchanged — assert 1 < 2 < 3 < 4 < 0 still reports assert 4 < 0.

Lands the order-chained-compare-lazy cases of the coverage matrix in #14813, as passing tests (+2). That completes the series and takes the matrix to 100 tests, none of them xfail. The two gaps nothing here fixes — introspect-container-literal and introspect-callable-variable — are recorded in #14916 on top.

@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-chained-compare-short-circuit branch from 02c9b51 to 8cb7a4a Compare July 31, 2026 20:19
@psf-chronographer psf-chronographer Bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Jul 31, 2026
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-chained-compare-short-circuit branch from 8cb7a4a to cdc4b90 Compare August 10, 2026 07:29
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-chained-compare-short-circuit branch from cdc4b90 to ac4705a Compare August 20, 2026 04:35
@SirHegel

Copy link
Copy Markdown

Found a regression in this branch while checking my own duplicate of it against yours. A boolop in a skipped comparator raises AttributeError instead of AssertionError:

def test_boolop_or():
    a = b = 0
    assert 1 < 0 < (a or b)
E       AttributeError: 'NoneType' object has no attribute 'append'
--assert=plain origin/main this branch
1 < 0 < (a or b) AssertionError assert 1 < 0 AttributeError
1 < 0 < (a and b) AssertionError assert 1 < 0 AttributeError
1 < 0 < (a or b) < 5 AssertionError assert 1 < 0 AttributeError
1 < 0 < f() AssertionError assert 1 < 0 assert 1 < 0

Plain calls are fine — it is boolops specifically.

Why: visit_BoolOp builds its explanation by creating a list in the main statement body and appending to it from expl_stmts. Nesting only self.statements puts the list creation inside the if, while the appends stay outside and run unconditionally in the failure branch, against a name the skipped operand never bound:

@py_assert3 = @py_assert6 = @py_assert7 = None
@py_assert2 = @py_assert0 < @py_assert4
if @py_assert2:
    @py_assert7 = []          # <- list created here
    ...
if not (@py_assert2 and @py_assert3):
    @py_format9 = '%(py8)s' % {...}
    @py_assert7.append(@py_format9)   # <- but appended here, unconditionally

Fix — nest expl_stmts on the same condition, the way visit_BoolOp nests both:

fail_inner: list[ast.stmt] = []
fail_if = ast.If(load_names[i - 1], fail_inner, [])
fail_ifs.append((self.expl_stmts, fail_if))
self.expl_stmts.append(fail_if)
self.expl_stmts = fail_inner

Two things fall out of it, in case they save you the same detour:

  • Most operands contribute nothing to the explanation, and an ast.If with an empty body is invalid — so the empty ones need dropping afterwards, innermost first, since pruning a child can empty its parent.
  • Once expl_stmts is nested, @py_format names created inside can be read from the outer level. Collecting the names to pre-bind by walking the conditional blocks for Store targets picks those up; tracking self.variables by index does not, because pop_format_context only records them when the assertion_pass hook is on.

Verified on this branch: 4359 passed, 97 skipped, 14 xfailed with it applied, and the case above returns assert 1 < 0.

I opened #14918 before seeing your stack — closing it, this is yours. Happy to push the above as a commit here if that is easier than re-typing it.

@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-chained-compare-short-circuit branch 3 times, most recently from 6297dbd to 86a550b Compare August 26, 2026 09:33
RonnyPfannschmidt added a commit to RonnyPfannschmidt/pytest that referenced this pull request Aug 26, 2026
Short-circuiting the chain nested the statements that evaluate a link
past the first, but not the statements that explain it, so the failure
branch ran a skipped link's explanation against temporaries the link
never assigned:

    assert 1 < 0 < (a or b)

raised ``AttributeError: 'NoneType' object has no attribute 'append'``
instead of failing.  visit_BoolOp builds its explanation by creating a
list in the main body and appending to it from expl_stmts; nesting only
the body left the list None while the appends ran unconditionally.

SirHegel found this against the branch and named the fix -- nest
expl_stmts on the same condition, the way visit_BoolOp nests both -- in
pytest-dev#14822 (comment),
having reduced it from his own pytest-dev#14918.  What follows is his idea; two
details are worth recording.

Most links explain themselves in the format context alone and contribute
no statements, and an ``if`` with an empty body is not valid syntax, so
the guards are attached innermost first and the empty ones dropped --
attaching a child fills its parent, so a parent is only known to be empty
once its child has been placed.

The names to pre-bind now include the @py_format ones created inside
those guarded blocks, which the outer format context reads.  Collecting
them by walking the blocks is what makes them reachable at all, but the
walk must not take everything it finds: a walrus target inside a skipped
link belongs to the user, and Python leaves it unbound.  Binding it to
None to keep the explanation readable would be visible after the
assertion, so _rewriter_temporaries() takes only names the rewriter
itself makes.

That last point is a second failure mode, which the report did not
cover: the explanation of a walrus reads its target to decide how to show
it, and a skipped link never bound it.

    assert 1 < 0 < (w := 1)          # UnboundLocalError: 'w'
    assert 1 < 0 < identity(w := 1)  # likewise

Nesting does not reach it -- the read sits in the compare's own format
dict, which is built eagerly and belongs to no link -- and ``'w' in
locals()`` does not guard it, because the fallback hands the value to
_should_repr_global_name().  visit_NamedExpr now asks whether the target
is a global before reading it, and shows the bare name when it is
neither.  An undefined name inside a skipped operand failed the same way
with NameError, and is fixed by the nesting.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
RonnyPfannschmidt and others added 18 commits August 26, 2026 12:03
Fixes pytest-dev#14445 - assertion rewriting evaluated NamedExpr (:=) expressions
multiple times, causing side effects to fire repeatedly.

The root cause was the `variables_overwrite` mechanism which stored and
re-evaluated NamedExpr AST nodes in subsequent assertions, in
`_call_reprcompare`'s results tuple, and in explanation formatting.

The fix:
- visit_NamedExpr: reference the target variable in explanations instead
  of re-evaluating the full expression
- visit_Compare: assign left-side NamedExpr to a temp before right-side
  hoisting; freeze left_res when a comparator walrus targets the same
  name; replace NamedExpr entries in `results` with target variables
- visit_BoolOp: capture short-circuit condition in a stable temp for the
  explanation path; remove walrus target rename logic
- visit_Call: remove variables_overwrite substitution (walrus now properly
  assigns to user variables in its natural evaluation position)
- Remove variables_overwrite, scope tracking, Sentinel class

Co-authored-by: Cursor AI <ai@cursor.sh>
Co-authored-by: Anthropic Claude Sonnet 4 <claude@anthropic.com>
Co-authored-by: Cursor AI <ai@cursor.sh>
Co-authored-by: Anthropic Claude Sonnet 4 <claude@anthropic.com>
Add tests for two remaining walrus double-evaluation scenarios:
- Bare NamedExpr as BoolOp operand evaluated twice via condition check
- Same walrus target in chained comparison evaluated multiple times

Co-authored-by: Cursor AI <ai@cursor.sh>
Co-authored-by: Anthropic Claude Sonnet 4 <claude@anthropic.com>
Use the already-assigned res_var to build the short-circuit condition
instead of the raw visitor result, preventing bare NamedExpr operands
from being evaluated a second time when checking truthiness.

Co-authored-by: Cursor AI <ai@cursor.sh>
Co-authored-by: Anthropic Claude Sonnet 4 <claude@anthropic.com>
In a chained comparison like `(x := f()) < (x := g()) < (x := h())`,
each NamedExpr comparator is now assigned to a temp variable so it
evaluates exactly once. Previously the raw NamedExpr node would be
reused as left_res in the next iteration, causing double evaluation.

Co-authored-by: Cursor AI <ai@cursor.sh>
Co-authored-by: Anthropic Claude Sonnet 4 <claude@anthropic.com>
When multiple walrus operators target the same variable in a BoolOp
(e.g., `assert (x := side_effect()) and (x := False)`), the assertion
explanation previously showed the final value of `x` for all operands
because the format context evaluated lazily after all operands ran.

Fix by tracking Name/NamedExpr operand values in stable @py_assert
variables (via self.assign) immediately after evaluation, then pointing
the explanation format context at the tracked copy. This uses the same
value-tracking mechanism already used by visit_Call, visit_Attribute, etc.

Fixes the case reported by @bluetech in PR review.

Co-authored-by: Cursor AI <ai@cursor.sh>
Co-authored-by: Anthropic Claude Sonnet 4 <claude@anthropic.com>
Replace the blanket snapshot-all-operands approach with a targeted one:
pre-scan the BoolOp to find walrus targets, then only snapshot operands
whose value a later walrus would corrupt.

Snapshot rules:
- NamedExpr (non-last): always, to avoid re-evaluating side effects
- Name with later walrus conflict: to freeze the pre-overwrite value
- Everything else: use res directly (stable @py_assert or plain name)

Non-walrus BoolOps now generate identical code to 8.3.5 (no snapshots).

Co-authored-by: Cursor AI <ai@cursor.sh>
Co-authored-by: Anthropic Claude Opus 4 <claude@anthropic.com>
The rewriter hoists each operand into its own statement, but a plain
name is left as a bare load evaluated when the enclosing expression is
assembled -- after the statements of the operands that follow it.  A
walrus operator in a later operand rebinds the name in between, so both
the value used and the value reported were the post-walrus one, while
Python evaluates the earlier operand first:

    assert value != identity(value := value.lower())

visit_BoolOp already guarded against this; extract its pre-scan as
_walrus_targets() and add visit_operand() to apply the same freeze in
visit_Compare, visit_Call and visit_BinOp.  visit_Compare previously
matched only a comparator that *was* a NamedExpr, missing walrus
operators nested inside it; visit_Call did not guard at all, so an
earlier argument saw a later argument's assignment.

These cases predate the walrus rework -- they fail on main too.

Closes the single-eval-walrus, order-compare-left, order-call-argument
and order-binop-left groups in the coverage matrix.  order-call-argument
keeps one entry: a bare walrus argument is still substituted into a
later one, which visit_operand does not yet see because the operand is a
NamedExpr rather than a Name.

Reported-by: Denis Scapin
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TestAssertionRewriteWalrusOperator predates the coverage matrix and asks
its questions through runpytest(): twelve tests that mostly check ret == 0.
Nine of them the matrix already answers in-process and more precisely --
what the failure message says, what the operands saw, how often each ran.

Two are worth more than a deletion, so they move rather than vanish:

  assert not (a and ((a := False) is False))

reads back as an introspection case, and the composite chain

  assert a and True and ((a := False) is False) and (a is False) and ...

as an evaluation-order one.  That second is why the deletion is not just
tidying: it passes on main, and it passes there because of the bug.  Main
rewrites the walrus target to an internal temp and substitutes the stored
NamedExpr into every later read of the name -- including the next
statement, so the `assert a is None` that is supposed to verify the outcome
re-runs the walrus and creates it.  Asked through returned values instead
of ret == 0, main fails the case.

TestIssue14445 loses the four tests that restate matrix single-evaluation
entries this PR already adds, and keeps the two reproducers from the issue.

One test survives in place: nothing the rewriter stores may outlive a
statement, which needs two tests in one module to observe and so cannot be
said in-process.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
visit_operand() only froze a bare name, so two other unhoisted operands
kept being evaluated after everything that follows them:

    assert collect((x := 1), identity(x := 2)) == (1, 2)
    assert collect(*items, identity(items := [9])) == (1, [9])

A walrus operator left in place assigns once the enclosing expression is
assembled, which is after the later arguments have run -- so the earlier
argument saw the later assignment.  A starred argument hid its value
inside an ast.Starred, where the existing Name check could not see it.

Closes the order-starred-argument group and the remaining
order-call-argument entry in the coverage matrix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eeds

visit_operand freezes a walrus operand whenever anything follows it, and
a comparison always has at least one comparator -- so by the time
visit_Compare looks at its left operand, a NamedExpr has already been
copied into a temporary.  The special case that did it here can never
run.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rewriter only ever visits expressions inside an assert condition, so
an attribute always arrives in Load context and the fallback never runs.
Removing it keeps the next visitor from copying a guard that cannot fire.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A subscript was opaque: the message showed the value it produced with no
indication of which container or key it came from.  Decompose it the way
attribute access already is.

The container goes through visit_operand() because taking the expression
away from generic_visit() takes away the hoisting that kept it ordered --
without that, `assert box[identity(box := other)] == 1` would start
reading the post-walrus container.  The order-axis guard in the coverage
matrix fails if this is dropped.

Slices keep the generic treatment; decomposing start/stop/step is rarely
what a failure message needs.

Closes the introspect-subscript group in the coverage matrix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A conditional expression showed only its result, so a failure gave no
hint which way it went.  Introspect the condition and report it as
"(... if <cond> else ...)".

The branches keep their original nodes: only the selected one may run,
so neither can be hoisted into a statement.  That leaves them evaluated
after the condition, which is Python's order, so unlike the subscript
container they need no freeze -- the order-axis guard covers it.

Closes the introspect-ifexp group in the coverage matrix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
obj.method() reported the bound method as an intermediate of its own:

    where 42 = compute()
      where compute = Obj().compute

which spends a line on something nobody asked about.  Build the
explanation from the receiver and the attribute name instead:

    where 42 = Obj().compute()

The bound method keeps its own temporary even though it no longer has
its own explanation, because Python looks it up before evaluating the
arguments -- inlining the attribute into the rewritten call would move
the lookup after them, and with it the read of the receiver.  Both
order-axis guards in the coverage matrix fail if that temporary goes.

Closes the introspect-method-call-flat group in the coverage matrix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ev#14820)

visit_operand() froze a name only when a walrus operator in a later
operand targeted it.  A call can rebind just as well, through global or
nonlocal, and then the name -- still unhoisted, still read when the
enclosing expression is assembled -- sees the new binding:

    count = 0
    def bump():
        global count
        count = 99
        return 0

    assert count == bump()   # Python compares 0 == 0 and passes

There is no way to tell from the assert which names a call might rebind,
so _walrus_targets() becomes _can_rebind(): a name is frozen whenever
anything that follows it can execute code at all.

That sounds expensive and is not.  An operand that was already hoisted
needs no freeze, so `assert len(items) == expected` rewrites unchanged;
only the bare-name-then-call shape gains one assignment.  visit_BoolOp
used the same pre-scan and is generalized with it, which leaves one rule
in one place instead of two spellings of half of it.

Closes the order-name-rebound-by-call group in the coverage matrix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Python evaluates a comparison chain lazily -- in `a < b < c`, c is never
evaluated when a < b is false.  visit_Compare walked the comparators in
a loop and only combined the results with `and` afterwards, by which
time everything had already run:

    assert 1 < 0 < 1 / 0    # ZeroDivisionError, not AssertionError

Each link past the first now goes inside an `if` on the link before it,
the same shape visit_BoolOp has used since #57 was fixed for and/or.

The failure path builds a tuple of every link's result and every
operand, so the temporaries belonging to links that never ran are set to
None ahead of the chain rather than left unbound.  None is falsey, which
is also what _call_reprcompare wants: it stops at the first falsey
result, and that is the link that actually failed.

Closes the order-chained-compare-lazy group in the coverage matrix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Short-circuiting the chain nested the statements that evaluate a link
past the first, but not the statements that explain it, so the failure
branch ran a skipped link's explanation against temporaries the link
never assigned:

    assert 1 < 0 < (a or b)

raised ``AttributeError: 'NoneType' object has no attribute 'append'``
instead of failing.  visit_BoolOp builds its explanation by creating a
list in the main body and appending to it from expl_stmts; nesting only
the body left the list None while the appends ran unconditionally.

SirHegel found this against the branch and named the fix -- nest
expl_stmts on the same condition, the way visit_BoolOp nests both -- in
pytest-dev#14822 (comment),
having reduced it from his own pytest-dev#14918.  What follows is his idea; two
details are worth recording.

Most links explain themselves in the format context alone and contribute
no statements, and an ``if`` with an empty body is not valid syntax, so
the guards are attached innermost first and the empty ones dropped --
attaching a child fills its parent, so a parent is only known to be empty
once its child has been placed.

The names to pre-bind now include the @py_format ones created inside
those guarded blocks, which the outer format context reads.  Collecting
them by walking the blocks is what makes them reachable at all, but the
walk must not take everything it finds: a walrus target inside a skipped
link belongs to the user, and Python leaves it unbound.  Binding it to
None to keep the explanation readable would be visible after the
assertion, so _rewriter_temporaries() takes only names the rewriter
itself makes.

That last point is a second failure mode, which the report did not
cover: the explanation of a walrus reads its target to decide how to show
it, and a skipped link never bound it.

    assert 1 < 0 < (w := 1)          # UnboundLocalError: 'w'
    assert 1 < 0 < identity(w := 1)  # likewise

Nesting does not reach it -- the read sits in the compare's own format
dict, which is built eagerly and belongs to no link -- and ``'w' in
locals()`` does not guard it, because the fallback hands the value to
_should_repr_global_name().  visit_NamedExpr now asks whether the target
is a global before reading it, and shows the bare name when it is
neither.  An undefined name inside a skipped operand failed the same way
with NameError, and is fixed by the nesting.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-chained-compare-short-circuit branch from bd69dff to 02d56fa Compare August 26, 2026 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:chronographer:provided (automation) changelog entry is part of PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Assertion rewriting does not short-circuit chained comparisons

2 participants