Skip to content

fix(extractors): emit per-element definitions for array-pattern destructuring#2038

Open
carlos-alm wants to merge 1 commit into
fix/issue-1897-engine-parity-wasm-doesnt-resolve-inlinefrom
fix/issue-1901-engine-parity-native-js-extractor-emits-no
Open

fix(extractors): emit per-element definitions for array-pattern destructuring#2038
carlos-alm wants to merge 1 commit into
fix/issue-1897-engine-parity-wasm-doesnt-resolve-inlinefrom
fix/issue-1901-engine-parity-native-js-extractor-emits-no

Conversation

@carlos-alm

Copy link
Copy Markdown
Contributor

Summary

For a top-level const [a, b] = someCall(); declaration, both engines previously emitted a single constant Definition whose name was the raw pattern source text (e.g. "[a, b]") — never a real identifier and never itself a valid call target:

Both engines now emit one constant Definition per bound identifier in the array pattern — mirroring how object-pattern destructuring already works per-property (extractDestructuredBindings / extract_destructured_bindings, fixed in #1773/#1902) — via new extractArrayPatternBindings (WASM/TS) and extract_array_pattern_bindings (native) helpers, shared by both extraction paths on the WASM side. Covers plain identifiers ([a, b]), default-value bindings ([a = 1]), and rest bindings ([...rest]).

Why this shape

Per the issue's own suggested fix: a single node named [a, b] is not a real identifier and can never resolve as a call target even though a/b themselves might be. Per-element extraction makes a()/b() call sites resolvable, exactly like the already-fixed object-pattern case.

Out-of-scope finding

While verifying the rest-binding (...rest) child-node structure via direct AST inspection, found that the unrelated dynamic-import/CJS-require name-collection helpers (extract_rest_identifier in Rust, and the array_pattern branch of extractDynamicImportNames in TS) read the wrong child index for rest_pattern and never actually extract the rest-bound name. Filed as #2037; not fixed here (different code path, different concern).

Test plan

  • npx vitest run tests/parsers/javascript.test.ts — updated the two existing array-pattern tests to the new per-element shape, added new default/rest-binding coverage
  • New integration test tests/integration/issue-1901-array-pattern-definition.test.ts — builds a real fixture with both engine: 'wasm' and engine: 'native', asserts identical per-element constant definitions in both, and that the old raw-pattern-text node is gone
  • cargo test --package codegraph-core — 592 passed (added 2 new tests for the array-pattern helper: default/rest bindings, and updated the existing call-expression-initializer test)
  • npm test — full suite green (249 files, 3982 passed)
  • npx vitest run tests/benchmarks/resolution/resolution-benchmark.test.ts (javascript/typescript/dynamic-javascript/pts-javascript) — no precision/recall regression
  • npx vitest run tests/benchmarks/resolution/jelly-micro.test.ts — all 64 fixtures pass, including destructuring (the fixture containing const [x, y] = new Set([...]))
  • npm run lint — clean (files touched); npm run build — clean; native addon rebuilt via napi build --platform --release, codesigned, verified directly against both engines

Stacked on #1897/#1900/#1902 (base branch fix/issue-1897-engine-parity-wasm-doesnt-resolve-inline) — only the array-pattern extractor/test diff is this issue's change.

…ucturing

Both engines previously emitted a single "constant" Definition for a
top-level const array pattern (const [a, b] = fn()) whose name was the raw
pattern source text (e.g. "[a, b]") -- never a real identifier and never a
valid call target. The native Rust extractor additionally emitted no
Definition at all for array_pattern name nodes, a genuine engine-parity gap.

Both engines now emit one "constant" Definition per bound identifier,
mirroring how object-pattern destructuring already works per-property
(#1773): extractArrayPatternBindings (WASM/TS) and
extract_array_pattern_bindings (native), covering plain identifiers,
default-value bindings, and rest bindings.

Fixes #1901

Impact: 3 functions changed, 8 affected
@greptile-apps

greptile-apps Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes engine parity for top-level const [a, b] = fn() declarations: both the WASM/TS extractor and the native Rust extractor now emit one constant Definition per bound identifier, replacing the previous single-node-named-by-raw-pattern-text approach (\"[a, b]\") that was never resolvable as a call target.

  • Core fix (TS & Rust): New extractArrayPatternBindings / extract_array_pattern_bindings helpers handle plain identifiers, default-value bindings ([a = 1]), and rest bindings ([...rest]); wired into both the walk path and the query path on the WASM side, and handle_var_decl on the native side.
  • Test coverage: Two updated unit tests, two new unit tests, and a full integration test exercising both engines against a real fixture.
  • Out-of-scope: The pre-existing wrong-child-index bug in extract_rest_identifier (Rust) and extractDynamicImportNames (TS) is filed as JS/TS: rest-pattern identifier never extracted in dynamic-import/CJS-require name collection (wrong child index) #2037 and not touched here.

Confidence Score: 5/5

The change is tightly scoped to the array-pattern extraction path, gated by the same function-scope guard that protects object-pattern handling, and exercised end-to-end by both unit and integration tests across both engines.

Both TS and Rust implementations mirror the already-working object-pattern counterparts. The only observation is a minor operator-ordering issue in the TS rest-binding lookup with no practical impact, as rest_pattern always places the identifier at child index 1.

No files require special attention; all four changed files are straightforward and well-covered by the new tests.

Important Files Changed

Filename Overview
crates/codegraph-core/src/extractors/javascript.rs Adds extract_array_pattern_bindings helper and wires it into handle_var_decl, correctly splitting the old single-node-per-pattern approach into per-identifier definitions.
src/extractors/javascript.ts Adds extractArrayPatternBindings helper and updates both walk path and query path to call it, replacing the raw-pattern-text approach with per-element constant definitions.
tests/integration/issue-1901-array-pattern-definition.test.ts New integration test exercising both WASM and native engines with assertions on per-element constant definitions and absence of old raw-pattern-text nodes.
tests/parsers/javascript.test.ts Updates existing array-pattern tests and adds new cases for default-value and rest bindings.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["handle_var_decl / handleVariableDeclarator"] --> B{name node kind?}
    B -- identifier --> C[push constant Definition]
    B -- object_pattern --> F[extract_destructured_bindings]
    B -- array_pattern --> G["extract_array_pattern_bindings (NEW)"]
    G --> H{child kind?}
    H -- identifier --> I[push constant Definition]
    H -- assignment_pattern --> J[left identifier → push constant Definition]
    H -- rest_pattern/rest_element --> K[scan for identifier → push constant Definition]
    H -- other/hole --> L[skip]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["handle_var_decl / handleVariableDeclarator"] --> B{name node kind?}
    B -- identifier --> C[push constant Definition]
    B -- object_pattern --> F[extract_destructured_bindings]
    B -- array_pattern --> G["extract_array_pattern_bindings (NEW)"]
    G --> H{child kind?}
    H -- identifier --> I[push constant Definition]
    H -- assignment_pattern --> J[left identifier → push constant Definition]
    H -- rest_pattern/rest_element --> K[scan for identifier → push constant Definition]
    H -- other/hole --> L[skip]
Loading

Fix All in Claude Code

Reviews (1): Last reviewed commit: "fix(extractors): emit per-element defini..." | Re-trigger Greptile

Comment on lines +1409 to +1412
// [...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') {

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

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Codegraph Impact Analysis

3 functions changed8 callers affected across 1 files

  • extractDestructuredDeclarators in src/extractors/javascript.ts:798 (3 transitive callers)
  • extractArrayPatternBindings in src/extractors/javascript.ts:1390 (6 transitive callers)
  • handleVariableDeclarator in src/extractors/javascript.ts:1434 (3 transitive callers)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant