gh-124697: avoid duplicate names in the same frame

[iritkatriel]

This reverts a change that was made as part of #156819 , to sometimes have separate entries in the frame locals for a nested comprehension target and a variable of the same name in the enclosing scope. Instead, this PR goes back to the scheme we had before where the same frame slot is reused.

• Issue: refactor the implementation of inlined comprehensions #124697

[read-the-docs-community]

Documentation build overview

📚 cpython-previews | 🛠️ Build #34975000 | 📁 Comparing 3c5c710 against main (ea0ee92)

  🔍 Preview build  

1 file changed

± library/dis.html

[carljm]

Seems promising, but I think there are still some unresolved issues around the slot re-use.

[carljm]

This skips a comprehension cell for special class-closure names, but _PyCompile_GetRefType() still unconditionally returns CELL for those names in class scope. The two paths then disagree about which slot to use. This source alone segfaults during compilation on this PR, without even calling outer; it compiles successfully on the parent commit (9d22a5334bd):

def outer(__class__):
    class C:
        result = [lambda: __class__ for __class__ in __class__]

__classdict__ reproduces the same crash. Without the lambda, the mismatch can instead mutate the enclosing cell:

def outer(__class__):
    class C:
        result = [__class__ for __class__ in __class__]
    return C.result, __class__

print(outer([1, 2]))

The parent prints ([1, 2], [1, 2]); this PR prints ([1, 2], 2). Could we reconcile the special-name handling in reference resolution, cell allocation, and save/restore before reusing these slots, with coverage for both variants?

[carljm]

Between this instruction and MAKE_CELL, the CO_FAST_FREE slot is NULL. Opcode tracing can observe that interval, but PyFrame_GetVar() assumes that a free-variable slot always contains a cell. This reproducer prints OK on main, but aborts on this PR at the assertion in frame_get_var() (Objects/frameobject.c:2223):

import sys
import _testcapi


def outer(x):
    def inner():
        return [x for x in x]
    return inner


f = outer([1])


def trace(frame, event, arg):
    if frame.f_code is f.__code__:
        frame.f_trace_opcodes = True
        if event == "opcode":
            try:
                _testcapi.frame_getvar(frame, "x")
            except NameError:
                pass
    return trace


sys.settrace(trace)
f()
sys.settrace(None)
print("OK")

_testcapi.frame_getvar exercises the public PyFrame_GetVar() API. Assigning frame.f_locals["x"] in the same interval also hits a free-cell invariant assertion.

I guess one option could be to just handle NULL somehow in the affected frame introspection APIs, but it seems even better if we can avoid creating this invalid state.

One way to do this would be to use LOAD_CLOSURE here instead of LOAD_CLOSURE_AND_CLEAR, and instead create a new opcode MAKE_CELL_EMPTY, which always initializes the new cell as empty instead of using the existing slot contents. Or maybe we don't even need MAKE_CELL_EMPTY? MAKE_CELL could decide to initialize empty based on CO_FAST_FREE, if that's not too implicit. The borrow optimizer would also need to recognize this subtlety in the behavior of MAKE_CELL in order to recognize when the existing value is or isn't kept alive.

[carljm]

Reusing the implicit __class__ free slot changes what zero-argument super() sees. The parent commit (9d22a5334bd) returns a super(C, self) here, but this PR raises TypeError because super() reads the comprehension's int value from that slot:

class C:
    def method(self):
        __class__
        return [super() for __class__ in (int,)]


print(C().method()[0].__thisclass__)

The explicit __class__ reference makes it an enclosing free variable. This is separate from the class-body compilation crash: save/restore is internally consistent here, but the builtin observes the temporary cell during the comprehension.

[iritkatriel]

Hmm. So we still need deduplication for these names.

[carljm]

The new FREE branch reuses the enclosing free-variable slot for the comprehension target and skips hidden-local marking here. Previously, the target had a separate CO_FAST_HIDDEN slot that was populated only while the comprehension was active.

That hidden slot also controls which locals mapping a class frame exposes. Normally, class-body locals() uses the class namespace dictionary. When _PyFrame_HasHiddenLocals() finds a populated hidden slot, the runtime switches to a FrameLocalsProxy, which includes the comprehension target stored in the frame. Clearing the hidden slot on exit restores the normal class-namespace view.

With this change, if there are no other populated hidden slots, that switch never happens: locals() keeps returning the class dictionary, even though the comprehension has correctly stored its iteration value in the reused free slot. The iteration variable is therefore missing from locals():

def outer(x):
    class C:
        values = [locals()["x"] for x in x]
    return C.values


print(outer([1, 2]))

The parent commit (9d22a5334bd) prints [1, 2]; this PR raises KeyError: 'x'. Replacing locals()["x"] with eval("x") likewise changes success to NameError. Adding another target, such as for y in (0,), masks the bug because the hidden slot for y activates the proxy.

Unfortunately I don't see an easy fix for this. Two possible fixes:

• A smaller change would extend _PyFrame_HasHiddenLocals() to recognize a populated free slot whose cell differs from the corresponding cell in the frame function's func_closure. Normally, these are the same cell before the comprehension, different while the temporary cell is installed, and the same again after restoration. Nesting works naturally too. This is not 100% reliable, though: PyFunction_SetClosure() can replace the function's closure while its frame is running, without replacing the cells already copied into that frame. An unchanged outer binding could then look like an active comprehension binding.
• A more involved option would record comprehension instruction ranges as static metadata on the code object, then use the frame's existing instruction pointer to select the locals view. This avoids depending on the function's current closure and needs no runtime activity counter.

[carljm]

Resolving a comprehension-local name as FREE also changes the exception for reading it before initialization. The resulting LOAD_DEREF uses a free-slot offset, so _PyEval_FormatExcUnbound() now chooses NameError rather than UnboundLocalError:

def outer(x):
    def inner():
        return [x for y in x for x in x]
    return inner()


try:
    outer([1, 2])
except UnboundLocalError:
    print("caught uninitialized comprehension local")

The parent commit (9d22a5334bd) reaches the handler; this PR lets a NameError escape, describing x as an uninitialized variable in an enclosing scope. The same change occurs with lambda: x as the element expression.

[carljm]

This condition also needs to handle DEF_FREE_CLASS: a class-local name can have that flag and an entry in u_freevars, even though _PyST_GetScope() returns LOCAL. Checking only enclosing_scope == FREE leaves the comprehension using a separate same-named slot:

import sys


def outer():
    x = 1
    class C:
        x = 2
        vals = [dict(**sys._getframe().f_locals) for x in [3]]

        def m():
            return x

    return C


print(outer().vals[0]["x"])

The parent commit (9d22a5334bd) prints 3; this PR raises TypeError: dict() got multiple values for keyword argument 'x'. The proxy's keys(), values(), items(), and len() also expose/count the duplicate entries.