Keep required parens around a new expression callee#31813
Conversation
WalkthroughThis PR fixes incorrect parenthesization in ChangesNew Expression Callee Parenthesization
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
CI status: the changed code is green on every lane that exercises it — all The red lanes are unrelated CI flake, not this diff:
Not pushing a |
|
CI status (build #60340, commit 3814510): the changed code is green on every lane that exercises it — all The red lanes are unrelated CI flake, confirmed by checking them on other open PRs right now — they fail intermittently regardless of the diff:
A JS-printer change cannot cause any of these. Not pushing a |
There was a problem hiding this comment.
All prior feedback has been addressed and the latest pass found nothing new — but this is ~80 lines of subtle precedence/flag-propagation logic in the JS printer (correctness-critical for all transpiled output), so it's worth a human sign-off.
Extended reasoning...
Overview
This PR fixes #31812: the JS printer was dropping required parentheses around new expression callees that are member/index/tagged-template/optional-chain expressions containing a call or optional link, causing transpiled output to reparse with the wrong semantics (or as a SyntaxError). The fix adds a helper new_callee_needs_parens (~50 lines) that walks the callee chain, and integrates it into the EDot/EIndex/ETemplate arms of print_expr in src/js_printer/lib.rs. It also fixes a pre-existing dead-code bug in the optional-chain template-tag check (expr.data → tag.is_optional_chain()). Test coverage is thorough: 24 expectPrinted_ assertions covering required-wrap, no-over-wrap, and isolated-optional-chain cases, plus 4 end-to-end bun -e runtime tests.
Security risks
None. This is pure AST-printing logic with no I/O, auth, network, or untrusted-input handling beyond what the printer already does.
Level of scrutiny
High. The JS printer determines the semantics of every program Bun transpiles — a regression here silently changes user code behavior in production. The change is not mechanical: it reasons about the interaction of three independent wrapping mechanisms (new_callee_needs_parens, HasNonOptionalChainParent, and the template-tag is_optional_chain wrap), Level::New vs Level::Postfix precedence, and ForbidCall flag propagation. Across three prior review rounds I flagged five edge cases (over-wrapping on isolated optional chains in EDot/EIndex, then ETemplate, then optional ECall; a pre-existing dead is_optional_chain check; and missing EImport/ERequireString handling) — all were fixed, but the iteration count itself confirms the problem space is subtle enough that a maintainer who knows the printer should give it a look.
Other factors
All six of my prior inline comments are resolved (commits 351b6f4, 3814510, c57542b), and the bug-hunting pass on c57542b found nothing new. Test coverage for the new logic is strong and includes both exact-output assertions and semantic round-trip tests. CI on the changed code is green per the author's analysis (red lanes are fleet-wide flake in unrelated bunx.test.ts and the baseline-verify tool). No CODEOWNERS entry covers src/js_printer/. The remaining reason to defer is purely the nature of the code path — printer parenthesization correctness — rather than any specific concern with the current diff.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Can you think of a faster way to do this?
|
CI status (build #60352, commit cbf7897): the The remaining red lanes are unrelated CI flake, not this diff:
No transpiler/printer/bundler test fails. The changed code builds on every platform and the transpiler + esbuild suites pass locally (incl. the debian-13 ASAN lane in CI, which runs the new tests). Not pushing a |
|
Independently reproduced #31812 and landed on the same three fixes (both new-callee cases plus the optional-chain template tag check reading the template instead of the tag). Alternative smaller-diff take for reference, propagating ForbidCall instead of walking the chain: https://github.com/oven-sh/bun/tree/farm/2ce2d2ab/new-callee-parens. Outputs verified against node for 20 cases under both plain and minify. This PR's coverage is a superset, nothing further needed. |
The JS printer dropped the parentheses required around the callee of a `new` expression when the callee was a tagged template whose tag contains a call, or an optional chain. This changed the meaning of the transpiled program: new (foo()`bar`)() -> new foo()`bar` // reparses as (new foo())`bar` new (baz()?.qux)() -> new baz()?.qux // new in optional chain: SyntaxError Only ECall consulted `Level::New` / `ForbidCall` to wrap itself as a `new` callee; EDot, EIndex, and tagged ETemplate forwarded ForbidCall to their target instead, so the inner call wrapped (parens on the wrong node) or, for templates, the flag was dropped entirely. Wrap the whole member/index/tagged-template chain in parens when it is a `new` callee and contains a call or optional chain that `new` would otherwise capture, and stop forwarding ForbidCall once wrapped so the inner call isn't parenthesized on its own. Plain member chains (`new (a.b.c)()`) still drop the redundant parens.
Two follow-ups in the same printer code path: - `new_callee_needs_parens` no longer forces the outer `new`-callee parens for an optional chain that is already isolated below a non-optional access. `new ((a?.b).c)()` is printed as `new (a?.b).c` (the existing HasNonOptionalChainParent handling already wraps the `(a?.b)`), not the redundant `new ((a?.b).c)`. A call is still always wrapped. - The tagged-template printer's optional-chain check matched on the template node instead of the tag, so it was dead code: `(a?.b)`tpl`` printed as the invalid `a?.b`tpl`` (an optional chain may not directly tag a template literal). Check `tag.is_optional_chain()` so the tag is parenthesized. Tests extended for both, and the spawning test.each is now concurrent.
…emplate tag `new_callee_needs_parens` now also marks the chain as having crossed a non-optional boundary when descending through a tagged template. The tag of a template is independently parenthesized when it's an optional chain (`(a?.b)`t``), which already makes the whole template a member expression that `new` accepts, so the outer wrap is redundant: new ((a?.b)`tpl`)() now prints new (a?.b)`tpl` (was new ((a?.b)`tpl`)) new ((a?.b)`tpl`.c)() now prints new (a?.b)`tpl`.c A call in the tag still forces the parens (`ECall` returns true unconditionally), so `new (a()`tpl`)()` stays wrapped. Added assertions for all three.
… callees
Two more cases in `new_callee_needs_parens`, both reached through a tagged
template tag (which prints the tag with ForbidCall dropped):
- `import(...)`, `require(...)`, and `require.resolve(...)` print as calls and
are captured by `new` exactly like `ECall`, but the helper fell through to
`false` for them, so a template-tagged one printed unwrapped:
`new (import(x)`tpl`)()` -> `new import(x)`tpl`` (SyntaxError) and bundled
`new (require("m").f`tpl`)()` -> `new require("m").f`tpl`` (wrong bind). Treat
them like `ECall`.
- An optional-chain call below a crossed non-optional access/template is already
isolated by the printer, so it no longer forces a redundant outer wrap:
`new ((a?.()).b)()` -> `new (a?.()).b`. A non-optional call still always wraps.
Tests added for both; the bundled require case round-trips to `{"ok":1}`.
…plate tags
The previous commit made `new_callee_needs_parens` return true for
`EImport`/`ERequireString`/`ERequireResolveString` anywhere in the chain, but
through an `EDot`/`EIndex` chain those nodes already self-wrap via the forwarded
`ForbidCall`. Forcing the outer wrap there changed the emitted parens for
`new require("m").a.b(...)` from `new (require("m")).a.b(...)` to
`new (require("m").a.b)(...)`, which broke node's test-buffer-indexof.js (it
pins the exact `new (require("buffer")).Buffer.prototype.lastIndexOf(...)`
serialization in an error-message assertion).
Track whether the walk has passed through a template tag (the only node that
drops `ForbidCall`) and hoist the wrap for these call-like leaves only then, so
`new (require("m").f`tpl`)()` still wraps as a whole while the `EDot` chain case
keeps wrapping just the call.
robobun
force-pushed
the
farm/34c957ba/fix-new-callee-parens
branch
from
cbf7897 to
5bd1450
Compare
There was a problem hiding this comment.
No further findings — all prior feedback addressed. Deferring to a maintainer since this touches the JS printer's precedence/paren logic (which already showed one regression mid-PR) and there's an alternative ForbidCall-propagation design on the table that a human may want to weigh.
Extended reasoning...
Overview
This PR fixes #31812: the JS printer was dropping required parentheses around the callee of a new expression when that callee was a member/index/tagged-template chain containing a call or optional-chain link, producing output with different semantics or a SyntaxError. The fix adds a helper new_callee_needs_parens in src/js_printer/lib.rs that walks the callee chain to decide whether the whole chain must be wrapped, and integrates it into the EDot, EIndex, and tagged-ETemplate arms of print_expr. It also fixes a pre-existing dead-code bug where the optional-chain-tag check matched on the template node instead of the tag. ~90 lines of printer logic plus ~80 lines of new test assertions in test/bundler/transpiler/transpiler.test.js.
Security risks
None. This is pure AST-to-source serialization logic with no I/O, auth, crypto, or untrusted-input parsing surface beyond what the printer already handles.
Level of scrutiny
High. The JS printer is on the hot path for every program Bun transpiles or bundles — a subtle paren-placement bug silently changes runtime semantics for end users. The new logic interacts with three existing wrap mechanisms (Level::New/ForbidCall, HasNonOptionalChainParent, and the template-tag optional-chain wrap) and adds two state flags (crossed_non_optional_access, crossed_template) whose correctness depends on precise reasoning about JS grammar precedence. The PR already hit one real regression during development (test-buffer-indexof.js broke in c57542b when the import/require hoist fired through EDot chains; fixed in the next commit by scoping it to template tags), which underscores how easy it is to perturb this code.
Other factors
I reviewed this across three prior rounds (six inline comments) covering over-wrapping edge cases, the dead expr.data vs tag.data check, the EImport/ERequireString call-like leaves, and a test-convention nit — all addressed and resolved, and the current bug-hunting pass found nothing new. Test coverage is thorough (21 expectPrinted_ assertions across member/index/template/optional-chain/isolated-chain/import shapes, plus 4 end-to-end bun -e runtime checks), and robobun independently reproduced the fix and verified 20 cases against Node under both plain and minify modes. That said, robobun also surfaced an alternative smaller-diff approach (propagating ForbidCall through the tag instead of walking the chain), which is a design choice a maintainer may want to weigh before merging. Given the criticality of the printer and that design question, I'm deferring rather than auto-approving.
Per review: the previous shape called new_callee_needs_parens from the
EDot/EIndex/ETemplate arms, and since ForbidCall keeps forwarding down a
plain chain, every level re-walked the remainder - O(n^2) walks for
`new a.b.c.d()`. ENew is the only producer of ForbidCall, so make the
decision there: one O(depth) walk per `new` expression, and zero added
work on the member arms (they revert to their original logic).
When the callee needs the parens, ENew prints them and the target at
Level::Lowest without ForbidCall; otherwise the original New + ForbidCall
path is unchanged, so a call at the base of a member chain still wraps
just itself (`new (require("m")).f`). The one other consumer change is
O(1): when the HasNonOptionalChainParent parens open around an isolated
optional chain, drop ForbidCall since those parens already protect the
call inside (`new (a()?.b).c`, not `new ((a())?.b).c`).
Output is byte-identical across the test matrix; no test changes.
|
Yes — done in fe824dd. The slow part was where the decision was made, not the walk itself: the helper ran in the
Output is byte-identical across the 30-case printer matrix and the 24-case semantic round-trip vs node; transpiler, esbuild, and node's |
There was a problem hiding this comment.
All my earlier feedback has been addressed and I didn't find anything new after the fe824dd refactor — but this is core printer logic that went through several iterations (including one mid-PR regression in c57542b) and a late structural change moving the wrap decision into ENew, so it's worth a maintainer's sign-off.
Extended reasoning...
Overview
This PR fixes #31812: the JS printer dropped required parentheses around new expression callees that are member/index/tagged-template chains containing a call or optional chain, producing output that reparsed with different semantics or as a SyntaxError. The fix adds a new_callee_needs_parens helper that walks the callee chain once per ENew and decides whether to wrap, modifies the ENew arm to print (target) at Level::Lowest when needed (otherwise unchanged), removes ForbidCall inside the HasNonOptionalChainParent wrap in EDot/EIndex to avoid double-wrapping, and fixes a pre-existing dead check in ETemplate (is_optional_chain was matching on the template node instead of its tag). ~90 net lines in src/js_printer/lib.rs plus ~85 lines of tests in transpiler.test.js covering 22 expectPrinted_ assertions and 4 end-to-end runtime checks.
Security risks
None. This is pure AST-to-text printer logic with no I/O, auth, or data handling; the only effect is on emitted JavaScript syntax.
Level of scrutiny
High. The JS printer is on the hot path for every file bun transpiles or bundles, and parenthesization bugs silently change program semantics. The fix itself is subtle: a two-flag (crossed_non_optional_access, crossed_template) state machine walking EDot/EIndex/ETemplate/ECall/EImport/ERequire* nodes, with reasoning that depends on how three separate downstream mechanisms (ForbidCall forwarding, HasNonOptionalChainParent, and the template-tag optional-chain wrap) interact. The PR went through five revision rounds in response to my inline comments — each a real edge case — and one of those (c57542b) introduced a regression in test-buffer-indexof.js that was caught by CI and fixed in cbf7897. The final commit fe824dd is a structural refactor (moving the decision from the member arms to ENew to avoid O(n²) re-walking) that landed after my last round of inline review.
Other factors
All seven of my prior inline comments are resolved with code changes and added test assertions. The bug-hunting system found nothing on the current revision. Test coverage is thorough (member/index/template/optional-chain/nested combinations, over-wrap regression cases, and runtime round-trips), and the author reports byte-identical output across a 30-case matrix plus passing transpiler/esbuild/node suites under ASAN. A second independent implementation on a separate branch reached the same three fixes via a different (ForbidCall-propagation) approach, which is reassuring but also means there was a real design choice here. Given the criticality of the code path, the iteration history, and the late refactor, I'm deferring rather than approving so a maintainer familiar with the printer can confirm the ENew-centred approach and the Level::Lowest + ExprFlag::none() target path are the right shape.
2 participants
Fixes #31812
Repro
Both should print
A {}(matchingnode -p).Cause
The printer mis-transpiles the callee (target) of a
newexpression.newprints its target at
Level::NewwithExprFlag::ForbidCall, and the targetis then responsible for wrapping itself in parens if needed. Only
ECallactually consulted
level.gte(Level::New) || ForbidCallto wrap;EDot,EIndex, and taggedETemplatedid not:ETemplateprinted its tag atLevel::PostfixwithExprFlag::none(),dropping
ForbidCall, sonew (foo()bar)()emittednew foo()bar, which reparses as `(new foo())`bar.EDot/EIndexonly forwardedForbidCalldown to their target, so for anoptional-chain callee the parens landed on the inner call
(
new (baz())?.qux) — andnewinside an optional chain is aSyntaxError.Fix
src/js_printer/lib.rs:ENewnow decides once, before printing its target,whether the whole callee must be parenthesized. A new helper
new_callee_needs_parenswalks the member/index/tagged-template chain towardits base and returns
truewhen it contains a call or a top-reaching optionalchain that
newwould otherwise capture. If so,ENewprints the parens andthe target at
Level::LowestwithoutForbidCall; otherwise the originalLevel::New+ForbidCallpath runs unchanged (so a call at the base of amember chain still wraps just itself, e.g.
new (require("m")).f).Deciding at
ENewkeeps it a single O(depth) walk pernewexpression; anearlier revision made the decision in the
EDot/EIndex/ETemplatearms,which re-walked the chain at every level while
ForbidCallforwarded(O(n^2) for
new a.b.c.d()). A plain member chain (new (a.b.c)()) stilldrops the redundant parens since
new a.b.c()is identical, and an optionalchain already isolated below a non-optional access keeps the minimal form
(
new (a?.b).c, notnew ((a?.b).c)).Also fixes a pre-existing dead check in the tagged-template printer: the
optional-chain test matched on the template node instead of the tag, so
(a?.b)tplprinted as the invalid `a?.b`tpl; it now usestag.is_optional_chain().Transpiled output before → after:
new (foo()bar)()new foo()bar`` ❌new (foo()bar)✅new (baz()?.qux)()new (baz())?.qux❌new (baz()?.qux)✅new (a?.b)()new a?.b❌new (a?.b)✅new (a.b.c)()new a.b.c✅new a.b.c✅Same bug was reported against esbuild (evanw/esbuild#4477) and Babel
(babel/babel#18045).
Verification
A {}(also under--minify).optional-chain / nested-
new/ isolated-optional-chain) matchesnodeexactly, with no over-wrapping; a 30-case printer matrix is byte-identical
before and after the decide-at-
ENewrefactor.test/bundler/transpiler/transpiler.test.js: anexpectPrinted_test asserting exact paren placement, and an end-to-endconcurrent
test.eachthat runs the transpiled program and checks it buildsA {}. All fail before the fix and pass after.transpiler,esbuild/default,dce,extra,lower,ts), and node'stest-buffer-indexof.js(which pins the exactnew (require("buffer")).Buffer.prototype.lastIndexOf(...)serialization)passes.