BUG: Fix name qualification for inlined comprehensions in the exporter - #272
Merged
Merged
Conversation
PEP 709 (Python 3.12) inlines list, dict and set comprehensions into the enclosing scope, so they no longer produce a symtable of their own. adjust_scope_table_mapping() compensates by inserting None into FormulaTransformer.symtables wherever libCST reports a ComprehensionScope that is not a GeneratorExp, keeping the two lists index-aligned. should_replace() then paired the node's scope with a table by index and, where the slot was None, hunted for the enclosing scope's table by scanning backwards through the flat list for the nearest non-None entry. Nearest-earlier-table is not enclosing-scope: generator expressions, lambdas and nested defs all own tables, and a comprehension only has to follow one of them in the same formula to be resolved against it. Two kinds of wrong code came out of that. A name the wrong table does not hold fell through to the branch meant for names between `from` and `import` and was emitted bare, so the exported formula raised NameError the first time it was called; lifelib's DIA_US_S.Projection.result_annual is the case that found this. A name the wrong table does hold, as a global, was qualified even where it is the comprehension's own loop variable: `for self.a in ys` is valid Python, so the exported model imports and runs, rebinds the `a` Cells on the Space instance, and every later call to that Cells fails with a TypeError pointing nowhere near the formula that broke it. enclosing_symbols() replaces the backward scan and walks the libCST scope's own parent chain until it reaches a scope that owns a table. `scope == v` becomes `scope is v`: libCST's Scope defines no __eq__, so the two are equivalent today, and identity states the intent. The next() that locates the scope gains a None default and falls through to the global table instead of raising StopIteration; a parent scope always has at least its own def node, so that fallback should be unreachable. Python 3.11 and earlier never had the defect. The None slots are inserted only under sys.version_info >= (3, 12), so the backward scan never ran there and the two lists are 1:1. Six tests cover it: a comprehension after a generator expression, after a lambda and after a nested def, which lose their `self.` on main; a loop variable shadowing a Cells name, which gains one, with a runtime assertion that the Cells still answers after the formula has run; and the two negatives, a comprehension *before* any generator expression and a plain reference after one, which are correct on main and stay correct. Verified by exporting all 28 uslib/uklib/jplib models of lifelib at 6d5b078: exactly one hunk of some 69,000 generated lines changes, the six lines of DIA_US_S.Projection.result_annual, and a symtable scan for names that resolve to nothing goes from 5 hits to 0. Exported from main, that model's result_annual raises NameError; with this change it reproduces the modelx original frame for frame. The twelve models covered by test_lifelib.py export byte-identically, modulo the id() literals the exporter embeds for IO and pickle data, which differ between any two runs. Reproduced and fixed on 3.12.9, 3.13.9 and 3.14.7. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
should_replace() decided whether a name is a Space member from the
symtable of its enclosing scope. Under PEP 709 that cannot answer the
question for the target of an inlined comprehension. CPython isolates
the target, so it appears in the enclosing scope's table as a local when
the name is used nowhere else in that scope, but as a global - not
assigned - as soon as the same name is also read as a global there. The
symbol test then qualified it, and `for self.d in ys` is valid Python:
the exported model imports and runs, rebinds the `d` Cells on the Space
instance, and every later call to that Cells fails somewhere unrelated.
def f():
return d(1), [d for d in ys] # -> [self.d for self.d in ys]
That is mode (b) of the defect, and it predates the previous commit;
`[d for d in ys]` with nothing between it and the def was already wrong
on main. Resolving the enclosing scope lexically removed the accident
that masked the variant with a lambda, a generator expression or a
nested def in between, and opened a second one: a comprehension in a
default value, an annotation or a decorator is evaluated in the scope
enclosing the def, and the lexical walk now correctly reaches the module
table, where the inlined target is an assigned global like any Space
member. `def f(t, u=[i for i in range(3)])` came out as
`u=[self.i for self.i in range(3)]`, and `self` does not exist while the
body of the generated class is executed, so importing the exported
package died with NameError.
comprehension_binds() answers the question from libCST instead, which
records the binding in the comprehension's own scope, where nothing is
inlined and nothing is ambiguous. A name bound there is a binding of
the comprehension and is never a Space member, whatever the symtable of
the enclosing scope says about it. The walk up through enclosing
comprehensions covers a name bound by an outer comprehension and read
inside an inner one. The first iterable is deliberately not part of it:
libCST records it in the enclosing scope, exactly as Python evaluates it
there, so `[c for c in c]` still qualifies the iterable and leaves the
two bound occurrences alone.
The guard costs nothing on Python 3.11 and earlier, where the
comprehension owns a symtable and its target is already a local.
Five tests: the loop variable also read by name, with and without an
intervening scope; a comprehension in a default value; a comprehension
nested in another comprehension after a generator expression, which is
the only shape that walks more than one level - the 28-model corpus
never does; and a name bound by an outer comprehension and read by an
inner one. Three fail on main; the other two pass on main and are there
to keep the lexical resolution of the previous commit from regressing
them.
The 28 uslib/uklib/jplib models still export with exactly one hunk of
difference from main and no unresolvable names, and the twelve models
covered by test_lifelib.py still export byte-identically.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The comprehension_scopes fixture exports one Space holding every formula
shape the comprehension tests assert on, and one of them cannot be
exported at all before Python 3.12:
def comp_in_default(t, u=sum([i for i in range(3)])):
return t + u
There a comprehension still owns a symtable, and a default value is
evaluated in the scope enclosing the def, so symtable makes that table a
child of the module while libCST orders the matching ComprehensionScope
after the FunctionScope. adjust_scope_table_mapping pairs the two lists
by position, and `assert s.name == t.get_name()` compares
`comp_in_default` against `listcomp` and fails - in __init__, before the
transformer visits anything. One unexportable formula therefore took the
whole fixture down and errored all eleven tests at setup on 3.9, 3.10 and
3.11, on every OS. It is the same defect that a generator expression or
a lambda in that position hits on every version, reported in GH271.
Move that formula to its own fixture and skip its test below 3.12. The
over-qualification it guards against is reachable only where PEP 709
inlines the comprehension into the module scope, so there is nothing for
it to assert on the versions it is now skipped for. The other ten tests
keep running everywhere.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Fixes two ways
Model.exportcould generate wrong code for a name inside a list,dict or set comprehension. Both live in
FormulaTransformer.should_replace.Found by exporting all 28 uslib/uklib/jplib models of lifelib:
DIA_US_S.Projection.result_annualcame out with five bare names and raisedNameErroron the first call. The other 27 exported clean.1. Resolve the enclosing scope lexically (b450515)
PEP 709 (Python 3.12) inlines list, dict and set comprehensions into the enclosing
scope, so they no longer produce a symtable.
adjust_scope_table_mappingcompensatesby inserting
Noneintoself.symtablesfor each such scope, keeping it aligned withself.scopes.should_replacethen found the enclosing scope's table by scanning theflat list backwards for the nearest non-
Noneentry.Nearest-earlier-table is not enclosing-scope. Generator expressions, lambdas and
nested
defs all still own tables, and a comprehension only has to follow one of themin the same formula to be resolved against it:
enclosing_symbolsreplaces the scan and walks the libCST scope's ownparentchainuntil it reaches a scope that owns a table.
2. Never qualify a name the comprehension itself binds (c67e51f)
These two commits have to ship together. Resolving the scope correctly exposes the
fact that under PEP 709 the enclosing symtable is lossy: it cannot distinguish an
inlined comprehension's isolated target from a global of the same name. Commit 1 alone
regresses two shapes that
maingets right:maindef f(t, u=[i for i in range(3)])u=[self.i for self.i in ...], andselfdoes not exist while the generated class body executes, so the exported package fails to importdef f(): return d(1), [d for d in ys], with a lambda / genexp / nesteddefin between[self.d for self.d in ys]The second is the dangerous mode:
for self.d in ysis valid Python — an attribute isa legal assignment target — so the exported model imports and runs, silently rebinds
the
dCells on the Space instance, and every later call to that Cells fails somewherethat points nowhere near the formula that broke it.
comprehension_bindsasks libCST instead, which records the binding in thecomprehension's own scope, where nothing is inlined and nothing is ambiguous. It also
closes the same defect in the no-intervening-scope case,
def f(): return d(1), [d for d in ys], which was already wrong onmain.The first iterable is deliberately not covered: libCST records it in the enclosing
scope, exactly as Python evaluates it there, so
[c for c in c]still qualifies theiterable and leaves the two bound occurrences alone.
Python 3.11 and earlier are unaffected by either change. The
Noneslots are insertedonly under
sys.version_info >= (3, 12), so the backward scan never ran there, and acomprehension that owns a symtable already reports its target as a local.
Tests
Eleven items in
modelx/tests/export/test_export.py, off one module-scoped fixture,following the
test_cacheless.pyidiom of asserting ongetsourceof the generatedmethod. Seven fail on
main:defthan one level; the 28-model corpus never does
same formula, both with a runtime assertion that the Cells still answers after the
formula has run
Four pass on
mainand are there to keep this change from regressing them: acomprehension before any generator expression, a plain reference after one, a
comprehension in a parameter default, and the loop-variable-also-read case with an
intervening lambda.
Verification
modelx/tests/export/50 passed; full suite 1405 passed, 6 skipped.mainand fixed here under Python 3.12.9, 3.13.9 and 3.14.7. There isno 3.11-or-earlier interpreter on this machine, so that path is read from the code,
not tested. (
test_lifelib.pycrashes on the free-threaded 3.14 build with a C-stackoverflow inside modelx's own evaluation — pre-existing on
main, unrelated.)exactly one hunk of 69,142 generated lines — the six lines of
DIA_US_S.Projection.result_annual— and a symtable scan for names that resolve tonothing goes from 5 hits to 0.
main,DIA_US_S.Projection.result_annualraisesNameError; withthis change it reproduces the modelx original frame for frame.
test_lifelib.pyexport byte-identically. The only rawdifferences are the
id()literals the exporter embeds for IO and pickle data, and afixed-vs-fixedrun produces the same noise, so it is not from this change.For the reviewer
relnotes_v0_33_0.rstmarked not yet released,following the draft pattern of 09671df, since 0.32.0 has shipped. Rename it if the
next release is 0.32.1.
id(scope)-keyed dict tidy-up that would replace the linear scan inenclosing_symbols. Measured over 12 uslib models it is 0.98x — libCST parsingdominates and the scan is free. Happy to add it if you want it anyway.
return self.name_to_symbol[0]inenclosing_symbolsis a fallback that appears tobe unreachable: instrumented over the 28-model corpus, 27,159 lookups never reached
it. Left as a fallback rather than an assertion, but it could be either.
touched here. Filed separately as Exporter generates wrong or invalid code for several formula shapes #271.
🤖 Generated with Claude Code