From a8cec54789b22168b749d16b3657b3547334c3f8 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Sat, 1 Aug 2026 06:09:45 +0000 Subject: [PATCH] refactor(resolution): collapse duplicated walkers, codec inverse, fold state (#2766) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three consolidations from `/simplify`, none behavioural. ONE SCOPE WALK, NOT TWO COPIES. `walkers.ts` carried six hand-rolled copies of the same scope-chain walk, and the one this branch added had already DIVERGED: it `break`-ed on a cycle or missing scope and fell through to the qualified-name tail, where `findAllCallableBindingsInScope` `return []`s — identical malformed input, different answer depending on which copy the caller reached. Extracted `findAllBindingsInScope(start, name, scopes, predicate)`; both "collect all at the nearest binding scope" functions now delegate. Six copies to five, and the divergence is gone. The remaining four are pre-existing and out of this change's scope. CODEC DERIVES BOTH DIRECTIONS FROM ONE TABLE. `SIGIL_BY_KIND` carried the comment "one table so encoder and decoder cannot drift" — but only the encoder read it. The decoder hand-wrote `sigil === 'c' ? 'call' : 'field'` plus its own await/index comparison, so the decoder was precisely the side that could drift from the table meant to prevent drift. `KIND_BY_SIGIL` and `NAME_FREE_KINDS` are now derived from it. ONE SIGNAL FOR "NO CLASS HERE". `FoldState` carried `unresolvedDeclaredType` alongside `def`, and `typeOfMemberOnClass` wrote `def: owner` — the PREVIOUS position — next to the marker. That value is never read on any path, because all three reads test the marker first. Two sources of truth for one fact, one of them a knowingly inconsistent state that reads as intentional. `def === undefined` is now the only signal; the trailing guard disappeared because `return current.def` already yields undefined there. Also moved the construction-selector veto BELOW the name-free continues instead of keeping the `step.name !== undefined` guard added earlier. The guard patched a reachability problem that position solves outright — only named steps reach the veto now. That line is the one that silently vetoed every await/index fold, so removing the need for the guard is the better resolution than keeping it. `DecorationStripper` is now used at all five sites rather than declared once and spelled longhand at four. Shape matrix unchanged at RESOLVES 55 / VISIBLE-GAP 23 / INVISIBLE-GAP 18. 4397 tests green. Co-Authored-By: Claude Opus 5 (1M context) --- .../contract/scope-resolver.ts | 5 +- .../passes/compound-receiver.ts | 79 ++++++++--------- .../scope-resolution/scope/walkers.ts | 87 +++++++++---------- .../ingestion/utils/receiver-chain-codec.ts | 35 +++++--- 4 files changed, 110 insertions(+), 96 deletions(-) diff --git a/gitnexus/src/core/ingestion/scope-resolution/contract/scope-resolver.ts b/gitnexus/src/core/ingestion/scope-resolution/contract/scope-resolver.ts index cc4f4f464..ffa0576c7 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/contract/scope-resolver.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/contract/scope-resolver.ts @@ -268,6 +268,7 @@ * `docs/plans/2026-04-20-001-refactor-emit-pipeline-generalization-plan.md`. */ +import type { DecorationStripper } from '../scope/walkers.js'; import type { BindingRef, Callsite, @@ -1145,7 +1146,7 @@ export interface ScopeResolver { * decoration. Measured: only Go needs it for a receiver base; Rust, C#, Swift, * TypeScript and C++ need it for field types. */ - readonly stripTypePreservingDecoration?: (typeName: string) => string | undefined; + readonly stripTypePreservingDecoration?: DecorationStripper; /** * Unwrap a COLLECTION spelling to its element type — `User[]` -> `User`, @@ -1166,7 +1167,7 @@ export interface ScopeResolver { * both kinds of path land on the element type without the core needing to know * which is which. */ - readonly unwrapCollectionElement?: (typeName: string) => string | undefined; + readonly unwrapCollectionElement?: DecorationStripper; /** * Whether the compound-receiver resolver should strip C-style cast diff --git a/gitnexus/src/core/ingestion/scope-resolution/passes/compound-receiver.ts b/gitnexus/src/core/ingestion/scope-resolution/passes/compound-receiver.ts index 5be4101a2..34810a3a2 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/passes/compound-receiver.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/passes/compound-receiver.ts @@ -27,6 +27,7 @@ import type { WorkspaceResolutionIndex } from '../workspace-index.js'; import { stripTemplateArguments } from '../../utils/template-arguments.js'; import type { DecodedReceiverChain } from '../../utils/receiver-chain-codec.js'; import { decodeReceiverChain } from '../../utils/receiver-chain-codec.js'; +import type { DecorationStripper } from '../scope/walkers.js'; import { findClassBindingInScope, findEnclosingClassDef, @@ -127,11 +128,11 @@ interface ResolveCompoundReceiverOptions { * languages whose declared types carry no such decoration, and never applied * by the shared lookup's other callers — see the contract's own note on why * this is opt-in rather than global. */ - readonly stripTypePreservingDecoration?: (typeName: string) => string | undefined; + readonly stripTypePreservingDecoration?: DecorationStripper; /** Collection -> element unwrap, consulted ONLY by an index step. See the * `ScopeResolver` field of the same name for why this is separate from the * type-preserving stripper. */ - readonly unwrapCollectionElement?: (typeName: string) => string | undefined; + readonly unwrapCollectionElement?: DecorationStripper; } /** Is this hop the language's construction selector applied to the class @@ -319,19 +320,20 @@ function resolveConstructionExpressionClass( * than guessing. */ interface FoldState { - /** Absent when the position's declared type named no class — see - * `unresolvedDeclaredType`. Only an unwrapping step can advance from there. */ + /** + * Absent when the position's declared type named no class — `Promise` + * and `[]Repo` name nothing in the workspace. ONE signal, not two: an earlier + * version carried a separate `unresolvedDeclaredType` flag alongside a `def` + * holding the PREVIOUS position, which no path ever read. Two sources of + * truth for one fact, and the dead `def` read as intentional. + * + * Only an unwrapping step (await, index) can advance from an absent `def`; + * every other step declines, because folding on against the previous class + * would look the next member up on the wrong owner. + */ readonly def: SymbolDefinition | undefined; readonly declaredTypeName?: string; readonly declaredAtScope?: ScopeId; - /** - * The declared type named no class in the workspace, so `def` is the PREVIOUS - * position rather than this member's type. Only an unwrapping step (await, - * index) can make progress from here; any other step must decline, because - * folding on against the previous class would silently look the next member up - * on the wrong owner. - */ - readonly unresolvedDeclaredType?: boolean; } function typeOfMemberOnClass( @@ -358,11 +360,12 @@ function typeOfMemberOnClass( // await or index step unwrapping them is exactly how they become // resolvable. Returning `undefined` here would strand those shapes. if (def === undefined) { + // `def` absent, NOT the previous owner: nothing may fold a member off + // this position except an unwrapping step. return { - def: owner, + def: undefined, declaredTypeName: memberType.rawName, declaredAtScope: memberType.declaredAtScope, - unresolvedDeclaredType: true, }; } return { @@ -472,33 +475,12 @@ export function foldReceiverChain( def: undefined, declaredTypeName: baseBinding.rawName, declaredAtScope: baseBinding.declaredAtScope, - unresolvedDeclaredType: true, }; } else { return undefined; } for (const step of chain.steps) { - // Construction is NOT an ordinary member lookup. `Factory.new` on a class - // constant denotes an instance of Factory, and the cascade already encodes - // that (`isConstructionSelectorHop`) along with the class-constant test that - // separates it from an instance method genuinely named `new`. The fold - // carries no such distinction — a chain step records a name, not whether its - // base was a class reference or a value — so folding one would resolve - // `Factory.new.run` against whatever member named `new` the lookup reaches - // first. That turned a correct edge into a WRONG one (Ruby - // `Factory.new.run` → `Product.run`), which is the failure mode this whole - // line of work exists to avoid. Decline and let the cascade answer. - // `step.name !== undefined` is load-bearing, not defensive. The name-free - // step kinds (`await`, `index`) carry no name, and a language with no - // `constructionSyntax` has no selector — so the bare equality below was - // `undefined === undefined`, which matched EVERY name-free step and vetoed - // the whole fold before it ran. That is why subscript and await receivers - // minted a chain, fired the gate, and still produced no edge. - if (step.name !== undefined && options.constructionSyntax?.selector === step.name) { - return undefined; - } - // A position whose declared type named no class can only be advanced by an // unwrapping step. Folding an ordinary member off it would look the member // up on the PREVIOUS class — a wrong owner, silently. @@ -537,20 +519,39 @@ export function foldReceiverChain( ); if (elementClass === undefined) return undefined; current = { def: elementClass, declaredTypeName: element, declaredAtScope: scopeForLookup }; - } else if (current.unresolvedDeclaredType === true) { + } else if (current.def === undefined) { // No unwrap available and the position never named a class: nothing to // fold on. Decline rather than continue against a stale owner. return undefined; } continue; } - if (current.unresolvedDeclaredType === true || current.def === undefined) return undefined; + // Construction is NOT an ordinary member lookup. `Factory.new` on a class + // constant denotes an instance of Factory, and the cascade already encodes + // that (`isConstructionSelectorHop`) along with the class-constant test that + // separates it from an instance method genuinely named `new`. The fold + // carries no such distinction — a chain step records a name, not whether its + // base was a class reference or a value — so folding one would resolve + // `Factory.new.run` against whatever member named `new` the lookup reaches + // first. That turned a correct edge into a WRONG one (Ruby + // `Factory.new.run` → `Product.run`), which is the failure mode this whole + // line of work exists to avoid. Decline and let the cascade answer. + // + // Placed AFTER the name-free continues above, deliberately. When it sat + // first, `options.constructionSyntax?.selector === step.name` compared + // `undefined === undefined` for every await/index step in a language with no + // construction selector, vetoing the entire fold before it ran — which is + // why those receivers minted a chain, fired the gate, and produced no edge. + // Position, not a guard, is what makes that unreachable: only named steps + // get here. + if (options.constructionSyntax?.selector === step.name) return undefined; + if (current.def === undefined) return undefined; const next = typeOfMemberOnClass(current.def, step.name, scopes, index, options); if (next === undefined) return undefined; current = next; } - // A chain that ended on an unresolved declared type never reached a class. - if (current.unresolvedDeclaredType === true) return undefined; + // A chain that ended without a class returns undefined naturally — no + // separate guard, because `def` IS the signal. return current.def; } diff --git a/gitnexus/src/core/ingestion/scope-resolution/scope/walkers.ts b/gitnexus/src/core/ingestion/scope-resolution/scope/walkers.ts index fdce9e5d2..b4b8aeab9 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/scope/walkers.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/scope/walkers.ts @@ -336,36 +336,12 @@ export function findAllClassBindingsInScope( name: string, scopes: ScopeResolutionIndexes, ): readonly SymbolDefinition[] { + const inScope = findAllBindingsInScope(startScope, name, scopes, (def) => isClassLike(def.type)); + // The scope chain wins outright when it binds the name: an inner binding + // shadows anything the qualified-name index would contribute. + if (inScope.length > 0) return inScope; + const byNodeId = new Map(); - let currentId: ScopeId | null = startScope; - const visited = new Set(); - while (currentId !== null) { - if (visited.has(currentId)) break; - visited.add(currentId); - const scope = scopes.scopeTree.getScope(currentId); - if (scope === undefined) break; - - // `Object` scopes are a hoist boundary only — see walkScopeChain (#2545). - if (scope.kind !== 'Object') { - const found: SymbolDefinition[] = []; - for (const b of scope.bindings.get(name) ?? []) { - if (isClassLike(b.def.type)) found.push(b.def); - } - for (const b of lookupBindingsAt(currentId, name, scopes)) { - if (isClassLike(b.def.type)) found.push(b.def); - } - // Stop at the first scope that binds the name at all: an inner binding - // SHADOWS an outer one, so continuing would report a shadowed outer - // definition as a competing candidate and decline a name that is actually - // unambiguous at this point. - if (found.length > 0) { - for (const def of found) byNodeId.set(def.nodeId, def); - return [...byNodeId.values()]; - } - } - currentId = scope.parent; - } - for (const id of scopes.qualifiedNames.get(name)) { const def = scopes.defs.get(id); if (def !== undefined && isClassLike(def.type)) byNodeId.set(def.nodeId, def); @@ -887,10 +863,28 @@ export function findAllCallableBindingCandidatesInScope( * `findCallableBindingInScope`: once any callable binding is found in a * scope, outer scopes are not consulted. */ -export function findAllCallableBindingsInScope( +/** + * Every definition visible for `name` at the NEAREST scope that binds it, + * filtered by `predicate` and deduped by `nodeId`. + * + * THE shared "collect all at the nearest binding scope" walk. `walkScopeChain` + * answers the first-match question; this answers the how-many question, which is + * what a caller needs before it can decline on ambiguity. + * + * Stops at the first scope that binds the name at all: an inner binding SHADOWS + * an outer one, so continuing would report a shadowed outer definition as a + * competing candidate and decline a name that is unambiguous at this point. + * + * Returns `[]` on a cycle or a missing scope. That is deliberate and matters: + * an earlier copy of this walk `break`-ed instead and fell through to a + * qualified-name fallback, so the same malformed input produced a different + * answer depending on which copy the caller happened to reach. + */ +function findAllBindingsInScope( startScope: ScopeId, - callableName: string, + name: string, scopes: ScopeResolutionIndexes, + predicate: (def: SymbolDefinition) => boolean, ): readonly SymbolDefinition[] { let currentId: ScopeId | null = startScope; const visited = new Set(); @@ -905,24 +899,16 @@ export function findAllCallableBindingsInScope( if (scope.kind !== 'Object') { const out: SymbolDefinition[] = []; const seen = new Set(); - const pushCallable = (def: SymbolDefinition): void => { - if (def.type !== 'Function' && def.type !== 'Method' && def.type !== 'Constructor') return; + const push = (def: SymbolDefinition): void => { + if (!predicate(def)) return; if (seen.has(def.nodeId)) return; seen.add(def.nodeId); out.push(def); }; - const localBindings = scope.bindings.get(callableName); - if (localBindings !== undefined) { - for (const b of localBindings) { - pushCallable(b.def); - } - } - - const importedBindings = lookupBindingsAt(currentId, callableName, scopes); - for (const b of importedBindings) { - pushCallable(b.def); - } + // Local first: a binding in this scope shadows an imported one. + for (const b of scope.bindings.get(name) ?? []) push(b.def); + for (const b of lookupBindingsAt(currentId, name, scopes)) push(b.def); if (out.length > 0) return out; } @@ -931,6 +917,19 @@ export function findAllCallableBindingsInScope( return []; } +export function findAllCallableBindingsInScope( + startScope: ScopeId, + callableName: string, + scopes: ScopeResolutionIndexes, +): readonly SymbolDefinition[] { + return findAllBindingsInScope( + startScope, + callableName, + scopes, + (def) => def.type === 'Function' || def.type === 'Method' || def.type === 'Constructor', + ); +} + /** * ISO C++ `[basic.lookup.unqual]` §7: ADL is suppressed when ordinary * unqualified lookup finds: diff --git a/gitnexus/src/core/ingestion/utils/receiver-chain-codec.ts b/gitnexus/src/core/ingestion/utils/receiver-chain-codec.ts index f09608e26..6d4b8678f 100644 --- a/gitnexus/src/core/ingestion/utils/receiver-chain-codec.ts +++ b/gitnexus/src/core/ingestion/utils/receiver-chain-codec.ts @@ -69,17 +69,27 @@ const VERSION = '2'; const SEPARATOR = '|'; const TRUNCATED = '~'; -const AWAIT_SIGIL = 'a'; -const INDEX_SIGIL = 'i'; - -/** Sigil per step kind. One table so encoder and decoder cannot drift. */ +/** Sigil per step kind. ONE table, and both directions derive from it — the + * decoder used to hand-write `sigil === 'c' ? 'call' : 'field'` and its own + * await/index comparison, which meant the decoder was exactly the side that + * could drift from the table claiming to prevent drift. */ const SIGIL_BY_KIND = { call: 'c', field: 'f', - await: AWAIT_SIGIL, - index: INDEX_SIGIL, + await: 'a', + index: 'i', } as const; +type StepKind = keyof typeof SIGIL_BY_KIND; + +/** Kinds that encode as a BARE sigil, because they have no member name to + * carry. Derived from the step union rather than listed twice. */ +const NAME_FREE_KINDS = new Set(['await', 'index']); + +const KIND_BY_SIGIL: ReadonlyMap = new Map( + Object.entries(SIGIL_BY_KIND).map(([kind, sigil]) => [sigil, kind as StepKind]), +); + /** Hard cap on the encoded payload. `MAX_CHAIN_DEPTH` already bounds the step * COUNT; this bounds the total bytes so a pathological identifier cannot grow * a shard without limit. Generous against real identifiers — the encoding for @@ -129,7 +139,7 @@ export function encodeReceiverChain( // Name-free kinds encode as a bare sigil. They are exempt from the // non-empty-name guard because they HAVE no name to check — not because the // guard is relaxed: an empty-name `call` or `field` is still refused below. - if (step.kind === 'await' || step.kind === 'index') { + if (NAME_FREE_KINDS.has(step.kind)) { parts.push(SIGIL_BY_KIND[step.kind]); continue; } @@ -172,14 +182,17 @@ export function decodeReceiverChain(value: unknown): DecodedReceiverChain | unde // Name-free kinds must be EXACTLY their sigil. Rejecting a trailing tail is // what keeps an accidentally empty-name call or field from decoding as one // of these: `c` alone stays malformed, it does not become an await. - if (sigil === AWAIT_SIGIL || sigil === INDEX_SIGIL) { + const kind = sigil === undefined ? undefined : KIND_BY_SIGIL.get(sigil); + if (kind === undefined) return undefined; + if (NAME_FREE_KINDS.has(kind)) { + // Must be EXACTLY the sigil. Rejecting a trailing tail is what keeps an + // accidentally empty-name call or field from decoding as one of these. if (name.length > 0) return undefined; - steps.push({ kind: sigil === AWAIT_SIGIL ? 'await' : 'index' }); + steps.push({ kind: kind as 'await' | 'index' }); continue; } - if (sigil !== 'c' && sigil !== 'f') return undefined; if (!isEncodableSegment(name)) return undefined; - steps.push({ kind: sigil === 'c' ? 'call' : 'field', name }); + steps.push({ kind: kind as 'call' | 'field', name }); } return { baseReceiverName, steps, truncated };