Repository navigation
bind names in JS/TS destructuring patterns (silent slice under-approximation) - #115
Merged
Merged
Conversation
`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
added a commit
to samiujan/rgctl
that referenced
this pull request
Oct 14, 2026
…monstrate
Two rows were out of date after the rebase onto main:
- `dashboard/dist` regen was marked **blocked**, "source fixes are not in the
embedded bundle until dist is rebuilt". That landed in 8f09655 as
sshaaf#110. The row now says done, and says why it mattered: `dist` is
compile-time embedded, so until it is rebuilt the source fixes are invisible in
a built binary while every test passes.
- Change 7 was blocked on two `rgctl slice` defects. Both are now fixed and under
review (issues sshaaf#113/sshaaf#114, PRs sshaaf#115/sshaaf#116), so the row records what actually
gates it.
Also adds open question 4, because the more interesting problem is a gap rather than
a stale value: the maintainer's parking comment on sshaaf#61 names the code browser as the
missing piece, and no change in this sequence is the code browser. Change 1 is the
shell, changes 2-6 are rooms, and none renders source. The prototype does not cover
for it either — the Code lens shows a count of source lines, not source, and Spine
renders an empty state. The one requirement singled out is the one this design
cannot demonstrate.
Recorded as an open question, not invented as a change number: folding source
rendering into change 1 versus giving it its own change ahead of the rooms is a
scoping decision, not a correction.
Signed-off-by: Sami Jan <sami.jan@gmail.com>
samiujan
added a commit
to samiujan/rgctl
that referenced
this pull request
Oct 14, 2026
…monstrate
Two rows were out of date after the rebase onto main:
- `dashboard/dist` regen was marked **blocked**, "source fixes are not in the
embedded bundle until dist is rebuilt". That landed in 8f09655 as
sshaaf#110. The row now says done, and says why it mattered: `dist` is
compile-time embedded, so until it is rebuilt the source fixes are invisible in
a built binary while every test passes.
- Change 7 was blocked on two `rgctl slice` defects. Both are now fixed and under
review (issues sshaaf#113/sshaaf#114, PRs sshaaf#115/sshaaf#116), so the row records what actually
gates it.
Also adds open question 4, because the more interesting problem is a gap rather than
a stale value: the maintainer's parking comment on sshaaf#61 names the code browser as the
missing piece, and no change in this sequence is the code browser. Change 1 is the
shell, changes 2-6 are rooms, and none renders source. The prototype does not cover
for it either — the Code lens shows a count of source lines, not source, and Spine
renders an empty state. The one requirement singled out is the one this design
cannot demonstrate.
Recorded as an open question, not invented as a change number: folding source
rendering into change 1 versus giving it its own change ahead of the rooms is a
scoping decision, not a correction.
Signed-off-by: Sami Jan <sami.jan@gmail.com>
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
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.
Fixes a silent soundness bug in
rgctl-analysisdef-use. Based onmain(8f09655).collect_pattern_defsdid not recognise the JS/TS destructuring pattern node kinds, so a destructured name was never recorded as defined.rgctl slicetherefore under-approximated silently — a well-formed slice with a plausiblereduction_percent, missing the destructuring statement, with nothing indicating a dependence had been dropped.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 withis_def_target = false— so destructured names were walked as uses.shorthand_property_identifier_patternwas not in the shared-identifier arm either.Fix: add
object_pattern/array_pattern(recurse),pair_pattern(recurse intovalueonly —keyis a property name),object_assignment_pattern/assignment_pattern(recurse intoleftonly —rightis a default that is read), andshorthand_property_identifier_pattern(bind its own text). Widenis_binding_pattern, which guards the routing arm.Not confined to
slice: reaching definitions, the PDG, taint andwl_hash.rsall inherit def-use.Verified: 6 new regression tests (shorthand, array, renaming, nested, rest, default) plus the full
rgctl-analysissuite — 365 passed, 0 failed. fmt and clippy add no new findings. End-to-end confirmed on the fixture above.Fixes #113. (Opening on
sshaaf/rgctl, where#113resolves to the reported issue, so the keywordlinks and auto-closes it. This branch is also staged in the fork as samiujan#2, where the same
keyword would point at a fork-local issue that does not exist.)