From 792fa39660ef95eca9d34196d661f6fe6696c3d8 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Fri, 8 May 2026 10:37:47 +0200 Subject: [PATCH 01/19] 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 54bd8e911aa376e715ec297aba0948924f2f24f5 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Fri, 8 May 2026 12:59:20 +0200 Subject: [PATCH 02/19] 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 32829ddda4e7649d3a827958a85191e4ce3af88d Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Fri, 8 May 2026 13:04:00 +0200 Subject: [PATCH 03/19] 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 960a58cb5b4641d3d20ff7ad5ea220a9652bd426 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Fri, 8 May 2026 13:05:41 +0200 Subject: [PATCH 04/19] 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 298ca957094b940d6e0e91723a1d6442a0028bf0 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Fri, 8 May 2026 13:08:10 +0200 Subject: [PATCH 05/19] 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 c90c70fc5fd2dbad4bfa5cfcdecadf1044ec4026 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Wed, 3 Jun 2026 10:55:00 +0200 Subject: [PATCH 06/19] 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 9f9e73872f92dd71938670f3ca1f9b7bca7ec68a Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Wed, 3 Jun 2026 11:56:28 +0200 Subject: [PATCH 07/19] 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 72f4a7db9000c733f12bfaac4fe4ec6a2b957764 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Fri, 31 Jul 2026 19:52:36 +0200 Subject: [PATCH 08/19] 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 Co-Authored-By: Claude Opus 5 (1M context) --- 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 025318045c231e98d894b3d3f44502fdea706011 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Wed, 26 Aug 2026 11:32:02 +0200 Subject: [PATCH 09/19] test(rewrite): retire the walrus tests the matrix subsumes 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) --- testing/test_assertrewrite.py | 278 ++----------------------- testing/test_assertrewrite_coverage.py | 60 ++++++ 2 files changed, 80 insertions(+), 258 deletions(-) diff --git a/testing/test_assertrewrite.py b/testing/test_assertrewrite.py index b9464f5aec3..df40e1faa32 100644 --- a/testing/test_assertrewrite.py +++ b/testing/test_assertrewrite.py @@ -1637,190 +1637,29 @@ def test_simple_failure(): result.stdout.fnmatch_lines(["*E*assert (1 + 1) == 3"]) -class TestAssertionRewriteWalrusOperator: - """See #10743""" - - def test_assertion_walrus_operator(self, pytester: Pytester) -> None: - pytester.makepyfile( - """ - def my_func(before, after): - return before == after - - def change_value(value): - return value.lower() - - def test_walrus_conversion(): - a = "Hello" - assert not my_func(a, a := change_value(a)) - assert a == "hello" - """ - ) - result = pytester.runpytest() - assert result.ret == 0 - - def test_assertion_walrus_operator_dont_rewrite(self, pytester: Pytester) -> None: - pytester.makepyfile( - """ - 'PYTEST_DONT_REWRITE' - def my_func(before, after): - return before == after - - def change_value(value): - return value.lower() - - def test_walrus_conversion_dont_rewrite(): - a = "Hello" - assert not my_func(a, a := change_value(a)) - assert a == "hello" - """ - ) - result = pytester.runpytest() - assert result.ret == 0 - - def test_assertion_inline_walrus_operator(self, pytester: Pytester) -> None: - pytester.makepyfile( - """ - def my_func(before, after): - return before == after - - def test_walrus_conversion_inline(): - a = "Hello" - assert not my_func(a, a := a.lower()) - assert a == "hello" - """ - ) - result = pytester.runpytest() - assert result.ret == 0 - - def test_assertion_inline_walrus_operator_reverse(self, pytester: Pytester) -> None: - pytester.makepyfile( - """ - def my_func(before, after): - return before == after - - def test_walrus_conversion_reverse(): - a = "Hello" - assert my_func(a := a.lower(), a) - assert a == 'hello' - """ - ) - result = pytester.runpytest() - assert result.ret == 0 - - def test_assertion_walrus_no_variable_name_conflict( - self, pytester: Pytester - ) -> None: - pytester.makepyfile( - """ - def test_walrus_conversion_no_conflict(): - a = "Hello" - assert a == (b := a.lower()) - """ - ) - result = pytester.runpytest() - assert result.ret == 1 - result.stdout.fnmatch_lines(["*AssertionError: assert 'Hello' == 'hello'"]) - - def test_assertion_walrus_operator_true_assertion_and_changes_variable_value( - self, pytester: Pytester - ) -> None: - pytester.makepyfile( - """ - def test_walrus_conversion_succeed(): - a = "Hello" - assert a != (a := a.lower()) - assert a == 'hello' - """ - ) - result = pytester.runpytest() - assert result.ret == 0 - - def test_assertion_walrus_operator_fail_assertion(self, pytester: Pytester) -> None: - pytester.makepyfile( - """ - def test_walrus_conversion_fails(): - a = "Hello" - assert a == (a := a.lower()) - """ - ) - result = pytester.runpytest() - assert result.ret == 1 - result.stdout.fnmatch_lines(["*AssertionError: assert 'Hello' == 'hello'"]) - - def test_assertion_walrus_operator_boolean_composite( - self, pytester: Pytester - ) -> None: - pytester.makepyfile( - """ - def test_walrus_operator_change_boolean_value(): - a = True - assert a and True and ((a := False) is False) and (a is False) and ((a := None) is None) - assert a is None - """ - ) - result = pytester.runpytest() - assert result.ret == 0 - - def test_assertion_walrus_operator_compare_boolean_fails( - self, pytester: Pytester - ) -> None: - pytester.makepyfile( - """ - def test_walrus_operator_change_boolean_value(): - a = True - assert not (a and ((a := False) is False)) - """ - ) - result = pytester.runpytest() - assert result.ret == 1 - result.stdout.fnmatch_lines(["*assert not (True and False is False)"]) - - def test_assertion_walrus_operator_boolean_none_fails( - self, pytester: Pytester - ) -> None: - pytester.makepyfile( - """ - def test_walrus_operator_change_boolean_value(): - a = True - assert not (a and ((a := None) is None)) - """ - ) - result = pytester.runpytest() - assert result.ret == 1 - 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 - ) -> None: - pytester.makepyfile( - """ - def test_walrus_operator_change_value(): - a = True - assert (a := None) is None +def test_walrus_rebinding_does_not_outlive_its_statement( + pytester: Pytester, +) -> None: + """A walrus target must not be rebound by a later, unrelated assertion. - def test_walrus_operator_not_override_value(): - a = True - assert a is True + The rest of #10743's suite moved to the coverage matrix, which runs + in-process. This one stays here because it needs two tests in one module + to say anything: the rewriter may keep no state that survives a statement, + let alone a test. The matrix cannot express that. + """ + pytester.makepyfile( """ - ) - result = pytester.runpytest() - assert result.ret == 0 + def test_walrus_operator_change_value(): + a = True + assert (a := None) is None - def test_assertion_namedexpr_compare_left_overwrite( - self, pytester: Pytester - ) -> None: - pytester.makepyfile( - """ - def test_namedexpr_compare_left_overwrite(): - a = "Hello" - b = "World" - c = "Test" - assert (a := b) == c and (a := "Test") == "Test" - """ - ) - result = pytester.runpytest() - assert result.ret == 1 - result.stdout.fnmatch_lines(["*assert ('World' == 'Test'*"]) + def test_walrus_operator_not_override_value(): + a = True + assert a is True + """ + ) + result = pytester.runpytest() + assert result.ret == 0 class TestIssue11028: @@ -1973,83 +1812,6 @@ def test_walrus_running_counter(): 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 - - 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 - - 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 - - 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" diff --git a/testing/test_assertrewrite_coverage.py b/testing/test_assertrewrite_coverage.py index 7517efb4210..43a3f668f73 100644 --- a/testing/test_assertrewrite_coverage.py +++ b/testing/test_assertrewrite_coverage.py @@ -674,6 +674,41 @@ def check(): assert (y := x * 2) == 100 """) + def test_walrus_in_boolop_reports_each_operand(self) -> None: + """Two walrus assignments to one name: each operand shows what it saw.""" + assert_introspects( + """ + def check(): + def side_effect(): + return True + assert (x := side_effect()) and (x := False) + """, + must_contain=["assert (True and False)"], + ) + + def test_walrus_in_boolop_reports_assigned_value(self) -> None: + assert_introspects( + """ + def check(): + a = True + assert not (a and ((a := False) is False)) + """, + must_contain=["assert not (True and False is False)"], + ) + + def test_walrus_in_boolop_reports_left_operand(self) -> None: + """A comparator walrus must not overwrite the left operand's report.""" + assert_introspects( + """ + def check(): + a = "Hello" + b = "World" + c = "Test" + assert (a := b) == c and (a := "Test") == "Test" + """, + must_contain=["assert ('World' == 'Test'"], + ) + # --------------------------------------------------------------------------- # Single-evaluation tests: ensure no expression is evaluated multiple times @@ -903,6 +938,31 @@ def collect(*values): return "passed", value """) + def test_call_later_argument_sees_walrus_value(self) -> None: + """The mirror of the case above: a later operand sees the new value.""" + assert_evaluation_order(""" + def check(): + def collect(*values): + return values + value = "Hello" + try: + assert collect(value := value.lower(), value) == ("hello", "hello") + except AssertionError: + return "raised", value + return "passed", value + """) + + def test_boolop_chain_rebinds_in_order(self) -> None: + assert_evaluation_order(""" + def check(): + a = True + try: + assert a and True and ((a := False) is False) and (a is False) and ((a := None) is None) + except AssertionError: + return "raised", a + return "passed", a + """) + def test_binop_left_operand_precedes_walrus(self) -> None: assert_evaluation_order(""" def check(): From 5663cf5c1b5be31cab88700fcfbc9f08a5a83d50 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Fri, 31 Jul 2026 20:42:11 +0200 Subject: [PATCH 10/19] 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. Co-Authored-By: Claude Opus 5 (1M context) --- 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 43a3f668f73..6ac2cecb4fe 100644 --- a/testing/test_assertrewrite_coverage.py +++ b/testing/test_assertrewrite_coverage.py @@ -976,6 +976,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(): @@ -989,6 +1004,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 338cd6778fa26c0f656c33475c8cc5a410d9f642 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Mon, 10 Aug 2026 09:11:18 +0200 Subject: [PATCH 11/19] 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] From b45dc77b72eff1e499b1483322f5b7bb95c1ebc4 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Mon, 10 Aug 2026 09:14:46 +0200 Subject: [PATCH 12/19] refactor(rewrite): drop the unreachable Load guard in visit_Attribute The rewriter only ever visits expressions inside an assert condition, so an attribute always arrives in Load context and the fallback never runs. Removing it keeps the next visitor from copying a guard that cannot fire. Co-Authored-By: Claude Opus 5 (1M context) --- 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 7d5c620316a..94a82bf9e87 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/src/_pytest/assertion/rewrite.py @@ -1110,8 +1110,6 @@ def visit_Starred(self, starred: ast.Starred) -> tuple[ast.Starred, str]: return new_starred, "*" + expl def visit_Attribute(self, attr: ast.Attribute) -> tuple[ast.Name, str]: - if not isinstance(attr.ctx, ast.Load): - return self.generic_visit(attr) value, value_expl = self.visit(attr.value) res = self.assign( ast.copy_location(ast.Attribute(value, attr.attr, ast.Load()), attr) From b3d36caa08ea1309877c948d0193d957efc6fe50 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Fri, 31 Jul 2026 20:43:29 +0200 Subject: [PATCH 13/19] feat(rewrite): introspect container[key] in failure messages 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. Co-Authored-By: Claude Opus 5 (1M context) --- changelog/14815.improvement.rst | 4 ++++ src/_pytest/assertion/rewrite.py | 15 ++++++++++++ testing/test_assertrewrite_coverage.py | 32 ++++++++++++++++++++++++++ 3 files changed, 51 insertions(+) create mode 100644 changelog/14815.improvement.rst diff --git a/changelog/14815.improvement.rst b/changelog/14815.improvement.rst new file mode 100644 index 00000000000..0e316edea07 --- /dev/null +++ b/changelog/14815.improvement.rst @@ -0,0 +1,4 @@ +Assertion failure messages now decompose subscript expressions, showing the container and the key that produced a value:: + + assert 1 == 99 + + where 1 = {'a': 1, 'b': 2}['a'] diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py index 94a82bf9e87..a09714a2fc2 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/src/_pytest/assertion/rewrite.py @@ -1109,6 +1109,21 @@ def visit_Starred(self, starred: ast.Starred) -> tuple[ast.Starred, str]: new_starred = ast.Starred(res, starred.ctx) return new_starred, "*" + expl + def visit_Subscript(self, subscript: ast.Subscript) -> tuple[ast.Name, str]: + # For Slice objects (a[1:3]), fall back to generic — decomposing + # start/stop/step is rarely useful in assertion messages. + if isinstance(subscript.slice, ast.Slice): + return self.generic_visit(subscript) + value, value_expl = self.visit_operand(subscript.value, [subscript.slice]) + slice_res, slice_expl = self.visit(subscript.slice) + res = self.assign( + ast.copy_location(ast.Subscript(value, slice_res, ast.Load()), subscript) + ) + res_expl = self.explanation_param(self.display(res)) + pat = "%s\n{%s = %s[%s]\n}" + expl = pat % (res_expl, res_expl, value_expl, slice_expl) + return res, expl + def visit_Attribute(self, attr: ast.Attribute) -> tuple[ast.Name, str]: value, value_expl = self.visit(attr.value) res = self.assign( diff --git a/testing/test_assertrewrite_coverage.py b/testing/test_assertrewrite_coverage.py index 6ac2cecb4fe..ce5ae6e7e17 100644 --- a/testing/test_assertrewrite_coverage.py +++ b/testing/test_assertrewrite_coverage.py @@ -535,6 +535,26 @@ def check(): class TestIntrospectionSubscript: """Subscript / indexing.""" + def test_dict_subscript_shows_key_and_container(self) -> None: + assert_introspects( + """ + def check(): + d = {"a": 1, "b": 2} + assert d["a"] == 99 + """, + must_contain=["where 1 = ", "['a']"], + ) + + def test_list_subscript_shows_index_and_container(self) -> None: + assert_introspects( + """ + def check(): + items = [10, 20, 30] + assert items[1] == 99 + """, + must_contain=["where 20 = ", "[1]"], + ) + def test_subscript_semantics_preserved(self) -> None: assert_semantically_equivalent(""" def check(): @@ -1150,6 +1170,18 @@ def identity(v): class TestEdgeCases: """Regression and edge-case tests combining multiple expression types.""" + def test_subscript_with_variable_key(self) -> None: + """Subscript where the key is a variable (not constant).""" + assert_introspects( + """ + def check(): + d = {"hello": 42} + key = "hello" + assert d[key] == 100 + """, + must_contain=["where 42 = ", "['hello']"], + ) + def test_subscript_with_call_key(self) -> None: """Subscript where the key is a function call.""" assert_introspects( From c812ceee0af01fbc9022d36c418370ac59e00c50 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Fri, 31 Jul 2026 20:44:22 +0200 Subject: [PATCH 14/19] feat(rewrite): introspect the condition of a ternary 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 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. Co-Authored-By: Claude Opus 5 (1M context) --- changelog/14816.improvement.rst | 4 ++++ src/_pytest/assertion/rewrite.py | 14 +++++++++++ testing/test_assertrewrite_coverage.py | 32 ++++++++++++++++++++++++++ 3 files changed, 50 insertions(+) create mode 100644 changelog/14816.improvement.rst diff --git a/changelog/14816.improvement.rst b/changelog/14816.improvement.rst new file mode 100644 index 00000000000..ca3ba124728 --- /dev/null +++ b/changelog/14816.improvement.rst @@ -0,0 +1,4 @@ +Assertion failure messages now show the condition of a conditional expression:: + + assert 0 == 99 + + where 0 = (... if True else ...) diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py index a09714a2fc2..27d9285aa82 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/src/_pytest/assertion/rewrite.py @@ -1109,6 +1109,20 @@ def visit_Starred(self, starred: ast.Starred) -> tuple[ast.Starred, str]: new_starred = ast.Starred(res, starred.ctx) return new_starred, "*" + expl + def visit_IfExp(self, ifexp: ast.IfExp) -> tuple[ast.Name, str]: + # Introspect the condition but keep the branches as they are: only the + # selected one may be evaluated, so neither can be hoisted. That also + # keeps them ordered after the condition, which is where Python puts + # them, so no freeze is needed here. + cond_res, cond_expl = self.visit(ifexp.test) + res = self.assign( + ast.copy_location(ast.IfExp(cond_res, ifexp.body, ifexp.orelse), ifexp) + ) + res_expl = self.explanation_param(self.display(res)) + pat = "%s\n{%s = (... if %s else ...)\n}" + expl = pat % (res_expl, res_expl, cond_expl) + return res, expl + def visit_Subscript(self, subscript: ast.Subscript) -> tuple[ast.Name, str]: # For Slice objects (a[1:3]), fall back to generic — decomposing # start/stop/step is rarely useful in assertion messages. diff --git a/testing/test_assertrewrite_coverage.py b/testing/test_assertrewrite_coverage.py index ce5ae6e7e17..8af2a741c23 100644 --- a/testing/test_assertrewrite_coverage.py +++ b/testing/test_assertrewrite_coverage.py @@ -577,6 +577,16 @@ def check(): class TestIntrospectionIfExp: """Ternary / if-expression.""" + def test_ifexp_shows_condition_value(self) -> None: + assert_introspects( + """ + def check(): + flag = True + assert (0 if flag else 1) == 1 + """, + must_contain=["if True else"], + ) + def test_ifexp_semantics_preserved(self) -> None: assert_semantically_equivalent(""" def check(): @@ -584,6 +594,16 @@ def check(): assert (0 if flag else 1) == 1 """) + def test_ifexp_in_compare_shows_result(self) -> None: + assert_introspects( + """ + def check(): + flag = True + assert (0 if flag else 1) == 99 + """, + must_contain=["assert 0 == 99", "if True else"], + ) + def test_ifexp_short_circuit_true(self) -> None: """Orelse branch must NOT be evaluated when condition is True.""" assert_passes_when_true(""" @@ -1222,6 +1242,18 @@ def __repr__(self): must_contain=["42", "100"], ) + def test_ifexp_with_call_condition(self) -> None: + """IfExp where condition is a function call.""" + assert_introspects( + """ + def check(): + def is_ready(): + return False + assert (1 if is_ready() else 0) == 1 + """, + must_contain=["if False else"], + ) + def test_walrus_in_subscript(self) -> None: """Walrus operator used as subscript key.""" assert_semantically_equivalent(""" From 3403e04bd88573f5edd760ab1529924569144204 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Fri, 31 Jul 2026 20:47:16 +0200 Subject: [PATCH 15/19] feat(rewrite): show a method call on one line obj.method() reported the bound method as an intermediate of its own: where 42 = compute() where compute = Obj().compute which spends a line on something nobody asked about. Build the explanation from the receiver and the attribute name instead: where 42 = Obj().compute() The bound method keeps its own temporary even though it no longer has its own explanation, because Python looks it up before evaluating the arguments -- inlining the attribute into the rewritten call would move the lookup after them, and with it the read of the receiver. Both order-axis guards in the coverage matrix fail if that temporary goes. Closes the introspect-method-call-flat group in the coverage matrix. Co-Authored-By: Claude Opus 5 (1M context) --- changelog/14817.improvement.rst | 4 + src/_pytest/assertion/rewrite.py | 16 +++- testing/python/raises_group.py | 5 +- testing/test_assertrewrite_coverage.py | 101 ++++++++++++++++++++++++- 4 files changed, 121 insertions(+), 5 deletions(-) create mode 100644 changelog/14817.improvement.rst diff --git a/changelog/14817.improvement.rst b/changelog/14817.improvement.rst new file mode 100644 index 00000000000..da6d5d47bf0 --- /dev/null +++ b/changelog/14817.improvement.rst @@ -0,0 +1,4 @@ +Assertion failure messages now show a method call on a single line, without the bound method as a separate intermediate:: + + assert 42 == 100 + + where 42 = Obj().compute() diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py index 27d9285aa82..a0e203debcc 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/src/_pytest/assertion/rewrite.py @@ -1080,7 +1080,21 @@ def visit_Call(self, call: ast.Call) -> tuple[ast.Name, str]: # 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) + if isinstance(call.func, ast.Attribute) and isinstance(call.func.ctx, ast.Load): + # obj.method(...) reads better flat -- "where 42 = Obj().compute()" + # rather than a separate "where compute = Obj().compute" line. The + # bound method still gets a temporary of its own, because Python + # looks it up before evaluating the arguments; that is also what + # keeps the receiver ordered ahead of them. + receiver, receiver_expl = self.visit(call.func.value) + new_func: ast.expr = self.assign( + ast.copy_location( + ast.Attribute(receiver, call.func.attr, ast.Load()), call.func + ) + ) + func_expl = f"{receiver_expl}.{call.func.attr}" + else: + new_func, func_expl = self.visit_operand(call.func, operands) arg_expls = [] new_args = [] new_kwargs = [] diff --git a/testing/python/raises_group.py b/testing/python/raises_group.py index 950e71753c2..4a53540267f 100644 --- a/testing/python/raises_group.py +++ b/testing/python/raises_group.py @@ -1237,11 +1237,10 @@ def test_assert_matches() -> None: match=wrap_escape( "`ValueError()` is not an instance of `TypeError`\n" "assert False\n" - " + where False = matches(ValueError())\n" - " + where matches = RaisesExc(TypeError).matches" + " + where False = RaisesExc(TypeError).matches(ValueError())" ), ): - # you'd need to do this arcane incantation + # binding the RaisesExc is still how you get at ``fail_reason`` assert (m := RaisesExc(TypeError)).matches(e), m.fail_reason # but even if we add assert_matches, will people remember to use it? diff --git a/testing/test_assertrewrite_coverage.py b/testing/test_assertrewrite_coverage.py index 8af2a741c23..e5f011c4c93 100644 --- a/testing/test_assertrewrite_coverage.py +++ b/testing/test_assertrewrite_coverage.py @@ -678,7 +678,41 @@ def check(): class TestIntrospectionMethodCall: - """Method calls — currently show the bound method as its own "where" line.""" + """Method calls — flat obj.method() display without bound-method noise.""" + + def test_method_call_flat_format(self) -> None: + """Method calls show 'where result = obj.method()' in one line.""" + assert_introspects( + """ + def check(): + class Obj: + def compute(self): + return 42 + def __repr__(self): + return "Obj()" + obj = Obj() + assert obj.compute() == 100 + """, + must_contain=["where 42 = Obj().compute()"], + ) + + def test_method_call_no_bound_method_noise(self) -> None: + """No separate 'where compute = obj.compute' line.""" + msg = get_failure_message(""" + def check(): + class Obj: + def compute(self): + return 42 + def __repr__(self): + return "Obj()" + obj = Obj() + assert obj.compute() == 100 + """) + lines = msg.splitlines() + for line in lines: + assert "where compute = " not in line, ( + f"Noisy bound-method intermediate found:\n{msg}" + ) def test_callable_variable_shows_result(self) -> None: # Current behavior: shows full function repr, not variable name @@ -1167,6 +1201,24 @@ def collect(**kwargs): return "passed", mapping """) + def test_method_lookup_precedes_arguments(self) -> None: + """Guard: the bound method is looked up before the arguments run.""" + assert_evaluation_order(""" + def check(): + trace = [] + class Box: + @property + def take(self): + trace.append("lookup") + return lambda value: value + obj = Box() + try: + assert obj.take(trace.append("argument")) is None + except AssertionError: + return "raised", trace + return "passed", trace + """) + def test_ifexp_branches_in_order(self) -> None: """Guard: the condition is evaluated before the selected branch.""" assert_evaluation_order(""" @@ -1226,6 +1278,42 @@ def check(): must_contain=["42", "100"], ) + def test_method_call_with_args(self) -> None: + """Method call with arguments shows flat format.""" + assert_introspects( + """ + def check(): + class Calculator: + def add(self, a, b): + return a + b + def __repr__(self): + return "Calc()" + c = Calculator() + assert c.add(2, 3) == 10 + """, + must_contain=["where 5 = Calc().add(2, 3)"], + ) + + def test_chained_method_calls(self) -> None: + """Chained method call: obj.method1().method2().""" + assert_introspects( + """ + def check(): + class Builder: + def __init__(self, val=0): + self.val = val + def add(self, n): + return Builder(self.val + n) + def result(self): + return self.val + def __repr__(self): + return f"Builder({self.val})" + b = Builder() + assert b.add(5).result() == 100 + """, + must_contain=["where 5 = ", ".result()"], + ) + def test_subscript_on_method_result(self) -> None: """Subscript on method return value: obj.method()[key].""" assert_introspects( @@ -1323,3 +1411,14 @@ def check(): assert d["key"] == 100, "custom failure message" """) assert "custom failure message" in msg + + def test_method_call_on_global(self) -> None: + """Method call on a global/module-level object.""" + assert_introspects( + """ + items = [1, 2, 3] + def check(): + assert items.count(99) == 1 + """, + must_contain=["where 0 = [1, 2, 3].count(99)"], + ) From fe23ee235ac9826c75b3139c8dbb935eba896a8c Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Fri, 31 Jul 2026 22:15:50 +0200 Subject: [PATCH 16/19] fix(rewrite): freeze operands against any later side effect (#14820) visit_operand() froze a name only when a walrus operator in a later operand targeted it. A call can rebind just as well, through global or nonlocal, and then the name -- still unhoisted, still read when the enclosing expression is assembled -- sees the new binding: count = 0 def bump(): global count count = 99 return 0 assert count == bump() # Python compares 0 == 0 and passes There is no way to tell from the assert which names a call might rebind, so _walrus_targets() becomes _can_rebind(): a name is frozen whenever anything that follows it can execute code at all. That sounds expensive and is not. An operand that was already hoisted needs no freeze, so `assert len(items) == expected` rewrites unchanged; only the bare-name-then-call shape gains one assignment. visit_BoolOp used the same pre-scan and is generalized with it, which leaves one rule in one place instead of two spellings of half of it. Closes the order-name-rebound-by-call group in the coverage matrix. Co-Authored-By: Claude Opus 5 (1M context) --- changelog/14821.bugfix.rst | 1 + src/_pytest/assertion/rewrite.py | 55 +++++++++++++++----------- testing/test_assertrewrite_coverage.py | 52 ++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 24 deletions(-) create mode 100644 changelog/14821.bugfix.rst diff --git a/changelog/14821.bugfix.rst b/changelog/14821.bugfix.rst new file mode 100644 index 00000000000..99539e26692 --- /dev/null +++ b/changelog/14821.bugfix.rst @@ -0,0 +1 @@ +Fixed assertion rewriting reading an operand after a later call had rebound its name, so ``assert count == bump()`` compares the ``count`` Python would have compared even when ``bump()`` assigns to it via ``global`` or ``nonlocal``. diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py index a0e203debcc..da58d4dc322 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/src/_pytest/assertion/rewrite.py @@ -530,14 +530,19 @@ 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 +def _can_rebind(nodes: Iterable[ast.expr]) -> bool: + """Return whether evaluating *nodes* could rebind a name read before them. + + A walrus operator rebinds its target outright. A call -- including the + implicit one behind ``await`` -- can rebind anything it declares ``global`` + or ``nonlocal``, and there is no way to tell from here which names those + are, so assume the worst. + """ + return any( + isinstance(sub, ast.NamedExpr | ast.Call | ast.Await) for node in nodes for sub in ast.walk(node) - if isinstance(sub, ast.NamedExpr) - } + ) @functools.lru_cache(maxsize=1) @@ -964,34 +969,32 @@ def visit_Name(self, name: ast.Name) -> tuple[ast.Name, str]: 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*. + """Visit an operand, freezing it against side effects in *later*. Operands are rewritten into statements that run in source order, but 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 plain name, whose binding anything in *later* may have changed by + then, so the value used and the value reported would be the new 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. + + Only a name or a walrus can reach the check below: the visit_* methods + hoist what they build into a temporary and generic_visit assigns + whatever is left -- a literal included -- so anything else is already + ordered by the time it arrives here. """ specifiers = set(self.explanation_specifiers) res, expl = self.visit(operand) 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) + assert isinstance(value, ast.NamedExpr | ast.Name) + needs_freeze = _can_rebind(later) if needs_freeze: snapshot = self.assign(value) for key in set(self.explanation_specifiers) - specifiers: @@ -1011,9 +1014,8 @@ 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 - later_walrus_targets = [ - _walrus_targets(boolop.values[idx + 1 :]) - for idx in range(len(boolop.values)) + later_can_rebind = [ + _can_rebind(boolop.values[idx + 1 :]) for idx in range(len(boolop.values)) ] self.push_format_context() # Process each operand, short-circuiting as needed. @@ -1030,10 +1032,10 @@ def visit_BoolOp(self, boolop: ast.BoolOp) -> tuple[ast.Name, str]: # 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. + # - Name whose binding a later operand may change: the + # explanation would show the value it ended up with. needs_snapshot = (isinstance(v, ast.NamedExpr) and i < levels) or ( - isinstance(v, ast.Name) and v.id in later_walrus_targets[i] + isinstance(v, ast.Name) and later_can_rebind[i] ) if needs_snapshot: snapshot = self.assign(ast.Name(res_var, ast.Load())) @@ -1167,6 +1169,11 @@ 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): + # visit_operand only freezes an operand something later can rebind, + # so a walrus with nothing but literals after it arrives raw -- and + # the comparison below would evaluate it a second time. + 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] diff --git a/testing/test_assertrewrite_coverage.py b/testing/test_assertrewrite_coverage.py index e5f011c4c93..837bf9a8a3c 100644 --- a/testing/test_assertrewrite_coverage.py +++ b/testing/test_assertrewrite_coverage.py @@ -1201,6 +1201,58 @@ def collect(**kwargs): return "passed", mapping """) + def test_global_rebound_by_call_precedes_compare(self) -> None: + assert_evaluation_order(""" + count = 0 + + def bump(): + global count + count = 99 + return 0 + + def check(): + try: + assert count == bump() + except AssertionError: + return "raised", count + return "passed", count + """) + + def test_nonlocal_rebound_by_call_precedes_compare(self) -> None: + assert_evaluation_order(""" + def check(): + value = 1 + def bump(): + nonlocal value + value = 99 + return 1 + try: + assert value == bump() + except AssertionError: + return "raised", value + return "passed", value + """) + + def test_global_rebound_by_call_precedes_argument(self) -> None: + assert_evaluation_order(""" + value = "a" + + def bump(): + global value + value = "b" + return "x" + + def collect(*args): + return args + + def check(): + try: + assert collect(value, bump()) == ("a", "x") + except AssertionError: + return "raised", value + return "passed", value + """) + def test_method_lookup_precedes_arguments(self) -> None: """Guard: the bound method is looked up before the arguments run.""" assert_evaluation_order(""" From d3d31515ba5937a8b7f53a7558db86d298951e14 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Fri, 31 Jul 2026 22:18:15 +0200 Subject: [PATCH 17/19] fix(rewrite): short-circuit chained comparisons (#14819) Python evaluates a comparison chain lazily -- in `a < b < c`, c is never evaluated when a < b is false. visit_Compare walked the comparators in a loop and only combined the results with `and` afterwards, by which time everything had already run: assert 1 < 0 < 1 / 0 # ZeroDivisionError, not AssertionError Each link past the first now goes inside an `if` on the link before it, the same shape visit_BoolOp has used since #57 was fixed for and/or. The failure path builds a tuple of every link's result and every operand, so the temporaries belonging to links that never ran are set to None ahead of the chain rather than left unbound. None is falsey, which is also what _call_reprcompare wants: it stops at the first falsey result, and that is the link that actually failed. Closes the order-chained-compare-lazy group in the coverage matrix. Co-Authored-By: Claude Opus 5 (1M context) --- changelog/14822.bugfix.rst | 1 + src/_pytest/assertion/rewrite.py | 26 ++++++++++++++++++++++++++ testing/test_assertrewrite_coverage.py | 24 ++++++++++++++++++++++++ 3 files changed, 51 insertions(+) create mode 100644 changelog/14822.bugfix.rst diff --git a/changelog/14822.bugfix.rst b/changelog/14822.bugfix.rst new file mode 100644 index 00000000000..490cb86670e --- /dev/null +++ b/changelog/14822.bugfix.rst @@ -0,0 +1 @@ +Chained comparisons in an ``assert`` now short-circuit the way Python does: in ``assert a < b < c``, ``c`` is no longer evaluated when ``a < b`` is false. Previously ``assert 1 < 0 < 1 / 0`` raised ``ZeroDivisionError`` instead of ``AssertionError``, and a call in an unreached position ran anyway. diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py index da58d4dc322..2c0bb7248a9 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/src/_pytest/assertion/rewrite.py @@ -1181,7 +1181,22 @@ def visit_Compare(self, comp: ast.Compare) -> tuple[ast.expr, str]: expls: list[ast.expr] = [] syms: list[ast.expr] = [] results = [left_res] + # A chain short-circuits: ``a < b < c`` leaves c unevaluated when a < b + # is false. Everything past the first link therefore goes inside an + # ``if`` on the link before it, and the temporaries it would have + # produced are set to None up front -- the failure path builds a tuple + # of all of them, and the ones that never ran must still be readable. + # None is also falsey, so _call_reprcompare still stops at the link + # that actually failed. + body = self.statements + deferred_at = deferred_from = None for i, op, next_operand in it: + if i: + if deferred_at is None: + deferred_at, deferred_from = len(body), len(self.variables) + inner: list[ast.stmt] = [] + self.statements.append(ast.If(load_names[i - 1], inner, [])) + self.statements = inner next_res, next_expl = self.visit_operand( next_operand, comp.comparators[i + 1 :] ) @@ -1197,6 +1212,17 @@ 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 + self.statements = body + if deferred_at is not None: + assert deferred_from is not None + deferred = [*res_variables[1:], *self.variables[deferred_from:]] + body.insert( + deferred_at, + ast.Assign( + [ast.Name(name, ast.Store()) for name in deferred], + ast.Constant(None), + ), + ) # Use pytest.assertion.util._reprcompare if that's available. expl_call = self.helper( "_call_reprcompare", diff --git a/testing/test_assertrewrite_coverage.py b/testing/test_assertrewrite_coverage.py index 837bf9a8a3c..7972e969018 100644 --- a/testing/test_assertrewrite_coverage.py +++ b/testing/test_assertrewrite_coverage.py @@ -1253,6 +1253,30 @@ def check(): return "passed", value """) + def test_chained_compare_stops_at_the_first_false(self) -> None: + assert_evaluation_order(""" + def check(): + trace = [] + def rec(label, value): + trace.append(label) + return value + try: + assert rec("a", 1) < rec("b", 0) < rec("c", 5) + except AssertionError: + return "raised", trace + return "passed", trace + """) + + def test_chained_compare_unreached_operand_does_not_raise(self) -> None: + assert_evaluation_order(""" + def check(): + try: + assert 1 < 0 < 1 / 0 + except AssertionError: + return "raised", None + return "passed", None + """) + def test_method_lookup_precedes_arguments(self) -> None: """Guard: the bound method is looked up before the arguments run.""" assert_evaluation_order(""" From 02d56fa3d1164930f1b1fda98ce8f4eaa2d5bb9c Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Wed, 26 Aug 2026 11:46:17 +0200 Subject: [PATCH 18/19] fix(rewrite): guard a chained comparison's explanation too Short-circuiting the chain nested the statements that evaluate a link past the first, but not the statements that explain it, so the failure branch ran a skipped link's explanation against temporaries the link never assigned: assert 1 < 0 < (a or b) raised ``AttributeError: 'NoneType' object has no attribute 'append'`` instead of failing. visit_BoolOp builds its explanation by creating a list in the main body and appending to it from expl_stmts; nesting only the body left the list None while the appends ran unconditionally. SirHegel found this against the branch and named the fix -- nest expl_stmts on the same condition, the way visit_BoolOp nests both -- in https://github.com/pytest-dev/pytest/pull/14822#issuecomment-5360778260, having reduced it from his own #14918. What follows is his idea; two details are worth recording. Most links explain themselves in the format context alone and contribute no statements, and an ``if`` with an empty body is not valid syntax, so the guards are attached innermost first and the empty ones dropped -- attaching a child fills its parent, so a parent is only known to be empty once its child has been placed. The names to pre-bind now include the @py_format ones created inside those guarded blocks, which the outer format context reads. Collecting them by walking the blocks is what makes them reachable at all, but the walk must not take everything it finds: a walrus target inside a skipped link belongs to the user, and Python leaves it unbound. Binding it to None to keep the explanation readable would be visible after the assertion, so _rewriter_temporaries() takes only names the rewriter itself makes. That last point is a second failure mode, which the report did not cover: the explanation of a walrus reads its target to decide how to show it, and a skipped link never bound it. assert 1 < 0 < (w := 1) # UnboundLocalError: 'w' assert 1 < 0 < identity(w := 1) # likewise Nesting does not reach it -- the read sits in the compare's own format dict, which is built eagerly and belongs to no link -- and ``'w' in locals()`` does not guard it, because the fallback hands the value to _should_repr_global_name(). visit_NamedExpr now asks whether the target is a global before reading it, and shows the bare name when it is neither. An undefined name inside a skipped operand failed the same way with NameError, and is fixed by the nesting. Co-Authored-By: Claude Opus 5 (1M context) --- src/_pytest/assertion/rewrite.py | 81 ++++++++++++++++++++++---- testing/test_assertrewrite_coverage.py | 67 +++++++++++++++++++++ 2 files changed, 137 insertions(+), 11 deletions(-) diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py index 2c0bb7248a9..c8f472efbb2 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/src/_pytest/assertion/rewrite.py @@ -545,6 +545,25 @@ def _can_rebind(nodes: Iterable[ast.expr]) -> bool: ) +def _rewriter_temporaries(nodes: Iterable[ast.AST]) -> list[str]: + """Return the names the rewriter binds inside *nodes*, in creation order. + + Only its own: a walrus target inside a conditional block belongs to the + user, and Python leaves it unbound when the block does not run. Binding it + to None to make it readable would be visible after the assertion. + """ + names: dict[str, None] = {} + for node in nodes: + for sub in ast.walk(node): + if ( + isinstance(sub, ast.Name) + and isinstance(sub.ctx, ast.Store) + and sub.id.startswith("@py") + ): + names[sub.id] = None + return list(names) + + @functools.lru_cache(maxsize=1) def _get_assertion_exprs(src: bytes) -> dict[int, str]: """Return a mapping from {lineno: "assertion test expression"}.""" @@ -951,7 +970,20 @@ def visit_NamedExpr(self, name: ast.NamedExpr) -> tuple[ast.NamedExpr, str]: 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", target_name) + # Unlike visit_Name, the target need not be bound when this runs: the + # walrus may sit in a branch that short-circuited away, and then the + # explanation is formatted for an assignment that never happened. + # Reading it to decide how to show it would raise instead -- so ask + # whether it exists as a global before passing it to a helper. + inglobals = ast.Compare( + ast.Constant(target_id), + [ast.In()], + [ast.Call(self.builtin("globals"), [], [])], + ) + dorepr = ast.BoolOp( + ast.And(), + [inglobals, self.helper("_should_repr_global_name", target_name)], + ) test = ast.BoolOp(ast.Or(), [inlocs, dorepr]) expr = ast.IfExp(test, self.display(target_name), ast.Constant(target_id)) return name, self.explanation_param(expr) @@ -1183,20 +1215,32 @@ def visit_Compare(self, comp: ast.Compare) -> tuple[ast.expr, str]: results = [left_res] # A chain short-circuits: ``a < b < c`` leaves c unevaluated when a < b # is false. Everything past the first link therefore goes inside an - # ``if`` on the link before it, and the temporaries it would have - # produced are set to None up front -- the failure path builds a tuple - # of all of them, and the ones that never ran must still be readable. - # None is also falsey, so _call_reprcompare still stops at the link - # that actually failed. + # ``if`` on the link before it -- both what evaluates the link and what + # explains it, because the explanation reads what the evaluation bound. + # The temporaries a skipped link would have produced are set to None up + # front, so the failure path can still read them: it builds a tuple of + # every link's result, and formats every link's explanation. None is + # also falsey, so _call_reprcompare still stops at the link that + # actually failed, and the entries behind it are never rendered. body = self.statements - deferred_at = deferred_from = None + fail_save = self.expl_stmts + deferred_at = None + deferred_stmt_ifs: list[ast.If] = [] + deferred_expl_ifs: list[tuple[list[ast.stmt], ast.If]] = [] for i, op, next_operand in it: if i: if deferred_at is None: - deferred_at, deferred_from = len(body), len(self.variables) + deferred_at = len(body) inner: list[ast.stmt] = [] - self.statements.append(ast.If(load_names[i - 1], inner, [])) + stmt_if = ast.If(load_names[i - 1], inner, []) + deferred_stmt_ifs.append(stmt_if) + self.statements.append(stmt_if) self.statements = inner + fail_inner: list[ast.stmt] = [] + deferred_expl_ifs.append( + (self.expl_stmts, ast.If(load_names[i - 1], fail_inner, [])) + ) + self.expl_stmts = fail_inner next_res, next_expl = self.visit_operand( next_operand, comp.comparators[i + 1 :] ) @@ -1213,9 +1257,24 @@ def visit_Compare(self, comp: ast.Compare) -> tuple[ast.expr, str]: self.statements.append(ast.Assign([store_names[i]], res_expr)) left_res, left_expl = next_res, next_expl self.statements = body + self.expl_stmts = fail_save + # Attach the explanation guards innermost first, dropping the ones that + # stayed empty -- most links explain themselves in the format context + # alone and contribute no statements, and an ``if`` with an empty body + # is not valid syntax. Attaching a child fills its parent, so the + # parent is only known to be empty once the child has been placed. + for parent, fail_if in reversed(deferred_expl_ifs): + if fail_if.body: + parent.append(fail_if) if deferred_at is not None: - assert deferred_from is not None - deferred = [*res_variables[1:], *self.variables[deferred_from:]] + deferred = dict.fromkeys( + [ + *res_variables[1:], + *_rewriter_temporaries( + [*deferred_stmt_ifs, *(f for _, f in deferred_expl_ifs)] + ), + ] + ) body.insert( deferred_at, ast.Assign( diff --git a/testing/test_assertrewrite_coverage.py b/testing/test_assertrewrite_coverage.py index 7972e969018..bf220082845 100644 --- a/testing/test_assertrewrite_coverage.py +++ b/testing/test_assertrewrite_coverage.py @@ -1277,6 +1277,73 @@ def check(): return "passed", None """) + def test_chained_compare_skipped_boolop_operand(self) -> None: + """A boolop in a skipped link builds its explanation in that link.""" + assert_evaluation_order(""" + def check(): + a = b = 0 + try: + assert 1 < 0 < (a or b) + except AssertionError: + return "raised", None + return "passed", None + """) + + def test_chained_compare_skipped_operand_reads_no_name(self) -> None: + """Nor may it read the names that operand would have read.""" + assert_evaluation_order(""" + def check(): + try: + assert 1 < 0 < (missing or 0) + except AssertionError: + return "raised", None + return "passed", None + """) + + def test_chained_compare_skipped_walrus_stays_unbound(self) -> None: + """A skipped walrus assigns nothing -- the explanation may not either.""" + assert_evaluation_order(""" + def check(): + try: + assert 1 < 0 < (w := 1) + except AssertionError: + return "raised", "w" in locals() + return "passed", "w" in locals() + """) + + def test_chained_compare_skipped_walrus_inside_call(self) -> None: + assert_evaluation_order(""" + def check(): + def identity(v): + return v + try: + assert 1 < 0 < identity(w := 1) + except AssertionError: + return "raised", "w" in locals() + return "passed", "w" in locals() + """) + + def test_chained_compare_reports_the_link_that_failed(self) -> None: + """The skipped link contributes nothing to the message either.""" + assert_introspects( + """ + def check(): + a = b = 0 + assert 1 < 0 < (a or b) + """, + must_contain=["assert 1 < 0"], + must_not_contain=["None"], + ) + + def test_chained_compare_reports_a_taken_walrus_link(self) -> None: + assert_introspects( + """ + def check(): + assert 0 < 1 < (w := 5) < 2 + """, + must_contain=["5"], + ) + def test_method_lookup_precedes_arguments(self) -> None: """Guard: the bound method is looked up before the arguments run.""" assert_evaluation_order(""" From d9cdf61b5e6a3df2cb35b8d5ad5f664dd1bd518a Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Thu, 20 Aug 2026 06:23:20 +0200 Subject: [PATCH 19/19] test(rewrite): record the introspection gaps nothing fixes yet Two expression types still show a value the rewriter never decomposes: a list/dict/set literal, and a name that happens to hold a callable. Neither has a fix in flight, so they stay strict xfails tagged with a group name; the tests state the message we would want instead. Kept out of the coverage matrix itself so that PR carries only tests that pass, and off the fix branches so each of those lands its own group green. Co-Authored-By: Claude Opus 5 (1M context) --- testing/test_assertrewrite_coverage.py | 56 ++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/testing/test_assertrewrite_coverage.py b/testing/test_assertrewrite_coverage.py index bf220082845..ab6343ac895 100644 --- a/testing/test_assertrewrite_coverage.py +++ b/testing/test_assertrewrite_coverage.py @@ -7,6 +7,12 @@ 2. Semantic correctness: rewritten code has identical behavior to original 3. Single evaluation: side-effecting expressions are not evaluated multiple times 4. Evaluation order: operands see the values Python would give them + +Two known gaps remain, recorded as strict xfails whose reason starts with a +group name, so that a change can state which group it closes:: + + introspect-container-literal list/dict/set literals are not decomposed + introspect-callable-variable a called variable shows """ from __future__ import annotations @@ -486,6 +492,23 @@ def f(x): must_contain=["where 6 = ", "(3)"], ) + @pytest.mark.xfail( + strict=True, + reason="introspect-callable-variable: a called name shows its repr", + ) + def test_simple_call_clean_name(self) -> None: + """Ideally the message should show 'f()' not '()'.""" + assert_introspects( + """ + def check(): + def f(): + return 42 + assert f() == 100 + """, + must_contain=["where 42 = f()"], + must_not_contain=[" None: assert_introspects( """ @@ -624,6 +647,21 @@ def check(): class TestIntrospectionContainerLiteral: """Container literals ([...], {...}, {k:v}).""" + @pytest.mark.xfail( + strict=True, + reason="introspect-container-literal: list/dict/set literals are not decomposed", + ) + def test_list_literal_shows_elements(self) -> None: + assert_introspects( + """ + def check(): + def f(): + return 99 + assert [f(), 2, 3] == [1, 2, 3] + """, + must_contain=["where 99 = f()"], + ) + def test_list_literal_semantics_preserved(self) -> None: assert_semantically_equivalent(""" def check(): @@ -727,6 +765,24 @@ def factory(): must_contain=["where 42 = ", "()"], ) + @pytest.mark.xfail( + strict=True, + reason="introspect-callable-variable: a called name shows its repr", + ) + def test_callable_variable_clean_name(self) -> None: + """Ideally should show 'fn()' not '()'.""" + assert_introspects( + """ + def check(): + def factory(): + return 42 + fn = factory + assert fn() == 100 + """, + must_contain=["where 42 = fn()"], + must_not_contain=["