cl: preserve generic local type identity in nested closures - #2229
cl: preserve generic local type identity in nested closures#2229cpunion wants to merge 7 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
020a2e3 to
35e4a41
Compare
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
35e4a41 to
42ff689
Compare
Summary
Parent/Originsyntax chainFixes #2304.
Bug
Go requires a local
boxdeclared inboxFuncs[int]to have a different type identity from the correspondingboxinboxFuncs[string]. On currentmain, nested closures omit the enclosing instantiation and both interface conversions use the same runtime type descriptor. Cross-instantiation assertions therefore incorrectly succeed:The runnable
tplocalclosureifaceregression reproduces this with anintbox and a differently laid-outstringbox. With this PR, their closures use distinctbox[int]andbox[string]descriptors and produce the expected result.The same omission also gave the generic Y-combinator's dynamic call and actual closure entry different LLVM nominal types. The IR regression requires both sides of each
intandstringcall edge to use the same instantiated type.Validation
go test ./cl -run 'TestGenericLocalRecursiveClosureContextTypePatch|TestRunAndTestFromTestgo/(tplocalclosureiface|tpycombinator)' -count=1go test ./ssa -run 'TestFromTestgo/(tplocalclosureiface|tpycombinator)' -count=1go run ./cl/_testgo/tplocalclosureifacegit diff --check