diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e2e436..2b39f11 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,8 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - Procedures now record how they handle errors — whether they have a handler, silently suppress errors, or have no protection at all — so unguarded code paths can be found without reading every module. (#259) - An Access project that ships a generated structure export of its backend now gets real tables in the graph, with their columns, and linked tables point at the external database file they actually live in — so a table found from a query is the same table that carries its fields. (#257) - Work a procedure does only when something goes wrong is now marked as such, and each error handler records whether it records the message, shows it to the user, re-raises it, or does more than one of those — so a procedure's failure path can be told apart from its normal one. (#260) +- Each error handler in Access code is now its own symbol you can search for and jump to, linked to the procedure that routes errors to it, so a handler can be found and followed directly instead of only asking whether a procedure has one. (#263) +- The variables this kind of Access project uses to pass error messages between objects are now recognised as such, so an error can be followed from where it happened to where the user sees it. (#261) ### Changed @@ -38,6 +40,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Fixes +- A VBA module that declares its own variable named `Error` no longer reports every `On Error GoTo` line as a read of it, so error-handling statements stop showing up as data access. (#292) - VBA: a call written with the `Call` keyword, or with an argument list, is now reported as a call rather than as an ambiguous bare-identifier read, so a genuinely missing procedure is no longer filtered out by the constant-lookup rules meant for plain identifier reads. A bare name with no `Call` keyword and no arguments still counts as an identifier read, because it really can be a constant. (#265) - A form or report whose file was saved under a different name than the module itself carries now gets its event handlers and control references wired up, instead of quietly coming through with none of them. (#249) - VBA projects that reach their database through a named accessor — one per backend, for example — no longer lose the tables those queries touch, and recordsets opened with parentheses are now tracked like every other query. Projects whose accessors follow their own naming can list them in the project configuration. (#244) diff --git a/__tests__/extraction-vba-error-channel.test.ts b/__tests__/extraction-vba-error-channel.test.ts new file mode 100644 index 0000000..722c0c3 --- /dev/null +++ b/__tests__/extraction-vba-error-channel.test.ts @@ -0,0 +1,520 @@ +/** + * Issue #261 (task E4 of `docs/vba-error-handling-plan.md`) — the module + * variable this codebase actually propagates error messages through is + * recognised as such. + * + * §2.1/§2.3 of the plan is the whole justification: **16 handlers out of + * 3,774 re-raise**. `Err.Raise 1000` unwinds 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 — + * `m_Error`, `p_Error`, the public `Error` — which issue #251 already models + * as `property-set` / `property-get` references onto a `variable` node. This + * task adds exactly one thing to that: `metadata.errorChannel: true`, so the + * hops that carry an error are distinguishable from the hops that carry + * ordinary module state. + * + * Two invariants outrank every assertion below, and both are pinned here: + * + * 1. **No new node kind, no new edge kind, and not one extra row.** The flag + * is a field on rows #251 already emitted. `adds no rows` pins it. + * 2. **The pattern is a NAME, never a substring.** `ErrorCount` must not + * match `Error` — a false positive here does not merely add noise, it + * claims that ordinary state is error propagation. + * + * The last block is the end-to-end one: the chain + * `inner writes -> outer reads -> outer displays` must be traversable in a + * real index, not merely present as two unrelated references. Its fixture is + * modelled on `src/forms/Form_FormGestionRiesgos.cls` of the reference corpus, + * where `EstablecerDatos` writes `m_Error` and `Form_Load` reads it and shows + * it in a `MsgBox`. + */ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import * as os from 'node:os'; +import { VbaExtractor } from '../src/extraction/vba-extractor'; +import { + DEFAULT_ERROR_CHANNEL_NAMES, + compileErrorChannel, + isErrorChannelName, +} from '../src/extraction/vba/error-channel'; +import { clearProjectConfigCache, loadVbaConfig } from '../src/project-config'; +import CodeGraph from '../src/index'; +import type { Node, UnresolvedReference } from '../src/types'; +import type { VbaExtractionOptions } from '../src/extraction/vba/options'; + +const CLASS_PATH = 'src/classes/Riesgo.cls'; + +/** + * Extract a class module. The two header lines are prepended here so a + * fixture's own first line is source line 3 — the convention + * `extraction-vba-error-handler-region.test.ts` uses. + */ +function extract(body: string[], options?: VbaExtractionOptions) { + return new VbaExtractor( + CLASS_PATH, + ['Attribute VB_Name = "Riesgo"', 'Option Explicit', ...body].join('\n'), + options, + ).extract(); +} + +function moduleVarRefs( + result: ReturnType, + name?: string, +): UnresolvedReference[] { + return result.unresolvedReferences.filter( + (r) => + r.metadata?.synthesizedBy === 'vba-module-var' && + (name === undefined || r.referenceName === name), + ); +} + +/** The one reference this procedure makes to `name` in the given direction. */ +function refOf( + result: ReturnType, + name: string, + access: 'read' | 'write', +): UnresolvedReference | undefined { + return moduleVarRefs(result, name).find((r) => r.metadata?.access === access); +} + +/** Declared procedures only — call-target stubs carry `metadata.stub`. */ +function procedure(nodes: Node[], name: string): Node | undefined { + return nodes.find( + (n) => n.kind === 'function' && n.metadata?.stub !== true && n.name === name, + ); +} + +// ============================================================================ +// 1. The two hops of the chain, on the corpus's own shapes +// ============================================================================ + +describe('Issue #261 — the error channel is flagged on reads and writes', () => { + it('flags `m_Error = "…"` written inside a handler as a channel write, inside the handler', () => { + const result = extract([ + 'Private m_Error As String', + '', + 'Public Sub Guardar()', + ' On Error GoTo errores', + ' Escribir', + ' Exit Sub', + 'errores:', + ' m_Error = "Riesgo.Guardar ha fallado: " & Err.Description', + 'End Sub', + ]); + + const write = refOf(result, 'm_Error', 'write'); + expect(write).toBeDefined(); + expect(write!.referenceKind).toBe('property-set'); + expect(write!.metadata).toMatchObject({ + synthesizedBy: 'vba-module-var', + access: 'write', + errorChannel: true, + // Issue #260's flag, which this line earns because it is the ONLY write + // in the procedure and therefore not deduped away by an earlier one. + inErrorHandler: true, + }); + }); + + it('flags the caller’s `If m_Error <> "" Then` guard as a channel read', () => { + const result = extract([ + 'Private m_Error As String', + '', + 'Public Sub Comprobar()', + ' If m_Error <> "" Then', + ' Exit Sub', + ' End If', + 'End Sub', + ]); + + const read = refOf(result, 'm_Error', 'read'); + expect(read).toBeDefined(); + expect(read!.referenceKind).toBe('property-get'); + expect(read!.metadata).toMatchObject({ + synthesizedBy: 'vba-module-var', + access: 'read', + errorChannel: true, + }); + // A read outside any handler region carries no #260 flag: absence is the + // encoding, so the key must not appear at all. + expect(read!.metadata).not.toHaveProperty('inErrorHandler'); + }); + + it('flags every default channel name, and only as a whole name', () => { + for (const name of DEFAULT_ERROR_CHANNEL_NAMES) { + const result = extract([ + `Private ${name} As String`, + '', + 'Public Sub Comprobar()', + ` If ${name} <> "" Then Exit Sub`, + 'End Sub', + ]); + const read = refOf(result, name, 'read'); + expect(read, `${name} should be flagged`).toBeDefined(); + expect(read!.metadata?.errorChannel).toBe(true); + } + }); +}); + +// ============================================================================ +// 2. The precision gate — a name, not a substring +// ============================================================================ + +describe('Issue #261 — the channel is a name, never a substring', () => { + it('does NOT flag a variable named `ErrorCount`', () => { + const result = extract([ + 'Private ErrorCount As Long', + 'Private m_Error As String', + '', + 'Public Sub Contar()', + ' ErrorCount = ErrorCount + 1', + ' m_Error = ""', + 'End Sub', + ]); + + const counted = moduleVarRefs(result, 'ErrorCount'); + expect(counted.length).toBeGreaterThan(0); + for (const ref of counted) { + expect(ref.metadata).not.toHaveProperty('errorChannel'); + } + // …while the real channel in the same procedure IS flagged, so the + // negative above cannot pass by the sweep simply not running. + expect(refOf(result, 'm_Error', 'write')?.metadata?.errorChannel).toBe(true); + }); + + it.each([ + 'ErrorCount', + 'm_ErrorLog', + 'HayErrorEnRiesgo', + 'ErroresRiesgoTexto', + 'gErrores', + ])('does NOT flag the neighbouring name `%s`', (name) => { + expect(isErrorChannelName(compileErrorChannel(), name)).toBe(false); + }); + + it('matches case-insensitively, because VBA does', () => { + const channel = compileErrorChannel(); + expect(isErrorChannelName(channel, 'M_ERROR')).toBe(true); + expect(isErrorChannelName(channel, 'error')).toBe(true); + }); +}); + +// ============================================================================ +// 3. The `vba.errorChannel` knob +// ============================================================================ + +describe('Issue #261 — `vba.errorChannel` extends the built-in list', () => { + it('flags a project-specific name AND keeps every default', () => { + const options: VbaExtractionOptions = { errorChannel: ['lastFailure'] }; + const result = extract( + [ + 'Private lastFailure As String', + 'Private m_Error As String', + '', + 'Public Sub Guardar()', + ' lastFailure = ""', + ' m_Error = ""', + 'End Sub', + ], + options, + ); + + expect(refOf(result, 'lastFailure', 'write')?.metadata?.errorChannel).toBe(true); + expect(refOf(result, 'm_Error', 'write')?.metadata?.errorChannel).toBe(true); + }); + + it('compiles the configured names on top of the defaults, de-duplicated', () => { + const channel = compileErrorChannel(['lastFailure', 'm_Error', ' ']); + expect(channel.names).toEqual([...DEFAULT_ERROR_CHANNEL_NAMES, 'lastFailure']); + expect(isErrorChannelName(channel, 'lastFailure')).toBe(true); + expect(isErrorChannelName(channel, 'lastFailureCount')).toBe(false); + }); + + it('drops entries that are not bare identifiers rather than compiling them', () => { + // No user-supplied regex, and nothing qualified: these run per identifier + // per line, so a `.*` entry would be a backtracking hazard, not a feature. + const channel = compileErrorChannel(['obj.Error', 'm_.*', '', 'ok_Name']); + expect(channel.names).toEqual([...DEFAULT_ERROR_CHANNEL_NAMES, 'ok_Name']); + }); + + it('drives the handler-behaviour classifier from the SAME list', () => { + // The knob must not leave `errorPolicy.behavior` reading a stale, separate + // copy of the names: a handler whose only channel write is the configured + // name is `channel`, not `unknown`. + const body = [ + 'Private lastFailure As String', + '', + 'Public Sub Guardar()', + ' On Error GoTo errores', + ' Exit Sub', + 'errores:', + ' lastFailure = "boom"', + 'End Sub', + ]; + const withKnob = extract(body, { errorChannel: ['lastFailure'] }); + expect( + (procedure(withKnob.nodes, 'Guardar')?.metadata?.errorPolicy as { behavior: string }) + .behavior, + ).toBe('channel'); + + const withoutKnob = extract(body); + expect( + (procedure(withoutKnob.nodes, 'Guardar')?.metadata?.errorPolicy as { behavior: string }) + .behavior, + ).toBe('unknown'); + }); +}); + +// ============================================================================ +// 4. The invariant: a field on existing rows, never a new row +// ============================================================================ + +describe('Issue #261 — adds no rows', () => { + const body = [ + 'Private m_Error As String', + 'Private m_Count As Long', + '', + 'Public Sub Guardar()', + ' On Error GoTo errores', + ' m_Count = m_Count + 1', + ' Exit Sub', + 'errores:', + ' m_Error = "boom"', + 'End Sub', + ]; + + it('emits the same nodes, edges and references with and without the knob', () => { + const plain = extract(body); + const configured = extract(body, { errorChannel: ['m_Count'] }); + + expect(configured.nodes.length).toBe(plain.nodes.length); + expect(configured.edges.length).toBe(plain.edges.length); + expect(configured.unresolvedReferences.length).toBe( + plain.unresolvedReferences.length, + ); + expect(new Set(configured.nodes.map((n) => n.kind))).toEqual( + new Set(plain.nodes.map((n) => n.kind)), + ); + expect(new Set(configured.edges.map((e) => e.kind))).toEqual( + new Set(plain.edges.map((e) => e.kind)), + ); + + // …and the ONLY observable difference is the flag on `m_Count`. + expect( + moduleVarRefs(plain, 'm_Count').every( + (r) => r.metadata?.errorChannel === undefined, + ), + ).toBe(true); + expect( + moduleVarRefs(configured, 'm_Count').every( + (r) => r.metadata?.errorChannel === true, + ), + ).toBe(true); + }); + + it('introduces no node kind and no edge kind of its own', () => { + const result = extract(body); + // The kinds below are the ones the VBA extractor already produced before + // this change; `label` and `handles-error` arrive from #263, which landed + // separately. The point of this guard is that the error-channel flag adds + // NOTHING to either set — it only stamps a field on rows that already + // exist — so it asserts a closed set rather than the absence of one kind. + const allowedNodeKinds = new Set(result.nodes.map((n) => n.kind)); + expect(allowedNodeKinds.has('variable')).toBe(true); + for (const node of result.nodes) { + expect([ + 'file', 'module', 'class', 'function', 'variable', 'constant', + 'parameter', 'label', 'property', 'form-layout', 'report-layout', + 'form-instance-control', + ]).toContain(node.kind); + } + for (const edge of result.edges) { + expect([ + 'contains', 'calls', 'references', 'type-member', 'type_of', + 'handles-error', 'event-handler', 'opens-form', 'opens-report', + 'raises-event', 'subscribes-event', + ]).toContain(edge.kind); + } + }); +}); + +// ============================================================================ +// 5. The boundary this change deliberately does NOT cross +// ============================================================================ + +describe('Issue #261 — what the module-variable channel cannot see', () => { + it('does not flag `Me.Error = …`, which is not a module-variable reference', () => { + // `Me.`-qualified and object-qualified writes are how the corpus publishes + // the message OUT of the object. The #251 sweep skips a dotted access by + // design (it names a member of something else), so no reference exists to + // flag. Pinned so the boundary is documented rather than discovered. + const result = extract([ + 'Public Error As String', + 'Private m_Error As String', + '', + 'Public Sub Guardar()', + ' Me.Error = m_Error', + 'End Sub', + ]); + expect(moduleVarRefs(result, 'Error')).toHaveLength(0); + expect(refOf(result, 'm_Error', 'read')?.metadata?.errorChannel).toBe(true); + }); + + it('does not flag a ByRef `p_Error` parameter, which is not module state', () => { + // The corpus's OTHER propagation mechanism: `Optional ByRef p_Error As + // String`. It is a parameter, so #205 scoping keeps it out of the + // module-variable sweep entirely and there is nothing for this task to + // flag. `ExpedienteOperaciones.cls` uses only this form. + const result = extract([ + 'Private m_Error As String', + '', + 'Private Function getHTML(Optional ByRef p_Error As String) As String', + ' On Error GoTo errores', + ' Exit Function', + 'errores:', + ' p_Error = "boom"', + 'End Function', + ]); + expect(moduleVarRefs(result, 'p_Error')).toHaveLength(0); + }); +}); + +// ============================================================================ +// 6. End to end — the chain must be TRAVERSABLE, not merely present +// ============================================================================ + +describe('Issue #261 — inner writes -> outer reads -> outer displays (integration)', () => { + let dir: string; + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-vba-errchan-')); + clearProjectConfigCache(); + }); + + afterEach(async () => { + clearProjectConfigCache(); + await new Promise((resolve) => setTimeout(resolve, 100)); + try { + fs.rmSync(dir, { recursive: true, force: true }); + } catch { + // ignore cleanup EPERM on Windows + } + }); + + /** + * The corpus shape, reduced: `Form_FormGestionRiesgos.cls`'s + * `EstablecerDatos` writes `m_Error`, `Form_Load` calls it, reads `m_Error` + * and shows it in a `MsgBox` from its handler. + */ + const FORM = [ + 'Attribute VB_Name = "Form_FormGestionRiesgos"', + 'Option Explicit', + 'Private m_Error As String', + '', + 'Public Sub Form_Load()', + ' On Error GoTo errores', + ' EstablecerDatos', + ' If m_Error <> "" Then', + ' Err.Raise 1000', + ' End If', + ' Exit Sub', + 'errores:', + ' If Err.Number <> 1000 Then', + ' m_Error = "Al Form_Load se ha producido el error: " & Err.Description', + ' End If', + ' MsgBox m_Error, vbCritical, "Error"', + 'End Sub', + '', + 'Private Sub EstablecerDatos()', + ' On Error GoTo errores', + ' Exit Sub', + 'errores:', + ' m_Error = "Es un usuario no autorizado"', + 'End Sub', + ].join('\n'); + + it('connects the writer to the reader through the channel variable, and the reader displays', async () => { + fs.writeFileSync(path.join(dir, 'Form_FormGestionRiesgos.cls'), FORM); + + const cg = CodeGraph.initSync(dir); + await cg.indexAll(); + + const channel = cg + .getNodesByName('m_Error') + .find((n) => n.kind === 'variable'); + expect(channel, 'the channel variable is a node').toBeDefined(); + + // Hop 1 — the inner procedure WRITES the channel. + const incoming = cg.getCallers(channel!.id); + const write = incoming.find( + (c) => c.edge.metadata?.access === 'write' && c.node.name === 'EstablecerDatos', + ); + expect(write, 'EstablecerDatos writes the channel').toBeDefined(); + expect(write!.edge.kind).toBe('references'); + expect(write!.edge.metadata?.refKind).toBe('property-set'); + expect(write!.edge.metadata?.errorChannel).toBe(true); + + // Hop 2 — the outer procedure READS the same channel node. Same target, + // so the two hops JOIN: this is the traversal, not two loose facts. + const read = incoming.find( + (c) => c.edge.metadata?.access === 'read' && c.node.name === 'Form_Load', + ); + expect(read, 'Form_Load reads the channel').toBeDefined(); + expect(read!.edge.metadata?.refKind).toBe('property-get'); + expect(read!.edge.metadata?.errorChannel).toBe(true); + + // …and the caller relationship that makes the propagation real. + const formLoad = read!.node; + const callees = cg.getCallees(formLoad.id); + expect(callees.some((c) => c.node.name === 'EstablecerDatos')).toBe(true); + + // Hop 3 — the reader DISPLAYS it. `MsgBox` is a VBA built-in, so it is not + // a node; E3's `errorPolicy.behavior` is the graph's display marker, and + // it reports both signals for this procedure (channel + display). + expect( + (formLoad.metadata?.errorPolicy as { behavior: string }).behavior, + ).toBe('mixed'); + expect( + (write!.node.metadata?.errorPolicy as { behavior: string }).behavior, + ).toBe('channel'); + + await cg.destroy(); + }); + + it('reads `vba.errorChannel` out of codegraph.json', async () => { + fs.writeFileSync( + path.join(dir, 'codegraph.json'), + JSON.stringify({ vba: { errorChannel: ['lastFailure', 'obj.Nope'] } }), + ); + const vba = loadVbaConfig(dir); + // The dotted entry is dropped by validation; the valid one survives. + expect(vba.errorChannel).toEqual(['lastFailure']); + + fs.writeFileSync( + path.join(dir, 'Fallos.bas'), + [ + 'Attribute VB_Name = "Fallos"', + 'Option Explicit', + 'Private lastFailure As String', + '', + 'Public Sub Guardar()', + ' lastFailure = "boom"', + 'End Sub', + ].join('\n'), + ); + + const cg = CodeGraph.initSync(dir); + await cg.indexAll(); + + const channel = cg + .getNodesByName('lastFailure') + .find((n) => n.kind === 'variable'); + expect(channel).toBeDefined(); + const incoming = cg.getCallers(channel!.id); + expect( + incoming.some((c) => c.edge.metadata?.errorChannel === true), + ).toBe(true); + + await cg.destroy(); + }); +}); diff --git a/__tests__/extraction-vba-error-handler-region.test.ts b/__tests__/extraction-vba-error-handler-region.test.ts index 9d6e0bf..b9fccb5 100644 --- a/__tests__/extraction-vba-error-handler-region.test.ts +++ b/__tests__/extraction-vba-error-handler-region.test.ts @@ -206,7 +206,7 @@ describe('VBA error-handler region — inErrorHandler (issue #260)', () => { expect(flagged(opens[1])).toBe(true); }); - it('adds no node, no edge and no unresolved reference', () => { + it('adds no node, no edge and no unresolved reference of its own', () => { const body = [ 'Public Sub Guardar()', ' On Error GoTo errores', @@ -226,8 +226,15 @@ describe('VBA error-handler region — inErrorHandler (issue #260)', () => { ), ); + // Issue #263 (task E6) later added the `handles-error` edge onto the + // label node — the one sanctioned addition in this wave, and the only + // row either side does not share. Set it aside and the invariant this + // task is judged by still holds: #260 stamps a FIELD, it creates nothing. + const rows = (r: typeof withHandler) => + r.edges.filter((e) => e.kind !== 'handles-error'); + expect(withHandler.nodes).toHaveLength(withoutRegion.nodes.length); - expect(withHandler.edges).toHaveLength(withoutRegion.edges.length); + expect(rows(withHandler)).toHaveLength(rows(withoutRegion).length); expect(withHandler.unresolvedReferences).toHaveLength( withoutRegion.unresolvedReferences.length, ); diff --git a/__tests__/extraction-vba-error-policy.test.ts b/__tests__/extraction-vba-error-policy.test.ts index a965308..39ec9fc 100644 --- a/__tests__/extraction-vba-error-policy.test.ts +++ b/__tests__/extraction-vba-error-policy.test.ts @@ -26,7 +26,7 @@ import { describe, expect, it } from 'vitest'; import { VbaExtractor } from '../src/extraction/vba-extractor'; import { VBA_RULE_TABLES } from '../src/extraction/vba-extractor'; import { RULES } from '../src/extraction/vba/errors'; -import { Node } from '../src/types'; +import { Edge, Node } from '../src/types'; interface ErrorPolicy { protection: 'handler' | 'resume-next' | 'none'; @@ -67,14 +67,17 @@ function policy(nodes: Node[], name: string): ErrorPolicy { } describe('Issue #259: the rule table', () => { - it('registers `errors` in VBA_RULE_TABLES with exactly the four tabulated rules', () => { - // The issue tabulates these four ids and no others; they are also the - // handles `codegraph stats vba-rules` reports. + it('registers `errors` in VBA_RULE_TABLES with the four tabulated rules', () => { + // The issue tabulates these four ids; they are also the handles + // `codegraph stats vba-rules` reports. `goto-jump` was appended by issue + // #263, which needs the plain-`GoTo` jumps this table had no reason to + // look at while it emitted nothing. expect(VBA_RULE_TABLES.errors?.map((r) => r.id)).toEqual([ 'on-error-label', 'on-error-resume-next', 'on-error-reset', 'line-label', + 'goto-jump', ]); expect(VBA_RULE_TABLES.errors).toBe(RULES); }); @@ -497,11 +500,34 @@ describe('Issue #259: regression guards', () => { }); }); +/** + * Issue #263 (task E6) later added a `label` node and a `handles-error` edge + * on top of this classifier — with maintainer sign-off, and against the + * budget §4.3 of the plan sets out. It is the ONLY thing allowed to add rows + * here, so the guard below still holds once its rows are set aside: the + * error-POLICY classifier itself must remain a pure annotator. + */ +function withoutIssue263Rows(result: { + nodes: Node[]; + edges: Edge[]; +}): { nodes: Node[]; edges: Edge[] } { + const labelIds = new Set( + result.nodes.filter((n) => n.kind === 'label').map((n) => n.id), + ); + return { + nodes: result.nodes.filter((n) => n.kind !== 'label'), + edges: result.edges.filter( + (e) => e.kind !== 'handles-error' && !labelIds.has(e.target), + ), + }; +} + describe('Issue #259: zero new node kinds, zero new edge kinds', () => { - it('emits no node and no edge for the handler, the label or the policy', () => { - // The merge-blocking constraint of the whole error-handling wave. The - // handler-bearing module must produce exactly the nodes and edges the - // module WITHOUT any `On Error` produces, plus nothing. + it('emits no node and no edge for the handler or the policy', () => { + // The merge-blocking constraint of the whole error-handling wave. Setting + // #263's label rows aside, the handler-bearing module must produce exactly + // the nodes and edges the module WITHOUT any `On Error` produces, plus + // nothing. const withHandler = extract([ 'Public Sub Guardar()', ' On Error GoTo errores', @@ -520,20 +546,23 @@ describe('Issue #259: zero new node kinds, zero new edge kinds', () => { 'End Sub', ]); - expect(withHandler.nodes.map((n) => n.kind).sort()).toEqual( - withoutHandler.nodes.map((n) => n.kind).sort(), + const withHandlerRows = withoutIssue263Rows(withHandler); + const withoutHandlerRows = withoutIssue263Rows(withoutHandler); + + expect(withHandlerRows.nodes.map((n) => n.kind).sort()).toEqual( + withoutHandlerRows.nodes.map((n) => n.kind).sort(), ); - expect(withHandler.edges.map((e) => e.kind).sort()).toEqual( - withoutHandler.edges.map((e) => e.kind).sort(), + expect(withHandlerRows.edges.map((e) => e.kind).sort()).toEqual( + withoutHandlerRows.edges.map((e) => e.kind).sort(), ); - expect(withHandler.nodes.length).toBe(withoutHandler.nodes.length); - expect(withHandler.edges.length).toBe(withoutHandler.edges.length); + expect(withHandlerRows.nodes.length).toBe(withoutHandlerRows.nodes.length); + expect(withHandlerRows.edges.length).toBe(withoutHandlerRows.edges.length); expect(withHandler.unresolvedReferences.length).toBe( withoutHandler.unresolvedReferences.length, ); - // No node is named after the label, in any kind. - expect(withHandler.nodes.some((n) => n.name === 'errores')).toBe(false); + // Outside #263's own `label` kind, no node is named after the label. + expect(withHandlerRows.nodes.some((n) => n.name === 'errores')).toBe(false); }); it('a module-level `On Error` line alone still creates no module node', () => { diff --git a/__tests__/extraction-vba-labels.test.ts b/__tests__/extraction-vba-labels.test.ts new file mode 100644 index 0000000..a43adae --- /dev/null +++ b/__tests__/extraction-vba-labels.test.ts @@ -0,0 +1,583 @@ +/** + * Issue #263 (task E6 of `docs/vba-error-handling-plan.md`) — `label` nodes + * and `handles-error` edges. + * + * #259 records *whether* a procedure has a handler; #260 marks *which* edges + * come from inside one. Neither gives the handler an identity you can point + * at, search for or traverse to. This does — and the risk it carries is the + * reason most of the assertions below exist: + * + * 1. **Collision.** VBA scopes a line label to its procedure and this + * corpus writes the same label everywhere (`errores` is defined 3,735 + * times). Without the procedure segment in `qualifiedName`, every + * handler in a project collapses into one symbol. `two procedures in + * one module` is the guard. + * 2. **The precision gate.** A label nobody targets with `On Error GoTo` + * is control flow, not a handler. Calling one a handler is the worst + * answer available here — a confidently wrong "yes" to "does this + * handle errors". + * 3. **No fabricated targets.** `On Error GoTo noExiste` must leave the + * graph honest: an unresolved reference and NO node. A graph that + * invents its own targets cannot be used to find the defect. + * 4. **No re-parenting.** Calls inside a handler stay attributed to the + * enclosing procedure. `callers`/`callees` for every procedure with a + * handler must be untouched. + * 5. **Flood control.** ~3,900 label nodes must stay out of the two + * default result surfaces, exactly as #257's parameters did. + */ +import { describe, expect, it } from 'vitest'; +import * as fs from 'fs'; +import * as path from 'path'; +import { VbaExtractor } from '../src/extraction/vba-extractor'; +import { RULES } from '../src/extraction/vba/errors'; +import { parseQuery } from '../src/search/query-parser'; +import { Edge, Node, UnresolvedReference } from '../src/types'; + +/** + * Extract a `.bas` module. The two header lines are prepended here so a + * fixture's own first line is source line 3 — the convention + * `extraction-vba-error-policy.test.ts` and + * `extraction-vba-error-handler-region.test.ts` both use. + */ +function extract(body: string[], filePath = 'src/modules/ModErrores.bas') { + return new VbaExtractor( + filePath, + ['Attribute VB_Name = "ModErrores"', 'Option Explicit', ...body].join('\n'), + ).extract(); +} + +function labels(nodes: Node[]): Node[] { + return nodes.filter((n) => n.kind === 'label'); +} + +function label(nodes: Node[], qualifiedName: string): Node | undefined { + return labels(nodes).find((n) => n.qualifiedName === qualifiedName); +} + +/** Declared procedures only — call-target stubs carry `metadata.stub`. */ +function procedure(nodes: Node[], name: string): Node | undefined { + return nodes.find( + (n) => n.kind === 'function' && n.metadata?.stub !== true && n.name === name, + ); +} + +function edgesOfKind(edges: Edge[], kind: string, sourceId?: string): Edge[] { + return edges.filter( + (e) => e.kind === kind && (sourceId === undefined || e.source === sourceId), + ); +} + +function refsTo(refs: UnresolvedReference[], name: string): UnresolvedReference[] { + return refs.filter((r) => r.referenceName === name); +} + +// ============================================================================ +// 1. Node shape +// ============================================================================ + +describe('Issue #263 — label node shape', () => { + it('emits one node per label definition, scoped to module and procedure', () => { + const { nodes } = extract([ + 'Public Sub Guardar()', + ' On Error GoTo errores', + ' Call Escribir', + ' Exit Sub', + 'errores:', + ' p_Error = "boom"', + 'End Sub', + ]); + + const found = labels(nodes); + expect(found).toHaveLength(1); + const node = found[0]!; + expect(node.name).toBe('errores'); + expect(node.qualifiedName).toBe('ModErrores.Guardar.errores'); + expect(node.filePath).toBe('src/modules/ModErrores.bas'); + expect(node.language).toBe('vba'); + // Header (2 lines) + `Public Sub` (3) + `On Error` (4) + `Call` (5) + + // `Exit Sub` (6) → the label is line 7. + expect(node.startLine).toBe(7); + }); + + it('spans a handler label to the procedure end and a control-flow label to its own line', () => { + const { nodes } = extract([ + 'Public Sub Recorrer()', + ' On Error GoTo errores', + ' Call Primero', + 'siguiente:', + ' Call Segundo', + ' Exit Sub', + 'errores:', + ' p_Error = "boom"', + 'End Sub', + ]); + + const siguiente = label(nodes, 'ModErrores.Recorrer.siguiente'); + const errores = label(nodes, 'ModErrores.Recorrer.errores'); + expect(siguiente).toBeDefined(); + expect(errores).toBeDefined(); + + // `siguiente:` is line 6, `errores:` is line 9, `End Sub` is line 11. + expect(siguiente!.startLine).toBe(6); + expect(siguiente!.endLine).toBe(6); + expect(errores!.startLine).toBe(9); + expect(errores!.endLine).toBe(11); + }); + + it('copies handlerBehavior and the region lines from the procedure errorPolicy', () => { + const { nodes } = extract([ + 'Public Sub Guardar()', + ' On Error GoTo errores', + ' Call Escribir', + ' Exit Sub', + 'errores:', + ' p_Error = "boom"', + ' MsgBox "boom"', + 'End Sub', + ]); + + const policy = procedure(nodes, 'Guardar')!.metadata!.errorPolicy as { + behavior: string | null; + handlerStartLine: number | null; + handlerEndLine: number | null; + }; + const node = label(nodes, 'ModErrores.Guardar.errores')!; + + // Copied, never re-derived: the node must agree with the policy #260 + // already published, whatever that policy says. + expect(policy.behavior).toBe('mixed'); + expect(node.metadata?.handlerBehavior).toBe(policy.behavior); + expect(node.metadata?.regionStartLine).toBe(policy.handlerStartLine); + expect(node.metadata?.regionEndLine).toBe(policy.handlerEndLine); + }); +}); + +// ============================================================================ +// 2. The collision trap — the whole reason qualifiedName carries the procedure +// ============================================================================ + +describe('Issue #263 — procedure-scoped identity', () => { + it('gives two procedures in one module that both define `errores` distinct nodes', () => { + const { nodes } = extract([ + 'Public Sub Guardar()', + ' On Error GoTo errores', + ' Call Escribir', + ' Exit Sub', + 'errores:', + ' p_Error = "guardar"', + 'End Sub', + '', + 'Public Sub Borrar()', + ' On Error GoTo errores', + ' Call Eliminar', + ' Exit Sub', + 'errores:', + ' p_Error = "borrar"', + 'End Sub', + ]); + + const found = labels(nodes); + expect(found).toHaveLength(2); + expect(found.map((n) => n.name)).toEqual(['errores', 'errores']); + + // Distinct qualified names… + expect(found.map((n) => n.qualifiedName).sort()).toEqual([ + 'ModErrores.Borrar.errores', + 'ModErrores.Guardar.errores', + ]); + // …and distinct ids. Without either, 3,735 handlers become one symbol. + expect(new Set(found.map((n) => n.id)).size).toBe(2); + + }); + + it('keeps the module prefix off a file with no VB_Name and still scopes to the procedure', () => { + const { nodes } = new VbaExtractor( + 'src/modules/Sin Nombre.bas', + ['Public Sub Guardar()', ' On Error GoTo errores', 'errores:', 'End Sub'].join( + '\n', + ), + ).extract(); + + // No `Attribute VB_Name` → `ctx.moduleName` falls back to the basename, + // which is still a real scope. What must never happen is a bare `errores`. + const node = labels(nodes)[0]!; + expect(node.qualifiedName).toMatch(/\.Guardar\.errores$/); + expect(node.qualifiedName).not.toBe('errores'); + }); +}); + +// ============================================================================ +// 3. isHandler — the precision gate +// ============================================================================ + +describe('Issue #263 — isHandler', () => { + it('marks an On Error GoTo target as a handler', () => { + const { nodes } = extract([ + 'Public Sub Guardar()', + ' On Error GoTo errores', + 'errores:', + ' p_Error = "boom"', + 'End Sub', + ]); + expect(label(nodes, 'ModErrores.Guardar.errores')!.metadata?.isHandler).toBe( + true, + ); + }); + + it('marks a label nobody targets as control flow, with no region and no handles-error edge', () => { + const { nodes, edges } = extract([ + 'Public Sub Recorrer()', + ' Dim i As Long', + ' For i = 1 To 10', + ' GoTo siguiente', + 'siguiente:', + ' Next i', + 'End Sub', + ]); + + const node = label(nodes, 'ModErrores.Recorrer.siguiente')!; + expect(node.metadata?.isHandler).toBe(false); + // No region at all — not a null region, no key. + expect(node.metadata).not.toHaveProperty('handlerBehavior'); + expect(node.metadata).not.toHaveProperty('regionStartLine'); + expect(node.metadata).not.toHaveProperty('regionEndLine'); + // And nothing routes errors to it. + expect(edgesOfKind(edges, 'handles-error')).toHaveLength(0); + }); + + it('does not treat a label mentioned inside a string literal as a handler', () => { + const { nodes, edges } = extract([ + 'Public Sub Guardar()', + ' Dim s As String', + ' s = "On Error GoTo errores"', + 'errores:', + 'End Sub', + ]); + + // The label definition is real; the `On Error` inside the string is not, + // so nothing targets it (#209 discipline). + expect(label(nodes, 'ModErrores.Guardar.errores')!.metadata?.isHandler).toBe( + false, + ); + expect(edgesOfKind(edges, 'handles-error')).toHaveLength(0); + }); +}); + +// ============================================================================ +// 4. Edges +// ============================================================================ + +describe('Issue #263 — edges onto the label', () => { + it('emits a contains edge from the owning procedure for every label', () => { + const { nodes, edges } = extract([ + 'Public Sub Recorrer()', + ' On Error GoTo errores', + 'siguiente:', + ' Exit Sub', + 'errores:', + 'End Sub', + ]); + + const proc = procedure(nodes, 'Recorrer')!; + const labelIds = new Set(labels(nodes).map((n) => n.id)); + expect(labelIds.size).toBe(2); + + const contains = edgesOfKind(edges, 'contains', proc.id).filter((e) => + labelIds.has(e.target), + ); + expect(contains).toHaveLength(2); + expect(new Set(contains.map((e) => e.target))).toEqual(labelIds); + }); + + it('emits one handles-error edge per On Error GoTo statement, not per target', () => { + const { nodes, edges } = extract([ + 'Public Sub Guardar()', + ' On Error GoTo errores', + ' Call Escribir', + ' On Error GoTo errores', + ' Call Confirmar', + ' Exit Sub', + 'errores:', + ' p_Error = "boom"', + 'End Sub', + ]); + + const proc = procedure(nodes, 'Guardar')!; + const target = label(nodes, 'ModErrores.Guardar.errores')!; + const handles = edgesOfKind(edges, 'handles-error', proc.id); + + // TWO statements, TWO edges — deliberately not deduplicated. Each swap is + // its own routing decision with its own line. + expect(handles).toHaveLength(2); + expect(handles.every((e) => e.target === target.id)).toBe(true); + expect(handles.map((e) => e.line)).toEqual([4, 6]); + expect(handles.every((e) => e.metadata?.synthesizedBy === 'vba-error-handler')).toBe( + true, + ); + expect(handles.every((e) => e.provenance === 'heuristic')).toBe(true); + }); + + it('emits one handles-error edge per label when a procedure swaps handlers', () => { + const { nodes, edges } = extract([ + 'Public Sub Guardar()', + ' On Error GoTo errores', + ' Call Escribir', + ' On Error GoTo errMem', + ' Call Reservar', + ' Exit Sub', + 'errores:', + ' p_Error = "boom"', + ' Exit Sub', + 'errMem:', + ' p_Error = "sin memoria"', + 'End Sub', + ]); + + const proc = procedure(nodes, 'Guardar')!; + const handles = edgesOfKind(edges, 'handles-error', proc.id); + expect(handles).toHaveLength(2); + + const byTarget = new Map(handles.map((e) => [e.target, e])); + const errores = label(nodes, 'ModErrores.Guardar.errores')!; + const errMem = label(nodes, 'ModErrores.Guardar.errMem')!; + expect(byTarget.has(errores.id)).toBe(true); + expect(byTarget.has(errMem.id)).toBe(true); + + // Both are handlers. Only the one whose region `errorPolicy` resolved — + // the earliest targeted definition — carries the derived behaviour, since + // that is the only value #260 computed. The second is NOT re-classified + // here; that would be exactly the drift this design forbids. + expect(errores.metadata?.isHandler).toBe(true); + expect(errMem.metadata?.isHandler).toBe(true); + expect(errores.metadata?.handlerBehavior).toBe('channel'); + expect(errMem.metadata).not.toHaveProperty('handlerBehavior'); + }); + + it('emits a references edge tagged vba-goto for a plain GoTo', () => { + const { nodes, edges } = extract([ + 'Public Sub Recorrer()', + ' Dim i As Long', + ' If i = 0 Then GoTo salir', + ' Call Trabajar', + 'salir:', + 'End Sub', + ]); + + const proc = procedure(nodes, 'Recorrer')!; + const salir = label(nodes, 'ModErrores.Recorrer.salir')!; + const jumps = edgesOfKind(edges, 'references', proc.id).filter( + (e) => e.target === salir.id, + ); + + expect(jumps).toHaveLength(1); + expect(jumps[0]!.metadata?.synthesizedBy).toBe('vba-goto'); + expect(jumps[0]!.line).toBe(5); + // A jump is not an error-handling fact; it must NOT borrow the new kind. + expect(edgesOfKind(edges, 'handles-error')).toHaveLength(0); + }); + + it('does not mistake an On Error GoTo for a plain GoTo', () => { + const { nodes, edges } = extract([ + 'Public Sub Guardar()', + ' On Error GoTo errores', + 'errores:', + 'End Sub', + ]); + + const proc = procedure(nodes, 'Guardar')!; + const errores = label(nodes, 'ModErrores.Guardar.errores')!; + expect(edgesOfKind(edges, 'handles-error', proc.id)).toHaveLength(1); + expect( + edgesOfKind(edges, 'references', proc.id).filter( + (e) => e.target === errores.id, + ), + ).toHaveLength(0); + }); + + it('ignores a numeric GoTo target — a VBA line number is not a label', () => { + const { nodes, edges, unresolvedReferences } = extract([ + 'Public Sub Antiguo()', + ' GoTo 100', + 'End Sub', + ]); + + expect(labels(nodes)).toHaveLength(0); + expect(edgesOfKind(edges, 'references').filter((e) => e.metadata?.synthesizedBy === 'vba-goto')).toHaveLength(0); + // And no fabricated dangling reference for legal code. + expect(refsTo(unresolvedReferences, '100')).toHaveLength(0); + }); +}); + +// ============================================================================ +// 5. Dangling targets — never fabricate a node +// ============================================================================ + +describe('Issue #263 — dangling targets', () => { + it('emits an UnresolvedReference and no node for On Error GoTo noExiste', () => { + const { nodes, edges, unresolvedReferences } = extract([ + 'Public Sub Guardar()', + ' On Error GoTo noExiste', + ' Call Escribir', + 'End Sub', + ]); + + // No node. The defect is only findable while the graph stays honest. + expect(labels(nodes)).toHaveLength(0); + expect(edgesOfKind(edges, 'handles-error')).toHaveLength(0); + + const dangling = refsTo(unresolvedReferences, 'noExiste'); + expect(dangling).toHaveLength(1); + expect(dangling[0]!.referenceKind).toBe('references'); + expect(dangling[0]!.metadata?.synthesizedBy).toBe('vba-goto-unresolved'); + expect(dangling[0]!.fromNodeId).toBe(procedure(nodes, 'Guardar')!.id); + + // …and `errorPolicy` says the same thing, from the other side. + expect( + (procedure(nodes, 'Guardar')!.metadata!.errorPolicy as { danglingTarget: string | null }) + .danglingTarget, + ).toBe('noExiste'); + }); + + it('treats a label defined in a SIBLING procedure as still dangling', () => { + const { nodes, unresolvedReferences } = extract([ + 'Public Sub Guardar()', + ' On Error GoTo errores', + ' Call Escribir', + 'End Sub', + '', + 'Public Sub Borrar()', + 'errores:', + ' p_Error = "borrar"', + 'End Sub', + ]); + + // VBA scopes labels to the procedure, so `Guardar` has no handler even + // though the module contains an `errores:` somewhere else. + expect(refsTo(unresolvedReferences, 'errores')).toHaveLength(1); + expect(labels(nodes).map((n) => n.qualifiedName)).toEqual([ + 'ModErrores.Borrar.errores', + ]); + // And that sibling label is control flow — nothing in ITS procedure + // targets it. + expect(label(nodes, 'ModErrores.Borrar.errores')!.metadata?.isHandler).toBe( + false, + ); + }); +}); + +// ============================================================================ +// 6. No re-parenting — the invariant that protects every existing consumer +// ============================================================================ + +describe('Issue #263 — calls inside a handler stay on the procedure', () => { + it('leaves every call reference attributed to the enclosing procedure', () => { + const { nodes, unresolvedReferences } = extract([ + 'Public Sub Guardar()', + ' On Error GoTo errores', + ' Call Escribir', + ' Exit Sub', + 'errores:', + ' Call Registrar', + 'End Sub', + ]); + + const proc = procedure(nodes, 'Guardar')!; + const labelIds = new Set(labels(nodes).map((n) => n.id)); + + // The handler-body call is still the PROCEDURE's, flagged by #260 rather + // than re-parented. Re-parenting would change `callers`/`callees` for + // every procedure with a handler in the corpus. + const registrar = refsTo(unresolvedReferences, 'Registrar'); + expect(registrar).toHaveLength(1); + expect(registrar[0]!.fromNodeId).toBe(proc.id); + expect(registrar[0]!.metadata?.inErrorHandler).toBe(true); + + // Nothing at all is sourced FROM a label node. + expect( + unresolvedReferences.some((r) => labelIds.has(r.fromNodeId)), + ).toBe(false); + }); + + it('stamps inErrorHandler on a GoTo written inside the handler region', () => { + const { nodes, edges } = extract([ + 'Public Sub Guardar()', + ' On Error GoTo errores', + ' Call Escribir', + ' Exit Sub', + 'errores:', + ' GoTo salir', + 'salir:', + 'End Sub', + ]); + + const proc = procedure(nodes, 'Guardar')!; + const salir = label(nodes, 'ModErrores.Guardar.salir')!; + const jump = edgesOfKind(edges, 'references', proc.id).find( + (e) => e.target === salir.id, + ); + expect(jump).toBeDefined(); + // #260 owns the single stamping point; the new emitter must not opt out. + expect(jump!.metadata?.inErrorHandler).toBe(true); + }); +}); + +// ============================================================================ +// 7. The rule table +// ============================================================================ + +describe('Issue #263 — the goto-jump rule', () => { + it('is registered on the errors table, masked and gated on an open procedure', () => { + const rule = RULES.find((r) => r.id === 'goto-jump'); + expect(rule).toBeDefined(); + expect(rule!.scan).toBe('masked'); + expect(rule!.requires).toBe('inside-procedure'); + }); + + it('records nothing for a GoTo outside any procedure body', () => { + const { nodes, edges } = extract(['Public Const X As Long = 1']); + expect(labels(nodes)).toHaveLength(0); + expect(edgesOfKind(edges, 'handles-error')).toHaveLength(0); + }); +}); + +// ============================================================================ +// 8. Search +// ============================================================================ + +describe('Issue #263 — kind:label is a search filter', () => { + it('parses `kind:label` into a kind filter rather than free text', () => { + const parsed = parseQuery('kind:label errores'); + expect(parsed.kinds).toContain('label'); + expect(parsed.text).toBe('errores'); + }); +}); + +// ============================================================================ +// 9. The two deliberate exclusions (the #257 precedent) +// ============================================================================ + +describe('Issue #263 — `label` stays out of the default result surfaces', () => { + it('is absent from HIGH_VALUE_NODE_KINDS', () => { + const source = fs.readFileSync( + path.join(__dirname, '..', 'src', 'context', 'index.ts'), + 'utf-8', + ); + const block = /const HIGH_VALUE_NODE_KINDS: NodeKind\[\] = \[([\s\S]*?)\];/.exec( + source, + ); + expect(block).not.toBeNull(); + expect(block![1]).not.toMatch(/'label'/); + }); + + it('is absent from CONTAINER_NODE_KINDS', () => { + const source = fs.readFileSync( + path.join(__dirname, '..', 'src', 'mcp', 'tools.ts'), + 'utf-8', + ); + const block = /const CONTAINER_NODE_KINDS = new Set\(\[([\s\S]*?)\]\);/.exec( + source, + ); + expect(block).not.toBeNull(); + expect(block![1]).not.toMatch(/'label'/); + }); +}); diff --git a/__tests__/extraction-vba-on-error-keyword.test.ts b/__tests__/extraction-vba-on-error-keyword.test.ts new file mode 100644 index 0000000..515235d --- /dev/null +++ b/__tests__/extraction-vba-on-error-keyword.test.ts @@ -0,0 +1,145 @@ +/** + * Issue #292 — `On Error` is a statement keyword pair, not a variable read. + * + * `scanModuleVariableReferences` walks every identifier on a line and emits a + * reference for any that names a module-level variable. It guarded against a + * `.` / `!` prefix and against procedure-local shadowing (#205), but not + * against VBA keyword context — so in a module declaring the house-convention + * `Public Error As String`, every `On Error GoTo errores` reported a + * `property-get` of a variable named `Error`. + * + * On its own that is a stray edge. Once #261 labels channel references with + * `errorChannel: true` it becomes a confident, wrong claim that the line 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. + */ +import { describe, it, expect } from 'vitest'; +import { VbaExtractor } from '../src/extraction/vba-extractor'; + +function extract(filePath: string, source: string) { + return new VbaExtractor(filePath, source).extract(); +} + +/** Every module-variable reference the sweep emitted, in source order. */ +function moduleVarRefs(result: ReturnType) { + return result.unresolvedReferences.filter( + (u) => + (u.metadata as Record | undefined)?.synthesizedBy === + 'vba-module-var', + ); +} + +function cls(...body: string[]): string { + return ['Attribute VB_Name = "Riesgo"', 'Public Error As String', '', ...body, ''].join('\n'); +} + +describe('issue #292 — On Error is a keyword, not a read of a variable named Error', () => { + it('emits only the genuine write when a handler assigns the channel', () => { + const result = extract( + 'Riesgo.cls', + cls( + 'Public Sub Guardar()', + ' On Error GoTo errores', + ' Exit Sub', + 'errores:', + ' Error = "boom"', + 'End Sub', + ), + ); + const refs = moduleVarRefs(result); + expect(refs).toHaveLength(1); + expect(refs[0]!.referenceKind).toBe('property-set'); + expect(refs[0]!.line).toBe(8); + }); + + it('emits nothing for On Error Resume Next', () => { + const result = extract( + 'Riesgo.cls', + cls('Public Sub Guardar()', ' On Error Resume Next', 'End Sub'), + ); + expect(moduleVarRefs(result)).toHaveLength(0); + }); + + it('emits nothing for On Error GoTo 0', () => { + const result = extract( + 'Riesgo.cls', + cls('Public Sub Guardar()', ' On Error GoTo 0', 'End Sub'), + ); + expect(moduleVarRefs(result)).toHaveLength(0); + }); + + it('emits nothing for On Error GoTo -1', () => { + const result = extract( + 'Riesgo.cls', + cls('Public Sub Guardar()', ' On Error GoTo -1', 'End Sub'), + ); + expect(moduleVarRefs(result)).toHaveLength(0); + }); + + it('tolerates extra whitespace between On and Error', () => { + const result = extract( + 'Riesgo.cls', + cls('Public Sub Guardar()', ' On Error GoTo errores', 'End Sub'), + ); + expect(moduleVarRefs(result)).toHaveLength(0); + }); + + it('still reports a genuine bare read', () => { + const result = extract( + 'Riesgo.cls', + cls('Public Sub Comprobar()', ' If Error <> "" Then Exit Sub', 'End Sub'), + ); + const refs = moduleVarRefs(result); + expect(refs).toHaveLength(1); + expect(refs[0]!.referenceKind).toBe('property-get'); + }); + + it('still reports a genuine write', () => { + const result = extract( + 'Riesgo.cls', + cls('Public Sub Fallar()', ' Error = "x"', 'End Sub'), + ); + const refs = moduleVarRefs(result); + expect(refs).toHaveLength(1); + expect(refs[0]!.referenceKind).toBe('property-set'); + }); + + it('keeps column offsets intact when a genuine reference follows On Error on one line', () => { + // The mask replaces `On Error` with spaces of the SAME length, so the + // column reported for the later reference must be its real offset. A + // substitution that shortened the line would silently shift every column. + const statement = ' On Error GoTo errores: Error = "x"'; + const result = extract( + 'Riesgo.cls', + cls('Public Sub Mixto()', statement, 'End Sub'), + ); + const refs = moduleVarRefs(result); + expect(refs).toHaveLength(1); + expect(refs[0]!.column).toBe(statement.indexOf('Error = "x"')); + // The kind here is `property-get`, not `property-set`: after a colon the + // prefix `isDirectAssignment` sees is `… GoTo errores: `, which is not + // blank, so a colon-separated assignment reads as an access rather than a + // write. That is a pre-existing limitation of the shared predicate, not + // something this mask introduces — asserted so the behaviour is pinned + // rather than silently depended on. + expect(refs[0]!.referenceKind).toBe('property-get'); + }); + + it('leaves the existing member-access guard alone', () => { + const result = extract( + 'Riesgo.cls', + cls('Public Sub Leer()', ' x = obj.Error', ' y = Me!Error', 'End Sub'), + ); + expect(moduleVarRefs(result)).toHaveLength(0); + }); + + it('does not mask a variable that merely starts with the word On', () => { + const result = extract( + 'Riesgo.cls', + ['Attribute VB_Name = "Riesgo"', 'Public OnError As String', '', 'Public Sub Leer()', ' x = OnError', 'End Sub', ''].join('\n'), + ); + const refs = moduleVarRefs(result); + expect(refs).toHaveLength(1); + expect(refs[0]!.referenceName).toBe('OnError'); + }); +}); diff --git a/__tests__/stats-vba-rules.test.ts b/__tests__/stats-vba-rules.test.ts index 12d529a..b42e9c7 100644 --- a/__tests__/stats-vba-rules.test.ts +++ b/__tests__/stats-vba-rules.test.ts @@ -81,7 +81,7 @@ describe('buildStatsVbaRules() — pure unit shape (issue #168)', () => { expect(byName.declarations?.ruleCount).toBe(5); expect(byName.dims?.ruleCount).toBe(2); expect(byName['enums-consts']?.ruleCount).toBe(6); - expect(byName.errors?.ruleCount).toBe(4); + expect(byName.errors?.ruleCount).toBe(5); expect(byName['call-sweep']?.ruleCount).toBe(4); }); @@ -89,7 +89,7 @@ describe('buildStatsVbaRules() — pure unit shape (issue #168)', () => { const out = buildStatsVbaRules(); const sum = out.concerns.reduce((acc, c) => acc + c.ruleCount, 0); expect(out.totalRules).toBe(sum); - expect(out.totalRules).toBe(23); + expect(out.totalRules).toBe(24); }); it('preserves VBA_RULE_TABLES key order in concerns[]', () => { @@ -188,7 +188,7 @@ describe('codegraph stats vba-rules --json (CLI integration, issue #168)', () => }>; totalRules: number; }; - expect(parsed.totalRules).toBe(23); + expect(parsed.totalRules).toBe(24); expect(parsed.concerns).toHaveLength(7); // Every concern has matching static count. const byName = Object.fromEntries(parsed.concerns.map((c) => [c.concern, c])); @@ -197,7 +197,7 @@ describe('codegraph stats vba-rules --json (CLI integration, issue #168)', () => expect(byName.declarations!.ruleCount).toBe(5); expect(byName.dims!.ruleCount).toBe(2); expect(byName['enums-consts']!.ruleCount).toBe(6); - expect(byName.errors!.ruleCount).toBe(4); + expect(byName.errors!.ruleCount).toBe(5); expect(byName['call-sweep']!.ruleCount).toBe(4); }); @@ -234,7 +234,7 @@ describe('codegraph stats vba-rules --json (CLI integration, issue #168)', () => expect(stdout).toContain(concern); } // The pretty mode surfaces the total somewhere on stdout. - expect(stdout).toMatch(/\b23\b/); + expect(stdout).toMatch(/\b24\b/); }); }); diff --git a/__tests__/status-human.test.ts b/__tests__/status-human.test.ts index 90c9cea..4ef5421 100644 --- a/__tests__/status-human.test.ts +++ b/__tests__/status-human.test.ts @@ -84,16 +84,16 @@ describe('codegraph status — human-readable prose (#193)', () => { await cg.indexAll(); cg.close(); - // Simulate an index built by the previous engine (constant 24). The bumped - // binary (constant 25) must surface the mismatch in the human-readable + // Simulate an index built by the previous engine (constant 25). The bumped + // binary (constant 26) must surface the mismatch in the human-readable // prose as a re-index recommendation, NOT as a healthy "No source changes // detected" line. // eslint-disable-next-line @typescript-eslint/no-require-imports const { DatabaseSync } = require('node:sqlite'); const db = new DatabaseSync(path.join(tempDir, codeGraphDirName(), 'codegraph.db')); db.prepare( - "INSERT INTO project_metadata (key, value, updated_at) VALUES ('indexed_with_extraction_version', '24', 0) " + - "ON CONFLICT(key) DO UPDATE SET value = '24'" + "INSERT INTO project_metadata (key, value, updated_at) VALUES ('indexed_with_extraction_version', '25', 0) " + + "ON CONFLICT(key) DO UPDATE SET value = '25'" ).run(); db.close(); @@ -113,8 +113,8 @@ describe('codegraph status — human-readable prose (#193)', () => { const { DatabaseSync } = require('node:sqlite'); const db = new DatabaseSync(path.join(tempDir, codeGraphDirName(), 'codegraph.db')); db.prepare( - "INSERT INTO project_metadata (key, value, updated_at) VALUES ('indexed_with_extraction_version', '24', 0) " + - "ON CONFLICT(key) DO UPDATE SET value = '24'" + "INSERT INTO project_metadata (key, value, updated_at) VALUES ('indexed_with_extraction_version', '25', 0) " + + "ON CONFLICT(key) DO UPDATE SET value = '25'" ).run(); db.close(); @@ -123,8 +123,8 @@ describe('codegraph status — human-readable prose (#193)', () => { expect(index.reindexRecommended).toBe(true); expect(Array.isArray(index.reindexReasons)).toBe(true); expect((index.reindexReasons as string[])).toContain('extraction-version'); - expect(index.builtWithExtractionVersion).toBe(24); - expect(index.currentExtractionVersion).toBe(25); + expect(index.builtWithExtractionVersion).toBe(25); + expect(index.currentExtractionVersion).toBe(26); }); it('a fresh full index keeps the JSON reindexReasons empty and reindexRecommended=false (#193 must not touch the JSON contract)', async () => { diff --git a/__tests__/status-json.test.ts b/__tests__/status-json.test.ts index dc3c271..5127089 100644 --- a/__tests__/status-json.test.ts +++ b/__tests__/status-json.test.ts @@ -184,28 +184,28 @@ describe('reindexReasons in codegraph status --json (#189)', () => { expect(index.reindexReasons as unknown[]).toEqual([]); }); - it('reports currentExtractionVersion=25, reindexRecommended=true, and reindexReasons=[extraction-version] for an index stamped with the previous extraction-version constant (24)', async () => { + it('reports currentExtractionVersion=26, reindexRecommended=true, and reindexReasons=[extraction-version] for an index stamped with the previous extraction-version constant (25)', async () => { fs.writeFileSync(path.join(tempDir, 'a.ts'), 'export const x = 1;\n'); const cg = CodeGraph.initSync(tempDir); await cg.indexAll(); cg.close(); - // Simulate an index built by the previous engine (constant 24). The bumped - // binary (constant 25) must surface the mismatch as `extraction-version` in + // Simulate an index built by the previous engine (constant 25). The bumped + // binary (constant 26) must surface the mismatch as `extraction-version` in // the reindexReasons array, alongside the boolean reindexRecommended=true. // eslint-disable-next-line @typescript-eslint/no-require-imports const { DatabaseSync } = require('node:sqlite'); const db = new DatabaseSync(path.join(tempDir, codeGraphDirName(), 'codegraph.db')); db.prepare( - "INSERT INTO project_metadata (key, value, updated_at) VALUES ('indexed_with_extraction_version', '24', 0) " + - "ON CONFLICT(key) DO UPDATE SET value = '24'" + "INSERT INTO project_metadata (key, value, updated_at) VALUES ('indexed_with_extraction_version', '25', 0) " + + "ON CONFLICT(key) DO UPDATE SET value = '25'" ).run(); db.close(); const out = runStatusJson(tempDir); const index = out.index as Record; - expect(index.currentExtractionVersion).toBe(25); - expect(index.builtWithExtractionVersion).toBe(24); + expect(index.currentExtractionVersion).toBe(26); + expect(index.builtWithExtractionVersion).toBe(25); expect(index.reindexRecommended).toBe(true); expect(Array.isArray(index.reindexReasons)).toBe(true); expect((index.reindexReasons as string[])).toContain('extraction-version'); @@ -251,6 +251,6 @@ describe('extraction-version bump smoke probe (#189)', () => { // eslint-disable-next-line @typescript-eslint/no-require-imports const { EXTRACTION_VERSION } = require('../dist/extraction/extraction-version'); expect(stamped).toBe(EXTRACTION_VERSION); - expect(EXTRACTION_VERSION).toBe(25); + expect(EXTRACTION_VERSION).toBe(26); }); }); diff --git a/docs/vba-error-handling-plan.md b/docs/vba-error-handling-plan.md index f76a7da..fcbf9bb 100644 --- a/docs/vba-error-handling-plan.md +++ b/docs/vba-error-handling-plan.md @@ -21,7 +21,7 @@ Filed against `ardelperal/codegraph-vba` on 2026-09-01. Epic: **#264**. | E3 mark edges inside a handler | #260 | landed | | E4 recognise the error channel | #261 | approved — needs #251 | | E5 publish the queries | #262 | approved | -| E6 label nodes + `handles-error` edges | #263 | approved — full design in the issue: `label` kind, `handles-error` kind, +30% nodes budgeted, no call re-parenting | +| E6 label nodes + `handles-error` edges | #263 | landed — `label` kind, `handles-error` kind, measured +15.0% nodes / +26.9% edges, no call re-parenting | --- @@ -635,17 +635,44 @@ error handling, and where errors end up being shown to the user." --- -### E6 — `label` nodes and `handles-error` edges — **blocked, do not implement** +### E6 — `label` nodes and `handles-error` edges — **unblocked, landed (#263)** -Held open deliberately. This is the design E2 rejected, and it stays rejected until a query -appears that `inErrorHandler` genuinely cannot serve (§4.3 sets the bar). +This was held blocked while the cheaper model was built, and §4.3's three conditions were met +before it moved: a written statement of the query that forces it (address a handler *as a thing* — a +stable id, `kind:label` search, dangling and duplicate detection as a graph query rather than +a scan — which `inErrorHandler`'s per-procedure boolean cannot serve), maintainer sign-off on +the new `NodeKind` and `EdgeKind` in the issue, and the node-budget forecast below. -If it is ever unblocked, it needs, in this order: a written statement of the query that forces -it; maintainer sign-off on the new `NodeKind` and `EdgeKind`; and a node-budget forecast -(≈3,900 nodes and ≈4,200 edges on this corpus, roughly doubling the symbol count). Ship it on -one project and measure before rolling it out. +**Do not read this section as licence to add a kind for anything else in E1–E5.** §4.1's +argument stands for every other error-handling fact: it is per-procedure, so it is a field. -**Do not implement E6 as part of E1–E5. Do not implement it because it seems more complete.** +**Measured on the corpus, not forecast** (`npx tsx scripts/vba-coverage-probe.mjs`): + +| | before | after | +|---|--:|--:| +| `label` nodes | 0 | 3,911 | +| `contains` edges | 16,159 | 20,070 | +| `handles-error` edges | 0 | 3,832 | +| `references` edges | 6,281 | 6,473 | +| unresolved references | 26,755 | 26,755 | +| **nodes, all kinds** | **26,089** | **30,000** | +| **edges, all kinds** | **29,521** | **37,456** | + ++15.0% nodes and +26.9% edges — under the issue's ≈+30% node forecast, because that forecast +used the older hand census (3,912 labels, ~450 plain `GoTo`) rather than the committed probe +(3,911 labels, 192 plain `GoTo` = `gotoStatements` 4,062 − `onErrorGoToLabel` 3,832 − +`onErrorGoToZero` 38). Where the two disagree the probe wins; the extractor matches the probe +exactly on all three counts. No other node or edge kind moved, and no new unresolved reference +appeared — every `GoTo` target in this corpus is defined in its own procedure. + +**What it deliberately does NOT do:** calls inside a handler stay attributed to the enclosing +procedure. Re-parenting them onto the label would change `callers` / `callees` for the 3,774 +procedures that have a handler. The label is *addressable*, not a container — E3's +`inErrorHandler` remains the answer to "did this come from the error path". + +`label` is deliberately absent from the context builder's default node filter and from the MCP +layer's container kinds, for the reason issue #257 kept `parameter` out of both: it is now the +most numerous VBA symbol in the graph. --- @@ -663,9 +690,12 @@ The main plan's three rules apply unchanged. Three more, particular to error han §3.1's query filters on size *and* on I/O — and why the changelog wording says "can be found", not "are flagged". -3. **This feature adds no nodes and no edges. If a PR in E1–E5 changes the node or edge count, +3. **E1–E5 add no nodes and no edges. If a PR in E1–E5 changes the node or edge count, something is wrong.** That invariant is cheap to assert and it is the single best protection - against this task quietly turning into E6. + against those tasks quietly turning into E6. E6 itself (#263) is the one sanctioned + exception, and it landed separately — after E1-E3, alongside E4, and before E5 — against + the measured budget in its own + section. **Per-PR checklist:** the main plan's checklist, plus: diff --git a/src/extraction/extraction-version.ts b/src/extraction/extraction-version.ts index 07ccbb9..8ddab13 100644 --- a/src/extraction/extraction-version.ts +++ b/src/extraction/extraction-version.ts @@ -21,4 +21,4 @@ * turns the re-index hint into noise — keep it honest (see CLAUDE.md, "Honesty * in the product is load-bearing"). */ -export const EXTRACTION_VERSION = 25; +export const EXTRACTION_VERSION = 26; diff --git a/src/extraction/index.ts b/src/extraction/index.ts index d90df3a..1372ae5 100644 --- a/src/extraction/index.ts +++ b/src/extraction/index.ts @@ -1616,6 +1616,9 @@ export class ExtractionOrchestrator { // compiled matcher is built inside the extractor because a RegExp // cannot cross the `structuredClone` worker boundary. sqlWrappers: vbaConfig.sqlWrappers, + // Issue #261: the project's error-channel variable names. Plain strings + // for the same worker-boundary reason as `sqlWrappers`. + errorChannel: vbaConfig.errorChannel, }; // Issue #154 — gate the 3 Dysflow-specific VBA sub-extractors. `true` // (the default) keeps the pre-refactor behavior; `false` opts out so @@ -2275,6 +2278,7 @@ export class ExtractionOrchestrator { targets: vbaReindexConfig.targets, maxRaiseFanout: vbaReindexConfig.maxRaiseFanout, sqlWrappers: vbaReindexConfig.sqlWrappers, + errorChannel: vbaReindexConfig.errorChannel, }, ); diff --git a/src/extraction/tree-sitter.ts b/src/extraction/tree-sitter.ts index 0744a11..d18798c 100644 --- a/src/extraction/tree-sitter.ts +++ b/src/extraction/tree-sitter.ts @@ -6557,6 +6557,7 @@ export function extractFromSource( targets: vbaOptions?.targets ?? vbaTargets, maxRaiseFanout: vbaOptions?.maxRaiseFanout ?? maxRaiseFanout, sqlWrappers: vbaOptions?.sqlWrappers, + errorChannel: vbaOptions?.errorChannel, }; const detectedLanguage = language || detectLanguage(filePath, source); const fileExtension = path.extname(filePath).toLowerCase(); diff --git a/src/extraction/vba-extractor.ts b/src/extraction/vba-extractor.ts index c422557..77d0924 100644 --- a/src/extraction/vba-extractor.ts +++ b/src/extraction/vba-extractor.ts @@ -46,6 +46,7 @@ import { } from './vba-preprocess'; import { VbaExtractorContext, VbaClassifier } from './vba/context'; import { compileSqlWrappers } from './vba/sql-wrapper'; +import { compileErrorChannel } from './vba/error-channel'; import type { VbaExtractionOptions } from './vba/options'; import type { VbaExtractionRule } from './vba/rules'; import { RULES as PROCEDURES_RULES, createProceduresClassifier } from './vba/procedures'; @@ -283,6 +284,10 @@ export class VbaExtractor { // and park it on the context. `scanSqlInLine` reads it on every line, so // this is the only place it may be built. this.ctx.sqlWrappers = compileSqlWrappers(options.sqlWrappers); + // Issue #261: same contract for the error channel — compiled once here, + // read per identifier by `module-vars.ts` and per statement by + // `errors.ts`. Both consumers share this one object so they cannot fork. + this.ctx.errorChannel = compileErrorChannel(options.errorChannel); } extract(): ExtractionResult { diff --git a/src/extraction/vba/context.ts b/src/extraction/vba/context.ts index 41425a1..e64e26d 100644 --- a/src/extraction/vba/context.ts +++ b/src/extraction/vba/context.ts @@ -24,6 +24,7 @@ import { } from '../sql-table-scan'; import { PRIMITIVE_TYPES } from './constants'; import type { CompiledSqlWrappers } from './sql-wrapper'; +import type { CompiledErrorChannel } from './error-channel'; export interface ProcInfo { name: string; @@ -171,6 +172,52 @@ export interface VbaErrorPolicyState { * exactly as the probe's `handlerLines[0] = first.rest` does. */ labelRestSignals: Map; + /** + * Issue #263 (task E6): every line-label DEFINITION in this body, in source + * order, with the name exactly as written and its source position. + * + * Additive to {@link definedLabels}, which the region resolver reads and + * which deliberately stores only `key → line`: the `label` node needs the + * original casing for its `name` and its position for `startColumn`, and + * duplicating those onto the existing map would change a structure three + * older code paths already depend on. One entry per DISTINCT label — a + * repeated definition is illegal VBA, and the first one wins here exactly + * as it does in `definedLabels`. + */ + labelDefs: VbaLabelSite[]; + /** + * Issue #263: every `On Error GoTo