From da2d10e18f2d1da0641c7e3fdddc9f9901a84829 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Fri, 29 May 2026 18:16:46 +0000 Subject: [PATCH] fix(csharp): harden worker-path scanner + localize workspace-map cast MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the two deferred PR-review findings plus the remaining test gap. #1 — Worker-path scanner false positives: the line scanner now tracks block- comment and string state across lines (advanceCsScanState), so a `namespace` / `using static` keyword at the start of a line inside a block comment, verbatim string (@"..."), or raw string literal ("""...""") is no longer mistaken for a declaration on the worker cache-miss path. It matches only at code-state line starts. 5 new scanner tests cover the block-comment / raw / verbatim cases. #4 — workspaceFqnBindings type safety: the ReadonlyMap->Map cast is localized to one documented line, and global-namespace writes go through a new getWorkspaceBucket helper (mirroring getAugmentationBucket) rather than an inline `.set()` at the mutation site. #2 — lookupBindingsAt workspace-channel coverage: walkers-augmentations.test.ts now exercises the third (workspace) channel: workspace-only, append-after- finalized/augmented, and dedup-loses-to-finalized/augmented precedence. #5 — OOM CI guard: the deterministic O(D) invariant (zero per-scope augmentation for global types) is already asserted by the always-on csharp-hooks unit tests added earlier; the scale/time benchmark stays appropriately opt-in (skipIf). Verified: tsc --noEmit clean; 69 unit tests (4 suites) + 210 C# integration resolver tests pass; prettier clean; eslint 0 errors. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../languages/csharp/namespace-siblings.ts | 192 +++++++++++++++--- .../unit/csharp-namespace-extraction.test.ts | 28 +++ .../walkers-augmentations.test.ts | 37 +++- 3 files changed, 225 insertions(+), 32 deletions(-) diff --git a/gitnexus/src/core/ingestion/languages/csharp/namespace-siblings.ts b/gitnexus/src/core/ingestion/languages/csharp/namespace-siblings.ts index 97348095d..59d191af6 100644 --- a/gitnexus/src/core/ingestion/languages/csharp/namespace-siblings.ts +++ b/gitnexus/src/core/ingestion/languages/csharp/namespace-siblings.ts @@ -50,10 +50,11 @@ export interface CsharpFileStructure { readonly usingStaticPaths: readonly string[]; } -// Line-anchored scanners for the worker-path fallback (see +// Line-anchored matchers for the worker-path fallback (see // `extractCsharpStructureViaScanner`). Anchored at line start (after -// indentation) so `// namespace X` comments and string literals don't -// match — the same false-positive trade-off PHP's scanner accepts. +// indentation); the scanner additionally tracks block-comment / string +// state across lines so a keyword at the start of a line inside one of +// those regions is skipped. const CS_NAMESPACE_RE = /^[ \t]*namespace[ \t]+([A-Za-z_@][A-Za-z0-9_.]*)/; // `global using static`, plain `using static`, and the aliased // `using static Alias = NS.Type;` form (the AST keeps the RHS path, so @@ -61,28 +62,144 @@ const CS_NAMESPACE_RE = /^[ \t]*namespace[ \t]+([A-Za-z_@][A-Za-z0-9_.]*)/; const CS_USING_STATIC_RE = /^[ \t]*(?:global[ \t]+)?using[ \t]+static[ \t]+(?:[A-Za-z_@][A-Za-z0-9_]*[ \t]*=[ \t]*)?([A-Za-z_@][A-Za-z0-9_.]*)/; -/** Line-scanner used when no cached tree is available (worker-parsed - * files can't transfer native tree-sitter Trees across MessageChannels, - * so `treeCache` is empty for them). Re-parsing every C# file here with +/** Multi-line lexical state carried line-to-line by the scanner. */ +type CsScanState = 'code' | 'block' | 'verbatim' | 'raw'; + +/** Advance the scanner's lexical state across one line, consuming block + * comments (slash-star), line comments (`//`), single-line regular / + * interpolated strings, verbatim strings (`@"…"`), and raw string literals + * (`"""…"""`, fence length tracked in `rawFence`). Returns the state and + * raw-fence length in effect at the START of the next line. Single-line + * strings and `//` comments resolve back to `code` before end of line; only + * block comments and multi-line strings carry state forward. */ +function advanceCsScanState( + line: string, + state: CsScanState, + rawFence: number, +): [CsScanState, number] { + const n = line.length; + let i = 0; + while (i < n) { + if (state === 'block') { + const end = line.indexOf('*/', i); + if (end === -1) return ['block', rawFence]; + i = end + 2; + state = 'code'; + } else if (state === 'verbatim') { + // Ends at a `"` that is not doubled (`""` is an escaped quote). + while (i < n) { + if (line[i] === '"') { + if (line[i + 1] === '"') { + i += 2; + continue; + } + break; + } + i++; + } + if (i >= n) return ['verbatim', rawFence]; + i += 1; + state = 'code'; + } else if (state === 'raw') { + // Ends at a run of `"` at least `rawFence` long. + let closed = false; + while (i < n) { + if (line[i] === '"') { + let k = i; + while (k < n && line[k] === '"') k++; + if (k - i >= rawFence) { + i = k; + state = 'code'; + rawFence = 0; + closed = true; + break; + } + i = k; + } else { + i++; + } + } + if (!closed) return ['raw', rawFence]; + } else { + const c = line[i]; + const next = line[i + 1]; + if (c === '/' && next === '/') return ['code', rawFence]; // line comment to EOL + if (c === '/' && next === '*') { + state = 'block'; + i += 2; + } else if (c === '@' && next === '"') { + state = 'verbatim'; + i += 2; + } else if ((c === '$' && next === '@') || (c === '@' && next === '$')) { + if (line[i + 2] === '"') { + state = 'verbatim'; // interpolated verbatim ($@"…" / @$"…") + i += 3; + } else { + i++; + } + } else if (c === '"') { + let k = i; + while (k < n && line[k] === '"') k++; + const run = k - i; + if (run >= 3) { + state = 'raw'; + rawFence = run; + i = k; + } else if (run === 2) { + i = k; // "" — empty string + } else { + // single-line regular / interpolated string; consume to closer + let j = i + 1; + while (j < n) { + if (line[j] === '\\') { + j += 2; + continue; + } + if (line[j] === '"') break; + j++; + } + i = j >= n ? n : j + 1; + } + } else { + i++; + } + } + } + return [state, rawFence]; +} + +/** Line-scanner used when no cached tree is available (worker-parsed files + * can't transfer native tree-sitter Trees across MessageChannels, so + * `treeCache` is empty for them). Re-parsing every C# file here with * tree-sitter was the dominant scope-resolution cost on large worker-mode * runs — for a multi-thousand-file solution this loop alone re-parsed the * whole repo a second time. The scanner extracts the same `namespaces` / - * `usingStaticPaths` the AST walk produces for the common (line-anchored) - * declarations, trading exact handling of the rare cases (declarations - * split across lines, namespace keywords inside block comments/strings) - * for eliminating those re-parses. Mirrors PHP's `extractNamespaceViaScanner` - * (issue #1741). */ + * `usingStaticPaths` the AST walk produces for line-anchored declarations, + * while tracking block-comment and string state across lines (via + * `advanceCsScanState`) so a `namespace` / `using static` keyword at the + * start of a line inside a block comment, verbatim string, or raw string + * literal is NOT mistaken for a declaration. The remaining trade-off vs the + * AST is a declaration whose keyword is not at the start of a code line + * (split across lines, or sharing a line with a comment/string closer). + * Mirrors PHP's `extractNamespaceViaScanner` (issue #1741). */ export function extractCsharpStructureViaScanner(content: string): CsharpFileStructure { const namespaces: string[] = []; const usingStaticPaths: string[] = []; + let state: CsScanState = 'code'; + let rawFence = 0; for (const line of content.split('\n')) { - const ns = CS_NAMESPACE_RE.exec(line); - if (ns !== null) { - namespaces.push(ns[1]!); - continue; + // Only match when the line START is real code — keywords reached while + // inside a block comment / multi-line string are skipped. + if (state === 'code') { + const ns = CS_NAMESPACE_RE.exec(line); + if (ns !== null) { + namespaces.push(ns[1]!); + } else { + const us = CS_USING_STATIC_RE.exec(line); + if (us !== null) usingStaticPaths.push(us[1]!); + } } - const us = CS_USING_STATIC_RE.exec(line); - if (us !== null) usingStaticPaths.push(us[1]!); + [state, rawFence] = advanceCsScanState(line, state, rawFence); } return { namespaces, usingStaticPaths }; } @@ -396,8 +513,12 @@ export function populateCsharpNamespaceSiblings( // Workspace-level binding channel for global-namespace types (see the // global fast-path below). `lookupBindingsAt` consults this as a third - // source after finalized + per-scope augmented bindings. - const fqnMap = indexes.workspaceFqnBindings as Map; + // source after finalized + per-scope augmented bindings. Its inner arrays + // are mutable by contract (append-only, like `bindingAugmentations` — see + // the ScopeResolutionIndexes doc + validateBindingsImmutability), so the + // ReadonlyMap→Map cast is localized to this one line and all writes go + // through `getWorkspaceBucket`. + const workspace = indexes.workspaceFqnBindings as Map; for (const [nsName, bucket] of buckets) { // Group sibling defs by simple name. Append in place — the previous @@ -433,20 +554,13 @@ export function populateCsharpNamespaceSiblings( // partial-class / duplicate declarations from double-emitting. if (nsName === '') { for (const [name, defs] of defsByName) { - let arr = fqnMap.get(name); - let seen: Set | null = null; + const bucket = getWorkspaceBucket(workspace, name); + const seen = new Set(); + for (const b of bucket) seen.add(b.def.nodeId); for (const def of defs) { - if (arr === undefined) { - arr = []; - fqnMap.set(name, arr); - } - if (seen === null) { - seen = new Set(); - for (const b of arr) seen.add(b.def.nodeId); - } - if (seen.has(def.nodeId)) continue; + if (seen.has(def.nodeId)) continue; // dedup by nodeId (keeps partials, drops re-emits) seen.add(def.nodeId); - arr.push({ def, origin: 'namespace' }); + bucket.push({ def, origin: 'namespace' }); } } continue; @@ -521,6 +635,22 @@ function getAugmentationBucket( return bucketArr; } +/** Get-or-create a mutable inner bucket inside the `workspaceFqnBindings` + * channel (the scope-independent third channel; see + * `ScopeResolutionIndexes.workspaceFqnBindings`). Like + * `getAugmentationBucket`, the inner arrays are mutable by contract — + * callers `push` directly. Keeping the get-or-create here means the one + * ReadonlyMap→Map cast at the call site is the only place the mutable + * view is taken. */ +function getWorkspaceBucket(workspace: Map, name: string): BindingRef[] { + let bucketArr = workspace.get(name); + if (bucketArr === undefined) { + bucketArr = []; + workspace.set(name, bucketArr); + } + return bucketArr; +} + function isTypeDef(def: SymbolDefinition): boolean { return ( def.type === 'Class' || diff --git a/gitnexus/test/unit/csharp-namespace-extraction.test.ts b/gitnexus/test/unit/csharp-namespace-extraction.test.ts index 11ccd24ed..7f39a061f 100644 --- a/gitnexus/test/unit/csharp-namespace-extraction.test.ts +++ b/gitnexus/test/unit/csharp-namespace-extraction.test.ts @@ -68,4 +68,32 @@ describe('extractCsharpStructureViaScanner', () => { expect(out.namespaces).toEqual([]); expect(out.usingStaticPaths).toEqual([]); }); + + // Cross-line comment/string state: a keyword at the start of a line inside + // a block comment or multi-line string must NOT be read as a declaration + // (the worker path would otherwise mis-bucket the file vs the AST). + it('skips a `namespace` line inside a block comment', () => { + const src = `/*\nnamespace Fake.InComment;\n*/\nnamespace App.Real;`; + expect(extractCsharpStructureViaScanner(src).namespaces).toEqual(['App.Real']); + }); + + it('skips a `using static` line inside a block comment', () => { + const src = `/*\nusing static Fake.Helpers;\n*/\nusing static App.Real.Helpers;`; + expect(extractCsharpStructureViaScanner(src).usingStaticPaths).toEqual(['App.Real.Helpers']); + }); + + it('skips a `namespace` line inside a raw string literal', () => { + const src = `var sql = """\nnamespace Fake.InRaw;\n""";\nnamespace App.Real;`; + expect(extractCsharpStructureViaScanner(src).namespaces).toEqual(['App.Real']); + }); + + it('skips a `namespace` line inside a verbatim string literal', () => { + const src = `var s = @"\nnamespace Fake.InVerbatim;\n";\nnamespace App.Real;`; + expect(extractCsharpStructureViaScanner(src).namespaces).toEqual(['App.Real']); + }); + + it('still reads a real declaration after a closed same-line block comment', () => { + const src = `/* header */ class C {}\nnamespace App.Real;`; + expect(extractCsharpStructureViaScanner(src).namespaces).toEqual(['App.Real']); + }); }); diff --git a/gitnexus/test/unit/scope-resolution/walkers-augmentations.test.ts b/gitnexus/test/unit/scope-resolution/walkers-augmentations.test.ts index d83e1fa36..21d48fc49 100644 --- a/gitnexus/test/unit/scope-resolution/walkers-augmentations.test.ts +++ b/gitnexus/test/unit/scope-resolution/walkers-augmentations.test.ts @@ -32,9 +32,11 @@ const ref = (nodeId: string, origin: BindingRef['origin'] = 'local'): BindingRef function indexesWith({ finalized, augmented, + workspace, }: { finalized?: readonly BindingRef[]; augmented?: readonly BindingRef[]; + workspace?: readonly BindingRef[]; }): ScopeResolutionIndexes { const bindings = new Map>(); if (finalized !== undefined) { @@ -43,7 +45,13 @@ function indexesWith({ } const bindingAugmentations = new Map>(); if (augmented !== undefined) bindingAugmentations.set(SCOPE, new Map([['name', augmented]])); - return { bindings, bindingAugmentations } as unknown as ScopeResolutionIndexes; + const workspaceFqnBindings = new Map(); + if (workspace !== undefined) workspaceFqnBindings.set('name', workspace); + return { + bindings, + bindingAugmentations, + workspaceFqnBindings, + } as unknown as ScopeResolutionIndexes; } function scope(id: ScopeId, bindings = new Map()): Scope { @@ -105,6 +113,33 @@ describe('lookupBindingsAt', () => { expect(out.find((b) => b.def.nodeId === 'A')!.origin).toBe('import'); }); + // Third channel: workspaceFqnBindings (scope-independent — global-namespace + // C# types / PHP FQNs). Consulted LAST, after finalized + augmented. + it('returns the workspace bucket when it is the only channel', () => { + const workspace = [ref('W', 'namespace')]; + const out = lookupBindingsAt(SCOPE, 'name', indexesWith({ workspace })); + expect(out).toEqual(workspace); + expect(out).toBe(workspace); // identity preserved when only one channel populates + }); + + it('appends workspace entries after finalized and augmented', () => { + const finalized = [ref('A', 'import')]; + const augmented = [ref('B', 'namespace')]; + const workspace = [ref('C', 'namespace')]; + const out = lookupBindingsAt(SCOPE, 'name', indexesWith({ finalized, augmented, workspace })); + expect(out.map((b) => b.def.nodeId)).toEqual(['A', 'B', 'C']); + }); + + it('dedupes workspace entries already present in finalized/augmented (workspace loses)', () => { + const finalized = [ref('A', 'import')]; + const augmented = [ref('B', 'namespace')]; + const workspace = [ref('A', 'namespace'), ref('B', 'namespace'), ref('C', 'namespace')]; + const out = lookupBindingsAt(SCOPE, 'name', indexesWith({ finalized, augmented, workspace })); + expect(out.map((b) => b.def.nodeId)).toEqual(['A', 'B', 'C']); + // The surviving A/B keep their finalized/augmented identity, not workspace's. + expect(out.find((b) => b.def.nodeId === 'A')!.origin).toBe('import'); + }); + it('keeps finalized metadata when the same nodeId appears in both channels', () => { const finalizedDef = { nodeId: 'A',