diff --git a/__tests__/extraction-vba.test.ts b/__tests__/extraction-vba.test.ts index d917d094..aaf0afd9 100644 --- a/__tests__/extraction-vba.test.ts +++ b/__tests__/extraction-vba.test.ts @@ -3496,3 +3496,150 @@ describe('VbaExtractor — With block implicit receiver calls (Issue #43)', () = }); }); +// --------------------------------------------------------------------------- +// Issue #52 — procedure-local Const must NOT leak to module scope (was: +// one module-level `constant` node per name with module→constant contains +// edge and `visibility: 'public'`, plus a single file-wide `localConstants` +// Map that caused two procs declaring the same Const name to collide on the +// last write for `DoCmd.OpenForm`/`OpenReport`/`OpenQuery` argument +// resolution). Fix: per-proc resolution bucket + a proc-stack-aware +// `currentProcKey` field. Module-level Const behavior is preserved bit-for- +// bit (regression-pinned below) — only the proc-local shape changes. +// +// Reference: the dedicated Enum/Const tests live in +// `__tests__/extraction-vba-enums-consts.test.ts` (REQ-CODE-12 / REQ-CODE-13 +// atoms). The atoms in this block pin the Issue #52 fix specifically. +// --------------------------------------------------------------------------- + +describe('VbaExtractor — Issue #52: procedure-local Const scoping', () => { + it('module-level Const still emits a constant node + module contains edge (regression)', () => { + const src = [ + 'Option Explicit', + 'Public Const FORM_EMPLOYEES As String = "frmEmployees"', + 'Public Sub DoSomething()', + 'End Sub', + ].join('\n'); + const r = extract('src/modules/modForms.bas', src); + + const c = r.nodes.find( + (n) => n.kind === 'constant' && n.name === 'FORM_EMPLOYEES', + ); + // Pre-fix this was the buggy behavior and post-fix must still hold + // because the Const lives at module scope (no enclosing proc). + expect(c).toBeDefined(); + expect(c?.visibility).toBe('public'); + const mod = r.nodes.find((n) => n.kind === 'module'); + expect(mod).toBeDefined(); + const containsEdge = r.edges.find( + (e) => + e.kind === 'contains' && e.source === mod?.id && e.target === c?.id, + ); + expect(containsEdge).toBeDefined(); + }); + + it('procedure-local Const emits NO constant node anywhere (the bug)', () => { + const src = [ + 'Sub Abrir()', + ' Const FORM_DESTINO As String = "FormDetalle"', + ' DoCmd.OpenForm FORM_DESTINO', + 'End Sub', + ].join('\n'); + const r = extract('src/modules/modForms.bas', src); + + // The pre-fix extractor emitted a `constant` node named FORM_DESTINO + // with `visibility: 'public'` and a module→constant contains edge — + // wrong containment for a local. Post-fix: zero `constant` nodes. + const consts = r.nodes.filter((n) => n.kind === 'constant'); + expect(consts).toEqual([]); + }); + + it('procedure-local Const still resolves in DoCmd.OpenForm (resolution preserved)', () => { + const src = [ + 'Sub Abrir()', + ' Const FORM_DESTINO As String = "FormDetalle"', + ' DoCmd.OpenForm FORM_DESTINO', + 'End Sub', + ].join('\n'); + const r = extract('src/modules/modForms.bas', src); + + const edges = r.edges.filter((e) => e.kind === 'opens-form'); + expect(edges).toHaveLength(1); + const target = r.nodes.find((n) => n.id === edges[0]?.target); + expect(target?.name).toBe('FormDetalle'); + expect(edges[0]?.metadata?.targetFormName).toBe('FormDetalle'); + }); + + it('two procs with same-named local consts resolve their own OpenForm targets', () => { + const src = [ + 'Sub A()', + ' Const TARGET As String = "FormA"', + ' DoCmd.OpenForm TARGET', + 'End Sub', + 'Sub B()', + ' Const TARGET As String = "FormB"', + ' DoCmd.OpenForm TARGET', + 'End Sub', + ].join('\n'); + const r = extract('src/modules/modForms.bas', src); + + // Pre-fix the file-wide `localConstants` Map made the second write + // (`FormB`) overwrite the first, so both OpenForm call sites + // resolved to `FormB`. Post-fix: each proc keeps its own bucket. + const edges = r.edges.filter((e) => e.kind === 'opens-form'); + expect(edges).toHaveLength(2); + const targets = edges + .map((e) => e.metadata?.targetFormName) + .sort(); + expect(targets).toEqual(['FormA', 'FormB']); + + // No `constant` nodes for TARGET at all — proc-local consts don't + // emit module-level symbol nodes anymore. + const consts = r.nodes.filter( + (n) => n.kind === 'constant' && n.name === 'TARGET', + ); + expect(consts).toEqual([]); + }); + + it('mixed-scope consts: module-level emits one node; proc-local shadows it for the inner call', () => { + const src = [ + 'Public Const SHARED_NAME As String = "ModuleShared"', + 'Sub Outer()', + ' Const SHARED_NAME As String = "ProcShared"', + ' DoCmd.OpenForm SHARED_NAME', + 'End Sub', + ].join('\n'); + const r = extract('src/modules/modForms.bas', src); + + // Module-level Const still emits one node with the visibility fold. + const moduleConst = r.nodes.find( + (n) => n.kind === 'constant' && n.name === 'SHARED_NAME', + ); + expect(moduleConst).toBeDefined(); + expect(moduleConst?.visibility).toBe('public'); + + // The OpenForm call inside Outer() sees the proc-local binding + // (shadowing the module-level one) — exactly one opens-form edge, + // resolved to the proc-local value. + const edges = r.edges.filter((e) => e.kind === 'opens-form'); + expect(edges).toHaveLength(1); + expect(edges[0]?.metadata?.targetFormName).toBe('ProcShared'); + + // Proc-local name does NOT get a second `constant` node. + expect( + r.nodes.filter((n) => n.kind === 'constant' && n.name === 'SHARED_NAME'), + ).toHaveLength(1); + }); + + it('multi-decl Const line at module scope still emits one constant node per name (regression)', () => { + const r = extract( + 'src/modules/c.bas', + 'Const FORM_EMPLOYEES = "frmEmployees", FORM_ORDERS As String = "frmOrders"', + ); + const names = r.nodes + .filter((n) => n.kind === 'constant') + .map((n) => n.name) + .sort(); + expect(names).toEqual(['FORM_EMPLOYEES', 'FORM_ORDERS']); + }); +}); + diff --git a/src/extraction/vba-extractor.ts b/src/extraction/vba-extractor.ts index b122c121..3cfce5b2 100644 --- a/src/extraction/vba-extractor.ts +++ b/src/extraction/vba-extractor.ts @@ -965,6 +965,17 @@ export class VbaExtractor { private static readonly CONST_DECL_RE = /^\s*(?:(Public|Private|Friend|Global)\s+)?Const\s+(.+)$/i; + /** + * Issue #52: shared `End Sub` / `End Function` / `End Property` marker. + * Promoted from a local regex in `sweepCallsAndSql` so `sweepEnumsAndConsts` + * can walk the same proc boundaries and decide Const scope per line. + * The `(?:^|:\s*)` prefix tolerates colon-separated single-line procs + * (`Public Sub X(): ... : End Sub`) so the proc stack pops on the same + * physical line. + */ + private static readonly PROCEDURE_END_RE = + /(?:^|:\s*)End\s+(?:Sub|Function|Property)\b/i; + /** * Fold a VBA visibility keyword to the canonical lowercase enum, matching * the procedure convention: `Private` → 'private'; `Public`, `Global`, @@ -993,10 +1004,39 @@ export class VbaExtractor { let count = 0; let currentEnum: { id: string; name: string } | null = null; + // Issue #52: reset the shared scope stack + lookup key so leftover + // state from a previous extract() (impossible in production but + // possible in unit tests that construct a fresh extractor and run + // twice) never leaks across sweeps. The walk below updates both + // every iteration; `sweepCallsAndSql` resets again at its own + // start, before any OpenForm/OpenQuery reader consults them. + this.procStack.length = 0; + this.currentProcKey = 'module'; + for (let i = 0; i < lines.length; i++) { const line = lines[i] ?? ''; const lineNum = i + 1; + // Issue #52: track proc scope so Const declarations on this line + // can decide whether they belong to the module (currentProcKey + // === 'module') or to the top-most procedure (write the per-proc + // bucket, skip the module-level `constant` node emission). + // + // PROC_RE cannot overlap with CONST_DECL_RE on the same physical + // line (different leading keywords), so it is safe to advance the + // stack here and then fall through to the rest of the body. + const procStart = VbaExtractor.PROC_RE.exec(line); + if (procStart) { + this.procStack.push(lineNum); + this.currentProcKey = String(lineNum); + } else if (VbaExtractor.PROCEDURE_END_RE.test(line) && this.procStack.length > 0) { + this.procStack.pop(); + this.currentProcKey = + this.procStack.length > 0 + ? String(this.procStack[this.procStack.length - 1]) + : 'module'; + } + if (currentEnum) { if (VbaExtractor.ENUM_END_RE.test(line)) { currentEnum = null; @@ -1070,9 +1110,19 @@ export class VbaExtractor { for (const declaration of declarations) { const constName = declaration.name; if (!constName) continue; + // Issue #52: every Const line (module-level or proc-local) + // writes into a per-scope resolution bucket so + // `DoCmd.OpenForm FORM_X` later resolves correctly; module-level + // Consts additionally emit a `constant` graph node + the + // module→constant `contains` edge. Proc-local Consts skip both + // (the const is not a module symbol, so the wrong-containment + // node + edge the pre-fix code emitted are gone), but the + // per-proc bucket keeps OpenForm/OpenQuery argument + // resolution working exactly as before. if (declaration.value !== null) { - this.localConstants.set(constName.toLowerCase(), declaration.value); + this.setLocalConstInScope(this.currentProcKey, constName, declaration.value); } + if (this.procStack.length > 0) continue; const constId = generateNodeId( this.filePath, 'constant', @@ -1659,14 +1709,17 @@ export class VbaExtractor { private sweepCallsAndSql(src: string): void { const lines = src.split('\n'); const procedureStartLines = new Set(); - // S5 fix: also match `End Sub`/`End Function`/`End Property` after a - // colon (`:`), so single-line `Public Sub X(): End Sub` is recognized - // as ending the procedure. The previous `/^\s*End...` only matched at - // line start, so the proc stack never popped for colon-separated - // single-line declarations. - const procedureEndRe = /(?:^|:\s*)End\s+(?:Sub|Function|Property)\b/i; const sqlTargetsThisFile = new Set(); + // Issue #52: reset the shared scope state before the per-line walk + // begins. `sweepEnumsAndConsts` already populated `procStack` / + // `currentProcKey` during its own walk; clearing here guarantees the + // `scanDoCmdOpenCalls` / `scanDoCmdOpenQuery` reads (which consult + // `currentProcKey` per call-site) start in module scope and follow + // the same push/pop discipline as the existing `stack` array below. + this.procStack.length = 0; + this.currentProcKey = 'module'; + // Walk the source once, emitting call edges and SQL edges per line and // tracking the current procedure stack. The previous implementation // did this in two passes; audit S1 (June 2026) flagged the first pass @@ -1687,6 +1740,15 @@ export class VbaExtractor { const procStart = VbaExtractor.PROC_RE.exec(line); if (procStart) { + // Issue #52: mirror the proc push into the shared + // `procStack` + `currentProcKey` so Const reads in this same + // sweep see the same scope as the const writes did (during + // `sweepEnumsAndConsts`). Uses the same `startLine` key used + // for the `localConstants` bucket. + const procStartLine = lineNum; + this.procStack.push(procStartLine); + this.currentProcKey = String(procStartLine); + const name = procStart[3] ?? ''; const bucket = this.localProcs.get(name); if (bucket) { @@ -1699,8 +1761,14 @@ export class VbaExtractor { if (proc) stack.push(proc); } procedureStartLines.add(lineNum); - } else if (procedureEndRe.test(line) && stack.length > 0) { + } else if (VbaExtractor.PROCEDURE_END_RE.test(line) && stack.length > 0) { const ending = stack.pop()!; + // Issue #52: mirror the pop into the shared scope state. + this.procStack.pop(); + this.currentProcKey = + this.procStack.length > 0 + ? String(this.procStack[this.procStack.length - 1]) + : 'module'; procEndLines.set(ending.startLine, lineNum); continue; } @@ -2486,9 +2554,13 @@ export class VbaExtractor { let m: RegExpExecArray | null; while ((m = localRe.exec(line)) !== null) { const rawArg = (m[1] ?? '').trim(); + // Issue #52: const lookup is now per-proc-bucket with module + // fallback (see `resolveLocalConst`). Two procs declaring the + // same Const name with different values no longer collide — + // each call site uses the value visible at its own scope. const targetName = rawArg.startsWith('"') ? unwrapVbaStringLiteral(rawArg) - : (this.localConstants.get(rawArg.toLowerCase()) ?? rawArg); + : (this.resolveLocalConst(rawArg) ?? rawArg); if (!targetName) continue; this.emitOpensStubEdge( dispatch, @@ -2628,9 +2700,15 @@ export class VbaExtractor { let m: RegExpExecArray | null; while ((m = localRe.exec(line)) !== null) { const rawArg = (m[1] ?? '').trim(); + // Issue #52: same per-proc-with-module-fallback lookup as + // `scanDoCmdOpenCalls` — proc-local consts resolve to their own + // values, so a `DoCmd.OpenQuery LOCAL_QUERY` inside `Sub X()` + // points at the correct query even when another proc declares a + // different `LOCAL_QUERY` (pre-fix this was whichever wrote + // the file-wide map last). const targetName = rawArg.startsWith('"') ? unwrapVbaStringLiteral(rawArg) - : (this.localConstants.get(rawArg.toLowerCase()) ?? rawArg); + : (this.resolveLocalConst(rawArg) ?? rawArg); if (!targetName) continue; this.unresolvedReferences.push({ fromNodeId: this.findOrCreateFunctionNodeId(caller), @@ -2874,8 +2952,78 @@ export class VbaExtractor { assignedWithSet?: boolean; }>(); - /** Local constant name (lowercase) → simple literal value for OpenForm resolution. */ - private localConstants = new Map(); + /** + * Issue #52: Const resolution buckets, scoped per procedure. Key is + * `'module'` for module-level Consts, or the procedure's `startLine` + * (stringified) for proc-local Consts. Each bucket maps the lowercase + * constant name to its simple-literal value (used by `DoCmd.OpenForm` / + * `OpenReport` / `OpenQuery` argument resolution via `resolveLocalConst`). + * + * Two procs declaring the same Const name with different values stay + * isolated (each in its own bucket); reads look up the current proc's + * bucket first and fall back to the module bucket. The bucket-per-proc + * model was chosen over a single file-wide Map (the pre-fix shape) so + * `DoCmd.OpenForm FORM_DESTINO` resolves to the proc-local value, not + * whichever was written last. + */ + private localConstants: Map<'module' | string, Map> = new Map(); + + /** + * Issue #52: shared lookup helper for `scanDoCmdOpenCalls` and + * `scanDoCmdOpenQuery`. The current scope is the procedure whose + * `startLine` is on top of `procStack` (or `'module'` when the stack is + * empty). Per-proc bucket first; module bucket is the fallback. + */ + private resolveLocalConst(name: string): string | undefined { + const lower = name.toLowerCase(); + const procBucket = this.localConstants.get(this.currentProcKey); + if (procBucket) { + const v = procBucket.get(lower); + if (v !== undefined) return v; + } + const moduleBucket = this.localConstants.get('module'); + return moduleBucket?.get(lower); + } + + /** + * Issue #52: shared writer. `scopeKey` is `'module'` or the procedure's + * startLine-as-string. Creates the bucket lazily so callers do not have + * to pre-allocate per proc. Returns the bucket the value was written to + * (mostly useful for tests; production code ignores it). + */ + private setLocalConstInScope( + scopeKey: 'module' | string, + name: string, + value: string, + ): Map { + let bucket = this.localConstants.get(scopeKey); + if (!bucket) { + bucket = new Map(); + this.localConstants.set(scopeKey, bucket); + } + bucket.set(name.toLowerCase(), value); + return bucket; + } + + /** + * Issue #52: the current Const-lookup scope. `'module'` when no procedure + * is open, otherwise the top-of-stack proc's `startLine` as a string. + * Both `sweepEnumsAndConsts` (to decide whether to emit a `constant` + * node) and `sweepCallsAndSql` (to drive OpenForm/OpenQuery resolution) + * keep this in sync with their per-line stack walk by pushing/popping + * `procStack` and writing the new top's key here. + */ + private currentProcKey: 'module' | string = 'module'; + + /** + * Issue #52: per-extraction proc-stack shared between `sweepEnumsAndConsts` + * and `sweepCallsAndSql`. Each sweep clears it at the start so the file's + * mid-proc structural state never leaks across sweeps. Holds the + * `startLine` (1-based, matches `ProcInfo.startLine`) of every procedure + * whose body the sweep has not yet emitted `End Sub`/`End Function`/ + * `End Property` for. + */ + private procStack: number[] = []; /** Local event name (lowercase) → event node for `RaiseEvent` edge emission. */ private localEvents = new Map();