Repository navigation
Conversation
{a:1, b:2, c:3}.c now folds to 3 under --minify-syntax, matching
esbuild. Previously only single-property objects ({c:3}.c) were folded.
The fold bails on any property that makes static resolution unsound:
spreads, computed keys, getters/setters/methods, and numeric keys. Every
dropped property's value must be side-effect free (checked with
expr_can_be_removed_if_unused), so {a: fn(), c: 2}.c is left alone.
A __proto__: null entry lets a miss fold to undefined; any other
__proto__ keeps the access intact since it may resolve via the
prototype.
The existing single-property fast path is kept so {f: sideEffect()}.f
still folds to sideEffect() without a side-effect check (the value is
what's returned, so nothing is dropped).
Fixes #3463
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…horthand __proto__
Tagged templates bind `this` to the receiver just like calls, so
folding the tag away changes observable behavior. Add p.template_tag
(same pattern as p.call_target/delete_target), set it in e_template
before visiting the tag, thread an is_template_tag bit through
IdentifierOpts to maybe_rewrite_property_access, and gate the EObject
fold on it. This also closes the pre-existing single-property case:
{f: fn}.f`x` now keeps the object literal.
Shorthand {__proto__} defines an own data property (Annex B.3.1 only
gives colon-form __proto__: the [[Prototype]] setter semantics), so
treat a WasShorthand __proto__ key as an ordinary own property instead
of the prototype setter. Colon-form __proto__: is skipped for key
matching and only contributes has_proto_null.
Tests tightened to use matching keys so the assign/template-tag guards
are actually load-bearing, and the runtime test now covers both edge
cases.
|
Updated 3:23 AM PT - Aug 1st, 2026
✅ @autofix-ci[bot], your commit aad4c6756acee1f144d7e9541b761bb123b1f4e0 passed in 🧪 To try this PR locally: bunx bun-pr 36599That installs a local version of the PR into your bun-36599 --bun |
There was a problem hiding this comment.
Beyond the inline finding, I re-checked the three fixes from 571480a: the template-tag guard is threaded through p.template_tag → IdentifierOpts.is_template_tag() and gates both the len==1 fast path and the general scan (tests cover .a and ["a"] tag forms plus the single-property case); shorthand __proto__ now falls through to normal key-match (runtime test covers {__proto__: null, __proto__}.__proto__ → 42); and the Unsafe assertions now use matching keys so they are load-bearing. Also checked that a non-null colon-form __proto__ correctly leaves has_proto_null = false so misses on {__proto__: obj, ...} do not fold to undefined.
Extended reasoning...
One new correctness finding (UTF-16 key vs UTF-8 name byte-widened comparison turning a false-negative match into void 0 via the new has_proto_null return) is posted inline; deferring on that. The prior-round fixes hold.
The file-local e_string_eql_bytes shim did a length-then-widened-byte
compare for UTF-16 keys, which only agrees with a UTF-8 name for pure
ASCII. With the new has_proto_null path, a false negative on a key like
{__proto__: null, "\u00e9": 1}.é folded to undefined instead of 1.
Route the UTF-16 branch through bun_core::strings::utf16_eql_string,
which decodes code points before comparing.
…transpiler cache version The ??/||/&&/?:/, folds in visit_binary.rs and e_if check is_call_target to keep a (0, obj.fn) wrapper when collapsing to a bare member access, but tagged-template tags bind this the same way. Compute is_template_tag at those sites from the new p.template_tag field and include it in the guard so (null ?? obj.fn)`x` keeps the wrapper instead of dropping it. Also bump RuntimeTranspilerCache EXPECTED_VERSION (24 -> 25) since the runtime transpiler runs with minify_syntax for bun-target files, and update the IdentifierOpts packed-layout comment for the new bit 5.
There was a problem hiding this comment.
No new issues found — all prior findings (template-tag guard, shorthand __proto__, UTF-16 key compare, the six has_value_for_this_in_call() sibling sites, cache version bump) are addressed. Deferring to a maintainer given this changes runtime transpiler output and touches parser fold semantics with Annex B.3.1 subtleties.
Checked: colon-form vs shorthand __proto__ handling in the general path; .__proto__ access on {__proto__: null} and on objects with no proto key (both correctly not over-folded); non-null __proto__: values leave has_proto_null false so misses don't fold; p.template_tag follows the same set-before-visit / pointer-identity pattern as call_target; utf16_eql_string handles both the false-negative and false-positive encoding directions the runtime test covers.
Extended reasoning...
Overview
Extends maybe_rewrite_property_access in src/js_parser/fold.rs to fold property access on multi-property object literals under --minify-syntax (previously only single-property). Adds p.template_tag parser state (p.rs, parser.rs) and threads an is_template_tag bit through IdentifierOpts, e_dot/e_index/e_if in visit_expr.rs, and four e_binary arms in visit_binary.rs. Fixes the UTF-16 branch of the file-local e_string_eql_bytes shim to use bun_core::strings::utf16_eql_string. Bumps RuntimeTranspilerCache version 24→25. Three new bundler tests plus runtime coverage.
Security risks
None. Pure minifier/parser optimization; no untrusted input handling, auth, crypto, or FFI surface changes.
Level of scrutiny
High. The JS parser is production-critical — a miscompile silently changes runtime behavior of user code. This change:
- Adds ~70 lines of new fold logic with Annex B.3.1
__proto__semantics (colon-form vs shorthand vs computed distinction) - Fixes three pre-existing miscompiles (template-tag
thisbinding, UTF-16 key false-match, six siblingthis-preservation sites) — each of which changes emitted code - Runtime transpiler enables
minify_syntaxfor bun-target files, so this affectsbun run, not justbun build - The cache version bump means the correctness of the fold matters for every re-transpiled file
Other factors
- Went through three prior review rounds; every finding I raised was fixed with a targeted commit and the threads are all resolved.
- Test coverage is solid: capture assertions for fold results, an
Unsafetest with load-bearing keys (post-fix), and a runtime test that would fail on any of the semantic mistakes flagged earlier (shorthand__proto__→ 42, tagged-templatethis→"OBJ", UTF-16 keys, all six??/||/&&/?:/comma tag cases). - I traced the fold through the non-obvious inputs (
{__proto__: {foo:1}, a:2}.foocorrectly not folded sincehas_proto_nullstays false;{__proto__: null}.__proto__→undefined;{__proto__}.__proto__with bound identifier → the identifier via the shorthand fall-through) and found nothing wrong. - The
p.template_tagfield follows the exact same set-before-visit / pointer-identity-compare / never-cleared pattern as the pre-existingcall_target/delete_target, so the staleness question is answered by precedent.
Given the scope (parser hot path, runtime-behavior fixes across six sibling sites, spec-subtle __proto__ handling), a maintainer should sign off even though I found nothing to flag on this revision.
|
Heads up: this PR's |
|
Closing this PR because it has been inactive for more than 90 days. |
|
The inactivity sweep closed this PR. No maintainer reviewed it. CI was green on aad4c67 (build 86710). Two corrections to the description. I edited it to match.
State of the branch:
|
Related to #3463. This PR does not change the output for the input in that issue (see the note below). It makes
{a:1, b:2, c:3}.cfold to3under--minify-syntax, as esbuild does. Before, only the single-property form ({c:3}.c) folded. The code had an explicitproperties.len() == 1guard.Repro
Fix
maybe_rewrite_property_accessinsrc/js_parser/fold.rsnow scans every property (esbuild-style): each one must bekind == Normal, have no computed/method/spread flag, carry anEStringkey, and have a value that passesexpr_can_be_removed_if_unused. The last matching key wins. A colon-form__proto__: nulllets a miss fold toundefined; a shorthand{__proto__}is treated as an ordinary own property per Annex B.3.1. The existing single-property fast path is kept so{f: sideEffect()}.fstill folds without a side-effect gate.The UTF-16 branch of the file-local
e_string_eql_bytesshim now routes throughbun_core::strings::utf16_eql_stringso a key like"\u00e9"compares correctly against a UTF-8.éname instead of producing a false miss.Cases that stay untouched (covered by the
Unsafetest): spreads, computed keys, getters/setters/methods, numeric keys, unbound identifier values, assignment targets, and tagged-template tags.Tagged-template tags
Threading the fold through multi-property objects exposed that Bun never tracked tagged-template tag position when rewriting property accesses (esbuild gates on
!isCallTarget && !isTemplateTag; Bun only had the first half). Addedp.template_tagalongsidep.call_target/p.delete_target, set ine_templatebefore visiting the tag, and anis_template_tagbit onIdentifierOpts. TheEObjectfold and the sixhas_value_for_this_in_call()sites ine_binary/e_if(comma,??,||,&&,?:both arms) now consult it, so e.g.(null ?? obj.fn)`x`keeps the(0, obj.fn)wrapper instead of collapsing toobj.fn`x`and changing the tag'sthis.RuntimeTranspilerCacheversion bumped to 25 since the runtime transpiler enablesminify_syntaxfor bun-target files.Note on the original issue's repro
#3463 uses
const obj = {a:1,b:1,c:1}; console.log(obj["c"])and expectsconsole.log(1). esbuild does not actually produce that (verified 0.17 through current): it emitsconst obj={a:1,b:1,c:1};console.log(obj.c);. Getting all the way toconsole.log(1)at module scope would require inlining single-useconstobject bindings, which neither tool does today. Only the direct{a:1,b:1,c:1}["c"]form reaches the fold. A binding does not.function f(){const obj={a:1,b:1,c:1};return obj["c"]}still printsreturn{a:1,b:1,c:1}.c, because single-use substitution runs after the property access is visited (measured on bfedf0a, the first commit of this PR. The later commits do not change the substitution). An earlier version of this description said that this form folds. That was wrong.Verification
3 new tests (fold results, unsafe cases that must not fold, runtime correctness covering
__proto__shorthand / UTF-16 keys / tagthis). Also ran the fullbundler_minify,bundler_edgecase,esbuild/dce,esbuild/default, andtranspilersuites: all pass.[review] gate passed · iteration 0 · 7 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 0
evidence per changed file