feat(rewrite): introspect the condition of a ternary - #14816
Draft
RonnyPfannschmidt wants to merge 13 commits into
Draft
feat(rewrite): introspect the condition of a ternary#14816RonnyPfannschmidt wants to merge 13 commits into
RonnyPfannschmidt wants to merge 13 commits into
Conversation
RonnyPfannschmidt
force-pushed
the
ronny/rewrite-visit-ifexp
branch
from
July 31, 2026 19:05
f3d9fd1 to
09585c5
Compare
psf-chronographer
Bot
added
the
bot:chronographer:provided
This was referenced Jul 31, 2026
The rewriter is tested through its failure messages, one regression test per issue. That makes it hard to say whether a change improved anything: there is no place that records what the rewriter cannot do yet. Add a matrix over expression types with four axes -- introspection depth, semantic equivalence, single evaluation, and evaluation order -- and record every known gap as a strict xfail tagged with a group name. A change that closes one names the group and flips its markers; strictness means it cannot forget. The evaluation-order axis is new. Comparing pass/fail is not enough: an operand read after a later walrus operator rebound its name runs exactly once and can still fail the assertion, having compared the wrong value. assert_evaluation_order() compares what check() returns, because the fragile operand is a bare name -- wrapping it in a call to observe it would hoist it and hide the bug. Behaviour is unchanged; this only describes it.
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>
Add scripts to inspect and compare assertion rewriting across pytest versions, useful for tracking changes in the rewriter output. - dump-assert-rewrite.py: dumps the rewritten form of a Python file using any pytest version (via uv ephemeral env) or the local worktree. Supports source, ast, and compact output formats. - diff-assert-rewrite.py: compares rewrite output between two versions (or worktree) with colored unified diff, runs both sides in parallel. - example_asserts.py: example file covering common assertion patterns including bluetech's walrus-in-BoolOp case from pytest-dev#14445. Co-authored-by: Cursor AI <ai@cursor.sh> Co-authored-by: Anthropic Claude Opus 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.
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.
RonnyPfannschmidt
force-pushed
the
ronny/rewrite-visit-ifexp
branch
from
July 31, 2026 20:18
09585c5 to
8bc3e80
Compare
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #14815 → #14814 → #14447 → #14813. Its diff includes theirs.
A conditional expression showed only its result, so a failure gave no hint which way it went.
The branches keep their original nodes: only the selected one may run, so neither can be hoisted into a statement. That is also why this visitor needs no freeze, unlike the subscript container in #14815 — leaving the branches in place leaves them evaluated after the condition, which is Python's order.
TestEvaluationOrder::test_ifexp_branches_in_orderin #14813 covers that claim rather than leaving it as a comment.Closes the
introspect-ifexpgroup: 11 xfails to 8.Supersedes part of #14448.