Skip to content

fix(rewrite): freeze operands against any later side effect - #14821

Draft
RonnyPfannschmidt wants to merge 7 commits into
pytest-dev:mainfrom
RonnyPfannschmidt:ronny/fix-rebound-name-operands
Draft

RonnyPfannschmidt wants to merge 7 commits into
pytest-dev:mainfrom
RonnyPfannschmidt:ronny/fix-rebound-name-operands

Conversation

@RonnyPfannschmidt

@RonnyPfannschmidt RonnyPfannschmidt commented Jul 31, 2026 •

Copy link
Copy Markdown
Member

Fixes #14820. Stacked on #14817 → #14816 → #14815 → #14814 → #14447 → #14921 → #14813; its diff includes theirs.

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

def test_left_operand_is_read_too_late():
    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(): freeze a name whenever anything that follows it can execute code at all (Call, Await, NamedExpr).

That sounds expensive and is not. An operand that was already hoisted needs no freeze, so the common shapes are byte-identical:

assert len(items) == expected     # unchanged, left operand is already a temporary
assert value == compute()         # one extra 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.

Lands the order-name-rebound-by-call cases of the coverage matrix in #14813, as passing tests (+3).

Found by sweeping the rewriter for evaluation-order divergences after the walrus work — this is the same defect class as #14445, reached by a different trigger.

@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-rebound-name-operands branch from c4f9811 to edc350c Compare July 31, 2026 20:19
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-rebound-name-operands branch from edc350c to 20d0b1c Compare August 10, 2026 07:28
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-rebound-name-operands branch from 20d0b1c to 382db6b Compare August 20, 2026 04:35
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-rebound-name-operands branch 4 times, most recently from 56e3c3b to fe23ee2 Compare August 26, 2026 10:04
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-rebound-name-operands branch from fe23ee2 to f41bb7f Compare September 1, 2026 11:54
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-rebound-name-operands branch from f41bb7f to af6db42 Compare September 15, 2026 11:34
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-rebound-name-operands branch from af6db42 to a925a4c Compare September 23, 2026 07:00
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-rebound-name-operands branch from a925a4c to d1dbe3d Compare October 4, 2026 13:06
RonnyPfannschmidt and others added 7 commits October 11, 2026 08:54
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>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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 <cond> 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
…ev#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>
@RonnyPfannschmidt
RonnyPfannschmidt force-pushed the ronny/fix-rebound-name-operands branch from d1dbe3d to 119e7d5 Compare October 11, 2026 07:26

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.

Assertion rewriting reorders an operand when a later call rebinds its name

1 participant