[robobun]

Fixes #9613.

Repro

// m.ts
export function abc() { return { a: "a", b: "b", c: "c" }; }
// entry.ts
import { abc } from "./m.ts" with { type: "macro" };
const { b, a, c } = abc();
console.log(a, b, c);

$ bun build entry.ts --target bun
var { b, a, c } = {
  b: "b",
  c: "c"          // a is gone
};
$ bun run entry.ts
undefined b c

Cause

visit_binding_and_expr_for_macro iterates the binding pattern's keys, looks each up in the macro-returned object with as_property, and compacts the object's property array to only the referenced entries. The compaction was an in-place props[end] = props[query.i] that ran inside the same loop, so when binding key order differed from the object's property order the copy into props[end] overwrote an entry a later binding key had not yet looked up. The subsequent as_property for that key then failed and the emitted initializer dropped the property.

Fix

Swap props[end] and props[query.i] instead of copying, so the displaced entry stays in the array (in the not-yet-placed tail) and remains reachable for later lookups. Skip the swap when query.i < end, which only occurs when the binding repeats a key already placed; the property is already in the kept prefix.

This matches the existing compaction pattern elsewhere in the visitor (scan_side_effects.rs, visit/mod.rs decl compaction) and removes an unsafe block.

Tests

test/bundler/transpiler/macro-destructure.test.ts covers every permutation of a three-key binding, a nested/aliased destructure where the old behaviour threw TypeError: Cannot destructure ... from undefined, and a bun build check that the subset dead-code-elimination still keeps only the referenced keys. The first two fail on the released bun and all three pass with this change.

The tests are in their own file rather than macro-test.test.ts because that file currently aborts the debug build (!m_topGCOwnedDataScope in Heap::clearConcurrentRetainedDataIfPossible, tracked separately).

Separate from #7196 / #12067 (see #35548), which are about whether a macro result may enter const_values at all; this is the property-matching bookkeeping once it does.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 24 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 87822118-7b0c-4fb1-9037-60d0c2576c87

📥 Commits

Reviewing files that changed from the base of the PR and between df6c7ee and 4c842a0.

📒 Files selected for processing (2)

• src/js_parser/visit/mod.rs
• test/bundler/transpiler/macro-destructure.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Duplicate of #35574, which was opened first for the same bug with a two-pass bitmap approach. Closing this in favour of that PR; left a note there about the swap-based alternative and the macro-test.test.ts debug-build abort that affects test placement. The branch stays up for reference.

[github-actions]

This PR may be a duplicate of:

1. js_parser: keep all macro-object properties when the destructuring pattern reorders keys #35574 - Also fixes Destructuring assignment from macro function fails #9613 by changing macro-object property compaction in visit_binding_and_expr_for_macro (uses a two-pass bitmap approach vs. this PR's swap-based fix)

🤖 Generated with Claude Code

[claude]

LGTM — swap-instead-of-copy is the right fix and drops an unsafe block.

What was reviewed:

• Traced all six 3-key permutations and the {d, b} subset case through the new swap loop; each produces the correct kept prefix.
• Verified as_property is a linear first-match scan, so swapped-to-tail entries stay reachable; the query.i < end guard only fires on repeated binding keys and correctly avoids pulling an unmatched property into the prefix.
• Checked the tests follow harness conventions (tempDir, bunEnv, concurrent pipe drain, test.concurrent) and that the debug-only [macro] call stdout line is handled.

Extended reasoning...

Overview

One-line logic fix in visit_binding_and_expr_for_macro (src/js_parser/visit/mod.rs): the in-place compaction of a macro-returned object's property array is changed from a raw-pointer copy to a safe slice_mut().swap(), and end is only advanced when query.i >= end. A new test file exercises every 3-key permutation, nested/aliased destructuring, and a bun build subset-elimination check.

Correctness

I traced the loop by hand. as_property (src/ast/e.rs:1302) is a linear scan returning the first key match with its index. With copy, props[end] was overwritten before later binding keys could look it up; with swap the displaced entry lands at query.i (in the not-yet-placed tail) and remains findable. The query.i < end case can only occur when the current binding key matches a property already placed — i.e. a repeated key like {a: x, a: y} — and swapping there would move an unmatched tail entry into the kept prefix, so skipping is required. The recursion / const_values.put uses query.expr captured before the swap, so it reads the correct value. Both swap indices are in-bounds (end ≤ matches-so-far ≤ len; query.i < len from as_property), so the removed unsafe is genuinely unnecessary.

Security risks

None. Purely build-time AST bookkeeping on macro-produced object literals; no untrusted-length arithmetic, no allocator changes.

Level of scrutiny

Low-to-medium. ~10 lines of logic in a narrow macro-only path (was_originally_macro gate), matching the existing swap-compact idiom used for decl compaction later in the same file. The fix is mechanically simple and the PR description's mechanism explanation checks out against as_property's implementation.

Other factors

Tests are thorough for the bug class (all permutations, nested, aliased, subset build output + runtime), use test.concurrent, drain pipes concurrently, and account for the debug build's [macro] call stdout via lastLine. The build-output regex is whitespace-tolerant across newlines. robobun confirmed the tests fail on release bun and pass on this branch. No prior reviewer comments to address; no CODEOWNERS on src/js_parser/.