fix(rewrite): prevent walrus operator double evaluation in assertions - #14447
fix(rewrite): prevent walrus operator double evaluation in assertions#14447RonnyPfannschmidt wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes assertion rewriting so walrus (:=) expressions are not evaluated multiple times, preventing side effects from running twice and producing incorrect rewritten-assert behavior (per #14445).
Changes:
- Removes the prior
variables_overwrite/scope-tracking mechanism and adjustsNamedExprhandling to avoid re-evaluation in explanations. - Updates BoolOp/Compare rewriting to stabilize conditions/operands for explanation formatting.
- Adds new regression tests for walrus side-effect/double-evaluation cases and updates existing expected assertion output.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/_pytest/assertion/rewrite.py |
Refactors assertion-rewrite AST generation around NamedExpr, BoolOp, and Compare to avoid walrus re-evaluation. |
testing/test_assertrewrite.py |
Updates expected assertion output and adds regression tests for #14445 scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
psf-chronographer
Bot
added
the
bot:chronographer:provided
Pierre-Sassoulas
left a comment
Pierre-Sassoulas
left a comment
There was a problem hiding this comment.
LGTM, but we probably want another person to look at it
There was a problem hiding this comment.
I haven't reviewed the code yet, before that I dumped the rewritten AST before/after this PR, on the following example:
def side_effect():
return True
def test_walrus_boolop():
assert (x := side_effect())Before
Module(
body=[
Import(
names=[
alias(name='builtins', asname='@py_builtins')]),
Import(
names=[
alias(name='_pytest.assertion.rewrite', asname='@pytest_ar')]),
FunctionDef(
name='side_effect',
args=arguments(),
body=[
Return(
value=Constant(value=True))]),
FunctionDef(
name='test_walrus_boolop',
args=arguments(),
body=[
If(
test=UnaryOp(
op=Not(),
operand=NamedExpr(
target=Name(id='x', ctx=Store()),
value=Call(
func=Name(id='side_effect', ctx=Load())))),
body=[
Assign(
targets=[
Name(id='@py_format1', ctx=Store())],
value=BinOp(
left=BinOp(
left=Constant(value=''),
op=Add(),
right=Constant(value='assert %(py0)s')),
op=Mod(),
right=Dict(
keys=[
Constant(value='py0')],
values=[
IfExp(
test=BoolOp(
op=Or(),
values=[
Compare(
left=Constant(value='x'),
ops=[
In()],
comparators=[
Call(
func=Attribute(
value=Name(id='@py_builtins', ctx=Load()),
attr='locals',
ctx=Load()))]),
Call(
func=Attribute(
value=Name(id='@pytest_ar', ctx=Load()),
attr='_should_repr_global_name',
ctx=Load()),
args=[
NamedExpr(
target=Name(id='x', ctx=Store()),
value=Call(
func=Name(id='side_effect', ctx=Load())))])]),
body=Call(
func=Attribute(
value=Name(id='@pytest_ar', ctx=Load()),
attr='_saferepr',
ctx=Load()),
args=[
NamedExpr(
target=Name(id='x', ctx=Store()),
value=Call(
func=Name(id='side_effect', ctx=Load())))]),
orelse=Constant(value='x'))]))),
Raise(
exc=Call(
func=Name(id='AssertionError', ctx=Load()),
args=[
Call(
func=Attribute(
value=Name(id='@pytest_ar', ctx=Load()),
attr='_format_explanation',
ctx=Load()),
args=[
Name(id='@py_format1', ctx=Load())])]))])])])After
Module(
body=[
Import(
names=[
alias(name='builtins', asname='@py_builtins')]),
Import(
names=[
alias(name='_pytest.assertion.rewrite', asname='@pytest_ar')]),
FunctionDef(
name='side_effect',
args=arguments(),
body=[
Return(
value=Constant(value=True))]),
FunctionDef(
name='test_walrus_boolop',
args=arguments(),
body=[
If(
test=UnaryOp(
op=Not(),
operand=NamedExpr(
target=Name(id='x', ctx=Store()),
value=Call(
func=Name(id='side_effect', ctx=Load())))),
body=[
Assign(
targets=[
Name(id='@py_format1', ctx=Store())],
value=BinOp(
left=BinOp(
left=Constant(value=''),
op=Add(),
right=Constant(value='assert %(py0)s')),
op=Mod(),
right=Dict(
keys=[
Constant(value='py0')],
values=[
IfExp(
test=BoolOp(
op=Or(),
values=[
Compare(
left=Constant(value='x'),
ops=[
In()],
comparators=[
Call(
func=Attribute(
value=Name(id='@py_builtins', ctx=Load()),
attr='locals',
ctx=Load()))]),
Call(
func=Attribute(
value=Name(id='@pytest_ar', ctx=Load()),
attr='_should_repr_global_name',
ctx=Load()),
args=[
Name(id='x', ctx=Load())])]),
body=Call(
func=Attribute(
value=Name(id='@pytest_ar', ctx=Load()),
attr='_saferepr',
ctx=Load()),
args=[
Name(id='x', ctx=Load())]),
orelse=Constant(value='x'))]))),
Raise(
exc=Call(
func=Name(id='AssertionError', ctx=Load()),
args=[
Call(
func=Attribute(
value=Name(id='@pytest_ar', ctx=Load()),
attr='_format_explanation',
ctx=Load()),
args=[
Name(id='@py_format1', ctx=Load())])]))])])])The diff is:
@@ -57,20 +57,14 @@
attr='_should_repr_global_name',
ctx=Load()),
args=[
- NamedExpr(
- target=Name(id='x', ctx=Store()),
- value=Call(
- func=Name(id='side_effect', ctx=Load())))])]),
+ Name(id='x', ctx=Load())])]),
body=Call(
func=Attribute(
value=Name(id='@pytest_ar', ctx=Load()),
attr='_saferepr',
ctx=Load()),
args=[
- NamedExpr(
- target=Name(id='x', ctx=Store()),
- value=Call(
- func=Name(id='side_effect', ctx=Load())))]),
+ Name(id='x', ctx=Load())]),
orelse=Constant(value='x'))]))),
Raise(
exc=Call(This looks good for the issue, since now we no longer run side_effect twice three times.
However if I tweak in this way:
def side_effect():
return True
def test_walrus_boolop():
assert (x := side_effect()) and (x := False)the assertion is
x.py:5: in test_walrus_boolop
assert (x := side_effect()) and (x := False)
E assert (False and False)
which is incorrect (should be assert (True and False)). That said, this also happens in main.
Let me know if you want to tackle this problem in this PR as well, in which I'll wait before reviewing, or if I should open a separate issue for that and review this PR as is.
|
good find, i'll address it in here |
|
i found a interesting issue about very duplicate tracking, investigating now |
|
now the change is a litte bigger than intended |
bluetech
added
backport 9.1.x
acd9277 to
c4369d0
Compare
|
Thanks for working on this. A differential check found three remaining def identity(value):
return value
def test_compare_preserves_pre_walrus_left_value():
value = "Hello"
assert value != identity(value := value.lower())
assert value == "hello"Python evaluates the left operand before the call, so this compares def collect(*values):
return values
def test_call_preserves_earlier_positional_argument():
value = "Hello"
assert collect(value, identity(value := value.lower())) == (
"Hello",
"hello",
)
assert value == "hello"The first positional argument should already be A related false-comparison case is: def test_failed_compare_uses_pre_walrus_left_value():
value = 2
try:
assert value == identity(value := 3)
except AssertionError:
pass
else:
raise AssertionError("assertion was rewritten as 3 == 3")
assert value == 3This must compare Values evaluated before a later walrus need to be captured before that
I can provide tests adapted to These cases were identified with automated assistance, then reduced and |
c4369d0 to
9fe133f
Compare
|
Rebased and extended. Two changes worth calling out. The reported evaluation-order cases are fixed here. @scapalive — thank you, all three reproduce and all three are now covered. They are not regressions from this PR (they fail on The mechanism was already in the PR, just too shallow, and inconsistently so between two visitors this PR touches: This now sits on #14813, a test-only PR that adds a coverage matrix for the rewriter and records every known gap as a strict xfail. This PR closes four of those groups — One |
9fe133f to
6caf74a
Compare
6caf74a to
6c9f83e
Compare
6c9f83e to
fa8ff7a
Compare
Pierre-Sassoulas
left a comment
Pierre-Sassoulas
left a comment
There was a problem hiding this comment.
I didn't see anything shocking by skimming. i'll review in details later.
Maybe the helper script to compare two pytest versions' output could be in their own PR ? In pylint and mypy there is a primer that permits to see the change in output in a feature branch compared to main by running pylint/mypy on a choice selection of open source repos. Maybe it could be adapted for pytest based on those two scripts.
Also the big file with separator as comment could be burst into a directory of multiple files?
|
It would be nice to see original/rewritten AST for some simple case, to get a quick sense of the new method, before diving into the code. If the AST can also be written in source code form it would make it even easier. Maybe for the example given above: def side_effect():
return True
def test_walrus_boolop():
assert (x := side_effect()) and (x := False)Sorry I'm too lazy to do it myself... |
| assert result.ret == 0 | ||
|
|
||
|
|
||
| class TestIssue14445: |
There was a problem hiding this comment.
Are these tests redundant with the coverage ones, or are they testing something separate?
Also in #14813 (comment) you said you'll delete TestAssertionRewriteWalrusOperator here. Is it not redundant now?
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
dead644 to
5fe5987
Compare
|
To bluetech comment, I made a pytest plugin to do golden master / caracterisation tests (pytest-remaster) which is what we want to do here for easy review and update of ast changes imo. Let me know what you think. |
Summary
Fixes #14445 — assertion rewriting evaluated walrus operator (
:=) expressions multiple times, causing incorrect test results when the expression had side effects.Root cause: The
variables_overwritemechanism storedNamedExprAST nodes and re-evaluated them in subsequent assertions, in_call_reprcompare's results tuple, and in explanation formatting.Fix: Remove
variables_overwriteentirely and instead:assign()when a comparator walrus targets the same nameTest plan
test_walrus_in_assertion_basicandtest_walrus_running_counter)test_assertrewrite.pysuite passes (118 tests; only pre-existing subprocess env failures excluded)TestIssue14445Coverage matrix
Lands the first ten tests of the matrix from #14813 — the
single-eval-walrus,order-compare-left,order-binop-leftandorder-call-argumentcases — as passing tests, next to the fix that makes them pass.Review tooling
The two rewrite-dump scripts that were here have moved to #14921, as
@Pierre-Sassoulas asked — they are review aid, not part of the fix. Reduced to
one script (410 lines over three files down to 130 over one), and its default
mode answers @bluetech's question: the plain source of a snippet diffed against
what this checkout's rewriter emits for it. That PR shows the output for the
walrus BoolOp example on today's
main. This branch merges it, so the scriptshows up here until it lands.
Made with Cursor