Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 108 additions & 13 deletions crates/codegraph-core/src/extractors/javascript.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1534,14 +1534,11 @@ fn handle_var_decl(node: &Node, source: &[u8], symbols: &mut FileSymbols) {
}
}
}
} else if is_const
&& (name_n.kind() == "identifier" || name_n.kind() == "array_pattern")
&& !in_function_scope
{
} else if is_const && name_n.kind() == "identifier" && !in_function_scope {
// Any other initializer shape becomes a "constant" Definition, regardless of
// complexity (call/member/parenthesized expressions, etc.) — mirroring how
// function declarations are captured regardless of body complexity, and the
// WASM/TS extractor's unconditional identifier + array_pattern branches (#1819).
// WASM/TS extractor's unconditional identifier branch (#1819).
symbols.definitions.push(Definition {
name: node_text(&name_n, source).to_string(),
kind: "constant".to_string(),
Expand All @@ -1555,10 +1552,14 @@ fn handle_var_decl(node: &Node, source: &[u8], symbols: &mut FileSymbols) {
// Phase 8.3f: extract function/arrow properties from object literals and seed
// typeMap composite keys so that this.method() inside Object.defineProperty
// accessor functions can resolve them.
if value_n.kind() == "object" && name_n.kind() == "identifier" {
if value_n.kind() == "object" {
let var_name = node_text(&name_n, source);
extract_object_literal_functions(&value_n, source, var_name, symbols);
}
} else if is_const && name_n.kind() == "array_pattern" && !in_function_scope {
// Array destructuring: `const [x, y] = ...` — one constant Definition per
// bound identifier (#1901). Scope guard mirrors the object_pattern branch above.
extract_array_pattern_bindings(&name_n, source, start_line(node), end_line(node), &mut symbols.definitions);
} else if !is_const && value_n.kind() == "object" && name_n.kind() == "identifier" && !in_function_scope {
// `let`/`var` object literals get no "constant" definition of their own (mirrors
// WASM extractLetVarObjLiteralDeclarators) but still need their function/method
Expand Down Expand Up @@ -3027,6 +3028,80 @@ fn extract_destructured_bindings(
}
}

/// Extract a per-element "constant" Definition from each bound identifier in
/// an array-destructuring pattern (`const [a, b] = fn()`) — the array-pattern
/// counterpart to `extract_destructured_bindings`'s per-property handling of
/// object patterns (#1773). Each bound name becomes its own resolvable node,
/// superseding the prior single-node-named-by-raw-pattern-text approach
/// (`[a, b]` as one unresolvable node), which was never a real identifier and
/// could never be a call target (#1901). Mirrors the TS extractor's
/// `extractArrayPatternBindings`.
fn extract_array_pattern_bindings(
pattern: &Node,
source: &[u8],
line: u32,
end_line: u32,
definitions: &mut Vec<Definition>,
) {
for i in 0..pattern.child_count() {
let Some(child) = pattern.child(i) else { continue };
match child.kind() {
"identifier" => {
definitions.push(Definition {
name: node_text(&child, source).to_string(),
kind: "constant".to_string(),
line,
end_line: Some(end_line),
decorators: None,
complexity: None,
cfg: None,
children: None,
});
}
"assignment_pattern" => {
if let Some(left) = child.child_by_field_name("left") {
if left.kind() == "identifier" {
definitions.push(Definition {
name: node_text(&left, source).to_string(),
kind: "constant".to_string(),
line,
end_line: Some(end_line),
decorators: None,
complexity: None,
cfg: None,
children: None,
});
}
}
}
"rest_pattern" | "rest_element" => {
// The identifier is at child index 1 (index 0 is the `...`
// token itself) — mirroring extract_js_parameters' own
// rest_pattern handling, which scans all children for the
// identifier rather than assuming a fixed index.
for j in 0..child.child_count() {
if let Some(inner) = child.child(j) {
if inner.kind() == "identifier" {
definitions.push(Definition {
name: node_text(&inner, source).to_string(),
kind: "constant".to_string(),
line,
end_line: Some(end_line),
decorators: None,
complexity: None,
cfg: None,
children: None,
});
break;
}
}
}
}
_ => {}
}
}
}

/// Mirrors `extractReceiverName` in src/extractors/javascript.ts: normalize a
/// call receiver node to a resolvable name. Inline-new (`new Foo().method()`)
/// and single-paren-wrapped new (`(new Foo()).method()`) yield the constructor
Expand Down Expand Up @@ -4653,14 +4728,34 @@ mod tests {
#[test]
fn extracts_const_array_pattern_with_call_expression_initializer() {
// Parity with the identifier case above: array-pattern names must also
// be discoverable regardless of initializer complexity.
// be discoverable regardless of initializer complexity — one
// definition per bound identifier (#1901), not a single node named
// by the raw pattern text.
let s = parse_js("const [a, b] = computePair();");
let def = s
.definitions
.iter()
.find(|d| d.name == "[a, b]")
.unwrap_or_else(|| panic!("[a, b] should be extracted as a definition"));
assert_eq!(def.kind, "constant");
for name in ["a", "b"] {
let def = s
.definitions
.iter()
.find(|d| d.name == name)
.unwrap_or_else(|| panic!("{name} should be extracted as a definition"));
assert_eq!(def.kind, "constant");
}
assert!(!s.definitions.iter().any(|d| d.name == "[a, b]"));
}

#[test]
fn extracts_array_pattern_default_and_rest_bindings_as_own_definitions() {
// #1901: array-pattern default-value and rest bindings each become
// their own "constant" Definition, matching the plain-identifier case.
let s = parse_js("const [a = 1, ...rest] = computeList();");
for name in ["a", "rest"] {
let def = s
.definitions
.iter()
.find(|d| d.name == name)
.unwrap_or_else(|| panic!("{name} should be extracted as a definition"));
assert_eq!(def.kind, "constant");
}
}

#[test]
Expand Down
66 changes: 49 additions & 17 deletions src/extractors/javascript.ts
Original file line number Diff line number Diff line change
Expand Up @@ -827,14 +827,13 @@ function extractDestructuredDeclarators(
if (binding) cjsRequireBindings.push(binding);
}
} else if (nameN && nameN.type === 'array_pattern') {
// `const [x, y] = ...` — emit a single constant node whose name is the
// full array pattern text (e.g. `[x, y]`), matching native engine behaviour.
definitions.push({
name: nameN.text,
kind: 'constant',
line: nodeStartLine(declNode),
endLine: nodeEndLine(declNode),
});
// `const [x, y] = ...` — one constant Definition per bound identifier (#1901).
extractArrayPatternBindings(
nameN,
nodeStartLine(declNode),
nodeEndLine(declNode),
definitions,
);
}
}
}
Expand Down Expand Up @@ -1378,6 +1377,45 @@ function extractDestructuredBindings(
}
}

/**
* Extract a per-element `constant` Definition from each bound identifier in an
* array-destructuring pattern (`const [a, b] = fn()`) — the array-pattern
* counterpart to `extractDestructuredBindings`'s per-property handling of
* object patterns (#1773). Each bound name becomes its own resolvable node
* (e.g. `a()`, `b()` calls can resolve to `a`/`b` directly), superseding the
* prior single-node-named-by-raw-pattern-text approach (`[a, b]` as one
* unresolvable node), which was never a real identifier and could never be a
* call target (#1901).
*/
function extractArrayPatternBindings(
pattern: TreeSitterNode,
line: number,
endLine: number,
definitions: Definition[],
): void {
for (let i = 0; i < pattern.childCount; i++) {
const child = pattern.child(i);
if (!child) continue;
if (child.type === 'identifier') {
// [a, b] — plain positional binding
definitions.push({ name: child.text, kind: 'constant', line, endLine });
} else if (child.type === 'assignment_pattern') {
// [a = defaultValue] — the bound name is the left-hand identifier
const left = child.childForFieldName('left');
if (left && left.type === 'identifier') {
definitions.push({ name: left.text, kind: 'constant', line, endLine });
}
} else if (child.type === 'rest_pattern' || child.type === 'rest_element') {
// [...rest] — the identifier is at child index 1 (index 0 is the `...`
// token itself), matching extractParameters' own rest_pattern handling.
const inner = child.child(1) || child.childForFieldName('name');
if (inner && inner.type === 'identifier') {
Comment on lines +1409 to +1412

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The || fallback to childForFieldName('name') only fires when child.child(1) returns a falsy value (null/undefined). If child(1) returns a truthy non-identifier node (e.g., array_pattern in a nested rest like ...[a, b]), the childForFieldName('name') branch is never reached and the definition is silently skipped. The Rust implementation avoids this by scanning all children for the first identifier. Flipping the order to prefer childForFieldName with a positional fallback would be more explicit about the intent.

Suggested change
// [...rest] — the identifier is at child index 1 (index 0 is the `...`
// token itself), matching extractParameters' own rest_pattern handling.
const inner = child.child(1) || child.childForFieldName('name');
if (inner && inner.type === 'identifier') {
// [...rest] — the identifier is the named child (field 'name'), with a
// positional fallback for grammar versions that don't expose the field.
const inner = child.childForFieldName('name') ?? child.child(1);
if (inner && inner.type === 'identifier') {

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code

definitions.push({ name: inner.text, kind: 'constant', line, endLine });
}
}
}
}

function handleVariableDecl(node: TreeSitterNode, ctx: ExtractorOutput): void {
const isConst = node.text.startsWith('const ');
for (let i = 0; i < node.childCount; i++) {
Expand Down Expand Up @@ -1431,15 +1469,9 @@ function handleVariableDeclarator(
} else if (isConst && nameN.type === 'object_pattern' && !hasFunctionScopeAncestor(node)) {
handleConstObjectPatternAssignment(node, nameN, valueN, ctx);
} else if (isConst && nameN.type === 'array_pattern' && !hasFunctionScopeAncestor(node)) {
// Array destructuring: `const [x, y] = ...` — emit a single constant node
// whose name is the full array pattern text (e.g. `[x, y]`), matching
// native engine behaviour. Scope guard mirrors the object_pattern branch above.
ctx.definitions.push({
name: nameN.text,
kind: 'constant',
line: nodeStartLine(node),
endLine: nodeEndLine(node),
});
// Array destructuring: `const [x, y] = ...` — one constant Definition per
// bound identifier (#1901). Scope guard mirrors the object_pattern branch above.
extractArrayPatternBindings(nameN, nodeStartLine(node), nodeEndLine(node), ctx.definitions);
}
}

Expand Down
103 changes: 103 additions & 0 deletions tests/integration/issue-1901-array-pattern-definition.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
/**
* Integration test for #1901: engine parity for `const [a, b] = fn();`
* top-level array-pattern destructuring.
*
* Root cause: the native Rust extractor (`crates/codegraph-core/src/
* extractors/javascript.rs`, `handle_var_decl`) emitted no `Definition` at
* all for array-pattern name nodes, while the WASM/TS extractor
* (`src/extractors/javascript.ts`, both the walk path in
* `handleVariableDeclarator` and the query path in
* `extractDestructuredDeclarators`) emitted a single `Definition` whose
* `name` was the raw pattern source text (e.g. `"[a, b]"`) — not a real
* identifier, and never itself a valid call target.
*
* Fix: both engines now emit one `constant`-kind `Definition` per bound
* identifier (`a`, `b`, ...), mirroring how object-pattern destructuring
* already works per-property (#1773) — via `extractArrayPatternBindings`
* (WASM/TS) and `extract_array_pattern_bindings` (native).
*/

import fs from 'node:fs';
import os from 'node:os';
import path from 'node:path';
import Database from 'better-sqlite3';
import { afterAll, beforeAll, describe, expect, it } from 'vitest';
import { buildGraph } from '../../src/domain/graph/builder.js';
import { isNativeAvailable } from '../../src/infrastructure/native.js';

const FIXTURE = {
'sample.js': `
function getArr() { return [1, 2]; }
const [a, b] = getArr();
console.log(a, b);

function withDefaultsAndRest() { return [1, 2, 3]; }
const [c = 0, ...rest] = withDefaultsAndRest();
console.log(c, rest);
`,
};

function readNode(dbPath: string, file: string, name: string) {
const db = new Database(dbPath, { readonly: true });
try {
return db.prepare('SELECT name, kind FROM nodes WHERE file = ? AND name = ?').get(file, name) as
| { name: string; kind: string }
| undefined;
} finally {
db.close();
}
}

function expectPerElementBindings(dbPath: string) {
for (const name of ['a', 'b', 'c', 'rest']) {
const node = readNode(dbPath, 'sample.js', name);
expect(node, `${name} node not found`).toBeDefined();
expect(node!.kind, `${name} must be kind constant`).toBe('constant');
}
// The old single-node-named-by-raw-pattern-text approach must be gone.
expect(readNode(dbPath, 'sample.js', '[a, b]')).toBeUndefined();
expect(readNode(dbPath, 'sample.js', '[c = 0, ...rest]')).toBeUndefined();
}

describe('array-pattern destructuring Definition extraction (#1901) — WASM', () => {
let tmpDir: string;

beforeAll(async () => {
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-1901-wasm-'));
for (const [rel, content] of Object.entries(FIXTURE)) {
fs.writeFileSync(path.join(tmpDir, rel), content);
}
await buildGraph(tmpDir, { engine: 'wasm', incremental: false, skipRegistry: true });
});

afterAll(() => {
fs.rmSync(tmpDir, { recursive: true, force: true });
});

it('extracts one constant definition per bound identifier', () => {
expectPerElementBindings(path.join(tmpDir, '.codegraph', 'graph.db'));
});
});

describe.skipIf(!isNativeAvailable())(
'array-pattern destructuring Definition extraction (#1901) — native',
() => {
let tmpDir: string;

beforeAll(async () => {
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-1901-native-'));
for (const [rel, content] of Object.entries(FIXTURE)) {
fs.writeFileSync(path.join(tmpDir, rel), content);
}
await buildGraph(tmpDir, { engine: 'native', incremental: false, skipRegistry: true });
}, 60_000);

afterAll(() => {
fs.rmSync(tmpDir, { recursive: true, force: true });
});

it('extracts one constant definition per bound identifier (previously emitted none)', () => {
expectPerElementBindings(path.join(tmpDir, '.codegraph', 'graph.db'));
});
},
);
29 changes: 25 additions & 4 deletions tests/parsers/javascript.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2341,16 +2341,34 @@ function runDemo(users: string[]): void {
});
});

describe('array destructuring constant extraction (#1471)', () => {
it('extracts const array pattern as a single constant node', () => {
describe('array destructuring constant extraction (#1471, #1901)', () => {
it('extracts one constant definition per bound identifier in a const array pattern', () => {
// Per-element extraction (#1901) supersedes the prior single-node
// ("[x, y]" as one unresolvable name) approach — `[x, y]` was never a
// real identifier and could never be a call target.
const symbols = parseJS(`const [x, y] = new Set([() => {}, () => {}]);`);
expect(symbols.definitions).toContainEqual(
expect.objectContaining({ name: '[x, y]', kind: 'constant' }),
expect.objectContaining({ name: 'x', kind: 'constant' }),
);
expect(symbols.definitions).toContainEqual(
expect.objectContaining({ name: 'y', kind: 'constant' }),
);
expect(symbols.definitions.every((d) => d.name !== '[x, y]')).toBe(true);
});

it('extracts the default-value binding and the rest binding as their own constants', () => {
const symbols = parseJS(`const [a = 1, ...rest] = computeList();`);
expect(symbols.definitions).toContainEqual(
expect.objectContaining({ name: 'a', kind: 'constant' }),
);
expect(symbols.definitions).toContainEqual(
expect.objectContaining({ name: 'rest', kind: 'constant' }),
);
});

it('does not extract let or var array destructuring', () => {
const symbols = parseJS(`let [a, b] = [1, 2];`);
expect(symbols.definitions.every((d) => d.name !== 'a' && d.name !== 'b')).toBe(true);
expect(symbols.definitions.every((d) => d.name !== '[a, b]')).toBe(true);
});
});
Expand Down Expand Up @@ -2496,7 +2514,10 @@ function runDemo(users: string[]): void {
it('extracts a const array pattern with a call-expression initializer (parity with identifier case)', () => {
const symbols = parseJS(`const [a, b] = computePair();`);
expect(symbols.definitions).toContainEqual(
expect.objectContaining({ name: '[a, b]', kind: 'constant' }),
expect.objectContaining({ name: 'a', kind: 'constant' }),
);
expect(symbols.definitions).toContainEqual(
expect.objectContaining({ name: 'b', kind: 'constant' }),
);
});
});
Expand Down
Loading