Repository navigation
Conversation
…ttern reorders keys The macro-destructuring visitor filtered the inlined object literal down to the properties referenced by the binding pattern by iterating the pattern and writing properties[end] = properties[query.i] in place. When the pattern's key order differed from the object's, an early write could overwrite an entry that a later as_property() lookup still needed to find, so that key was dropped from the emitted literal and the binding became undefined. Split the loop into two passes: resolve every bound key against the unmodified property list (recursing / recording const values as before) and mark which source indices are referenced, then compact forward in source order so end <= i always holds and no entry is read after being overwritten. Fixes #9613.
WalkthroughChangesMacro destructuring
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/bundler/transpiler/macro-test.test.ts`:
- Around line 173-177: The subprocess assertions at
test/bundler/transpiler/macro-test.test.ts lines 173-177 and 185-189 should
compare the complete trimmed stdout to JSON.stringify(expected), while retaining
stderr and exitCode in the same combined result assertion; remove final-line
parsing so both transpiler and built-output tests enforce exact child output.
- Around line 149-162: Add a destructuring test case in the macro test matrix
that binds the same source key twice under different local names, using a
pattern equivalent to `{ a, a: a2 }` and asserting both values are emitted.
Append the corresponding `"a,a"` result to the `expected` array.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2e9976bf-4c29-46ea-81fe-73bfad56aff6
📒 Files selected for processing (2)
src/js_parser/visit/mod.rstest/bundler/transpiler/macro-test.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I checked: query.i is bounded by object.properties.len() (it comes from enumerating that same slice in as_property), so used[query.i as usize] cannot index out of bounds; the copy_nonoverlapping is sound because it's gated on i != end and the forward pass guarantees end <= i. On CodeRabbit's "assert full stdout" suggestion — the .split("\n").pop() matches the neighboring sparse-key test in this file, which documents that debug builds print [macro] call <name> to stdout before script output, so asserting the full stdout would fail on debug CI.
Extended reasoning...
The two-pass mark-then-compact fix is correct and the SAFETY invariants hold; the only finding is a comment-style nit that overlaps comment-cop's existing flag. Deferring rather than approving because this is unsafe pointer arithmetic in the parser hot path and the comment-cop / style feedback hasn't been addressed yet. The note above records what was verified so a human reviewer doesn't re-derive it.
There was a problem hiding this comment.
LGTM — the comment/test-assertion feedback from the earlier pass is addressed in f1bc86c, and the remaining comment-cop firing on the 2-line invariant comment is a false positive.
What was reviewed:
- Mark-then-compact:
usedis sized toobject.properties.len(),query.iis bounded byas_property, and the compaction loop maintainsend <= iwith thei != endguard beforecopy_nonoverlapping. - Repeated-key binding (
{ a, a: a2 }) now marks one source slot and emits one property — correct destructuring semantics; output order shifts to source order, which is immaterial for keyed destructuring. - Tests: all six permutations, subset, alias, duplicate key, missing key, nested pattern; both
bun runandbun buildpaths; exact-stdout assertion with the debug[macro]prefix filtered.
Extended reasoning...
Overview
Two files: src/js_parser/visit/mod.rs (the visit_binding_and_expr_for_macro object arm, ~20 lines net) and test/bundler/transpiler/macro-test.test.ts (a new describe block with two concurrent subprocess tests). The fix replaces an in-place compact-during-lookup with a two-pass mark-then-compact so as_property always searches the unmodified property list.
Security risks
None. This is transpiler-side AST rewriting of a compile-time macro result; no untrusted-length arithmetic, no new external input surface. The unsafe block moves G::Property (no Drop) within an arena slice under a proven end <= i invariant, matching the pattern already used elsewhere in this file.
Level of scrutiny
Moderate — it touches parser/visitor code with an unsafe block, but the change is mechanical and the invariant is trivially provable. I verified as_property returns i as an enumeration index into properties.slice() (src/ast/e.rs:1302), so used[query.i as usize] cannot go out of bounds, and used.len() == object.properties.len() bounds the compaction loop. The switch from core::ptr::copy to copy_nonoverlapping is justified by the i != end guard.
Other factors
All prior review feedback is addressed: my earlier note about bug-history comments (mod.rs is now a 2-line invariant comment; the test has just the issue URL), and CodeRabbit's two notes (repeated-key case added; exact combined {stdout, stderr, exitCode} assertion instead of last-line JSON parsing). The comment-cop bot fired again on the trimmed 2-line comment, but that action flags any multi-line comment in claude PRs — the comment now states an invariant, not a workaround justification, which is exactly what REVIEW.md asks for. No CODEOWNERS on src/js_parser/. Test coverage is thorough for the variant matrix (permutations, subset, alias, duplicate key, missing key, nested) across both entry points.
|
I opened #35619 for the same bug before spotting this one; closing mine in favour of this. Two observations from that work, in case they are useful here:
|
There was a problem hiding this comment.
LGTM — the mark-then-compact split correctly fixes the in-place clobbering, and all prior review feedback has been addressed.
What was reviewed:
- Verified
query.ifromas_property()indexes intoobject.properties, soused[query.i]is in bounds and the SAFETY invariantend <= iholds forcopy_nonoverlapping. - Checked the sibling
BArrayarm — it truncates then zips in order, so it never had this bug. - Confirmed the second comment-cop message is stale (fired on f1bc86c; 3881d92 removed the comment it flagged).
Extended reasoning...
Overview
This PR fixes #9613: destructuring a macro-returned object with keys in a different order than the object literal dropped some bindings. The change is confined to visit_binding_and_expr_for_macro in src/js_parser/visit/mod.rs (~15 net lines) plus a new regression test block in test/bundler/transpiler/macro-test.test.ts.
Correctness
The root cause is well-explained: the old single-pass loop wrote properties[end] = properties[query.i] while still calling as_property() on the same slice, so an early write could overwrite an entry a later lookup needed. The fix splits this into (1) resolve every bound key against the unmodified list and mark used[query.i], then (2) compact forward in source order. I verified via src/ast/e.rs:1302 that query.i is the enumeration index into object.properties, so the SmallVec<[bool; 8]> sized to properties.len() cannot be indexed out of bounds. The compaction loop maintains end <= i, so the i != end guard makes copy_nonoverlapping sound; G::Property has no Drop (same assumption the old code made). The const_values recording and nested-object recursion still happen in pass 1 exactly as before. The BArray arm is unchanged and doesn't share the bug — it truncates to min(len) and iterates in lockstep, never searching.
Security risks
None. This is a compile-time AST rewrite of an already-materialized macro result; no user-controlled sizes drive allocation, and the only index (query.i) is bounded by construction.
Level of scrutiny
Moderate — it touches an unsafe block in the parser, but the unsafe was already there and the new version has a strictly stronger invariant (forward-only compaction vs. arbitrary-index copy). The change is localized to a niche macro-inlining optimization with no effect on non-macro code paths.
Other factors
All earlier review feedback is resolved: my prior note about bug-history comments (both trimmed — test now has only the issue URL; the mod.rs explanatory comment was dropped in 3881d92, leaving just the SAFETY line), CodeRabbit's request for a repeated-key case ({ a, a: a2 } added) and exact-stdout assertion (now compares the full {stdout, stderr, exitCode} object with the [macro] debug lines filtered). The second comment-cop message on the thread predates 3881d92 and no longer applies. Tests cover all six key-order permutations, a reordered subset, aliased/repeated/missing keys, and a nested pattern under both bun run and bun build. smallvec is already a dependency of this crate. No CODEOWNERS entry gates src/js_parser.
Matches the existing G::PropertyList compaction in scan_side_effects.rs and the decl compaction earlier in this file; the second pass reads only the used bitmap, never the properties themselves, so no raw-pointer provenance is needed.
macro-test.test.ts evaluates top-level macro imports at module load, which can trip a pre-existing !m_topGCOwnedDataScope debug assertion and abort before any test runs. The destructuring tests are self-contained subprocess tests, so isolating them keeps local debug verification deterministic.
There was a problem hiding this comment.
LGTM — all prior feedback is addressed (tests moved to macro-destructure.test.ts, explanatory comment dropped, safe swap replaces the unsafe compaction, repeated-key and exact-stdout coverage added).
What was reviewed:
- Two-pass mark-then-compact: first pass only reads
object.propertiesviaas_propertyand writes to a separateusedbitmap, so lookups can't be clobbered; second pass maintainsend <= isoswap(end, i)is a stable in-place partition. { a, a: a2 }setsused[query.i]idempotently — the property is kept once and both bindings resolve.- Test file is hermetic, drains pipes concurrently, asserts the full
{stdout, stderr, exitCode}, and the[macro]stdout filter matches the real debug-buildprettyln!insrc/js_parser_jsc/Macro.rs.
Extended reasoning...
Overview
Fixes #9613: destructuring a macro-returned object with keys in a different order than the object literal dropped properties. The change is ~15 net lines in src/js_parser/visit/mod.rs::visit_binding_and_expr_for_macro plus a new 61-line test file test/bundler/transpiler/macro-destructure.test.ts.
The old code compacted object.properties in place while still calling as_property() against it, so an early write could overwrite an entry a later lookup needed. The fix splits into (1) a lookup pass that marks referenced source indices in a SmallVec<[bool; 8]> without touching the property array, and (2) a forward compaction pass using safe slice_mut().swap(end, i) and truncate(end). The nested-object recursion and const_values recording are preserved verbatim in the first pass.
Security risks
None. This is AST-shaping logic that only runs when a macro's inlined object is destructured; no untrusted-length arithmetic, no allocation sized from external data, and the change removes an unsafe block rather than adding one.
Level of scrutiny
Moderate. js_parser is core, but this function is narrow (macro-object destructuring only) and the new logic is a textbook mark-then-compact with a trivially-verifiable end <= i invariant. I traced as_property (src/ast/e.rs:1302) to confirm it linearly scans self.properties.slice() — since pass 1 never mutates that slice, every lookup sees the original list. Compaction preserves source order, which is fine for destructuring semantics.
Other factors
This is my third look at the PR. Every piece of prior feedback — mine (comment verbosity, test-file placement), CodeRabbit's (repeated-key coverage, exact-stdout assertion), comment-cop's, and robobun's optional note about the unsafe block — has been addressed in commits f1bc86c, 3881d92, 3eb0e44, and 4dbed42. robobun's alternative single-pass-swap design was explicitly offered as "either approach is correct"; the two-pass chosen here is equally sound and the SmallVec<[bool; 8]> stays on the stack for typical objects. The new test file has no top-level macro imports, so it runs under bun bd test for local fail-before/pass-after verification, covers all six key permutations plus subset/alias/repeated/missing/nested, uses test.concurrent, and asserts exact combined subprocess output. No CODEOWNERS entry covers src/js_parser/.
|
The diff is green: |
|
Closing: this was fixed on main by #37669, which replaced the aliasing ptr::copy compaction in visit_binding_and_expr_for_macro with a swap-based one, so a destructuring pattern whose key order differs from the macro result no longer drops properties. The test file this PR adds (test/bundler/transpiler/macro-destructure.test.ts, covering every permutation plus the alias, repeated-key, missing-key and nested cases) passes unmodified against a debug build of main at 04148c8, twice in a row, and the repro from #9613 now prints a b c. |
Fixes #9613.
Repro
Cause
visit_binding_and_expr_for_macrofilters the inlined object literal down to the properties the binding pattern actually uses. It did this by iterating the pattern keys, looking each one up in the object withas_property(), and writingproperties[end] = properties[query.i]into the same array it was searching. When the pattern's key order differed from the object's, an early write overwrote an entry that a later lookup still needed, so that key was dropped from the emitted literal and the binding becameundefined.For
{ c, a, b }against{ a, b, c }:cmatches index 2, writeprops[0] = props[2]; array is now[c, b, c]asearches[c, b, c]and is not foundbmatches index 1, writeprops[1] = props[1]{ c, b }Fix
Split into two passes. The first pass resolves every bound key against the unmodified property list (recursing into nested objects and recording
const_valuesexactly as before) and marks which source indices are referenced in aSmallVec<[bool; 8]>. The second pass compacts forward in source order withslice::swap(end, i), matching the existing compaction pattern inscan_side_effects.rsand elsewhere in this file; nounsafeis needed.Verification
test/bundler/transpiler/macro-destructure.test.tsruns the repro under bothbun runandbun build, covering all six permutations of{a,b,c}, a reordered subset, an aliased key, the same key bound twice ({ a, a: a2 }), a missing key, and a nested{ outer: { z, x } }pattern. Both tests fail on the released bun and pass with this change.The tests live in their own file because
macro-test.test.tsevaluates top-level macro imports at module load and can intermittently trip a pre-existing!m_topGCOwnedDataScopedebug assertion before any test runs.[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file