cl: stabilize generic local ABI names across cache loads - #2299
Conversation
There was a problem hiding this comment.
Review summary
The fix is correct and well-targeted. Replacing t.Obj().Pos() with token.NoPos for the detached TypeName in patchLocalGenericNamed (cl/compile.go) removes the source of cross-process non-determinism: scopeIndices in ssa/abi/abi.go only appends the loader-relative .p<pos> suffix when pos.IsValid(), so token.NoPos drops that varying suffix while type identity is fully preserved by the type-arg-encoded localNamedName. The explanatory comment is accurate, and the subprocess-based cold/hot cache test is the right shape — an in-process test could not reproduce #2297 since the offending token.Pos comes from a per-process FileSet.
Security, documentation-accuracy, and performance passes surfaced no issues (token.NoPos is in fact a net reduction in per-type work). One minor, optional test-robustness finding is left inline.
No blocking issues.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
Summary
TypeNameobjects positionless because their generated names already encode the complete local and generic identity.p<token.Pos>suffix that differs between cached archives and a later synthetic root moduleThis preserves the existing position fallback for genuine detached local-scope objects while making generic-local cache hits link reliably.
Fixes #2297
Testing
go test ./cl -run "^TestGenericLocalTypePatchHelpers$" -count=1go test ./cl -run "^TestRunAndTestFromTestgo$" -count=1go test -tags=dev ./internal/build -run "^TestGenericLocalTypeColdAndHotPackageCache$" -count=1go test ./ssa/abi -count=1llgo test -a -p=1 ...followed by the immediate cache-hitllgo test -p=1 ...; both pass