Skip to content

cl: preserve generic local type identity in nested closures - #2229

Open
cpunion wants to merge 5 commits into
xgo-dev:mainfrom
cpunion:codex/xfail-run-audit-20260730
Open

cl: preserve generic local type identity in nested closures#2229
cpunion wants to merge 5 commits into
xgo-dev:mainfrom
cpunion:codex/xfail-run-audit-20260730

Conversation

@cpunion

@cpunion cpunion commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • patch captured free-variable types under the callee closure's generic function context
  • resolve parentless instantiated local type declarations through the closure Parent/Origin syntax chain
  • keep nested closure signatures and closure-context fields on the same LLVM nominal type
  • remove only the Darwin/arm64 fixedbugs/issue72063.go xfail

The 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 (int and string), follows the two-level AnonFuncs chain, and verifies that the inner h context 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=20
  • the same unit test with Go 1.24.11
  • go test ./cl -run "^TestRunAndTestFromTestgo$/^tpycombinator$" -count=1
  • go test ./test/goroot -count=1
  • fixedbugs/issue72063.go with an empty xfail set on Go 1.24.11, 1.25.0, 1.26.0, and 1.26.5
  • git diff --check upstream/main...HEAD

No CI, wasm, dwarf, stack object, pthread, or TLS behavior is changed.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) matches factorial(10) and repeat(""); println goes to stderr, and the cltest harness captures stderr (cl/cltest/cltest.go), so the golden convention holds.
  • No security surface: enclosingFunctionSyntax nil-checks fn.Origin()/Syntax(), the parent walk terminates at nil, and the xfail.yaml removal / comment changes are consistent.

Findings below are maintainability/efficiency only — none block merge.

Maintainability

  • cl/compile.gocurrentFunctionSyntax is now dead production code. After migrating inCurrentFunction and localTypeOrdinalBySyntax to enclosingFunctionSyntax, currentFunctionSyntax has no remaining production caller (grep finds only its definition and the nil-goFn assertion in funcname_nested_closure_test.go:405). It duplicates the p.goFn == nil / fn.Origin() / Syntax() logic and can silently diverge from enclosingFunctionSyntax — e.g. it returns the raw p.goFn syntax even when that syntax does not bracket a given position, whereas the new helper returns nil. 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:2544 enclosingFunctionSyntax — undocumented behavioral widening. The old inCurrentFunction matched only p.goFn's own syntax bounds; the new helper walks the fn.Parent() chain and returns the first ancestor whose [Pos, End] brackets pos. 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 current goFn are 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 — redundant enclosingFunctionSyntax recomputation. A single localNamedName call can reach enclosingFunctionSyntax(pos) twice for the same pos (via localTypeOrdinallocalTypeOrdinalBySyntax, and via localTypeOuterArgsisGenericLocalTypeinCurrentFunction), each re-walking the parent chain, and localTypeOrdinalBySyntax runs a fresh ast.Inspect over the enclosing function each time. Bounded, narrow-case, compile-time only — but if nested-closure + local-generic mangling shows up hot, memoizing enclosingFunctionSyntax / caching the per-function TypeSpec ordinal would remove the repeated traversals. Not required for this PR.

Comment thread cl/compile.go Outdated
@codecov

codecov Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@cpunion

cpunion commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator Author

Fixed the Ubuntu Go workflow failure in 876d13d8f. The new tpycombinator runtime fixture was also discovered by ssa.TestFromTestgo, whose IR-only path requires an IR spec. Instead of skipping that check, the source now has FileCheck assertions requiring distinct int and string local-generic closure-context types, which directly covers this PR’s type-identity invariant.

Validation:

  • go test ./ssa -run "TestFromTestgo/tpycombinator" -count=1 -v
  • go test ./cl -run "TestRunAndTestFromTestgo/tpycombinator" -count=1 -v
  • go test ./ssa -count=1
  • git diff --check

The earlier review-body suggestions are also present in the current head: the dead currentFunctionSyntax helper was removed, and enclosingFunctionSyntax documents why it walks parent functions.

@cpunion
cpunion force-pushed the codex/xfail-run-audit-20260730 branch from 876d13d to 020a2e3 Compare July 30, 2026 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant