Skip to content

js_printer: fold adjacent member reads into a pattern only when minifying a bundle - #47870

Open
robobun wants to merge 1 commit into
mainfrom
robobun/e4bb8ba8/fold-member-reads-only-when-bundling
Open

js_printer: fold adjacent member reads into a pattern only when minifying a bundle#47870
robobun wants to merge 1 commit into
mainfrom
robobun/e4bb8ba8/fold-member-reads-only-when-bundling

Conversation

@robobun

@robobun robobun commented Nov 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Related to #17153

Problem

  • At run time const a = o.x, b = o.y with a nullish o throws Cannot destructure property 'x' from null or undefined value on the line of o.y. Two statements and a for initializer do the same. toString() shows a pattern.
  • print_decls (src/js_printer/lib.rs:2196) folds such a run into { x: a, y: b } = o for every caller.

Fix

  • The fold now needs minify_syntax and bundling, like the other rewrites that only save bytes. Minified bundles keep their bytes (51 of 51 files).
  • The ahead-of-time compiler (--compile --aot) drops its copy of the rule. The transpiler cache version is 52.
  • Verified: test/js/bun/sourcemap/source-positions.test.ts (main fails 37 tests). Also the transpiler, bundler, REPL, dev-server, bun pm diff and AOT suites (Notes).
  • Self-reviewed: 21 concerns raised, 19 addressed, 2 are follow-ups.

Background

Downsides

  • Bun.Transpiler, bun build --no-bundle and bundles without --minify-syntax print another text (rxjs.umd.js: +0.43%). bun pm diff reports it as a change.
  • Files dense in such runs load with more instructions: rxjs.umd.js +0.14% cold, +0.18% warm (noise 0.02%).
  • Minified bundles and built-in modules keep the fold, with its message and line.
Notes

Question for a maintainer. Should a bundle built without --minify-syntax keep this fold? This PR says no: the fold is a syntax minification, and with it a default bun build --compile throws another TypeError than bun run of the same file. If such bundles must keep their bytes, the gate is bundling alone, and the dev server then needs its own term. That variant was built first: every bundle was byte for byte the same (45 of 45 files).

What each lane prints. Release builds of main (3bf1a06) and of this branch, same profile.

Lane main this PR
bun run, bun test, bun -e, REPL pattern as written
bun test --coverage, --inspect (they turn minify_syntax off) pattern for one statement and a for head as written
Bun.Transpiler, bun build --no-bundle, with and without minify pattern as written
Dev server (minify_syntax is off in development) pattern as written
bun build, bun build --compile without --minify-syntax pattern for one statement and a for head as written
bun build --minify or --minify-syntax, Bun's built-in modules pattern pattern, same bytes

Without minify_syntax the parser does not merge two statements. So in those lanes main folds only the one-statement list and the for head.

  • The repro one.mjs (a one-statement and a two-statement function, called with undefined): main reports Cannot destructure property 'first' from null or undefined value at 3:14 and 9:18. This PR reports undefined is not an object (evaluating 'props.first') at 2:17 and 8:17. As written (// @bun first line) the lines are the same, at column 22. Node reports 2:23 and 8:23.
  • A default bun build --compile of one.mjs reports 3:14 with the pattern message on main, and 2:17 with the member message here. With --minify it is unchanged.
  • A third member whose getter throws: main reports the frame of the reading function on the line of the second declarator, this PR on the line of the third.
  • toString() of the two-statement function still shows one statement, const first = props.first, second = props.second;. The parser merges adjacent statements at run time. That is not part of this PR.

Wrong values, not only wrong messages. A pattern reads its object once. Two programs that show it, both fixed here:

// reads.cjs: `exports` is a parameter of the CommonJS wrapper, and `arguments[0]` writes it.
const other = { y: 2 };
Object.defineProperty(exports, "x", { get: () => { arguments[0] = other; return 1; } });
exports.y = 1;
function f() { var a = exports.x, b = exports.y; return [a, b]; }
console.log(JSON.stringify(f())); // node and this PR: [1,2]. main: [1,1]
// A classic script: a top-level `var` is a property of the global object.
var N = { a: "a2", b: "b2" };
var o = { get a() { globalThis.o = N; return "a1"; }, b: "b1" };
var a = o.a, b = o.b; // a + b is "a1b2" as written, "a1b1" after `Bun.Transpiler` on main

The second one is why Bun.Transpiler with minify: { syntax: true } does not keep the fold. Those lanes never applied the other bundle-only rewrites either.

Who reported it. No issue reports this in a user's own file. #17913, #23556 and #24374 show the message of this fold in checkServerIdentity of the built-in node:tls, which release builds still fold. #27135 fixed that caller. The case for this PR is the repro and the precedent: #8659 ("Joining statements with commas will only happen when bundling, making runtime error stack traces more readable"), #22492 and #31723.

Printer Options literals.

  • bundling: true with the user's minify_syntax: LinkerContext.rs:2382, postProcessJSChunk.rs:199. The entry point tail at postProcessJSChunk.rs:1256 now says bundling: true too. It moves no byte: each local it prints has one declarator.
  • bundling: false: transpiler.rs:2386 and :2463 (runtime transpiler, Bun.Transpiler, --no-bundle), repl.rs, aot/services.rs (what the AOT compiler prints as bun run does), CodeCoverage.rs, pm_diff_normalize.rs.
  • SAME_TARGET_BECOMES_DESTRUCTURING was a constant true with this one reader. It is removed.

Cost, measured. User-mode instructions come from perf_event_open, the minimum of 5 interleaved runs. Noise is two series of one build.

  • Run-time text (from the transpiler cache entry): typescript.js 6.0.2 +582 bytes (+0.007%, 35 folded runs before), rxjs.umd.js +1,031 (+0.46%, 35 runs), yaml 1.10.2 browser build +2,301 (+1.18%, 69 runs).
  • Load: typescript.js -0.001% cold, +0.08% warm (noise 0.17%). rxjs.umd.js +0.139% and +0.179% (noise 0.02%). yaml +0.245% and +0.353% (noise 0.03%). yaml load and 20 parses: +0.40% and +0.19% (noise 0.33%).
  • Each call of a function with such a run is cheaper. function f(o){const a=o.x,b=o.y;return a+b}: 14 to 9 bytecode instructions, 472 to 435 machine instructions per call in the interpreter, 95 to 86 with the JIT. var: 10 to 7 bytecode instructions.
  • Minified bundles: 51 of 51 files are the same, .js and .map. That is 8 flag sets (--minify or --minify-syntax, with --target=bun, --format=cjs, --splitting and three kinds of source map) over a two-entry fixture, the repro, typescript.js and rxjs.umd.js.
  • Bundling typescript.js: -0.008%, with --minify -0.014% (noise 0.07%).
  • Bundles without --minify-syntax: typescript.js is the same, rxjs.umd.js 234,111 to 235,129 bytes.
  • Release binary (size): text 140,303,961 to 140,298,841 bytes. The five copies of print_decls: 15,058 to 13,790 bytes.
  • bun build --no-bundle --minify-syntax --target=bun typescript.js: 8,194,709 to 8,195,291 bytes.
  • Transpiler cache: an entry of version 51 is written again once, and not on the second run.
  • Not in the container: perf, valgrind, bloaty, strace.

AOT compiler (bun build --compile --aot). It predicts what plain Bun prints and reports, so it held a copy of the rule of the fold. That copy is gone (163 lines).

  • 7,184 parity programs of test/aot/parallel: 7,168 pass, 12 skip themselves, 4 fail. The same 4 fail on main.
  • test/aot/tools/same-output.ts, main against this branch: identical on the 289 benchmark programs.
  • cargo test -p bun_aot --test ir_tests: 2,268 of 2,270 files. The other 2 hit a nesting limit of the unoptimized test binary, on main too. Two IR tests lose the NotDestructurable test of a joined run.
  • test/bundler/compile-aot-size.test.ts skips itself on a local build. In CI (Linux x64), main against this branch: the compiler takes 23,663,481 to 23,663,546 bytes, a hello world maps 68,134,014 to 68,125,822 bytes, and with --trim 25,961,598 to 25,965,694 bytes (one page more). Its bound of 25,880,000 is exceeded on main too.

Tests.

  • The corpus gets declarations.ts: two statements, one statement, a for initializer and a third member whose getter throws. A probe can name another text for the minified bundle, where the fold stays.
  • transpiler.test.js held 36 expectations and 2 programs about the rules of the fold, through Bun.Transpiler. They would pass there with or without the rules now. The same shapes are itBundled cases in bundler_minify.test.ts, with with and the mapped arguments object in a CommonJS bundle.
  • bundler_bytecode_portable.test.ts: the 5 bundles without --minify that hold such declarators have new hashes and a smaller payload. The 3 minified ones are unchanged.
  • Run by hand, not in a test: --inspect and a Worker. On main both report the pattern message for the one-statement function, here the member message.

Left for later.

Other open pull requests. This is the state on 2026-11-03 at main 675ee5b. It comes from the head of each open PR and from test merges with this branch (10c7d9a).

no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/sourcemap/source-positions.test.ts, test/js/bun/repl/repl.test.ts, test/cli/install/bun-pm-diff.test.ts, test/bundler/bundler_bytecode_portable.test.ts

@robobun
js_printer: fold adjacent member reads into a pattern only when minif…
10c7d9a 
…ying a bundle

`print_decls` printed `a = o.x, b = o.y` as `{ x: a, y: b } = o` for every
caller. Where the text runs in place of a file, a nullish `o` then threw
"Cannot destructure property" from the line of the second declarator, and
`toString()` showed the pattern.

The fold is a syntax minification, so it now needs `minify_syntax` and
`bundling`, as the other size-only rewrites do. The runtime transpiler,
`Bun.Transpiler`, `bun build --no-bundle`, the REPL, the dev server and a
bundle that is not minified print the declarators as they are written. A
minified bundle prints the same bytes as before.

The AOT compiler no longer models the printed pattern, and the transpiler
cache version is 52. The tests of the rules of the fold are `itBundled`
cases now: `Bun.Transpiler` does not fold, so there they would pass with
or without the rules.
@github-actions github-actions Bot added the claude label Nov 3, 2026
@robobun

robobun commented Nov 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Status: ready for review. The diff is green in CI. The red tests of the build are red on main too.

How to reproduce on main (save as one.mjs, run bun one.mjs):

function load(props) {
  const first = props.first,
    second = props.second;
  return [first, second];
}
try { load(undefined); } catch (e) { console.log(e.message, "|", e.stack.split("\n")[1].trim()); }
  • main prints Cannot destructure property 'first' from null or undefined value with the frame at one.mjs:3:14.
  • This branch prints undefined is not an object (evaluating 'props.first') with the frame at one.mjs:2:17.

CI, build 137326 (finished):

  • Every test that this pull request adds or changes passes on every lane. That includes bundler_bytecode_portable.test.ts, whose snapshot has to be the same on every platform.
  • 28 test files are red. All 28 are also red on main at the base commit of this branch (build 137162), with the same failures. This diff touches none of them.
  • 26 of them are still red on main at its newest commit (build 137327): the module mock tests under test/aot, test/aot/engine/ir-1, one program of test/aot/parallel/module-loaders, and test/bundler/compile-aot-size.test.ts.
  • The other 2, test/aot/engine/native-5.test.ts and native-6.test.ts, pass on main since 675ee5b. A rebase picks that up.

Pull request: #47870

@robobun

robobun commented Nov 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author
Updated 10:05 AM PT - Nov 3rd, 2026

❌ @robobun, your commit 10c7d9a has 40 failures in Build #137326 (All Failures):

🧪   To try this PR locally:

bunx bun-pr 47870

That installs a local version of the PR into your bun-47870 executable, so you can run:

bun-47870 --bun

@robobun robobun mentioned this pull request Nov 3, 2026
Open
@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Walkthrough

The JavaScript printer now emits same-target destructuring only when syntax minification and bundling are enabled. AOT lowering no longer tracks joined declarators or emits their associated nullish checks. Tests cover output, runtime errors, IR, cache versioning, and source positions.

Changes

Member-read printing and lowering

Layer / File(s) Summary
Printer conditions and transform coverage
src/bun_core/feature_flags.rs, src/js_printer/lib.rs, src/bundler/linker_context/postProcessJSChunk.rs, src/jsc/RuntimeTranspilerCache.rs, test/bundler/bundler_minify.test.ts, test/bundler/transpiler/transpiler.test.js, test/aot/minifying-first-saves-no-code/known-today.ts		 The printer now limits declaration-run destructuring to syntax-minified bundled output and excludes using and await using. Tests cover stable and unstable targets, output modes, and runtime transpilation. The runtime transpiler cache version increases to 52.
AOT lowering without joined-declarator metadata
src/aot/lower/transpiled.rs, src/aot/lower/transpiled/first_recorded_position.rs, src/aot/lower/call/callee.rs, src/aot/lower/statement/declarations.rs, test/aot/ir/lower/programs/*, test/bake/dev/bundle.test.ts, test/js/bun/repl/repl.test.ts		 AOT lowering removes joined-declarator tracking, related position lookups, and nullish checks. Updated IR expectations and runtime tests cover direct member reads and their errors.
Source positions and output snapshots
test/js/bun/sourcemap/source-positions-corpus.ts, test/js/bun/sourcemap/source-positions.test.ts, test/bundler/bundler_bytecode_portable.test.ts, test/cli/install/bun-pm-diff.test.ts, test/aot/parallel/errors/neighboring-declarations-read-members.mjs		 Source-position tests cover member reads and minified patterns across execution lanes. Portable bundle snapshots have updated hashes and sizes. A fixture and comment describe separate member reads.

Suggested reviewers: jarred-sumner, alii, dylan-conway

Merge Risk: Minimal · up to 10c7d

The change preserves member reads outside syntax-minified bundles while retaining the optimization within them. No actionable merge-blocking risk remains identified; merge after normal checks pass.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title names the main change: print_decls now folds adjacent member reads into an object pattern only when minifying a bundle. The diff matches this: the js_printer gate now requires minify_syntax …
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description provides the purpose and scope of the PR in the Problem, Fix, Background, and Downsides sections. It also provides extensive verification details, including test suites, results, limit…
Full details: Title check

Explanation

The title names the main change: print_decls now folds adjacent member reads into an object pattern only when minifying a bundle. The diff matches this: the js_printer gate now requires minify_syntax and bundling, the SAME_TARGET_BECOMES_DESTRUCTURING flag is removed, and the tests and cache version were updated to match. The title is short and specific, and it does not use vague terms.

Full details: Description check

Explanation

The description provides the purpose and scope of the PR in the Problem, Fix, Background, and Downsides sections. It also provides extensive verification details, including test suites, results, limitations, and CI coverage. It does not use the template headings exactly, but it contains the required information.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between a2691bd and 10c7d9a.

Files selected for processing: 20
  • src/aot/lower/call/callee.rs
  • src/aot/lower/statement/declarations.rs
  • src/aot/lower/transpiled.rs
  • src/aot/lower/transpiled/first_recorded_position.rs
  • src/bun_core/feature_flags.rs
  • src/bundler/linker_context/postProcessJSChunk.rs
  • src/js_printer/lib.rs
  • src/jsc/RuntimeTranspilerCache.rs
  • test/aot/ir/lower/programs/host-classes/server-request-getters.ts
  • test/aot/ir/lower/programs/typed-objects/external-objects-loops-branches-handlers.ts
  • test/aot/minifying-first-saves-no-code/known-today.ts
  • test/aot/parallel/errors/neighboring-declarations-read-members.mjs
  • test/bake/dev/bundle.test.ts
  • test/bundler/bundler_bytecode_portable.test.ts
  • test/bundler/bundler_minify.test.ts
  • test/bundler/transpiler/transpiler.test.js
  • test/cli/install/bun-pm-diff.test.ts
  • test/js/bun/repl/repl.test.ts
  • test/js/bun/sourcemap/source-positions-corpus.ts
  • test/js/bun/sourcemap/source-positions.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

claude[bot]
claude Bot reviewed Nov 3, 2026
View reviewed changes

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@claude claude[bot] claude[bot] left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

claude

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@robobun