fix[next]: collect closure vars via the compiler's scope analysis - #2864
Open
havogt wants to merge 1 commit into
Open
fix[next]: collect closure vars via the compiler's scope analysis#2864havogt wants to merge 1 commit into
havogt wants to merge 1 commit into
Conversation
`get_closure_vars_from_function` used `inspect.getclosurevars`, which disassembles
the enclosing function's own code object and collects the names loaded by its
`LOAD_GLOBAL` instructions. A generator expression compiles to a separate code
object, so a name referenced only inside a tuple comprehension body is recorded
there and nowhere else, and was never collected. Type deduction then failed with
`UndefinedSymbolError` on a name that is visibly imported:
@gtx.field_operator
def testee(tracers: tuple[EField, ...]) -> tuple[CField, ...]:
return tuple(neighbor_sum(t(C2E), axis=C2EDim) for t in tracers)
# UndefinedSymbolError: Undeclared symbol 'neighbor_sum'
This affected every module-level name: builtins, `FieldOffset`s, `Dimension`s
and module-level field operators, so `tuple(_inner(t, ...) for t in tracers)`
did not work either.
Free variables were unaffected, because closing over one forces a cell whose
name is recorded on the enclosing code object as `co_freevars`. Globals need no
cell, so nothing links them back. That is also why the existing comprehension
tests pass: they all define their helpers inside the test function.
Take the global names from the compiler's own scope analysis instead, via the
`symtable` module this file already imports. Every scope the compiler creates,
the generator expression's included, is a child `symtable.Function` whose
`get_globals()` is exactly the set of names that compile to `LOAD_GLOBAL` there,
so locals shadowing a global and comprehension targets are excluded by
construction. The source is analyzed under `from __future__ import annotations`
so annotations contribute no names on any supported version. Free variables keep
coming from `inspect.getclosurevars`.
Claude-Session: https://claude.ai/code/session_01VR1cyTQ4wysovMBAwPBAWh
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.
Targets
sf_n_tracer_support(#2833) rather thanmain, because the bug is only reachable through the tuple comprehensions that PR introduces. Follow-up to this review comment.This is the alternative to #2861 — same bug, same tests, different mechanism. They are mutually exclusive; pick one and close the other. Comparison at the bottom.
Problem
A name referenced only inside a tuple comprehension body is not collected as a closure variable, so type deduction rejects it — naming a symbol that is visibly imported at module scope:
This covers every module-level name a real stencil uses: gt4py builtins,
FieldOffsets,Dimensions, and module-level field operators — so the natural factoringtuple(_inner(t, ...) for t in tracers)does not work either.Two things make it more than a limitation: the diagnostic names a symbol that is plainly in scope, and the workaround is to also reference the name outside the comprehension — so whether a stencil compiles depends on an unrelated line elsewhere in the function.
Cause
get_closure_vars_from_functionusesinspect.getclosurevars, which disassembles the enclosing function's own code object and collects the names loaded by itsLOAD_GLOBALinstructions. A generator expression compiles to its own code object, so the names it uses are recorded there and nowhere else.Free variables are unaffected, because closing over one forces a cell whose name is recorded on the enclosing code object as
co_freevars. Globals need no cell, so nothing links them back. That asymmetry is also why the existing comprehension tests are green — they all define their helpers inside the test function, putting them on the working side of the bug.Fix
Take the global names from the compiler's own scope analysis, via the
symtablemodule this file already imports, rather than from code objects.Every scope the compiler creates — the generator expression's included — is a child
symtable.Functionwhoseget_globals()is exactly the set of names that compile toLOAD_GLOBALin that scope. Scoping is therefore correct by construction: a local shadowing a global isis_localin the function andis_freein the comprehension, never global, and the comprehension target is local to the comprehension. Attribute names, whichco_namesalso holds, never appear.The source is analyzed under a prepended
from __future__ import annotationsso annotations contribute no names on any supported version — Python evaluates parameter and return annotations in the enclosing scope and never evaluates local variable annotations, but only 3.14'ssymtablestops reporting the latter. Filtering onisinstance(table, symtable.Function)also skips theANNOTATIONtables 3.14 introduces. Free variables keep coming frominspect.getclosurevars.Tests
test_tuples.py: three integration tests placed next totest_tuple_comprehension_other_fo, so the contrast is visible in one screen — that test defines its helper inside the test (free variable, always worked), the new ones use a module-level operator and a module-level builtin. Plus the used-outside-too variant that documents the workaround.test_tuple_comprehension_closure_vars.py: mechanism-level unit tests pinning that the names live in the nested code object, that the comprehension target is not collected, and that an enclosing local shadowing a module-level name is not picked up as a global.All new tests fail without the source change.
Validation
tests/next_tests/unit_tests/ffront_tests/+type_system_tests/: 443 passed, 2 skipped, 3 xfailed — on 3.12, 3.13 and 3.14.uv run mypy src/: clean (361 files).pre-commit: all hooks pass.Versus #2861
symtable)LOAD_GLOBALLOAD_GLOBALis filteredsymtable.Functionclassification + PEP 563disopnamesNeither is obviously better. This one depends on documented
symtablesemantics and reuses machinery already imported here; #2861 stays with the mechanisminspect.getclosurevarsitself uses and avoids a second source read.https://claude.ai/code/session_01VR1cyTQ4wysovMBAwPBAWh