feat(shared): MethodDispatchIndex materialized view over HeritageMap (#914, RFC #909 Ring 2 SHARED)#960
Conversation
…914, RFC #909 Ring 2 SHARED) Implements RFC §3.1 `MethodDispatchIndex`: a two-way materialized view keyed by `DefId` for O(1) method-dispatch resolution: - `mroByOwnerDefId` — owner class → full MRO ancestor chain (excludes self, per-language strategy order) - `implsByInterfaceDefId` — interface/trait → classes that implement it **Not an MRO implementation.** `buildMethodDispatchIndex` is a pure aggregator that calls back into caller-provided `computeMro` and `implementsOf` functions. The five existing strategies (Python C3, Ruby kind-aware, Java/Kotlin linear, Rust qualified-syntax, COBOL none) stay where they are today (`model/resolve.ts`, `languages/ruby.ts`); this index does not reimplement them. Why callbacks rather than a shared registry: the strategies depend on the CLI's `HeritageMap` + `SemanticModel`. Migrating both to `gitnexus-shared` is out of scope for #914; callbacks let the shared build stay pure. Module placement: `gitnexus-shared/src/scope-resolution/method-dispatch-index.ts` for consistency with the other RFC §3.1 indexes (#913 DefIndex / ModuleScopeIndex / QualifiedNameIndex; #916 resolveTypeRef). Safety surface mirrors sibling indexes: - First-write-wins on duplicate owners. - Repeated (interface, owner) pairs deduplicated. - Stored arrays are `Object.freeze`d; caller mutation of the source array does not leak into the index. - Miss returns a shared frozen empty array. Tests (19, all passing): empty input, single-inheritance chain, Python C3 diamond, Java BFS, Ruby kind-aware mixin, Rust qualified-syntax empty, interface inversion (single, multiple, ordered), dedup within and across callback calls, frozen miss + bucket arrays, callback-array isolation, readonly Map iteration. Closes part of #909.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@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 27s —— View job PR Review: feat(shared): MethodDispatchIndex #960Tasks
Production-ready bar for this PRBased on AGENTS.md, GUARDRAILS.md, CONTRIBUTING.md, TESTING.md, ARCHITECTURE.md, and the affected area (
Findings1. Correctness & functional completeness
Verified:
2. Code clarity & clean code
Verified against repo conventions:
NIT: C3 and BFS linearization tests are identical
3. Test coverage & change safety
Verified:
NON-BLOCKING:
|
| Check | Status |
|---|---|
| No secrets / tokens / machine paths | ✅ |
| No blind find-and-replace renames | ✅ (new file only) |
| Upstream impact considered | ✅ (new symbols, no existing code modified) |
| No HIGH/CRITICAL unaddressed | ✅ |
| PR title conventional commit format | ✅ feat(shared): |
| PolyForm license untouched | ✅ |
| No language-specific code in shared ingestion | ✅ (callbacks keep strategies CLI-local) |
RFC #909 Ring 2 tracker
| Issue | Description | Status |
|---|---|---|
| #910 | scope-resolution types + constants | ✅ merged (#949) |
| #911 | LanguageProvider scope-resolution hooks | ✅ merged (#950) |
| #913 | DefIndex / ModuleScopeIndex / QualifiedNameIndex | ✅ merged (#958) |
| #914 | MethodDispatchIndex | ← this PR |
| #916 | resolveTypeRef strict resolver | ✅ merged (#959) |
| #918 | shadow-mode diff + aggregate | ✅ merged (#951) |
| #917 | Registry.lookupMethod MRO fast path | 🔜 unblocked by this PR |
This PR is the last outstanding Ring 2 SHARED deliverable before #917 can proceed. The RFC is on track.
What I could not fully verify
tsc --noEmitclean: PR description asserts it passes for bothgitnexus-sharedandgitnexus. Cannot run locally without installing deps, but the TypeScript is structurally sound — no type errors are visible in the diff.- Combined 170/170 test pass: PR description asserts this. Cannot verify CI results from the diff alone.
implementsOfis never called with a duplicate owner whencomputeMrofirst-write-wins guards apply — this asymmetry exists by design (dedup handles the output), but the call-count behavior is unverified in tests (see NON-BLOCKING finding above).
Final verdict
✅ Ready to merge — no blocking issues.
The implementation is correct, the safety surface matches sibling indexes precisely, the test suite is thorough, and the RFC Ring 2 tracker is in good shape. The two findings (NON-BLOCKING + NIT) are worth addressing in a follow-up or pre-merge if time permits, but neither affects correctness or safety.
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 6602 tests passed 97 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
…bhigyanpatwari#914, RFC abhigyanpatwari#909 Ring 2 SHARED) (abhigyanpatwari#960) Implements RFC §3.1 `MethodDispatchIndex`: a two-way materialized view keyed by `DefId` for O(1) method-dispatch resolution: - `mroByOwnerDefId` — owner class → full MRO ancestor chain (excludes self, per-language strategy order) - `implsByInterfaceDefId` — interface/trait → classes that implement it **Not an MRO implementation.** `buildMethodDispatchIndex` is a pure aggregator that calls back into caller-provided `computeMro` and `implementsOf` functions. The five existing strategies (Python C3, Ruby kind-aware, Java/Kotlin linear, Rust qualified-syntax, COBOL none) stay where they are today (`model/resolve.ts`, `languages/ruby.ts`); this index does not reimplement them. Why callbacks rather than a shared registry: the strategies depend on the CLI's `HeritageMap` + `SemanticModel`. Migrating both to `gitnexus-shared` is out of scope for abhigyanpatwari#914; callbacks let the shared build stay pure. Module placement: `gitnexus-shared/src/scope-resolution/method-dispatch-index.ts` for consistency with the other RFC §3.1 indexes (abhigyanpatwari#913 DefIndex / ModuleScopeIndex / QualifiedNameIndex; abhigyanpatwari#916 resolveTypeRef). Safety surface mirrors sibling indexes: - First-write-wins on duplicate owners. - Repeated (interface, owner) pairs deduplicated. - Stored arrays are `Object.freeze`d; caller mutation of the source array does not leak into the index. - Miss returns a shared frozen empty array. Tests (19, all passing): empty input, single-inheritance chain, Python C3 diamond, Java BFS, Ruby kind-aware mixin, Rust qualified-syntax empty, interface inversion (single, multiple, ordered), dedup within and across callback calls, frozen miss + bucket arrays, callback-array isolation, readonly Map iteration. Closes part of abhigyanpatwari#909.
Closes #914.
Scope
Implements RFC §3.1
MethodDispatchIndex: a two-way,DefId-keyed materialized view for O(1) method dispatch.gitnexus-shared/src/scope-resolution/method-dispatch-index.tsgitnexus/test/unit/scope-resolution/method-dispatch-index.test.ts(19 tests)What it provides
mroByOwnerDefId— owner class → full MRO ancestor chain (excludes self, in per-language strategy order).implsByInterfaceDefId— interface/trait → classes that implement it.Why callbacks, not a shared strategy registry
The five existing MRO strategies (Python C3, Ruby kind-aware, Java/Kotlin linear, Rust qualified-syntax, COBOL none) live in the CLI package and depend on CLI-local `HeritageMap` + `SemanticModel`. Migrating them to `gitnexus-shared` would require pulling both — out of scope for #914.
`buildMethodDispatchIndex` instead takes `computeMro` / `implementsOf` callbacks and inverts + materializes. No MRO logic reimplemented.
Safety surface (matches sibling indexes)
Tests (19, all passing)
Verification
Module placement
`gitnexus-shared/src/scope-resolution/method-dispatch-index.ts` — keeps all RFC §3.1 indexes under one folder alongside `def-index`, `module-scope-index`, `qualified-name-index`, and `resolve-type-ref`. Minor deviation from the issue's suggested `gitnexus-shared/src/method-dispatch-index.ts` for consistency.
Part of