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) <noreply@anthropic.com>

Author: RonnyPfannschmidt

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``.

src/_pytest/assertion/rewrite.py

@@ -568,14 +568,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)
@@ -1002,34 +1007,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:
@@ -1049,9 +1052,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.
@@ -1068,10 +1070,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()))
@@ -1205,6 +1207,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]

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("""