fix(rewrite): short-circuit chained comparisons - #14822
fix(rewrite): short-circuit chained comparisons#14822RonnyPfannschmidt wants to merge 17 commits into
Conversation
02c9b51 to
8cb7a4a
Compare
8cb7a4a to
cdc4b90
Compare
cdc4b90 to
ac4705a
Compare
|
Found a regression in this branch while checking my own duplicate of it against yours. A boolop in a skipped comparator raises def test_boolop_or():
a = b = 0
assert 1 < 0 < (a or b)
Plain calls are fine — it is boolops specifically. Why: @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, unconditionallyFix — nest 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_innerTwo things fall out of it, in case they save you the same detour:
Verified on this branch: 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. |
The rewriter is read through its failure messages; what it actually
generates is invisible unless one hand-writes an ast.unparse harness.
Reviewing a change to it means asking "what does the emitted code look
like now, and how does that differ from what it was".
Add a script that answers exactly that: it dumps a snippet's rewritten
form -- as source, or as an AST -- for the source as written, for this
checkout, or for any released pytest version, and diffs two of them.
Released versions are fetched on demand via ``uv run --with``, so no
version under comparison has to be installed.
By default it diffs the snippet as written against this checkout, which
is the "show me what rewriting does here" case:
python scripts/diff-assert-rewrite.py -c 'assert (x := f()) and (x := False)'
It exits 1 when the sides differ, so it can also be used as a check.
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
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.
…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.
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.
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.
…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.
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.
ac4705a to
dcbec2d
Compare
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,cis never evaluated whena < bis false.visit_Comparewalked the comparators in a loop and only combined the results withandafterwards, by which time everything had already run:Each link past the first now goes inside an
ifon the link before it — the same shapevisit_BoolOphas used since #57 was fixed forand/or. Comparison chains never got the same treatment.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
Noneahead of the chain rather than left unbound.Noneis falsey, which is also what_call_reprcomparewants: it stops at the first falsey result, and that is the link that actually failed. Messages are unchanged —assert 1 < 2 < 3 < 4 < 0still reportsassert 4 < 0.Lands the
order-chained-compare-lazycases 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-literalandintrospect-callable-variable— are recorded in #14916 on top.