Skip to content

fix(rewrite): freeze walrus and starred operands too - #14814

Draft
RonnyPfannschmidt wants to merge 11 commits into
pytest-dev:mainfrom
RonnyPfannschmidt:ronny/fix-walrus-raw-operands
Draft

fix(rewrite): freeze walrus and starred operands too#14814
RonnyPfannschmidt wants to merge 11 commits into
pytest-dev:mainfrom
RonnyPfannschmidt:ronny/fix-walrus-raw-operands

Conversation

@RonnyPfannschmidt

@RonnyPfannschmidt RonnyPfannschmidt commented Jul 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Stacked on #14447 → #14921 → #14813. Its diff includes theirs.

visit_operand() (added in #14447) 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)      # passes (1, 2) to plain Python, (2, 2) rewritten
assert collect(*items, identity(items := [9])) == (1, [9]) # passes (1, [9]) plain, (9, [9]) rewritten

A walrus operator left in place assigns only 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 reach it.

Lands the order-starred-argument case and the last order-call-argument case of the coverage matrix in #14813, as passing tests (+2).

The starred case was found by the matrix, not by hand — it is what the order-* axis is for.

@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-walrus-raw-operands branch from 16b94a1 to cc6afa4 Compare July 31, 2026 19:04
This was referenced Jul 31, 2026
Draft
Draft
Draft
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-walrus-raw-operands branch from cc6afa4 to d271ad5 Compare July 31, 2026 20:18
This was referenced Jul 31, 2026
Draft
Draft
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-walrus-raw-operands branch from d271ad5 to 5af71a7 Compare August 10, 2026 07:12
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-walrus-raw-operands branch from 5af71a7 to fd41d89 Compare August 20, 2026 04:35
This was referenced Aug 20, 2026
Draft
Merged
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-walrus-raw-operands branch 3 times, most recently from ac7b2a4 to 837173f Compare August 26, 2026 09:33
RonnyPfannschmidt and others added 11 commits August 26, 2026 12:03
@RonnyPfannschmidt Cursor AI @claude
fix(rewrite): prevent walrus operator double evaluation in assertions
792fa39 
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>
@RonnyPfannschmidt Cursor AI @claude
Add changelog fragment for pytest-dev#14445
54bd8e9 
Co-authored-by: Cursor AI <ai@cursor.sh>
Co-authored-by: Anthropic Claude Sonnet 4 <claude@anthropic.com>
@RonnyPfannschmidt Cursor AI @claude
test(rewrite): add xfail tests for remaining walrus edge cases
32829dd 
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>
@RonnyPfannschmidt Cursor AI @claude
fix(rewrite): avoid double evaluation of walrus in BoolOp condition
960a58c 
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>
@RonnyPfannschmidt Cursor AI @claude
fix(rewrite): assign walrus comparators to temps in chained comparisons
298ca95 
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>
@RonnyPfannschmidt Cursor AI @claude
fix(rewrite): show correct walrus values in BoolOp explanations
c90c70f 
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>
@RonnyPfannschmidt Cursor AI @claude
refactor(rewrite): minimal snapshots in BoolOp for walrus conflicts
9f9e738 
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>
@RonnyPfannschmidt @claude
fix(rewrite): freeze operands a later walrus would clobber
72f4a7d 
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>
@RonnyPfannschmidt @claude
test(rewrite): retire the walrus tests the matrix subsumes
0253180 
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>
@RonnyPfannschmidt @claude
fix(rewrite): freeze walrus and starred operands too
5663cf5 
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>
@RonnyPfannschmidt @claude
refactor(rewrite): drop the walrus snapshot visit_Compare no longer n…
338cd67 
…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>
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-walrus-raw-operands branch from 837173f to 338cd67 Compare August 26, 2026 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@RonnyPfannschmidt