From c6ed890a14932f74dc65911896c39b417728a653 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Sat, 18 Apr 2026 18:09:57 +0100 Subject: [PATCH 1/2] chore(shared): apply Ring 2 SHARED review follow-ups in one diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Aggregates all actionable follow-ups from the 9 Ring 2 SHARED PRs (#949–#963) before proceeding to Ring 2 PKG. No behavior changes; docstring edits, test refinements, and one structural cleanup. ## #913 (DefIndex / ModuleScopeIndex / QualifiedNameIndex) - Rename `freezeIndex` → `wrapIndex` across all three index builders. The old name implied `Object.freeze` on the wrapper, which we never applied; `wrapIndex` more accurately describes the lightweight readonly-interface wrap. Safety surface (frozen bucket arrays, frozen miss-empty array, readonly Maps) is unchanged. - Document in `buildModuleScopeIndex` JSDoc that callers must pre-normalize `filePath` keys (no path-separator canonicalization happens here). Prevents silent cross-platform misses. - Add an explicit hit-path freeze assertion in `qualified-name-index.test.ts` (the existing test covered only the miss-path `EMPTY` array). ## #914 (MethodDispatchIndex) - Differentiate the C3 and BFS test cases: both tests now use distinct MRO orderings so they prove the materializer stores whatever order the `computeMro` callback produces (not that C3 and BFS yield identical output). - Add `implementsOfCalls` counter in the first-write-wins test, and document the call-count contract in `MethodDispatchInput.implementsOf` JSDoc: `implementsOf` fires **per occurrence** in `input.owners` (not per unique owner); `computeMro` fires at most once per unique owner. Callers with expensive `implementsOf` implementations should pre-dedupe `owners`. ## #916 (resolveTypeRef) - Document the deliberate exclusion of `'Type'` from `TYPE_KINDS` (verified no extractor in `gitnexus/src/core/ingestion/` emits `type: 'Type'` for annotation-relevant symbols). - Rename the namespace-origin test from `'resolves ...'` to `'returns null for a namespace-origin binding whose def is not a type kind'`, matching the failure-case intent. ## #918 (shadow diff + aggregate) - Remove the partial re-export `export type { ShadowAgreement, ShadowDiff };` from `aggregate.ts` — it omitted `ShadowCallsite` and diverged from the top-level barrel. Consumers import all three from the `gitnexus-shared` entry point. - Fix the invalid `'wildcard'` evidence kind in `diff.test.ts` fixture (that kind is not a valid `ResolutionEvidence.kind`). Replaced with `'global-name'`, a real kind the test treats identically. ## #912 (ScopeTree / PositionIndex / makeScopeId) - Document the touching-boundary semantics on `PositionIndex.atPosition`: when siblings share a boundary point, the right (later-start) sibling wins per the existing innermost-wins sort contract. - Resolve the layer-inversion flagged by review: move `ScopeLookup` from `resolve-type-ref.ts` to `types.ts` (its natural home in the data-model layer). `scope-tree.ts` now imports `ScopeLookup` from `types.js` directly; the old re-export from `resolve-type-ref.ts` is removed per repo convention (`feedback_no_reexport`). Barrel export moved alongside. ## #917 (ClassRegistry / MethodRegistry / FieldRegistry) - Replace the dangling "try a name-match among class-like defs" comment in `lookupReceiverType` with explicit prose that callers must pre-resolve via `resolveTypeRef` if they want richer semantics. No behavior change — the function already returned `undefined` on ambiguous/missing qnames. - Fix `tieBreakKey.origin` default for pure Step-2 candidates. Type-binding-only hits no longer falsely inherit `'local'` from `ensureCandidate`'s neutral default; they now demote to `'import'` on their first type-binding hit, and only a later Step-1 lexical hit can upgrade them back to `'local'`. Keeps the Appendix B cascade faithful to the true origin. - Document `'global-name'` in `evidence.ts`: currently reserved for Ring 3's byName global index; `lookupCore` never emits it today. The weight stays live so `composeEvidence` remains exhaustive over the origin union. - Rename the mislabeled Step-7 test from `'confidence DESC is the primary key'` (which actually tested hard-shadow baseline) to `'inner scope shadows outer, yielding single result'`, and add a separate test that actually exercises multi-candidate confidence ordering (local vs wildcard at the same scope). ## Verification - `tsc --noEmit` clean (both `gitnexus-shared` and `gitnexus`) - `gitnexus-shared` build clean - Combined scope-resolution / model / shadow suite: **260/260 pass** (+1 from the new multi-candidate ordering test in #917) ## Not addressed (non-actionable) - #949 CI "failure with zero failing tests": pre-existing Swift Node 22 grammar flake unrelated to #910 scope. - #950: the two non-blocking findings were already addressed in follow-up commit `cbac32ba` (ParsedImport discriminated union + `ScopeId | null` on the two hooks). - #915: the five in-scope findings were already addressed in follow-up commit `54515a7e` (dead code, unused params, multi-hop docs, cap-hit test, stats granularity). - #915 LanguageProvider.resolveImportTarget signature divergence + `findDefById` O(F×D) perf: tracked separately as follow-up issues for the Ring 3 migration window. --- gitnexus-shared/src/index.ts | 5 ++- .../src/scope-resolution/def-index.ts | 4 +- .../scope-resolution/method-dispatch-index.ts | 8 ++++ .../scope-resolution/module-scope-index.ts | 12 +++++- .../src/scope-resolution/position-index.ts | 10 +++++ .../scope-resolution/qualified-name-index.ts | 4 +- .../scope-resolution/registries/evidence.ts | 5 +++ .../registries/lookup-core.ts | 20 +++++++--- .../src/scope-resolution/resolve-type-ref.ts | 18 ++++----- .../src/scope-resolution/scope-tree.ts | 3 +- .../src/scope-resolution/shadow/aggregate.ts | 2 - gitnexus-shared/src/scope-resolution/types.ts | 12 ++++++ .../method-dispatch-index.test.ts | 37 ++++++++++++++----- .../qualified-name-index.test.ts | 3 ++ .../unit/scope-resolution/registries.test.ts | 28 +++++++++++++- .../scope-resolution/resolve-type-ref.test.ts | 8 ++-- gitnexus/test/unit/shadow/diff.test.ts | 6 ++- 17 files changed, 141 insertions(+), 44 deletions(-) diff --git a/gitnexus-shared/src/index.ts b/gitnexus-shared/src/index.ts index 255aa9345e..f036637395 100644 --- a/gitnexus-shared/src/index.ts +++ b/gitnexus-shared/src/index.ts @@ -48,6 +48,7 @@ export type { ParsedTypeBinding, WorkspaceIndex, Callsite, + ScopeLookup, } from './scope-resolution/types.js'; // Evidence + tie-break constants (RFC Appendix A, Appendix B) @@ -71,8 +72,10 @@ export { buildQualifiedNameIndex } from './scope-resolution/qualified-name-index export type { QualifiedNameIndex } from './scope-resolution/qualified-name-index.js'; // Strict type-reference resolver (RFC §4.6; Ring 2 SHARED #916) +// `ScopeLookup` now lives in `./scope-resolution/types.js` (canonical); +// it is re-exported there — no separate export from this module. export { resolveTypeRef } from './scope-resolution/resolve-type-ref.js'; -export type { ResolveTypeRefContext, ScopeLookup } from './scope-resolution/resolve-type-ref.js'; +export type { ResolveTypeRefContext } from './scope-resolution/resolve-type-ref.js'; // Method-dispatch materialized view over HeritageMap (RFC §3.1; Ring 2 SHARED #914) export { buildMethodDispatchIndex } from './scope-resolution/method-dispatch-index.js'; diff --git a/gitnexus-shared/src/scope-resolution/def-index.ts b/gitnexus-shared/src/scope-resolution/def-index.ts index bc27f773ed..a34eab961c 100644 --- a/gitnexus-shared/src/scope-resolution/def-index.ts +++ b/gitnexus-shared/src/scope-resolution/def-index.ts @@ -41,12 +41,12 @@ export function buildDefIndex(defs: readonly SymbolDefinition[]): DefIndex { if (byId.has(def.nodeId)) continue; // first-write-wins byId.set(def.nodeId, def); } - return freezeIndex(byId); + return wrapIndex(byId); } // ─── Internal ─────────────────────────────────────────────────────────────── -function freezeIndex(byId: Map): DefIndex { +function wrapIndex(byId: Map): DefIndex { return { byId, get size() { diff --git a/gitnexus-shared/src/scope-resolution/method-dispatch-index.ts b/gitnexus-shared/src/scope-resolution/method-dispatch-index.ts index 050538db57..ad625c807d 100644 --- a/gitnexus-shared/src/scope-resolution/method-dispatch-index.ts +++ b/gitnexus-shared/src/scope-resolution/method-dispatch-index.ts @@ -71,6 +71,14 @@ export interface MethodDispatchInput { * returned. * * Repeated IDs in the output are deduplicated automatically. + * + * **Call-count contract.** `implementsOf` is invoked **once per + * occurrence** of an owner in `input.owners`, not once per unique + * owner. Duplicate owners therefore re-invoke it; dedup happens at + * the bucket layer (after the callback returns). Callers with + * expensive `implementsOf` implementations should pass a deduplicated + * `owners` list. `computeMro`, by contrast, is memoized by the first- + * write-wins policy and fires at most once per unique owner. */ readonly implementsOf: (ownerDefId: DefId) => readonly DefId[]; } diff --git a/gitnexus-shared/src/scope-resolution/module-scope-index.ts b/gitnexus-shared/src/scope-resolution/module-scope-index.ts index a71c5d3d74..a57c02d27b 100644 --- a/gitnexus-shared/src/scope-resolution/module-scope-index.ts +++ b/gitnexus-shared/src/scope-resolution/module-scope-index.ts @@ -36,6 +36,14 @@ export interface ModuleScopeEntry { * entry preserves the first-stable id the rest of the pipeline may already * have registered against. * + * **Caller contract: filePath keys must be pre-normalized.** This index + * keys on the raw `filePath` string and does NOT canonicalize separators, + * case, or trailing slashes. Callers upstream of this function must agree + * on a canonical form (typically repo-root-relative, POSIX separators, + * no trailing slash) before constructing entries — otherwise `C:\foo\bar.ts`, + * `C:/foo/bar.ts`, and `foo/bar.ts` will all hash to distinct buckets and + * `get()` will miss. + * * Pure function — safe to call repeatedly; no side effects. */ export function buildModuleScopeIndex(entries: readonly ModuleScopeEntry[]): ModuleScopeIndex { @@ -44,12 +52,12 @@ export function buildModuleScopeIndex(entries: readonly ModuleScopeEntry[]): Mod if (byFilePath.has(filePath)) continue; // first-write-wins byFilePath.set(filePath, moduleScopeId); } - return freezeIndex(byFilePath); + return wrapIndex(byFilePath); } // ─── Internal ─────────────────────────────────────────────────────────────── -function freezeIndex(byFilePath: Map): ModuleScopeIndex { +function wrapIndex(byFilePath: Map): ModuleScopeIndex { return { byFilePath, get size() { diff --git a/gitnexus-shared/src/scope-resolution/position-index.ts b/gitnexus-shared/src/scope-resolution/position-index.ts index 82954a19ff..3e9b8be129 100644 --- a/gitnexus-shared/src/scope-resolution/position-index.ts +++ b/gitnexus-shared/src/scope-resolution/position-index.ts @@ -37,6 +37,16 @@ export interface PositionIndex { * Innermost scope containing `(line, col)` in `filePath`, or `undefined` * when nothing contains it (position before file start, after file end, * or filePath not indexed). + * + * **Touching-boundary semantics.** Ranges are inclusive on both ends. + * When two sibling scopes share a boundary point — e.g. + * `[5:0, 10:0]` and `[10:0, 15:0]`, which is legal under `ScopeTree`'s + * non-overlap invariant — a query at the shared point `(10, 0)` is + * contained by **both**. The innermost-wins tie-break rule applies as + * usual: since neither is nested inside the other, the one that + * **starts latest** wins, i.e. the **right** sibling. Queries at + * non-boundary positions between them naturally fall to the unique + * containing scope. */ atPosition(filePath: string, line: number, col: number): ScopeId | undefined; } diff --git a/gitnexus-shared/src/scope-resolution/qualified-name-index.ts b/gitnexus-shared/src/scope-resolution/qualified-name-index.ts index 64dbd9630e..e231b01d51 100644 --- a/gitnexus-shared/src/scope-resolution/qualified-name-index.ts +++ b/gitnexus-shared/src/scope-resolution/qualified-name-index.ts @@ -69,14 +69,14 @@ export function buildQualifiedNameIndex(defs: readonly SymbolDefinition[]): Qual frozen.set(k, Object.freeze(v.slice())); } - return freezeIndex(frozen); + return wrapIndex(frozen); } // ─── Internal ─────────────────────────────────────────────────────────────── const EMPTY: readonly DefId[] = Object.freeze([]); -function freezeIndex(byQualifiedName: Map): QualifiedNameIndex { +function wrapIndex(byQualifiedName: Map): QualifiedNameIndex { return { byQualifiedName, get size() { diff --git a/gitnexus-shared/src/scope-resolution/registries/evidence.ts b/gitnexus-shared/src/scope-resolution/registries/evidence.ts index bacf6c30d1..cabeb6a95f 100644 --- a/gitnexus-shared/src/scope-resolution/registries/evidence.ts +++ b/gitnexus-shared/src/scope-resolution/registries/evidence.ts @@ -168,6 +168,11 @@ function getOriginWeight(origin: NonNullable): number { case 'global-qualified': return EvidenceWeights.globalQualified; case 'global-name': + // Reserved for Ring 3 byName global index. `lookupCore` today only + // emits `'global-qualified'` (via `lookupQualified`, dotted-name + // fallback); no code path constructs `origin: 'global-name'` yet. + // Kept here so the Appendix A weight stays live and `composeEvidence` + // remains exhaustive over the origin union. return EvidenceWeights.globalName; } } diff --git a/gitnexus-shared/src/scope-resolution/registries/lookup-core.ts b/gitnexus-shared/src/scope-resolution/registries/lookup-core.ts index baea0d55ea..2551b9e6d0 100644 --- a/gitnexus-shared/src/scope-resolution/registries/lookup-core.ts +++ b/gitnexus-shared/src/scope-resolution/registries/lookup-core.ts @@ -318,8 +318,9 @@ function lookupReceiverType( // callers pre-resolve if they want the richer semantics. const candidateIds = ctx.qualifiedNames.get(typeRef.rawName); if (candidateIds.length === 1) return candidateIds[0]; - // If ambiguous or missing, try a name-match among class-like defs — - // but only when the rawName has no dots (simple name). + // Ambiguous (≥ 2) or missing (0) — caller must pre-resolve via + // `resolveTypeRef` (#916) if they want the richer semantics. We + // intentionally do NOT re-implement a simple-name fallback here. return undefined; } currentId = scope.parent; @@ -358,17 +359,24 @@ function recordTypeBindingHit( receiverOwner: DefId, ): void { const state = ensureCandidate(perCandidate, def); + const firstHit = state.signals.typeBindingMroDepth === undefined; // Only replace if this hit is shallower (smaller MRO depth). - if ( - state.signals.typeBindingMroDepth === undefined || - mroDepth < state.signals.typeBindingMroDepth - ) { + if (firstHit || mroDepth < state.signals.typeBindingMroDepth!) { state.signals.typeBindingMroDepth = mroDepth; state.tieBreakKey.mroDepth = mroDepth; } if (def.ownerId === receiverOwner) { state.signals.ownerMatch = true; } + // Pure type-binding candidates (no lexical hit) would otherwise keep the + // `ensureCandidate` default `tieBreakKey.origin === 'local'`, making the + // Appendix B cascade lump them with local-origin candidates. Demote them + // to `'import'` — the strongest non-local origin — only on the FIRST + // type-binding hit, so a later lexical Step 1 walk can still upgrade + // them back to `'local'` via `recordLexicalHit`. + if (firstHit && state.signals.origin === undefined) { + state.tieBreakKey.origin = 'import'; + } } // ─── Step 3 implementation ───────────────────────────────────────────────── diff --git a/gitnexus-shared/src/scope-resolution/resolve-type-ref.ts b/gitnexus-shared/src/scope-resolution/resolve-type-ref.ts index 63bd6a8da2..2f8ba7bd53 100644 --- a/gitnexus-shared/src/scope-resolution/resolve-type-ref.ts +++ b/gitnexus-shared/src/scope-resolution/resolve-type-ref.ts @@ -38,22 +38,12 @@ import type { NodeLabel } from '../graph/types.js'; import type { SymbolDefinition } from './symbol-definition.js'; -import type { BindingRef, Scope, ScopeId, TypeRef } from './types.js'; +import type { BindingRef, ScopeId, ScopeLookup, TypeRef } from './types.js'; import type { DefIndex } from './def-index.js'; import type { QualifiedNameIndex } from './qualified-name-index.js'; // ─── Public contracts ─────────────────────────────────────────────────────── -/** - * Minimal scope-lookup contract required by `resolveTypeRef`. Implemented by - * the `ScopeTree` from #912; declared here so #916 can ship as a standalone - * piece without a hard dependency on the full scope-tree implementation. Any - * structure that hands back a `Scope` by `ScopeId` satisfies this contract. - */ -export interface ScopeLookup { - getScope(id: ScopeId): Scope | undefined; -} - /** * All inputs `resolveTypeRef` needs from the semantic model. Bundled into a * context object so the call site stays short and the interface is stable as @@ -83,6 +73,12 @@ const STRICT_ORIGINS: ReadonlySet = new Set = new Set([ 'Class', diff --git a/gitnexus-shared/src/scope-resolution/scope-tree.ts b/gitnexus-shared/src/scope-resolution/scope-tree.ts index 705dfb731e..0b030abee3 100644 --- a/gitnexus-shared/src/scope-resolution/scope-tree.ts +++ b/gitnexus-shared/src/scope-resolution/scope-tree.ts @@ -25,8 +25,7 @@ * `Object.freeze`d; miss lookups return a shared frozen empty array. */ -import type { Scope, ScopeId, Range } from './types.js'; -import type { ScopeLookup } from './resolve-type-ref.js'; +import type { Scope, ScopeId, ScopeLookup, Range } from './types.js'; // ─── Public contract ──────────────────────────────────────────────────────── diff --git a/gitnexus-shared/src/scope-resolution/shadow/aggregate.ts b/gitnexus-shared/src/scope-resolution/shadow/aggregate.ts index 08ff253227..88e19d7537 100644 --- a/gitnexus-shared/src/scope-resolution/shadow/aggregate.ts +++ b/gitnexus-shared/src/scope-resolution/shadow/aggregate.ts @@ -181,5 +181,3 @@ function buildOverallRow( const parity = resolved > 0 ? bothAgree / resolved : 0; return { totalCalls, bothAgree, onlyLegacy, onlyNew, bothDisagree, bothEmpty, parity }; } - -export type { ShadowAgreement, ShadowDiff }; diff --git a/gitnexus-shared/src/scope-resolution/types.ts b/gitnexus-shared/src/scope-resolution/types.ts index 9a08a3a381..3e16115939 100644 --- a/gitnexus-shared/src/scope-resolution/types.ts +++ b/gitnexus-shared/src/scope-resolution/types.ts @@ -209,6 +209,18 @@ export type WorkspaceIndex = unknown; // The former opaque placeholder lived here during Ring 1; removed now that // the concrete type exists. Consumers import from `gitnexus-shared` directly. +/** + * Minimal scope-lookup contract: map a `ScopeId` back to its `Scope` record. + * + * Lives in the data-model layer so both `ScopeTree` (§3.1) and + * `resolveTypeRef` / `Registry.lookup` (§4) can depend on it without + * inverting each other. `ScopeTree` is the canonical implementation; + * tests and future alternative containers may supply their own. + */ +export interface ScopeLookup { + getScope(id: ScopeId): Scope | undefined; +} + /** Call-site description passed to `arityCompatibility`. */ export interface Callsite { /** Number of arguments at the call site. */ diff --git a/gitnexus/test/unit/scope-resolution/method-dispatch-index.test.ts b/gitnexus/test/unit/scope-resolution/method-dispatch-index.test.ts index bc4f758120..386474723f 100644 --- a/gitnexus/test/unit/scope-resolution/method-dispatch-index.test.ts +++ b/gitnexus/test/unit/scope-resolution/method-dispatch-index.test.ts @@ -59,8 +59,9 @@ describe('buildMethodDispatchIndex', () => { }); it('records a C3 linearization verbatim (Python diamond)', () => { - // D(B, C) where B(A), C(A). Classical C3: D, B, C, A. - // Our index stores mro excluding self: [B, C, A]. + // D(B, C) where B(A), C(A). Classical C3 keeps A last because the + // merge step defers A until both B and C have been emitted. + // Our index stores MRO excluding self: [B, C, A]. const idx = buildMethodDispatchIndex( input(['def:D'], { 'def:D': ['def:B', 'def:C', 'def:A'] }), ); @@ -68,11 +69,15 @@ describe('buildMethodDispatchIndex', () => { }); it('records a BFS linearization verbatim (Java-style first-wins)', () => { - // D extends B, C; B extends A; C extends A. BFS: B, C, A. + // Same class hierarchy as the C3 case, but the BFS walker visits + // A before C via the B→A edge. Expected MRO differs from C3: [B, A, C]. + // This test proves the materializer preserves whatever ordering the + // per-language `computeMro` callback produces — NOT that C3 and BFS + // produce identical output. const idx = buildMethodDispatchIndex( - input(['def:D'], { 'def:D': ['def:B', 'def:C', 'def:A'] }), + input(['def:D'], { 'def:D': ['def:B', 'def:A', 'def:C'] }), ); - expect(idx.mroFor('def:D')).toEqual(['def:B', 'def:C', 'def:A']); + expect(idx.mroFor('def:D')).toEqual(['def:B', 'def:A', 'def:C']); }); it('records a Ruby-style kind-aware ancestry verbatim', () => { @@ -133,9 +138,19 @@ describe('buildMethodDispatchIndex', () => { it('deduplicates when the same owner is listed in `owners` twice (first-write-wins)', () => { // First-write-wins parity with sibling indexes; subsequent owner entries - // should not re-invoke callbacks for existing MRO, and should not create - // duplicate implementor entries. + // should not re-invoke `computeMro` for existing MRO, and should not + // create duplicate implementor entries. + // + // NOTE on `implementsOf` call count: the builder calls `implementsOf` + // ONCE PER OCCURRENCE of an owner in `input.owners`, not once per + // unique owner. Duplicate owners therefore re-invoke `implementsOf`; + // the dedup lives at the bucket layer (via `implsSeen`), not the + // callback layer. Callers with expensive `implementsOf` callbacks + // should dedupe `input.owners` upfront. This counter assertion pins + // that contract so a future refactor can't silently collapse the + // second call without updating the docstring. let mroCalls = 0; + let implementsOfCalls = 0; const impls: Record = { 'def:A': ['def:I'] }; const idx = buildMethodDispatchIndex({ owners: ['def:A', 'def:A'], @@ -143,9 +158,13 @@ describe('buildMethodDispatchIndex', () => { mroCalls++; return ['def:B']; }, - implementsOf: (o) => impls[o] ?? [], + implementsOf: (o) => { + implementsOfCalls++; + return impls[o] ?? []; + }, }); - expect(mroCalls).toBe(1); + expect(mroCalls).toBe(1); // MRO dedup is at the callback layer (first-write-wins) + expect(implementsOfCalls).toBe(2); // implementsOf fires per occurrence; dedup at bucket expect(idx.mroFor('def:A')).toEqual(['def:B']); expect(idx.implementorsOf('def:I')).toEqual(['def:A']); }); diff --git a/gitnexus/test/unit/scope-resolution/qualified-name-index.test.ts b/gitnexus/test/unit/scope-resolution/qualified-name-index.test.ts index 77fc5ecc76..a9cbced57c 100644 --- a/gitnexus/test/unit/scope-resolution/qualified-name-index.test.ts +++ b/gitnexus/test/unit/scope-resolution/qualified-name-index.test.ts @@ -47,6 +47,9 @@ describe('buildQualifiedNameIndex', () => { }); const idx = buildQualifiedNameIndex([a, b]); expect(idx.get('app.User')).toEqual(['def:app.User:Core', 'def:app.User:Api']); + // Hit-path bucket is frozen just like the miss path — consumers cannot + // mutate the returned array. + expect(() => (idx.get('app.User') as unknown as string[]).push('x')).toThrow(); }); it('preserves input order in the bucket', () => { diff --git a/gitnexus/test/unit/scope-resolution/registries.test.ts b/gitnexus/test/unit/scope-resolution/registries.test.ts index d47badc786..803c4eafd3 100644 --- a/gitnexus/test/unit/scope-resolution/registries.test.ts +++ b/gitnexus/test/unit/scope-resolution/registries.test.ts @@ -337,7 +337,11 @@ describe('Step 6: global-qualified fallback', () => { // ─── §4.2 Step 7 — tie-breaks ────────────────────────────────────────────── describe('Step 7: tie-break cascade', () => { - it('confidence DESC is the primary key', () => { + it('inner scope shadows outer, yielding single result (hard-shadow baseline)', () => { + // Baseline: the hard-shadow rule in Step 1 means a near binding fully + // replaces the far one. No "confidence DESC" ordering to observe here + // because there is only one candidate — the far class never enters + // the result set. See the next test for true multi-candidate ranking. const nearClass = mkDef({ nodeId: 'def:near', type: 'Class' }); const farClass = mkDef({ nodeId: 'def:far', type: 'Class' }); const mod = mkScope({ @@ -355,11 +359,31 @@ describe('Step 7: tie-break cascade', () => { const ctx = makeCtx([mod, fn], [nearClass, farClass]); const results = buildClassRegistry(ctx).lookup('User', 'scope:f'); - // Inner binding shadows; only the near class should appear. expect(results).toHaveLength(1); expect(results[0]!.def).toBe(nearClass); }); + it('orders multiple same-scope candidates by confidence DESC', () => { + // Two candidates co-exist at the same scope, one with origin=local + // (weight 0.55) and one with origin=wildcard (weight 0.30). Both pass + // the Class kind filter; confidence DESC should sort local first. + const localClass = mkDef({ nodeId: 'def:local', type: 'Class' }); + const wildcardClass = mkDef({ nodeId: 'def:wildcard', type: 'Class' }); + const mod = mkScope({ + id: 'scope:m', + parent: null, + bindings: { + User: [mkBinding(wildcardClass, 'wildcard'), mkBinding(localClass, 'local')], + }, + }); + const ctx = makeCtx([mod], [localClass, wildcardClass]); + const results = buildClassRegistry(ctx).lookup('User', 'scope:m'); + expect(results).toHaveLength(2); + expect(results[0]!.def).toBe(localClass); // local (0.55) > wildcard (0.30) + expect(results[1]!.def).toBe(wildcardClass); + expect(results[0]!.confidence).toBeGreaterThan(results[1]!.confidence); + }); + it('breaks ties by DefId.localeCompare when all secondary keys are equal', () => { const a = mkDef({ nodeId: 'def:aaa', type: 'Class' }); const b = mkDef({ nodeId: 'def:bbb', type: 'Class' }); diff --git a/gitnexus/test/unit/scope-resolution/resolve-type-ref.test.ts b/gitnexus/test/unit/scope-resolution/resolve-type-ref.test.ts index 2e2c082631..3b7573d53c 100644 --- a/gitnexus/test/unit/scope-resolution/resolve-type-ref.test.ts +++ b/gitnexus/test/unit/scope-resolution/resolve-type-ref.test.ts @@ -139,16 +139,16 @@ describe('resolveTypeRef', () => { expect(resolveTypeRef(typeRef('Account', 'scope:module'), ctx)).toBe(userClass); }); - it('resolves a namespace-origin binding (e.g., `import * as np`)', () => { + it('returns null for a namespace-origin binding whose def is not a type kind', () => { const numpyMod = mkDef({ nodeId: 'def:numpy-mod', type: 'Namespace' }); // A namespace binding must resolve to a type-kind def to satisfy strict - // mode. Here the binding is the namespace module itself — treat it as a - // shadowing non-type and expect null. + // mode. Here the binding is the namespace module itself — `Namespace` + // is intentionally NOT in `TYPE_KINDS` (see resolve-type-ref.ts), so + // the binding is treated as a shadowing non-type and we fail fast. const moduleScope = mkScope('scope:module', null, { np: [mkBinding(numpyMod, 'namespace')], }); const ctx = mkCtx([moduleScope], [numpyMod]); - // Namespace is NOT a type kind → strict returns null. expect(resolveTypeRef(typeRef('np', 'scope:module'), ctx)).toBeNull(); }); diff --git a/gitnexus/test/unit/shadow/diff.test.ts b/gitnexus/test/unit/shadow/diff.test.ts index 700f08b914..e9baa606fa 100644 --- a/gitnexus/test/unit/shadow/diff.test.ts +++ b/gitnexus/test/unit/shadow/diff.test.ts @@ -160,7 +160,11 @@ describe('diffResolutions — metadata + ordering', () => { ]; const next = [ makeResolution('def:User.save', ['local']), - makeResolution('def:yet-another', ['wildcard']), + // The 2nd entry is here to verify index-0 isolation — the only kind + // requirement is that it be a valid `ResolutionEvidence.kind` so the + // fixture is type-correct. `'global-name'` is a real kind that + // `diffResolutions` never treats specially. + makeResolution('def:yet-another', ['global-name']), ]; const result = diffResolutions(callsite, legacy, next); expect(result.agreement).toBe('both-agree'); From a9fef0b78a20010b40f1fcb7d1b5ec55d8b6abc8 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Sat, 18 Apr 2026 18:27:15 +0100 Subject: [PATCH 2/2] chore(shared): address ce:review findings on the follow-up diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ce:review (interactive) on PR #964 surfaced two P2s and several P3s. This commit applies all `safe_auto` fixes + both manual tests in-line so the PR ships with a cleaner review trail. ## P2 fixes - **Complete `freezeIndex` → `wrapIndex` rename.** The prior commit renamed 3 of 5 sibling index files; `method-dispatch-index.ts` and `position-index.ts` still carried the old name. Now all 5 helpers use the consistent `wrapIndex` naming. (maintainability + project-standards reviewers both flagged this.) - **Add regression tests for the `recordTypeBindingHit` origin demotion.** The prior commit introduced the `tieBreakKey.origin = 'import'` demotion for Step-2-only candidates without a direct test. Added: - `registries.test.ts`: two Step-2-only siblings under the same interface, asserting deterministic DefId.localeCompare tie-break AND the stronger invariant that composeEvidence never emits a where-found signal for Step-2-only candidates (no `signals.origin`). - `position-index.test.ts`: touching-boundary test proving the right-sibling-wins rule documented in the new JSDoc. (testing + kieran-typescript + api-contract reviewers all flagged these gaps.) ## P3 fixes - Fix wrong comment in `recordTypeBindingHit` that claimed Step 1 could later upgrade a demoted origin. Step 1 runs BEFORE Step 2 — the actual upgrade path is Step 3 (`seedFromOwnerScopedContributor`). Comment now describes execution order correctly. - Fix inaccurate "re-exported there" comment in `index.ts`. `types.ts` *defines* ScopeLookup natively; it's not a re-export. Phrasing now says "defined in types.ts and exported from the type-export block above — not from this module." - Update stale `scope-tree.ts` file-header prose that still referenced `ScopeLookup` as living in #916/resolve-type-ref.ts. Now points to `./types.js` with a cross-ref to both #916 and #917 consumers. - Expand `atPosition` touching-boundary JSDoc to name the mechanism (backward scan through start-sorted array) so readers can trace the binary-search code to the claim. - Add breadcrumb to `aggregate.ts` module header pointing future readers to `./diff.ts` / the top-level barrel for `ShadowAgreement`, `ShadowCallsite`, and `ShadowDiff`. - Remove unnecessary non-null assertion in `recordTypeBindingHit`. Local `const existingMroDepth = ...` lets TS narrow to `number` in the else-branch, eliminating the `!` without behavior change. ## Verification - `tsc --noEmit` clean (both `gitnexus-shared` and `gitnexus`) - `gitnexus-shared` build clean - Combined scope-resolution / model / shadow suite: **262/262 pass** (+2 from the new origin-demotion + touching-boundary regression tests) --- gitnexus-shared/src/index.ts | 4 +- .../scope-resolution/method-dispatch-index.ts | 4 +- .../src/scope-resolution/position-index.ts | 12 +++-- .../registries/lookup-core.ts | 18 ++++--- .../src/scope-resolution/scope-tree.ts | 5 +- .../src/scope-resolution/shadow/aggregate.ts | 5 ++ .../scope-resolution/position-index.test.ts | 18 +++++++ .../unit/scope-resolution/registries.test.ts | 53 +++++++++++++++++++ 8 files changed, 102 insertions(+), 17 deletions(-) diff --git a/gitnexus-shared/src/index.ts b/gitnexus-shared/src/index.ts index f036637395..beeb53587e 100644 --- a/gitnexus-shared/src/index.ts +++ b/gitnexus-shared/src/index.ts @@ -72,8 +72,8 @@ export { buildQualifiedNameIndex } from './scope-resolution/qualified-name-index export type { QualifiedNameIndex } from './scope-resolution/qualified-name-index.js'; // Strict type-reference resolver (RFC §4.6; Ring 2 SHARED #916) -// `ScopeLookup` now lives in `./scope-resolution/types.js` (canonical); -// it is re-exported there — no separate export from this module. +// `ScopeLookup` is defined in `./scope-resolution/types.js` and exported +// from the type-export block above — not from this module. export { resolveTypeRef } from './scope-resolution/resolve-type-ref.js'; export type { ResolveTypeRefContext } from './scope-resolution/resolve-type-ref.js'; diff --git a/gitnexus-shared/src/scope-resolution/method-dispatch-index.ts b/gitnexus-shared/src/scope-resolution/method-dispatch-index.ts index ad625c807d..d09e8fa89f 100644 --- a/gitnexus-shared/src/scope-resolution/method-dispatch-index.ts +++ b/gitnexus-shared/src/scope-resolution/method-dispatch-index.ts @@ -121,14 +121,14 @@ export function buildMethodDispatchIndex(input: MethodDispatchInput): MethodDisp implsByInterfaceDefId.set(ifaceId, Object.freeze(owners.slice())); } - return freezeIndex(mroByOwnerDefId, implsByInterfaceDefId); + return wrapIndex(mroByOwnerDefId, implsByInterfaceDefId); } // ─── Internal ─────────────────────────────────────────────────────────────── const EMPTY: readonly DefId[] = Object.freeze([]); -function freezeIndex( +function wrapIndex( mroByOwnerDefId: Map, implsByInterfaceDefId: Map, ): MethodDispatchIndex { diff --git a/gitnexus-shared/src/scope-resolution/position-index.ts b/gitnexus-shared/src/scope-resolution/position-index.ts index 3e9b8be129..a2fd808287 100644 --- a/gitnexus-shared/src/scope-resolution/position-index.ts +++ b/gitnexus-shared/src/scope-resolution/position-index.ts @@ -44,9 +44,11 @@ export interface PositionIndex { * non-overlap invariant — a query at the shared point `(10, 0)` is * contained by **both**. The innermost-wins tie-break rule applies as * usual: since neither is nested inside the other, the one that - * **starts latest** wins, i.e. the **right** sibling. Queries at - * non-boundary positions between them naturally fall to the unique - * containing scope. + * **starts latest** wins, i.e. the **right** sibling. The mechanism + * is the backward scan through the start-position-sorted array (see + * `findLastStartLteIndex` below) — both siblings land before the + * upper-bound cursor, and the right sibling is scanned first. Queries at non-boundary positions between them naturally + * fall to the unique containing scope. */ atPosition(filePath: string, line: number, col: number): ScopeId | undefined; } @@ -79,7 +81,7 @@ export function buildPositionIndex(scopes: readonly Scope[]): PositionIndex { bucket.sort(compareEntry); } - return freezeIndex(entriesByFile, seen.size); + return wrapIndex(entriesByFile, seen.size); } // ─── Internals ────────────────────────────────────────────────────────────── @@ -138,7 +140,7 @@ function findLastStartLteIndex(arr: readonly Entry[], line: number, col: number) return lo - 1; } -function freezeIndex(entriesByFile: Map, size: number): PositionIndex { +function wrapIndex(entriesByFile: Map, size: number): PositionIndex { return { get size() { return size; diff --git a/gitnexus-shared/src/scope-resolution/registries/lookup-core.ts b/gitnexus-shared/src/scope-resolution/registries/lookup-core.ts index 2551b9e6d0..a18ad4930f 100644 --- a/gitnexus-shared/src/scope-resolution/registries/lookup-core.ts +++ b/gitnexus-shared/src/scope-resolution/registries/lookup-core.ts @@ -359,9 +359,12 @@ function recordTypeBindingHit( receiverOwner: DefId, ): void { const state = ensureCandidate(perCandidate, def); - const firstHit = state.signals.typeBindingMroDepth === undefined; - // Only replace if this hit is shallower (smaller MRO depth). - if (firstHit || mroDepth < state.signals.typeBindingMroDepth!) { + const existingMroDepth = state.signals.typeBindingMroDepth; + const firstHit = existingMroDepth === undefined; + // Only replace if this hit is shallower (smaller MRO depth). The local + // const lets TS narrow to `number` in the `else` branch so no `!` + // assertion is needed. + if (firstHit || mroDepth < existingMroDepth) { state.signals.typeBindingMroDepth = mroDepth; state.tieBreakKey.mroDepth = mroDepth; } @@ -371,9 +374,12 @@ function recordTypeBindingHit( // Pure type-binding candidates (no lexical hit) would otherwise keep the // `ensureCandidate` default `tieBreakKey.origin === 'local'`, making the // Appendix B cascade lump them with local-origin candidates. Demote them - // to `'import'` — the strongest non-local origin — only on the FIRST - // type-binding hit, so a later lexical Step 1 walk can still upgrade - // them back to `'local'` via `recordLexicalHit`. + // to `'import'` — the strongest non-local origin — only when no earlier + // phase set an origin for this candidate. Lexical hits from Step 1 set + // `signals.origin` before Step 2 runs, so the guard skips them; Step 3 + // (`seedFromOwnerScopedContributor`) runs AFTER Step 2 and unconditionally + // overrides `tieBreakKey.origin` back to `'local'` for direct-owner + // members, so any same-def overlap still ends up ranked correctly. if (firstHit && state.signals.origin === undefined) { state.tieBreakKey.origin = 'import'; } diff --git a/gitnexus-shared/src/scope-resolution/scope-tree.ts b/gitnexus-shared/src/scope-resolution/scope-tree.ts index 0b030abee3..7f2b546841 100644 --- a/gitnexus-shared/src/scope-resolution/scope-tree.ts +++ b/gitnexus-shared/src/scope-resolution/scope-tree.ts @@ -18,8 +18,9 @@ * pointers would be a category error — a `File` scope is not the * parent of another file's scopes; imports do that job.) * - * Satisfies the `ScopeLookup` contract from #916 (`resolve-type-ref`), so - * `resolveTypeRef` can take a `ScopeTree` directly without adapters. + * Satisfies the `ScopeLookup` contract (defined in `./types.js`), so + * `resolveTypeRef` (#916) and the scope-aware registries (#917) can take a + * `ScopeTree` directly without adapters. * * Immutable surface: `byId` is a `ReadonlyMap`; children arrays are * `Object.freeze`d; miss lookups return a shared frozen empty array. diff --git a/gitnexus-shared/src/scope-resolution/shadow/aggregate.ts b/gitnexus-shared/src/scope-resolution/shadow/aggregate.ts index 88e19d7537..27c24ff926 100644 --- a/gitnexus-shared/src/scope-resolution/shadow/aggregate.ts +++ b/gitnexus-shared/src/scope-resolution/shadow/aggregate.ts @@ -5,6 +5,11 @@ * Pure functions; no I/O. The harness persists per-run JSON; the dashboard * reads `.gitnexus/shadow-parity/latest.json` and renders. * + * Related types — `ShadowAgreement`, `ShadowCallsite`, `ShadowDiff` — are + * defined alongside `diffResolutions` in `./diff.ts` and re-exported + * through the top-level `gitnexus-shared` barrel. Consumers import all + * three from `gitnexus-shared`, not from this module. + * * Part of RFC #909 Ring 2 SHARED — #918. */ diff --git a/gitnexus/test/unit/scope-resolution/position-index.test.ts b/gitnexus/test/unit/scope-resolution/position-index.test.ts index 8c3a410b1f..ae037e4b28 100644 --- a/gitnexus/test/unit/scope-resolution/position-index.test.ts +++ b/gitnexus/test/unit/scope-resolution/position-index.test.ts @@ -135,6 +135,24 @@ describe('buildPositionIndex', () => { expect(idx.atPosition('a.ts', 30, 0)).toBe('scope:b'); expect(idx.atPosition('a.ts', 22, 0)).toBe('scope:mod'); // gap between siblings }); + + it('returns the right (later-start) sibling when two siblings share a boundary point', () => { + // Legal touching-boundary scenario per ScopeTree's non-overlap rule: + // [5:0..10:0] and [10:0..15:0] meet at (10, 0) but do not overlap + // (rangesOverlap treats end == start as "touches, not overlaps"). + // A query AT the shared point is contained by BOTH siblings; the + // innermost-wins comparator breaks the tie by start position ASC: + // the right sibling (starts at 10:0) is scanned first during the + // backward pass and wins. See `atPosition` JSDoc. + const idx = buildPositionIndex([ + mkScope('scope:mod', 'a.ts', 'Module', r(1, 0, 100, 0)), + mkScope('scope:left', 'a.ts', 'Block', r(5, 0, 10, 0), 'scope:mod'), + mkScope('scope:right', 'a.ts', 'Block', r(10, 0, 15, 0), 'scope:mod'), + ]); + expect(idx.atPosition('a.ts', 10, 0)).toBe('scope:right'); // shared boundary + expect(idx.atPosition('a.ts', 7, 0)).toBe('scope:left'); // inside left only + expect(idx.atPosition('a.ts', 12, 0)).toBe('scope:right'); // inside right only + }); }); describe('multi-file isolation', () => { diff --git a/gitnexus/test/unit/scope-resolution/registries.test.ts b/gitnexus/test/unit/scope-resolution/registries.test.ts index 803c4eafd3..b204287b54 100644 --- a/gitnexus/test/unit/scope-resolution/registries.test.ts +++ b/gitnexus/test/unit/scope-resolution/registries.test.ts @@ -557,6 +557,59 @@ describe('Step 2: type-binding + MRO walk', () => { expect(typeBinding?.weight).toBe(EvidenceWeights.typeBindingByMroDepth[0]); }); + it('demotes Step-2-only candidates to tieBreakKey.origin=import (pins rank vs same-origin siblings)', () => { + // Two method defs named `impl`, both owned by the same interface and + // both reached ONLY via the Step 2 type-binding MRO walk (no lexical + // binding). Each candidate's `recordTypeBindingHit` path demotes its + // `tieBreakKey.origin` from the `ensureCandidate` default `'local'` + // to `'import'`. With both at equal confidence (owner-match + type- + // binding at depth 0), the tie-break cascade must fall through + // scope-depth / MRO-depth / origin (all equal) to DefId.localeCompare. + // + // If the demotion regressed (e.g., tieBreakKey.origin left as `'local'`), + // both candidates would still share the same origin and this test would + // pass by coincidence — so the test ALSO asserts `signals.origin` is + // absent from the evidence list (no false where-found weight emitted), + // which is the strongest observable invariant the demotion guarantees. + const iface = mkDef({ + nodeId: 'def:Iface', + type: 'Interface', + qualifiedName: 'Iface', + }); + const implA = mkDef({ + nodeId: 'def:aaa.impl', + type: 'Method', + qualifiedName: 'Iface.impl', + ownerId: 'def:Iface', + }); + const implB = mkDef({ + nodeId: 'def:bbb.impl', + type: 'Method', + qualifiedName: 'Iface.impl', + ownerId: 'def:Iface', + }); + const scope = mkScope({ + id: 'scope:call', + parent: null, + typeBindings: { x: typeRef('Iface', 'scope:call') }, + }); + const ctx = makeCtx([scope], [iface, implA, implB]); + const results = buildMethodRegistry(ctx).lookup('impl', 'scope:call', { + explicitReceiver: { name: 'x' }, + }); + expect(results).toHaveLength(2); + // DefId.localeCompare: 'def:aaa.impl' < 'def:bbb.impl'. + expect(results[0]!.def).toBe(implA); + expect(results[1]!.def).toBe(implB); + // Demotion invariant: Step-2-only candidates have no `signals.origin`, + // so composeEvidence never emits a where-found signal for them. + for (const res of results) { + expect(evidenceOfKind(res, 'local')).toBeUndefined(); + expect(evidenceOfKind(res, 'import')).toBeUndefined(); + expect(evidenceOfKind(res, 'type-binding')).toBeDefined(); + } + }); + it('walks up the MRO when the method is declared on an ancestor', () => { const baseClass = mkDef({ nodeId: 'def:Base', type: 'Class', qualifiedName: 'Base' }); const derivedClass = mkDef({ nodeId: 'def:Derived', type: 'Class', qualifiedName: 'Derived' });