Skip to content

fix(extractors): extract CJS require() rest binding in TS/WASM engine - #2269

Merged
carlos-alm merged 1 commit into
mainfrom
fix/issue-2037-rest-pattern-child-index
Aug 4, 2026
Merged

fix(extractors): extract CJS require() rest binding in TS/WASM engine#2269
carlos-alm merged 1 commit into
mainfrom
fix/issue-2037-rest-pattern-child-index

Conversation

@carlos-alm

Copy link
Copy Markdown
Contributor

Summary

Issue #2037 diagnosed a wrong-child-index bug in both engines' rest-pattern
identifier extraction for dynamic-import()/CJS-require() name
collection: rest_pattern's child(0) is the ... token, not the bound
identifier, so const [a, ...rest] = fn()-style destructures silently
dropped rest.

By the time this was investigated, the exact locations #2037 cited had
already been fixed
— PR #2052 (merged 2026-07-11, closing #1920) fixed
extract_rest_identifier (Rust) and extractDynamicImportNames (TS) for
both array- and object-pattern dynamic import() destructures, in both
engines, three days after #2037 was filed. Verified via cargo test/
vitest (all existing #1920 regression tests pass) and a direct repro:
const [a, ...rest] = await import('./mod.js') already extracts
["a", "rest"] in both engines.

What was still actually broken, discovered while chasing #2037's own
CJS-require() repro (const { a, ...rest } = require('./mod')): the
Rust require() path reuses the already-fixed collect_object_pattern_names
helper directly, so it was already correct — but the TS/WASM require()
path uses a separate, never-fixed function, extractCjsRequireBinding,
whose object-pattern loop only recognized shorthand_property_identifier_pattern
and pair_pattern children. It had no rest_pattern/rest_element branch
at all, so rest was silently dropped from cjsRequireBindings (the
CJS-require import-artifact classification, #1661) — a genuine, currently
existing engine-parity gap.

Fix

  • src/extractors/javascript.ts: added a rest_pattern/rest_element
    branch to extractCjsRequireBinding, reusing the existing
    extractRestPatternIdentifier helper (the same one extractDynamicImportNames
    already uses) — mirrors Rust's collect_object_pattern_names behavior.
  • crates/codegraph-core/src/extractors/javascript.rs: no logic change
    needed (already correct) — added a regression test locking in the
    already-correct native behavior for the same repro, to prevent
    future regressions and keep the two engines' test coverage symmetric.
  • tests/parsers/javascript.test.ts: added #2037 coverage for the
    CJS-require rest-binding case (plain + renamed-pair mixes).

Repro confirmation

Both engines now agree for both of the issue's repros:

const [a, ...rest] = await import('./mod.js')   -> names: ["a", "rest"]   (already correct, both engines)
const { a, ...rest } = require('./mod')         -> names: ["a", "rest"]   (native already correct; WASM fixed here)

Verified via codegraph diff-impact --staged -T — change is scoped to
extractCjsRequireBinding (6 transitive callers, 1 file).

Test plan

  • npx vitest run tests/parsers/javascript.test.ts — 290 passed
  • npm test — 273 files, 4424 passed, 30 skipped, 2 todo, 0 failed
  • cargo test --lib (crates/codegraph-core) — 749 passed, 0 failed
  • npm run lint — clean
  • node scripts/parity-compare.mjs --langs javascript,typescript — PARITY OK, wasm vs native identical for both fixtures
  • npx vitest run tests/benchmarks/resolution/resolution-benchmark.test.ts — 206 passed

Follow-up

Filed #2268 for an out-of-scope finding discovered during investigation:
both engines' CJS-require import-artifact classification only handles
object-pattern destructures, never array-pattern ones (symmetric gap, not
a parity divergence).

Closes #2037

… (docs check acknowledged)

Impact: 1 functions changed, 6 affected
@greptile-apps

greptile-apps Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR fixes missing object-rest names in the TypeScript/WASM CJS require() binding classification and adds symmetric regression coverage for both extraction engines.

  • Handles rest_pattern and rest_element nodes using the existing identifier-scanning helper.
  • Tests plain and renamed-pair CJS destructuring in the TypeScript extractor.
  • Locks in the already-correct native Rust behavior with a matching regression test.

Confidence Score: 5/5

The PR appears safe to merge, with the focused extractor change matching the existing rest-binding behavior and native implementation.

The changed branch delegates to the established identifier-scanning helper, applies to both supported rest node variants, and is covered by focused WASM and native regression tests.

Important Files Changed

Filename Overview
src/extractors/javascript.ts Reuses the established rest-pattern helper to include object-rest identifiers in CJS require binding metadata.
tests/parsers/javascript.test.ts Adds focused regression cases for plain and renamed-pair object destructuring with a rest binding.
crates/codegraph-core/src/extractors/javascript.rs Adds native-engine regression coverage confirming existing parity for CJS object-rest extraction.

Reviews (1): Last reviewed commit: "fix(extractors): extract CJS require() r..." | Re-trigger Greptile

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Codegraph Impact Analysis

1 functions changed6 callers affected across 1 files

  • extractCjsRequireBinding in src/extractors/javascript.ts:1071 (6 transitive callers)

@carlos-alm
carlos-alm merged commit 79db1ad into main Aug 4, 2026
31 of 33 checks passed
@carlos-alm
carlos-alm deleted the fix/issue-2037-rest-pattern-child-index branch August 4, 2026 07:35
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 4, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JS/TS: rest-pattern identifier never extracted in dynamic-import/CJS-require name collection (wrong child index)

1 participant