Skip to content

fix(rewrite): freeze walrus and starred operands too - #14814

Open
RonnyPfannschmidt wants to merge 2 commits into
pytest-dev:mainfrom
RonnyPfannschmidt:ronny/fix-walrus-raw-operands
Open

RonnyPfannschmidt wants to merge 2 commits into
pytest-dev:mainfrom
RonnyPfannschmidt:ronny/fix-walrus-raw-operands

Conversation

@RonnyPfannschmidt

@RonnyPfannschmidt RonnyPfannschmidt commented Jul 31, 2026 •

Copy link
Copy Markdown
Member

Stacked on #14447 → #14921 → #14813. Its diff includes theirs.

visit_operand() (added in #14447) 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)      # passes (1, 2) to plain Python, (2, 2) rewritten
assert collect(*items, identity(items := [9])) == (1, [9]) # passes (1, [9]) plain, (9, [9]) rewritten

A walrus operator left in place assigns only 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 reach it.

Lands the order-starred-argument case and the last order-call-argument case of the coverage matrix in #14813, as passing tests (+2).

The starred case was found by the matrix, not by hand — it is what the order-* axis is for.

@psf-chronographer psf-chronographer Bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Jul 31, 2026
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-walrus-raw-operands branch from 16b94a1 to cc6afa4 Compare July 31, 2026 19:04
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-walrus-raw-operands branch from cc6afa4 to d271ad5 Compare July 31, 2026 20:18
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-walrus-raw-operands branch from d271ad5 to 5af71a7 Compare August 10, 2026 07:12
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-walrus-raw-operands branch from 5af71a7 to fd41d89 Compare August 20, 2026 04:35
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-walrus-raw-operands branch 4 times, most recently from 837173f to 338cd67 Compare August 26, 2026 10:04
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-walrus-raw-operands branch from 338cd67 to 6446871 Compare September 1, 2026 11:54
@RonnyPfannschmidt
RonnyPfannschmidt marked this pull request as ready for review September 10, 2026 05:14
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-walrus-raw-operands branch from 6446871 to 3487074 Compare September 15, 2026 11:34
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-walrus-raw-operands branch from 3487074 to 70aad30 Compare September 23, 2026 07:00
Comment on lines +1027 to +1032
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)

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

@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-walrus-raw-operands branch from 70aad30 to 25fc1e8 Compare October 4, 2026 13:06
@ChihebBENCHEIKH1

Copy link
Copy Markdown

@RonnyPfannschmidt this PR fixes assert (x := 5) == (x := 6), but a rebound walrus target still shows the wrong value when it is read through an attribute.

class Page:
    def __init__(self, status):
        self.status = status

    def __repr__(self):
        return f"Page({self.status})"


def test_attribute():
    assert (p := Page(200)).status == 200 and (p := Page(404)).status == 200

On this PR (25fc1e8), on #14822 (b044239) and on main:

E       assert (200 == 200 and 404 == 200)
E        +  where 200 = Page(404).status
E        +  and   404 = Page(404).status

9.1.1 shows where 200 = Page(200).status.

Would you like a PR for this on top of your stack, or would you rather fold it into one of these?

@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-walrus-raw-operands branch from 25fc1e8 to ffea765 Compare October 11, 2026 07:26
RonnyPfannschmidt and others added 2 commits October 14, 2026 22:10
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) <noreply@anthropic.com>
…eeds

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) <noreply@anthropic.com>
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-walrus-raw-operands branch from ffea765 to 4be88ec Compare October 14, 2026 20:28

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:chronographer:provided (automation) changelog entry is part of PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants