test(rewrite): record the introspection gaps nothing fixes yet - #14916
Draft
RonnyPfannschmidt wants to merge 10 commits into
Draft
test(rewrite): record the introspection gaps nothing fixes yet#14916RonnyPfannschmidt wants to merge 10 commits into
RonnyPfannschmidt wants to merge 10 commits into
Conversation
psf-chronographer
Bot
added
the
bot:chronographer:provided
This was referenced Aug 20, 2026
RonnyPfannschmidt
force-pushed
the
ronny/rewrite-remaining-gaps
branch
5 times, most recently
from
August 26, 2026 10:04
8946b42 to
d9cdf61
Compare
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>
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) <noreply@anthropic.com>
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
pytest-dev#14822 (comment),
having reduced it from his own pytest-dev#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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
RonnyPfannschmidt
force-pushed
the
ronny/rewrite-remaining-gaps
branch
from
September 1, 2026 11:54
d9cdf61 to
857139c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #14822 → #14821 → #14817 → #14816 → #14815 → #14814 → #14447 → #14921 → #14813; its diff includes theirs.
The coverage matrix in #14813 carries only tests that pass, and each PR in the series above lands its own group green. Two gaps are left over that nothing in the series fixes:
They live here, as strict xfails whose body states the message we would want, so the backlog is written down in executable form without asking any of the merging PRs to accept a claim it does not fix. This one stays draft until somebody picks a group up; whoever does flips its markers and the strictness makes it impossible to forget.