fix(vba): stop reading On Error GoTo as a reference to a variable named Error - #293
Merged
Conversation
…ed Error `scanModuleVariableReferences` walks every identifier on a line and emits a reference for any that names a module-level variable. It guarded a `.` / `!` prefix and procedure-local shadowing (#205), but not VBA keyword context — so in a module declaring `Public Error As String`, the word `Error` in `On Error GoTo errores` was read as an access to that variable. `Public Error As String` is this codebase's error-channel convention and appears in dozens of classes, so this fired constantly. On its own it is a stray edge; #261 labels channel references with `errorChannel: true`, which would have turned every one of them into a confident claim that an `On Error` statement takes part in error propagation — the failure mode `CLAUDE.md` and guardrail 1 of `docs/vba-error-handling-plan.md` both name as the worst available here. #261 is held until this lands so it is measured on clean data. The `On Error` pair is blanked out before the identifier walk, replaced with spaces of the SAME length. That is load-bearing rather than incidental: the emitted reference carries `column: m.index`, so a substitution that shifted offsets would corrupt every column on the line. A fixture pins the column of a genuine reference sharing a line with `On Error GoTo`. Scoped to the `On Error` pair only. VBA spells the `Error` statement (`Error 5`) and the `Error$()` function with the same word; both have zero occurrences in this corpus, and telling those from an identically-named variable is a parser problem rather than a masking one. They are left for a corpus that contains them. Measured on the corpus (`00_EXPEDIENTES`, `00_GESTION_RIESGOS`, `HPS_SOLICITUDES`): unresolved references fall 26,755 -> 25,211. The entire delta is `property-get`, 5,636 -> 4,092 — 1,544 false reads removed, and no other reference kind, node kind or edge kind moves. Nodes stay at 30,000 and edges at 37,456. The issue estimated ~909; the measured figure is 1,544. The estimate counted handler bodies, while the sweep de-duplicates per (procedure, variable, direction) — so every procedure whose ONLY apparent read of `Error` came from its own `On Error` line contributed one, including procedures the estimate did not look at. Closes #292 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gmKKUq1ng5ESk6Qhxu77d
ardelperal
added a commit
that referenced
this pull request
Sep 3, 2026
* feat(vba): recognise the module-variable error channel Error propagation in an Access codebase of this shape does not use VBA's error mechanism. 16 handlers out of 3,774 re-raise; `Err.Raise 1000` unwinds exactly one frame and the house guard `If Err.Number <> 1000` means "an inner procedure already wrote a human-readable message". The message itself travels through a field the failing procedure writes and the caller reads. That is module-variable data flow, which #251 already models as `property-set` / `property-get` references onto a `variable` node. This change only labels it: a read or write of a channel variable now carries `metadata.errorChannel: true`, on the reference and on the resolved edge. No new node kind, no new edge kind, and no new row — the corpus indexes to byte-identical `nodesByKind` / `edgesByKind` / `unresolvedByKind` totals (26,089 / 29,521 / 26,755, unchanged against origin/main). Decisions taken: - The channel names and the write matcher move into a new leaf module, `src/extraction/vba/error-channel.ts`. `errors.ts` owned both before, and its own comment deferred the config knob to this task; leaving the list there and importing it from `module-vars.ts` would have forked two matchers the moment the knob became config-aware. Both consumers now read one compiled object, so `vba.errorChannel` drives the reference flag AND `errorPolicy.behavior` rather than only the former. - `vba.errorChannel` takes bare VBA identifiers, matched as whole names, and EXTENDS the built-in list — the same contract `vba.sqlWrappers` established in #244. No user-supplied regex: this runs per identifier per line, which is exactly where one is a backtracking hazard. Matching a name rather than a substring is what keeps `ErrorCount` out. - The compiled form (a `Set` plus RegExps) lives on the extractor context, not in `VbaExtractionOptions`, because the options object crosses the `structuredClone` worker boundary. - The flag is only ever `true`; its absence encodes "not the channel", so it is added to a minority of rows instead of a `false` to every one — the shape #260 chose for `inErrorHandler`. Closes #261 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gmKKUq1ng5ESk6Qhxu77d * feat(vba): emit label nodes and handles-error edges (#290) * feat(vba): emit label nodes and handles-error edges Task E6 of `docs/vba-error-handling-plan.md`. #259 records *whether* a procedure has an error handler and #260 marks *which* edges come from inside one, but neither gives the handler an identity you can point at, search for or traverse to. This does: one `label` node per VBA line label, and a `handles-error` edge from the procedure to the handler it routes to. The plan's §4 rejected this design for #259 and §4.3 named the condition that reopens it — a query `inErrorHandler`'s per-procedure boolean cannot serve. Addressing a handler as a thing is that query: a stable id per handler, `kind:label` search, and dangling/duplicate/control-flow-label detection as a graph query rather than a scan. This adds no parsing. Every fact published here was already computed by the error-policy classifier while the procedure body was open — the label definitions, the `On Error GoTo` targets, the handler region and the dangling-target resolution. `handlerBehavior` is #260's derived `errorPolicy.behavior`, copied verbatim. The one genuinely new signal is the plain-`GoTo` jump, which the policy classifier had no reason to look at while it emitted nothing, and which arrives as a fifth rule on the same declarative table rather than as a second scanner. Decisions taken: - `qualifiedName` is always `<ModuleOrClass>.<Procedure>.<label>`. VBA scopes labels to the procedure and this corpus writes `errores` 3,735 times; without the procedure segment every handler in a project collapses into one symbol. Same shape #257's parameters and #251's module variables chose. - `handles-error` is not deduplicated per procedure. 47 procedures issue more than one `On Error GoTo`, and each is a distinct routing decision with its own line, so each emits its own edge. - A plain `GoTo` reuses the generic `references` kind tagged `vba-goto`. A jump is not an error-handling fact and 192 sites do not justify a second kind; the synthesizer tag keeps them filterable. - A `GoTo` whose target the procedure never defines emits an `UnresolvedReference` and **no node**. A graph that invents its own targets cannot be used to find that defect, which is the only reason to look for it. - Calls inside a handler stay attributed to the enclosing procedure. The label is addressable, not a container; re-parenting would change `callers`/`callees` for the 3,774 procedures that have a handler. - Only the label whose region `errorPolicy` actually resolved carries `handlerBehavior` and the region lines. A procedure that swaps to a second handler label has a second region nobody classified, and deriving one here would be exactly the drift this split avoids. - A numeric `GoTo` target is a VBA line number, not a line label. The label detector cannot define one, so referencing it would fabricate a permanent dangling reference for legal code. - `label` stays out of `HIGH_VALUE_NODE_KINDS` and `CONTAINER_NODE_KINDS`, for the reason #257 kept `parameter` out of both: it is now the most numerous VBA symbol in the graph. Measured on the three-project Access corpus with the committed probe: 3,911 label nodes, 3,832 `handles-error` edges, 192 `vba-goto` references, zero new unresolved references, and no other node or edge kind moved. That is +15.0% nodes and +26.9% edges — the extractor matches the probe's census exactly on all three counts. `EXTRACTION_VERSION` is bumped to 26: a new node kind and a new edge kind change what a re-index would produce. Closes #263 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gmKKUq1ng5ESk6Qhxu77d * docs(vba): correct the E6 landing-order claim in the plan The E6 section said it landed "after E1-E5 were built". E4 (#261) is still in flight and E5 (#262) has not started, so that sentence asserted an order that did not happen. Corrected to what is true: E6 landed after E1-E3, alongside E4, and before E5. The rest of the section — the three §4.3 conditions, the measured budget, and the warning not to read it as licence to add a kind elsewhere — is unchanged and accurate. Refs #263 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gmKKUq1ng5ESk6Qhxu77d --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * fix(vba): stop reading On Error GoTo as a reference to a variable named Error (#293) `scanModuleVariableReferences` walks every identifier on a line and emits a reference for any that names a module-level variable. It guarded a `.` / `!` prefix and procedure-local shadowing (#205), but not VBA keyword context — so in a module declaring `Public Error As String`, the word `Error` in `On Error GoTo errores` was read as an access to that variable. `Public Error As String` is this codebase's error-channel convention and appears in dozens of classes, so this fired constantly. On its own it is a stray edge; #261 labels channel references with `errorChannel: true`, which would have turned every one of them into a confident claim that an `On Error` statement takes part in error propagation — the failure mode `CLAUDE.md` and guardrail 1 of `docs/vba-error-handling-plan.md` both name as the worst available here. #261 is held until this lands so it is measured on clean data. The `On Error` pair is blanked out before the identifier walk, replaced with spaces of the SAME length. That is load-bearing rather than incidental: the emitted reference carries `column: m.index`, so a substitution that shifted offsets would corrupt every column on the line. A fixture pins the column of a genuine reference sharing a line with `On Error GoTo`. Scoped to the `On Error` pair only. VBA spells the `Error` statement (`Error 5`) and the `Error$()` function with the same word; both have zero occurrences in this corpus, and telling those from an identically-named variable is a parser problem rather than a masking one. They are left for a corpus that contains them. Measured on the corpus (`00_EXPEDIENTES`, `00_GESTION_RIESGOS`, `HPS_SOLICITUDES`): unresolved references fall 26,755 -> 25,211. The entire delta is `property-get`, 5,636 -> 4,092 — 1,544 false reads removed, and no other reference kind, node kind or edge kind moves. Nodes stay at 30,000 and edges at 37,456. The issue estimated ~909; the measured figure is 1,544. The estimate counted handler bodies, while the sweep de-duplicates per (procedure, variable, direction) — so every procedure whose ONLY apparent read of `Error` came from its own `On Error` line contributed one, including procedures the estimate did not look at. Closes #292 Claude-Session: https://claude.ai/code/session_019gmKKUq1ng5ESk6Qhxu77d Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * feat(vba): recognise the module-variable error channel Error propagation in an Access codebase of this shape does not use VBA's error mechanism. 16 handlers out of 3,774 re-raise; `Err.Raise 1000` unwinds exactly one frame and the house guard `If Err.Number <> 1000` means "an inner procedure already wrote a human-readable message". The message itself travels through a field the failing procedure writes and the caller reads. That is module-variable data flow, which #251 already models as `property-set` / `property-get` references onto a `variable` node. This change only labels it: a read or write of a channel variable now carries `metadata.errorChannel: true`, on the reference and on the resolved edge. No new node kind, no new edge kind, and no new row — the corpus indexes to byte-identical `nodesByKind` / `edgesByKind` / `unresolvedByKind` totals (26,089 / 29,521 / 26,755, unchanged against origin/main). Decisions taken: - The channel names and the write matcher move into a new leaf module, `src/extraction/vba/error-channel.ts`. `errors.ts` owned both before, and its own comment deferred the config knob to this task; leaving the list there and importing it from `module-vars.ts` would have forked two matchers the moment the knob became config-aware. Both consumers now read one compiled object, so `vba.errorChannel` drives the reference flag AND `errorPolicy.behavior` rather than only the former. - `vba.errorChannel` takes bare VBA identifiers, matched as whole names, and EXTENDS the built-in list — the same contract `vba.sqlWrappers` established in #244. No user-supplied regex: this runs per identifier per line, which is exactly where one is a backtracking hazard. Matching a name rather than a substring is what keeps `ErrorCount` out. - The compiled form (a `Set` plus RegExps) lives on the extractor context, not in `VbaExtractionOptions`, because the options object crosses the `structuredClone` worker boundary. - The flag is only ever `true`; its absence encodes "not the channel", so it is added to a minority of rows instead of a `false` to every one — the shape #260 chose for `inErrorHandler`. Closes #261 --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
scanModuleVariableReferenceswalks every identifier on a line and emits a reference for any that names a module-level variable. It guarded a./!prefix and procedure-local shadowing (#205), but not VBA keyword context — so in a module declaringPublic Error As String, the wordErrorinOn Error GoTo erroreswas read as an access to that variable.Public Error As Stringis this codebase's error-channel convention and appears in dozens of classes, so this fired constantly.Why now
On its own it is a stray edge. #261 (task E4) labels channel references with
errorChannel: true, andErroris in the default channel list — which would have turned every one of these into a confident claim that anOn Errorstatement takes part in error propagation. That is the failure modeCLAUDE.mdand guardrail 1 ofdocs/vba-error-handling-plan.mdboth name as the worst available here. #261 is held until this lands so it is measured on clean data.The fix
Blank the
On Errorkeyword pair out of the line before the identifier walk, replacing it with spaces of the same length.The same-length part is load-bearing, not incidental: the emitted reference carries
column: m.index, so a substitution that shifted offsets would corrupt every column on the line. There is a fixture that pins the column of a genuine reference sharing a line withOn Error GoTo(On Error GoTo errores: Error = "x"→ column 25, the real offset).Scope, deliberately narrow
Only the
On Errorpair. VBA spells theErrorstatement (Error 5) and theError$()function with the same word; both have zero occurrences in this corpus, and telling those from an identically-named variable is a parser problem rather than a masking one. Left for a corpus that contains them.The alternative fix — dropping
Errorfrom #261's default channel list — was rejected: it kills genuine channel matches to suppress false ones, and leaves the underlying edge defect in place for every other consumer.Corpus measurement
Probe
--jsonover00_EXPEDIENTES,00_GESTION_RIESGOSandHPS_SOLICITUDES, this branch versusorigin/main:property-get1,544 false reads removed. The entire delta is
property-get; no other reference kind, node kind or edge kind moves — which is the shape you want, since this removes references and never nodes.The measured figure is larger than the issue's estimate, and that is expected
The issue estimated ~909. The measurement says 1,544. The estimate counted handler bodies; the sweep de-duplicates per
(procedure, variable, direction), so every procedure whose only apparent read ofErrorcame from its ownOn Errorline contributed one — including procedures the estimate never looked at. Reporting the measured number, not the forecast.Verification
npx tsc --noEmit— clean.__tests__/extraction-vba-on-error-keyword.test.ts— 10/10.One test pins a behaviour it does not fix: on
On Error GoTo errores: Error = "x"the write is reported asproperty-get, because after a colon the prefixisDirectAssignmentsees is not blank. That is a pre-existing limitation of the shared predicate, unrelated to this mask; asserted so it is pinned rather than silently depended on.Not verified
The full local suite was not run to completion —
npx vitest runon this machine dies withFatal process out of memory, an environment limit unrelated to this change. CI runs the full suite on Ubuntu and Windows.Closes #292
🤖 Generated with Claude Code
https://claude.ai/code/session_019gmKKUq1ng5ESk6Qhxu77d