cl: preserve generic local type identity in nested closures - #2229
cl: preserve generic local type identity in nested closures#2229cpunion wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Review summary
The core fix is well-scoped and correct: makeClosureCtx now becomes a method that temporarily sets p.goFn = fn (save/restore via defer, single exit path — correct) and patches each free-var field type with p.patchType(v.Type()). This makes patchLocalGenericNamed/localTypeOuterArgs resolve captured local generic types against the correct instantiated closure, so a closure-context field and the closure signature land on the same nominal type. The enclosingFunctionSyntax refactor is a sensible generalization, and the unit test plus the tpycombinator runtime golden lock the behavior in.
Verified as correct (no action needed):
- Golden
expect.txt(3628800/xxx) matchesfactorial(10)andrepeat("");printlngoes to stderr, and thecltestharness captures stderr (cl/cltest/cltest.go), so the golden convention holds. - No security surface:
enclosingFunctionSyntaxnil-checksfn.Origin()/Syntax(), the parent walk terminates at nil, and thexfail.yamlremoval / comment changes are consistent.
Findings below are maintainability/efficiency only — none block merge.
Maintainability
-
cl/compile.go—currentFunctionSyntaxis now dead production code. After migratinginCurrentFunctionandlocalTypeOrdinalBySyntaxtoenclosingFunctionSyntax,currentFunctionSyntaxhas no remaining production caller (grepfinds only its definition and the nil-goFnassertion infuncname_nested_closure_test.go:405). It duplicates thep.goFn == nil/fn.Origin()/Syntax()logic and can silently diverge fromenclosingFunctionSyntax— e.g. it returns the rawp.goFnsyntax even when that syntax does not bracket a given position, whereas the new helper returnsnil. Consider deleting it (and the test assertion) so there is one source of truth;enclosingFunctionSyntax's self-case (fn == p.goFn) already covers its former use. (See inline comment.) -
cl/compile.go:2544enclosingFunctionSyntax— undocumented behavioral widening. The oldinCurrentFunctionmatched onlyp.goFn's own syntax bounds; the new helper walks thefn.Parent()chain and returns the first ancestor whose[Pos, End]bracketspos. This is the intended fix (a closure still sees generic locals declared in an enclosing generic function), and it is safe because nested-closure syntax is lexically contained, so innermost matches first and types declared in the currentgoFnare unaffected. But the parent walk is load-bearing and non-obvious — a one-line comment ("walk parents so a closure still sees generic locals declared in an enclosing generic function") would protect it from a future "simplification."
Efficiency (compile-time only, optional)
cl/compile.go:2521-2542— redundantenclosingFunctionSyntaxrecomputation. A singlelocalNamedNamecall can reachenclosingFunctionSyntax(pos)twice for the samepos(vialocalTypeOrdinal→localTypeOrdinalBySyntax, and vialocalTypeOuterArgs→isGenericLocalType→inCurrentFunction), each re-walking the parent chain, andlocalTypeOrdinalBySyntaxruns a freshast.Inspectover the enclosing function each time. Bounded, narrow-case, compile-time only — but if nested-closure + local-generic mangling shows up hot, memoizingenclosingFunctionSyntax/ caching the per-functionTypeSpecordinal would remove the repeated traversals. Not required for this PR.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Fixed the Ubuntu Go workflow failure in Validation:
The earlier review-body suggestions are also present in the current head: the dead |
876d13d to
020a2e3
Compare
Summary
Parent/Originsyntax chainfixedbugs/issue72063.goxfailThe regression test was committed before the implementation. On unmodified main the Y-combinator reproducer reaches LLVM verification with a call-parameter type mismatch: the instantiated local named function type is patched in one closure signature but not in its captured environment.
The direct unit test uses two instances (
intandstring), follows the two-levelAnonFuncschain, and verifies that the innerhcontext field has the same patched local type name as the outer closure parameter while the two instances remain distinct. The runtime golden executes both instances.Validation
go test ./cl -run "^TestGenericLocalRecursiveClosureContextTypePatch$" -count=20go test ./cl -run "^TestRunAndTestFromTestgo$/^tpycombinator$" -count=1go test ./test/goroot -count=1fixedbugs/issue72063.gowith an empty xfail set on Go 1.24.11, 1.25.0, 1.26.0, and 1.26.5git diff --check upstream/main...HEADNo CI, wasm, dwarf, stack object, pthread, or TLS behavior is changed.