mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-04 02:31:36 +00:00
fix(csharp): apply PR-review polish to namespace-siblings (tests, types, docs)
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) <noreply@anthropic.com>
This commit is contained in:
Gergo Magyar
parent
f0381b2616
commit
47d8472c03
8 changed files with 139 additions and 26 deletions
|
|
@ -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<string> | 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<string> } | 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<string>();
|
||||
for (const b of bucketArr) seen.add(b.def.nodeId);
|
||||
const seen = new Set<string>();
|
||||
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' });
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -79,13 +79,13 @@ export interface ScopeResolutionIndexes {
|
|||
readonly bindingAugmentations: ReadonlyMap<ScopeId, ReadonlyMap<string, readonly BindingRef[]>>;
|
||||
/** 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<string, readonly BindingRef[]>;
|
||||
/** Pre-resolution usage facts; consumed by the resolution phase. */
|
||||
readonly referenceSites: readonly ReferenceSite[];
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -99,6 +99,14 @@ const EMPTY_NAMES: Iterable<string> = 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<string> {
|
||||
const finalized = scopes.bindings.get(scopeId);
|
||||
|
|
|
|||
|
|
@ -147,17 +147,18 @@ async function runBenchmark(
|
|||
if (heap > peakHeapMB) peakHeapMB = heap;
|
||||
}, 50);
|
||||
|
||||
let budgetTimer: ReturnType<typeof setTimeout> | undefined;
|
||||
try {
|
||||
const start = Date.now();
|
||||
const result = await Promise.race([
|
||||
runPipelineFromRepo(dir, () => {}, { skipGraphPhases: true }),
|
||||
new Promise<never>((_, reject) =>
|
||||
setTimeout(
|
||||
new Promise<never>((_, 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 });
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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<ScopeId, ReadonlyMap<string, readonly BindingRef[]>>();
|
||||
const workspaceFqnBindings = new Map<string, readonly BindingRef[]>();
|
||||
|
||||
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);
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -22,10 +22,12 @@ const mkRef = (nodeId: string): BindingRef =>
|
|||
const mkIndexes = (
|
||||
bindings: Map<ScopeId, Map<string, readonly BindingRef[]>>,
|
||||
augmentations: Map<ScopeId, Map<string, BindingRef[]>>,
|
||||
workspace: Map<string, readonly BindingRef[]> = 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<string, readonly BindingRef[]>([
|
||||
['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<ScopeId, Map<string, readonly BindingRef[]>>([
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue