diff --git a/crates/codegraph-core/src/extractors/javascript.rs b/crates/codegraph-core/src/extractors/javascript.rs index 4f17814d..2798969d 100644 --- a/crates/codegraph-core/src/extractors/javascript.rs +++ b/crates/codegraph-core/src/extractors/javascript.rs @@ -1353,7 +1353,13 @@ fn handle_var_decl(node: &Node, source: &[u8], symbols: &mut FileSymbols) { if let Some(str_arg) = find_child(&args, "string") { let mod_path = node_text(&str_arg, source) .replace(&['\'', '"'][..], ""); - let names = collect_object_pattern_names(&name_n, source); + // CJS require bindings never populate renamed_imports — + // resolve_call_targets deliberately ignores the original + // name for these (empty target_file forces a same-file + // fallback match, matching WASM's importedNamesMap + // exclusion, #1678) — so the rename pairs collected here + // are discarded. + let names = collect_object_pattern_names(&name_n, source, &mut Vec::new()); if !names.is_empty() { let mut imp = Import::new( mod_path, names, start_line(node), @@ -1672,9 +1678,13 @@ fn handle_dynamic_import(node: &Node, _fn_node: &Node, source: &[u8], symbols: & if let Some(str_node) = str_node { let mod_path = node_text(&str_node, source) .replace(&['\'', '"', '`'][..], ""); - let names = extract_dynamic_import_names(node, source); + let mut renamed_imports = Vec::new(); + let names = extract_dynamic_import_names(node, source, &mut renamed_imports); let mut imp = Import::new(mod_path, names, start_line(node)); imp.dynamic_import = Some(true); + if !renamed_imports.is_empty() { + imp.renamed_imports = Some(renamed_imports); + } symbols.imports.push(imp); } } @@ -3086,9 +3096,19 @@ const DYNAMIC_IMPORT_WRAPPER_KINDS: &[&str] = /// Extract named bindings from a dynamic `import()` call expression. /// Handles: `const { a, b } = await import(...)`, `const mod = await import(...)`, -/// and casts/parens wrapping the awaited call, e.g. -/// `const { a } = (await import(...)) as { a: Fn }`. -fn extract_dynamic_import_names(call_node: &Node, source: &[u8]) -> Vec { +/// casts/parens wrapping the awaited call, e.g. +/// `const { a } = (await import(...)) as { a: Fn }`, and destructuring +/// renames, e.g. `const { a: b } = await import(...)`. +/// +/// `renamed_out` is populated with `{ local, imported }` pairs for every +/// `{ imported: local }` specifier — mirrors `extract_import_names_with_renames`'s +/// static-import convention (#1730) so call-edge resolution can recover the +/// original exported name when a call site uses the local alias (#1824). +fn extract_dynamic_import_names( + call_node: &Node, + source: &[u8], + renamed_out: &mut Vec, +) -> Vec { // Walk up through any combination/nesting of await/parenthesized/as-cast // wrappers to reach the variable_declarator. let mut current = call_node.parent(); @@ -3107,7 +3127,7 @@ fn extract_dynamic_import_names(call_node: &Node, source: &[u8]) -> Vec return Vec::new(); }; match name_node.kind() { - "object_pattern" => collect_object_pattern_names(&name_node, source), + "object_pattern" => collect_object_pattern_names(&name_node, source, renamed_out), "identifier" => vec![node_text(&name_node, source).to_string()], "array_pattern" => collect_array_pattern_names(&name_node, source), _ => Vec::new(), @@ -3115,7 +3135,11 @@ fn extract_dynamic_import_names(call_node: &Node, source: &[u8]) -> Vec } /// Collect names from `const { a, b } = await import(...)` -fn collect_object_pattern_names(pattern: &Node, source: &[u8]) -> Vec { +fn collect_object_pattern_names( + pattern: &Node, + source: &[u8], + renamed_out: &mut Vec, +) -> Vec { let mut names = Vec::new(); for i in 0..pattern.child_count() { let Some(child) = pattern.child(i) else { continue }; @@ -3124,9 +3148,46 @@ fn collect_object_pattern_names(pattern: &Node, source: &[u8]) -> Vec { names.push(node_text(&child, source).to_string()); } "pair_pattern" | "pair" => { - // { exportName: localAlias } → extract the key (export name) - if let Some(key) = child.child_by_field_name("key") { - names.push(node_text(&key, source).to_string()); + // { imported: local } → the local binding (`value`) is what + // call sites actually reference; `key` is the name exported + // by the target module. Preferring `key` unconditionally (as + // this branch used to) silently dropped the local alias for + // every renamed destructure — the same class of bug fixed for + // static `import { X as Y }` specifiers in #1730 (#1824). + let key = child.child_by_field_name("key"); + let value = child.child_by_field_name("value"); + let local_node = match value.map(|v| (v.kind(), v)) { + Some(("identifier", v)) | Some(("shorthand_property_identifier_pattern", v)) => { + Some(v) + } + Some(("assignment_pattern", v)) => { + // { imported: local = defaultValue } — the local + // binding is the assignment_pattern's left identifier. + v.child_by_field_name("left") + .filter(|left| left.kind() == "identifier") + } + _ => None, + }; + match (local_node, key) { + (Some(local_node), Some(key)) => { + let local_text = node_text(&local_node, source).to_string(); + let key_text = node_text(&key, source).to_string(); + if local_text != key_text { + renamed_out.push(RenamedImport { + local: local_text.clone(), + imported: key_text, + }); + } + names.push(local_text); + } + (None, Some(key)) => { + // Nested pattern (`{ foo: { nested } }`) or other + // unsupported value shape — no single local binding + // to extract; fall back to the key so the specifier + // isn't dropped entirely. + names.push(node_text(&key, source).to_string()); + } + _ => {} } } "object_assignment_pattern" => { @@ -4318,12 +4379,25 @@ mod tests { #[test] fn finds_dynamic_import_with_aliased_destructuring() { + // #1824: the local binding actually referenced by call sites + // (`fromBarrel`) must be recorded in `names`, not the name exported by + // the target module (`buildGraph`) — mirrors the static + // `import { X as Y }` fix from #1730. `renamed_imports` carries the + // local → original mapping so call-edge resolution can still find + // `buildGraph` in the target file. let s = parse_js("const { buildGraph: fromBarrel } = await import('./builder.js');"); let dyn_imports: Vec<_> = s.imports.iter().filter(|i| i.dynamic_import == Some(true)).collect(); assert_eq!(dyn_imports.len(), 1); assert_eq!(dyn_imports[0].source, "./builder.js"); - assert!(dyn_imports[0].names.contains(&"buildGraph".to_string())); - assert!(!dyn_imports[0].names.contains(&"fromBarrel".to_string())); + assert!(dyn_imports[0].names.contains(&"fromBarrel".to_string())); + assert!(!dyn_imports[0].names.contains(&"buildGraph".to_string())); + let renamed = dyn_imports[0] + .renamed_imports + .as_ref() + .expect("renamed_imports should be populated for a renamed destructure"); + assert_eq!(renamed.len(), 1); + assert_eq!(renamed[0].local, "fromBarrel"); + assert_eq!(renamed[0].imported, "buildGraph"); } #[test] @@ -4333,9 +4407,16 @@ mod tests { assert_eq!(dyn_imports.len(), 1); assert_eq!(dyn_imports[0].source, "./mod.js"); assert!(dyn_imports[0].names.contains(&"a".to_string())); - assert!(dyn_imports[0].names.contains(&"buildGraph".to_string())); + assert!(dyn_imports[0].names.contains(&"fromBarrel".to_string())); assert!(dyn_imports[0].names.contains(&"c".to_string())); - assert!(!dyn_imports[0].names.contains(&"fromBarrel".to_string())); + assert!(!dyn_imports[0].names.contains(&"buildGraph".to_string())); + let renamed = dyn_imports[0] + .renamed_imports + .as_ref() + .expect("renamed_imports should be populated for a renamed destructure"); + assert_eq!(renamed.len(), 1); + assert_eq!(renamed[0].local, "fromBarrel"); + assert_eq!(renamed[0].imported, "buildGraph"); } #[test] @@ -4343,8 +4424,15 @@ mod tests { let s = parse_js("const { buildGraph: local = null } = await import('./builder.js');"); let dyn_imports: Vec<_> = s.imports.iter().filter(|i| i.dynamic_import == Some(true)).collect(); assert_eq!(dyn_imports.len(), 1); - assert!(dyn_imports[0].names.contains(&"buildGraph".to_string())); - assert!(!dyn_imports[0].names.contains(&"local".to_string())); + assert!(dyn_imports[0].names.contains(&"local".to_string())); + assert!(!dyn_imports[0].names.contains(&"buildGraph".to_string())); + let renamed = dyn_imports[0] + .renamed_imports + .as_ref() + .expect("renamed_imports should be populated for a renamed destructure"); + assert_eq!(renamed.len(), 1); + assert_eq!(renamed[0].local, "local"); + assert_eq!(renamed[0].imported, "buildGraph"); } #[test] diff --git a/src/extractors/javascript.ts b/src/extractors/javascript.ts index ac5ec8a9..95d17325 100644 --- a/src/extractors/javascript.ts +++ b/src/extractors/javascript.ts @@ -810,12 +810,14 @@ function collectDynamicImport(node: TreeSitterNode, imports: Import[]): boolean const strArg = findChild(args, 'string'); if (strArg) { const modPath = strArg.text.replace(/['"]/g, ''); - const names = extractDynamicImportNames(node); + const renamedImports: Array<{ local: string; imported: string }> = []; + const names = extractDynamicImportNames(node, renamedImports); imports.push({ source: modPath, names, line: nodeStartLine(node), dynamicImport: true, + ...(renamedImports.length > 0 ? { renamedImports } : {}), }); } else { debug( @@ -1555,8 +1557,15 @@ function handleDynamicImportCall(node: TreeSitterNode, imports: Import[]): void const strArg = findChild(args, 'string'); if (strArg) { const modPath = strArg.text.replace(/['"]/g, ''); - const names = extractDynamicImportNames(node); - imports.push({ source: modPath, names, line: nodeStartLine(node), dynamicImport: true }); + const renamedImports: Array<{ local: string; imported: string }> = []; + const names = extractDynamicImportNames(node, renamedImports); + imports.push({ + source: modPath, + names, + line: nodeStartLine(node), + dynamicImport: true, + ...(renamedImports.length > 0 ? { renamedImports } : {}), + }); } else { debug( `Skipping non-static dynamic import() at line ${nodeStartLine(node)} (template literal or variable)`, @@ -4177,13 +4186,22 @@ const DYNAMIC_IMPORT_WRAPPER_TYPES = new Set([ * const { a, b } = await import('./foo.js') → ['a', 'b'] * const mod = await import('./foo.js') → ['mod'] * const { a } = (await import('./foo.js')) as { a: Fn } → ['a'] + * const { a: b } = await import('./foo.js') → ['b'] * import('./foo.js') → [] (no names extractable) * * Walks up the AST from the call_expression — through any nesting of * await/parenthesized/as-cast wrappers — to find the enclosing * variable_declarator and reads the name/object_pattern. + * + * `renamedOut`, when supplied, is populated with `{ local, imported }` pairs + * for every `{ imported: local }` specifier — mirrors `extractImportNames`'s + * static-import convention (#1730) so call-edge resolution can recover the + * original exported name when a call site uses the local alias (#1824). */ -function extractDynamicImportNames(callNode: TreeSitterNode): string[] { +function extractDynamicImportNames( + callNode: TreeSitterNode, + renamedOut?: Array<{ local: string; imported: string }>, +): string[] { // Walk up through await_expression / parenthesized_expression / as_expression // wrappers, in any combination or order, to reach the variable_declarator. let current = callNode.parent; @@ -4204,10 +4222,37 @@ function extractDynamicImportNames(callNode: TreeSitterNode): string[] { if (child.type === 'shorthand_property_identifier_pattern') { names.push(child.text); } else if (child.type === 'pair_pattern') { - // { a: localName } → use localName (the alias) for the local binding, - // but use the key (original name) for import resolution + // { imported: local } → the local binding (`value`) is what call + // sites actually reference; `key` is the name exported by the target + // module. Preferring `key` unconditionally (as this branch used to) + // silently dropped the local alias for every renamed destructure, + // the same class of bug fixed for static `import { X as Y }` + // specifiers in #1730 (#1824). const key = child.childForFieldName('key'); - if (key) names.push(key.text); + const value = child.childForFieldName('value'); + let localNode: TreeSitterNode | undefined; + if ( + value?.type === 'identifier' || + value?.type === 'shorthand_property_identifier_pattern' + ) { + localNode = value; + } else if (value?.type === 'assignment_pattern') { + // { imported: local = defaultValue } — the local binding is the + // assignment_pattern's left-hand identifier. + const left = value.childForFieldName('left'); + if (left?.type === 'identifier') localNode = left; + } + if (localNode && key) { + names.push(localNode.text); + if (localNode.text !== key.text) { + renamedOut?.push({ local: localNode.text, imported: key.text }); + } + } else if (key) { + // Nested pattern (`{ foo: { nested } }`) or other unsupported + // value shape — no single local binding to extract; fall back to + // the key so the specifier isn't dropped entirely. + names.push(key.text); + } } } return names; diff --git a/src/types.ts b/src/types.ts index d22e7196..ecc8bf83 100644 --- a/src/types.ts +++ b/src/types.ts @@ -529,6 +529,10 @@ export interface Import { * `extractImportNames`), so `resolveBarrelExport` uses this map to translate * a consumer's requested external name back to X before matching against * `names`/looking up the underlying definition (#1823). + * + * Also populated for dynamic `import()` destructuring renames + * (`const { X: Y } = await import(...)`): same local/original split as the + * static case, produced by `extractDynamicImportNames` (#1824). */ renamedImports?: Array<{ local: string; imported: string }>; /** diff --git a/tests/integration/issue-1824-dynamic-import-destructure-rename.test.ts b/tests/integration/issue-1824-dynamic-import-destructure-rename.test.ts new file mode 100644 index 00000000..90b06e79 --- /dev/null +++ b/tests/integration/issue-1824-dynamic-import-destructure-rename.test.ts @@ -0,0 +1,173 @@ +/** + * Integration test for #1824: `const { a: b } = await import(...)` recorded + * the wrong binding name, dropping the call edge for `b(...)`. + * + * Root cause: `extractDynamicImportNames` (TS) / `extract_dynamic_import_names` + * (Rust) preferred the tree-sitter `pair_pattern`'s `key` field (the name + * exported by the target module) over `value` (the local binding actually + * referenced by call sites) — the same class of bug fixed for static + * `import { X as Y }` specifiers in #1730. Since `names` held the original + * exported name (`a`) instead of the local alias (`b`), `importedNames` never + * gained an entry for `b`, so a call to `b(...)` never resolved to the + * imported symbol. + * + * Fix: `names` now carries the local alias, with the local -> original + * mapping recorded in `Import.renamedImports` so call-edge resolution can + * still find the target module's real export — mirroring #1730's + * `renamedImports` mechanism for static imports. + */ + +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'; +import { isNativeAvailable } from '../../src/infrastructure/native.js'; + +const DEAD_ROLES = new Set(['dead-unresolved', 'dead-leaf', 'dead-entry', 'dead-ffi']); + +const TARGET_FILE = 'features/math.ts'; + +const FIXTURE = { + [TARGET_FILE]: ` +export function add(a: number, b: number): number { + return a + b; +} +`, + 'domain/stages/consumer.ts': ` +export async function usesRenamedImport() { + const { add: sum } = await import('../../features/math.js'); + return sum(1, 2); +} +`, +}; + +function getCallEdges(dbPath: string) { + const db = new Database(dbPath, { readonly: true }); + try { + return db + .prepare( + `SELECT n1.name AS src, 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; tgt: string; tgt_file: string }>; + } finally { + db.close(); + } +} + +function readNodesWithRoles(dbPath: string) { + const db = new Database(dbPath, { readonly: true }); + try { + return db.prepare('SELECT name, kind, role FROM nodes ORDER BY name').all() as Array<{ + name: string; + kind: string; + role: string | null; + }>; + } finally { + db.close(); + } +} + +function writeFixture(rootDir: string) { + for (const [rel, content] of Object.entries(FIXTURE)) { + const abs = path.join(rootDir, rel); + fs.mkdirSync(path.dirname(abs), { recursive: true }); + fs.writeFileSync(abs, content); + } +} + +describe('dynamic import() destructuring rename (#1824) — WASM', () => { + let tmpDir: string; + + beforeAll(async () => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-1824-wasm-')); + writeFixture(tmpDir); + await buildGraph(tmpDir, { engine: 'wasm', incremental: false, skipRegistry: true }); + }); + + afterAll(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + it('creates a calls edge through the local alias, not the original name', () => { + const dbPath = path.join(tmpDir, '.codegraph', 'graph.db'); + const edges = getCallEdges(dbPath); + const edge = edges.find((e) => e.src === 'usesRenamedImport' && e.tgt === 'add'); + expect(edge, `Expected usesRenamedImport -> add; got: ${JSON.stringify(edges)}`).toBeDefined(); + expect(edge?.tgt_file).toBe(TARGET_FILE); + }); + + it('codegraph exports credits the dynamically-imported, renamed export with a real consumer', () => { + const dbPath = path.join(tmpDir, '.codegraph', 'graph.db'); + const data = exportsData(TARGET_FILE, dbPath); + const addExport = data.results.find((r: { name: string }) => r.name === 'add'); + expect(addExport).toBeDefined(); + expect(addExport.consumerCount).toBeGreaterThanOrEqual(1); + expect(addExport.consumers.map((c: { name: string }) => c.name)).toContain('usesRenamedImport'); + }); + + it('does not classify the dynamically-imported export as dead', () => { + const dbPath = path.join(tmpDir, '.codegraph', 'graph.db'); + const nodes = readNodesWithRoles(dbPath); + const node = nodes.find((n) => n.name === 'add' && n.kind === 'function'); + expect(node, 'add node not found').toBeDefined(); + expect( + DEAD_ROLES.has(node!.role ?? ''), + `add was classified as ${node!.role} — expected a non-dead role now that a real edge exists`, + ).toBe(false); + }); +}); + +// ── Native engine parity ──────────────────────────────────────────────────── +// Skipped when the native addon is not installed. + +describe.skipIf(!isNativeAvailable())( + 'dynamic import() destructuring rename (#1824) — native', + () => { + let nativeTmpDir: string; + + beforeAll(async () => { + nativeTmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-1824-native-')); + writeFixture(nativeTmpDir); + await buildGraph(nativeTmpDir, { engine: 'native', incremental: false, skipRegistry: true }); + }, 60_000); + + afterAll(() => { + fs.rmSync(nativeTmpDir, { recursive: true, force: true }); + }); + + it('creates a calls edge through the local alias, not the original name', () => { + const dbPath = path.join(nativeTmpDir, '.codegraph', 'graph.db'); + const edges = getCallEdges(dbPath); + const edge = edges.find((e) => e.src === 'usesRenamedImport' && e.tgt === 'add'); + expect( + edge, + `Expected native usesRenamedImport -> add; got: ${JSON.stringify(edges)}`, + ).toBeDefined(); + expect(edge?.tgt_file).toBe(TARGET_FILE); + }); + + it('codegraph exports credits the dynamically-imported, renamed export with a real consumer', () => { + const dbPath = path.join(nativeTmpDir, '.codegraph', 'graph.db'); + const data = exportsData(TARGET_FILE, dbPath); + const addExport = data.results.find((r: { name: string }) => r.name === 'add'); + expect(addExport).toBeDefined(); + expect(addExport.consumerCount).toBeGreaterThanOrEqual(1); + }); + + it('does not classify the dynamically-imported export as dead', () => { + const dbPath = path.join(nativeTmpDir, '.codegraph', 'graph.db'); + const nodes = readNodesWithRoles(dbPath); + const node = nodes.find((n) => n.name === 'add' && n.kind === 'function'); + expect(node, 'add node not found (native)').toBeDefined(); + expect(DEAD_ROLES.has(node!.role ?? '')).toBe(false); + }); + }, +); diff --git a/tests/parsers/javascript.test.ts b/tests/parsers/javascript.test.ts index 6ff5f94f..ae5aded5 100644 --- a/tests/parsers/javascript.test.ts +++ b/tests/parsers/javascript.test.ts @@ -298,6 +298,57 @@ describe('JavaScript parser', () => { }); }); + describe('dynamic import() destructuring rename (#1824)', () => { + function parseTS(code) { + const parser = parsers.get('typescript'); + const tree = parser.parse(code); + return extractSymbols(tree, 'test.ts'); + } + + // `extractDynamicImportNames`'s pair_pattern branch preferred the + // tree-sitter `key` field (the name exported by the target module) over + // `value` (the local binding actually referenced by call sites) — the + // same class of bug fixed for static `import { X as Y }` specifiers in + // #1730. `names` must carry the local alias, with the local -> original + // mapping recorded in `renamedImports` so call-edge resolution can still + // find the target module's real export. + + it('records the local alias, not the exported name, for a renamed destructure', () => { + const symbols = parseJS(`const { a: b } = await import('./mod.js');`); + expect(symbols.imports).toHaveLength(1); + expect(symbols.imports[0].names).toEqual(['b']); + expect(symbols.imports[0].renamedImports).toEqual([{ local: 'b', imported: 'a' }]); + }); + + it('handles a mix of renamed and plain destructured bindings', () => { + const symbols = parseJS(`const { a, realName: alias, c } = await import('./mod.js');`); + expect(symbols.imports).toHaveLength(1); + expect(symbols.imports[0].names).toEqual(['a', 'alias', 'c']); + expect(symbols.imports[0].renamedImports).toEqual([{ local: 'alias', imported: 'realName' }]); + }); + + it('does not record renamedImports when no specifier is renamed', () => { + const symbols = parseJS(`const { a, b } = await import('./mod.js');`); + expect(symbols.imports[0].names).toEqual(['a', 'b']); + expect(symbols.imports[0].renamedImports).toBeUndefined(); + }); + + it('records the local alias through a default value on a renamed destructure', () => { + const symbols = parseJS(`const { a: b = null } = await import('./mod.js');`); + expect(symbols.imports[0].names).toEqual(['b']); + expect(symbols.imports[0].renamedImports).toEqual([{ local: 'b', imported: 'a' }]); + }); + + it('records the rename through parens + as-cast wrappers', () => { + const symbols = parseTS( + `const { realName: alias } = (await import('./mod.js')) as { realName: Fn };`, + ); + expect(symbols.imports).toHaveLength(1); + expect(symbols.imports[0].names).toEqual(['alias']); + expect(symbols.imports[0].renamedImports).toEqual([{ local: 'alias', imported: 'realName' }]); + }); + }); + it('extracts call expressions', () => { const symbols = parseJS(`import { foo } from './bar'; foo(); baz();`); expect(symbols.calls).toContainEqual(expect.objectContaining({ name: 'foo' }));