Skip to content

bind names in JS/TS destructuring patterns (silent slice under-approximation) - #2

Closed
samiujan wants to merge 1 commit into
mainfrom
upstream/fix-destructuring-defs
Closed

samiujan wants to merge 1 commit into
mainfrom
upstream/fix-destructuring-defs

Conversation

@samiujan

@samiujan samiujan commented Oct 14, 2026 •

Copy link
Copy Markdown
Owner

Fixes a silent soundness bug in rgctl-analysis def-use. Based on main (8f09655).

collect_pattern_defs did not recognise the JS/TS destructuring pattern node kinds, so a destructured name was never recorded as defined. rgctl slice therefore under-approximated silently — a well-formed slice with a plausible reduction_percent, missing the destructuring statement, with nothing indicating a dependence had been dropped.

export function pick(src: { a: number; b: number }) {
  const { a, b } = src;   // line 2 — binds a and b
  const total = a + b;    // line 3
  return total;
}

slice src/d.ts --line 3 --variable total --function pick
  before: { "lines": [3], "edges": [] }
  after:  { "lines": [2, 3], "edges": [...] }

Array destructuring failed the same way; a plain const base = 10; control was already correct, which isolates the gap to destructuring.

Cause: an unrecognised pattern reached the catch-all arm, which delegates to collect_def_use_node, whose own final fallback pushes children with is_def_target = false — so destructured names were walked as uses. shorthand_property_identifier_pattern was not in the shared-identifier arm either.

Fix: add object_pattern/array_pattern (recurse), pair_pattern (recurse into value only — key is a property name), object_assignment_pattern/assignment_pattern (recurse into left only — right is a default that is read), and shorthand_property_identifier_pattern (bind its own text). Widen is_binding_pattern, which guards the routing arm.

Not confined to slice: reaching definitions, the PDG, taint and wl_hash.rs all inherit def-use.

Verified: 6 new regression tests (shorthand, array, renaming, nested, rest, default) plus the full rgctl-analysis suite — 365 passed, 0 failed. fmt and clippy add no new findings. End-to-end confirmed on the fixture above.

Tracked upstream as sshaaf#113. This fork PR deliberately carries no closing keyword: GitHub
resolves those within a single repository, and this fork has no matching issue.

`collect_pattern_defs` did not recognise the destructuring pattern node kinds, so a
destructured name was never recorded as *defined*. The visible effect is that
`rgctl slice` silently under-approximated: it returned a well-formed slice with a
plausible `reduction_percent` that was missing the destructuring statement, and
nothing indicated a dependence had been dropped.

    export function pick(src: { a: number; b: number }) {
      const { a, b } = src;   // line 2 — binds a and b
      const total = a + b;    // line 3
      return total;
    }

    slice src/d.ts --line 3 --variable total --function pick
      before: { "lines": [3], "edges": [] }          # line 2 missing
      after:  { "lines": [2, 3], "edges": [...] }    # dependence present

Array destructuring failed the same way. A plain `const base = 10;` control was
already correct, which isolates the gap to destructuring rather than to def-use in
general.

Cause: an unrecognised pattern reached the catch-all arm, which delegates to
`collect_def_use_node`, whose own final fallback pushes children with
`is_def_target = false` — so the destructured names were walked as *uses*.
`shorthand_property_identifier_pattern` was not in the shared-identifier arm
either, so it was not picked up as anything at all.

Add `object_pattern` and `array_pattern` (recurse), `pair_pattern` (recurse into
`value` only — the `key` is a property name, and binding it would invent a variable
that does not exist in the source), `object_assignment_pattern` and
`assignment_pattern` (recurse into `left` only — `right` is a default expression
that is read, so descending into it would invert the dependence), and
`shorthand_property_identifier_pattern` (bind its own text; it is a leaf whose kind
is not `identifier`). Widen `is_binding_pattern` to match, since it guards the
routing arm.

This is not confined to `slice`: reaching definitions, the PDG, taint and the clone
fragment hasher (`wl_hash.rs` consumes `defined_vars`) all inherit def-use.

Regression tests cover shorthand, array, renaming, nested, rest and default
patterns — the last two asserting the things that must *not* be bound. Also
verified end to end: the three fixtures above now return `lines: [2, 3]`.

Not addressed: a default expression (`{ a = fallback }`) is read, and this
function has no way to record a use. Recording it needs a `used` set threaded
through, which is a larger change; leaving it unrecorded is better than recording
it as a definition.

Signed-off-by: Sami Jan <sami.jan@gmail.com>
@samiujan

Copy link
Copy Markdown
Owner Author

Closing without merging — redundant now that the change is proposed upstream as sshaaf#115 (issue sshaaf#113).

This PR only ever existed for visibility in the fork. Merging it would put a commit into main that upstream will carry under a different SHA once sshaaf#115 lands, which would stop the fork's main from fast-forwarding to upstream/main.

The branch upstream/fix-destructuring-defs stays — it is still the head of sshaaf#115. Once that merges, main gets the change by fast-forwarding to upstream/main.

@samiujan samiujan closed this Oct 14, 2026
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