fix(vba-extractor): scope Const declarations - proc-local no longer leaks to module scope (closes #52) - #73
Merged
Conversation
…eaks to module scope (closes #52) Bug: `sweepEnumsAndConsts` matched `CONST_DECL_RE` on every line of a `.bas`/`.cls` file with no awareness of the procedure stack. A `Const` declared inside a `Sub`/`Function` would emit a module-level `constant` node with `visibility: 'public'` and a `contains` edge from the module — wrong containment. Additionally `localConstants` was a single file-wide `Map<string, string>`, so two procs declaring the same Const name with different values collided (last write wins) and could mis-resolve `DoCmd.OpenForm` targets. Fix (option (b) from the issue spec — simpler and loses little): - Procedure-local Consts no longer emit a graph node; they keep their resolution value for OpenForm/OpenQuery argument lookup. - Module-level Consts are bit-for-bit unchanged (visibility fold, multi-declaration lines, module → constant `contains` edge). Changes: * `localConstants` re-typed from `Map<string, string>` to `Map<'module' | string, Map<string, string>>` — the inner key is the current proc's startLine (a string-coerced number) when inside a proc, or the literal `'module'` at file scope. The new helper `resolveLocalConst(name)` looks up the current-proc bucket first and falls back to `'module'`; `setLocalConstInScope` is the lazy-bucket writer. * `sweepEnumsAndConsts` and `sweepCallsAndSql` now share a single proc-stack discipline. Both maintain `currentProcKey` (`'module'` at file scope, `${startLine}` inside a proc) and a parallel `procStack: number[]` reset at sweep start. The const sweep consults `currentProcKey` per line: when inside a proc it populates the per-proc bucket via `setLocalConstInScope` and skips both the `constant` node emission and the `pushContainsFromModule` edge; when at module scope it emits as before. * `PROCEDURE_END_RE` (used to be a local const inside `sweepCallsAndSql`) promoted to a class-level static so both sweeps share one canonical regex. Pure refactor — no behavior change in `sweepCallsAndSql`. * `scanDoCmdOpenCalls` and `scanDoCmdOpenQuery` use the new `resolveLocalConst(name)` for argument resolution — per-proc bucket first, module fallback. The user-visible behavior of OpenForm/OpenReport/OpenQuery resolution is preserved for module- level consts and now correctly scoped for proc-local consts. * 6 regression atoms in `__tests__/extraction-vba.test.ts` (new describe block "Issue #52: procedure-local Const scoping"): 1. Module-level `Public Const FORM_EMPLOYEES` still emits a `constant` node + module contains edge (regression guard). 2. Procedure-local `Const FORM_DESTINO` inside a `Sub` emits ZERO `constant` nodes (the bug being fixed). 3. Procedure-local Const still resolves in `DoCmd.OpenForm` (resolution preserved). 4. Two procs with same-named local consts (`Sub A` / `Sub B`, each declaring `Const TARGET`) resolve their own `OpenForm` targets correctly — the proc-local shadowing works. 5. Mixed-scope consts — module-level + proc-local with the same name; module emits one `constant` node, proc-local resolves the shadowed value, only one node exists. 6. Multi-decl `Const FORM_EMPLOYEES = "...", FORM_ORDERS As String = "..."` at module scope still emits one node per name (regression guard). Validation: * `pnpm exec vitest run __tests__/extraction-vba.test.ts -t "Issue #52"` → 6 passed in 24 ms * Full VBA suite (6 files): **242 passed** in 1.4 s — zero regressions * `pnpm run build` → tsc clean, no TS errors Scope: this is a behavioral fix (no API change). The `constant` node kind, `vba-name-resolution` synthesizedBy tag, OpenForm dispatch, and existing module-level Const paths are unchanged. A Dysflow-managed project that previously emitted N module-level constant nodes for proc-local Const will now emit fewer — by exactly the number of proc-local Consts in its `.bas`/`.cls` files. Out of scope (intentional, per issue spec): * Option (a) — emitting a `constant` node with `visibility: 'private'` + `metadata.scope: 'local'` attached to the enclosing function via `contains`. Maintainer's call; option (b) is simpler and preserves all user-visible resolution. * Moving Const detection INTO `sweepCallsAndSql` — the line-range precompute approach (shared `currentProcKey` discipline) achieves the same scope awareness with a smaller code-surface delta. * Variable / parameter scoping — orthogonal issue.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #52.
What
Bug fix:
sweepEnumsAndConstswas matchingCONST_DECL_REon everyline of a
.bas/.clsfile with no awareness of the procedurestack. A
Constdeclared inside aSub/Functionwould emit amodule-level
constantnode withvisibility: 'public'and acontainsedge from the module — wrong containment.localConstantswas a single file-wide
Map<string, string>, so two procs declaringthe same Const name with different values collided (last write wins)
and could mis-resolve
DoCmd.OpenFormtargets.Why
In a real Dysflow-managed Access project, a
Const FORM_DESTINO As String = "FormDetalle"declared insidePublic Sub Abrir()wouldemit a module-level public constant node for
FORM_DESTINO— whichbreaks graph containment (the constant isn't actually module-scoped)
and pollutes the per-proc resolution cache. Two procs declaring the
same Const name with different values would collide in the cache and
produce wrong OpenForm edges.
Diff
src/extraction/vba-extractor.ts__tests__/extraction-vba.test.tsInside the 400-line review budget.
Design
Option (b) from the issue spec — simpler
Per the issue body's recommendation, option (b) was chosen: emit
NO graph node for procedure-local Const, but keep the resolution
value for
DoCmd.OpenForm/DoCmd.OpenReport/DoCmd.OpenQueryargument lookup. Module-level Const behavior is bit-for-bit
unchanged.
Implementation
localConstantsre-typed fromMap<string, string>toMap<'module' | string, Map<string, string>>. Inner key is thecurrent proc's startLine (a string-coerced number) when inside a
proc, or the literal
'module'at file scope. New helperresolveLocalConst(name)looks up the current-proc bucket firstand falls back to
'module';setLocalConstInScopeis thelazy-bucket writer.
sweepEnumsAndConstsandsweepCallsAndSqlnow share a singleproc-stack discipline via two parallel fields on the extractor:
currentProcKey('module'at file scope,${startLine}inside aproc) and
procStack: number[]. Both resets at sweep start.sweepEnumsAndConstsconsultscurrentProcKeyper line: wheninside a proc it populates the per-proc bucket via
setLocalConstInScopeand skips both theconstantnode emissionand the
pushContainsFromModuleedge; at module scope it emitsexactly as before.
PROCEDURE_END_REpromoted from a local const insidesweepCallsAndSqlto a class-level static so both sweeps shareone canonical regex. Pure refactor.
scanDoCmdOpenCallsandscanDoCmdOpenQueryuse the newresolveLocalConst(name)for argument resolution — per-procbucket first, module fallback.
Test coverage
6 atoms in
__tests__/extraction-vba.test.ts(new describe block"Issue #52: procedure-local Const scoping"):
Public Const FORM_EMPLOYEESstill emits aconstantnode + module contains edge (regression guard).
Const FORM_DESTINOinside aSubemits ZEROconstantnodes (the bug being fixed).DoCmd.OpenForm(resolution preserved).
Sub A/Sub B, eachdeclaring
Const TARGET) resolve their ownOpenFormtargetscorrectly — the proc-local shadowing works.
name; module emits one
constantnode, proc-local resolves theshadowed value, only one node exists.
Const FORM_EMPLOYEES = "...", FORM_ORDERS As String = "..."at module scope still emits one node per name(regression guard).
Validation
pnpm exec vitest run __tests__/extraction-vba.test.ts -t "Issue #52"→ 6 passed in 24 ms
pnpm run build→ tsc clean, no TS errorsOut of scope (intentional, per issue spec)
constantnode withvisibility: 'private'+metadata.scope: 'local'attached to the enclosingfunction via
contains. Maintainer's call; option (b) is simplerand preserves all user-visible resolution.
sweepCallsAndSql— the line-rangeprecompute approach (shared
currentProcKeydiscipline) achievesthe same scope awareness with a smaller code-surface delta.