@markshannon@iritkatriel would be grateful for a pyperformance run on this one (presuming signal all looks good.)

I'll run the benchmarks now

Benchmarking is delayed a bit, but should be back tomorrow at the latest.

You don't seem to be clearing the inner variable before use, so that

deff():l= [None ]    [ (l[0],l)in [[1,2]] ]print(l)

will, I think, print[1] instead of raising an UnboundLocalError

You can actually make the code more efficient by clearing the local when stashing it on the stack:

inst(LOAD_FAST_AND_CLEAR, (--val)) {val=LOCALS[oparg];LOCALS[oparg]=NULL;}

as there is no need for aNULL check orINCREF.

A couple of obscure issues, but this looks like a very nice improvement.

I think the gain in performance is well worth the cost in code size, given how common comprehensions are.

You don't seem to be clearing the inner variable before use

Your example code doesn't involve a comprehension, I'm assuming you meant something like

def f():    l = [ None ]    [ (l[0], l) for l in [[1,2]] ]    print(l)

But this doesn't raiseUnboundLocalError on main either, it works fine; the iteration variablel is always bound within the comprehension before the expression is evaluated.

So far I haven't been able to construct a test case where failing to clear an incoming variable bound inside the comprehension has any visible effect, because AFAICT any locals in a comprehension are always (necessarily by the syntactic limitations of how a comprehension is built) defined before use. Let me know if you can think of a test case I'm missing!

You can actually make the code more efficient by clearing the local when stashing it on the stack

But I think this is sufficient reason regardless to justify switching fromLOAD_FAST_OR_NULL toLOAD_FAST_AND_CLEAR, so I'll do that!

Actually I don't thinkLOAD_FAST_AND_CLEAR will work, because of cases like this:

def f(x):    return [x for x in x]

We need to save/restore the outer value ofx, but we cannot clear it on entry because we are also relying on being able to load it when we evaluate the iterable part of the comprehension (which in the non-inlined case would have been evaluated outside the function and passed in as the.0 parameter.) So I think we need to stick with a non-clearingLOAD_FAST_OR_NULL on entry.

If there were a correctness reason for needing to clear in other cases, I could add an additionalDELETE_FAST after theLOAD_FAST_OR_NULL, but so far I haven't found any such correctness need, and clearing this way is not an efficiency improvement.

EDIT: there is also the option of doing clearing pushes and also moving evaluation of the outermost iterable to before the pushes; this would require some non-elidable SWAPs to preserve it as TOS through the pushes, though...

@markshannon I thinkLOAD_FAST_OR_NULL could also become a pseudo-instruction that resolves toLOAD_FAST (which would get around having it turned intoLOAD_FAST_CHECK), but we'd have to remove theassert(value != NULL) in theLOAD_FAST implementation. Do you prefer keeping that assertion at the cost of another (non-pseudo) opcode?

I've addressed the review comments. I have some TODOs for potential optimizations that could be done here or in a follow-up diff:

• Eliminate some unnecessary push/pops in cases where the value is never loaded after the comprehension. This would fall into the general category of dead store elimination. I'm not sure if we can actually do this or if it violates some guarantees about introspectable frame state in tracing?
• Better handling of SWAPs on the post-comprehension pops.apply_static_swaps can't help us here if the comprehension value is immediately returned (because we can't "swap" aRETURN_VALUE -- this case would go away if we can do the dead store elimination, since if it's right before aRETURN_VALUE it's clearly a dead store) or if it's immediately popped (because thePOP_TOP will have a lineno of-1 andapply_static_swaps will thus refuse to swap it). Instead of having oneSWAP 2 per pop, we could have a singleSWAP N and do the last pop first.
• Teachadd_checks_for_loads_of_uninitialized_variables to understand and trace through the push/pop pairs.

Sorry for posting a gibberish example.
Here's what I should have posted:

deff():l= [None ]    [1for (l[0],l)in [[1,2]] ]print(l)f()

3.11

$ python3.11 ~/test/test.py Traceback (most recent call last):   ...UnboundLocalError: cannot access local variable 'l' where it is not associated with a value

This PR

$ ./python ~/test/test.py [1]

🤖 New build scheduled with the buildbot fleet by@carljm for commita8425a6 🤖

If you want to schedule another build, you need to add the🔨 test-with-refleak-buildbots label again.

🤖 New build scheduled with the buildbot fleet by@carljm for commit95401fe 🤖

If you want to schedule another build, you need to add the🔨 test-with-buildbots label again.

Thes390x Fedora LTO + PGO PR ands390x Fedora LTO PR buildbots look generally unhealthy. The latter has a full disk.

AMD64 Debian root PR is weird. The failure looks a bit flaky (segfault intest_threads_join) and unlikely to be related, but that failure isn't showing up on any of its other builds. Requested a re-build to see if it repros.

The buildbot worker hosting the variouss390x Fedora ... builds appears to be out of disk space and unhealthy. So I'm satisfied that there aren't any buildbot issues caused by this PR.

Merging!