diff --git a/gitnexus/src/core/ingestion/scope-resolution/passes/unique-name-properties.ts b/gitnexus/src/core/ingestion/scope-resolution/passes/unique-name-properties.ts index e365a3e05..a46c71497 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/passes/unique-name-properties.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/passes/unique-name-properties.ts @@ -26,6 +26,35 @@ * global fallback, because this is the same kind of claim: a name matched * workspace-wide with no scope evidence behind it. * + * ── R2: workspace uniqueness is too blunt on its own ──────────────────────── + * + * Measured on the repo this pass was written for: `exitMinAtrMult` has 26 + * `Property` definitions — 16 of them in one-off `scripts/`, 7 in the frontend, + * one in a test, and exactly ONE in the backend that actually reads it. Strict + * uniqueness declined every backend read because research scripts the backend + * has no relationship with each carry a same-named key. The gate was not + * wrong, it was scope-blind: it compared against the whole workspace when the + * reader can only plausibly mean something it can SEE. + * + * So a name with several definitions is now narrowed before being abandoned: + * Tier 1 — a definition in the READING FILE itself. + * Tier 2 — a definition in a file the reading file DIRECTLY IMPORTS. + * Exactly one survivor at the first non-empty tier resolves; anything else is + * still refused. Narrowing uses the finalized import graph, so it is real + * evidence rather than a path-shape heuristic, and it is language-neutral. + * + * A tier that finds SEVERAL candidates stops the walk instead of falling + * through to the next one. Two same-named keys in the reading file mean the + * read is genuinely ambiguous where the reader is standing; reaching past them + * to an imported file would answer a question the local evidence already + * contradicts. + * + * Confidence stays 0.5 for every tier. Narrowing improves which candidate is + * chosen, not the kind of claim being made — it is still a name match, and the + * round-1 contract is that a consumer filtering on confidence can drop all + * name inference without dropping scope-resolved edges. The reason string + * records which tier fired. + * * WHY GRAPH NODES, NOT SCOPE DEFS: an object-literal key mints a `Property` * NODE (parse query) but no scope-resolution DEF, so `scope.bindings` and * `localDefs` are both empty for exactly the population this pass exists to @@ -34,7 +63,7 @@ * rather than through `tryEmitEdge`, whose target side takes a def. */ -import type { ParsedFile } from 'gitnexus-shared'; +import type { ImportEdge, ParsedFile, ScopeId } from 'gitnexus-shared'; import type { KnowledgeGraph } from '../../../graph/types.js'; import type { ScopeResolutionIndexes } from '../../model/scope-resolution-indexes.js'; import type { GraphNodeLookup } from '../graph-bridge/node-lookup.js'; @@ -49,43 +78,163 @@ const UNIQUE_NAME_CONFIDENCE = 0.5; const EDGE_REASON = 'scope-resolution: unique-name property'; -/** Sentinel for "more than one node carries this name" — never resolved. */ -const AMBIGUOUS = null; +/** + * Most candidates any one name will keep for narrowing. A name carried by more + * definitions than this is a generic key (`id`, `type`, `value`) that no tier + * is going to disambiguate, so the candidate list is dropped and the name is + * treated as ambiguous outright. This is what keeps the index from + * materializing a long array per generic key in a large repo — the concern + * that made the original implementation store a sentinel instead of a list. + */ +const MAX_TRACKED_CANDIDATES = 32; -export interface UniqueNamePropertyStats { - /** Edges emitted from a workspace-unique name match. */ - readonly emitted: number; - /** - * Sites skipped because two or more `Property` nodes share the name, so a - * unique-name match would have been a coin flip. Reported rather than - * silently dropped: this is the population a receiver-typing improvement - * would convert into precise edges. - */ - readonly ambiguous: number; +/** Sentinel for "too many nodes carry this name to narrow" — never resolved. */ +const OVERSATURATED = null; + +interface PropertyCandidate { + readonly id: string; + readonly filePath: string; } /** - * Index `Property` nodes by name, collapsing any name with two or more nodes - * to {@link AMBIGUOUS} immediately. Storing the sentinel instead of a list - * keeps a repo full of same-named keys (`id`, `type`, `value`) from - * materializing an array per key. + * The only part of the finalized scope model this pass reads. Narrowed to a + * structural type so the pass does not depend on the full finalize result. */ -function indexUniquePropertyNodes(graph: KnowledgeGraph): ReadonlyMap { - const byName = new Map(); +interface FinalizedImportView { + readonly imports: ReadonlyMap; +} + +/** Most distinct names reported back; enough to act on, bounded for logs. */ +const MAX_REPORTED_AMBIGUOUS_NAMES = 25; + +export interface UniqueNamePropertyStats { + /** Edges emitted from a name match at any tier. */ + readonly emitted: number; + /** + * Sites skipped because the name could not be narrowed to one definition, + * so a match would have been a coin flip. Reported rather than silently + * dropped: this is the population a receiver-typing improvement would + * convert into precise edges. + */ + readonly ambiguous: number; + /** + * Of {@link emitted}, how many needed scope narrowing — the name carried + * several definitions and same-file or direct-import evidence picked one. + * Strict workspace uniqueness would have refused every one of these. + */ + readonly narrowed: number; + /** + * The distinct names behind {@link ambiguous}, capped. A bare count says a + * gap exists; the names say WHICH fields are unanswerable, which is the + * difference between a metric and something a reader can act on. + */ + readonly ambiguousNames: readonly string[]; +} + +/** + * Index `Property` nodes by name, keeping each name's candidates up to + * {@link MAX_TRACKED_CANDIDATES} so a multi-candidate name can still be + * narrowed by scope. Past the cap the list is dropped for {@link OVERSATURATED} + * — that many same-named keys is a generic name no tier can disambiguate, and + * holding the array would cost memory for a question that has no answer. + */ +function indexPropertyNodesByName( + graph: KnowledgeGraph, +): ReadonlyMap { + const byName = new Map(); for (const node of graph.iterNodes()) { if (node.label !== 'Property') continue; const name = node.properties.name; if (typeof name !== 'string' || name.length === 0) continue; + const filePath = node.properties.filePath; const existing = byName.get(name); + if (existing === OVERSATURATED) continue; + const candidate: PropertyCandidate = { + id: node.id, + filePath: typeof filePath === 'string' ? filePath : '', + }; if (existing === undefined) { - byName.set(name, node.id); - } else if (existing !== AMBIGUOUS && existing !== node.id) { - byName.set(name, AMBIGUOUS); + byName.set(name, [candidate]); + continue; } + if (existing.some((c) => c.id === candidate.id)) continue; + if (existing.length >= MAX_TRACKED_CANDIDATES) { + byName.set(name, OVERSATURATED); + continue; + } + existing.push(candidate); } return byName; } +/** + * Files each file directly imports, from the FINALIZED import graph. + * + * Built from `finalized.imports` rather than the raw per-scope edges because + * only the finalized form has `targetFile` linked — pre-finalize the field is + * still null for anything the resolver had to look up, which would silently + * narrow every read to nothing. + */ +function buildDirectImportMap( + parsedFiles: readonly ParsedFile[], + finalized: FinalizedImportView, +): ReadonlyMap> { + const scopeToFile = new Map(); + for (const parsed of parsedFiles) { + for (const scope of parsed.scopes) { + scopeToFile.set(scope.id, parsed.filePath); + } + } + + const byFile = new Map>(); + for (const [scopeId, edges] of finalized.imports) { + const fromFile = scopeToFile.get(scopeId); + if (fromFile === undefined) continue; + for (const edge of edges) { + if (edge.targetFile === null || edge.targetFile === fromFile) continue; + let set = byFile.get(fromFile); + if (set === undefined) { + set = new Set(); + byFile.set(fromFile, set); + } + set.add(edge.targetFile); + } + } + return byFile; +} + +/** + * Pick the single candidate a read in `readingFile` can plausibly mean. + * + * Tiers are tried in order and the FIRST non-empty one decides — including + * deciding to refuse. A tier holding several candidates returns null rather + * than falling through, because local evidence that is itself ambiguous is + * still evidence: reaching past two same-named keys in the reading file to an + * imported third would answer a question the reader's own file contradicts. + */ +function narrowToSingleCandidate( + candidates: readonly PropertyCandidate[], + readingFile: string, + importedFiles: ReadonlySet | undefined, +): { readonly id: string; readonly tier: string } | null { + if (candidates.length === 1) { + return { id: candidates[0]!.id, tier: 'workspace-unique' }; + } + + const sameFile = candidates.filter((c) => c.filePath === readingFile); + if (sameFile.length > 0) { + return sameFile.length === 1 ? { id: sameFile[0]!.id, tier: 'same-file' } : null; + } + + if (importedFiles === undefined) return null; + const imported = candidates.filter((c) => importedFiles.has(c.filePath)); + if (imported.length > 0) { + return imported.length === 1 ? { id: imported[0]!.id, tier: 'imported-file' } : null; + } + + return null; +} + export function emitUniqueNamePropertyAccesses( graph: KnowledgeGraph, indexes: ScopeResolutionIndexes, @@ -93,12 +242,22 @@ export function emitUniqueNamePropertyAccesses( nodeLookup: GraphNodeLookup, /** Sites a precise pass already owns — never second-guessed here. */ skipSites: ReadonlySet, + /** Finalized import graph; narrows a name carried by several definitions. */ + finalized?: FinalizedImportView, ): UniqueNamePropertyStats { - const byName = indexUniquePropertyNodes(graph); - if (byName.size === 0) return { emitted: 0, ambiguous: 0 }; + const byName = indexPropertyNodesByName(graph); + if (byName.size === 0) { + return { emitted: 0, ambiguous: 0, narrowed: 0, ambiguousNames: [] }; + } + const directImports = + finalized === undefined + ? new Map>() + : buildDirectImportMap(parsedFiles, finalized); let emitted = 0; let ambiguous = 0; + let narrowed = 0; + const ambiguousNames = new Set(); const seen = new Set(); for (const parsed of parsedFiles) { @@ -111,13 +270,27 @@ export function emitUniqueNamePropertyAccesses( const siteKey = `${parsed.filePath}:${site.atRange.startLine}:${site.atRange.startCol}`; if (skipSites.has(siteKey)) continue; - const targetId = byName.get(site.name); - if (targetId === undefined) continue; - if (targetId === AMBIGUOUS) { + const candidates = byName.get(site.name); + if (candidates === undefined) continue; + if (candidates === OVERSATURATED) { ambiguous++; + ambiguousNames.add(site.name); continue; } + const choice = narrowToSingleCandidate( + candidates, + parsed.filePath, + directImports.get(parsed.filePath), + ); + if (choice === null) { + ambiguous++; + ambiguousNames.add(site.name); + continue; + } + const targetId = choice.id; + if (choice.tier !== 'workspace-unique') narrowed++; + const callerGraphId = resolveCallerGraphId(site.inScope, indexes, nodeLookup, site.atRange); if (callerGraphId === undefined) continue; // A property reading itself is not a fact about anything. @@ -135,12 +308,17 @@ export function emitUniqueNamePropertyAccesses( targetId, type: 'ACCESSES', confidence: UNIQUE_NAME_CONFIDENCE, - reason: `${EDGE_REASON}: ${site.kind}`, + reason: `${EDGE_REASON} (${choice.tier}): ${site.kind}`, evidence: [], }); emitted++; } } - return { emitted, ambiguous }; + return { + emitted, + ambiguous, + narrowed, + ambiguousNames: Array.from(ambiguousNames).sort().slice(0, MAX_REPORTED_AMBIGUOUS_NAMES), + }; } diff --git a/gitnexus/src/core/ingestion/scope-resolution/pipeline/run.ts b/gitnexus/src/core/ingestion/scope-resolution/pipeline/run.ts index 5c38087ae..5ee383081 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/pipeline/run.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/pipeline/run.ts @@ -453,6 +453,18 @@ interface RunScopeResolutionStats { * population a receiver-typing improvement would convert into precise edges. */ readonly uniqueNamePropertyAmbiguous: number; + /** + * Of `uniqueNamePropertyEdges`, how many the name carried several definitions + * for and same-file or direct-import evidence narrowed to one. Strict + * workspace uniqueness refused every one of these (R2). + */ + readonly uniqueNamePropertyNarrowed: number; + /** + * The distinct field names behind `uniqueNamePropertyAmbiguous`, capped. A + * count says a coverage gap exists; the names say WHICH fields are + * unanswerable, so the gap is actionable rather than merely measured. + */ + readonly uniqueNamePropertyAmbiguousNames: readonly string[]; readonly resolutionOutcomes: readonly ResolutionOutcome[]; /** * Per-function taint summaries harvested in the pdg window (#2084 M4 U1). @@ -584,6 +596,8 @@ export function runScopeResolution( importedValueRefEdges: 0, uniqueNamePropertyEdges: 0, uniqueNamePropertyAmbiguous: 0, + uniqueNamePropertyNarrowed: 0, + uniqueNamePropertyAmbiguousNames: [], resolutionOutcomes, functionSummaries: [], callSummaries: [], @@ -616,6 +630,8 @@ export function runScopeResolution( importedValueRefEdges: 0, uniqueNamePropertyEdges: 0, uniqueNamePropertyAmbiguous: 0, + uniqueNamePropertyNarrowed: 0, + uniqueNamePropertyAmbiguousNames: [], resolutionOutcomes, functionSummaries: [], callSummaries: [], @@ -973,13 +989,14 @@ export function runScopeResolution( const uniqueNameProperties = callableFlowOnly || provider.fieldFallbackOnMethodLookup === false - ? { emitted: 0, ambiguous: 0 } + ? { emitted: 0, ambiguous: 0, narrowed: 0, ambiguousNames: [] } : emitUniqueNamePropertyAccesses( graph, indexes, emitParsedFiles, postHeritageNodeLookup, uniqueNameSkipSites, + finalized, ); // value-ref registrations (#2437): USES edges at the registration sites @@ -1463,6 +1480,8 @@ export function runScopeResolution( importedValueRefEdges: importedValueRefs.emitted, uniqueNamePropertyEdges: uniqueNameProperties.emitted, uniqueNamePropertyAmbiguous: uniqueNameProperties.ambiguous, + uniqueNamePropertyNarrowed: uniqueNameProperties.narrowed, + uniqueNamePropertyAmbiguousNames: uniqueNameProperties.ambiguousNames, resolutionOutcomes, functionSummaries: harvestedSummaries, callSummaries: harvestedCallSummaries, diff --git a/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/narrow-alpha.js b/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/narrow-alpha.js new file mode 100644 index 000000000..02965873d --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/narrow-alpha.js @@ -0,0 +1,5 @@ +// R2 narrowing, candidate A. Same key name as narrow-beta.js, so a +// workspace-wide uniqueness check sees two definitions and refuses. +export const alphaCfg = { + narrowedTimeoutMs: 1000, +}; diff --git a/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/narrow-beta.js b/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/narrow-beta.js new file mode 100644 index 000000000..53d15dc60 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/narrow-beta.js @@ -0,0 +1,6 @@ +// R2 narrowing, candidate B — deliberately NOT imported by the reader below. +// This is the "one-off script carrying the same key" case that made strict +// workspace uniqueness decline every real read in the reporting repo. +export const betaCfg = { + narrowedTimeoutMs: 2000, +}; diff --git a/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/narrow-both.js b/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/narrow-both.js new file mode 100644 index 000000000..b5508c626 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/narrow-both.js @@ -0,0 +1,11 @@ +// CONTROL: imports BOTH candidates, so direct-import evidence does not +// disambiguate and the read must stay refused. Narrowing is meant to use +// scope evidence, not to lower the bar for guessing. +import { alphaCfg } from './narrow-alpha.js'; +import { betaCfg } from './narrow-beta.js'; + +export function readsBothVisible(cfg) { + return cfg.narrowedTimeoutMs; +} + +export const bothSeen = [alphaCfg, betaCfg]; diff --git a/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/narrow-reader.js b/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/narrow-reader.js new file mode 100644 index 000000000..9a55a635a --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/narrow-reader.js @@ -0,0 +1,12 @@ +// Imports exactly ONE of the two definitions. The read below cannot plausibly +// mean the other — the reader cannot see it — so direct-import evidence picks +// the candidate that workspace uniqueness alone had to abandon. +import { alphaCfg } from './narrow-alpha.js'; + +export function readsNarrowed(cfg) { + return cfg.narrowedTimeoutMs; +} + +export function readsViaBinding() { + return alphaCfg.narrowedTimeoutMs; +} diff --git a/gitnexus/test/integration/resolvers/javascript-object-properties.test.ts b/gitnexus/test/integration/resolvers/javascript-object-properties.test.ts index 48e6387db..367b58549 100644 --- a/gitnexus/test/integration/resolvers/javascript-object-properties.test.ts +++ b/gitnexus/test/integration/resolvers/javascript-object-properties.test.ts @@ -104,6 +104,51 @@ describe('JavaScript plain-object property access (A1/A5)', () => { for (const e of inferred) expect(e.rel.confidence).toBeLessThan(0.85); }); + // R2. Strict workspace uniqueness was measurably too blunt: in the reporting + // repo `exitMinAtrMult` had 26 definitions, 16 of them in one-off scripts the + // backend has no relationship with, so every backend read was refused because + // of competitors the reader cannot even see. + describe('scope narrowing for multi-candidate names (R2)', () => { + const reasonsFor = (field: string): string[] => + getRelationships(result, 'ACCESSES') + .filter((e) => e.target === field) + .map((e) => String(e.rel.reason ?? '')); + + it('still sees two definitions of the narrowed name', () => { + // Precondition. Without this the narrowing assertions below would pass + // trivially by there being nothing to narrow. + expect(propertyNames().filter((n) => n === 'narrowedTimeoutMs')).toHaveLength(2); + }); + + it('resolves an untyped read using direct-import evidence', () => { + expect(readersOf('narrowedTimeoutMs')).toContain('readsNarrowed'); + }); + + it('records which tier resolved it, not just that something did', () => { + expect(reasonsFor('narrowedTimeoutMs').some((r) => r.includes('imported-file'))).toBe(true); + }); + + // The bound. Narrowing exists to USE scope evidence, not to lower the bar + // for guessing — a reader that can see both candidates is exactly as stuck + // as before, and must stay refused. + it('still refuses when the reader imports BOTH candidates', () => { + expect(readersOf('narrowedTimeoutMs')).not.toContain('readsBothVisible'); + }); + + // Same-file evidence that is itself ambiguous must stop the walk rather + // than fall through to a weaker tier. + it('keeps refusing two same-named keys in the reading file', () => { + expect(readersOf('sharedTimeoutMs')).toEqual([]); + }); + + it('reports the names it could not resolve, not only a count', () => { + const stats = (result as unknown as { scopeResolution?: Record }) + .scopeResolution; + if (stats === undefined) return; + expect(stats.uniqueNamePropertyAmbiguousNames).toContain('sharedTimeoutMs'); + }); + }); + // R2-1a. Reported as the cheapest remaining win and it is: freezing a config // object is how JS publishes an immutable contract, so the shape whose fields // are most worth querying was the one shape the rule could not see.