feat(shared): resolveTypeRef strict type resolver (#916, RFC #909 Ring 2 SHARED)#959
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
… RFC #909 Ring 2 SHARED) Implements RFC §4.6: a strict, pure resolver for `TypeRef`s used by `Registry.lookup` Step 2 (type-binding propagation) and by any caller that wants the single best type-target for an annotation without paying for the full evidence pipeline. Algorithm (strict): 1. Walk the scope chain from `ref.declaredAtScope`: - Return the first binding for `rawName` whose origin is in `{'local','import','namespace','reexport'}` AND whose `def.type` is a type-kind (class-like, interface-like, enum-like, alias-like). - If bindings exist but none qualify (non-type shadow, wildcard-only origin), return null immediately — do NOT fall through to the global qualified-name index. 2. If `rawName` is dotted and the scope walk produced no match, consult `QualifiedNameIndex.byQualifiedName`. Only accept a UNIQUE type-kind hit; ambiguous or non-type results return null. `'wildcard'` is deliberately excluded from strict origins — a wildcard-expanded name is too loose to anchor type resolution. Module placement: `gitnexus-shared/src/scope-resolution/resolve-type-ref.ts` (alongside sibling indexes) rather than the issue's suggested `gitnexus-shared/src/resolve-type-ref.ts`, for consistency with the rest of the RFC §2/§3 surface. A minimal `ScopeLookup` interface is declared inline so #916 ships standalone; #912's `ScopeTree` will satisfy this contract without change. Closes part of #909.
511b7ac to
d58af02
Compare
|
@claude Act as a senior reviewer for GitNexus. Your job is to determine whether this PR is production-ready for this repo, not to give a generic code review. You are reviewing a PR in the GitNexus monorepo:
Your task has 2 phases, in this exact order: PHASE 1 — DEFINE THE BAR PHASE 2 — REVIEW THE PR AGAINST THAT BAR CONTEXT TO LOAD FIRST
Additional context:
PRIMARY OBJECTIVE Do not optimize for completeness at the expense of signal. REVIEW RULES
For each finding, assign one severity:
REPO-SPECIFIC REVIEW CHECKLIST 1. Correctness & functional completenessCheck:
2. Code clarity & clean codeCheck:
3. Test coverage & change safetyEvaluate against TESTING.md:
4. PerformanceInspect for:
5. Operational riskCheck:
6. MaintainabilityCheck:
NON-NEGOTIABLES TO VERIFY
OUTPUT FORMAT Production-ready bar for this PRGive a concise repo-specific definition in 5–10 bullets. FindingsFor each finding, use this structure exactly: [BLOCKING|NON-BLOCKING|NIT] Short title
Only include findings that are real and supported.
What I could not fully verifyList any behavior you could not prove from the diff, tests, or available context. Final verdictChoose exactly one:
If verdict is not ready, list the blocking items in priority order. IMPORTANT REVIEW BEHAVIOR
You must also ensure we are on track with the RFC plan! Check the issue tickets involved! |
|
Claude finished @magyargergo's task in 3m 18s —— View job Senior Review — PR #959
|
| Ticket | Status | Note |
|---|---|---|
| #910 types | ✅ Merged | Prerequisite; BindingRef, TypeRef, Scope, etc. imported correctly |
| #911 hooks | ✅ Merged | Prerequisite; no hooks called here |
| #913 indexes (#958) | ✅ Merged | DefIndex + QualifiedNameIndex dependency satisfied |
| #916 resolveTypeRef | This PR | Implements RFC §4.6; unblocks #917 |
| #918 shadow diff | ✅ Merged (#951) | No dependency from this PR; already exported in index.ts |
| #912 ScopeTree | 🔲 Pending | ScopeLookup interface here is forward-compatible per design |
| #917 Registry.lookup | 🔲 Pending | Unblocked by this PR |
Stacking note: PR description says "rebase to main once #958 merges." Commit ac2012e confirms #958 is already merged into main. This PR should be rebased (or the stacking dependency is now resolved — check if the branch already incorporates it).
What I could not fully verify
- Whether
type: 'Type'is ever emitted by the ingestion pipeline for annotation-relevant symbols. Requires greppinggitnexus/src/core/ingestion/extraction code, which is outside this diff. - CI passing — cannot re-run
tsc --noEmitorvitestfrom this context. The PR claims both clean; code is consistent with that claim. - RING2-SHARED-1: Scope / ScopeTree / PositionIndex in gitnexus-shared #912 ScopeTree's concrete interface — cannot verify that it will satisfy
ScopeLookupverbatim until RING2-SHARED-1: Scope / ScopeTree / PositionIndex in gitnexus-shared #912 lands, but the PR design intent is sound.
Final verdict
✅ Ready to merge — no blocking issues.
The implementation correctly and completely delivers RFC §4.6. Algorithm, type safety, test coverage, barrel exports, and operational posture are all sound. The one NON-BLOCKING finding ('Type' NodeLabel) should be confirmed before #917 consumes this API at scale, but does not block merge. The NIT test-name fix is optional.
Before or alongside #917: confirm via ingestion code grep that type: 'Type' is never emitted for type-annotation targets (or add it to TYPE_KINDS if it is).
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 6583 tests passed 97 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
…gyanpatwari#916, RFC abhigyanpatwari#909 Ring 2 SHARED) (abhigyanpatwari#959) Implements RFC §4.6: a strict, pure resolver for `TypeRef`s used by `Registry.lookup` Step 2 (type-binding propagation) and by any caller that wants the single best type-target for an annotation without paying for the full evidence pipeline. Algorithm (strict): 1. Walk the scope chain from `ref.declaredAtScope`: - Return the first binding for `rawName` whose origin is in `{'local','import','namespace','reexport'}` AND whose `def.type` is a type-kind (class-like, interface-like, enum-like, alias-like). - If bindings exist but none qualify (non-type shadow, wildcard-only origin), return null immediately — do NOT fall through to the global qualified-name index. 2. If `rawName` is dotted and the scope walk produced no match, consult `QualifiedNameIndex.byQualifiedName`. Only accept a UNIQUE type-kind hit; ambiguous or non-type results return null. `'wildcard'` is deliberately excluded from strict origins — a wildcard-expanded name is too loose to anchor type resolution. Module placement: `gitnexus-shared/src/scope-resolution/resolve-type-ref.ts` (alongside sibling indexes) rather than the issue's suggested `gitnexus-shared/src/resolve-type-ref.ts`, for consistency with the rest of the RFC §2/§3 surface. A minimal `ScopeLookup` interface is declared inline so abhigyanpatwari#916 ships standalone; abhigyanpatwari#912's `ScopeTree` will satisfy this contract without change. Closes part of abhigyanpatwari#909.
Closes #916. Stacked on #913 (
rfc/scope-resolution/913-indexes) — rebase tomainonce #958 merges.Scope
Implements RFC §4.6 (
resolveTypeRef): a strict, single-return type-reference resolver. Narrower contract thanRegistry.lookup— no name-only global fallback, no confidence ranking, no arity.gitnexus-shared/src/scope-resolution/resolve-type-ref.tsgitnexus/test/unit/scope-resolution/resolve-type-ref.test.ts(22 tests)Algorithm
ref.declaredAtScopeviascope.parent:rawNamewhoseorigin ∈ {local, import, namespace, reexport}AND whosedef.typeis a type-kind (class-like / interface-like / enum-like / alias-like).nullimmediately. No fallthrough to the global qname index.QualifiedNameIndex.byQualifiedName— only whenrawName.includes('.')and the scope walk produced nothing. Accepts a unique type-kind hit; ambiguous →null.Design notes
'wildcard'deliberately excluded from strict origins. Afrom x import *binding is too loose to anchor type resolution.Registry.lookupmay still accept wildcard bindings with lower evidence weight — that's its contract, not ours.Namespaceexcluded from TYPE_KINDS — it is a scope container, not a value type.ScopeLookupinterface declared inline so RING2-SHARED-5: resolveTypeRef strict helper #916 ships as a standalone piece. RING2-SHARED-1: Scope / ScopeTree / PositionIndex in gitnexus-shared #912'sScopeTreewill satisfy this contract verbatim; no churn when RING2-SHARED-1: Scope / ScopeTree / PositionIndex in gitnexus-shared #912 lands.scope-resolution/resolve-type-ref.ts(notsrc/resolve-type-ref.tsas the issue suggested) for consistency with the rest of the RFC §2/§3 surface.Dependency note
Issue #916 lists
Depends on: #910 #911 #912. Practical deps: #910 (types), #911 (hooks — none used here, but part of the Ring 1 surface), #913 (DefIndex/QualifiedNameIndexfor the dotted fallback). The issue's dependency on #912 is aspirational — we only need the minimalScopeLookupinterface, which lives in this PR.Tests (22, all passing)
import { X as Y }) · namespace returns null (not a type kind) · re-export · wildcard ignored · strict origin wins over non-strict at same scopedeclaredAtScope→ null · broken parent chain → null · cycle guard · all 7TypeRef.sourcevalues uniformlyVerification
tsc --noEmitclean (bothgitnexus-sharedandgitnexus)gitnexus-sharedbuild cleanPart of
resolveTypeReffor type-binding propagation)