fix(go): generic composite literal constructor inference (F33)#1976
Conversation
|
@evander-wang is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 10928 tests passed 13 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
magyargergo
left a comment
There was a problem hiding this comment.
PR Tri-Review — F33 generic composite-literal constructor inference
Verdict: production-ready with minor follow-ups. No correctness, security, or performance regression was found across three review methods; the implementation is correct and the primary cases are well-tested. The remaining items are maintainability/coverage follow-ups, none merge-blocking.
Methods & engine independence (stated honestly). GitNexus risk lane + 5 Compound-Engineering personas (correctness, adversarial, maintainability, performance, testing) + Codex (gpt-5.5, xhigh). 7 of the 8 perspectives (6 review lanes + this synthesis) are Claude — their agreement is "consistent across personas," not independent confirmation. Only Codex is a different engine, so Codex-corroborated points carry the real cross-engine weight. The coordinator re-verified every load-bearing claim with real tree-sitter parses in a worktree.
What's solid (validated, not merely unchallenged):
- Correctness lane refuted all six probed failure modes with real parses; the adversarial lane could not break it (exotic / multi-arg /
*T/ slice / map type-args, ~2000-level nesting ≈28 ms with no overflow, capture leakage from type-param lists / field types / signatures /new/make/assertions → no spurious edges, container-of-generic correctly non-capturing, no double-emission). Codex independently concluded "no runtime defect." - Strictly additive for non-generic Go: the new
normalizeGenericConstructorCaptureguard cannot fire fortype_identifier/qualified_typenodes, so existing capture output is unchanged — zero regression surface for non-generic repos. - Performance: the new normalization is O(1) per match; the #1848/#1915 hot-path linearity is intact.
bench --check→ scaling ratio 1.144 (budget 1.5);scaling_budgetcorrectly left unchanged, only the fingerprint rebaselined. - Legacy/registry split is test-accounting, not a production divergence: Go is registry-primary by default (
MIGRATED_LANGUAGES), so no production consumer hits the legacy DAG; theLEGACY_RESOLVER_PARITY_EXPECTED_FAILURESentry is correctly scoped to the one new test. - Test runs (correctness/adversarial lanes, worktree sequential parser): Go scope-resolution unit 85/85, constructor-inference integration 9/9 (incl. the new
Boxtest), captures-golden 9/9; bench fingerprint matches the rebaseline.
Inline findings (maintainability, non-blocking):
- [P2]
extractSimpleTypeNameTextis now duplicated verbatim incaptures.tsandtype-binding.ts— drift hazard. - [P3]
extractTypeNode's fallback gainedgeneric_typebut its siblingextractCompositeLiteralTypeNodedid not — harmless today (thevar x = Box[T]{}path is rescued bychildForFieldName('type'); coordinator reproduced →{name:b, type:Box}), but a latent inconsistency.
Codex's independent edge case — investigated, NOT a confirmed defect. Codex flagged that generic-qualified models.Box[T]{} drops the package qualifier at capture time (emits Box), while plain models.Box{} keeps models.Box (coordinator reproduced both). Verified consequence: for the type-binding / receiver path this is provably benign — interpretGoTypeBinding → normalizeGoTypeName strips both the qualifier and the [...], so plain and generic both reduce to Box (interpret.ts:37, 93–96); any shadowing would already affect plain models.Box{}, so this PR introduces no regression there. The reference/CALLS path is untested under shadowing (a package that both declares a local Box and instantiates an imported models.Box[T]), but Codex itself downgraded this to "coverage, not a runtime defect" after tracing the call path, and the non-shadowing case is proven correct by the new integration test. Recommend a shadowed-qualified-generic regression test to close it.
Coverage gaps (nice-to-have; behaviors verified to work but untested): var x = Box[T]{} (var-spec path), multi-assign generic (a, b := X{}, Box[int]{}), nested generic (Box[Container[int]]{}), multi-type-arg (Box[K,V]{}), qualified generic at the unit level, and a negative assertion that the type argument models.User does not create a spurious User edge (adversarial confirmed it doesn't). Separately, the scope-capture bench synthetic Go source contains no generic literals, so the F33 path isn't exercised at the 250/800-entity scale — perf for it rests on the O(1)-per-match analysis plus the 1.144 measurement on the small fixture; one generic literal in the bench generator would gate it at scale.
Merge state & branch hygiene. mergeable=MERGEABLE (no conflicts) but mergeStateStatus=BLOCKED → checks failing. Branch hygiene: one merge-from-main commit (3412d4c0) present but harmless and merge-safe (no overlap with the F33 production files); the other three commits are the F33 work.
CI. All substantive gates green — typecheck, lint, format, benchmarks, scope-resolution parity, coverage, tree-sitter ABI (all platforms), CodeQL, both Build & Push, macOS platform-sensitive. Failing: Vercel (deploy-auth gate, not code) and tests / windows-latest (platform-sensitive) → CI Gate. The Windows failure is a pre-existing, unrelated flake: 8 cli-e2e.test.ts failures about CLI stdout / fd-1 / EPIPE handling and the eval-server "No indexed repositories found" startup (cli-e2e.test.ts:1242/1259/1270/1283/1334/289) — this PR touches only Go ingestion code, with no CLI/eval-server/stdout surface, so it cannot be the cause.
Automated multi-tool digest — 6 Claude review lanes + Codex (gpt-5.5, xhigh), one independent engine; coordinator-verified against real parses. Treat as input to your judgment, not a substitute — verify before acting.
| } | ||
| } | ||
|
|
||
| function extractSimpleTypeNameText(node: SyntaxNode): string { |
There was a problem hiding this comment.
[P2 · maintainability · code-read] This extractSimpleTypeNameText is byte-identical to the copy in type-binding.ts:247–257 (same qualified_type handling + the new generic_type branch). Two copies will drift independently the next time a node type needs unwrapping. captures.ts already imports from type-binding.ts (line 13), so exporting the existing definition there and importing it here removes the duplicate with zero behavior change. Non-blocking.
| expr.childForFieldName('type') ?? | ||
| expr.namedChildren.find((c) => ['type_identifier', 'qualified_type'].includes(c.type)) ?? | ||
| expr.namedChildren.find((c) => | ||
| ['type_identifier', 'qualified_type', 'generic_type'].includes(c.type), |
There was a problem hiding this comment.
[P3 · maintainability/consistency · reproduced] This fallback now includes generic_type, but the sibling extractCompositeLiteralTypeNode (line 264, the var-spec path) still lists only ['type_identifier', 'qualified_type']. It's harmless today — var x = Box[T]{} is rescued by childForFieldName('type') (reproduced: yields {name:'b', type:'Box'}), so this fallback is effectively dead code — but the asymmetry is a latent trap and the var-form generic case currently has no test. Suggest mirroring the list here (or adding a var-form generic test). Non-blocking.
…eLiteralTypeNode fallback
…t-phase benchmark
…idance in interface detection
| expr.childForFieldName('type') ?? | ||
| expr.namedChildren.find((c) => ['type_identifier', 'qualified_type'].includes(c.type)) ?? | ||
| expr.namedChildren.find((c) => | ||
| ['type_identifier', 'qualified_type', 'generic_type'].includes(c.type), |
There was a problem hiding this comment.
Can we move this array into a set so we won't need to recreate it in every iteration? Plus we can reuse it at line 282.
| if (result.size === 0) return []; // Early exit: no struct has all required methods | ||
| } | ||
| return best === undefined ? [...indexes.structsById.keys()] : [...best]; | ||
| return result === undefined ? [...indexes.structsById.keys()] : [...result]; |
There was a problem hiding this comment.
If we are carrying a lot of elements in either case, it will result in stackoverflow because of the spread operator.
What changed
Go generic composite literal constructors (e.g.
Box[User]{}) were not captured by the scope-resolution pipeline. The tree-sitter parser producesgeneric_typenodes for these patterns, but the Go ingestion code only handled plaintype_identifiernodes in composite literal positions.This is Go language coverage gap F33.
Why
Generic composite literals are a core Go 1.18+ feature. Without this fix, any codebase using
pkg.T[Type]{}patterns gets no constructor-inferred type binding, producing missing CALLS edges and incomplete call graphs.How
query.ts): Added(generic_type)to constructor detection patterns inGO_SCOPE_QUERYcaptures.ts):normalizeGenericConstructorCapture()strips type parameters fromgeneric_typenodes to extract the base type name (Box[User]→Box)type-binding.ts): EnhancedextractSimpleTypeNameText()to recursively unwrapgeneric_typeandqualified_typenestingHow to verify
All pass: 16/16 unit ✅, 9/9 golden ✅, 136/136 integration ✅.
Risk and rollback
Box[Container[User]]{}) and qualified generics (pkg.Box[User]{}) can be added in a separate PRRefs #1927