Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/14814.bugfix.rst
Original file line number Diff line number Diff line change
@@ -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``.
39 changes: 29 additions & 10 deletions src/_pytest/assertion/rewrite.py
Original file line number Diff line number Diff line change
Expand Up @@ -1003,19 +1003,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)
Comment on lines +1025 to +1030

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude found an issue here, this crashes with chained comparisons (a < b < c) because in this case value will be ast.BoolOp. Here is a test exercising the regression:

    @pytest.mark.parametrize(
        "expr",
        [
            "identity(a < b < c)",
            "(a < b < c) == True",
            "(a < b < c) + 1 == 2",
        ],
        ids=["call-arg", "compare-left", "binop-left"],
    )
    def test_chained_compare_as_operand(self, expr: str) -> None:
        """A chained comparison visits to a BoolOp, not a name; it must still rewrite."""
        assert_evaluation_order(f"""
                 def check():
                     def identity(v):
                         return v
                     a, b, c = 1, 2, 3
                     try:
                         assert {expr}
                     except AssertionError:
                         return "raised", None
                     return "passed", None
                 """)

This test crashes with:

            # 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), value
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^
E           AssertionError: <ast.BoolOp object at 0x0000016EFEB6B390>

(I added the , value to the assertion for debugging).

This is the suggested change to fix it:

Suggested change
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)
elif isinstance(value, ast.Name):
needs_freeze = value.id in _walrus_targets(later)
else:
# Anything else is built only from temporaries the visit_* methods
# already hoisted -- e.g. the BoolOp a chained comparison yields --
# so a later walrus cannot change what it evaluates to.
needs_freeze = False

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]:
Expand Down Expand Up @@ -1141,8 +1162,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]
Expand Down
30 changes: 30 additions & 0 deletions testing/test_assertrewrite_coverage.py
Original file line number Diff line number Diff line change
Expand Up @@ -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():
Expand All @@ -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("""
Expand Down
Loading