From be39f2fffef341b37edc3fa3420ddcc7d7928123 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Sat, 22 Aug 2026 07:52:33 +0200 Subject: [PATCH 01/11] scripts: add an assert-rewrite diff tool 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. --- changelog/14921.contrib.rst | 1 + scripts/diff-assert-rewrite.py | 130 +++++++++++++++++++++++++++++++++ 2 files changed, 131 insertions(+) create mode 100644 changelog/14921.contrib.rst create mode 100644 scripts/diff-assert-rewrite.py diff --git a/changelog/14921.contrib.rst b/changelog/14921.contrib.rst new file mode 100644 index 00000000000..fdf44c5ea6a --- /dev/null +++ b/changelog/14921.contrib.rst @@ -0,0 +1 @@ +Added `scripts/diff-assert-rewrite.py`, which diffs the assert-rewritten form of a snippet between the plain source, this checkout and released pytest versions. diff --git a/scripts/diff-assert-rewrite.py b/scripts/diff-assert-rewrite.py new file mode 100644 index 00000000000..18c7b38b824 --- /dev/null +++ b/scripts/diff-assert-rewrite.py @@ -0,0 +1,130 @@ +"""Show what assertion rewriting does to a snippet, as a diff. + +Each side is one of ``plain`` (the source as written), ``worktree`` (this +checkout's ``src/``) or a released pytest version, which is fetched on demand +with ``uv run --with pytest==VERSION``. Sides are dumped as rewritten source +(``ast.unparse``) or as an AST, then diffed. + +Usage:: + + # what rewriting does to a snippet -- plain vs worktree, as source: + python scripts/diff-assert-rewrite.py -c 'assert (x := f()) and (x := False)' + + # a behaviour change against a release, over a whole file: + python scripts/diff-assert-rewrite.py --left 8.3.4 testing/example.py + + # same, as AST, when the source form hides the difference: + python scripts/diff-assert-rewrite.py --left 8.3.4 --format ast -c 'assert a == b' + +Exits 1 when the two sides differ, 0 when they do not. +""" + +from __future__ import annotations + +import argparse +import difflib +import os +from pathlib import Path +import subprocess +import sys +import tempfile + + +# Runs inside the environment of the pytest version under inspection: reads +# the source file named on its command line, writes the dump to stdout. +_WORKER = """ +import ast, sys +fmt, mode, path = sys.argv[1:4] +source = open(path, "rb").read() +tree = ast.parse(source) +if mode == "rewrite": + from _pytest.assertion.rewrite import rewrite_asserts + rewrite_asserts(tree, source) + ast.fix_missing_locations(tree) +print(ast.unparse(tree) if fmt == "source" else ast.dump(tree, indent=2)) +""" + +_COLORS = {"-": "\033[31m", "+": "\033[32m", "@": "\033[36m"} + + +def spawn(spec: str, fmt: str, path: Path) -> subprocess.Popen[bytes]: + """Start the dump of one side -- callers start both, then collect.""" + args = [fmt, "plain" if spec == "plain" else "rewrite", str(path)] + env = None + if spec in ("plain", "worktree"): + cmd = [sys.executable, "-c", _WORKER, *args] + if spec == "worktree": + src = Path(__file__).parent.parent / "src" + env = os.environ | {"PYTHONPATH": str(src)} + else: + cmd = ["uv", "run", "--no-project", "--with", f"pytest=={spec}"] + cmd += ["--", "python", "-c", _WORKER, *args] + try: + return subprocess.Popen(cmd, env=env, stdout=subprocess.PIPE) + except FileNotFoundError as exc: + raise SystemExit( + f"{exc.filename} not found (uv: https://docs.astral.sh/uv/)" + ) from None + + +def collect(spec: str, proc: subprocess.Popen[bytes]) -> list[str]: + assert proc.stdout is not None + out: bytes = proc.stdout.read() + if proc.wait(): + raise SystemExit(f"dumping {spec} failed") + return out.decode().splitlines() + + +def main(argv: list[str] | None = None) -> None: + parser = argparse.ArgumentParser( + description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter + ) + parser.add_argument( + "file", type=Path, nargs="?", help="file to rewrite (default: stdin)" + ) + parser.add_argument("-c", "--code", help="snippet to rewrite instead of a file") + parser.add_argument( + "--left", + default="plain", + metavar="SPEC", + help="'plain', 'worktree' or a pytest version", + ) + parser.add_argument( + "--right", default="worktree", metavar="SPEC", help="the same, other side" + ) + parser.add_argument("--format", choices=("source", "ast"), default="source") + parser.add_argument("--no-color", action="store_true") + args = parser.parse_args(argv) + + if args.code is not None: + source = args.code.encode() + elif args.file is not None: + source = args.file.read_bytes() + else: + source = sys.stdin.buffer.read() + + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp, "snippet.py") + path.write_bytes(source) + procs = [ + (side, spawn(side, args.format, path)) for side in (args.left, args.right) + ] + left, right = [collect(side, proc) for side, proc in procs] + diff = list( + difflib.unified_diff( + left, right, fromfile=args.left, tofile=args.right, lineterm="" + ) + ) + if not diff: + print(f"{args.left} and {args.right} agree on the {args.format} form") + return + + color = not args.no_color and sys.stdout.isatty() + for line in diff: + prefix = _COLORS.get(line[:1], "") if color else "" + print(f"{prefix}{line}\033[0m" if prefix else line) + raise SystemExit(1) + + +if __name__ == "__main__": + main() From a39f7393c7888c7febf77b5ac8c534de1d2575d4 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Fri, 8 May 2026 10:37:47 +0200 Subject: [PATCH 02/11] fix(rewrite): prevent walrus operator double evaluation in assertions Fixes #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 Co-authored-by: Anthropic Claude Sonnet 4 --- src/_pytest/assertion/rewrite.py | 102 +++++++++++-------------------- testing/test_assertrewrite.py | 68 ++++++++++++++++++++- 2 files changed, 102 insertions(+), 68 deletions(-) diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py index 362c93d7253..7b3be3fb107 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/src/_pytest/assertion/rewrite.py @@ -3,7 +3,6 @@ from __future__ import annotations import ast -from collections import defaultdict from collections.abc import Callable from collections.abc import Iterable from collections.abc import Iterator @@ -57,10 +56,6 @@ from _pytest.assertion import AssertionState -class Sentinel: - pass - - assertstate_key = StashKey["AssertionState"]() # pytest caches rewritten pycs in pycache dirs @@ -68,9 +63,6 @@ class Sentinel: PYC_EXT = ".py" + ((__debug__ and "c") or "o") PYC_TAIL = "." + PYTEST_TAG + PYC_EXT -# Special marker that denotes we have just left a scope definition -_SCOPE_END_MARKER = Sentinel() - class AssertionRewritingHook(importlib.abc.MetaPathFinder, importlib.abc.Loader): """PEP302/PEP451 import hook which rewrites asserts.""" @@ -642,14 +634,8 @@ class AssertionRewriter(ast.NodeVisitor): .push_format_context() and .pop_format_context() which allows to build another %-formatted string while already building one. - :scope: A tuple containing the current scope used for variables_overwrite. - - :variables_overwrite: A dict filled with references to variables - that change value within an assert. This happens when a variable is - reassigned with the walrus operator - - This state, except the variables_overwrite, is reset on every new assert - statement visited and used by the other visitors. + This state is reset on every new assert statement visited and used by + the other visitors. """ def __init__( @@ -665,10 +651,6 @@ def __init__( else: self.enable_assertion_pass_hook = False self.source = source - self.scope: tuple[ast.AST, ...] = () - self.variables_overwrite: defaultdict[tuple[ast.AST, ...], dict[str, str]] = ( - defaultdict(dict) - ) def run(self, mod: ast.Module) -> None: """Find all assert statements in *mod* and rewrite them.""" @@ -718,16 +700,9 @@ def run(self, mod: ast.Module) -> None: mod.body[pos:pos] = imports # Collect asserts. - self.scope = (mod,) - nodes: list[ast.AST | Sentinel] = [mod] + nodes: list[ast.AST] = [mod] while nodes: node = nodes.pop() - if isinstance(node, ast.FunctionDef | ast.AsyncFunctionDef | ast.ClassDef): - self.scope = tuple((*self.scope, node)) - nodes.append(_SCOPE_END_MARKER) - if node == _SCOPE_END_MARKER: - self.scope = self.scope[:-1] - continue assert isinstance(node, ast.AST) for name, field in ast.iter_fields(node): if isinstance(field, list): @@ -954,15 +929,17 @@ def visit_Assert(self, assert_: ast.Assert) -> list[ast.stmt]: return self.statements def visit_NamedExpr(self, name: ast.NamedExpr) -> tuple[ast.NamedExpr, str]: - # This method handles the 'walrus operator' repr of the target - # name if it's a local variable or _should_repr_global_name() - # thinks it's acceptable. + # Return the NamedExpr as-is so it evaluates in its natural position + # (preserving left-to-right evaluation order). For the explanation, + # reference the target variable (already assigned by the walrus) to + # avoid re-evaluating the expression. locs = ast.Call(self.builtin("locals"), [], []) target_id = name.target.id + target_name = ast.Name(target_id, ast.Load()) inlocs = ast.Compare(ast.Constant(target_id), [ast.In()], [locs]) - dorepr = self.helper("_should_repr_global_name", name) + dorepr = self.helper("_should_repr_global_name", target_name) test = ast.BoolOp(ast.Or(), [inlocs, dorepr]) - expr = ast.IfExp(test, self.display(name), ast.Constant(target_id)) + expr = ast.IfExp(test, self.display(target_name), ast.Constant(target_id)) return name, self.explanation_param(expr) def visit_Name(self, name: ast.Name) -> tuple[ast.Name, str]: @@ -988,20 +965,9 @@ def visit_BoolOp(self, boolop: ast.BoolOp) -> tuple[ast.Name, str]: for i, v in enumerate(boolop.values): if i: fail_inner: list[ast.stmt] = [] - # cond is set in a prior loop iteration below - self.expl_stmts.append(ast.If(cond, fail_inner, [])) # noqa: F821 + # expl_cond is set in a prior loop iteration below + self.expl_stmts.append(ast.If(expl_cond, fail_inner, [])) # noqa: F821 self.expl_stmts = fail_inner - match v: - # Check if the left operand is an ast.NamedExpr and the value has already been visited - case ast.Compare( - left=ast.NamedExpr(target=ast.Name(id=target_id)) - ) if target_id in [ - e.id for e in boolop.values[:i] if hasattr(e, "id") - ]: - pytest_temp = self.variable() - self.variables_overwrite[self.scope][target_id] = v.left # type:ignore[assignment] - # mypy's false positive, we're checking that the 'target' attribute exists. - v.left.target.id = pytest_temp # type:ignore[attr-defined] self.push_format_context() res, expl = self.visit(v) body.append(ast.Assign([ast.Name(res_var, ast.Store())], res)) @@ -1012,8 +978,16 @@ def visit_BoolOp(self, boolop: ast.BoolOp) -> tuple[ast.Name, str]: cond: ast.expr = res if is_or: cond = ast.UnaryOp(ast.Not(), cond) + # Capture the condition in a temp variable so the explanation + # path (which runs after walrus operators may have modified + # the original variable) sees the correct truthiness. + cond_var = self.variable() + body.append(ast.Assign([ast.Name(cond_var, ast.Store())], cond)) + expl_cond: ast.expr = ast.Name(cond_var, ast.Load()) # noqa: F841 inner: list[ast.stmt] = [] - self.statements.append(ast.If(cond, inner, [])) + self.statements.append( + ast.If(ast.Name(cond_var, ast.Load()), inner, []) + ) self.statements = body = inner self.statements = save self.expl_stmts = fail_save @@ -1043,19 +1017,10 @@ def visit_Call(self, call: ast.Call) -> tuple[ast.Name, str]: new_args = [] new_kwargs = [] for arg in call.args: - if isinstance(arg, ast.Name) and arg.id in self.variables_overwrite.get( - self.scope, {} - ): - arg = self.variables_overwrite[self.scope][arg.id] # type:ignore[assignment] res, expl = self.visit(arg) arg_expls.append(expl) new_args.append(res) for keyword in call.keywords: - match keyword.value: - case ast.Name(id=id) if id in self.variables_overwrite.get( - self.scope, {} - ): - keyword.value = self.variables_overwrite[self.scope][id] # type:ignore[assignment] res, expl = self.visit(keyword.value) new_kwargs.append(ast.keyword(keyword.arg, res)) if keyword.arg: @@ -1090,17 +1055,13 @@ def visit_Attribute(self, attr: ast.Attribute) -> tuple[ast.Name, str]: def visit_Compare(self, comp: ast.Compare) -> tuple[ast.expr, str]: self.push_format_context() - # We first check if we have overwritten a variable in the previous assert - match comp.left: - case ast.Name(id=name_id) if name_id in self.variables_overwrite.get( - self.scope, {} - ): - comp.left = self.variables_overwrite[self.scope][name_id] # type: ignore[assignment] - case ast.NamedExpr(target=ast.Name(id=target_id)): - self.variables_overwrite[self.scope][target_id] = comp.left # type: ignore[assignment] left_res, left_expl = self.visit(comp.left) if isinstance(comp.left, ast.Compare | ast.BoolOp): left_expl = f"({left_expl})" + # If the left operand is a NamedExpr, assign it to a temp so the + # walrus executes before any right-side expressions are hoisted. + if isinstance(left_res, ast.NamedExpr): + left_res = self.assign(left_res) res_variables = [self.variable() for i in range(len(comp.ops))] load_names: list[ast.expr] = [ast.Name(v, ast.Load()) for v in res_variables] store_names = [ast.Name(v, ast.Store()) for v in res_variables] @@ -1109,13 +1070,16 @@ def visit_Compare(self, comp: ast.Compare) -> tuple[ast.expr, str]: syms: list[ast.expr] = [] results = [left_res] for i, op, next_operand in it: + # If the next operand is a walrus that assigns to the same name as + # the current left_res, we must freeze left_res's value before the + # walrus modifies it. match (next_operand, left_res): case ( ast.NamedExpr(target=ast.Name(id=target_id)), ast.Name(id=name_id), ) if target_id == name_id: - next_operand.target.id = self.variable() - self.variables_overwrite[self.scope][name_id] = next_operand # type: ignore[assignment] + left_res = self.assign(left_res) + results[-1] = left_res next_res, next_expl = self.visit(next_operand) if isinstance(next_operand, ast.Compare | ast.BoolOp): @@ -1128,6 +1092,12 @@ def visit_Compare(self, comp: ast.Compare) -> tuple[ast.expr, str]: res_expr = ast.copy_location(ast.Compare(left_res, [op], [next_res]), comp) self.statements.append(ast.Assign([store_names[i]], res_expr)) left_res, left_expl = next_res, next_expl + # Replace NamedExpr entries in results with their target variable + # to avoid re-evaluating walrus operators in the explanation path. + results = [ + ast.Name(r.target.id, ast.Load()) if isinstance(r, ast.NamedExpr) else r + for r in results + ] # Use pytest.assertion.util._reprcompare if that's available. expl_call = self.helper( "_call_reprcompare", diff --git a/testing/test_assertrewrite.py b/testing/test_assertrewrite.py index 12e12449693..e667ffe03f1 100644 --- a/testing/test_assertrewrite.py +++ b/testing/test_assertrewrite.py @@ -1773,7 +1773,7 @@ def test_walrus_operator_change_boolean_value(): ) result = pytester.runpytest() assert result.ret == 1 - result.stdout.fnmatch_lines(["*assert not (True and False is False)"]) + result.stdout.fnmatch_lines(["*assert not (False and False is False)"]) def test_assertion_walrus_operator_boolean_none_fails( self, pytester: Pytester @@ -1787,7 +1787,7 @@ def test_walrus_operator_change_boolean_value(): ) result = pytester.runpytest() assert result.ret == 1 - result.stdout.fnmatch_lines(["*assert not (True and None is None)"]) + result.stdout.fnmatch_lines(["*assert not (None and None is None)"]) def test_assertion_walrus_operator_value_changes_cleared_after_each_test( self, pytester: Pytester @@ -1931,6 +1931,70 @@ def test_2(): assert result.ret == 0 +class TestIssue14445: + """Regression tests for #14445: walrus operator double evaluation.""" + + def test_walrus_no_double_eval_basic(self, pytester: Pytester) -> None: + """Walrus captures the value at assignment time, not re-evaluated later.""" + pytester.makepyfile( + """ + class Counter: + def __init__(self): + self.value = 0 + def increment(self): + self.value += 1 + + def test_walrus_in_assertion_basic(): + c = Counter() + assert (before := c.value) == 0 + c.increment() + assert before != (after := c.value) + """ + ) + result = pytester.runpytest() + assert result.ret == 0 + + def test_walrus_no_double_eval_running_counter(self, pytester: Pytester) -> None: + """Walrus increments fire exactly once per assert statement.""" + pytester.makepyfile( + """ + def test_walrus_running_counter(): + count = 0 + items = [] + items.append("a") + assert (count := count + 1) == len(items) + items.append("b") + assert (count := count + 1) == len(items) + items.append("c") + assert (count := count + 1) == len(items) + assert count == 3 + """ + ) + result = pytester.runpytest() + assert result.ret == 0 + + def test_walrus_no_double_eval_in_function_call(self, pytester: Pytester) -> None: + """Walrus in function call arguments not evaluated twice.""" + pytester.makepyfile( + """ + call_count = 0 + + def side_effect(): + global call_count + call_count += 1 + return call_count + + def test_walrus_side_effect(): + assert (val := side_effect()) == 1 + assert val == 1 + assert (val := side_effect()) == 2 + assert val == 2 + """ + ) + result = pytester.runpytest() + assert result.ret == 0 + + @pytest.mark.skipif( sys.maxsize <= (2**31 - 1), reason="Causes OverflowError on 32bit systems" ) From 5167153a774dedc5a9f01a022aad27c58da553f6 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Fri, 8 May 2026 12:59:20 +0200 Subject: [PATCH 03/11] Add changelog fragment for #14445 Co-authored-by: Cursor AI Co-authored-by: Anthropic Claude Sonnet 4 --- changelog/14445.bugfix.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog/14445.bugfix.rst diff --git a/changelog/14445.bugfix.rst b/changelog/14445.bugfix.rst new file mode 100644 index 00000000000..aaae0c615f5 --- /dev/null +++ b/changelog/14445.bugfix.rst @@ -0,0 +1 @@ +Fixed assertion rewriting evaluating walrus operator (``:=``) expressions multiple times, causing incorrect test results when the expression had side effects (e.g., incrementing a counter or calling a function). From b83ae76b1d15b17958441cdc8beee1dc44ca90ed Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Fri, 8 May 2026 13:04:00 +0200 Subject: [PATCH 04/11] test(rewrite): add xfail tests for remaining walrus edge cases 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 Co-authored-by: Anthropic Claude Sonnet 4 --- testing/test_assertrewrite.py | 40 +++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/testing/test_assertrewrite.py b/testing/test_assertrewrite.py index e667ffe03f1..147df42eb0b 100644 --- a/testing/test_assertrewrite.py +++ b/testing/test_assertrewrite.py @@ -1994,6 +1994,46 @@ def test_walrus_side_effect(): result = pytester.runpytest() assert result.ret == 0 + @pytest.mark.xfail(reason="BoolOp condition re-evaluates walrus operand") + def test_walrus_no_double_eval_in_boolop(self, pytester: Pytester) -> None: + """Bare walrus as a BoolOp operand must not be evaluated twice.""" + pytester.makepyfile( + """ + call_count = 0 + + def side_effect(): + global call_count + call_count += 1 + return call_count + + def test_walrus_boolop(): + assert (x := side_effect()) and x == 1 + assert call_count == 1 + """ + ) + result = pytester.runpytest() + assert result.ret == 0 + + @pytest.mark.xfail(reason="Chained compare re-evaluates walrus with same target") + def test_walrus_no_double_eval_chained_compare(self, pytester: Pytester) -> None: + """Same walrus target in chained comparison must evaluate each once.""" + pytester.makepyfile( + """ + call_count = 0 + + def track(value): + global call_count + call_count += 1 + return value + + def test_walrus_chained(): + assert (x := track(1)) < (x := track(3)) < (x := track(5)) + assert call_count == 3 + """ + ) + result = pytester.runpytest() + assert result.ret == 0 + @pytest.mark.skipif( sys.maxsize <= (2**31 - 1), reason="Causes OverflowError on 32bit systems" From 8fe2b3a56eae775796328383aa448b69c5fa6c45 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Fri, 8 May 2026 13:05:41 +0200 Subject: [PATCH 05/11] fix(rewrite): avoid double evaluation of walrus in BoolOp condition 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 Co-authored-by: Anthropic Claude Sonnet 4 --- src/_pytest/assertion/rewrite.py | 9 +++++---- testing/test_assertrewrite.py | 1 - 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py index 7b3be3fb107..f015b703b6a 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/src/_pytest/assertion/rewrite.py @@ -975,12 +975,13 @@ def visit_BoolOp(self, boolop: ast.BoolOp) -> tuple[ast.Name, str]: call = ast.Call(app, [expl_format], []) self.expl_stmts.append(ast.Expr(call)) if i < levels: - cond: ast.expr = res + # Use res_var (already assigned above) rather than res directly, + # so that NamedExpr operands aren't evaluated a second time. + cond: ast.expr = ast.Name(res_var, ast.Load()) if is_or: cond = ast.UnaryOp(ast.Not(), cond) - # Capture the condition in a temp variable so the explanation - # path (which runs after walrus operators may have modified - # the original variable) sees the correct truthiness. + # Capture the condition in a stable temp for the explanation + # path — res_var is overwritten by subsequent operands. cond_var = self.variable() body.append(ast.Assign([ast.Name(cond_var, ast.Store())], cond)) expl_cond: ast.expr = ast.Name(cond_var, ast.Load()) # noqa: F841 diff --git a/testing/test_assertrewrite.py b/testing/test_assertrewrite.py index 147df42eb0b..513eec42f3f 100644 --- a/testing/test_assertrewrite.py +++ b/testing/test_assertrewrite.py @@ -1994,7 +1994,6 @@ def test_walrus_side_effect(): result = pytester.runpytest() assert result.ret == 0 - @pytest.mark.xfail(reason="BoolOp condition re-evaluates walrus operand") def test_walrus_no_double_eval_in_boolop(self, pytester: Pytester) -> None: """Bare walrus as a BoolOp operand must not be evaluated twice.""" pytester.makepyfile( From c5c9443f38c2ac5b4da4fe17014539074076c93f Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Fri, 8 May 2026 13:08:10 +0200 Subject: [PATCH 06/11] fix(rewrite): assign walrus comparators to temps in chained comparisons 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 Co-authored-by: Anthropic Claude Sonnet 4 --- src/_pytest/assertion/rewrite.py | 11 +++++------ testing/test_assertrewrite.py | 1 - 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py index f015b703b6a..0bc34eb3463 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/src/_pytest/assertion/rewrite.py @@ -1085,6 +1085,11 @@ def visit_Compare(self, comp: ast.Compare) -> tuple[ast.expr, str]: next_res, next_expl = self.visit(next_operand) if isinstance(next_operand, ast.Compare | ast.BoolOp): next_expl = f"({next_expl})" + # Assign NamedExpr comparators to a temp so each walrus evaluates + # exactly once — critical for chained comparisons where the same + # node would otherwise be re-evaluated as left_res next iteration. + if isinstance(next_res, ast.NamedExpr): + next_res = self.assign(next_res) results.append(next_res) sym = BINOP_MAP[op.__class__] syms.append(ast.Constant(sym)) @@ -1093,12 +1098,6 @@ def visit_Compare(self, comp: ast.Compare) -> tuple[ast.expr, str]: res_expr = ast.copy_location(ast.Compare(left_res, [op], [next_res]), comp) self.statements.append(ast.Assign([store_names[i]], res_expr)) left_res, left_expl = next_res, next_expl - # Replace NamedExpr entries in results with their target variable - # to avoid re-evaluating walrus operators in the explanation path. - results = [ - ast.Name(r.target.id, ast.Load()) if isinstance(r, ast.NamedExpr) else r - for r in results - ] # Use pytest.assertion.util._reprcompare if that's available. expl_call = self.helper( "_call_reprcompare", diff --git a/testing/test_assertrewrite.py b/testing/test_assertrewrite.py index 513eec42f3f..103b900cd6a 100644 --- a/testing/test_assertrewrite.py +++ b/testing/test_assertrewrite.py @@ -2013,7 +2013,6 @@ def test_walrus_boolop(): result = pytester.runpytest() assert result.ret == 0 - @pytest.mark.xfail(reason="Chained compare re-evaluates walrus with same target") def test_walrus_no_double_eval_chained_compare(self, pytester: Pytester) -> None: """Same walrus target in chained comparison must evaluate each once.""" pytester.makepyfile( From 555a1a2d5c3a4ec9dba234f2f9e4d9988e4b7b94 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Wed, 3 Jun 2026 10:55:00 +0200 Subject: [PATCH 07/11] fix(rewrite): show correct walrus values in BoolOp explanations 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 Co-authored-by: Anthropic Claude Sonnet 4 --- src/_pytest/assertion/rewrite.py | 19 +++++++++---------- testing/test_assertrewrite.py | 22 ++++++++++++++++++++-- 2 files changed, 29 insertions(+), 12 deletions(-) diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py index 0bc34eb3463..3ad244a0ff8 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/src/_pytest/assertion/rewrite.py @@ -930,9 +930,8 @@ def visit_Assert(self, assert_: ast.Assert) -> list[ast.stmt]: def visit_NamedExpr(self, name: ast.NamedExpr) -> tuple[ast.NamedExpr, str]: # Return the NamedExpr as-is so it evaluates in its natural position - # (preserving left-to-right evaluation order). For the explanation, - # reference the target variable (already assigned by the walrus) to - # avoid re-evaluating the expression. + # (preserving left-to-right evaluation order in function calls, etc.). + # For the explanation, reference the target variable. locs = ast.Call(self.builtin("locals"), [], []) target_id = name.target.id target_name = ast.Name(target_id, ast.Load()) @@ -971,12 +970,17 @@ def visit_BoolOp(self, boolop: ast.BoolOp) -> tuple[ast.Name, str]: self.push_format_context() res, expl = self.visit(v) body.append(ast.Assign([ast.Name(res_var, ast.Store())], res)) + # For Name/NamedExpr operands, track the value in a stable + # @py_assert variable so the explanation shows the value at + # evaluation time — even if a later walrus overwrites the name. + if isinstance(v, ast.NamedExpr | ast.Name): + tracked = self.assign(ast.Name(res_var, ast.Load())) + for key in self.stack[-1]: + self.stack[-1][key] = self.display(tracked) expl_format = self.pop_format_context(ast.Constant(expl)) call = ast.Call(app, [expl_format], []) self.expl_stmts.append(ast.Expr(call)) if i < levels: - # Use res_var (already assigned above) rather than res directly, - # so that NamedExpr operands aren't evaluated a second time. cond: ast.expr = ast.Name(res_var, ast.Load()) if is_or: cond = ast.UnaryOp(ast.Not(), cond) @@ -1059,8 +1063,6 @@ def visit_Compare(self, comp: ast.Compare) -> tuple[ast.expr, str]: left_res, left_expl = self.visit(comp.left) if isinstance(comp.left, ast.Compare | ast.BoolOp): left_expl = f"({left_expl})" - # If the left operand is a NamedExpr, assign it to a temp so the - # walrus executes before any right-side expressions are hoisted. if isinstance(left_res, ast.NamedExpr): left_res = self.assign(left_res) res_variables = [self.variable() for i in range(len(comp.ops))] @@ -1085,9 +1087,6 @@ def visit_Compare(self, comp: ast.Compare) -> tuple[ast.expr, str]: next_res, next_expl = self.visit(next_operand) if isinstance(next_operand, ast.Compare | ast.BoolOp): next_expl = f"({next_expl})" - # Assign NamedExpr comparators to a temp so each walrus evaluates - # exactly once — critical for chained comparisons where the same - # node would otherwise be re-evaluated as left_res next iteration. if isinstance(next_res, ast.NamedExpr): next_res = self.assign(next_res) results.append(next_res) diff --git a/testing/test_assertrewrite.py b/testing/test_assertrewrite.py index 103b900cd6a..b9464f5aec3 100644 --- a/testing/test_assertrewrite.py +++ b/testing/test_assertrewrite.py @@ -1773,7 +1773,7 @@ def test_walrus_operator_change_boolean_value(): ) result = pytester.runpytest() assert result.ret == 1 - result.stdout.fnmatch_lines(["*assert not (False and False is False)"]) + result.stdout.fnmatch_lines(["*assert not (True and False is False)"]) def test_assertion_walrus_operator_boolean_none_fails( self, pytester: Pytester @@ -1787,7 +1787,7 @@ def test_walrus_operator_change_boolean_value(): ) result = pytester.runpytest() assert result.ret == 1 - result.stdout.fnmatch_lines(["*assert not (None and None is None)"]) + result.stdout.fnmatch_lines(["*assert not (True and None is None)"]) def test_assertion_walrus_operator_value_changes_cleared_after_each_test( self, pytester: Pytester @@ -2032,6 +2032,24 @@ def test_walrus_chained(): result = pytester.runpytest() assert result.ret == 0 + def test_walrus_boolop_same_target_correct_explanation( + self, pytester: Pytester + ) -> None: + """Multiple walrus operators to the same name in a BoolOp must show + each operand's value at evaluation time, not the final value.""" + pytester.makepyfile( + """ + def side_effect(): + return True + + def test_walrus_boolop(): + assert (x := side_effect()) and (x := False) + """ + ) + result = pytester.runpytest() + assert result.ret == 1 + result.stdout.fnmatch_lines(["*assert (True and False)"]) + @pytest.mark.skipif( sys.maxsize <= (2**31 - 1), reason="Causes OverflowError on 32bit systems" From d115f0e680e35af1c4b7acf7679087cc36c33995 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Wed, 3 Jun 2026 11:56:28 +0200 Subject: [PATCH 08/11] refactor(rewrite): minimal snapshots in BoolOp for walrus conflicts 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 Co-authored-by: Anthropic Claude Opus 4 --- src/_pytest/assertion/rewrite.py | 52 +++++++++++++++++++++----------- 1 file changed, 34 insertions(+), 18 deletions(-) diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py index 3ad244a0ff8..781cb27b45c 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/src/_pytest/assertion/rewrite.py @@ -959,40 +959,56 @@ def visit_BoolOp(self, boolop: ast.BoolOp) -> tuple[ast.Name, str]: body = save = self.statements fail_save = self.expl_stmts levels = len(boolop.values) - 1 + # Pre-scan: for each operand position, collect the set of variable + # names that a *later* operand's walrus operator will overwrite. + # An operand needs a snapshot only when its value references a name + # in this set (otherwise the explanation would show the post-walrus + # value instead of the value at evaluation time). + later_walrus_targets: list[set[str]] = [set() for _ in boolop.values] + seen: set[str] = set() + for idx in range(len(boolop.values) - 1, -1, -1): + later_walrus_targets[idx] = set(seen) + for node in ast.walk(boolop.values[idx]): + if isinstance(node, ast.NamedExpr): + seen.add(node.target.id) self.push_format_context() - # Process each operand, short-circuiting if needed. + # Process each operand, short-circuiting as needed. for i, v in enumerate(boolop.values): if i: fail_inner: list[ast.stmt] = [] - # expl_cond is set in a prior loop iteration below - self.expl_stmts.append(ast.If(expl_cond, fail_inner, [])) # noqa: F821 + # cond is set in a prior loop iteration below + self.expl_stmts.append(ast.If(cond, fail_inner, [])) # noqa: F821 self.expl_stmts = fail_inner self.push_format_context() res, expl = self.visit(v) body.append(ast.Assign([ast.Name(res_var, ast.Store())], res)) - # For Name/NamedExpr operands, track the value in a stable - # @py_assert variable so the explanation shows the value at - # evaluation time — even if a later walrus overwrites the name. - if isinstance(v, ast.NamedExpr | ast.Name): - tracked = self.assign(ast.Name(res_var, ast.Load())) + # Snapshot when the raw ``res`` node would be unsafe to reuse + # as a condition or explanation reference: + # - NamedExpr (non-last): reusing the node re-evaluates the + # walrus expression including any side effects. + # - Name whose variable a later walrus overwrites: the + # explanation would show the post-walrus value. + needs_snapshot = (isinstance(v, ast.NamedExpr) and i < levels) or ( + isinstance(v, ast.Name) and v.id in later_walrus_targets[i] + ) + if needs_snapshot: + snapshot = self.assign(ast.Name(res_var, ast.Load())) + res = snapshot for key in self.stack[-1]: - self.stack[-1][key] = self.display(tracked) + self.stack[-1][key] = self.display(snapshot) expl_format = self.pop_format_context(ast.Constant(expl)) call = ast.Call(app, [expl_format], []) self.expl_stmts.append(ast.Expr(call)) if i < levels: - cond: ast.expr = ast.Name(res_var, ast.Load()) + # Short-circuit: and → continue if truthy; or → if falsy. + # ``res`` is a stable reference (Name vars are only + # snapshotted when a later walrus would corrupt them; + # calls/compares return @py_assert vars from assign()). + cond: ast.expr = res if is_or: cond = ast.UnaryOp(ast.Not(), cond) - # Capture the condition in a stable temp for the explanation - # path — res_var is overwritten by subsequent operands. - cond_var = self.variable() - body.append(ast.Assign([ast.Name(cond_var, ast.Store())], cond)) - expl_cond: ast.expr = ast.Name(cond_var, ast.Load()) # noqa: F841 inner: list[ast.stmt] = [] - self.statements.append( - ast.If(ast.Name(cond_var, ast.Load()), inner, []) - ) + self.statements.append(ast.If(cond, inner, [])) self.statements = body = inner self.statements = save self.expl_stmts = fail_save From 5fe5987f18761f4fe2ff57b7aced8bc0fdfa203f Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Fri, 31 Jul 2026 19:52:36 +0200 Subject: [PATCH 09/11] fix(rewrite): freeze operands a later walrus would clobber 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 --- changelog/14445.bugfix.rst | 2 + src/_pytest/assertion/rewrite.py | 79 +++++++++------- testing/test_assertrewrite_coverage.py | 123 +++++++++++++++++++++++++ 3 files changed, 173 insertions(+), 31 deletions(-) diff --git a/changelog/14445.bugfix.rst b/changelog/14445.bugfix.rst index aaae0c615f5..a9547581073 100644 --- a/changelog/14445.bugfix.rst +++ b/changelog/14445.bugfix.rst @@ -1 +1,3 @@ Fixed assertion rewriting evaluating walrus operator (``:=``) expressions multiple times, causing incorrect test results when the expression had side effects (e.g., incrementing a counter or calling a function). + +Operands preceding a walrus operator are now evaluated -- and reported -- before it rebinds their name, so ``assert value != identity(value := value.lower())`` keeps Python's left-to-right evaluation order. diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py index 781cb27b45c..27953336c5c 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/src/_pytest/assertion/rewrite.py @@ -530,6 +530,16 @@ def traverse_node(node: ast.AST) -> Iterator[ast.AST]: yield from traverse_node(child) +def _walrus_targets(nodes: Iterable[ast.expr]) -> set[str]: + """Return the names any walrus operator in *nodes* rebinds.""" + return { + sub.target.id + for node in nodes + for sub in ast.walk(node) + if isinstance(sub, ast.NamedExpr) + } + + @functools.lru_cache(maxsize=1) def _get_assertion_exprs(src: bytes) -> dict[int, str]: """Return a mapping from {lineno: "assertion test expression"}.""" @@ -951,6 +961,27 @@ def visit_Name(self, name: ast.Name) -> tuple[ast.Name, str]: expr = ast.IfExp(test, self.display(name), ast.Constant(name.id)) return name, self.explanation_param(expr) + def visit_operand( + self, operand: ast.expr, later: Sequence[ast.expr] + ) -> tuple[ast.expr, str]: + """Visit an operand, freezing it against walrus operators in *later*. + + Operands are rewritten into statements that run in source order, but + a plain name is left as a bare load evaluated at the very end, when + the enclosing expression is assembled. A walrus operator in a later + operand rebinds that name in between, so both the value used and the + value reported would be the post-walrus one -- Python evaluates the + earlier operand first. Copy the value into a temporary instead. + """ + specifiers = set(self.explanation_specifiers) + res, expl = self.visit(operand) + if isinstance(res, ast.Name) and res.id in _walrus_targets(later): + snapshot = self.assign(res) + for key in set(self.explanation_specifiers) - specifiers: + self.explanation_specifiers[key] = self.display(snapshot) + res = snapshot + return res, expl + def visit_BoolOp(self, boolop: ast.BoolOp) -> tuple[ast.Name, str]: res_var = self.variable() expl_list = self.assign(ast.List([], ast.Load())) @@ -959,18 +990,10 @@ def visit_BoolOp(self, boolop: ast.BoolOp) -> tuple[ast.Name, str]: body = save = self.statements fail_save = self.expl_stmts levels = len(boolop.values) - 1 - # Pre-scan: for each operand position, collect the set of variable - # names that a *later* operand's walrus operator will overwrite. - # An operand needs a snapshot only when its value references a name - # in this set (otherwise the explanation would show the post-walrus - # value instead of the value at evaluation time). - later_walrus_targets: list[set[str]] = [set() for _ in boolop.values] - seen: set[str] = set() - for idx in range(len(boolop.values) - 1, -1, -1): - later_walrus_targets[idx] = set(seen) - for node in ast.walk(boolop.values[idx]): - if isinstance(node, ast.NamedExpr): - seen.add(node.target.id) + later_walrus_targets = [ + _walrus_targets(boolop.values[idx + 1 :]) + for idx in range(len(boolop.values)) + ] self.push_format_context() # Process each operand, short-circuiting as needed. for i, v in enumerate(boolop.values): @@ -1024,7 +1047,7 @@ def visit_UnaryOp(self, unary: ast.UnaryOp) -> tuple[ast.Name, str]: def visit_BinOp(self, binop: ast.BinOp) -> tuple[ast.Name, str]: symbol = BINOP_MAP[binop.op.__class__] - left_expr, left_expl = self.visit(binop.left) + left_expr, left_expl = self.visit_operand(binop.left, [binop.right]) right_expr, right_expl = self.visit(binop.right) explanation = f"({left_expl} {symbol} {right_expl})" res = self.assign( @@ -1033,16 +1056,19 @@ def visit_BinOp(self, binop: ast.BinOp) -> tuple[ast.Name, str]: return res, explanation def visit_Call(self, call: ast.Call) -> tuple[ast.Name, str]: - new_func, func_expl = self.visit(call.func) + # The callee and every argument are evaluated left to right, so each of + # them has to be frozen against walrus operators in what follows. + operands = [*call.args, *(keyword.value for keyword in call.keywords)] + new_func, func_expl = self.visit_operand(call.func, operands) arg_expls = [] new_args = [] new_kwargs = [] - for arg in call.args: - res, expl = self.visit(arg) + for i, arg in enumerate(call.args): + res, expl = self.visit_operand(arg, operands[i + 1 :]) arg_expls.append(expl) new_args.append(res) - for keyword in call.keywords: - res, expl = self.visit(keyword.value) + for i, keyword in enumerate(call.keywords, start=len(call.args)): + res, expl = self.visit_operand(keyword.value, operands[i + 1 :]) new_kwargs.append(ast.keyword(keyword.arg, res)) if keyword.arg: arg_expls.append(keyword.arg + "=" + expl) @@ -1076,7 +1102,7 @@ def visit_Attribute(self, attr: ast.Attribute) -> tuple[ast.Name, str]: def visit_Compare(self, comp: ast.Compare) -> tuple[ast.expr, str]: self.push_format_context() - left_res, left_expl = self.visit(comp.left) + left_res, left_expl = self.visit_operand(comp.left, comp.comparators) if isinstance(comp.left, ast.Compare | ast.BoolOp): left_expl = f"({left_expl})" if isinstance(left_res, ast.NamedExpr): @@ -1089,18 +1115,9 @@ def visit_Compare(self, comp: ast.Compare) -> tuple[ast.expr, str]: syms: list[ast.expr] = [] results = [left_res] for i, op, next_operand in it: - # If the next operand is a walrus that assigns to the same name as - # the current left_res, we must freeze left_res's value before the - # walrus modifies it. - match (next_operand, left_res): - case ( - ast.NamedExpr(target=ast.Name(id=target_id)), - ast.Name(id=name_id), - ) if target_id == name_id: - left_res = self.assign(left_res) - results[-1] = left_res - - next_res, next_expl = self.visit(next_operand) + next_res, next_expl = self.visit_operand( + next_operand, comp.comparators[i + 1 :] + ) if isinstance(next_operand, ast.Compare | ast.BoolOp): next_expl = f"({next_expl})" if isinstance(next_res, ast.NamedExpr): diff --git a/testing/test_assertrewrite_coverage.py b/testing/test_assertrewrite_coverage.py index 3bc16d2917a..7517efb4210 100644 --- a/testing/test_assertrewrite_coverage.py +++ b/testing/test_assertrewrite_coverage.py @@ -763,6 +763,33 @@ def __getitem__(self, key): assert d["a"] == 100 """) + def test_walrus_in_compare_evaluated_once(self) -> None: + assert_single_evaluation(""" + def check(): + def side_effect(): + counter[0] += 1 + return 42 + assert (x := side_effect()) == 100 + """) + + def test_walrus_in_boolean_evaluated_once(self) -> None: + assert_single_evaluation(""" + def check(): + def side_effect(): + counter[0] += 1 + return 42 + assert (x := side_effect()) and False + """) + + def test_walrus_in_chained_compare_evaluated_once(self) -> None: + assert_single_evaluation(""" + def check(): + def side_effect(): + counter[0] += 1 + return 5 + assert 1 < (x := side_effect()) < 3 + """) + def test_method_call_evaluated_once(self) -> None: assert_single_evaluation(""" def check(): @@ -836,6 +863,72 @@ class TestEvaluationOrder: given it. """ + def test_compare_left_operand_precedes_walrus(self) -> None: + assert_evaluation_order(""" + def check(): + def identity(v): + return v + value = "Hello" + try: + assert value != identity(value := value.lower()) + except AssertionError: + return "raised", value + return "passed", value + """) + + def test_compare_reports_left_operand(self) -> None: + assert_introspects( + """ + def check(): + def identity(v): + return v + value = 2 + assert value == identity(value := 3) + """, + must_contain=["assert 2 == 3"], + ) + + def test_call_earlier_argument_precedes_walrus(self) -> None: + assert_evaluation_order(""" + def check(): + def identity(v): + return v + def collect(*values): + return values + value = "Hello" + try: + assert collect(value, identity(value := value.lower())) == ("Hello", "hello") + except AssertionError: + return "raised", value + return "passed", value + """) + + def test_binop_left_operand_precedes_walrus(self) -> None: + assert_evaluation_order(""" + def check(): + def identity(v): + return v + value = 1 + try: + assert value + identity(value := 5) == 6 + except AssertionError: + return "raised", value + return "passed", value + """) + + def test_chained_compare_operands_in_order(self) -> None: + assert_evaluation_order(""" + def check(): + def identity(v): + return v + value = 1 + try: + assert value < identity(value := 5) < 9 + except AssertionError: + return "raised", value + return "passed", value + """) + def test_container_literal_operand_in_order(self) -> None: """Guard: ``generic_visit`` hoists container literals into a temporary.""" assert_evaluation_order(""" @@ -914,6 +1007,36 @@ def take(self, value): return "passed", obj """) + def test_keyword_argument_precedes_walrus(self) -> None: + assert_evaluation_order(""" + def check(): + def identity(v): + return v + def collect(**kwargs): + return kwargs + value = 1 + try: + assert collect(a=value, b=identity(value := 2)) == {"a": 1, "b": 2} + except AssertionError: + return "raised", value + return "passed", value + """) + + def test_double_star_argument_precedes_walrus(self) -> None: + assert_evaluation_order(""" + def check(): + def identity(v): + return v + def collect(**kwargs): + return kwargs + mapping = {"a": 1} + try: + assert collect(**mapping, b=identity(mapping := {"a": 9})) == {"a": 1, "b": {"a": 9}} + except AssertionError: + return "raised", mapping + return "passed", mapping + """) + def test_ifexp_branches_in_order(self) -> None: """Guard: the condition is evaluated before the selected branch.""" assert_evaluation_order(""" From 242d5827d70897f02abf0534b0555af2cec23037 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Fri, 31 Jul 2026 20:42:11 +0200 Subject: [PATCH 10/11] fix(rewrite): freeze walrus and starred operands too 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. --- changelog/14814.bugfix.rst | 1 + src/_pytest/assertion/rewrite.py | 37 ++++++++++++++++++++------ testing/test_assertrewrite_coverage.py | 30 +++++++++++++++++++++ 3 files changed, 60 insertions(+), 8 deletions(-) create mode 100644 changelog/14814.bugfix.rst diff --git a/changelog/14814.bugfix.rst b/changelog/14814.bugfix.rst new file mode 100644 index 00000000000..6e44ab4976f --- /dev/null +++ b/changelog/14814.bugfix.rst @@ -0,0 +1 @@ +Fixed assertion rewriting evaluating a walrus operator (``:=``) or a starred argument out of order when a later argument assigned to the same name, so ``assert collect(*items, identity(items := [9]))`` now passes the pre-assignment ``items``. diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py index 27953336c5c..d9d9d0b1390 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/src/_pytest/assertion/rewrite.py @@ -967,19 +967,40 @@ def visit_operand( """Visit an operand, freezing it against walrus operators in *later*. Operands are rewritten into statements that run in source order, but - a plain name is left as a bare load evaluated at the very end, when - the enclosing expression is assembled. A walrus operator in a later - operand rebinds that name in between, so both the value used and the - value reported would be the post-walrus one -- Python evaluates the - earlier operand first. Copy the value into a temporary instead. + two of them stay unhoisted and are evaluated at the very end, when the + enclosing expression is assembled -- after everything that follows + them: + + * a plain name, which a walrus operator in a later operand rebinds in + between, so the value used and the value reported would be the + post-walrus one; + * a walrus operator itself, which would then assign in the wrong + order, and be visible to the operands that were meant to precede it. + + Either way Python evaluates the earlier operand first, so copy it into + a temporary here. A starred argument is unwrapped and rewrapped, its + value being subject to the same problem. """ specifiers = set(self.explanation_specifiers) res, expl = self.visit(operand) - if isinstance(res, ast.Name) and res.id in _walrus_targets(later): - snapshot = self.assign(res) + value = res.value if isinstance(res, ast.Starred) else res + if isinstance(value, ast.NamedExpr): + needs_freeze = bool(later) + else: + # Every other operand arrives as a temporary: the visit_* methods + # hoist what they build, and generic_visit assigns whatever is left + # -- a literal included -- so a name is all that can reach here. + assert isinstance(value, ast.Name) + needs_freeze = value.id in _walrus_targets(later) + if needs_freeze: + snapshot = self.assign(value) for key in set(self.explanation_specifiers) - specifiers: self.explanation_specifiers[key] = self.display(snapshot) - res = snapshot + res = ( + ast.copy_location(ast.Starred(snapshot, res.ctx), res) + if isinstance(res, ast.Starred) + else snapshot + ) return res, expl def visit_BoolOp(self, boolop: ast.BoolOp) -> tuple[ast.Name, str]: diff --git a/testing/test_assertrewrite_coverage.py b/testing/test_assertrewrite_coverage.py index 7517efb4210..827899b292d 100644 --- a/testing/test_assertrewrite_coverage.py +++ b/testing/test_assertrewrite_coverage.py @@ -916,6 +916,21 @@ def identity(v): return "passed", value """) + def test_starred_argument_precedes_walrus(self) -> None: + assert_evaluation_order(""" + def check(): + def identity(v): + return v + def collect(*values): + return values + items = [1] + try: + assert collect(*items, identity(items := [9])) == (1, [9]) + except AssertionError: + return "raised", items + return "passed", items + """) + def test_chained_compare_operands_in_order(self) -> None: assert_evaluation_order(""" def check(): @@ -929,6 +944,21 @@ def identity(v): return "passed", value """) + def test_bare_walrus_argument_in_order(self) -> None: + """A walrus argument is evaluated in place, before the ones after it.""" + assert_evaluation_order(""" + def check(): + def identity(v): + return v + def collect(*values): + return values + try: + assert collect((x := 1), identity(x := 2)) == (1, 2) + except AssertionError: + return "raised", x + return "passed", x + """) + def test_container_literal_operand_in_order(self) -> None: """Guard: ``generic_visit`` hoists container literals into a temporary.""" assert_evaluation_order(""" From 14cbbbf547435e7ae445c507c543e5fdda4a89a3 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Mon, 10 Aug 2026 09:11:18 +0200 Subject: [PATCH 11/11] refactor(rewrite): drop the walrus snapshot visit_Compare no longer needs 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) --- src/_pytest/assertion/rewrite.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py index d9d9d0b1390..7d5c620316a 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/src/_pytest/assertion/rewrite.py @@ -1126,8 +1126,6 @@ def visit_Compare(self, comp: ast.Compare) -> tuple[ast.expr, str]: left_res, left_expl = self.visit_operand(comp.left, comp.comparators) if isinstance(comp.left, ast.Compare | ast.BoolOp): left_expl = f"({left_expl})" - if isinstance(left_res, ast.NamedExpr): - left_res = self.assign(left_res) res_variables = [self.variable() for i in range(len(comp.ops))] load_names: list[ast.expr] = [ast.Name(v, ast.Load()) for v in res_variables] store_names = [ast.Name(v, ast.Store()) for v in res_variables]