From 47d8472c03825471bf612fef55171b78916e09b4 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Fri, 29 May 2026 18:07:29 +0000 Subject: [PATCH] fix(csharp): apply PR-review polish to namespace-siblings (tests, types, docs) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the multi-agent code review of this PR — the concrete, defensible findings. Two items intentionally deferred (below). - namespace-siblings.ts: couple the augmentation bucket + its de-dup set into one nullable lifecycle, removing the seen!/bucketArr! non-null assertions (identical runtime, still lazy). - validate-bindings-immutability.ts: extend the dev-mode immutability validator to the third channel (workspaceFqnBindings) + a test; complete the validator test mock with workspaceFqnBindings. - walkers.ts: document that namesAtScope deliberately excludes the scope-independent workspaceFqnBindings channel (enumerating workspace names at every scope would flood per-scope callers; lookupBindingsAt still consults it when resolving a specific name). - scope-resolution-indexes.ts: reframe the workspaceFqnBindings doc to describe the key-format contract language-neutrally (examples, not language branching). - csharp-hooks.test.ts: assert workspace entries carry origin:'namespace'; add a partial-class test (same simple name, distinct nodeIds across global files → both kept); rename the stale "parses" cache-miss test to "scans". - csharp-pipeline-benchmark.test.ts: clearTimeout the Promise.race budget timer (dangling handle when the pipeline won the race). - csharp.test.ts: correct the #1066 comment — extractFileStructure no longer re-parses on cache miss (line scanner); only emitCsharpScopeCaptures re-parses. Deferred (surfaced, not applied): (1) worker-path scanner mis-reads namespace/using-static inside block comments and verbatim/raw strings — an explicitly documented trade-off mirroring the PHP scanner; hardening it to track comment/string state is a separate decision. (2) workspaceFqnBindings is read via an `as Map` cast; a type-safe mutable handle from finalize-orchestrator is a cross-module contract change. Verified: tsc --noEmit clean; 49 unit tests pass (incl. 3 new); prettier clean; eslint 0 errors. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../languages/csharp/namespace-siblings.ts | 22 ++++--- .../model/scope-resolution-indexes.ts | 14 ++--- .../validate-bindings-immutability.ts | 20 +++++- .../scope-resolution/scope/walkers.ts | 8 +++ .../csharp-pipeline-benchmark.test.ts | 10 +-- .../test/integration/resolvers/csharp.test.ts | 9 ++- .../csharp/csharp-hooks.test.ts | 62 ++++++++++++++++++- .../validate-bindings-immutability.test.ts | 20 ++++++ 8 files changed, 139 insertions(+), 26 deletions(-) diff --git a/gitnexus/src/core/ingestion/languages/csharp/namespace-siblings.ts b/gitnexus/src/core/ingestion/languages/csharp/namespace-siblings.ts index 15938fb59..97348095d 100644 --- a/gitnexus/src/core/ingestion/languages/csharp/namespace-siblings.ts +++ b/gitnexus/src/core/ingestion/languages/csharp/namespace-siblings.ts @@ -472,21 +472,25 @@ export function populateCsharpNamespaceSiblings( const local = localScope?.bindings.get(name); if (local !== undefined && local.some((b) => b.origin === 'local')) continue; - let bucketArr: BindingRef[] | null = null; - let seen: Set | null = null; + // Bind the augmentation bucket and its seeded de-dup set together + // under one nullable lifecycle, so neither needs a non-null + // assertion (they are always set or unset as a pair). Stays lazy: + // nothing is allocated for a name with no cross-file defs. + let inject: { bucket: BindingRef[]; seen: Set } | null = null; for (const def of defs) { if (def.filePath === filePath) continue; // don't self-reference - if (bucketArr === null) { - bucketArr = getAugmentationBucket(augmentations, scopeId, name); + if (inject === null) { + const bucket = getAugmentationBucket(augmentations, scopeId, name); // Seed the de-dup set from any entries an earlier pass // (using-static / cross-namespace imports) already added, // replacing the per-def O(A) `.some` scan. - seen = new Set(); - for (const b of bucketArr) seen.add(b.def.nodeId); + const seen = new Set(); + for (const b of bucket) seen.add(b.def.nodeId); + inject = { bucket, seen }; } - if (seen!.has(def.nodeId)) continue; - seen!.add(def.nodeId); - bucketArr.push({ def, origin: 'namespace' }); + if (inject.seen.has(def.nodeId)) continue; + inject.seen.add(def.nodeId); + inject.bucket.push({ def, origin: 'namespace' }); } } } diff --git a/gitnexus/src/core/ingestion/model/scope-resolution-indexes.ts b/gitnexus/src/core/ingestion/model/scope-resolution-indexes.ts index 435a058ab..71e78f567 100644 --- a/gitnexus/src/core/ingestion/model/scope-resolution-indexes.ts +++ b/gitnexus/src/core/ingestion/model/scope-resolution-indexes.ts @@ -79,13 +79,13 @@ export interface ScopeResolutionIndexes { readonly bindingAugmentations: ReadonlyMap>; /** Workspace-level binding lookup, shared instead of per-scope * duplication. Consulted by `lookupBindingsAt` as a third source after - * finalized and per-scope augmented bindings. Two languages populate it - * with disjoint key formats that never collide: - * - PHP namespace-siblings (Step 3b): backslash-separated FQNs - * (e.g. `App\Models\User`). - * - C# namespace-siblings: simple names for global-(default-)namespace - * types (e.g. `User`), which are visible from every file — one entry - * per simple name instead of O(scopes × defs) per-scope augmentation. */ + * finalized and per-scope augmented bindings. Language-specific + * namespace-sibling hooks populate it with disjoint key formats that + * never collide — e.g. backslash-separated FQNs (`App\Models\User`) for + * backslash-namespace languages, and bare simple names (`User`) for + * global-/default-namespace types that are visible from every file. The + * shared map gives those workspace-wide names one entry each instead of + * O(scopes × defs) per-scope augmentation. */ readonly workspaceFqnBindings: ReadonlyMap; /** Pre-resolution usage facts; consumed by the resolution phase. */ readonly referenceSites: readonly ReferenceSite[]; diff --git a/gitnexus/src/core/ingestion/scope-resolution/pipeline/validate-bindings-immutability.ts b/gitnexus/src/core/ingestion/scope-resolution/pipeline/validate-bindings-immutability.ts index 60e5fe179..5ccba91ac 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/pipeline/validate-bindings-immutability.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/pipeline/validate-bindings-immutability.ts @@ -1,6 +1,6 @@ /** - * Dev-mode runtime validator for the two-channel binding lifecycle - * (Contract Invariant I8 in `contract/scope-resolver.ts`). + * Dev-mode runtime validator for the post-finalize binding-channel + * lifecycle (Contract Invariant I8 in `contract/scope-resolver.ts`). * * The two channels: * - `indexes.bindings` — finalize-output channel. After @@ -74,5 +74,21 @@ export function validateBindingsImmutability( } } + // Third channel: `workspaceFqnBindings` (scope-independent, shared map + // populated by language namespace-sibling hooks — PHP FQN keys, C# + // global-namespace simple names). Like bindingAugmentations its inner + // arrays are mutable by contract (hooks `push()` directly), so freezing + // one is the same defect as freezing an augmentation bucket. + for (const [name, bucket] of indexes.workspaceFqnBindings) { + if (Object.isFrozen(bucket)) { + onWarn( + `binding-immutability: indexes.workspaceFqnBindings[${name}] is FROZEN — ` + + `the workspace channel is mutable by contract; freezing it defeats the ` + + `append-only purpose. See ScopeResolver Invariant I8.`, + ); + violations++; + } + } + return violations; } diff --git a/gitnexus/src/core/ingestion/scope-resolution/scope/walkers.ts b/gitnexus/src/core/ingestion/scope-resolution/scope/walkers.ts index df6d259b5..6bf967fff 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/scope/walkers.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/scope/walkers.ts @@ -99,6 +99,14 @@ const EMPTY_NAMES: Iterable = Object.freeze([]) as readonly string[]; * Fast paths (zero allocation) when at most one channel is populated: * returns the underlying `Map.keys()` iterator directly. Only when both * channels carry names do we materialize a `Set` for deduplication. + * + * Scope: enumerates only the per-scope `bindings` and `bindingAugmentations` + * channels. It deliberately EXCLUDES the scope-independent + * `workspaceFqnBindings` channel (PHP FQN keys, C# global-namespace simple + * names). `lookupBindingsAt` consults that third channel when resolving a + * specific name, but name *enumeration* here does not — those names apply at + * every scope and would flood per-scope callers. Callers that need + * workspace-level names must read `workspaceFqnBindings` directly. */ export function namesAtScope(scopeId: ScopeId, scopes: ScopeResolutionIndexes): Iterable { const finalized = scopes.bindings.get(scopeId); diff --git a/gitnexus/test/integration/csharp-pipeline-benchmark.test.ts b/gitnexus/test/integration/csharp-pipeline-benchmark.test.ts index 6aed2cd8b..998c3c3e9 100644 --- a/gitnexus/test/integration/csharp-pipeline-benchmark.test.ts +++ b/gitnexus/test/integration/csharp-pipeline-benchmark.test.ts @@ -147,17 +147,18 @@ async function runBenchmark( if (heap > peakHeapMB) peakHeapMB = heap; }, 50); + let budgetTimer: ReturnType | undefined; try { const start = Date.now(); const result = await Promise.race([ runPipelineFromRepo(dir, () => {}, { skipGraphPhases: true }), - new Promise((_, reject) => - setTimeout( + new Promise((_, reject) => { + budgetTimer = setTimeout( () => reject(new Error(`Pipeline exceeded ${budgetMs}ms at ${fileCount} files (${shape})`)), budgetMs, - ), - ), + ); + }), ]); const elapsedMs = Date.now() - start; @@ -172,6 +173,7 @@ async function runBenchmark( }; } finally { clearInterval(heapSampler); + clearTimeout(budgetTimer); fs.rmSync(dir, { recursive: true, force: true }); } } diff --git a/gitnexus/test/integration/resolvers/csharp.test.ts b/gitnexus/test/integration/resolvers/csharp.test.ts index 073b2da16..d837c32d3 100644 --- a/gitnexus/test/integration/resolvers/csharp.test.ts +++ b/gitnexus/test/integration/resolvers/csharp.test.ts @@ -2447,9 +2447,12 @@ describe('C# class-name receiver write ACCESSES (merged Case 2 kind-aware branch // cross-namespace `using` and a colliding local class. Pins both fixes in // the resolver dataset: // -// 1. emitCsharpScopeCaptures + extractFileStructure must use the adaptive -// `getTreeSitterBufferSize` on cache miss, otherwise UserService.cs -// fails to reparse with "Invalid argument" and CreateUser is dropped. +// 1. emitCsharpScopeCaptures must use the adaptive `getTreeSitterBufferSize` +// on cache miss, otherwise UserService.cs fails to reparse with "Invalid +// argument" and CreateUser is dropped. (extractFileStructure no longer +// re-parses on cache miss — it uses the line scanner, +// extractCsharpStructureViaScanner — so this fixture's line-anchored +// namespaces are read identically by either branch.) // 2. populateCsharpNamespaceSiblings must append to bindingAugmentations // instead of mutating frozen finalize-produced BindingRef[] arrays; // otherwise the cross-namespace inject loop throws "Cannot add property diff --git a/gitnexus/test/unit/scope-resolution/csharp/csharp-hooks.test.ts b/gitnexus/test/unit/scope-resolution/csharp/csharp-hooks.test.ts index 63f490919..d0bfb3784 100644 --- a/gitnexus/test/unit/scope-resolution/csharp/csharp-hooks.test.ts +++ b/gitnexus/test/unit/scope-resolution/csharp/csharp-hooks.test.ts @@ -283,7 +283,7 @@ describe('populateCsharpNamespaceSiblings', () => { expect(Object.isFrozen(augmented)).toBe(false); }); - it('parses UTF-8-heavy cache-miss files before namespace sibling injection', () => { + it('scans (no re-parse) UTF-8-heavy cache-miss files before namespace sibling injection', () => { const sibling = classDef('def:b.B', 'b.cs', 'Demo.B'); const moduleA = scope('scope:a:module', 'Module', 'a.cs'); const moduleB = scope('scope:b:module', 'Module', 'b.cs'); @@ -383,6 +383,66 @@ describe('populateCsharpNamespaceSiblings', () => { expect(workspaceFqnBindings.size).toBe(2); // The whole point of the fast path: no per-scope augmentation explosion. expect(bindingAugmentations.size).toBe(0); + // Workspace entries carry the cross-file `namespace` origin (so shadowing + // precedence in lookupBindingsAt orders them after local/finalized). + expect(workspaceFqnBindings.get('A')?.[0]?.origin).toBe('namespace'); + }); + + it('keeps every declaration of a repeated global simple name (partial classes across files)', () => { + // Two global-namespace files each declare `Foo` (a partial class split + // across files => distinct nodeIds, same simple name). Both must survive + // in the workspace channel — the fast path de-dups by nodeId, not by name, + // so partial-class members from both files stay resolvable. + const foo1 = classDef('def:foo1.Foo', 'foo1.cs', 'Foo'); + const foo2 = classDef('def:foo2.Foo', 'foo2.cs', 'Foo'); + const moduleA = scope('scope:foo1:module', 'Module', 'foo1.cs'); + const classA = scope('scope:foo1:class', 'Class', 'foo1.cs', moduleA.id, [foo1]); + const moduleB = scope('scope:foo2:module', 'Module', 'foo2.cs'); + const classB = scope('scope:foo2:class', 'Class', 'foo2.cs', moduleB.id, [foo2]); + const parsedFiles: ParsedFile[] = [ + { + filePath: 'foo1.cs', + moduleScope: moduleA.id, + scopes: Object.freeze([moduleA, classA]), + parsedImports: Object.freeze([]), + localDefs: Object.freeze([foo1]), + referenceSites: Object.freeze([]), + } as ParsedFile, + { + filePath: 'foo2.cs', + moduleScope: moduleB.id, + scopes: Object.freeze([moduleB, classB]), + parsedImports: Object.freeze([]), + localDefs: Object.freeze([foo2]), + referenceSites: Object.freeze([]), + } as ParsedFile, + ]; + const bindingAugmentations = new Map>(); + const workspaceFqnBindings = new Map(); + + populateCsharpNamespaceSiblings( + parsedFiles, + { + bindings: new Map(), + bindingAugmentations, + workspaceFqnBindings, + } as unknown as ScopeResolutionIndexes, + { + fileContents: new Map([ + ['foo1.cs', 'class Foo { }\n'], + ['foo2.cs', 'class Foo { }\n'], + ]), + }, + ); + + // Both partial declarations are kept (de-dup is by nodeId, not name). + expect( + workspaceFqnBindings + .get('Foo') + ?.map((b) => b.def.nodeId) + .sort(), + ).toEqual(['def:foo1.Foo', 'def:foo2.Foo']); + expect(bindingAugmentations.size).toBe(0); }); }); diff --git a/gitnexus/test/unit/scope-resolution/validate-bindings-immutability.test.ts b/gitnexus/test/unit/scope-resolution/validate-bindings-immutability.test.ts index bcf23200c..880fcedf1 100644 --- a/gitnexus/test/unit/scope-resolution/validate-bindings-immutability.test.ts +++ b/gitnexus/test/unit/scope-resolution/validate-bindings-immutability.test.ts @@ -22,10 +22,12 @@ const mkRef = (nodeId: string): BindingRef => const mkIndexes = ( bindings: Map>, augmentations: Map>, + workspace: Map = new Map(), ): ScopeResolutionIndexes => ({ bindings, bindingAugmentations: augmentations, + workspaceFqnBindings: workspace, }) as unknown as ScopeResolutionIndexes; describe('validateBindingsImmutability', () => { @@ -88,6 +90,24 @@ describe('validateBindingsImmutability', () => { expect(onWarn.mock.calls[0][0]).toMatch(/I8/); }); + it('warns when a bucket in indexes.workspaceFqnBindings IS frozen', () => { + vi.stubEnv('NODE_ENV', 'development'); + const workspace = new Map([ + ['User', Object.freeze([mkRef('def:User')]) as BindingRef[]], + ]); + const onWarn = vi.fn(); + + const violations = validateBindingsImmutability( + mkIndexes(new Map(), new Map(), workspace), + onWarn, + ); + + expect(violations).toBe(1); + expect(onWarn).toHaveBeenCalledTimes(1); + expect(onWarn.mock.calls[0][0]).toMatch(/indexes\.workspaceFqnBindings/); + expect(onWarn.mock.calls[0][0]).toMatch(/I8/); + }); + it('does not detect semantically wrong frozen replacements in indexes.bindings', () => { vi.stubEnv('NODE_ENV', 'development'); const bindings = new Map>([