diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e2e436..bf43d5b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ 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) ### Changed 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__/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/vba/context.ts b/src/extraction/vba/context.ts index 41425a1..0630d81 100644 --- a/src/extraction/vba/context.ts +++ b/src/extraction/vba/context.ts @@ -171,6 +171,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