From 9db911b08d0a1e3911264787cafdcdd562eef40b Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Mon, 6 Jul 2026 10:48:33 -0600 Subject: [PATCH] fix: classify destructured bindings as constant, not function (docs check acknowledged) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Object-pattern destructuring targets (const { a, b } = x; and renamed const { a: b } = x;) were hardcoded to kind: 'function' in both the TS/WASM extractor (extractDestructuredBindings, shared by the walk and query paths) and its native Rust mirror (extract_destructured_bindings), regardless of what the destructured value actually held. Non-function bindings (e.g. const { dbPath } = workerData) had no call-graph edges pointing at them by name, so the dead-code classifier risked flagging them dead-unresolved even when read repeatedly elsewhere in the file. Both engines now emit kind: 'constant', matching the existing convention for plain `const x = ` bindings and array-pattern destructuring (which was already correct). Every call site of these functions is const-gated, so 'constant' is unconditionally correct — no let/var case exists here. Call- target resolution is kind-agnostic and constant is already included in the top-level-binding caller-attribution fallback, so destructured callback-style bindings (e.g. `const { handler } = router; handler(req)`) still resolve correctly; verified via the resolution-benchmark suite showing identical precision/recall across all 40 fixture languages before and after. Array-pattern destructuring was checked and does not have this bug (already kind: 'constant' in both engines' TS path); a separate, pre-existing native vs WASM parity gap for array patterns was found and filed as #1901. Docs check acknowledged: kind-classification bug fix only, no new languages/features/architecture changes — README/ROADMAP tables unaffected. Fixes #1773 Impact: 2 functions changed, 7 affected --- .../src/extractors/javascript.rs | 48 +++++- src/extractors/javascript.ts | 27 ++- ...sue-1773-destructured-binding-kind.test.ts | 163 ++++++++++++++++++ tests/parsers/javascript.test.ts | 32 +++- 4 files changed, 256 insertions(+), 14 deletions(-) create mode 100644 tests/integration/issue-1773-destructured-binding-kind.test.ts diff --git a/crates/codegraph-core/src/extractors/javascript.rs b/crates/codegraph-core/src/extractors/javascript.rs index 2168a30d..2e17db27 100644 --- a/crates/codegraph-core/src/extractors/javascript.rs +++ b/crates/codegraph-core/src/extractors/javascript.rs @@ -2502,6 +2502,19 @@ fn handle_object_literal_shorthand_value_ref(node: &Node, source: &[u8], calls: }); } +/// Extract definitions from destructured object bindings: `const { handleToken, +/// checkPermissions } = initAuth(...)` creates definitions for `handleToken` +/// and `checkPermissions`, kind `constant` — matching the convention for plain +/// `const x = ` bindings and array-pattern destructuring. +/// +/// Every call site of this function is already gated to `const` declarations +/// (never `let`/`var`), so `constant` is unconditionally correct here. Prior to +/// #1773 this used `kind: "function"` on the theory that destructured names +/// are usually callbacks, but that miscategorized every non-function +/// destructured value (e.g. `const { dbPath } = workerData`). `constant`-kind +/// nodes remain fully resolvable as call targets — call-target resolution is +/// kind-agnostic — so callback-style destructured bindings still resolve. +/// Mirrors the TS extractor's `extractDestructuredBindings`. fn extract_destructured_bindings( pattern: &Node, source: &[u8], @@ -2515,7 +2528,7 @@ fn extract_destructured_bindings( "shorthand_property_identifier_pattern" | "shorthand_property_identifier" => { definitions.push(Definition { name: node_text(&child, source).to_string(), - kind: "function".to_string(), + kind: "constant".to_string(), line, end_line: Some(end_line), decorators: None, @@ -2531,7 +2544,7 @@ fn extract_destructured_bindings( { definitions.push(Definition { name: node_text(&value, source).to_string(), - kind: "function".to_string(), + kind: "constant".to_string(), line, end_line: Some(end_line), decorators: None, @@ -4527,12 +4540,33 @@ mod tests { #[test] fn extracts_destructured_const_bindings() { + // kind is "constant" (#1773), not "function" — matches the plain + // `const x = ` and array-pattern destructuring convention. + // Destructured names remain resolvable as call targets regardless of + // kind (call-target resolution is kind-agnostic), so callback-style + // destructured bindings like `handleToken` still resolve when called. let s = parse_js("const { handleToken, checkPermissions } = initAuth(config);"); let names: Vec<&str> = s.definitions.iter().map(|d| d.name.as_str()).collect(); assert!(names.contains(&"handleToken"), "should extract handleToken definition"); assert!(names.contains(&"checkPermissions"), "should extract checkPermissions definition"); let ht = s.definitions.iter().find(|d| d.name == "handleToken").unwrap(); - assert_eq!(ht.kind, "function"); + assert_eq!(ht.kind, "constant"); + } + + #[test] + fn extracts_non_renamed_destructured_bindings_with_kind_constant() { + // Regression guard for issue #1773: plain (non-renamed) destructured + // bindings from a non-call RHS (e.g. `workerData`) must not default to + // kind "function" — they hold arbitrary values, not callables. + let s = parse_js("const { dbPath, name, force } = workerData;"); + for expected in ["dbPath", "name", "force"] { + let def = s + .definitions + .iter() + .find(|d| d.name == expected) + .unwrap_or_else(|| panic!("should extract {expected} definition")); + assert_eq!(def.kind, "constant"); + } } #[test] @@ -4573,7 +4607,13 @@ mod tests { #[test] fn extracts_renamed_destructured_binding() { let s = parse_js("const { original: renamed } = initAuth();"); - assert!(s.definitions.iter().any(|d| d.name == "renamed"), "should use the local alias"); + let renamed = s + .definitions + .iter() + .find(|d| d.name == "renamed") + .expect("should use the local alias"); + // kind is "constant" (#1773) — see comment on extracts_destructured_const_bindings. + assert_eq!(renamed.kind, "constant"); assert!(!s.definitions.iter().any(|d| d.name == "original"), "should not use the original key"); } diff --git a/src/extractors/javascript.ts b/src/extractors/javascript.ts index f780290b..bf0a0e74 100644 --- a/src/extractors/javascript.ts +++ b/src/extractors/javascript.ts @@ -1112,7 +1112,23 @@ function handleTypeAliasDecl(node: TreeSitterNode, ctx: ExtractorOutput): void { /** * Extract definitions from destructured object bindings. * `const { handleToken, checkPermissions } = initAuth(...)` creates definitions - * for handleToken and checkPermissions so they can be resolved as call targets. + * for handleToken and checkPermissions, kind `constant` — matching the + * convention for plain `const x = ` bindings (handleConstIdentifierAssignment) + * and array-pattern destructuring (the sibling branch in the callers below). + * + * Every call site of this function is already gated to `const` declarations + * (never `let`/`var`), so `constant` is unconditionally correct here — there is + * no live binding-mutability to branch on. Prior to #1773 this used `kind: + * 'function'` on the theory that destructured names are usually callbacks, but + * that miscategorized every non-function destructured value (e.g. `const { + * dbPath } = workerData`), which polluted `--kind function` queries and caused + * the dead-code classifier to misjudge them via the wrong kind's heuristics. + * `constant`-kind nodes remain fully resolvable as call targets — call-target + * resolution (`resolveByGlobal`'s exact by-name lookup) is kind-agnostic, and + * `constant` is already in the caller-attribution fallback tier + * (`TOP_LEVEL_BINDING_KINDS` in call-resolver.ts) — so callback-style + * destructured bindings (`const { handleToken } = router; handleToken(req)`) + * still resolve correctly. */ function extractDestructuredBindings( pattern: TreeSitterNode, @@ -1128,7 +1144,7 @@ function extractDestructuredBindings( child.type === 'shorthand_property_identifier' ) { // { handleToken } — shorthand binding - definitions.push({ name: child.text, kind: 'function', line, endLine }); + definitions.push({ name: child.text, kind: 'constant', line, endLine }); } else if (child.type === 'pair_pattern' || child.type === 'pair') { // { original: renamed } — renamed binding, use the local alias const value = child.childForFieldName('value'); @@ -1136,7 +1152,7 @@ function extractDestructuredBindings( value && (value.type === 'identifier' || value.type === 'shorthand_property_identifier_pattern') ) { - definitions.push({ name: value.text, kind: 'function', line, endLine }); + definitions.push({ name: value.text, kind: 'constant', line, endLine }); } } } @@ -1259,8 +1275,9 @@ function handleConstObjectPatternAssignment( ctx: ExtractorOutput, ): void { // Destructured bindings: const { handleToken, checkPermissions } = initAuth(...) - // Each destructured property becomes a function definition so it can be - // resolved when passed as a callback (e.g. router.use(handleToken)). + // Each destructured property becomes a constant definition (#1773) — still + // resolvable when passed as a callback (e.g. router.use(handleToken)), since + // call-target resolution is kind-agnostic (see extractDestructuredBindings). // Restricted to const to avoid creating spurious definitions for // transient let/var destructuring (e.g. let { userId } = parseRequest(req)). // Scope guard mirrors extractDestructuredBindingsWalk (query path) and diff --git a/tests/integration/issue-1773-destructured-binding-kind.test.ts b/tests/integration/issue-1773-destructured-binding-kind.test.ts new file mode 100644 index 00000000..a0b7e0fb --- /dev/null +++ b/tests/integration/issue-1773-destructured-binding-kind.test.ts @@ -0,0 +1,163 @@ +/** + * Integration test for #1773: destructured object-pattern binding targets + * (renamed and non-renamed) were classified with `kind: "function"` instead + * of `kind: "constant"`, regardless of what the destructured value actually + * held. Because these bindings had no call-graph edges pointing at them by + * name, `codegraph roles --role dead` risked flagging them `dead-unresolved` + * (the "genuinely dead callable" bucket) even when read repeatedly elsewhere. + * + * Root cause: `extractDestructuredBindings` (src/extractors/javascript.ts, + * shared by both the walk and query extraction paths) and its native mirror + * `extract_destructured_bindings` (crates/codegraph-core/src/extractors/ + * javascript.rs) hardcoded `kind: 'function'` for every object-pattern + * binding target, on the theory that destructured names are usually + * callbacks. That miscategorized any destructured value that wasn't a + * function (e.g. `const { dbPath } = workerData`). + * + * Fix: both engines now emit `kind: 'constant'`, matching the existing + * convention for plain `const x = ` bindings and array-pattern + * destructuring. Constants remain fully resolvable as call targets (call- + * target resolution is kind-agnostic), so callback-style destructured + * bindings still resolve; and constants with active call-graph-connected + * siblings in the same file are classified `leaf`, not `dead-unresolved` — + * exactly like any other same-file constant. + */ + +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import Database from 'better-sqlite3'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { buildGraph } from '../../src/domain/graph/builder.js'; +import { isNativeAvailable } from '../../src/infrastructure/native.js'; + +// Repro 1 (renamed destructuring, issue #1773): a file with other real, +// call-graph-connected functions (giving it "active file siblings") plus a +// renamed destructured binding read repeatedly afterward — mirrors +// scripts/token-benchmark.ts's `const { values: flags } = parseArgs(...)`. +// Repro 2 (non-renamed destructuring, issue #1773): an isolated worker-style +// file with *only* a destructured binding from a non-call RHS and no other +// callables — mirrors tests/unit/snapshot-race-worker.mjs's +// `const { dbPath, name, force } = workerData`. +const FIXTURE = { + 'renamed-repro.js': ` +function parseArgs(opts) { return { values: computeDefaults(opts) }; } +function computeDefaults(opts) { return { runs: 3, model: opts.model }; } + +const { values: flags } = parseArgs({ model: 'x' }); + +function main() { + console.log(flags.runs, flags.model); +} +main(); +`, + 'worker-repro.mjs': ` +const { dbPath, name, force } = workerData; +save(name, { dbPath, force }); +`, +}; + +function readNode(dbPath: string, file: string, name: string) { + const db = new Database(dbPath, { readonly: true }); + try { + return db + .prepare('SELECT name, kind, role FROM nodes WHERE file = ? AND name = ?') + .get(file, name) as { name: string; kind: string; role: string | null } | undefined; + } finally { + db.close(); + } +} + +function expectFixedKindAndRole(dbPath: string) { + // Repro 1: renamed destructured binding, active file siblings present. + const flags = readNode(dbPath, 'renamed-repro.js', 'flags'); + expect(flags, 'flags node not found').toBeDefined(); + expect(flags!.kind, 'flags must be kind constant, not function').toBe('constant'); + expect( + flags!.role, + `flags was classified as ${flags!.role} — must not be dead-unresolved`, + ).not.toBe('dead-unresolved'); + + // Repro 2: non-renamed destructured bindings, no active file siblings. + for (const varName of ['dbPath', 'name', 'force']) { + const node = readNode(dbPath, 'worker-repro.mjs', varName); + expect(node, `${varName} node not found`).toBeDefined(); + expect(node!.kind, `${varName} must be kind constant, not function`).toBe('constant'); + // With no other call-graph-connected callable in the file, these fall + // back to 'dead-leaf' — the same honest classification any isolated, + // unreferenced-by-calls constant gets (properties/constants are leaf + // nodes by definition; call-graph reachability can't prove liveness for + // pure value bindings). The bug this test guards against is the far more + // misleading 'dead-unresolved' ("genuinely dead callable") label that a + // wrong kind: 'function' classification used to produce. + expect( + node!.role, + `${varName} was classified as ${node!.role} — must not be dead-unresolved`, + ).not.toBe('dead-unresolved'); + } +} + +describe('destructured binding kind classification (#1773) — WASM', () => { + let tmpDir: string; + + beforeAll(async () => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-1773-')); + for (const [rel, content] of Object.entries(FIXTURE)) { + fs.writeFileSync(path.join(tmpDir, rel), content); + } + await buildGraph(tmpDir, { engine: 'wasm', incremental: false, skipRegistry: true }); + }); + + afterAll(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + it('classifies renamed and non-renamed destructured bindings as kind constant, not dead-unresolved', () => { + expectFixedKindAndRole(path.join(tmpDir, '.codegraph', 'graph.db')); + }); + + it('still resolves calls made through a destructured callback-style binding', () => { + // `flags.runs`/`flags.model` are property reads, not calls, but `flags` + // itself must still show up as the attributed caller of `parseArgs` — the + // fix must not break caller attribution for the top-level binding. + const dbPath = path.join(tmpDir, '.codegraph', 'graph.db'); + const db = new Database(dbPath, { readonly: true }); + try { + const row = db + .prepare( + `SELECT COUNT(*) AS cnt + FROM edges e + JOIN nodes s ON e.source_id = s.id + JOIN nodes t ON e.target_id = t.id + WHERE s.name = 'flags' AND t.name = 'parseArgs' AND e.kind = 'calls'`, + ) + .get() as { cnt: number }; + expect(row.cnt, 'expected flags -> parseArgs calls edge to survive the kind fix').toBe(1); + } finally { + db.close(); + } + }); +}); + +describe.skipIf(!isNativeAvailable())( + 'destructured binding kind classification (#1773) — native', + () => { + let tmpDir: string; + + beforeAll(async () => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-1773-native-')); + for (const [rel, content] of Object.entries(FIXTURE)) { + fs.writeFileSync(path.join(tmpDir, rel), content); + } + await buildGraph(tmpDir, { engine: 'native', incremental: false, skipRegistry: true }); + }, 60_000); + + afterAll(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + it('classifies renamed and non-renamed destructured bindings as kind constant, not dead-unresolved', () => { + expectFixedKindAndRole(path.join(tmpDir, '.codegraph', 'graph.db')); + }); + }, +); diff --git a/tests/parsers/javascript.test.ts b/tests/parsers/javascript.test.ts index 79174055..0d45c26b 100644 --- a/tests/parsers/javascript.test.ts +++ b/tests/parsers/javascript.test.ts @@ -981,22 +981,43 @@ describe('JavaScript parser', () => { // Destructured bindings it('extracts definitions from destructured const bindings', () => { + // kind is 'constant' (#1773), not 'function' — matches the plain + // `const x = ` and array-pattern destructuring convention. + // Destructured names remain resolvable as call targets regardless of + // kind (call-target resolution is kind-agnostic), so callback-style + // destructured bindings like `handleToken` still resolve when called. const symbols = parseJS(`const { handleToken, checkPermissions } = initAuth(config);`); expect(symbols.definitions).toContainEqual( - expect.objectContaining({ name: 'handleToken', kind: 'function' }), + expect.objectContaining({ name: 'handleToken', kind: 'constant' }), ); expect(symbols.definitions).toContainEqual( - expect.objectContaining({ name: 'checkPermissions', kind: 'function' }), + expect.objectContaining({ name: 'checkPermissions', kind: 'constant' }), ); }); it('extracts definitions from exported destructured const bindings', () => { const symbols = parseJS(`export const { handleToken, checkPermissions } = initAuth(config);`); expect(symbols.definitions).toContainEqual( - expect.objectContaining({ name: 'handleToken', kind: 'function' }), + expect.objectContaining({ name: 'handleToken', kind: 'constant' }), ); expect(symbols.definitions).toContainEqual( - expect.objectContaining({ name: 'checkPermissions', kind: 'function' }), + expect.objectContaining({ name: 'checkPermissions', kind: 'constant' }), + ); + }); + + it('extracts non-renamed destructured const bindings with kind constant (#1773)', () => { + // Regression guard for issue #1773: plain (non-renamed) destructured + // bindings from a non-call RHS (e.g. `workerData`) must not default to + // kind 'function' — they hold arbitrary values, not callables. + const symbols = parseJS(`const { dbPath, name, force } = workerData;`); + expect(symbols.definitions).toContainEqual( + expect.objectContaining({ name: 'dbPath', kind: 'constant' }), + ); + expect(symbols.definitions).toContainEqual( + expect.objectContaining({ name: 'name', kind: 'constant' }), + ); + expect(symbols.definitions).toContainEqual( + expect.objectContaining({ name: 'force', kind: 'constant' }), ); }); @@ -1013,9 +1034,10 @@ describe('JavaScript parser', () => { }); it('extracts renamed destructured const binding under its local alias', () => { + // kind is 'constant' (#1773) — see comment on the non-renamed case above. const symbols = parseJS(`const { original: renamed } = initAuth();`); expect(symbols.definitions).toContainEqual( - expect.objectContaining({ name: 'renamed', kind: 'function' }), + expect.objectContaining({ name: 'renamed', kind: 'constant' }), ); expect(symbols.definitions).not.toContainEqual(expect.objectContaining({ name: 'original' })); });