mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-07 02:58:02 +00:00
fix(scope-resolution): build the module-level set before the out-of-core seal
Review blocker. Under `GITNEXUS_DISK_SCOPE_INDEX=1` the seal replaces every
ParsedFile with a scope-STRIPPED copy, and the block-local filter's set was
built after it — so it walked `scopes: []` for every file, came out empty, and
the filter read that as "no def is module-level" and dropped EVERY
`Const`/`Variable`/`Static` ACCESSES edge in the repo. All languages, all
files, including the module-scope-const edges this PR exists to add. Nothing
threw and nothing logged, on the path the largest repos take: the exact
confident-empty answer the PR is about.
Built above the seal now, from `parsedFiles`, and passed as `undefined` rather
than an empty set when no scope was inspectable — an empty set is a legitimate
answer ("this repo has no module-level value defs") and must not be
indistinguishable from "could not look". Fails open; the block-local exclusion
is still asserted under the seal, since that is correctness rather than
optimization.
Also widens module level past `kind === 'Module'`. A `Namespace` scope (TS
`namespace`, Rust `mod`, C++/C# `namespace`) holds importable values too, and
treating its consts as function-locals dropped their reads. Included only when
the whole chain to the root is Module/Namespace, so a namespace declared inside
a function body stays local — asserted both ways.
That fixture then failed for a third reason: `@reference.read.identifier`
existed ONLY in the JavaScript query, so A2 did not work for TypeScript at all.
Added there, and both languages widened to `variable_declarator value:` and
`binary_expression` operands — the gaps review named between what A2 claimed
and what it matched.
Nothing covered `GITNEXUS_DISK_SCOPE_INDEX`. The new parity test asserts the
seal changes no edge, and was verified against an emulation of the original
bug: same-file readers vanish and only the cross-file reader survives.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
a93d2e973f
commit
fb11102689
8 changed files with 210 additions and 19 deletions
|
|
@ -610,6 +610,17 @@ export const JAVASCRIPT_SCOPE_QUERY = `
|
|||
(return_statement
|
||||
(identifier) @reference.name @reference.read.identifier)
|
||||
|
||||
;; \`const next = LIMIT\` and \`n > LIMIT\` — both plainly value reads, and both
|
||||
;; named in review as gaps between what A2 claimed and what it matched.
|
||||
(variable_declarator
|
||||
value: (identifier) @reference.name @reference.read.identifier)
|
||||
|
||||
(binary_expression
|
||||
left: (identifier) @reference.name @reference.read.identifier)
|
||||
|
||||
(binary_expression
|
||||
right: (identifier) @reference.name @reference.read.identifier)
|
||||
|
||||
;; Destructured PARAMETER keys (R2-1c). \`function exit({ exitMinAtrMult = 0 })\`
|
||||
;; reads that property off whatever the caller passes, exactly as
|
||||
;; \`cfg.exitMinAtrMult\` would — the field just never appears in a
|
||||
|
|
|
|||
|
|
@ -1219,6 +1219,34 @@ export const TYPESCRIPT_SCOPE_QUERY = `
|
|||
(object
|
||||
(shorthand_property_identifier) @reference.name @reference.property-key @reference.value-ref)
|
||||
|
||||
;; Bare-identifier reads (A2), VALUE POSITIONS ONLY — a blanket \`(identifier)\`
|
||||
;; rule would mint a site for every token in the file.
|
||||
;;
|
||||
;; These existed only in the JavaScript query, so A2 did not work for
|
||||
;; TypeScript AT ALL: a \`.ts\` module reading its own \`const\` by bare name
|
||||
;; produced no reference site, and "who uses this constant?" answered a
|
||||
;; confident zero for an entire language. Found by writing the namespace
|
||||
;; fixture below and watching it fail for the wrong reason.
|
||||
(arguments
|
||||
(identifier) @reference.name @reference.read.identifier)
|
||||
|
||||
(assignment_pattern
|
||||
right: (identifier) @reference.name @reference.read.identifier)
|
||||
|
||||
(return_statement
|
||||
(identifier) @reference.name @reference.read.identifier)
|
||||
|
||||
;; \`const next = LIMIT\` and \`n > LIMIT\` — both plainly value reads, and both
|
||||
;; named in review as gaps between what A2 claimed and what it matched.
|
||||
(variable_declarator
|
||||
value: (identifier) @reference.name @reference.read.identifier)
|
||||
|
||||
(binary_expression
|
||||
left: (identifier) @reference.name @reference.read.identifier)
|
||||
|
||||
(binary_expression
|
||||
right: (identifier) @reference.name @reference.read.identifier)
|
||||
|
||||
;; References — TYPE POSITION (R2-2). An annotation naming a declared type is
|
||||
;; the only thing that makes that type's declaration reachable from the code
|
||||
;; that depends on it, and TypeScript captured none: only cpp and csharp emitted
|
||||
|
|
|
|||
|
|
@ -25,6 +25,7 @@ import { resolveCallerGraphId, resolveDefGraphId } from '../graph-bridge/ids.js'
|
|||
import { mapReferenceKindToEdgeType } from '../graph-bridge/edges.js';
|
||||
import type { GraphNodeLookup } from '../graph-bridge/node-lookup.js';
|
||||
import type { CalleeIdSink } from '../graph-bridge/callee-id-sink.js';
|
||||
import { isValueDefinitionLabel } from '../../utils/ast-helpers.js';
|
||||
|
||||
/**
|
||||
* Optional opaque skip key — providers may pre-emit edges (e.g. via
|
||||
|
|
@ -40,7 +41,6 @@ type ReferenceSiteSkipSet = ReadonlySet<string>;
|
|||
* only worth an edge when the def lives at module scope — see
|
||||
* `moduleScopeValueDefIds`.
|
||||
*/
|
||||
const LOCALIZABLE_VALUE_LABELS: ReadonlySet<string> = new Set(['Const', 'Variable', 'Static']);
|
||||
|
||||
export function emitReferencesViaLookup(
|
||||
graph: KnowledgeGraph,
|
||||
|
|
@ -112,7 +112,7 @@ export function emitReferencesViaLookup(
|
|||
if (
|
||||
moduleScopeValueDefIds !== undefined &&
|
||||
edgeType === 'ACCESSES' &&
|
||||
LOCALIZABLE_VALUE_LABELS.has(targetDef.type) &&
|
||||
isValueDefinitionLabel(targetDef.type) &&
|
||||
!moduleScopeValueDefIds.has(targetDef.nodeId)
|
||||
) {
|
||||
skipped++;
|
||||
|
|
|
|||
|
|
@ -94,6 +94,7 @@ import { buildWorkspaceResolutionIndex } from '../workspace-index.js';
|
|||
import type { ResolutionOutcome, ResolutionOutcomeRecorder } from '../resolution-outcome.js';
|
||||
import { logHeapProbe } from '../../utils/heap-probe.js';
|
||||
import { parseTruthyEnv } from '../../utils/env.js';
|
||||
import { isValueDefinitionLabel } from '../../utils/ast-helpers.js';
|
||||
import { TransitionalScopeTree } from '../../../../storage/scope-index-store.js';
|
||||
import { forceGc } from '../../../../storage/parsedfile-store.js';
|
||||
|
||||
|
|
@ -794,6 +795,51 @@ export function runScopeResolution(
|
|||
const tResolve = PROF ? process.hrtime.bigint() : 0n;
|
||||
logHeapProbe('sr-post-resolve', `lang=${provider.language}`);
|
||||
|
||||
// Value defs bound at MODULE LEVEL. A read of a block-local `const` must not
|
||||
// mint an edge — that would retain the inert locals `pruneLocalSymbols` drops.
|
||||
//
|
||||
// Built HERE, above the out-of-core seal, and deliberately from `parsedFiles`
|
||||
// rather than `emitParsedFiles`. The seal below replaces the latter with a
|
||||
// scope-STRIPPED copy, so building this after it walked `scopes: []` for every
|
||||
// file and produced an empty set — which the filter then reads as "no def is
|
||||
// module-level" and drops EVERY `Const`/`Variable`/`Static` ACCESSES edge in
|
||||
// the repo, in all languages, on the one path (`GITNEXUS_DISK_SCOPE_INDEX=1`)
|
||||
// taken by the largest repos. Nothing failed and nothing logged; the edges
|
||||
// were simply absent, which is the confident-empty answer this PR exists to
|
||||
// remove.
|
||||
//
|
||||
// `Module` is also not the only module level. A `Namespace` scope (TS
|
||||
// `namespace`, Rust `mod`, C++/C# `namespace`) holds importable values too,
|
||||
// and treating its consts as function-locals dropped their edges as well.
|
||||
// Included when the whole chain to the root is Module/Namespace — a namespace
|
||||
// declared inside a function body is a local like anything else there.
|
||||
const moduleScopeValueDefIds = new Set<string>();
|
||||
let moduleScopesInspected = false;
|
||||
for (const parsed of parsedFiles) {
|
||||
const scopeById = new Map(parsed.scopes.map((sc) => [sc.id, sc]));
|
||||
for (const scope of parsed.scopes) {
|
||||
if (scope.kind !== 'Module' && scope.kind !== 'Namespace') continue;
|
||||
let ancestor = scope.parent === null ? undefined : scopeById.get(scope.parent);
|
||||
let atModuleLevel = true;
|
||||
while (ancestor !== undefined) {
|
||||
if (ancestor.kind !== 'Module' && ancestor.kind !== 'Namespace') {
|
||||
atModuleLevel = false;
|
||||
break;
|
||||
}
|
||||
ancestor = ancestor.parent === null ? undefined : scopeById.get(ancestor.parent);
|
||||
}
|
||||
if (!atModuleLevel) continue;
|
||||
moduleScopesInspected = true;
|
||||
for (const [, refs] of scope.bindings) {
|
||||
for (const ref of refs) {
|
||||
if (isValueDefinitionLabel(ref.def.type)) {
|
||||
moduleScopeValueDefIds.add(ref.def.nodeId);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// ── Out-of-core scope seal boundary ─────────────────────────────────────
|
||||
// Pass-A (finalize + propagate + resolve) is done; all whole-language reads
|
||||
// of `Scope.bindings` are behind us. Emit reaches scopes ONLY via
|
||||
|
|
@ -927,22 +973,6 @@ export function runScopeResolution(
|
|||
);
|
||||
const referenceSkipSites = new Set(handledSites);
|
||||
for (const key of deferredIndirectSites) referenceSkipSites.add(key);
|
||||
// Value defs bound at MODULE scope. A read of a block-local `const` must not
|
||||
// mint an edge — that would retain the inert locals `pruneLocalSymbols`
|
||||
// drops. Built once here, where the parsed scopes are already in hand.
|
||||
const moduleScopeValueDefIds = new Set<string>();
|
||||
for (const parsed of emitParsedFiles) {
|
||||
const moduleScope = parsed.scopes.find((sc) => sc.kind === 'Module');
|
||||
if (moduleScope === undefined) continue;
|
||||
for (const [, refs] of moduleScope.bindings) {
|
||||
for (const ref of refs) {
|
||||
const t = ref.def.type;
|
||||
if (t === 'Const' || t === 'Variable' || t === 'Static') {
|
||||
moduleScopeValueDefIds.add(ref.def.nodeId);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
const { emitted, skipped } = callableFlowOnly
|
||||
? { emitted: 0, skipped: 0 }
|
||||
: emitReferencesViaLookup(
|
||||
|
|
@ -952,7 +982,12 @@ export function runScopeResolution(
|
|||
postHeritageNodeLookup,
|
||||
referenceSkipSites,
|
||||
calleeIdAccumulator,
|
||||
moduleScopeValueDefIds,
|
||||
// FAIL OPEN, not closed. An empty set is a legitimate answer ("this repo
|
||||
// has no module-level value defs, so every such target is a local"), but
|
||||
// it is indistinguishable from "the scopes could not be inspected" — and
|
||||
// in the second case arming the filter deletes a whole edge class. Only
|
||||
// pass the set when scopes were actually walked.
|
||||
moduleScopesInspected ? moduleScopeValueDefIds : undefined,
|
||||
);
|
||||
// Last-resort property resolution by workspace-unique name (A1/A5). Runs
|
||||
// after every precise pass and only sees what they left behind, so a
|
||||
|
|
|
|||
23
gitnexus/test/fixtures/lang-resolution/typescript-namespace-const/config.ts
vendored
Normal file
23
gitnexus/test/fixtures/lang-resolution/typescript-namespace-const/config.ts
vendored
Normal file
|
|
@ -0,0 +1,23 @@
|
|||
// RV-9: a const declared inside a TS `namespace`. Its binding scope is
|
||||
// `Namespace`, not `Module`, so the module-level set built for the block-local
|
||||
// filter did not contain it and its reads were dropped as if it were a local.
|
||||
//
|
||||
// The same shape exists in Rust (`mod`), C++ and C# — anywhere a language nests
|
||||
// an importable value one level below the file root.
|
||||
export namespace Limits {
|
||||
export const NAMESPACED_MAX = 42;
|
||||
|
||||
export function withinNamespace(): number {
|
||||
return NAMESPACED_MAX;
|
||||
}
|
||||
}
|
||||
|
||||
// CONTROL: a namespace declared INSIDE a function body is a local like anything
|
||||
// else there, so its const must stay excluded.
|
||||
export function makeLocalNamespace(): number {
|
||||
// eslint-disable-next-line @typescript-eslint/no-namespace
|
||||
namespace Inner {
|
||||
export const innerLocalValue = 7;
|
||||
}
|
||||
return Inner.innerLocalValue;
|
||||
}
|
||||
5
gitnexus/test/fixtures/lang-resolution/typescript-namespace-const/consumer.ts
vendored
Normal file
5
gitnexus/test/fixtures/lang-resolution/typescript-namespace-const/consumer.ts
vendored
Normal file
|
|
@ -0,0 +1,5 @@
|
|||
import { Limits } from './config.js';
|
||||
|
||||
export function readsNamespacedConst(): number {
|
||||
return Limits.NAMESPACED_MAX;
|
||||
}
|
||||
|
|
@ -53,6 +53,54 @@ describe('JavaScript module-scope const references (A2)', () => {
|
|||
expect(toLocal).toEqual([]);
|
||||
});
|
||||
|
||||
// The out-of-core path (#RV-2). Nothing in the suite exercised
|
||||
// `GITNEXUS_DISK_SCOPE_INDEX`, and that is where the module-level set was
|
||||
// being built from scope-STRIPPED files: it came out empty, which the filter
|
||||
// read as "no def is module-level" and used to drop every
|
||||
// `Const`/`Variable`/`Static` ACCESSES edge in the repo — including the ones
|
||||
// this suite exists to prove exist. It failed silently, on the path large
|
||||
// repos take, and no test could see it.
|
||||
//
|
||||
// Parity is the assertion: the seal is a memory optimization and must not
|
||||
// change a single edge.
|
||||
describe('under the out-of-core scope seal', () => {
|
||||
let sealed: PipelineResult;
|
||||
|
||||
beforeAll(async () => {
|
||||
const prev = process.env.GITNEXUS_DISK_SCOPE_INDEX;
|
||||
process.env.GITNEXUS_DISK_SCOPE_INDEX = '1';
|
||||
try {
|
||||
sealed = await runPipelineFromRepo(
|
||||
path.join(FIXTURES, 'javascript-const-references'),
|
||||
() => {},
|
||||
);
|
||||
} finally {
|
||||
if (prev === undefined) delete process.env.GITNEXUS_DISK_SCOPE_INDEX;
|
||||
else process.env.GITNEXUS_DISK_SCOPE_INDEX = prev;
|
||||
}
|
||||
}, 60000);
|
||||
|
||||
const sealedReaders = (): Set<string> =>
|
||||
new Set(
|
||||
getRelationships(sealed, 'ACCESSES')
|
||||
.filter((e) => e.target === 'DEFAULT_FETCH_LIMIT')
|
||||
.map((e) => e.source),
|
||||
);
|
||||
|
||||
it('keeps the const edges the unsealed run produced', () => {
|
||||
expect([...sealedReaders()].sort()).toEqual([...readersOfConst()].sort());
|
||||
});
|
||||
|
||||
it('still withholds the block-local edge', () => {
|
||||
// The filter must fail OPEN when scopes are unavailable, not be disabled:
|
||||
// the block-local exclusion is a correctness property, not an optimization.
|
||||
const toLocal = getRelationships(sealed, 'ACCESSES').filter(
|
||||
(e) => e.target === 'localScratchValue',
|
||||
);
|
||||
expect(toLocal).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
it('targets the Const node itself, not a same-named local', () => {
|
||||
const toConst = getRelationships(result, 'ACCESSES').filter(
|
||||
(e) => e.target === 'DEFAULT_FETCH_LIMIT',
|
||||
|
|
|
|||
|
|
@ -0,0 +1,41 @@
|
|||
/**
|
||||
* RV-9 — a const bound in a `Namespace` scope is module-level, not a local.
|
||||
*
|
||||
* The block-local filter added for A2 keeps a read of a block-scoped `const`
|
||||
* from minting an edge, because such an edge would retain exactly the inert
|
||||
* locals `pruneLocalSymbols` exists to drop. It decided "is this module-level?"
|
||||
* by asking `kind === 'Module'`, which is true of the file root and of nothing
|
||||
* else — so a value declared in a TS `namespace` (or a Rust `mod`, or a C++ /
|
||||
* C# namespace) was classified as a function-local and its reads were dropped.
|
||||
*
|
||||
* The feature simply did not work there. Reported as a gap rather than a
|
||||
* regression: no pre-existing edge was deleted, because the other languages'
|
||||
* read/write captures are member-shaped and target `Property`, not
|
||||
* `Const`/`Variable`/`Static`.
|
||||
*/
|
||||
import { describe, it, expect, beforeAll } from 'vitest';
|
||||
import path from 'path';
|
||||
import { FIXTURES, getRelationships, runPipelineFromRepo, type PipelineResult } from './helpers.js';
|
||||
|
||||
describe('TypeScript namespace-scoped const references (RV-9)', () => {
|
||||
let result: PipelineResult;
|
||||
|
||||
beforeAll(async () => {
|
||||
result = await runPipelineFromRepo(path.join(FIXTURES, 'typescript-namespace-const'), () => {});
|
||||
}, 60000);
|
||||
|
||||
const readersOf = (name: string): string[] =>
|
||||
getRelationships(result, 'ACCESSES')
|
||||
.filter((e) => e.target === name)
|
||||
.map((e) => e.source);
|
||||
|
||||
it('emits an edge for a read of a namespace-scoped const', () => {
|
||||
expect(readersOf('NAMESPACED_MAX')).toContain('withinNamespace');
|
||||
});
|
||||
|
||||
// The bound. A namespace nested in a function body is a local like anything
|
||||
// else declared there, so widening "module level" must not reach into one.
|
||||
it('still withholds an edge to a const in a function-local namespace', () => {
|
||||
expect(readersOf('innerLocalValue')).toEqual([]);
|
||||
});
|
||||
});
|
||||
Loading…
Add table
Reference in a new issue