gh-124697: Represent inlined comprehensions as subscopes in the symbol table - #156819
gh-124697: Represent inlined comprehensions as subscopes in the symbol table#156819iritkatriel wants to merge 7 commits into
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
…l tables Co-authored-by: Cursor <cursoragent@cursor.com>
Documentation build overview
14 files changed ·
|
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
This looks great! Thank you for working on this ❤️
| if (child_free == NULL) { | ||
| return 0; | ||
| } | ||
| int ok = finalize_inlined_comprehension(ste, child, child_free, |
There was a problem hiding this comment.
I think nested inlined comprehensions need to be finalized against their immediate parent here, rather than continuing to use the original class scope.
With:
class C:
x = 99
result = [[x for _ in (0,)] for x in (42,)]this PR segfaults while compiling; main returns [[42]]. The inner x stays FREE because class_binds_free_name() sees C.x, instead of falling through to the outer comprehension's CELL.
| if (remove_dunder_cond_annotations && PyDict_DelItemString(comp->ste_symbols, "__conditional_annotations__") < 0) { | ||
| return 0; | ||
| Py_CLEAR(to_remove); | ||
| for (Py_ssize_t i = 0; i < PyList_GET_SIZE(comp->ste_children); i++) { |
There was a problem hiding this comment.
Could we avoid recursively walking the already-hoisted descendants here? analyze_block() splices each inlined comprehension's children into ancestor lists, so a depth-n chain is revisited roughly 2**n times.
On a debug build, the following took about 0.74s on this PR versus 0.0014s on the base; depth 26 took several seconds:
source = "[" * 24 + "0" + " for x in ()]" * 24
symtable.symtable(source, "?", "exec")A recursive public get_children() walk also visits seven comprehension entries for only three lexical comprehensions. Maybe finalization needs a lexical-only child list or a visited marker.
| if (is_inlined) { | ||
| VISIT(c, expr, outermost->iter); | ||
| if (push_inlined_comprehension_state(c, loc, entry, &inline_state)) { | ||
| if (push_inlined_comprehension_state(c, loc, entry, &inline_state) < 0) { |
There was a problem hiding this comment.
What restores inline_state.saved_ste if setup fails after _PyCompile_EnterInlinedComprehensionScope() succeeds? codegen_push_inlined_comprehension_locals() can fail while allocating pushed_locals or emitting instructions, and this jumps straight to error, which only clears pushed_locals.
| if (keep < 0) { | ||
| goto error; | ||
| } | ||
| if (!keep) { |
There was a problem hiding this comment.
I'm not sure we want the sparse delta to erase USE here.
def outer(x):
def inner():
return [x for y in ()]On main, inner.lookup("x") is FREE|USE. Here it is FREE with is_referenced() == False, while the comprehension's lookup("x") raises KeyError; no public table reports that x is referenced.
| /* True if name is FREE in the comprehension and bound in the enclosing class. | ||
| * Those names are kept in the compressed delta so lookup does not treat them | ||
| * as class locals. */ | ||
| static int | ||
| inline_comprehension(PySTEntryObject *ste, PySTEntryObject *comp, | ||
| PyObject *scopes, PyObject *comp_free, | ||
| PyObject *inlined_cells) | ||
| class_binds_free_name(PySTEntryObject *ste, PyObject *name, long comp_flags) |
There was a problem hiding this comment.
This comment / function name are low on context. The function name doesn't suggest that this has anything to do with comprehensions, but the comment assumes the reader knows that it does, and knows what a "compressed delta" is.
In general it would be really nice to have somewhere in a comment a more comprehensive explanation of how comprehension inlining is intended to work. (Yes, that would have been good in the previous version too!)
2 participants
Resolves #124697
Inlined comprehensions are now represented in the symbol table as block of a new type
InlinedComprehensionBlock, which is a subscope of the enclosing scope and is interpreted as the delta between the containing scope and the comprehension scope. Symbol lookups thatfail in the subscope continue in the containing scope.
This moves the complexity of compiling inlined comprehensions from codegen to the symbol table
construction.
It removes the smell of the compiler modifying the symbol table in codegen.