From 2b6a00d2a80d4f03e3080dd55c4f3fb7b8a086e3 Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Sun, 5 Jul 2026 11:24:15 -0600 Subject: [PATCH 1/8] fix: scope codegraph batch complexity targets to file paths BATCH_COMMANDS entries with sig: 'dbOnly' (currently only complexity) always wrote their target into opts.target. complexityData treats opts.target as a symbol-name filter and opts.file as the file-path filter, so batch complexity never matched anything and the function list silently fell back to empty with a whole-repo summary regardless of which file was requested. Add an optional targetKey on BatchCommandEntry so each dbOnly command can declare which opts key its targets map to, and set it to 'file' for complexity. batchData and multiBatchData now route the target through the declared key instead of assuming opts.target. Fixes #1721 Impact: 4 functions changed, 6 affected --- src/features/batch.ts | 28 ++++++++++++++-- tests/integration/batch.test.ts | 57 ++++++++++++++++++++++++++++++--- 2 files changed, 77 insertions(+), 8 deletions(-) diff --git a/src/features/batch.ts b/src/features/batch.ts index 12a6aefb0..d8f343e34 100644 --- a/src/features/batch.ts +++ b/src/features/batch.ts @@ -18,6 +18,15 @@ type BatchSig = 'name' | 'target' | 'file' | 'dbOnly'; interface BatchCommandEntry { fn: (...args: unknown[]) => unknown; sig: BatchSig; + /** + * For `sig: 'dbOnly'` commands only: the `opts` key each target should be + * written to before calling `fn(customDbPath, opts)`. Defaults to `'target'` + * (a symbol/name filter). Commands whose underlying query function expects a + * *file path* target (rather than a symbol name) — e.g. `complexityData`, + * which treats `opts.target` as a name filter and `opts.file` as the file + * scope — must override this so the batch target lands in the right filter. + */ + targetKey?: string; } export const BATCH_COMMANDS: Record = { @@ -31,9 +40,22 @@ export const BATCH_COMMANDS: Record = { exports: { fn: exportsData as (...args: unknown[]) => unknown, sig: 'file' }, flow: { fn: flowData as (...args: unknown[]) => unknown, sig: 'name' }, dataflow: { fn: dataflowData as (...args: unknown[]) => unknown, sig: 'name' }, - complexity: { fn: complexityData as (...args: unknown[]) => unknown, sig: 'dbOnly' }, + complexity: { + fn: complexityData as (...args: unknown[]) => unknown, + sig: 'dbOnly', + targetKey: 'file', + }, }; +/** Resolve the opts key a `dbOnly` command's target should be written to. */ +function dbOnlyTargetOpts( + entry: BatchCommandEntry, + opts: Record, + target: string, +): Record { + return { ...opts, [entry.targetKey ?? 'target']: target }; +} + interface BatchResultOk { target: string; ok: true; @@ -77,7 +99,7 @@ export function batchData( try { let data: unknown; if (entry.sig === 'dbOnly') { - data = entry.fn(customDbPath, { ...opts, target }); + data = entry.fn(customDbPath, dbOnlyTargetOpts(entry, opts, target)); } else { data = entry.fn(target, customDbPath, opts); } @@ -166,7 +188,7 @@ export function multiBatchData( try { let data: unknown; if (entry.sig === 'dbOnly') { - data = entry.fn(customDbPath, { ...merged, target }); + data = entry.fn(customDbPath, dbOnlyTargetOpts(entry, merged, target)); } else { data = entry.fn(target, customDbPath, merged); } diff --git a/tests/integration/batch.test.ts b/tests/integration/batch.test.ts index 0dc44e3b0..8d84ea344 100644 --- a/tests/integration/batch.test.ts +++ b/tests/integration/batch.test.ts @@ -39,6 +39,12 @@ function insertEdge(db, sourceId, targetId, kind = 'calls', confidence = 1.0) { ).run(sourceId, targetId, kind, confidence); } +function insertComplexity(db, nodeId, cognitive, cyclomatic, maxNesting = 0) { + db.prepare( + 'INSERT INTO function_complexity (node_id, cognitive, cyclomatic, max_nesting) VALUES (?, ?, ?, ?)', + ).run(nodeId, cognitive, cyclomatic, maxNesting); +} + // ─── Fixture DB ──────────────────────────────────────────────────────── let tmpDir: string, dbPath: string; @@ -71,6 +77,13 @@ beforeAll(() => { insertEdge(db, fnRoute, fnAuth); insertEdge(db, fnRoute, fnFormat); + // Complexity data, distinct per file, so batch complexity can be asserted + // to scope results to the requested file rather than the whole repo. + insertComplexity(db, fnAuth, 5, 2); + insertComplexity(db, fnValidate, 3, 1); + insertComplexity(db, fnRoute, 10, 4); + insertComplexity(db, fnFormat, 1, 1); + db.close(); }); @@ -197,12 +210,46 @@ describe('batchData — edge cases', () => { // ─── complexity (dbOnly sig) ───────────────────────────────────────── describe('batchData — complexity (dbOnly signature)', () => { - test('complexity command uses target as opts.target', () => { - const data = batchData('complexity', ['authenticate'], dbPath); - expect(data.total).toBe(1); - // complexityData returns functions array — it won't error for unknown targets + test('complexity command scopes each target to a file, not a symbol name (#1721)', () => { + // Regression test for #1721: batch previously forwarded the file-path + // target as opts.target (a symbol-name filter), so complexityData never + // matched anything and silently fell back to identical, unfiltered + // whole-repo results for every target. It must land in opts.file instead. + const data = batchData('complexity', ['src/auth.js', 'src/routes.js'], dbPath); + expect(data.total).toBe(2); + expect(data.succeeded).toBe(2); + + const auth = data.results.find((r) => r.target === 'src/auth.js'); + const routes = data.results.find((r) => r.target === 'src/routes.js'); + expect(auth.ok).toBe(true); + expect(routes.ok).toBe(true); + + // src/auth.js has authenticate (cognitive 5) + validateToken (cognitive 3) + const authNames = auth.data.functions.map((f) => f.name).sort(); + expect(authNames).toEqual(['authenticate', 'validateToken']); + + // src/routes.js has only handleRoute (cognitive 10) + const routeNames = routes.data.functions.map((f) => f.name); + expect(routeNames).toEqual(['handleRoute']); + + // The two targets must not collapse into identical, unfiltered results. + // (Note: `summary` is a whole-repo health metric independent of the file + // filter, by design of complexityData — see #1807 for a separate, + // pre-existing gap where it should but doesn't reflect the file scope. + // The bug this test guards against is that `functions` came back empty + // for every target because the file path was routed into a symbol-name + // filter instead of the file filter.) + expect(auth.data.functions).not.toEqual(routes.data.functions); + }); + + test('complexity command still accepts a bare symbol name via opts.target passthrough', () => { + // BATCH_COMMANDS commands other than complexity keep symbol-name targets; + // this guards that complexity's own --target-style filtering (via shared + // opts, not the batch target) continues to work for direct callers. + const data = batchData('complexity', ['src/auth.js'], dbPath, { target: 'valid' }); expect(data.results[0].ok).toBe(true); - expect(data.results[0].data).toHaveProperty('functions'); + const names = data.results[0].data.functions.map((f) => f.name); + expect(names).toEqual(['validateToken']); }); }); From b28051ab3b220a745a6bf7b012b68fbe948716be Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Sun, 5 Jul 2026 13:06:28 -0600 Subject: [PATCH 2/8] fix: exclude parameters and interface/type members from dead-role classification codegraph roles --role dead flagged every function parameter as dead-leaf and every interface/type member as dead-unresolved, regardless of actual usage. Root cause: the role classifier's fan-in-based "no callers = dead" heuristic is meaningless for these two symbol categories, since neither can ever have inbound call edges by construction: - Parameters: `kind = 'parameter'` nodes were unconditionally force-assigned dead-leaf via a fast-path bypass in classifyNodeRolesFull/Incremental (TS) and do_classify_full/do_classify_incremental (native), before any fan-in was even computed. A parameter's liveness is a local dataflow question (is it referenced within its own function body), not a call-graph reachability question, so this produced ~90% noise in --role dead output. - Interface/type members: every language extractor qualifies interface/type members as `Owner.member` top-level definitions (mirroring class method qualification), and they never receive inbound call edges (they're consumed via type annotations, not calls). Lacking any recognition of this, they fell through the normal fan-in/fan-out path and landed in dead-unresolved. Fix: - Parameters are now fully excluded from role classification (role stays NULL), the same treatment already given to file/directory nodes. - Interface/type members are now recognized in the classifier by resolving the Owner.member prefix against same-file TYPE_DEF_KINDS declarations (interface/type/struct/enum/trait/record) and classified `leaf` unconditionally. Class methods use the identical Owner.member naming convention but are unaffected since `class` is not in TYPE_DEF_KINDS, so real dead methods/functions remain detected. Applied to both the TS/WASM classifier (graph/classifiers/roles.ts, features/structure.ts) and the native Rust classifier (graph/classifiers/roles.rs) per the dual-engine parity requirement; verified both engines produce identical results end-to-end against this repo's own graph. Fixes #1723 Impact: 6 functions changed, 5 affected --- .../src/graph/classifiers/roles.rs | 88 ++++++++++++-- src/features/structure.ts | 17 ++- src/graph/classifiers/roles.ts | 85 ++++++++++++- tests/graph/classifiers/roles.test.ts | 113 ++++++++++++++++++ tests/unit/roles.test.ts | 69 +++++++++++ 5 files changed, 356 insertions(+), 16 deletions(-) diff --git a/crates/codegraph-core/src/graph/classifiers/roles.rs b/crates/codegraph-core/src/graph/classifiers/roles.rs index c45184650..1b45f03bc 100644 --- a/crates/codegraph-core/src/graph/classifiers/roles.rs +++ b/crates/codegraph-core/src/graph/classifiers/roles.rs @@ -106,6 +106,55 @@ fn median(sorted: &[u32]) -> f64 { } } +/// Compute, per file, the set of symbol names that are `TYPE_DEF_KINDS`-kind +/// declarations (interface/type/struct/enum/trait/record). Mirrors JS +/// `computeTypeDefNamesByFile` in `graph/classifiers/roles.ts`. +fn compute_type_def_names_by_file( + rows: &[(i64, String, String, String, u32, u32)], +) -> HashMap> { + let mut by_file: HashMap> = HashMap::new(); + for (_id, name, kind, file, _fan_in, _fan_out) in rows { + if TYPE_DEF_KINDS.iter().any(|k| *k == kind.as_str()) { + by_file + .entry(file.clone()) + .or_default() + .insert(name.clone()); + } + } + by_file +} + +/// True when `name`/`kind` is a `method`/`property` member of an interface/type +/// declared in the same file — e.g. TS `interface Foo { bar: string }` extracts +/// `bar` as a top-level `method`-kind definition named `Foo.bar` (#1723). Every +/// language extractor qualifies interface/type members as `Owner.member` +/// (mirroring class method qualification), so the owner name is recovered from +/// the prefix before the first `.` and looked up against same-file +/// `TYPE_DEF_KINDS` declarations. Class methods use the identical `Owner.member` +/// convention but are unaffected here because `class` is not in `TYPE_DEF_KINDS` +/// — they remain subject to normal dead-code detection. +/// +/// These members can never gain inbound call edges by construction, so a +/// `fan_in == 0` reading carries zero dead-code signal for them, unlike a real +/// function/method where it does. Mirrors JS `isTypeDeclarationMember`. +fn is_type_declaration_member( + name: &str, + kind: &str, + file: &str, + type_def_names_by_file: &HashMap>, +) -> bool { + if kind != "method" && kind != "property" { + return false; + } + let Some(dot_idx) = name.find('.') else { + return false; + }; + let owner_name = &name[..dot_idx]; + type_def_names_by_file + .get(file) + .is_some_and(|names| names.contains(owner_name)) +} + /// Dead sub-role classification matching JS `classifyDeadSubRole`. fn classify_dead_sub_role(_name: &str, kind: &str, file: &str) -> &'static str { // Leaf kinds @@ -126,6 +175,7 @@ fn classify_dead_sub_role(_name: &str, kind: &str, file: &str) -> &'static str { } /// Classify a single node into a role. +#[allow(clippy::too_many_arguments)] fn classify_node( name: &str, kind: &str, @@ -135,9 +185,16 @@ fn classify_node( is_exported: bool, production_fan_in: u32, has_active_file_siblings: bool, + is_type_member: bool, median_fan_in: f64, median_fan_out: f64, ) -> &'static str { + // Interface/type members (#1723) — never subject to call-graph dead-code + // detection, regardless of fan-in/fan-out/export status. + if is_type_member { + return "leaf"; + } + // Framework entry if FRAMEWORK_ENTRY_PREFIXES.iter().any(|p| name.starts_with(p)) { return "entry"; @@ -267,10 +324,14 @@ pub(crate) fn do_classify_full(conn: &Connection) -> rusqlite::Result dead-leaf + // 1. Property kind (class/struct fields) -> dead-leaf. + // `parameter` is deliberately NOT included here (#1723): a parameter's liveness + // is a local dataflow question, not a call-graph reachability question, so + // "no incoming call edges" carries zero dead-code signal for it. Parameters are + // also excluded from the main rows query below, so they never receive a role + // at all — the same treatment as `file`/`directory` nodes. let leaf_ids: Vec = { - let mut stmt = - tx.prepare("SELECT id FROM nodes WHERE kind IN ('parameter', 'property')")?; + let mut stmt = tx.prepare("SELECT id FROM nodes WHERE kind = 'property'")?; let rows = stmt.query_map([], |row| row.get::<_, i64>(0))?; rows.filter_map(|r| r.ok()).collect() }; @@ -408,6 +469,11 @@ pub(crate) fn do_classify_full(conn: &Connection) -> rusqlite::Result> = HashMap::new(); @@ -423,6 +489,7 @@ pub(crate) fn do_classify_full(conn: &Connection) -> rusqlite::Result, prod_fan_in: &HashMap, active_files: &std::collections::HashSet, called_active_files: &std::collections::HashSet, + type_def_names_by_file: &HashMap>, median_fan_in: f64, median_fan_out: f64, ids_by_role: &mut HashMap<&'static str, Vec>, @@ -573,6 +642,7 @@ fn classify_rows( } else { false }; + let is_type_member = is_type_declaration_member(name, kind, file, type_def_names_by_file); let role = classify_node( name, kind, @@ -582,6 +652,7 @@ fn classify_rows( is_exported, prod_fi, has_active_siblings, + is_type_member, median_fan_in, median_fan_out, ); @@ -629,16 +700,15 @@ fn find_neighbour_files( } /// Query leaf kind node IDs and callable node rows for a set of files. +/// `parameter` is intentionally excluded from the leaf query (#1723) — see +/// `do_classify_full`'s leaf_ids comment for the rationale. fn query_nodes_for_files( tx: &rusqlite::Transaction, files: &[&str], ) -> rusqlite::Result<(Vec, Vec<(i64, String, String, String, u32, u32)>)> { let ph: String = files.iter().map(|_| "?").collect::>().join(","); - let leaf_sql = format!( - "SELECT id FROM nodes WHERE kind IN ('parameter', 'property') AND file IN ({})", - ph - ); + let leaf_sql = format!("SELECT id FROM nodes WHERE kind = 'property' AND file IN ({})", ph); let leaf_ids: Vec = { let mut stmt = tx.prepare(&leaf_sql)?; for (i, f) in files.iter().enumerate() { @@ -804,6 +874,9 @@ pub(crate) fn do_classify_incremental( let (active_files, called_active_files) = compute_active_files(&rows); + // Compute interface/type owner names per file (#1723) — see do_classify_full. + let type_def_names_by_file = compute_type_def_names_by_file(&rows); + let mut ids_by_role: HashMap<&str, Vec> = HashMap::new(); if !leaf_ids.is_empty() { @@ -818,6 +891,7 @@ pub(crate) fn do_classify_incremental( &prod_fan_in, &active_files, &called_active_files, + &type_def_names_by_file, median_fan_in, median_fan_out, &mut ids_by_role, diff --git a/src/features/structure.ts b/src/features/structure.ts index 74b88ef4a..85f3afff9 100644 --- a/src/features/structure.ts +++ b/src/features/structure.ts @@ -777,13 +777,20 @@ function writeMedianCache( } function classifyNodeRolesFull(db: BetterSqlite3Database, emptySummary: RoleSummary): RoleSummary { - // Leaf kinds (parameter, property) can never have callers/callees. + // Property kind (class/struct fields) can never have callers/callees. // Classify them directly as dead-leaf without the expensive fan-in/fan-out JOINs. + // + // `parameter` is deliberately NOT included here (#1723): a parameter's liveness + // is a local dataflow question (is it referenced within its own function body), + // not a call-graph reachability question, so "no incoming call edges" carries + // zero dead-code signal for it. Parameters are also excluded from the main + // query below, so they never receive a role at all — the same treatment as + // `file`/`directory` nodes. const leafRows = db .prepare( `SELECT n.id FROM nodes n - WHERE n.kind IN ('parameter', 'property')`, + WHERE n.kind = 'property'`, ) .all() as { id: number }[]; @@ -967,11 +974,13 @@ function classifyNodeRolesIncremental( globalMedians = computed; } - // 2a. Leaf kinds (parameter, property) in affected files — always dead-leaf + // 2a. Property kind (class/struct fields) in affected files — always dead-leaf. + // `parameter` is intentionally excluded (#1723) — see classifyNodeRolesFull for + // the rationale. Parameters stay unclassified (role = NULL) after the reset below. const leafRows = db .prepare( `SELECT n.id FROM nodes n - WHERE n.kind IN ('parameter', 'property') + WHERE n.kind = 'property' AND n.file IN (${placeholders})`, ) .all(...allAffectedFiles) as { id: number }[]; diff --git a/src/graph/classifiers/roles.ts b/src/graph/classifiers/roles.ts index 2b17c33ba..fe278ff31 100644 --- a/src/graph/classifiers/roles.ts +++ b/src/graph/classifiers/roles.ts @@ -4,10 +4,23 @@ * Roles: entry, core, utility, adapter, leaf, dead-*, test-only * * Dead sub-categories refine the coarse "dead" bucket: - * dead-leaf — parameters, properties, constants (leaf nodes by definition) + * dead-leaf — properties, constants (leaf nodes by definition) * dead-entry — framework dispatch: CLI commands, MCP tools, event handlers * dead-ffi — cross-language FFI boundaries (e.g. Rust napi-rs bindings) * dead-unresolved — genuinely unreferenced callables (the real dead code) + * + * `parameter`-kind nodes never reach this module in production — callers + * (`features/structure.ts`, native `graph/classifiers/roles.rs`) exclude them + * entirely, leaving `role` unset, the same treatment as `file`/`directory` + * nodes. A parameter's liveness is a local dataflow question (is it referenced + * within its own function body), not a call-graph reachability question, so + * "no incoming call edges" carries zero dead-code signal for it (#1723). + * + * `method`/`property`-kind members of an interface/type declaration (e.g. + * `interface Foo { bar: string }`) DO reach this module, but are recognized by + * `isTypeDeclarationMember` and classified `leaf` unconditionally — they can + * never gain inbound call edges by construction, so call-graph reachability + * doesn't apply to them either (#1723). */ import type { DeadSubRole, Role } from '../../types.js'; @@ -53,6 +66,56 @@ export interface ClassifiableNode { file?: string; } +/** + * Compute, per file, the set of symbol names that are `TYPE_DEF_KINDS`-kind + * declarations (interface/type/struct/enum/trait/record). Used by + * `isTypeDeclarationMember` to recognize `Owner.member`-qualified nodes whose + * owner is a type-level declaration rather than a class. + */ +function computeTypeDefNamesByFile(nodes: RoleClassificationNode[]): Map> { + const byFile = new Map>(); + for (const n of nodes) { + if (n.file && n.kind && TYPE_DEF_KINDS.has(n.kind)) { + let names = byFile.get(n.file); + if (!names) { + names = new Set(); + byFile.set(n.file, names); + } + names.add(n.name); + } + } + return byFile; +} + +/** + * True when `node` is a `method`/`property`-kind member of an interface/type + * declared in the same file — e.g. TS `interface Foo { bar: string }` extracts + * `bar` as a top-level `method`-kind definition named `Foo.bar` (#1723). Every + * language extractor qualifies interface/type members as `Owner.member` + * (mirroring class method qualification), so the owner name is recovered from + * the prefix before the first `.` and looked up against same-file + * `TYPE_DEF_KINDS` declarations. Class methods use the identical `Owner.member` + * convention but are unaffected here because `class` is not in `TYPE_DEF_KINDS` + * — they remain subject to normal dead-code detection. + * + * These members can never gain inbound call edges by construction — they are + * consumed via type annotations and structural typing, never calls — so a + * `fanIn === 0` reading carries zero dead-code signal for them, unlike a real + * function/method where it does. Call-edge-based reachability just doesn't + * apply to type-level declarations, so they must never be judged dead by it. + */ +function isTypeDeclarationMember( + node: RoleClassificationNode, + typeDefNamesByFile: Map>, +): boolean { + if (node.kind !== 'method' && node.kind !== 'property') return false; + if (!node.file) return false; + const dotIdx = node.name.indexOf('.'); + if (dotIdx === -1) return false; + const ownerName = node.name.slice(0, dotIdx); + return typeDefNamesByFile.get(node.file)?.has(ownerName) ?? false; +} + /** * Refine a "dead" classification into a sub-category. */ @@ -176,10 +239,21 @@ function classifyByFanShape(highIn: boolean, highOut: boolean): Role { /** * Apply role-classification rules to a single node. - * Order matters — framework entries are tagged first, then dead/test cases, - * then the fan-in/fan-out shape decides among the structural roles. + * Order matters — type-level members are ruled out first (they can never be + * judged by call-graph reachability at all), then framework entries, then + * dead/test cases, then the fan-in/fan-out shape decides among the structural + * roles. */ -function classifyNodeRole(node: RoleClassificationNode, medFanIn: number, medFanOut: number): Role { +function classifyNodeRole( + node: RoleClassificationNode, + medFanIn: number, + medFanOut: number, + typeDefNamesByFile: Map>, +): Role { + // Interface/type members (#1723) — never subject to call-graph dead-code + // detection, regardless of fan-in/fan-out/export status. + if (isTypeDeclarationMember(node, typeDefNamesByFile)) return 'leaf'; + if (FRAMEWORK_ENTRY_PREFIXES.some((p) => node.name.startsWith(p))) return 'entry'; if (node.fanIn === 0) { @@ -217,10 +291,11 @@ export function classifyRoles( if (nodes.length === 0) return new Map(); const { fanIn: medFanIn, fanOut: medFanOut } = medianOverrides ?? computeFanMedians(nodes); + const typeDefNamesByFile = computeTypeDefNamesByFile(nodes); const result = new Map(); for (const node of nodes) { - result.set(node.id, classifyNodeRole(node, medFanIn, medFanOut)); + result.set(node.id, classifyNodeRole(node, medFanIn, medFanOut, typeDefNamesByFile)); } return result; } diff --git a/tests/graph/classifiers/roles.test.ts b/tests/graph/classifiers/roles.test.ts index 03ae894c7..b1de630ee 100644 --- a/tests/graph/classifiers/roles.test.ts +++ b/tests/graph/classifiers/roles.test.ts @@ -515,4 +515,117 @@ describe('classifyRoles', () => { const roles = classifyRoles(nodes); expect(roles.get('1')).toBe('dead-unresolved'); }); + + // ── Interface/type member exemption (#1723) ───────────────────────── + + it('classifies interface method-signature member as leaf, not dead', () => { + // TS `interface Foo { bar(): void }` extracts `bar` as a top-level + // `method`-kind definition named `Foo.bar`. It can never gain an inbound + // call edge (nothing "calls" a type-level declaration), so fanIn === 0 + // carries zero dead-code signal here — unlike a real function/method. + const nodes = [ + { + id: '1', + name: 'Foo', + kind: 'interface', + file: 'src/a.ts', + fanIn: 0, + fanOut: 0, + isExported: false, + }, + { + id: '2', + name: 'Foo.bar', + kind: 'method', + file: 'src/a.ts', + fanIn: 0, + fanOut: 0, + isExported: false, + }, + ]; + const roles = classifyRoles(nodes); + expect(roles.get('2')).toBe('leaf'); + }); + + it('classifies type-alias property-signature member as leaf, not dead', () => { + // TS `type Foo = { bar: string }` — a property-kind member of a `type` owner. + const nodes = [ + { + id: '1', + name: 'Foo', + kind: 'type', + file: 'src/a.ts', + fanIn: 0, + fanOut: 0, + isExported: false, + }, + { + id: '2', + name: 'Foo.bar', + kind: 'property', + file: 'src/a.ts', + fanIn: 0, + fanOut: 0, + isExported: false, + }, + ]; + const roles = classifyRoles(nodes); + expect(roles.get('2')).toBe('leaf'); + }); + + it('classifies interface member as leaf even when the interface is isolated (no active siblings)', () => { + // Unlike bare TYPE_DEF_KINDS nodes (which need hasActiveFileSiblings to + // avoid being marked dead), members are exempt unconditionally — "no call + // edges" is a structural certainty for them, not merely a likelihood. + const nodes = [ + { + id: '1', + name: 'Isolated', + kind: 'interface', + file: 'src/isolated.ts', + fanIn: 0, + fanOut: 0, + isExported: false, + }, + { + id: '2', + name: 'Isolated.onlyMember', + kind: 'method', + file: 'src/isolated.ts', + fanIn: 0, + fanOut: 0, + isExported: false, + }, + ]; + const roles = classifyRoles(nodes); + expect(roles.get('2')).toBe('leaf'); + }); + + it('does not exempt class methods sharing the Owner.member naming convention', () => { + // Class methods are qualified identically to interface members + // (`ClassName.method`), but `class` is not in TYPE_DEF_KINDS — they must + // remain subject to normal dead-code detection. + const nodes = [ + { + id: '1', + name: 'Foo', + kind: 'class', + file: 'src/a.ts', + fanIn: 5, + fanOut: 0, + isExported: true, + }, + { + id: '2', + name: 'Foo.deadMethod', + kind: 'method', + file: 'src/a.ts', + fanIn: 0, + fanOut: 0, + isExported: false, + }, + ]; + const roles = classifyRoles(nodes); + expect(roles.get('2')).toBe('dead-unresolved'); + }); }); diff --git a/tests/unit/roles.test.ts b/tests/unit/roles.test.ts index 110e1c7bd..88d5ee1e0 100644 --- a/tests/unit/roles.test.ts +++ b/tests/unit/roles.test.ts @@ -380,4 +380,73 @@ describe('classifyNodeRoles', () => { // Should be entry (exported, fan-in 0), not dead-unresolved expect(role.role).toBe('entry'); }); + + // ── Parameters and interface members are not dead-code targets (#1723) ── + + it('excludes parameter-kind nodes from role classification entirely', () => { + // A parameter's liveness is a local dataflow question (is it referenced + // within its own function body), not a call-graph reachability question. + // "No incoming call edges" is guaranteed for every parameter regardless of + // usage, so parameters must never receive a `dead-*` role — or any role at + // all, the same treatment as file/directory nodes. + insertNode('src/helpers.ts', 'file', 'src/helpers.ts', 0); + const fn = insertNode('findChild', 'function', 'src/helpers.ts', 26); + insertNode('node', 'parameter', 'src/helpers.ts', 26); + insertNode('type', 'parameter', 'src/helpers.ts', 26); + // An external caller so findChild itself is not incidentally dead too — + // isolates the assertion to parameter handling specifically. + const caller = insertNode('caller', 'function', 'other.ts', 1); + insertEdge(caller, fn, 'calls'); + + classifyNodeRoles(db); + + const paramRoles = db.prepare("SELECT role FROM nodes WHERE kind = 'parameter'").all() as { + role: string | null; + }[]; + expect(paramRoles).toHaveLength(2); + for (const row of paramRoles) { + expect(row.role).toBeNull(); + } + }); + + it('does not classify interface members as dead', () => { + // TS `interface ExtractParametersOptions { typeMap?: Map<...> }` extracts + // `typeMap` as a top-level `method`-kind definition named + // `ExtractParametersOptions.typeMap` (property-signature members are + // currently mislabeled `method` by the extractor — tracked separately). + // It has no callers by construction; it must not be flagged dead. + insertNode('src/helpers.ts', 'file', 'src/helpers.ts', 0); + insertNode('ExtractParametersOptions', 'interface', 'src/helpers.ts', 359); + insertNode('ExtractParametersOptions.typeMap', 'method', 'src/helpers.ts', 361); + + classifyNodeRoles(db); + + const role = db + .prepare("SELECT role FROM nodes WHERE name = 'ExtractParametersOptions.typeMap'") + .get() as { role: string | null }; + expect(role.role).toBe('leaf'); + }); + + it('regression: used parameters, interface members, and a genuinely dead function are classified correctly together', () => { + // Mirrors the exact issue #1723 repro: `findChild(node, type)` in + // src/extractors/helpers.ts, alongside an interface and a truly dead function. + insertNode('src/helpers.ts', 'file', 'src/helpers.ts', 0); + const findChildFn = insertNode('findChild', 'function', 'src/helpers.ts', 26); + insertNode('node', 'parameter', 'src/helpers.ts', 26); + insertNode('type', 'parameter', 'src/helpers.ts', 26); + insertNode('ChaContext', 'interface', 'src/helpers.ts', 18); + insertNode('ChaContext.implementors', 'method', 'src/helpers.ts', 20); + insertNode('trulyDeadHelper', 'function', 'src/helpers.ts', 400); + + const caller = insertNode('caller', 'function', 'other.ts', 1); + insertEdge(caller, findChildFn, 'calls'); + + classifyNodeRoles(db); + const getRole = (name) => db.prepare('SELECT role FROM nodes WHERE name = ?').get(name)?.role; + + expect(getRole('node')).toBeNull(); + expect(getRole('type')).toBeNull(); + expect(getRole('ChaContext.implementors')).toBe('leaf'); + expect(getRole('trulyDeadHelper')).toBe('dead-unresolved'); + }); }); From 17fbcba97423e96693bfc2b689b090abd4ea45ff Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Sun, 5 Jul 2026 13:25:38 -0600 Subject: [PATCH 3/8] fix: credit import-type usages as exports consumers codegraph exports --json (and the --unused dead-export filter it feeds) reported zero consumers for interfaces/types that are demonstrably imported and used elsewhere via `import type { X }`, even though `codegraph deps ` correctly showed the importing file in `importedBy`. Example: ChaContext in src/domain/graph/builder/cha.ts is imported (type-only) by build-edges.ts and native-orchestrator.ts, but `codegraph exports` showed consumerCount: 0. Root cause: exportsFileImpl's per-symbol consumers query (domain/analysis/exports.ts) only looked at kind = 'calls' edges. The builder already emits a symbol-level `imports-type` edge for `import type { X }` statements (source = importing file node, target = the specific imported symbol -- see emitTypeOnlySymbolEdges in build-edges.ts/incremental.ts), which `codegraph deps` reads from, but the exports consumer query never looked at this edge kind. Role classification (features/structure.ts) already includes 'imports-type' in its fan-in formula, so `codegraph roles --role dead` was unaffected -- this was purely a gap in exports's independent consumer list. Fix: widen the consumers query to `kind IN ('calls', 'imports-type')`, matching the edge-kind set already used everywhere else in the codebase for cross-file usage credit (structure.ts, graph-enrichment.ts, boundaries.ts, dependencies.ts). No native Rust changes needed -- domain/analysis/exports.ts is pure query-layer code that reads the already-built edges table and has no engine-specific mirror. Deliberately does NOT add `extends`/`implements`: investigation found those edges are resolved by symbol name only, with no file/import scoping (buildClassHierarchyEdges in both build-edges.ts/incremental.ts and the native emit_hierarchy_edges), so they link same-named declarations across unrelated files (verified: this repo's own graph has false `implements`/`extends` edges between unrelated fixture classes across languages and a `Repository` interface in src/types.ts). Filed as #1812. Also filed #1813 for a related but distinct gap: `import { type X }` inline per-specifier modifiers aren't tracked as type-only in either engine's extractor, so such X still gets no credit even after this fix. Added tests/integration/exports.test.ts coverage: an interface consumed only via a symbol-level `imports-type` edge gets consumerCount >= 1 and is excluded from --unused, while a genuinely unreferenced interface still shows 0 consumers. Verified against this repo's own graph: `codegraph exports src/domain/graph/builder/cha.ts -T --json` now credits ChaContext with 2 consumers (build-edges.ts, native-orchestrator.ts). Full test suite (201 files, 3359 tests) and lint pass clean. docs check acknowledged: internal bug fix to exports consumer computation, no new feature/language/CLI surface/architecture change -- README.md, CLAUDE.md, and ROADMAP.md are unaffected. Fixes #1724 Impact: 1 functions changed, 3 affected --- src/domain/analysis/exports.ts | 15 ++++++- tests/integration/exports.test.ts | 73 +++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 1 deletion(-) diff --git a/src/domain/analysis/exports.ts b/src/domain/analysis/exports.ts index 62f97c434..3c9710df6 100644 --- a/src/domain/analysis/exports.ts +++ b/src/domain/analysis/exports.ts @@ -164,11 +164,24 @@ function exportsFileImpl( "SELECT * FROM nodes WHERE file = ? AND kind != 'file' AND exported = 1 ORDER BY line", ) : null; + // Consumers include real call/construct edges plus `imports-type` edges — + // the symbol-level edge emitted for `import type { X }` statements (source + // is the importing *file* node, since the import statement references the + // type rather than a specific function). Without this, interfaces/types + // that are only ever used as type annotations are misclassified as dead + // exports even though `codegraph deps` already reports the importing file + // via this same edge (#1724). + // + // `extends`/`implements` edges are deliberately NOT included here: they are + // resolved by symbol name only, with no file/import scoping (see + // buildClassHierarchyEdges), so they can link same-named declarations + // across unrelated files — crediting them as consumers would surface false + // positives instead of fixing false negatives. const consumersStmt = cachedStmt( _consumersStmtCache, db, `SELECT n.name, n.file, n.line FROM edges e JOIN nodes n ON e.source_id = n.id - WHERE e.target_id = ? AND e.kind = 'calls'`, + WHERE e.target_id = ? AND e.kind IN ('calls', 'imports-type')`, ); const reexportsFromStmt = cachedStmt( _reexportsFromStmtCache, diff --git a/tests/integration/exports.test.ts b/tests/integration/exports.test.ts index 467621280..f869ec115 100644 --- a/tests/integration/exports.test.ts +++ b/tests/integration/exports.test.ts @@ -240,3 +240,76 @@ describe('exportsData', () => { expect(data.totalReexportedUnused).toBe(0); }); }); + +// ─── import type / type-only consumer crediting (#1724) ────────────────── +// +// Regression coverage for: interfaces/types that are only ever consumed via +// `import type { X }` (never called/constructed) were reported as zero- +// consumer dead exports, even though the builder already emits a +// symbol-level `imports-type` edge (source = importing file node, target = +// the specific imported symbol) for exactly this case. `codegraph deps` +// already surfaced the file-level import correctly; `exportsData`'s +// per-symbol consumer query only looked at `kind = 'calls'` and missed it. + +describe('exportsData — import type consumer crediting (#1724)', () => { + let tmpDir2: string, dbPath2: string; + + beforeAll(() => { + tmpDir2 = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-exports-typeonly-')); + fs.mkdirSync(path.join(tmpDir2, '.codegraph')); + dbPath2 = path.join(tmpDir2, '.codegraph', 'graph.db'); + + const db = new Database(dbPath2); + db.pragma('journal_mode = WAL'); + initSchema(db); + + // File nodes + insertNode(db, 'types.ts', 'file', 'types.ts', 0); + const fConsumer = insertNode(db, 'consumer.ts', 'file', 'consumer.ts', 0); + + // Interface exported from types.ts, referenced elsewhere only via a type + // annotation (never called/constructed). + const configIface = insertNode(db, 'Config', 'interface', 'types.ts', 1); + // Interface exported from types.ts, genuinely never referenced anywhere. + const unusedIface = insertNode(db, 'Unused', 'interface', 'types.ts', 10); + + const markExported = db.prepare('UPDATE nodes SET exported = 1 WHERE id = ?'); + markExported.run(configIface); + markExported.run(unusedIface); + + // consumer.ts does `import type { Config } from './types'`. The builder + // emits the symbol-level edge with the importing *file* as source (see + // emitTypeOnlySymbolEdges in domain/graph/builder/stages/build-edges.ts + // and incremental.ts) since the import statement — not a specific + // function — is what references the type. + insertEdge(db, fConsumer, configIface, 'imports-type'); + + db.close(); + }); + + afterAll(() => { + if (tmpDir2) fs.rmSync(tmpDir2, { recursive: true, force: true }); + }); + + test('interface consumed only via `import type` is credited with a consumer', () => { + const data = exportsData('types.ts', dbPath2); + const config = data.results.find((r) => r.name === 'Config'); + expect(config).toBeDefined(); + expect(config.consumerCount).toBe(1); + expect(config.consumers.length).toBe(1); + expect(config.consumers[0].file).toBe('consumer.ts'); + }); + + test('interface consumed only via `import type` is excluded from --unused', () => { + const data = exportsData('types.ts', dbPath2, { unused: true }); + expect(data.results.find((r) => r.name === 'Config')).toBeUndefined(); + }); + + test('interface with no references anywhere is still classified unused', () => { + const data = exportsData('types.ts', dbPath2, { unused: true }); + const unused = data.results.find((r) => r.name === 'Unused'); + expect(unused).toBeDefined(); + expect(unused.consumerCount).toBe(0); + expect(unused.consumers).toEqual([]); + }); +}); From 26918bb90ebad99480933cf54adeb5a1cf7e336c Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Sun, 5 Jul 2026 13:56:17 -0600 Subject: [PATCH 4/8] fix: prevent fn-impact/query crash when -f/--file is passed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CLI's `-f/--file` option is a repeatable Commander accumulator (collectFile) that always produces a string[], even on first use. The native composite bindings backing fn-impact (findNodesWithFanIn) and query (fnDeps) forwarded that array straight into napi-rs functions whose Rust signatures only accepted a single String, so any use of -f/--file crashed with "Failed to convert JavaScript value ... into rust type `String`" regardless of engine defaults. context worked only because its code path never touches the native repository for symbol lookup. Widen the native Rust signatures (find_nodes_with_fan_in, fn_deps) to accept Vec and build an OR-of-LIKE clause for multiple files, mirroring buildFileConditionSQL/NodeQuery.fileFilter on the JS side. Normalize the file option before calling into the native binding on the TS side, and thread the widened string | string[] type through QueryOpts, fnDeps's opts, and the call chain down to findMatchingNodes. This also makes repeated -f/--file genuinely multi-file end to end for fn-impact/query, matching context's existing behavior, instead of crashing on any use. findNodesByScope shares QueryOpts but has no CLI caller and its native binding still only accepts one file; it now takes the first value defensively rather than crash or fail to type-check. Fixes #1726 docs check acknowledged: pure bug fix, no new CLI options/languages/ architecture — the -f/--file "repeatable" docs are now accurate rather than needing correction. Impact: 14 functions changed, 34 affected --- .../src/db/repository/graph_read.rs | 47 +++++++++++-- src/db/repository/base.ts | 2 +- src/db/repository/native-repository.ts | 17 +++-- src/domain/analysis/dependencies.ts | 2 +- src/domain/analysis/fn-impact.ts | 2 +- src/domain/analysis/symbol-lookup.ts | 7 +- src/presentation/queries-cli/impact.ts | 2 +- src/types.ts | 9 +-- tests/integration/cli.test.ts | 66 +++++++++++++++++++ tests/integration/queries.test.ts | 47 +++++++++++++ 10 files changed, 179 insertions(+), 22 deletions(-) diff --git a/crates/codegraph-core/src/db/repository/graph_read.rs b/crates/codegraph-core/src/db/repository/graph_read.rs index b55bec051..5f36382e8 100644 --- a/crates/codegraph-core/src/db/repository/graph_read.rs +++ b/crates/codegraph-core/src/db/repository/graph_read.rs @@ -28,6 +28,41 @@ fn escape_like(s: &str) -> String { out } +/// Append an `AND (col LIKE ? [OR col LIKE ? ...])` fragment for a multi-value +/// file filter and push the corresponding escaped LIKE params. Mirrors +/// `buildFileConditionSQL()` / `NodeQuery.fileFilter()` in +/// `src/db/query-builder.ts`, which is why callers must accept `Vec` +/// (repeatable `-f/--file`) rather than a single `String`. No-op when `files` +/// is empty. Advances `*idx` past the placeholders it consumes. +fn push_file_filter( + sql: &mut String, + params_v: &mut Vec>, + idx: &mut usize, + column: &str, + files: &[String], +) { + if files.is_empty() { + return; + } + let start = *idx; + if files.len() == 1 { + sql.push_str(&format!(" AND {column} LIKE ?{start} ESCAPE '\\'")); + params_v.push(Box::new(format!("%{}%", escape_like(&files[0])))); + *idx += 1; + return; + } + let clauses: Vec = files + .iter() + .enumerate() + .map(|(i, _)| format!("{column} LIKE ?{} ESCAPE '\\'", start + i)) + .collect(); + sql.push_str(&format!(" AND ({})", clauses.join(" OR "))); + for f in files { + params_v.push(Box::new(format!("%{}%", escape_like(f)))); + } + *idx += files.len(); +} + /// Check if a file path looks like a test file (mirrors `isTestFile` in JS). fn is_test_file(file: &str) -> bool { file.contains(".test.") @@ -143,7 +178,7 @@ struct FnDepsCallerWithId { fn build_fn_deps_match_query( name: &str, kind: Option<&str>, - file: Option<&str>, + file: Option<&[String]>, ) -> (String, Vec>) { let default_kinds: Vec = vec![ "function".to_string(), @@ -180,8 +215,7 @@ fn build_fn_deps_match_query( idx += kinds.len(); } if let Some(f) = file { - sql.push_str(&format!(" AND n.file LIKE ?{idx} ESCAPE '\\'")); - params_v.push(Box::new(format!("%{}%", escape_like(f)))); + push_file_filter(&mut sql, &mut params_v, &mut idx, "n.file", f); } (sql, params_v) @@ -1086,7 +1120,7 @@ impl NativeDatabase { &self, name_pattern: String, kinds: Option>, - file: Option, + file: Option>, ) -> napi::Result> { let conn = self.conn()?; @@ -1112,8 +1146,7 @@ impl NativeDatabase { } } if let Some(ref f) = file { - sql.push_str(&format!(" AND n.file LIKE ?{idx} ESCAPE '\\'")); - param_values.push(Box::new(format!("%{}%", escape_like(f)))); + push_file_filter(&mut sql, &mut param_values, &mut idx, "n.file", f); } let mut stmt = conn @@ -2114,7 +2147,7 @@ impl NativeDatabase { name: String, depth: Option, no_tests: Option, - file: Option, + file: Option>, kind: Option, ) -> napi::Result { let conn = self.conn()?; diff --git a/src/db/repository/base.ts b/src/db/repository/base.ts index 4c1546c2e..9ab137a86 100644 --- a/src/db/repository/base.ts +++ b/src/db/repository/base.ts @@ -231,7 +231,7 @@ export class Repository implements IRepository { */ fnDeps( _name: string, - _opts?: { depth?: number; noTests?: boolean; file?: string; kind?: string }, + _opts?: { depth?: number; noTests?: boolean; file?: string | string[]; kind?: string }, ): FnDepsResult | null { return null; } diff --git a/src/db/repository/native-repository.ts b/src/db/repository/native-repository.ts index a822c6912..202b3bb74 100644 --- a/src/db/repository/native-repository.ts +++ b/src/db/repository/native-repository.ts @@ -45,6 +45,7 @@ import type { TriageNodeRow, TriageQueryOpts, } from '../../types.js'; +import { normalizeFileFilter } from '../query-builder.js'; import { type FnDepsCallerNode, type FnDepsEntry, @@ -267,8 +268,9 @@ export class NativeRepository extends Repository { } findNodesWithFanIn(namePattern: string, opts: QueryOpts = {}): NodeRowWithFanIn[] { + const files = normalizeFileFilter(opts.file); return this.#ndb - .findNodesWithFanIn(namePattern, opts.kinds ?? null, opts.file ?? null) + .findNodesWithFanIn(namePattern, opts.kinds ?? null, files.length > 0 ? files : null) .map(toNodeRowWithFanIn); } @@ -301,9 +303,11 @@ export class NativeRepository extends Repository { } findNodesByScope(scopeName: string, opts: QueryOpts = {}): NodeRow[] { - return this.#ndb - .findNodesByScope(scopeName, opts.kind ?? null, opts.file ?? null) - .map(toNodeRow); + // Unlike findNodesWithFanIn/fnDeps, this native binding only accepts a single + // file (no CLI command currently wires -f/--file to it); take the first value + // rather than crash if a caller ever passes the repeatable-flag array form. + const [file] = normalizeFileFilter(opts.file); + return this.#ndb.findNodesByScope(scopeName, opts.kind ?? null, file ?? null).map(toNodeRow); } findNodeByQualifiedName(qualifiedName: string, opts: { file?: string } = {}): NodeRow[] { @@ -526,14 +530,15 @@ export class NativeRepository extends Repository { // ── Composite queries ────────────────────────────────────────────── fnDeps( name: string, - opts?: { depth?: number; noTests?: boolean; file?: string; kind?: string }, + opts?: { depth?: number; noTests?: boolean; file?: string | string[]; kind?: string }, ): FnDepsResult | null { if (typeof this.#ndb.fnDeps !== 'function') return null; + const files = normalizeFileFilter(opts?.file); const raw = this.#ndb.fnDeps( name, opts?.depth ?? undefined, opts?.noTests ?? undefined, - opts?.file ?? undefined, + files.length > 0 ? files : undefined, opts?.kind ?? undefined, ); // Convert from native format (transitive_callers as array of groups) diff --git a/src/domain/analysis/dependencies.ts b/src/domain/analysis/dependencies.ts index 4a759e6e1..e602f0ab6 100644 --- a/src/domain/analysis/dependencies.ts +++ b/src/domain/analysis/dependencies.ts @@ -175,7 +175,7 @@ export function fnDepsData( opts: { depth?: number; noTests?: boolean; - file?: string; + file?: string | string[]; kind?: string; limit?: number; offset?: number; diff --git a/src/domain/analysis/fn-impact.ts b/src/domain/analysis/fn-impact.ts index 148cf5b59..d7cfc2901 100644 --- a/src/domain/analysis/fn-impact.ts +++ b/src/domain/analysis/fn-impact.ts @@ -268,7 +268,7 @@ export function fnImpactData( opts: { depth?: number; noTests?: boolean; - file?: string; + file?: string | string[]; kind?: string; includeImplementors?: boolean; limit?: number; diff --git a/src/domain/analysis/symbol-lookup.ts b/src/domain/analysis/symbol-lookup.ts index 5009d043a..065c11f4e 100644 --- a/src/domain/analysis/symbol-lookup.ts +++ b/src/domain/analysis/symbol-lookup.ts @@ -38,7 +38,12 @@ const FUNCTION_KINDS: SymbolKind[] = ['function', 'method', 'class', 'constant'] export function findMatchingNodes( dbOrRepo: BetterSqlite3Database | InstanceType, name: string, - opts: { noTests?: boolean; file?: string; kind?: string; kinds?: readonly string[] } = {}, + opts: { + noTests?: boolean; + file?: string | string[]; + kind?: string; + kinds?: readonly string[]; + } = {}, ): Array { const kinds = ( opts.kind ? [opts.kind] : opts.kinds?.length ? [...opts.kinds] : FUNCTION_KINDS diff --git a/src/presentation/queries-cli/impact.ts b/src/presentation/queries-cli/impact.ts index e9eafe052..94b1fa64d 100644 --- a/src/presentation/queries-cli/impact.ts +++ b/src/presentation/queries-cli/impact.ts @@ -118,7 +118,7 @@ interface OutputOpts { format?: string; staged?: boolean; ref?: string; - file?: string; + file?: string | string[]; kind?: string; [key: string]: unknown; } diff --git a/src/types.ts b/src/types.ts index 980713528..8374a2a0c 100644 --- a/src/types.ts +++ b/src/types.ts @@ -265,7 +265,8 @@ export interface ComplexityMetrics { export interface QueryOpts { kind?: SymbolKind; kinds?: SymbolKind[]; - file?: string; + /** Partial-match file filter. Repeatable on the CLI (`-f/--file`), so a single value arrives as a one-element array. */ + file?: string | string[]; noTests?: boolean; } @@ -346,7 +347,7 @@ export interface Repository { // ── Composite queries ────────────────────────────────────────────── fnDeps( name: string, - opts?: { depth?: number; noTests?: boolean; file?: string; kind?: string }, + opts?: { depth?: number; noTests?: boolean; file?: string | string[]; kind?: string }, ): { name: string; results: Array<{ @@ -2496,7 +2497,7 @@ export interface NativeDatabase { findNodesWithFanIn( namePattern: string, kinds: string[] | null | undefined, - file: string | null | undefined, + file: string[] | null | undefined, ): NativeNodeRowWithFanIn[]; findNodesForTriage( kind: string | null | undefined, @@ -2553,7 +2554,7 @@ export interface NativeDatabase { name: string, depth: number | null | undefined, noTests: boolean | null | undefined, - file: string | null | undefined, + file: string[] | null | undefined, kind: string | null | undefined, ): NativeFnDepsResult; diff --git a/tests/integration/cli.test.ts b/tests/integration/cli.test.ts index f61b69153..28c3a0a76 100644 --- a/tests/integration/cli.test.ts +++ b/tests/integration/cli.test.ts @@ -110,6 +110,40 @@ describe('CLI smoke tests', () => { expect(data).toHaveProperty('results'); }); + // Regression test for #1726: `-f/--file` is a repeatable Commander option + // (collectFile) that always produces a string[], even for a single use. + // query's native composite path used to forward that array straight into + // a napi binding typed for a single String and crash. + test('query -f scopes results to a single file without crashing', () => { + const out = run('query', 'add', '-f', 'math.js', '--db', dbPath, '--json'); + const data = JSON.parse(out); + expect(data.results.length).toBeGreaterThan(0); + for (const r of data.results) expect(r.file).toContain('math.js'); + }); + + test('query --file (long form) scopes results to a single file without crashing', () => { + const out = run('query', 'add', '--file', 'math.js', '--db', dbPath, '--json'); + const data = JSON.parse(out); + expect(data.results.length).toBeGreaterThan(0); + for (const r of data.results) expect(r.file).toContain('math.js'); + }); + + test('query with a single -f excludes non-matching files', () => { + // `add` is only defined in math.js, so scoping to utils.js must be empty. + const out = run('query', 'add', '-f', 'utils.js', '--db', dbPath, '--json'); + const data = JSON.parse(out); + expect(data.results).toHaveLength(0); + }); + + test('query supports repeated -f (multi-file scoping)', () => { + // "square" substring-matches square() in math.js and sumOfSquares() in utils.js. + const out = run('query', 'square', '-f', 'math.js', '-f', 'utils.js', '--db', dbPath, '--json'); + const data = JSON.parse(out); + const names = data.results.map((r) => r.name); + expect(names).toContain('square'); + expect(names).toContain('sumOfSquares'); + }); + // ─── Fn-Impact ─────────────────────────────────────────────────────── test('fn-impact --json returns valid JSON with results', () => { const out = run('fn-impact', 'add', '--db', dbPath, '--json'); @@ -117,6 +151,38 @@ describe('CLI smoke tests', () => { expect(data).toHaveProperty('results'); }); + // Regression test for #1726 (see query -f test above for full context). + test('fn-impact -f scopes results to a single file without crashing', () => { + const out = run('fn-impact', 'add', '-f', 'math.js', '--db', dbPath, '--json'); + const data = JSON.parse(out); + expect(data.results.length).toBeGreaterThan(0); + for (const r of data.results) expect(r.file).toContain('math.js'); + }); + + test('fn-impact with a single -f excludes non-matching files', () => { + const out = run('fn-impact', 'add', '-f', 'utils.js', '--db', dbPath, '--json'); + const data = JSON.parse(out); + expect(data.results).toHaveLength(0); + }); + + test('fn-impact supports repeated -f (multi-file scoping)', () => { + const out = run( + 'fn-impact', + 'square', + '-f', + 'math.js', + '-f', + 'utils.js', + '--db', + dbPath, + '--json', + ); + const data = JSON.parse(out); + const names = data.results.map((r) => r.name); + expect(names).toContain('square'); + expect(names).toContain('sumOfSquares'); + }); + // ─── Query (path mode, formerly path) ──────────────────────────────── test('query --path --json returns valid JSON with path info', () => { const out = run('query', 'sumOfSquares', '--path', 'add', '--db', dbPath, '--json'); diff --git a/tests/integration/queries.test.ts b/tests/integration/queries.test.ts index 11fe0190c..c76d4ac86 100644 --- a/tests/integration/queries.test.ts +++ b/tests/integration/queries.test.ts @@ -361,6 +361,31 @@ describe('fnDepsData', () => { expect(names).not.toContain('authMiddleware'); }); + // Regression test for #1726: the CLI's `-f/--file` option is a repeatable + // Commander accumulator (collectFile) that always produces a string[], even + // for a single use. The native composite fnDeps() path used to forward that + // array straight into a napi binding typed for a single String and crash + // with "Failed to convert JavaScript value ... into rust type `String`". + test('--file as a single-element array (real CLI shape) scopes to a single file', () => { + const data = fnDepsData('auth', dbPath, { file: ['auth.js'] }); + for (const r of data.results) { + expect(r.file).toContain('auth.js'); + } + const names = data.results.map((r) => r.name); + expect(names).not.toContain('authMiddleware'); + }); + + test('--file with multiple values (repeated -f) scopes across all of them', () => { + const data = fnDepsData('auth', dbPath, { file: ['auth.js', 'middleware.js'] }); + const names = data.results.map((r) => r.name); + expect(names).toContain('authenticate'); // auth.js + expect(names).toContain('authMiddleware'); // middleware.js + // routes.js/utils.js should still be excluded + for (const r of data.results) { + expect(['auth.js', 'middleware.js']).toContain(r.file); + } + }); + test('--kind method filters to methods only', () => { // All fixtures are functions, so filtering by method should return empty const data = fnDepsData('auth', dbPath, { kind: 'method' }); @@ -414,6 +439,28 @@ describe('fnImpactData', () => { expect(r.direct + r.transitive).toBe(r.totalDependents); }); + // Regression test for #1726 — see the equivalent fnDepsData test above for + // full context. fnImpactData's native path (findNodesWithFanIn) forwards + // the file filter used to resolve the *matched root symbol(s)* (not their + // callers) into rusqlite, and used to crash on the array shape the CLI + // always sends. + test('--file as a single-element array scopes matched root symbols to that file', () => { + const data = fnImpactData('auth', dbPath, { file: ['auth.js'] }); + const names = data.results.map((r) => r.name); + expect(names).toContain('authenticate'); + expect(names).not.toContain('authMiddleware'); // middleware.js + for (const r of data.results) expect(r.file).toBe('auth.js'); + }); + + test('--file with multiple values scopes matched root symbols across all of them', () => { + const data = fnImpactData('auth', dbPath, { file: ['auth.js', 'middleware.js'] }); + const names = data.results.map((r) => r.name); + expect(names).toContain('authenticate'); // auth.js + expect(names).toContain('authMiddleware'); // middleware.js + // preAuthenticate (utils.js) also substring-matches "auth" — must stay excluded + for (const r of data.results) expect(['auth.js', 'middleware.js']).toContain(r.file); + }); + test('direct and transitive are 0 for a function with no callers', () => { // handleRoute is the root of the call graph — nothing calls it const data = fnImpactData('handleRoute', dbPath); From be6432cda8580230d9ed0401ceb7bc5720abc281 Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Sun, 5 Jul 2026 14:29:26 -0600 Subject: [PATCH 5/8] fix: correct exported-symbol detection for literal and object-literal exports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two compounding root causes made `where --file`'s `exported` array (and `codegraph exports`) unreliable for entire classes of exports: 1. The JS/TS export-statement handler (WASM query path, WASM walk path, and the mirrored native Rust extractor) only recognized `export function`, `export class`, `export interface`, and `export type` declarations. It never matched `lexical_declaration`/`variable_declaration`, so `export const/let/var …` was never added to the extractor's export list — regardless of the initializer's shape — leaving the `exported=1` DB column permanently unset for every exported constant in the codebase (including ones that happened to look "correct", like a `new Set(...)`-initialized constant referenced as a call argument elsewhere). 2. `where --file`'s `exported` list ignored the `exported` DB column entirely and computed membership from `findCrossFileCallTargets` (symbols targeted by a cross-file `calls` edge) — a heuristic that only "worked" by coincidence for symbols that happened to be passed as call arguments elsewhere. `codegraph exports` already had the right idea (prefer the `exported` column, fall back to the heuristic only for pre-migration DBs) but duplicated that logic locally instead of sharing it. Fixes: - src/extractors/javascript.ts: extract the function/class/interface/type kind map plus a new lexical/variable-declaration branch into a shared `collectExportedDeclarations`, used by both `handleExportCapture` (query path) and `handleExportStmt` (walk path) so they can't drift apart again. Declarator values are classified with the same predicate already used to build the matching Definition (function-valued -> kind 'function', literal/array/object/new-expression-valued `const` -> kind 'constant'). - crates/codegraph-core/src/extractors/javascript.rs: mirror the same fix in `handle_export_declaration` via a new `collect_exported_var_declarations`. - src/db/repository/nodes.ts: add `findExportedNodesByFile`, the single shared implementation of "prefer exported=1, fall back to cross-file calls for pre-migration DBs" — re-exported through db/repository/index.ts and db/index.ts. - src/domain/analysis/exports.ts: use the shared helper instead of a locally duplicated hasExportedCol/exportedNodesStmt implementation. - src/domain/analysis/symbol-lookup.ts: `whereFileImpl` now uses the shared helper instead of `findCrossFileCallTargets` directly, so `where --file` and `exports` agree on what "exported" means. Tests: - tests/parsers/javascript.test.ts: unit coverage for bare-literal, new Set(...), object-literal-with-methods, and arrow-function exported consts, plus a non-exported-const negative case and a function/class regression guard for the shared-helper refactor. - tests/engines/query-walk-parity.test.ts, tests/engines/parity.test.ts: cross-path/cross-engine regression cases for the same shapes. - tests/integration/queries.test.ts: new fixture node with exported=1 but zero incoming edges of any kind, proving `where --file`'s exported list no longer depends on cross-file call presence. No user-facing command/language/architecture changes, so README/CLAUDE.md/ ROADMAP.md are unaffected — docs check acknowledged. Fixes #1728 Impact: 8 functions changed, 17 affected --- .../src/extractors/javascript.rs | 53 +++++++++++ src/db/index.ts | 1 + src/db/repository/index.ts | 1 + src/db/repository/nodes.ts | 52 +++++++++++ src/domain/analysis/exports.ts | 52 +---------- src/domain/analysis/symbol-lookup.ts | 6 +- src/extractors/javascript.ts | 92 ++++++++++++------- tests/engines/parity.test.ts | 22 +++++ tests/engines/query-walk-parity.test.ts | 18 ++++ tests/integration/queries.test.ts | 25 ++++- tests/parsers/javascript.test.ts | 63 +++++++++++++ 11 files changed, 298 insertions(+), 87 deletions(-) diff --git a/crates/codegraph-core/src/extractors/javascript.rs b/crates/codegraph-core/src/extractors/javascript.rs index 5a9292c7e..21d91a8c6 100644 --- a/crates/codegraph-core/src/extractors/javascript.rs +++ b/crates/codegraph-core/src/extractors/javascript.rs @@ -1700,6 +1700,10 @@ fn handle_export_declaration(node: &Node, decl: &Node, source: &[u8], symbols: & "class_declaration" | "abstract_class_declaration" => ("class", "name"), "interface_declaration" => ("interface", "name"), "type_alias_declaration" => ("type", "name"), + "lexical_declaration" | "variable_declaration" => { + collect_exported_var_declarations(node, decl, source, symbols); + return; + } _ => return, }; if let Some(n) = decl.child_by_field_name(field) { @@ -1711,6 +1715,55 @@ fn handle_export_declaration(node: &Node, decl: &Node, source: &[u8], symbols: & } } +/// Push `ExportInfo` entries for `export const/let/var …`, one per declarator. +/// +/// Named function/class/interface/type declarations carry their own `name` +/// field (handled above); a lexical/variable declaration doesn't, so each +/// declarator's value is classified the same way `handle_var_decl` classifies +/// it when creating the matching `Definition`: function-valued declarators +/// become kind "function", `const` declarators with a literal/array/object/ +/// new-expression value (per `is_js_literal`) become kind "constant". +/// Mirrors the WASM/TS extractor's `collectExportedDeclarations`. +/// +/// This predicate must stay identical to `handle_var_decl`'s: `insert_nodes.rs` +/// marks `exported = 1` by matching (name, kind, file, line) against +/// already-inserted definition rows, so a mismatched kind here silently +/// no-ops the UPDATE instead of marking the symbol exported (#1728). +fn collect_exported_var_declarations( + node: &Node, + decl: &Node, + source: &[u8], + symbols: &mut FileSymbols, +) { + let is_const = decl + .child(0) + .map(|c| node_text(&c, source) == "const") + .unwrap_or(false); + let line = start_line(node); + for i in 0..decl.child_count() { + let Some(declarator) = decl.child(i) else { continue }; + if declarator.kind() != "variable_declarator" { continue; } + let name_n = declarator.child_by_field_name("name"); + let value_n = declarator.child_by_field_name("value"); + let (Some(name_n), Some(value_n)) = (name_n, value_n) else { continue }; + if name_n.kind() != "identifier" { continue; } + let vt = value_n.kind(); + if vt == "arrow_function" || vt == "function_expression" || vt == "function" || vt == "generator_function" { + symbols.exports.push(ExportInfo { + name: node_text(&name_n, source).to_string(), + kind: "function".to_string(), + line, + }); + } else if is_const && is_js_literal(&value_n) { + symbols.exports.push(ExportInfo { + name: node_text(&name_n, source).to_string(), + kind: "constant".to_string(), + line, + }); + } + } +} + fn handle_reexport(node: &Node, source_node: &Node, source: &[u8], symbols: &mut FileSymbols) { let mod_path = node_text(source_node, source) .replace(&['\'', '"'][..], ""); diff --git a/src/db/index.ts b/src/db/index.ts index 65b81de9c..f08a37677 100644 --- a/src/db/index.ts +++ b/src/db/index.ts @@ -39,6 +39,7 @@ export { findCallers, findCrossFileCallTargets, findDistinctCallers, + findExportedNodesByFile, findFileNodes, findImplementors, findImportDependents, diff --git a/src/db/repository/index.ts b/src/db/repository/index.ts index a7011806a..5fd5d1ff9 100644 --- a/src/db/repository/index.ts +++ b/src/db/repository/index.ts @@ -35,6 +35,7 @@ export { countEdges, countFiles, countNodes, + findExportedNodesByFile, findFileNodes, findNodeById, findNodeByQualifiedName, diff --git a/src/db/repository/nodes.ts b/src/db/repository/nodes.ts index b9465383e..cab617a5d 100644 --- a/src/db/repository/nodes.ts +++ b/src/db/repository/nodes.ts @@ -15,6 +15,7 @@ import type { } from '../../types.js'; import { buildFileConditionSQL, NodeQuery } from '../query-builder.js'; import { cachedStmt } from './cached-stmt.js'; +import { findCrossFileCallTargets } from './edges.js'; // ─── Query-builder based lookups (moved from src/db/repository.js) ───── @@ -179,6 +180,57 @@ export function findNodesByFile(db: BetterSqlite3Database, file: string): NodeRo ).all(file); } +/** Cache the schema probe for the `exported` column per db handle. */ +const _hasExportedColCache: WeakMap = new WeakMap(); + +const _findExportedNodesByFileStmt: StmtCache = new WeakMap(); + +/** + * Detect whether this DB's `nodes` table has the `exported` column populated + * (older DBs built before it existed fall back to a cross-file-calls + * heuristic). Cached per db handle to avoid re-probing on every call. + */ +function hasExportedColumn(db: BetterSqlite3Database): boolean { + if (_hasExportedColCache.has(db)) return _hasExportedColCache.get(db)!; + let has = false; + try { + db.prepare('SELECT exported FROM nodes LIMIT 0').raw(true); + has = true; + } catch { + /* older DB predates the exported column */ + } + _hasExportedColCache.set(db, has); + return has; +} + +/** + * Find nodes in `file` that are part of its exported surface. + * + * Prefers the `exported = 1` column — set by the extractor at build time from + * the file's actual `export` statements, the authoritative "was this + * declared as exported" signal — and only falls back to + * `findCrossFileCallTargets` (symbols targeted by a cross-file `calls` edge) + * for DBs built before the column existed. + * + * Shared by `codegraph exports` and `codegraph where --file` so both commands + * agree on what "exported" means; before this was extracted, `where --file` + * used the cross-file-calls heuristic unconditionally, which misses any + * exported symbol that's only ever read as a value (not called) from another + * file — e.g. an exported constant used in a comparison (#1728). + */ +export function findExportedNodesByFile(db: BetterSqlite3Database, file: string): NodeRow[] { + if (hasExportedColumn(db)) { + return cachedStmt( + _findExportedNodesByFileStmt, + db, + "SELECT * FROM nodes WHERE file = ? AND kind != 'file' AND exported = 1 ORDER BY line", + ).all(file); + } + const symbols = findNodesByFile(db, file); + const exportedIds = findCrossFileCallTargets(db, file); + return symbols.filter((s) => exportedIds.has(s.id)); +} + /** * Find file-kind nodes matching a LIKE pattern. */ diff --git a/src/domain/analysis/exports.ts b/src/domain/analysis/exports.ts index 3c9710df6..f4780f8a3 100644 --- a/src/domain/analysis/exports.ts +++ b/src/domain/analysis/exports.ts @@ -1,12 +1,11 @@ import path from 'node:path'; import { - findCrossFileCallTargets, findDbPath, + findExportedNodesByFile, findFileNodes, findNodesByFile, } from '../../db/index.js'; import { cachedStmt } from '../../db/repository/cached-stmt.js'; -import { debug } from '../../infrastructure/logger.js'; import { isTestFile } from '../../infrastructure/test-filter.js'; import { createFileLinesReader, @@ -17,10 +16,6 @@ import { paginateResult } from '../../shared/paginate.js'; import type { BetterSqlite3Database, NodeRow, StmtCache } from '../../types.js'; import { resolveAnalysisOpts, withReadonlyDb } from './query-helpers.js'; -/** Cache the schema probe for the `exported` column per db handle. */ -const _hasExportedColCache: WeakMap = new WeakMap(); - -const _exportedNodesStmtCache: StmtCache = new WeakMap(); const _consumersStmtCache: StmtCache<{ name: string; file: string; line: number }> = new WeakMap(); const _reexportsFromStmtCache: StmtCache<{ file: string }> = new WeakMap(); const _reexportsToStmtCache: StmtCache<{ file: string }> = new WeakMap(); @@ -104,8 +99,6 @@ function collectReexportedSymbols( db: BetterSqlite3Database, fileNodeId: number, reexportsToStmt: ReturnType, - exportedNodesStmt: ReturnType | null, - hasExportedCol: boolean, getFileLines: (file: string) => string[] | null, buildSymbolResult: (s: NodeRow, fileLines: string[] | null) => any, ) { @@ -113,14 +106,7 @@ function collectReexportedSymbols( const reexportedSymbols: Array & { originFile: string }> = []; for (const reexTarget of reexportTargets) { - let targetExported: NodeRow[]; - if (hasExportedCol) { - targetExported = exportedNodesStmt!.all(reexTarget.file) as NodeRow[]; - } else { - const targetSymbols = findNodesByFile(db, reexTarget.file) as NodeRow[]; - const exportedIds = findCrossFileCallTargets(db, reexTarget.file) as Set; - targetExported = targetSymbols.filter((s) => exportedIds.has(s.id)); - } + const targetExported = findExportedNodesByFile(db, reexTarget.file); for (const s of targetExported) { reexportedSymbols.push({ ...buildSymbolResult(s, getFileLines(reexTarget.file)), @@ -142,28 +128,6 @@ function exportsFileImpl( const fileNodes = findFileNodes(db, `%${target}%`) as NodeRow[]; if (fileNodes.length === 0) return []; - // Detect whether exported column exists (cached per db handle) - let hasExportedCol: boolean; - if (_hasExportedColCache.has(db)) { - hasExportedCol = _hasExportedColCache.get(db)!; - } else { - hasExportedCol = false; - try { - db.prepare('SELECT exported FROM nodes LIMIT 0').raw(true); - hasExportedCol = true; - } catch (e: unknown) { - debug(`exported column not available, using fallback: ${(e as Error).message}`); - } - _hasExportedColCache.set(db, hasExportedCol); - } - - const exportedNodesStmt = hasExportedCol - ? cachedStmt( - _exportedNodesStmtCache, - db, - "SELECT * FROM nodes WHERE file = ? AND kind != 'file' AND exported = 1 ORDER BY line", - ) - : null; // Consumers include real call/construct edges plus `imports-type` edges — // the symbol-level edge emitted for `import type { X }` statements (source // is the importing *file* node, since the import statement references the @@ -199,15 +163,7 @@ function exportsFileImpl( return fileNodes.map((fn) => { const symbols = findNodesByFile(db, fn.file) as NodeRow[]; - let exported: NodeRow[]; - if (hasExportedCol) { - // Use the exported column populated during build - exported = exportedNodesStmt!.all(fn.file) as NodeRow[]; - } else { - // Fallback: symbols that have incoming calls from other files - const exportedIds = findCrossFileCallTargets(db, fn.file) as Set; - exported = symbols.filter((s) => exportedIds.has(s.id)); - } + const exported = findExportedNodesByFile(db, fn.file); const internalCount = symbols.length - exported.length; const buildSymbolResult = (s: NodeRow, fileLines: string[] | null) => { @@ -244,8 +200,6 @@ function exportsFileImpl( db, fn.id, reexportsToStmt, - exportedNodesStmt, - hasExportedCol, getFileLines, buildSymbolResult, ); diff --git a/src/domain/analysis/symbol-lookup.ts b/src/domain/analysis/symbol-lookup.ts index 065c11f4e..90a39934e 100644 --- a/src/domain/analysis/symbol-lookup.ts +++ b/src/domain/analysis/symbol-lookup.ts @@ -3,7 +3,7 @@ import { findAllIncomingEdges, findAllOutgoingEdges, findCallers, - findCrossFileCallTargets, + findExportedNodesByFile, findFileNodes, findImportSources, findImportTargets, @@ -177,9 +177,7 @@ function whereFileImpl(db: BetterSqlite3Database, target: string) { const importedBy = (findImportSources(db, fn.id) as ImportEdgeRow[]).map((r) => r.file); - const exportedIds = findCrossFileCallTargets(db, fn.file) as Set; - - const exported = symbols.filter((s) => exportedIds.has(s.id)).map((s) => s.name); + const exported = findExportedNodesByFile(db, fn.file).map((s) => s.name); return { file: fn.file, diff --git a/src/extractors/javascript.ts b/src/extractors/javascript.ts index 4d189fa4c..7c4b1ea9e 100644 --- a/src/extractors/javascript.ts +++ b/src/extractors/javascript.ts @@ -195,6 +195,64 @@ function handleMethodCapture(c: Record, definitions: Def }); } +/** Node types whose own `name` field is the exported symbol's name. */ +const EXPORT_DECL_KIND: Record = { + function_declaration: 'function', + generator_function_declaration: 'function', + class_declaration: 'class', + abstract_class_declaration: 'class', + interface_declaration: 'interface', + type_alias_declaration: 'type', +}; + +/** + * Push Export entries for the declaration wrapped by an `export` statement. + * Shared by both extraction paths (query-based `handleExportCapture` and + * walk-based `handleExportStmt`) so they can't drift apart on what counts as + * an export — see the "two code paths" gotcha for this extractor. + * + * Named function/class/interface/type declarations carry their own `name` + * field. `export const/let/var …` has no such field — each declarator's value + * is classified the same way `handleVariableDeclarator` classifies it when + * building the matching Definition (function-valued → kind 'function', + * literal/array/object/new-expression-valued `const` → kind 'constant'). + * This predicate must stay identical to the Definition-building one: the + * exported=1 UPDATE it feeds matches DB rows by (name, kind, file, line), so + * a mismatched kind silently no-ops instead of marking the symbol exported (#1728). + */ +function collectExportedDeclarations( + decl: TreeSitterNode, + exportLine: number, + exps: Export[], +): void { + const kind = EXPORT_DECL_KIND[decl.type]; + if (kind) { + const n = decl.childForFieldName('name'); + if (n) exps.push({ name: n.text, kind: kind as Export['kind'], line: exportLine }); + return; + } + if (decl.type !== 'lexical_declaration' && decl.type !== 'variable_declaration') return; + const isConst = decl.text.startsWith('const '); + for (let i = 0; i < decl.childCount; i++) { + const declarator = decl.child(i); + if (declarator?.type !== 'variable_declarator') continue; + const nameN = declarator.childForFieldName('name'); + const valueN = declarator.childForFieldName('value'); + if (nameN?.type !== 'identifier' || !valueN) continue; + const valType = valueN.type; + if ( + valType === 'arrow_function' || + valType === 'function_expression' || + valType === 'function' || + valType === 'generator_function' + ) { + exps.push({ name: nameN.text, kind: 'function', line: exportLine }); + } else if (isConst && isConstantValue(valueN)) { + exps.push({ name: nameN.text, kind: 'constant', line: exportLine }); + } + } +} + /** Handle export_statement capture. */ function handleExportCapture( c: Record, @@ -203,22 +261,7 @@ function handleExportCapture( ): void { const exportLine = nodeStartLine(c.exp_node!); const decl = c.exp_node!.childForFieldName('declaration'); - if (decl) { - const declType = decl.type; - const kindMap: Record = { - function_declaration: 'function', - generator_function_declaration: 'function', - class_declaration: 'class', - abstract_class_declaration: 'class', - interface_declaration: 'interface', - type_alias_declaration: 'type', - }; - const kind = kindMap[declType]; - if (kind) { - const n = decl.childForFieldName('name'); - if (n) exps.push({ name: n.text, kind: kind as Export['kind'], line: exportLine }); - } - } + if (decl) collectExportedDeclarations(decl, exportLine, exps); const source = c.exp_node!.childForFieldName('source') || findChild(c.exp_node!, 'string'); if (source && !decl) { const modPath = source.text.replace(/['"]/g, ''); @@ -1448,22 +1491,7 @@ function handleImportStmt(node: TreeSitterNode, ctx: ExtractorOutput): void { function handleExportStmt(node: TreeSitterNode, ctx: ExtractorOutput): void { const exportLine = nodeStartLine(node); const decl = node.childForFieldName('declaration'); - if (decl) { - const declType = decl.type; - const kindMap: Record = { - function_declaration: 'function', - generator_function_declaration: 'function', - class_declaration: 'class', - abstract_class_declaration: 'class', - interface_declaration: 'interface', - type_alias_declaration: 'type', - }; - const kind = kindMap[declType]; - if (kind) { - const n = decl.childForFieldName('name'); - if (n) ctx.exports.push({ name: n.text, kind: kind as Export['kind'], line: exportLine }); - } - } + if (decl) collectExportedDeclarations(decl, exportLine, ctx.exports); const source = node.childForFieldName('source') || findChild(node, 'string'); if (source && !decl) { const modPath = source.text.replace(/['"]/g, ''); diff --git a/tests/engines/parity.test.ts b/tests/engines/parity.test.ts index 679207e79..f9c5b5c4d 100644 --- a/tests/engines/parity.test.ts +++ b/tests/engines/parity.test.ts @@ -144,6 +144,28 @@ add(1, 2); const MAX_RETRIES = 3; const APP_NAME = "codegraph"; const add = (a, b) => a + b; +`, + }, + { + // Regression guard for #1728: `export const …` must be recognized as an + // export in both engines regardless of the initializer's shape — a bare + // literal, a `new Expr(...)` call, and an arrow function must all + // produce a matching exports entry, and a non-exported const must not. + // + // The object-literal-with-methods shape (e.g. `export const command = { + // execute() {} }`, issue #1728's other repro) is deliberately not + // included here: both engines already agree it's exported (see + // tests/parsers/javascript.test.ts and tests/engines/query-walk-parity.test.ts), + // but they emit its qualified/unqualified method definitions in different + // relative array order — a separate, pre-existing, content-neutral gap + // (see #1818) that this test's unsorted `normalize()` would otherwise trip on. + name: 'JavaScript — exported constants of varying initializer shapes', + file: 'exported-const.js', + code: ` +export const MAX_WALK_DEPTH = 200; +export const PUNCTUATION_TOKENS = new Set([',', ';']); +export const add = (a, b) => a + b; +const INTERNAL = 'not exported'; `, }, { diff --git a/tests/engines/query-walk-parity.test.ts b/tests/engines/query-walk-parity.test.ts index 2b1f3e787..37f660264 100644 --- a/tests/engines/query-walk-parity.test.ts +++ b/tests/engines/query-walk-parity.test.ts @@ -170,6 +170,24 @@ export function serve(port) { listen(port); } export class Server { start() { this.init(); } } +`, + }, + { + // Regression guard for #1728: `export const …` must be recognized as an + // export regardless of the initializer's shape (bare literal, new-expression, + // object-literal-with-methods, arrow function) — both extraction paths must + // agree on the resulting (name, kind, line) exports entries. + name: 'exported const declarations of varying initializer shapes', + file: 'test.js', + code: ` +export const MAX_WALK_DEPTH = 200; +export const PUNCTUATION_TOKENS = new Set([',', ';']); +export const add = (a, b) => a + b; +export const command = { + name: 'info', + execute(args, opts, ctx) {}, +}; +const INTERNAL = 'not exported'; `, }, { diff --git a/tests/integration/queries.test.ts b/tests/integration/queries.test.ts index c76d4ac86..38672ced0 100644 --- a/tests/integration/queries.test.ts +++ b/tests/integration/queries.test.ts @@ -84,6 +84,14 @@ beforeAll(() => { const formatResponse = insertNode(db, 'formatResponse', 'function', 'utils.js', 1); const preAuthenticate = insertNode(db, 'preAuthenticate', 'function', 'utils.js', 10); + // Exported constant with zero incoming edges of any kind — regression fixture + // for #1728: "exported" must reflect the DB's exported=1 column (set by the + // extractor from the source's `export` keyword), not "has an incoming + // cross-file `calls` edge." A value-only export like this is never the + // target of a calls edge, so a cross-file-calls heuristic incorrectly + // omitted it from `where --file`'s exported list. + const apiVersion = insertNode(db, 'API_VERSION', 'constant', 'middleware.js', 1); + // Test file + function const fTest = insertNode(db, 'auth.test.js', 'file', 'auth.test.js', 0); const testAuth = insertNode(db, 'testAuthenticate', 'function', 'auth.test.js', 5); @@ -125,7 +133,14 @@ beforeAll(() => { // Mark exported symbols (those declared as exports in source) const markExported = db.prepare('UPDATE nodes SET exported = 1 WHERE id = ?'); - for (const id of [authenticate, validateToken, authMiddleware, formatResponse, preAuthenticate]) { + for (const id of [ + authenticate, + validateToken, + authMiddleware, + formatResponse, + preAuthenticate, + apiVersion, + ]) { markExported.run(id); } @@ -767,8 +782,14 @@ describe('whereData', () => { test('file: exported list', () => { const data = whereData('middleware.js', dbPath, { file: true }); const r = data.results[0]; - // authMiddleware is called from routes.js (cross-file) + // authMiddleware has exported=1 in the DB (and, incidentally, is also + // called from routes.js — but presence in `exported` must not depend on + // that; see the next assertion). expect(r.exported).toContain('authMiddleware'); + // API_VERSION has exported=1 but no incoming edges of any kind (#1728): + // "exported" reflects the DB's exported=1 column, not "has an incoming + // cross-file `calls` edge" — a value-only export is never call-targeted. + expect(r.exported).toContain('API_VERSION'); }); test('file: empty for unknown', () => { diff --git a/tests/parsers/javascript.test.ts b/tests/parsers/javascript.test.ts index b31040e2a..c825df2a9 100644 --- a/tests/parsers/javascript.test.ts +++ b/tests/parsers/javascript.test.ts @@ -1482,4 +1482,67 @@ describe('JavaScript parser', () => { ); }); }); + + describe('export-list detection for `export const/let/var …` (#1728)', () => { + it('lists named exported function/class declarations (refactor regression guard)', () => { + const symbols = parseJS(`export function greet() {}\nexport class Widget {}`); + expect(symbols.exports).toContainEqual( + expect.objectContaining({ name: 'greet', kind: 'function' }), + ); + expect(symbols.exports).toContainEqual( + expect.objectContaining({ name: 'Widget', kind: 'class' }), + ); + }); + + it('lists an exported const with a bare numeric-literal initializer (repro 1)', () => { + const symbols = parseJS(`export const MAX_WALK_DEPTH = 200;`); + expect(symbols.definitions).toContainEqual( + expect.objectContaining({ name: 'MAX_WALK_DEPTH', kind: 'constant', line: 1 }), + ); + expect(symbols.exports).toContainEqual( + expect.objectContaining({ name: 'MAX_WALK_DEPTH', kind: 'constant', line: 1 }), + ); + }); + + it('lists an exported const initialized with new Set(...) (sibling regression guard)', () => { + const symbols = parseJS(`export const PUNCTUATION_TOKENS = new Set([',', ';']);`); + expect(symbols.exports).toContainEqual( + expect.objectContaining({ name: 'PUNCTUATION_TOKENS', kind: 'constant', line: 1 }), + ); + }); + + it('lists an exported object-literal-with-methods const, without independently exporting its methods (repro 2)', () => { + const symbols = parseJS( + `export const command = {\n name: 'info',\n execute(args, opts, ctx) {},\n};`, + ); + expect(symbols.definitions).toContainEqual( + expect.objectContaining({ name: 'command', kind: 'constant', line: 1 }), + ); + expect(symbols.definitions).toContainEqual( + expect.objectContaining({ name: 'command.execute', kind: 'function' }), + ); + expect(symbols.exports).toContainEqual( + expect.objectContaining({ name: 'command', kind: 'constant', line: 1 }), + ); + // Qualified child methods aren't independently listed as exports — mirrors + // how `Foo.method` isn't exported when only `export class Foo` is (only the + // top-level declared name is; see the class-method exported=0 convention). + expect(symbols.exports.some((e) => e.name === 'command.execute')).toBe(false); + }); + + it('lists an exported arrow-function const with kind "function"', () => { + const symbols = parseJS(`export const add = (a, b) => a + b;`); + expect(symbols.exports).toContainEqual( + expect.objectContaining({ name: 'add', kind: 'function', line: 1 }), + ); + }); + + it('does not list a non-exported const', () => { + const symbols = parseJS(`const INTERNAL = 42;`); + expect(symbols.definitions).toContainEqual( + expect.objectContaining({ name: 'INTERNAL', kind: 'constant' }), + ); + expect(symbols.exports.some((e) => e.name === 'INTERNAL')).toBe(false); + }); + }); }); From 4ead84062278ec31e1d08b26fa962b4026e98c4b Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Sun, 5 Jul 2026 14:56:16 -0600 Subject: [PATCH 6/8] fix: exclude primitive type keywords from ast --kind string matching MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tree-sitter-typescript's predefined_type production (string, number, boolean, ... primitive type keywords) lexes its keyword as an anonymous grammar token whose type string is identical to the named `string` node type used for real string literals. Both engines matched by node type alone, so `codegraph ast --kind string` spuriously matched bare `string` type annotations (interface fields, parameter types, return types) as if they were string literals. Add an isNamed/is_named() guard scoped to the JS/TS/TSX family on both engines: WASM via astRequiresNamedNode() in ast-analysis/rules/index.ts feeding the shared resolveAstKind() in ast-store-visitor.ts, native via a match guard in javascript.rs's walk_ast_nodes_depth. Genuine string, template, and string-literal-type nodes are always named and unaffected. No user-facing command/language/architecture changes, so README/CLAUDE.md/ ROADMAP.md are unaffected — docs check acknowledged. Fixes #1729 Impact: 8 functions changed, 13 affected --- .../src/extractors/javascript.rs | 9 +- src/ast-analysis/engine.ts | 2 + src/ast-analysis/rules/index.ts | 20 ++ .../visitors/ast-store-visitor.ts | 15 +- src/domain/wasm-worker-entry.ts | 2 + src/features/ast.ts | 2 + src/types.ts | 10 + tests/engines/ast-parity.test.ts | 2 + tests/parsers/ast-nodes.test.ts | 229 ++++++++++++++++++ 9 files changed, 288 insertions(+), 3 deletions(-) diff --git a/crates/codegraph-core/src/extractors/javascript.rs b/crates/codegraph-core/src/extractors/javascript.rs index 21d91a8c6..5c343e820 100644 --- a/crates/codegraph-core/src/extractors/javascript.rs +++ b/crates/codegraph-core/src/extractors/javascript.rs @@ -1896,7 +1896,14 @@ fn walk_ast_nodes_depth(node: &Node, source: &[u8], ast_nodes: &mut Vec } return; } - "string" | "template_string" => { + // Guard on `is_named()`: tree-sitter-typescript's `predefined_type` + // production (the `string`/`number`/`boolean`/... primitive type + // keywords) lexes its keyword as an anonymous token whose `kind()` + // string is identical to the *named* `string` literal node type. + // Without this guard, `name: string` type annotations are + // misclassified as string-literal ast_nodes (#1729). Mirrors the + // WASM-side guard in `ast-store-visitor.ts::resolveAstKind`. + "string" | "template_string" if node.is_named() => { let raw = node_text(node, source); // Strip quotes to get content let content = raw diff --git a/src/ast-analysis/engine.ts b/src/ast-analysis/engine.ts index 6edc65413..706df0161 100644 --- a/src/ast-analysis/engine.ts +++ b/src/ast-analysis/engine.ts @@ -45,6 +45,7 @@ import { computeLOCMetrics, computeMaintainabilityIndex } from './metrics.js'; import { AST_STRING_CONFIGS, AST_TYPE_MAPS, + astRequiresNamedNode, astStopRecurseKinds, CFG_RULES, COMPLEXITY_RULES, @@ -475,6 +476,7 @@ function setupAstVisitor( nodeIdMap, stringConfig, astStopRecurseKinds(langId), + astRequiresNamedNode(langId), ); } diff --git a/src/ast-analysis/rules/index.ts b/src/ast-analysis/rules/index.ts index 01a599eb4..298ce6fb0 100644 --- a/src/ast-analysis/rules/index.ts +++ b/src/ast-analysis/rules/index.ts @@ -378,3 +378,23 @@ export function astStopRecurseKinds(langId: string): ReadonlySet { } return EMPTY_STOP_RECURSE; } + +// ─── Per-language "named-node-required" guard ──────────────────────────── +// +// tree-sitter-typescript's `predefined_type` production (the `string`, +// `number`, `boolean`, `void`, ... primitive type keywords) lexes its +// keyword as an anonymous grammar token whose `type` string is identical to +// the *named* `string` node type used for real string literals — the +// grammar's own node-types.json lists both a `"string", named: true` entry +// (the literal) and a `"string", named: false` entry (the keyword token). +// Matching by `node.type` alone therefore misclassifies `name: string` +// type annotations as string-literal ast_nodes (#1729). Mirrors the native +// `is_named()` guard in `extractors/javascript.rs::walk_ast_nodes_depth`. +// +// Scoped to the JS family: no other bundled grammar currently maps an +// AST_TYPE_MAPS key that collides with an anonymous token of the same name. +const JS_REQUIRES_NAMED_NODE: ReadonlySet = new Set(['javascript', 'typescript', 'tsx']); + +export function astRequiresNamedNode(langId: string): boolean { + return JS_REQUIRES_NAMED_NODE.has(langId); +} diff --git a/src/ast-analysis/visitors/ast-store-visitor.ts b/src/ast-analysis/visitors/ast-store-visitor.ts index dd63515be..963840f9f 100644 --- a/src/ast-analysis/visitors/ast-store-visitor.ts +++ b/src/ast-analysis/visitors/ast-store-visitor.ts @@ -273,9 +273,19 @@ function collectNode(ctx: CollectCtx, node: TreeSitterNode, kind: string): void * Object() function), which then crashes the worker boundary with * "function Object() { [native code] } could not be cloned" when the * resulting astNodes row is structured-cloned back to the main thread. + * + * When `requireNamedNode` is set, anonymous grammar tokens are rejected even + * if their `type` string matches a mapped key — e.g. TypeScript's + * `predefined_type` keyword (`string`, `number`, ...) lexes as an unnamed + * token whose type collides with the named `string` literal node (#1729). */ -function resolveAstKind(node: TreeSitterNode, astTypeMap: Record): string | null { +function resolveAstKind( + node: TreeSitterNode, + astTypeMap: Record, + requireNamedNode: boolean, +): string | null { if (!Object.hasOwn(astTypeMap, node.type)) return null; + if (requireNamedNode && !node.isNamed) return null; return astTypeMap[node.type] || null; } @@ -286,6 +296,7 @@ export function createAstStoreVisitor( nodeIdMap: Map, stringConfig: AstStringConfig = DEFAULT_STRING_CONFIG, stopRecurseKinds: ReadonlySet = new Set(), + requireNamedNode = false, ): Visitor { const newTypes = newTypesFor(astTypeMap); // When nodeIdMap is empty, parentNodeId resolution is wasted work — the @@ -311,7 +322,7 @@ export function createAstStoreVisitor( // unrelated subtree. The parent call's skipChildren handles the intended case. if (ctx.matched.has(node.id)) return; - const kind = resolveAstKind(node, astTypeMap); + const kind = resolveAstKind(node, astTypeMap, requireNamedNode); if (!kind) return; collectNode(ctx, node, kind); diff --git a/src/domain/wasm-worker-entry.ts b/src/domain/wasm-worker-entry.ts index d55f5f7e6..2e02b4ed8 100644 --- a/src/domain/wasm-worker-entry.ts +++ b/src/domain/wasm-worker-entry.ts @@ -30,6 +30,7 @@ import { computeLOCMetrics, computeMaintainabilityIndex } from '../ast-analysis/ import { AST_STRING_CONFIGS, AST_TYPE_MAPS, + astRequiresNamedNode, astStopRecurseKinds, CFG_RULES, COMPLEXITY_RULES, @@ -606,6 +607,7 @@ function buildAstVisitor( new Map(), stringConfig, astStopRecurseKinds(langId), + astRequiresNamedNode(langId), ); } diff --git a/src/features/ast.ts b/src/features/ast.ts index da9aff195..d227b4bcc 100644 --- a/src/features/ast.ts +++ b/src/features/ast.ts @@ -2,6 +2,7 @@ import path from 'node:path'; import { AST_STRING_CONFIGS, AST_TYPE_MAPS, + astRequiresNamedNode, astStopRecurseKinds, } from '../ast-analysis/rules/index.js'; import { buildExtensionSet } from '../ast-analysis/shared.js'; @@ -244,6 +245,7 @@ function walkAst( nodeIdMap, stringConfig, astStopRecurseKinds(langId), + astRequiresNamedNode(langId), ); const results = walkWithVisitors(rootNode, [visitor], langId); const collected = (results['ast-store'] || []) as AstRow[]; diff --git a/src/types.ts b/src/types.ts index 8374a2a0c..81fafbe03 100644 --- a/src/types.ts +++ b/src/types.ts @@ -777,6 +777,16 @@ export interface TreeSitterNode { id: number; type: string; text: string; + /** + * Whether this node is a named grammar production vs. an anonymous token + * (keyword/punctuation). Tree-sitter grammars can define an anonymous + * token whose text matches a *named* node's type string — e.g. + * tree-sitter-typescript's `predefined_type` wraps an anonymous `string` + * keyword token that collides with the named `string` literal node type + * (#1729). Consumers matching by `type` alone must also check `isNamed` + * to avoid misclassifying keyword tokens as literal/expression nodes. + */ + isNamed: boolean; startPosition: { row: number; column: number }; endPosition: { row: number; column: number }; childCount: number; diff --git a/tests/engines/ast-parity.test.ts b/tests/engines/ast-parity.test.ts index a73c5852f..4e4e50192 100644 --- a/tests/engines/ast-parity.test.ts +++ b/tests/engines/ast-parity.test.ts @@ -11,6 +11,7 @@ import { beforeAll, describe, expect, it } from 'vitest'; import { AST_STRING_CONFIGS, AST_TYPE_MAPS, + astRequiresNamedNode, astStopRecurseKinds, } from '../../src/ast-analysis/rules/index.js'; import { walkWithVisitors } from '../../src/ast-analysis/visitor.js'; @@ -420,6 +421,7 @@ let run () = new Map(), stringConfig, astStopRecurseKinds(langId), + astRequiresNamedNode(langId), ); const results = walkWithVisitors(tree.rootNode as any, [visitor], langId); const rows = (results['ast-store'] || []) as unknown[]; diff --git a/tests/parsers/ast-nodes.test.ts b/tests/parsers/ast-nodes.test.ts index 4ebd79f7d..b6d31a503 100644 --- a/tests/parsers/ast-nodes.test.ts +++ b/tests/parsers/ast-nodes.test.ts @@ -182,6 +182,168 @@ describe('buildAstNodes — JS extraction', () => { }); }); +// ─── TypeScript fixture (#1729: predefined_type keyword false-positives) ── +// +// tree-sitter-typescript's `predefined_type` production (the `string`, +// `number`, `boolean`, ... primitive type keywords) lexes its keyword as an +// anonymous token whose `type` string collides with the *named* `string` +// literal node type. `--kind string` must only match the latter. + +const TS_FIXTURE_CODE = ` +interface Person { + name: string; + age: number; +} + +type UserId = "user-id-literal"; + +function greet(name: string): string { + return \`Hello, \${name}!\`; +} + +function processItems(items: string[]): void { + console.log(items); +} + +import { helper } from './helper.js'; + +const greeting = "hello world"; +`; + +describe('buildAstNodes — TypeScript extraction (#1729)', () => { + let tsTmpDir: string, tsDb: any; + + function queryTsAstNodes(kind: string) { + return tsDb.prepare('SELECT * FROM ast_nodes WHERE kind = ? ORDER BY line').all(kind); + } + + beforeAll(async () => { + tsTmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-ast-ts-extract-')); + const srcDir = path.join(tsTmpDir, 'src'); + fs.mkdirSync(srcDir, { recursive: true }); + fs.mkdirSync(path.join(tsTmpDir, '.codegraph')); + + const fixturePath = path.join(srcDir, 'fixture.ts'); + fs.writeFileSync(fixturePath, TS_FIXTURE_CODE); + + const allSymbols = await parseFilesAuto([fixturePath], tsTmpDir, { engine: 'wasm' }); + const symbols = allSymbols.get('src/fixture.ts'); + if (!symbols) throw new Error('Failed to parse TS fixture file'); + + const dbPath = path.join(tsTmpDir, '.codegraph', 'graph.db'); + tsDb = new Database(dbPath); + tsDb.pragma('journal_mode = WAL'); + initSchema(tsDb); + + const insertNode = tsDb.prepare( + 'INSERT INTO nodes (name, kind, file, line, end_line) VALUES (?, ?, ?, ?, ?)', + ); + for (const def of symbols.definitions) { + insertNode.run(def.name, def.kind, 'src/fixture.ts', def.line, def.endLine); + } + + await buildAstNodes(tsDb, allSymbols, tsTmpDir); + }); + + afterAll(() => { + if (tsDb) tsDb.close(); + fs.rmSync(tsTmpDir, { recursive: true, force: true }); + }); + + test('does not misclassify interface field type annotation as kind:string', () => { + // `name: string;` (line 3) must never surface as a bare, unquoted "string" + // row — genuine literals are always quoted in `text` (e.g. "hello world"). + const nodes = queryTsAstNodes('string'); + expect(nodes.some((n) => n.text === 'string')).toBe(false); + }); + + test('does not misclassify parameter or return type annotations as kind:string', () => { + // `greet(name: string): string` (param + return type, line 9) must not + // contribute bare "string" rows. + const nodes = queryTsAstNodes('string'); + expect(nodes.filter((n) => n.text === 'string').length).toBe(0); + }); + + test('does not misclassify array-of-primitive parameter type as kind:string', () => { + // `processItems(items: string[])` (line 13) wraps predefined_type in + // array_type — must not contribute a bare "string" row either. + const nodes = queryTsAstNodes('string'); + expect(nodes.some((n) => n.line === 13)).toBe(false); + }); + + test('still captures genuine string literals, template literals, and string-literal types', () => { + const nodes = queryTsAstNodes('string'); + const names = nodes.map((n) => n.name); + expect(names).toContain('user-id-literal'); // type UserId = "user-id-literal" + expect(names).toContain('./helper.js'); // import source path + expect(names).toContain('hello world'); // genuine string literal + expect(nodes.some((n) => n.text?.startsWith('`Hello, '))).toBe(true); // template literal + }); + + test('captures exactly the 4 genuine literals — no keyword false-positives', () => { + const nodes = queryTsAstNodes('string'); + expect(nodes.length).toBe(4); + }); +}); + +// ─── TSX fixture (#1729) — shares JS_AST_TYPES + grammar family with TS ─── + +const TSX_FIXTURE_CODE = ` +interface Props { + label: string; +} + +function Greeting(props: Props) { + return
{"hello world"}
; +} +`; + +describe('buildAstNodes — TSX extraction (#1729)', () => { + let tsxTmpDir: string, tsxDb: any; + + beforeAll(async () => { + tsxTmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-ast-tsx-extract-')); + const srcDir = path.join(tsxTmpDir, 'src'); + fs.mkdirSync(srcDir, { recursive: true }); + fs.mkdirSync(path.join(tsxTmpDir, '.codegraph')); + + const fixturePath = path.join(srcDir, 'fixture.tsx'); + fs.writeFileSync(fixturePath, TSX_FIXTURE_CODE); + + const allSymbols = await parseFilesAuto([fixturePath], tsxTmpDir, { engine: 'wasm' }); + const symbols = allSymbols.get('src/fixture.tsx'); + if (!symbols) throw new Error('Failed to parse TSX fixture file'); + + const dbPath = path.join(tsxTmpDir, '.codegraph', 'graph.db'); + tsxDb = new Database(dbPath); + tsxDb.pragma('journal_mode = WAL'); + initSchema(tsxDb); + + const insertNode = tsxDb.prepare( + 'INSERT INTO nodes (name, kind, file, line, end_line) VALUES (?, ?, ?, ?, ?)', + ); + for (const def of symbols.definitions) { + insertNode.run(def.name, def.kind, 'src/fixture.tsx', def.line, def.endLine); + } + + await buildAstNodes(tsxDb, allSymbols, tsxTmpDir); + }); + + afterAll(() => { + if (tsxDb) tsxDb.close(); + fs.rmSync(tsxTmpDir, { recursive: true, force: true }); + }); + + test('does not misclassify interface field type annotation as kind:string, still captures JSX-embedded literal', () => { + const nodes = tsxDb + .prepare('SELECT * FROM ast_nodes WHERE kind = ? ORDER BY line') + .all('string'); + expect(nodes.some((n) => n.text === 'string')).toBe(false); + expect(nodes.length).toBe(1); + expect(nodes[0].name).toBe('hello world'); + }); +}); + // ─── Native engine AST node extraction ─────────────────────────────── // Check if native addon is available AND supports ast_nodes. @@ -308,3 +470,70 @@ describe.skipIf(!canTestNative)('buildAstNodes — native engine', () => { expect(withParent.length).toBeGreaterThan(0); }); }); + +// ─── Native engine: TypeScript predefined_type false-positives (#1729) ──── + +describe.skipIf(!canTestNative)('buildAstNodes — native TypeScript extraction (#1729)', () => { + let nativeTsTmpDir: string, nativeTsDb: any; + + function queryNativeTsAstNodes(kind: string) { + return nativeTsDb.prepare('SELECT * FROM ast_nodes WHERE kind = ? ORDER BY line').all(kind); + } + + beforeAll(async () => { + nativeTsTmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-ast-ts-native-')); + const srcDir = path.join(nativeTsTmpDir, 'src'); + fs.mkdirSync(srcDir, { recursive: true }); + fs.mkdirSync(path.join(nativeTsTmpDir, '.codegraph')); + + const fixturePath = path.join(srcDir, 'fixture.ts'); + fs.writeFileSync(fixturePath, TS_FIXTURE_CODE); + + const allSymbols = await parseFilesAuto([fixturePath], nativeTsTmpDir, { engine: 'native' }); + const symbols = allSymbols.get('src/fixture.ts'); + if (!symbols) throw new Error('Failed to parse TS fixture file with native engine'); + + const dbPath = path.join(nativeTsTmpDir, '.codegraph', 'graph.db'); + nativeTsDb = new Database(dbPath); + nativeTsDb.pragma('journal_mode = WAL'); + initSchema(nativeTsDb); + + const insertNode = nativeTsDb.prepare( + 'INSERT INTO nodes (name, kind, file, line, end_line) VALUES (?, ?, ?, ?, ?)', + ); + for (const def of symbols.definitions) { + insertNode.run(def.name, def.kind, 'src/fixture.ts', def.line, def.endLine); + } + + await buildAstNodes(nativeTsDb, allSymbols, nativeTsTmpDir); + }); + + afterAll(() => { + if (nativeTsDb) nativeTsDb.close(); + if (nativeTsTmpDir) fs.rmSync(nativeTsTmpDir, { recursive: true, force: true }); + }); + + test('does not misclassify interface field type annotation as kind:string', () => { + const nodes = queryNativeTsAstNodes('string'); + expect(nodes.some((n) => n.text === 'string')).toBe(false); + }); + + test('does not misclassify parameter, return, or array-element type annotations as kind:string', () => { + const nodes = queryNativeTsAstNodes('string'); + expect(nodes.filter((n) => n.text === 'string').length).toBe(0); + }); + + test('still captures genuine string literals, template literals, and string-literal types', () => { + const nodes = queryNativeTsAstNodes('string'); + const names = nodes.map((n) => n.name); + expect(names).toContain('user-id-literal'); + expect(names).toContain('./helper.js'); + expect(names).toContain('hello world'); + expect(nodes.some((n) => n.text?.startsWith('`Hello, '))).toBe(true); + }); + + test('captures exactly the 4 genuine literals — no keyword false-positives', () => { + const nodes = queryNativeTsAstNodes('string'); + expect(nodes.length).toBe(4); + }); +}); From f6326bff7b3925ce80edf0e6ca0271b5efbdc0e7 Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Sun, 5 Jul 2026 15:45:20 -0600 Subject: [PATCH 7/8] fix: resolve call edges through renamed import specifiers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit codegraph exports reported zero consumers for an exported function/symbol that is genuinely imported and used elsewhere, whenever the importing file renamed the binding at the import site (import { X as Y } from '...'). Example: collectFiles in src/domain/graph/builder/helpers.ts is imported (as collectFilesUtil) and called by collect-files.ts and native-orchestrator.ts, but `codegraph exports helpers.ts -T --json` showed consumerCount: 0. Root cause (extractor, both engines): extractImportNames (javascript.ts) and its mirrored scan_import_names_depth (javascript.rs) handled import_specifier nodes with `node.childForFieldName('name') || childForFieldName('alias')`. Per the tree-sitter grammar, `name` is *always* present on import_specifier (the name as declared by the source module); `alias` only exists for `X as Y` and holds the *local* binding actually referenced by call sites. Preferring `name` unconditionally meant the local alias was silently dropped for every renamed import — `imp.names` recorded "collectFiles" instead of "collectFilesUtil", so `importedNames` (keyed by call-site text) never had a matching entry and the call fell through unresolved. Fixing only the extractor was not sufficient: once `imp.names` correctly holds the local alias, `resolveCallTargets` (call-resolver.ts) / resolve_call_targets (build_edges.rs) still searched the *target file* for a symbol literally named "collectFilesUtil" — which doesn't exist there (only "collectFiles" does). This required threading a second piece of information end-to-end: for each renamed specifier, the local alias's *original* exported name, so target-file/barrel lookups search by the right name while importedNames stays keyed by the call-site text. Changes (both engines, full-build + incremental/native-orchestrator + native-hybrid paths): - types.ts / types.rs: new `Import.renamedImports` / `Import.renamed_imports` field — { local, imported } pairs, populated only for specifiers that actually rename a binding. - extractors/javascript.ts, .rs: extractImportNames / extract_import_names_with_renames now prefer `alias` (local binding) for import_specifier and record the rename pair. export_specifier is deliberately left unchanged (see scope notes below). Fixed in both the walk path (handleImportStmt) and the query/WASM-worker path (handleImportCapture) on the TS side. - build-edges.ts, build_edges.rs, import_edges.rs, pipeline.rs, call-resolver.ts, incremental.ts: buildImportedNamesMap / collect_imported_names_for_file now also produce a local-alias -> original-name map; resolveCallTargets / resolve_call_targets consult it to search the target file by the original name. Applied consistently to the primary call-resolution path, the Object.defineProperty post-pass, points-to alias resolution, barrel edge / import-type edge emission, and cross-file return-type propagation — every place that previously assumed a call site's local name equals the target file's declared name. Native Rust: touched and verified. Rebuilt locally via `npx napi build --platform --release` (crates/codegraph-core), codesigned (`codesign --sign - --force`), and installed over the loaded node_modules/@optave/codegraph-darwin-arm64/codegraph-core.node so the fix was exercised by the actual native engine, not the prebuilt npm binary. `cargo test --lib`: 427 passed. Tests added: - tests/parsers/javascript.test.ts: extractor-level coverage for the local-name/rename-pair extraction, a mixed renamed+non-renamed specifier list, and confirmation export_specifier is unaffected. - crates/codegraph-core/src/extractors/javascript.rs: matching Rust unit tests. - tests/integration/issue-1730-renamed-import-consumer.test.ts: new end-to-end test building real files through buildGraph() on both wasm and native engines, asserting the calls edge exists, no edge is created against a nonexistent "collectFilesUtil" symbol, and `codegraph exports` credits the consumer — on both engines. tests/integration/exports.test.ts was not extended: that file hand-inserts DB rows and only exercises the query layer, which is unaffected by this bug (the missing piece was edge creation, not the consumer query added for #1724). Verified against this repo's own graph (native engine, rebuilt): `codegraph exports src/domain/graph/builder/helpers.ts -T --json` now credits collectFiles with 2 consumers (collect-files.ts, native-orchestrator.ts); WASM engine agrees exactly. Without -T, also picks up tests/unit/builder.test.ts via the (unrenamed) builder.ts barrel re-export, which already worked pre-fix and was only hidden by -T in the original repro. Scope notes — filed as separate issues rather than expanding this fix: - #1823: export { X as Y } from '...' (barrel re-export with rename) is not tracked — resolveBarrelExport/resolve_barrel_export key off the original name, and export_specifier extraction was deliberately left unchanged here since barrel/reexport tracing is a distinct mechanism. - #1824: dynamic import destructuring rename (const { a: b } = await import(...)) has the same class of bug in extractDynamicImportNames, a different extraction function. - #1825: a renamed import used as a call receiver (import { X as Y }; Y.method()) still fails to resolve — confirmed empirically (no calls edge created) — a different resolution strategy (resolveByReceiver/resolveViaDirectQualifiedMethod in resolver/strategy.ts) than the one this fix addresses. Full test suite (202 files, 3398 tests) and lint pass clean. docs check acknowledged: internal resolver/extractor bug fix, no new feature/language/CLI surface/architecture change -- README.md, CLAUDE.md, and ROADMAP.md are unaffected. Fixes #1730 Impact: 29 functions changed, 47 affected --- .../src/domain/graph/builder/pipeline.rs | 38 +++-- .../graph/builder/stages/build_edges.rs | 38 ++++- .../graph/builder/stages/import_edges.rs | 51 ++++--- .../src/extractors/javascript.rs | 102 ++++++++++++- crates/codegraph-core/src/types.rs | 20 +++ src/domain/graph/builder/call-resolver.ts | 11 +- src/domain/graph/builder/incremental.ts | 89 +++++++++--- .../graph/builder/stages/build-edges.ts | 134 +++++++++++++----- src/extractors/javascript.ts | 47 +++++- src/types.ts | 11 ++ ...issue-1730-renamed-import-consumer.test.ts | 125 ++++++++++++++++ tests/parsers/javascript.test.ts | 46 ++++++ 12 files changed, 618 insertions(+), 94 deletions(-) create mode 100644 tests/integration/issue-1730-renamed-import-consumer.test.ts diff --git a/crates/codegraph-core/src/domain/graph/builder/pipeline.rs b/crates/codegraph-core/src/domain/graph/builder/pipeline.rs index 53f05601d..f191b9f66 100644 --- a/crates/codegraph-core/src/domain/graph/builder/pipeline.rs +++ b/crates/codegraph-core/src/domain/graph/builder/pipeline.rs @@ -1277,17 +1277,22 @@ fn load_file_node_id_map(conn: &Connection) -> HashMap { /// Resolve a file's imports to the list of `ImportedName` entries the edge /// builder consumes. Walks barrel chains to the ultimate definition file so /// the edge builder's name-lookup can find the right target (#976 P1). +/// +/// For renamed specifiers (`import { X as Y }`), `ImportedName.imported` +/// carries the original name (X) so `resolve_call_targets` can look it up in +/// the target file instead of the local alias (Y), which only exists in the +/// importing file (#1730). fn collect_imported_names_for_file( abs_str: &str, symbols: &FileSymbols, import_ctx: &crate::domain::graph::builder::stages::import_edges::ImportEdgeContext, ) -> Vec { use crate::domain::graph::builder::stages::build_edges::ImportedName; + use crate::domain::graph::builder::stages::import_edges::import_name_pairs; let mut imported_names: Vec = Vec::new(); for imp in &symbols.imports { let resolved_path = import_ctx.get_resolved(abs_str, &imp.source); - for name in &imp.names { - let clean_name = name.strip_prefix("* as ").unwrap_or(name).to_string(); + for (local, original) in import_name_pairs(imp) { // CJS require bindings are included in imported_names so the receiver-edge // resolver treats them as import artifacts (not locally-defined symbols). // We use an empty target_file so the import-aware call-target lookup @@ -1295,20 +1300,25 @@ fn collect_imported_names_for_file( // through to the same-file shadow node — matching WASM call-resolution // behaviour where CJS bindings are not in importedNamesMap (#1678). if imp.cjs_require.unwrap_or(false) { - imported_names.push(ImportedName { name: clean_name, file: String::new() }); + imported_names.push(ImportedName { + name: local, + file: String::new(), + imported: None, + }); continue; } let mut target_file = resolved_path.clone(); if import_ctx.is_barrel_file(&resolved_path) { let mut visited = HashSet::new(); if let Some(actual) = - import_ctx.resolve_barrel_export(&resolved_path, &clean_name, &mut visited) + import_ctx.resolve_barrel_export(&resolved_path, &original, &mut visited) { target_file = actual; } } imported_names.push(ImportedName { - name: clean_name, + imported: if original != local { Some(original) } else { None }, + name: local, file: target_file, }); } @@ -1411,8 +1421,14 @@ fn inject_return_types_for_file( let imported_names = collect_imported_names_for_file(abs_str, symbols, import_ctx); // Later entries overwrite earlier ones on duplicate names — same as the // HashMap collect in build_call_edges. - let imported_map: HashMap = - imported_names.into_iter().map(|e| (e.name, e.file)).collect(); + let mut imported_map: HashMap = HashMap::new(); + let mut imported_original_map: HashMap = HashMap::new(); + for e in imported_names { + if let Some(original) = e.imported { + imported_original_map.insert(e.name.clone(), original); + } + imported_map.insert(e.name, e.file); + } let mut injections: Vec = Vec::new(); let mut injected: HashSet = HashSet::new(); @@ -1429,7 +1445,13 @@ fn inject_return_types_for_file( global_return_types.get(&format!("{receiver}.{}", ca.callee_name)) } None => imported_map.get(&ca.callee_name).and_then(|from| { - return_type_index.get(from).and_then(|m| m.get(&ca.callee_name)) + // The return-type index for the imported file is keyed by the + // function's own declared name — use the original (pre-rename) + // name when the callee is a renamed import binding (#1730). + let callee_original_name = imported_original_map + .get(&ca.callee_name) + .unwrap_or(&ca.callee_name); + return_type_index.get(from).and_then(|m| m.get(callee_original_name)) }), }; diff --git a/crates/codegraph-core/src/domain/graph/builder/stages/build_edges.rs b/crates/codegraph-core/src/domain/graph/builder/stages/build_edges.rs index d86c42872..8d14f7e4d 100644 --- a/crates/codegraph-core/src/domain/graph/builder/stages/build_edges.rs +++ b/crates/codegraph-core/src/domain/graph/builder/stages/build_edges.rs @@ -46,6 +46,12 @@ pub struct CallInfo { pub struct ImportedName { pub name: String, pub file: String, + /// For renamed specifiers (`import { X as Y }`): the original name + /// exported by `file` (X), when it differs from `name` (the local + /// binding Y). `resolve_call_targets` looks this up in `file` instead of + /// `name` — the renamed local alias only exists in the importing file, + /// not in `file` itself (#1730). + pub imported: Option, } #[napi(object)] @@ -384,6 +390,7 @@ struct PtsAliasCtx<'a> { is_dynamic: u32, rel_path: &'a str, imported_names: &'a HashMap<&'a str, &'a str>, + imported_original_names: &'a HashMap<&'a str, &'a str>, type_map: &'a HashMap<&'a str, (&'a str, f64)>, } @@ -399,6 +406,7 @@ fn emit_pts_alias_edges<'a>( ) { for alias in resolve_via_points_to(alias_ctx.lookup_name, alias_ctx.pts) { let alias_imported_from = alias_ctx.imported_names.get(alias).copied(); + let alias_imported_original_name = alias_ctx.imported_original_names.get(alias).copied(); let alias_call = CallInfo { name: alias.to_string(), line: alias_ctx.call_line, @@ -409,6 +417,7 @@ fn emit_pts_alias_edges<'a>( }; let mut alias_targets = resolve_call_targets( ctx, &alias_call, alias_ctx.rel_path, alias_imported_from, alias_ctx.type_map, alias_ctx.caller_name, + alias_imported_original_name, ); sort_targets_by_confidence(&mut alias_targets, alias_ctx.rel_path, alias_imported_from); for t in &alias_targets { @@ -458,6 +467,11 @@ struct FileContext<'a> { rel_path: &'a str, file_node_id: u32, imported_names: HashMap<&'a str, &'a str>, + /// Local import alias -> original exported name, for renamed specifiers + /// (`import { X as Y }`) only — entries where local === original are + /// omitted. Consulted by `resolve_call_targets` so a call to the local + /// alias resolves against the correct exported symbol (#1730). + imported_original_names: HashMap<&'a str, &'a str>, type_map: HashMap<&'a str, (&'a str, f64)>, defs_with_ids: Vec>, pts_map: Option>>, @@ -567,6 +581,10 @@ fn build_file_context<'a>( .imported_names.iter() .map(|im| (im.name.as_str(), im.file.as_str())) .collect(); + let imported_original_names: HashMap<&str, &str> = file_input + .imported_names.iter() + .filter_map(|im| im.imported.as_deref().map(|orig| (im.name.as_str(), orig))) + .collect(); let type_map = build_type_map(file_input); let file_nodes: Vec<&NodeInfo> = all_nodes.iter().filter(|n| n.file == rel_path).collect(); let defs_with_ids: Vec = file_input.definitions.iter().map(|d| { @@ -590,6 +608,7 @@ fn build_file_context<'a>( rel_path, file_node_id: file_input.file_node_id, imported_names, + imported_original_names, type_map, defs_with_ids, pts_map, @@ -655,6 +674,7 @@ fn emit_no_receiver_pts_edges<'a>( is_dynamic, rel_path: fc.rel_path, imported_names: &fc.imported_names, + imported_original_names: &fc.imported_original_names, type_map: &fc.type_map, }, seen_edges, @@ -697,6 +717,7 @@ fn emit_receiver_pts_edges<'a>( is_dynamic, rel_path: fc.rel_path, imported_names: &fc.imported_names, + imported_original_names: &fc.imported_original_names, type_map: &fc.type_map, }, seen_edges, @@ -734,8 +755,11 @@ fn process_file<'a>( let (caller_id, caller_name) = find_enclosing_caller(&fc.defs_with_ids, call.line, fc.file_node_id); let is_dynamic = if call.dynamic.unwrap_or(false) { 1u32 } else { 0u32 }; let imported_from = fc.imported_names.get(call.name.as_str()).copied(); + let imported_original_name = fc.imported_original_names.get(call.name.as_str()).copied(); - let mut targets = resolve_call_targets(ctx, call, fc.rel_path, imported_from, &fc.type_map, caller_name); + let mut targets = resolve_call_targets( + ctx, call, fc.rel_path, imported_from, &fc.type_map, caller_name, imported_original_name, + ); sort_targets_by_confidence(&mut targets, fc.rel_path, imported_from); emit_call_edges(&targets, caller_id, is_dynamic, fc.rel_path, imported_from, &mut seen_edges, &mut pts_edge_map, edges); @@ -882,6 +906,7 @@ fn resolve_call_targets<'a>( imported_from: Option<&str>, type_map: &HashMap<&str, (&str, f64)>, caller_name: &str, + imported_original_name: Option<&str>, ) -> Vec<&'a NodeInfo> { // Flagged dynamic calls use synthetic names like "". Short-circuit // so they never accidentally match a real symbol via name lookup. @@ -889,10 +914,15 @@ fn resolve_call_targets<'a>( return vec![]; } + // When the call site uses a renamed import binding (`import { X as Y }`), + // the imported file's actual symbol is declared under the *original* name + // (X) — look that up instead of the local alias the call site wrote (#1730). + let target_name = imported_original_name.unwrap_or(call.name.as_str()); + // 1. Import-aware resolution if let Some(imp_file) = imported_from { let targets = ctx.nodes_by_name_and_file - .get(&(call.name.as_str(), imp_file)) + .get(&(target_name, imp_file)) .cloned().unwrap_or_default(); if !targets.is_empty() { return targets; } } @@ -2081,7 +2111,7 @@ mod call_edge_tests { ); // Mark `Calculator` as an imported name so the resolver treats the // same-file kind="function" node as an import artifact and falls through. - file.imported_names = vec![ImportedName { name: "Calculator".to_string(), file: "utils.js".to_string() }]; + file.imported_names = vec![ImportedName { name: "Calculator".to_string(), file: "utils.js".to_string(), imported: None }]; let edges = build_call_edges(vec![file], all_nodes, vec![]); @@ -2464,7 +2494,7 @@ mod call_edge_tests { vec![], vec![], ); - file.imported_names = vec![ImportedName { name: "e4".to_string(), file: "other.js".to_string() }]; + file.imported_names = vec![ImportedName { name: "e4".to_string(), file: "other.js".to_string(), imported: None }]; file.param_bindings = Some(vec![ParamBinding { callee: "f3".to_string(), arg_index: 0, diff --git a/crates/codegraph-core/src/domain/graph/builder/stages/import_edges.rs b/crates/codegraph-core/src/domain/graph/builder/stages/import_edges.rs index fd09d4a2f..c8f0487e8 100644 --- a/crates/codegraph-core/src/domain/graph/builder/stages/import_edges.rs +++ b/crates/codegraph-core/src/domain/graph/builder/stages/import_edges.rs @@ -115,6 +115,29 @@ impl BarrelContext for ImportEdgeContext { } } +/// Pairs each locally-bound name from an import statement with its original +/// (pre-rename) exported name — identical to the local name unless the +/// specifier renames a binding (`import { X as Y }`). Barrel tracing and +/// target-file symbol lookups must search using the *original* name — the +/// renamed local alias only exists in the importing file, not in the file +/// being imported from (#1730). Mirrors `importNamePairs` in build-edges.ts. +pub(crate) fn import_name_pairs(imp: &crate::types::Import) -> Vec<(String, String)> { + let mut original_name_for: HashMap<&str, &str> = HashMap::new(); + if let Some(renamed) = &imp.renamed_imports { + for r in renamed { + original_name_for.insert(&r.local, &r.imported); + } + } + imp.names + .iter() + .map(|name| { + let local = name.strip_prefix("* as ").unwrap_or(name); + let original = original_name_for.get(local).copied().unwrap_or(local); + (local.to_string(), original.to_string()) + }) + .collect() +} + /// Build the reexport map from parsed file symbols. pub fn build_reexport_map(ctx: &ImportEdgeContext) -> HashMap> { let mut reexport_map = HashMap::new(); @@ -249,18 +272,17 @@ fn collect_type_only_lookup_pairs(ctx: &ImportEdgeContext) -> HashSet<(String, S continue; } let resolved_path = ctx.get_resolved(abs_str, &imp.source); - for name in &imp.names { - let clean_name = name.strip_prefix("* as ").unwrap_or(name); + for (_local, original) in import_name_pairs(imp) { let mut target_file = resolved_path.clone(); if ctx.is_barrel_file(&resolved_path) { let mut visited = HashSet::new(); if let Some(actual) = - ctx.resolve_barrel_export(&resolved_path, clean_name, &mut visited) + ctx.resolve_barrel_export(&resolved_path, &original, &mut visited) { target_file = actual; } } - pairs.insert((clean_name.to_string(), target_file)); + pairs.insert((original, target_file)); } } } @@ -303,18 +325,16 @@ fn emit_type_only_symbol_rows( if !imp.type_only.unwrap_or(false) { return; } - for name in &imp.names { - let clean_name = name.strip_prefix("* as ").unwrap_or(name); + for (_local, original) in import_name_pairs(imp) { let mut target_file = resolved_path.to_string(); if ctx.is_barrel_file(resolved_path) { let mut visited = HashSet::new(); - if let Some(actual) = - ctx.resolve_barrel_export(resolved_path, clean_name, &mut visited) + if let Some(actual) = ctx.resolve_barrel_export(resolved_path, &original, &mut visited) { target_file = actual; } } - if let Some(&sym_id) = symbol_node_ids.get(&(clean_name.to_string(), target_file)) { + if let Some(&sym_id) = symbol_node_ids.get(&(original, target_file)) { edges.push(EdgeRow { source_id: file_node_id, target_id: sym_id, @@ -347,14 +367,13 @@ fn emit_barrel_through_rows( _ => "imports", }; let mut resolved_sources: HashSet = HashSet::new(); - for name in &imp.names { - let clean_name = name.strip_prefix("* as ").unwrap_or(name); + for (_local, original) in import_name_pairs(imp) { let mut visited = HashSet::new(); - let actual_source = - match ctx.resolve_barrel_export(resolved_path, clean_name, &mut visited) { - Some(s) => s, - None => continue, - }; + let actual_source = match ctx.resolve_barrel_export(resolved_path, &original, &mut visited) + { + Some(s) => s, + None => continue, + }; if actual_source == resolved_path || !resolved_sources.insert(actual_source.clone()) { continue; } diff --git a/crates/codegraph-core/src/extractors/javascript.rs b/crates/codegraph-core/src/extractors/javascript.rs index 5c343e820..daf2ff9d3 100644 --- a/crates/codegraph-core/src/extractors/javascript.rs +++ b/crates/codegraph-core/src/extractors/javascript.rs @@ -1672,11 +1672,15 @@ fn handle_import_stmt(node: &Node, source: &[u8], symbols: &mut FileSymbols) { if let Some(source_node) = source_node { let mod_path = node_text(&source_node, source) .replace(&['\'', '"'][..], ""); - let names = extract_import_names(node, source); + let mut renamed_imports = Vec::new(); + let names = extract_import_names_with_renames(node, source, &mut renamed_imports); let mut imp = Import::new(mod_path, names, start_line(node)); if is_type_only { imp.type_only = Some(true); } + if !renamed_imports.is_empty() { + imp.renamed_imports = Some(renamed_imports); + } symbols.imports.push(imp); } } @@ -2972,20 +2976,80 @@ fn extract_rest_identifier(rest_node: &Node, source: &[u8], names: &mut Vec Vec { let mut names = Vec::new(); - scan_import_names(node, source, &mut names); + let mut renamed = Vec::new(); + scan_import_names(node, source, &mut names, &mut renamed); + names +} + +/// Extract import names and collect `{ local, imported }` pairs for +/// `import_specifier` nodes that rename a binding (`import { X as Y }`). +/// Mirrors `extractImportNames`'s `renamedOut` parameter in +/// src/extractors/javascript.ts (#1730). +fn extract_import_names_with_renames( + node: &Node, + source: &[u8], + renamed_out: &mut Vec, +) -> Vec { + let mut names = Vec::new(); + scan_import_names(node, source, &mut names, renamed_out); names } -fn scan_import_names(node: &Node, source: &[u8], names: &mut Vec) { - scan_import_names_depth(node, source, names, 0); +fn scan_import_names( + node: &Node, + source: &[u8], + names: &mut Vec, + renamed_out: &mut Vec, +) { + scan_import_names_depth(node, source, names, renamed_out, 0); } -fn scan_import_names_depth(node: &Node, source: &[u8], names: &mut Vec, depth: usize) { +/// Grammar note (see tree-sitter-javascript): for `import_specifier`, the +/// `name` field is *always* present — it holds the name as declared by the +/// source module. `alias` is only present for `X as Y` and holds the *local* +/// binding actually referenced by call sites in this file. Preferring `name` +/// unconditionally (as this function used to, and as the `export_specifier` +/// branch below still deliberately does — see its comment) silently drops the +/// local alias for every renamed import: call sites use `Y`, not `X` (#1730). +fn scan_import_names_depth( + node: &Node, + source: &[u8], + names: &mut Vec, + renamed_out: &mut Vec, + depth: usize, +) { if depth >= MAX_WALK_DEPTH { return; } match node.kind() { - "import_specifier" | "export_specifier" => { + "import_specifier" => { + let source_name_node = node.child_by_field_name("name"); + let alias_node = node.child_by_field_name("alias"); + let local_node = alias_node.or(source_name_node); + if let Some(local_node) = local_node { + names.push(node_text(&local_node, source).to_string()); + if let (Some(alias), Some(source_name)) = (alias_node, source_name_node) { + let alias_text = node_text(&alias, source); + let source_text = node_text(&source_name, source); + if alias_text != source_text { + renamed_out.push(RenamedImport { + local: alias_text.to_string(), + imported: source_text.to_string(), + }); + } + } + } else { + names.push(node_text(node, source).to_string()); + } + } + "export_specifier" => { + // export_specifier's `name` is the local declaration being (re-)exported; + // `alias` is the external name it's exposed as. Barrel/reexport tracing + // keys off the *original* declaration name, so this branch is + // deliberately left picking `name` first — do not unify with the + // import_specifier branch above. Rename-aware barrel tracing for + // `export { X as Y } from …` is a distinct, separate gap (#1730 + // investigation). let name_node = node .child_by_field_name("name") .or_else(|| node.child_by_field_name("alias")); @@ -3009,7 +3073,7 @@ fn scan_import_names_depth(node: &Node, source: &[u8], names: &mut Vec, } for i in 0..node.child_count() { if let Some(child) = node.child(i) { - scan_import_names_depth(&child, source, names, depth + 1); + scan_import_names_depth(&child, source, names, renamed_out, depth + 1); } } } @@ -3659,6 +3723,30 @@ mod tests { assert_eq!(s.imports[0].names, vec!["readFile"]); } + /// Regression test for #1730: `import { X as Y }` must record the *local* + /// binding (Y) in `names` — that's what call sites reference — plus the + /// `{ local: Y, imported: X }` pair in `renamed_imports` so call-edge + /// resolution can recover the original exported name X. + #[test] + fn renamed_import_records_local_name_and_rename_pair() { + let s = parse_js("import { collectFiles as collectFilesUtil } from './helpers';"); + assert_eq!(s.imports.len(), 1); + assert_eq!(s.imports[0].names, vec!["collectFilesUtil"]); + let renamed = s.imports[0] + .renamed_imports + .as_ref() + .expect("renamed_imports should be populated for a renamed specifier"); + assert_eq!(renamed.len(), 1); + assert_eq!(renamed[0].local, "collectFilesUtil"); + assert_eq!(renamed[0].imported, "collectFiles"); + } + + #[test] + fn non_renamed_import_has_no_renamed_imports() { + let s = parse_js("import { readFile } from 'fs';"); + assert!(s.imports[0].renamed_imports.is_none()); + } + #[test] fn finds_calls() { let s = parse_js("function f() { console.log('hi'); foo(); }"); diff --git a/crates/codegraph-core/src/types.rs b/crates/codegraph-core/src/types.rs index a1c3ccdc2..e41c50034 100644 --- a/crates/codegraph-core/src/types.rs +++ b/crates/codegraph-core/src/types.rs @@ -114,6 +114,16 @@ pub struct Call { pub key_expr: Option, } +/// `import { X as Y }`: the local binding name (Y) paired with the original +/// name exported by the source module (X). Mirrors TS `Import.renamedImports`. +/// See `Import.renamed_imports` for why this is needed (#1730). +#[napi(object)] +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct RenamedImport { + pub local: String, + pub imported: String, +} + #[napi(object)] #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Import { @@ -125,6 +135,15 @@ pub struct Import { pub reexport: Option, #[napi(js_name = "wildcardReexport")] pub wildcard_reexport: Option, + /// For `import { X as Y }` specifiers: the local binding name (Y) mapped to + /// the original name exported by the source module (X). `names` always + /// carries the local (post-rename) binding — this field lets call-edge + /// resolution recover the *original* symbol name to look up in the + /// imported file when a call site uses the local alias (#1730). Only + /// populated for specifiers that actually rename a binding. Mirrors TS + /// `Import.renamedImports`. + #[napi(js_name = "renamedImports")] + pub renamed_imports: Option>, // Language-specific flags #[napi(js_name = "pythonImport")] pub python_import: Option, @@ -168,6 +187,7 @@ impl Import { type_only: None, reexport: None, wildcard_reexport: None, + renamed_imports: None, python_import: None, go_import: None, rust_use: None, diff --git a/src/domain/graph/builder/call-resolver.ts b/src/domain/graph/builder/call-resolver.ts index b39a158a7..68a5a2589 100644 --- a/src/domain/graph/builder/call-resolver.ts +++ b/src/domain/graph/builder/call-resolver.ts @@ -217,6 +217,7 @@ export function resolveCallTargets( importedNames: Map, typeMap: Map, callerName?: string | null, + importedOriginalNames?: ReadonlyMap, ): { targets: Array<{ id: number; file: string }>; importedFrom: string | undefined } { // Flagged dynamic calls use synthetic names like ''. Short-circuit // so they never accidentally match a real symbol via lookup.byName. @@ -225,14 +226,18 @@ export function resolveCallTargets( } const importedFrom = importedNames.get(call.name); + // When the call site uses a renamed import binding (`import { X as Y }`), + // the imported file's actual symbol is declared under the *original* name + // (X) — look that up instead of the local alias the call site wrote (#1730). + const targetName = importedOriginalNames?.get(call.name) ?? call.name; let targets: ReadonlyArray<{ id: number; file: string }> | undefined; if (importedFrom) { - targets = lookup.byNameAndFile(call.name, importedFrom); + targets = lookup.byNameAndFile(targetName, importedFrom); if (targets.length === 0 && lookup.isBarrel(importedFrom)) { - const actualSource = lookup.resolveBarrel(importedFrom, call.name); + const actualSource = lookup.resolveBarrel(importedFrom, targetName); if (actualSource) { - targets = lookup.byNameAndFile(call.name, actualSource); + targets = lookup.byNameAndFile(targetName, actualSource); } } } diff --git a/src/domain/graph/builder/incremental.ts b/src/domain/graph/builder/incremental.ts index 863ef00f7..909230fb4 100644 --- a/src/domain/graph/builder/incremental.ts +++ b/src/domain/graph/builder/incremental.ts @@ -16,6 +16,7 @@ import type { BetterSqlite3Database, EngineOpts, ExtractorOutput, + Import, PathAliases, SqliteStatement, } from '../../../types.js'; @@ -209,8 +210,22 @@ function rebuildReverseDepEdges( aliases, skipBarrel ? null : db, ); - const importedNames = buildImportedNamesMap(symbols, rootDir, depRelPath, aliases, db); - edgesAdded += buildCallEdges(db, stmts, depRelPath, symbols, fileNodeRow, importedNames); + const { importedNames, importedOriginalNames } = buildImportedNamesMap( + symbols, + rootDir, + depRelPath, + aliases, + db, + ); + edgesAdded += buildCallEdges( + db, + stmts, + depRelPath, + symbols, + fileNodeRow, + importedNames, + importedOriginalNames, + ); edgesAdded += buildClassHierarchyEdges(stmts, depRelPath, symbols); return edgesAdded; } @@ -304,6 +319,23 @@ function resolveBarrelTarget( return null; } +/** + * Pairs each locally-bound name from an import statement with its original + * (pre-rename) exported name — identical to the local name unless the + * specifier renames a binding (`import { X as Y }`). Barrel tracing and + * target-file symbol lookups must search using the *original* name — the + * renamed local alias only exists in the importing file, not in the file + * being imported from (#1730). Mirrors `importNamePairs` in build-edges.ts. + */ +function importNamePairs(imp: Import): Array<{ local: string; original: string }> { + const originalNameFor = new Map(); + for (const r of imp.renamedImports ?? []) originalNameFor.set(r.local, r.imported); + return imp.names.map((name) => { + const local = name.replace(/^\*\s+as\s+/, ''); + return { local, original: originalNameFor.get(local) ?? local }; + }); +} + /** * Resolve barrel imports for a single import statement and create edges to actual source files. * Shared by buildImportEdges (primary file) and Pass 2 of the reverse-dep cascade. @@ -318,9 +350,8 @@ function resolveBarrelImportEdges( let edgesAdded = 0; if (!isBarrelFile(db, resolvedPath)) return edgesAdded; const resolvedSources = new Set(); - for (const name of imp.names) { - const cleanName = name.replace(/^\*\s+as\s+/, ''); - const actualSource = resolveBarrelTarget(db, resolvedPath, cleanName); + for (const { original } of importNamePairs(imp)) { + const actualSource = resolveBarrelTarget(db, resolvedPath, original); if (actualSource && actualSource !== resolvedPath && !resolvedSources.has(actualSource)) { resolvedSources.add(actualSource); const actualRow = stmts.getNodeId.get(actualSource, 'file', actualSource, 0); @@ -343,14 +374,13 @@ function emitTypeOnlySymbolEdges( fileNodeId: number, ): number { let edgesAdded = 0; - for (const name of imp.names) { - const cleanName = name.replace(/^\*\s+as\s+/, ''); + for (const { original } of importNamePairs(imp)) { let targetFile = resolvedPath; if (db && isBarrelFile(db, resolvedPath)) { - const actual = resolveBarrelTarget(db, resolvedPath, cleanName); + const actual = resolveBarrelTarget(db, resolvedPath, original); if (actual) targetFile = actual; } - const candidates = stmts.findNodeInFile.all(cleanName, targetFile) as Array<{ + const candidates = stmts.findNodeInFile.all(original, targetFile) as Array<{ id: number; file: string; }>; @@ -413,14 +443,23 @@ function buildImportEdges( return edgesAdded; } +/** + * Mirrors the full-build `buildImportedNamesMap` in build-edges.ts: maps each + * locally-bound import name to its defining file (`importedNames`), plus, for + * renamed specifiers (`import { X as Y }`), the *original* exported name + * (`importedOriginalNames`, keyed by local name Y). Barrel tracing and the + * downstream target-file symbol lookup must use the original name — the + * renamed local alias only exists in the importing file (#1730). + */ function buildImportedNamesMap( symbols: ExtractorOutput, rootDir: string, relPath: string, aliases: PathAliases, db: BetterSqlite3Database, -): Map { +): { importedNames: Map; importedOriginalNames: Map } { const importedNames = new Map(); + const importedOriginalNames = new Map(); for (const imp of symbols.imports) { const resolvedPath = resolveImportPath( path.join(rootDir, relPath), @@ -428,21 +467,21 @@ function buildImportedNamesMap( rootDir, aliases, ); - for (const name of imp.names) { - const cleanName = name.replace(/^\*\s+as\s+/, ''); + for (const { local, original } of importNamePairs(imp)) { // Mirror full-build's `buildImportedNamesMap`: follow barrel re-exports so // `importedNames` maps to the *defining* file, not the barrel. This ensures // `computeConfidence` gets `importedFrom === targetFile` and returns 1.0 // instead of the cross-directory fallback (0.3). let targetFile = resolvedPath; if (isBarrelFile(db, resolvedPath)) { - const actual = resolveBarrelTarget(db, resolvedPath, cleanName); + const actual = resolveBarrelTarget(db, resolvedPath, original); if (actual) targetFile = actual; } - importedNames.set(cleanName, targetFile); + importedNames.set(local, targetFile); + if (original !== local) importedOriginalNames.set(local, original); } } - return importedNames; + return { importedNames, importedOriginalNames }; } // ── Class hierarchy edges ─────────────────────────────────────────────── @@ -690,6 +729,7 @@ function buildCallEdges( symbols: ExtractorOutput, fileNodeRow: { id: number }, importedNames: Map, + importedOriginalNames?: ReadonlyMap, ): number { const typeMap = buildIncrementalTypeMap(symbols); const seenCallEdges = new Set(); @@ -707,6 +747,7 @@ function buildCallEdges( importedNames, typeMap, caller.callerName, + importedOriginalNames, ); const targets = applyThisReceiverFallbacks( @@ -773,8 +814,22 @@ function rebuildEdgesForTargetFile( let edgesAdded = buildContainmentEdges(db, stmts, relPath, symbols); edgesAdded += rebuildDirContainment(db, stmts, relPath); edgesAdded += buildImportEdges(stmts, relPath, symbols, rootDir, fileNodeRow.id, aliases, db); - const importedNames = buildImportedNamesMap(symbols, rootDir, relPath, aliases, db); - edgesAdded += buildCallEdges(db, stmts, relPath, symbols, fileNodeRow, importedNames); + const { importedNames, importedOriginalNames } = buildImportedNamesMap( + symbols, + rootDir, + relPath, + aliases, + db, + ); + edgesAdded += buildCallEdges( + db, + stmts, + relPath, + symbols, + fileNodeRow, + importedNames, + importedOriginalNames, + ); edgesAdded += buildClassHierarchyEdges(stmts, relPath, symbols); return edgesAdded; } diff --git a/src/domain/graph/builder/stages/build-edges.ts b/src/domain/graph/builder/stages/build-edges.ts index 011ce246b..33ebafa84 100644 --- a/src/domain/graph/builder/stages/build-edges.ts +++ b/src/domain/graph/builder/stages/build-edges.ts @@ -88,7 +88,7 @@ interface NativeFileEntry { params?: string[]; }>; calls: Call[]; - importedNames: Array<{ name: string; file: string }>; + importedNames: Array<{ name: string; file: string; imported?: string }>; classes: ClassRelation[]; typeMap: Array<{ name: string; typeName: string; confidence: number }>; /** Phase 8.3: function-reference bindings for pts analysis. */ @@ -148,6 +148,23 @@ function importEdgeKind(imp: Import): string { return 'imports'; } +/** + * Pairs each locally-bound name from an import statement with its original + * (pre-rename) exported name — identical to the local name unless the + * specifier renames a binding (`import { X as Y }`). Barrel tracing and + * target-file symbol lookups must search using the *original* name — the + * renamed local alias only exists in the importing file, not in the file + * being imported from (#1730). + */ +function importNamePairs(imp: Import): Array<{ local: string; original: string }> { + const originalNameFor = new Map(); + for (const r of imp.renamedImports ?? []) originalNameFor.set(r.local, r.imported); + return imp.names.map((name) => { + const local = name.replace(/^\*\s+as\s+/, ''); + return { local, original: originalNameFor.get(local) ?? local }; + }); +} + /** * For a `import type` statement, emit symbol-level `imports-type` edges so * the target symbols get fan-in credit and aren't classified as dead code. @@ -160,14 +177,13 @@ function emitTypeOnlySymbolEdges( allEdgeRows: EdgeRowTuple[], ): void { if (!ctx.nodesByNameAndFile) return; - for (const name of imp.names) { - const cleanName = name.replace(/^\*\s+as\s+/, ''); + for (const { original } of importNamePairs(imp)) { let targetFile = resolvedPath; if (isBarrelFile(ctx, resolvedPath)) { - const actual = resolveBarrelExportCached(ctx, resolvedPath, cleanName); + const actual = resolveBarrelExportCached(ctx, resolvedPath, original); if (actual) targetFile = actual; } - const candidates = ctx.nodesByNameAndFile.get(`${cleanName}|${targetFile}`); + const candidates = ctx.nodesByNameAndFile.get(`${original}|${targetFile}`); if (candidates && candidates.length > 0) { allEdgeRows.push([fileNodeId, candidates[0]!.id, 'imports-type', 1.0, 0, null, null]); } @@ -233,9 +249,8 @@ function buildBarrelEdges( edgeRows: EdgeRowTuple[], ): void { const resolvedSources = new Set(); - for (const name of imp.names) { - const cleanName = name.replace(/^\*\s+as\s+/, ''); - const actualSource = resolveBarrelExportCached(ctx, resolvedPath, cleanName); + for (const { original } of importNamePairs(imp)) { + const actualSource = resolveBarrelExportCached(ctx, resolvedPath, original); if (actualSource && actualSource !== resolvedPath && !resolvedSources.has(actualSource)) { resolvedSources.add(actualSource); const actualRow = getNodeIdStmt.get(actualSource, 'file', actualSource, 0); @@ -474,7 +489,12 @@ function propagateReturnTypesAcrossFiles( // file rather than the barrel. This means returnTypeIndex.get(importedFrom) // now finds entries it previously missed, improving cross-file return-type // propagation through re-export chains (Phase 8.2 improvement). - const importedNamesMap = buildImportedNamesMap(ctx, relPath, symbols, rootDir); + const { importedNames: importedNamesMap, importedOriginalNames } = buildImportedNamesMap( + ctx, + relPath, + symbols, + rootDir, + ); for (const ca of symbols.callAssignments) { if (symbols.typeMap.has(ca.varName)) continue; // already resolved locally @@ -484,7 +504,11 @@ function propagateReturnTypesAcrossFiles( returnEntry = globalReturnTypeMap.get(`${ca.receiverTypeName}.${ca.calleeName}`); } else { const importedFrom = importedNamesMap.get(ca.calleeName); - if (importedFrom) returnEntry = returnTypeIndex.get(importedFrom)?.get(ca.calleeName); + // The return-type index for the imported file is keyed by the + // function's own declared name — use the original (pre-rename) name + // when the call-assignment's callee is a renamed import binding (#1730). + const calleeOriginalName = importedOriginalNames.get(ca.calleeName) ?? ca.calleeName; + if (importedFrom) returnEntry = returnTypeIndex.get(importedFrom)?.get(calleeOriginalName); } if (returnEntry) { @@ -639,7 +663,12 @@ function buildDefinePropertyPostPass( const fileNodeRow = getNodeIdStmt.get(relPath, 'file', relPath, 0); if (!fileNodeRow) continue; - const importedNames = buildImportedNamesMap(ctx, relPath, symbols, rootDir); + const { importedNames, importedOriginalNames } = buildImportedNamesMap( + ctx, + relPath, + symbols, + rootDir, + ); const typeMap: Map = symbols.typeMap || new Map(); const definePropertyReceivers = symbols.definePropertyReceivers!; @@ -660,6 +689,7 @@ function buildDefinePropertyPostPass( importedNames, typeMap as Map, caller.callerName, + importedOriginalNames, ); if (directTargets.length > 0) continue; @@ -789,21 +819,28 @@ function buildImportedNamesForNative( relPath: string, symbols: ExtractorOutput, rootDir: string, -): Array<{ name: string; file: string }> { - const importedNames: Array<{ name: string; file: string }> = []; +): Array<{ name: string; file: string; imported?: string }> { + const importedNames: Array<{ name: string; file: string; imported?: string }> = []; // Process dynamic imports first (lower priority), then static imports // (higher priority). Rust HashMap::collect keeps the last entry per key, // so static imports win when both contribute the same name. const addImports = (imp: (typeof symbols.imports)[number]) => { const resolvedPath = getResolved(ctx, path.join(rootDir, relPath), imp.source); - for (const name of imp.names) { - const cleanName = name.replace(/^\*\s+as\s+/, ''); + for (const { local, original } of importNamePairs(imp)) { let targetFile = resolvedPath; if (isBarrelFile(ctx, resolvedPath)) { - const actual = resolveBarrelExportCached(ctx, resolvedPath, cleanName); + const actual = resolveBarrelExportCached(ctx, resolvedPath, original); if (actual) targetFile = actual; } - importedNames.push({ name: cleanName, file: targetFile }); + // `imported` carries the original (pre-rename) exported name so the + // native resolver can look it up in `targetFile` instead of the local + // alias, which only exists in this file (#1730). Omitted when unrenamed. + const entry: { name: string; file: string; imported?: string } = { + name: local, + file: targetFile, + }; + if (original !== local) entry.imported = original; + importedNames.push(entry); } }; for (const imp of symbols.imports) { @@ -831,7 +868,12 @@ function buildCallEdgesJS( const fileNodeRow = getNodeIdStmt.get(relPath, 'file', relPath, 0); if (!fileNodeRow) continue; - const importedNames = buildImportedNamesMap(ctx, relPath, symbols, rootDir); + const { importedNames, importedOriginalNames } = buildImportedNamesMap( + ctx, + relPath, + symbols, + rootDir, + ); const typeMap: Map = new Map( symbols.typeMap instanceof Map ? symbols.typeMap : [], ); @@ -887,48 +929,58 @@ function buildCallEdgesJS( ptsMap, chaCtx, importArtifactNames, + importedOriginalNames, ); buildClassHierarchyEdges(ctx, relPath, symbols, allEdgeRows); } } +/** + * Maps each locally-bound import name in `relPath` to the file it comes from + * (`importedNames`), plus, for renamed specifiers (`import { X as Y }`), the + * *original* exported name (`importedOriginalNames`, keyed by local name Y). + * + * Barrel tracing and downstream target-file symbol lookups must search using + * the original name — the renamed local alias only exists in the importing + * file, not in the file being imported from (#1730). + */ function buildImportedNamesMap( ctx: PipelineContext, relPath: string, symbols: ExtractorOutput, rootDir: string, -): Map { +): { importedNames: Map; importedOriginalNames: Map } { const importedNames = new Map(); - // Process dynamic imports first (lower priority), then static imports - // (higher priority). Static imports represent direct bindings while dynamic - // imports often use aliased destructuring (`{ foo: bar } = await import(…)`). - // When both contribute the same name, the static binding is authoritative. - // + const importedOriginalNames = new Map(); // Phase 8.4: trace through barrel files so that symbol names map to their // actual definition file, not the re-exporting barrel. Mirrors the tracing // already done in buildImportedNamesForNative (the native path). - const traceBarrel = (resolvedPath: string, cleanName: string): string => { + const traceBarrel = (resolvedPath: string, originalName: string): string => { if (!isBarrelFile(ctx, resolvedPath)) return resolvedPath; - const actual = resolveBarrelExportCached(ctx, resolvedPath, cleanName); + const actual = resolveBarrelExportCached(ctx, resolvedPath, originalName); return actual ?? resolvedPath; }; + const addImportNames = (imp: (typeof symbols.imports)[number], resolvedPath: string) => { + for (const { local, original } of importNamePairs(imp)) { + importedNames.set(local, traceBarrel(resolvedPath, original)); + if (original !== local) importedOriginalNames.set(local, original); + } + }; + // Process dynamic imports first (lower priority), then static imports + // (higher priority). Static imports represent direct bindings while dynamic + // imports often use aliased destructuring (`{ foo: bar } = await import(…)`). + // When both contribute the same name, the static binding is authoritative. for (const imp of symbols.imports) { if (!imp.dynamicImport) continue; const resolvedPath = getResolved(ctx, path.join(rootDir, relPath), imp.source); - for (const name of imp.names) { - const cleanName = name.replace(/^\*\s+as\s+/, ''); - importedNames.set(cleanName, traceBarrel(resolvedPath, cleanName)); - } + addImportNames(imp, resolvedPath); } for (const imp of symbols.imports) { if (imp.dynamicImport) continue; const resolvedPath = getResolved(ctx, path.join(rootDir, relPath), imp.source); - for (const name of imp.names) { - const cleanName = name.replace(/^\*\s+as\s+/, ''); - importedNames.set(cleanName, traceBarrel(resolvedPath, cleanName)); - } + addImportNames(imp, resolvedPath); } - return importedNames; + return { importedNames, importedOriginalNames }; } /** @@ -1226,6 +1278,7 @@ function resolveFallbackTargets( lookup: CallNodeLookup, typeMap: Map, definePropertyReceivers: Map | undefined, + importedOriginalNames?: ReadonlyMap, ): { targets: ReadonlyArray<{ id: number; file: string; kind?: string }>; importedFrom: string | null | undefined; @@ -1245,6 +1298,7 @@ function resolveFallbackTargets( importedNames, typeMap as Map, caller.callerName, + importedOriginalNames, ); // Fallback strategies, applied in order until one yields a match. Each @@ -1403,6 +1457,7 @@ function emitPtsNoReceiverEdges( seenCallEdges: Set, ptsEdgeRows: Map, allEdgeRows: EdgeRowTuple[], + importedOriginalNames?: ReadonlyMap, ): void { const scopedPtsKey = caller.callerName != null ? `${caller.callerName}::${call.name}` : null; // Module-level calls (callerName === null) use the '' sentinel emitted by @@ -1445,6 +1500,8 @@ function emitPtsNoReceiverEdges( relPath, importedNames, typeMap as Map, + undefined, + importedOriginalNames, ); const sortedAliasTargets = aliasTargets.length > 1 @@ -1488,6 +1545,7 @@ function emitPtsReceiverEdges( seenCallEdges: Set, ptsEdgeRows: Map, allEdgeRows: EdgeRowTuple[], + importedOriginalNames?: ReadonlyMap, ): void { const receiverKey = `${call.receiver}.${call.name}`; if (!ptsMap.has(receiverKey)) return; @@ -1499,6 +1557,8 @@ function emitPtsReceiverEdges( relPath, importedNames, typeMap as Map, + undefined, + importedOriginalNames, ); const sortedAliasTargets = aliasTargets.length > 1 @@ -1621,6 +1681,7 @@ function buildFileCallEdges( ptsMap?: PointsToMap | null, chaCtx?: ChaContext, importArtifactNames?: ReadonlyMap, + importedOriginalNames?: ReadonlyMap, ): void { // Tracks edges that were inserted by the pts fallback (edgeKey → allEdgeRows index). // Kept separate from seenCallEdges so that a subsequent direct-call edge for the same @@ -1656,6 +1717,7 @@ function buildFileCallEdges( lookup, typeMap, symbols.definePropertyReceivers, + importedOriginalNames, ); // Step 2: Emit direct-call edges (upgrades any pending pts edge in-place). @@ -1687,6 +1749,7 @@ function buildFileCallEdges( seenCallEdges, ptsEdgeRows, allEdgeRows, + importedOriginalNames, ); } @@ -1712,6 +1775,7 @@ function buildFileCallEdges( seenCallEdges, ptsEdgeRows, allEdgeRows, + importedOriginalNames, ); } diff --git a/src/extractors/javascript.ts b/src/extractors/javascript.ts index 7c4b1ea9e..65326e366 100644 --- a/src/extractors/javascript.ts +++ b/src/extractors/javascript.ts @@ -311,12 +311,14 @@ function handleImportCapture(c: Record, imports: Import[ const impNode = c.imp_node!; const isTypeOnly = impNode.text.startsWith('import type'); const modPath = c.imp_source!.text.replace(/['"]/g, ''); - const names = extractImportNames(impNode); + const renamedImports: Array<{ local: string; imported: string }> = []; + const names = extractImportNames(impNode, renamedImports); imports.push({ source: modPath, names, line: nodeStartLine(impNode), typeOnly: isTypeOnly, + ...(renamedImports.length > 0 ? { renamedImports } : {}), }); } @@ -1478,12 +1480,14 @@ function handleImportStmt(node: TreeSitterNode, ctx: ExtractorOutput): void { const source = node.childForFieldName('source') || findChild(node, 'string'); if (source) { const modPath = source.text.replace(/['"]/g, ''); - const names = extractImportNames(node); + const renamedImports: Array<{ local: string; imported: string }> = []; + const names = extractImportNames(node, renamedImports); ctx.imports.push({ source: modPath, names, line: nodeStartLine(node), typeOnly: isTypeOnly, + ...(renamedImports.length > 0 ? { renamedImports } : {}), }); } } @@ -3843,10 +3847,45 @@ function findParentClass(node: TreeSitterNode): string | null { return findParentNode(node, JS_CLASS_TYPES); } -function extractImportNames(node: TreeSitterNode): string[] { +/** + * Extract the local binding names introduced by an import/export statement. + * + * `renamedOut`, when passed, collects `{ local, imported }` pairs for + * `import_specifier` nodes that rename a binding (`import { X as Y }`). + * + * Grammar note (see tree-sitter-javascript): for `import_specifier`, the + * `name` field is *always* present — it holds the name as declared by the + * source module. `alias` is only present for `X as Y` and holds the *local* + * binding actually referenced by call sites in this file. Preferring `name` + * unconditionally (as this function used to, and as the `export_specifier` + * branch below still deliberately does — see its comment) silently drops the + * local alias for every renamed import: call sites use `Y`, not `X` (#1730). + */ +function extractImportNames( + node: TreeSitterNode, + renamedOut?: Array<{ local: string; imported: string }>, +): string[] { const names: string[] = []; function scan(n: TreeSitterNode): void { - if (n.type === 'import_specifier' || n.type === 'export_specifier') { + if (n.type === 'import_specifier') { + const sourceNameNode = n.childForFieldName('name'); + const aliasNode = n.childForFieldName('alias'); + const localNode = aliasNode || sourceNameNode; + if (localNode) { + names.push(localNode.text); + if (aliasNode && sourceNameNode && aliasNode.text !== sourceNameNode.text) { + renamedOut?.push({ local: aliasNode.text, imported: sourceNameNode.text }); + } + } else { + names.push(n.text); + } + } else if (n.type === 'export_specifier') { + // export_specifier's `name` is the local declaration being (re-)exported; + // `alias` is the external name it's exposed as. Barrel/reexport tracing + // (resolveBarrelExport) keys off the *original* declaration name, so this + // branch is deliberately left picking `name` first — do not unify with + // the import_specifier branch above. Rename-aware barrel tracing for + // `export { X as Y } from …` is a distinct, separate gap (#1730 investigation). const nameNode = n.childForFieldName('name') || n.childForFieldName('alias'); if (nameNode) names.push(nameNode.text); else names.push(n.text); diff --git a/src/types.ts b/src/types.ts index 81fafbe03..96f7d2d46 100644 --- a/src/types.ts +++ b/src/types.ts @@ -510,6 +510,17 @@ export interface Import { reexport?: boolean; wildcardReexport?: boolean; dynamicImport?: boolean; + /** + * For `import { X as Y }` specifiers: the local binding name (Y) mapped to + * the original name exported by the source module (X). `names` always + * carries the local (post-rename) binding — this field lets call-edge + * resolution recover the *original* symbol name to look up in the imported + * file when a call site uses the local alias (#1730). Only populated for + * specifiers that actually rename a binding; entries where local === source + * name are omitted. Not populated for `export { X as Y } from …` reexports + * — barrel/reexport tracing is a distinct mechanism (see resolveBarrelExport). + */ + renamedImports?: Array<{ local: string; imported: string }>; // Language-specific flags (mutually exclusive at runtime) pythonImport?: boolean; goImport?: boolean; diff --git a/tests/integration/issue-1730-renamed-import-consumer.test.ts b/tests/integration/issue-1730-renamed-import-consumer.test.ts new file mode 100644 index 000000000..4d43843c0 --- /dev/null +++ b/tests/integration/issue-1730-renamed-import-consumer.test.ts @@ -0,0 +1,125 @@ +/** + * Regression test for #1730: consumer resolution must follow a call site + * through a renamed import specifier (`import { X as Y } from '...'`) back + * to the original exported symbol. + * + * Setup: two files. + * - helpers.js: `export function collectFiles() {...}` + * - consumer.js: `import { collectFiles as collectFilesUtil } from './helpers.js';` + * calls `collectFilesUtil()` from an exported function. + * + * Before the fix, the extractor recorded the *original* exported name + * (`collectFiles`) instead of the local alias actually used at the call site + * (`collectFilesUtil`) — so `importedNames` never had a key matching the call + * site text, and no `calls` edge was ever created. `codegraph exports + * helpers.js` reported zero consumers for a genuinely-used export. + * + * Verified on both engines — this is resolver/extractor logic mirrored in + * `crates/codegraph-core/`, so WASM and native must agree (#1730 root cause + * was duplicated in both). + */ + +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 { exportsData } from '../../src/domain/queries.js'; + +const FILE_HELPERS = ` +export function collectFiles() { + return ['a.js', 'b.js']; +} +`; + +const FILE_CONSUMER = ` +import { collectFiles as collectFilesUtil } from './helpers.js'; + +export function useIt() { + return collectFilesUtil(); +} +`; + +let tmpWasm: string; +let tmpNative: string; + +beforeAll(async () => { + tmpWasm = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-1730-wasm-')); + fs.writeFileSync(path.join(tmpWasm, 'helpers.js'), FILE_HELPERS); + fs.writeFileSync(path.join(tmpWasm, 'consumer.js'), FILE_CONSUMER); + + tmpNative = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-1730-native-')); + fs.writeFileSync(path.join(tmpNative, 'helpers.js'), FILE_HELPERS); + fs.writeFileSync(path.join(tmpNative, 'consumer.js'), FILE_CONSUMER); + + await Promise.all([ + buildGraph(tmpWasm, { incremental: false, skipRegistry: true, engine: 'wasm' }), + buildGraph(tmpNative, { incremental: false, skipRegistry: true, engine: 'native' }), + ]); +}); + +afterAll(() => { + fs.rmSync(tmpWasm, { recursive: true, force: true }); + fs.rmSync(tmpNative, { recursive: true, force: true }); +}); + +function getCallEdges(dbPath: string) { + const db = new Database(dbPath, { readonly: true }); + try { + return db + .prepare( + `SELECT n1.name AS src, n1.file AS src_file, n2.name AS tgt, n2.file AS tgt_file + FROM edges e + JOIN nodes n1 ON e.source_id = n1.id + JOIN nodes n2 ON e.target_id = n2.id + WHERE e.kind = 'calls' + ORDER BY n1.name, n2.name`, + ) + .all() as Array<{ src: string; src_file: string; tgt: string; tgt_file: string }>; + } finally { + db.close(); + } +} + +describe('call-edge resolution through a renamed import specifier (#1730)', () => { + it('WASM: useIt -> collectFiles calls edge exists (resolved through the rename)', () => { + const edges = getCallEdges(path.join(tmpWasm, '.codegraph', 'graph.db')); + const edge = edges.find((e) => e.src === 'useIt' && e.tgt === 'collectFiles'); + expect(edge).toBeDefined(); + expect(edge?.tgt_file).toBe('helpers.js'); + }); + + it('Native: useIt -> collectFiles calls edge exists (resolved through the rename)', () => { + const edges = getCallEdges(path.join(tmpNative, '.codegraph', 'graph.db')); + const edge = edges.find((e) => e.src === 'useIt' && e.tgt === 'collectFiles'); + expect(edge).toBeDefined(); + expect(edge?.tgt_file).toBe('helpers.js'); + }); + + it('no spurious edge is created against a nonexistent "collectFilesUtil" symbol', () => { + for (const dbPath of [ + path.join(tmpWasm, '.codegraph', 'graph.db'), + path.join(tmpNative, '.codegraph', 'graph.db'), + ]) { + const edges = getCallEdges(dbPath); + expect(edges.find((e) => e.tgt === 'collectFilesUtil')).toBeUndefined(); + } + }); + + it('WASM: codegraph exports credits collectFiles with the renamed-import consumer', () => { + const data = exportsData('helpers.js', path.join(tmpWasm, '.codegraph', 'graph.db')); + const collectFiles = data.results.find((r: { name: string }) => r.name === 'collectFiles'); + expect(collectFiles).toBeDefined(); + expect(collectFiles.consumerCount).toBeGreaterThanOrEqual(1); + expect(collectFiles.consumers.map((c: { name: string }) => c.name)).toContain('useIt'); + }); + + it('Native: codegraph exports credits collectFiles with the renamed-import consumer', () => { + const data = exportsData('helpers.js', path.join(tmpNative, '.codegraph', 'graph.db')); + const collectFiles = data.results.find((r: { name: string }) => r.name === 'collectFiles'); + expect(collectFiles).toBeDefined(); + expect(collectFiles.consumerCount).toBeGreaterThanOrEqual(1); + expect(collectFiles.consumers.map((c: { name: string }) => c.name)).toContain('useIt'); + }); +}); diff --git a/tests/parsers/javascript.test.ts b/tests/parsers/javascript.test.ts index c825df2a9..5f9994fbe 100644 --- a/tests/parsers/javascript.test.ts +++ b/tests/parsers/javascript.test.ts @@ -130,6 +130,52 @@ describe('JavaScript parser', () => { expect(symbols.imports[0].names).toContain('bar'); }); + // Regression coverage for #1730: `import { X as Y }` must record the local + // binding (Y) — what call sites actually reference — in `names`, plus the + // `{ local, imported }` rename pair so call-edge resolution can recover the + // original exported symbol (X) when a call site uses the local alias. + describe('renamed import specifiers (#1730)', () => { + it('records the local alias, not the source name, in imports[].names', () => { + const symbols = parseJS(`import { collectFiles as collectFilesUtil } from './helpers';`); + expect(symbols.imports).toHaveLength(1); + expect(symbols.imports[0].names).toEqual(['collectFilesUtil']); + }); + + it('records the local -> original rename pair in renamedImports', () => { + const symbols = parseJS(`import { collectFiles as collectFilesUtil } from './helpers';`); + expect(symbols.imports[0].renamedImports).toEqual([ + { local: 'collectFilesUtil', imported: 'collectFiles' }, + ]); + }); + + it('does not set renamedImports for non-renamed specifiers', () => { + const symbols = parseJS(`import { foo, bar } from './baz';`); + expect(symbols.imports[0].renamedImports).toBeUndefined(); + }); + + it('handles a mix of renamed and non-renamed specifiers in one statement', () => { + const symbols = parseJS( + `import { foo, collectFiles as collectFilesUtil, bar } from './mixed';`, + ); + expect(symbols.imports[0].names).toEqual(['foo', 'collectFilesUtil', 'bar']); + expect(symbols.imports[0].renamedImports).toEqual([ + { local: 'collectFilesUtil', imported: 'collectFiles' }, + ]); + }); + + it('does not apply rename tracking to export_specifier (reexport) statements', () => { + // export_specifier semantics differ (name = local declaration being + // re-exported, alias = external name) — barrel/reexport tracing keys off + // the original declaration name, so this is intentionally left as-is. + // See resolveBarrelExport / issues filed from the #1730 investigation. + const symbols = parseJS(`export { collectFiles as friendlyName } from './helpers';`); + expect(symbols.imports).toHaveLength(1); + expect(symbols.imports[0].reexport).toBe(true); + expect(symbols.imports[0].names).toEqual(['collectFiles']); + expect(symbols.imports[0].renamedImports).toBeUndefined(); + }); + }); + it('extracts call expressions', () => { const symbols = parseJS(`import { foo } from './bar'; foo(); baz();`); expect(symbols.calls).toContainEqual(expect.objectContaining({ name: 'foo' })); From f2e052506cc58671036661cf52527aaf37409e65 Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Sun, 5 Jul 2026 19:55:46 -0600 Subject: [PATCH 8/8] refactor: extract duplicated importNamePairs into shared import-utils.ts (#1826) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit docs check acknowledged — internal dedup of an existing helper, no new functionality, public API, or architecture change. Impact: 1 functions changed, 22 affected --- src/domain/graph/builder/import-utils.ts | 22 +++++++++++++++++++ src/domain/graph/builder/incremental.ts | 19 +--------------- .../graph/builder/stages/build-edges.ts | 18 +-------------- 3 files changed, 24 insertions(+), 35 deletions(-) create mode 100644 src/domain/graph/builder/import-utils.ts diff --git a/src/domain/graph/builder/import-utils.ts b/src/domain/graph/builder/import-utils.ts new file mode 100644 index 000000000..a56c7517a --- /dev/null +++ b/src/domain/graph/builder/import-utils.ts @@ -0,0 +1,22 @@ +import type { Import } from '../../../types.js'; + +/** + * Pairs each locally-bound name from an import statement with its original + * (pre-rename) exported name — identical to the local name unless the + * specifier renames a binding (`import { X as Y }`). Barrel tracing and + * target-file symbol lookups must search using the *original* name — the + * renamed local alias only exists in the importing file, not in the file + * being imported from (#1730). + * + * Shared by the full-build (`stages/build-edges.ts`) and incremental + * (`incremental.ts`) pipelines so the rename-stripping logic can't drift + * between them. + */ +export function importNamePairs(imp: Import): Array<{ local: string; original: string }> { + const originalNameFor = new Map(); + for (const r of imp.renamedImports ?? []) originalNameFor.set(r.local, r.imported); + return imp.names.map((name) => { + const local = name.replace(/^\*\s+as\s+/, ''); + return { local, original: originalNameFor.get(local) ?? local }; + }); +} diff --git a/src/domain/graph/builder/incremental.ts b/src/domain/graph/builder/incremental.ts index 909230fb4..29c17ee10 100644 --- a/src/domain/graph/builder/incremental.ts +++ b/src/domain/graph/builder/incremental.ts @@ -16,7 +16,6 @@ import type { BetterSqlite3Database, EngineOpts, ExtractorOutput, - Import, PathAliases, SqliteStatement, } from '../../../types.js'; @@ -30,6 +29,7 @@ import { resolveSameClassQualifiedMethod, } from './call-resolver.js'; import { BUILTIN_RECEIVERS, readFileSafe } from './helpers.js'; +import { importNamePairs } from './import-utils.js'; // ── Local types ───────────────────────────────────────────────────────── @@ -319,23 +319,6 @@ function resolveBarrelTarget( return null; } -/** - * Pairs each locally-bound name from an import statement with its original - * (pre-rename) exported name — identical to the local name unless the - * specifier renames a binding (`import { X as Y }`). Barrel tracing and - * target-file symbol lookups must search using the *original* name — the - * renamed local alias only exists in the importing file, not in the file - * being imported from (#1730). Mirrors `importNamePairs` in build-edges.ts. - */ -function importNamePairs(imp: Import): Array<{ local: string; original: string }> { - const originalNameFor = new Map(); - for (const r of imp.renamedImports ?? []) originalNameFor.set(r.local, r.imported); - return imp.names.map((name) => { - const local = name.replace(/^\*\s+as\s+/, ''); - return { local, original: originalNameFor.get(local) ?? local }; - }); -} - /** * Resolve barrel imports for a single import statement and create edges to actual source files. * Shared by buildImportEdges (primary file) and Pass 2 of the reverse-dep cascade. diff --git a/src/domain/graph/builder/stages/build-edges.ts b/src/domain/graph/builder/stages/build-edges.ts index 33ebafa84..96f03b9ca 100644 --- a/src/domain/graph/builder/stages/build-edges.ts +++ b/src/domain/graph/builder/stages/build-edges.ts @@ -56,6 +56,7 @@ import { CHA_TYPED_DISPATCH_CONFIDENCE, runChaPostPass, } from '../helpers.js'; +import { importNamePairs } from '../import-utils.js'; import { getResolved, isBarrelFile, resolveBarrelExportCached } from './resolve-imports.js'; // ── Local types ────────────────────────────────────────────────────────── @@ -148,23 +149,6 @@ function importEdgeKind(imp: Import): string { return 'imports'; } -/** - * Pairs each locally-bound name from an import statement with its original - * (pre-rename) exported name — identical to the local name unless the - * specifier renames a binding (`import { X as Y }`). Barrel tracing and - * target-file symbol lookups must search using the *original* name — the - * renamed local alias only exists in the importing file, not in the file - * being imported from (#1730). - */ -function importNamePairs(imp: Import): Array<{ local: string; original: string }> { - const originalNameFor = new Map(); - for (const r of imp.renamedImports ?? []) originalNameFor.set(r.local, r.imported); - return imp.names.map((name) => { - const local = name.replace(/^\*\s+as\s+/, ''); - return { local, original: originalNameFor.get(local) ?? local }; - }); -} - /** * For a `import type` statement, emit symbol-level `imports-type` edges so * the target symbols get fan-in credit and aren't classified as dead code.