fix: classify destructured bindings as constant, not function#1902
Open
carlos-alm wants to merge 1 commit into
Open
fix: classify destructured bindings as constant, not function#1902carlos-alm wants to merge 1 commit into
carlos-alm wants to merge 1 commit into
Conversation
…heck acknowledged)
Object-pattern destructuring targets (const { a, b } = x; and renamed
const { a: b } = x;) were hardcoded to kind: 'function' in both the TS/WASM
extractor (extractDestructuredBindings, shared by the walk and query paths)
and its native Rust mirror (extract_destructured_bindings), regardless of
what the destructured value actually held. Non-function bindings (e.g.
const { dbPath } = workerData) had no call-graph edges pointing at them by
name, so the dead-code classifier risked flagging them dead-unresolved even
when read repeatedly elsewhere in the file.
Both engines now emit kind: 'constant', matching the existing convention for
plain `const x = <literal>` bindings and array-pattern destructuring (which
was already correct). Every call site of these functions is const-gated, so
'constant' is unconditionally correct — no let/var case exists here. Call-
target resolution is kind-agnostic and constant is already included in the
top-level-binding caller-attribution fallback, so destructured
callback-style bindings (e.g. `const { handler } = router; handler(req)`)
still resolve correctly; verified via the resolution-benchmark suite showing
identical precision/recall across all 40 fixture languages before and after.
Array-pattern destructuring was checked and does not have this bug (already
kind: 'constant' in both engines' TS path); a separate, pre-existing native
vs WASM parity gap for array patterns was found and filed as #1901.
Docs check acknowledged: kind-classification bug fix only, no new
languages/features/architecture changes — README/ROADMAP tables unaffected.
Fixes #1773
Impact: 2 functions changed, 7 affected
Contributor
Greptile SummaryFixes a misclassification in both the TypeScript/WASM and Rust/native JavaScript extractors where
Confidence Score: 4/5Safe to merge. The two-character change per extractor is narrowly scoped to object-pattern destructuring inside const declarations, call-target resolution is kind-agnostic so no regressions are expected, and the fix is backed by unit, parser, and integration tests across both engines. The core change is straightforward and correct in both engines. The one gap worth noting is that the native integration describe block omits the call-edge preservation test that the WASM block includes, leaving a small blind spot for future native-engine regressions on that specific behaviour. tests/integration/issue-1773-destructured-binding-kind.test.ts — the native describe block covers only the kind assertion, not the call-edge preservation check that the WASM block verifies. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["AST: object_pattern node\n(inside a const declaration)"] --> B{child node type}
B --> C["shorthand_property_identifier_pattern\nor shorthand_property_identifier\ne.g. const { handleToken } = …"]
B --> D["pair_pattern or pair\ne.g. const { values: flags } = …"]
B --> E["other (ignored)"]
C --> F["emit Definition\nname = child.text\nkind = 'constant' ✅\n(was 'function' ❌)"]
D --> G["value = child.childForFieldName('value')\n(the local alias, e.g. flags)"]
G --> H["emit Definition\nname = value.text\nkind = 'constant' ✅\n(was 'function' ❌)"]
F --> I["DB: nodes table"]
H --> I
I --> J{role classification}
J --> K["kind='constant'\n→ dead-leaf (isolated)\nor leaf (active siblings)"]
J --> L["kind='function' (old)\n→ dead-unresolved\n(misleading)"]
%%{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["AST: object_pattern node\n(inside a const declaration)"] --> B{child node type}
B --> C["shorthand_property_identifier_pattern\nor shorthand_property_identifier\ne.g. const { handleToken } = …"]
B --> D["pair_pattern or pair\ne.g. const { values: flags } = …"]
B --> E["other (ignored)"]
C --> F["emit Definition\nname = child.text\nkind = 'constant' ✅\n(was 'function' ❌)"]
D --> G["value = child.childForFieldName('value')\n(the local alias, e.g. flags)"]
G --> H["emit Definition\nname = value.text\nkind = 'constant' ✅\n(was 'function' ❌)"]
F --> I["DB: nodes table"]
H --> I
I --> J{role classification}
J --> K["kind='constant'\n→ dead-leaf (isolated)\nor leaf (active siblings)"]
J --> L["kind='function' (old)\n→ dead-unresolved\n(misleading)"]
Reviews (1): Last reviewed commit: "fix: classify destructured bindings as c..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+142
to
+163
| describe.skipIf(!isNativeAvailable())( | ||
| 'destructured binding kind classification (#1773) — native', | ||
| () => { | ||
| let tmpDir: string; | ||
|
|
||
| beforeAll(async () => { | ||
| tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-1773-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('classifies renamed and non-renamed destructured bindings as kind constant, not dead-unresolved', () => { | ||
| expectFixedKindAndRole(path.join(tmpDir, '.codegraph', 'graph.db')); | ||
| }); | ||
| }, | ||
| ); |
Contributor
There was a problem hiding this comment.
The WASM describe block has two assertions — kind correctness (expectFixedKindAndRole) and a flags → parseArgs call-edge preservation check — but the native block only covers the first. If a future change to the native engine's call-attribution for top-level const bindings silently drops that edge, this test suite won't catch it. Parity with the WASM block's second it(...) would close the gap.
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!
Contributor
Codegraph Impact Analysis2 functions changed → 7 callers affected across 1 files
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
extractDestructuredBindings/extract_destructured_bindings) unconditionally assignedkind: 'function'to every binding target, regardless of the destructured value's actual nature. Since these bindings have no call-graph edges pointing at them by name under that wrong kind,roles --role deadflagged themdead-unresolvedeven when read repeatedly.kind: 'constant'(verified againstshared/kinds.tsas the already-used correct kind for JS/TSconstbindings — all three extraction call sites are unconditionally gated onconst) in both engines. Confirmed this doesn't break call-target resolution:'constant'is already inNODE_KIND_FILTER_SQL/TOP_LEVEL_BINDING_KINDS, so theflags -> parseArgscall-attribution edge survives via the correct tier instead of the old bogus "function scope" tier.kind: 'constant'; native emits no definition at all for array-patternconstdeclarations — a different bug shape (missing node, not wrong kind) and a genuine engine-parity gap. Filed separately as Engine parity: native JS extractor emits no definition for const array-pattern destructuring #1901, out of scope for this fix.Closes #1773
Test plan
npm test— full suite green (3564 passed, 0 failed)npm run lint— cleancargo test --package codegraph-core— 466 passedflagsnowkind: constant, roleleaf(not dead);dbPath/name/forcenowkind: constant, roledead-leaf(not the misleadingdead-unresolved) — verified this matches how any isolated referenced-but-unproven-live constant is classified via a control testStacked on #1900 (base branch
fix/issue-1772-dedupe-callref-tests-rendering) — only the extractor/test diff is this issue's change.