Repository navigation
Conversation
The BinPow left-operand check in binary_check_and_prepare bumps left_level
to Call for ENumber so a negative number prints as `(-5) ** 2` instead of
the syntax error `-5 ** 2`. An EInlinedEnum wrapping an ENumber fell
through the match and reached print_number at Level::Exponentiation, which
does not wrap negatives, so `const enum E { X = -5 }; E.X ** 2` emitted
`-5 /* X */ ** 2`.
Look through EInlinedEnum at the wrapped value before the match. Apply the
same level bump inside print_inlined_enum for the bundler's cross-module
EDot/EIndex inlining path, which hits the same SyntaxError.
|
Warning Review limit reached
Next review available in: 8 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 (3)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Keeps all the BinPow left-operand level bumps in one place instead of a level rewrite inside print_inlined_enum. EDot/EIndex only ever use the incoming level on the print_inlined_enum path, so the bump is a no-op for a member expression that is not inlined.
|
Updated 12:28 PM PT - Jul 27th, 2026
❌ @robobun, your commit b87a5cb has 3 failures in
🧪 To try this PR locally: bunx bun-pr 36133That installs a local version of the PR into your bun-36133 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js_printer/lib.rs:6313-6317— Thelevel == Level::Exponentiationbump inprint_inlined_enumalso fires for prefix-unary andawaitoperands (both printed atLevel::Prefix.sub(1) == Exponentiation), so cross-module-E.Neg/await E.Negnow emit-(-5)/await (-5)instead of- -5/await -5. It's cosmetic (output is still valid), but it diverges from the same-fileEInlinedEnumpath at lib.rs:4177-4178, which still emits- -5 /* X */. If you want to avoid the extra parens you could gate the bump onnum.is_sign_negative()— a non-negative number never needed the wrap anyway — or pass a flag from the BinPow LHS position rather than keying on the level alone.Extended reasoning...
What happens
Level::Exponentiationsits directly belowLevel::Prefixin the enum ordering (src/ast/op.rs:173-174, values 17 and 18), soLevel::Prefix.sub(1) == Level::Exponentiation. The printer usesLevel::Prefix.sub(1)for the operand of prefixEUnary(lib.rs:4107) and the operand ofEAwait(lib.rs:4042). That meansLevel::Exponentiationis not unique to the LHS-of-**position — any cross-module inlined enum used as-E.Neg,~E.Neg,void E.Neg,await E.Neg, etc. reachesprint_inlined_enumwithlevel == Level::Exponentiationand gets bumped toLevel::Call.Step-by-step
Given:
// enums.ts export enum E { Neg = -5 } // entry.ts import { E } from './enums'; console.log(-E.Neg);
EUnary(UnNeg)prints-, setsprev_op = UnNeg, then callsprint_expr(E.Neg, Level::Prefix.sub(1), ...)at lib.rs:4107.Level::Prefix.sub(1) == Level::Exponentiation.E.Negis anEDotover anEImportIdentifier; the EDot handler resolves the cross-module enum and callsprint_inlined_enum(Number(-5), "Neg", Level::Exponentiation).- The new code sees
level == Level::Exponentiationand bumps toLevel::Call. print_number(-5, Level::Call)at lib.rs:6508 testslevel.gte(Level::Prefix)—Call >= Prefixis true — and prints(-5).- Final output:
-(-5) /* Neg */.
Before this PR, step 3 didn't bump, so
print_number(-5, Level::Exponentiation)fell through to theelsebranch (lib.rs:6516-6518), calledprint_space_before_operator(UnNeg)(which inserts a space becauseprev_op == UnNeg), and printed- -5 /* Neg */. Same story forawait E.Neg: previouslyawait -5, nowawait (-5).Inconsistency with the same-file path
The same-file
const enumpath routes throughExprData::EInlinedEnumat lib.rs:4177-4178, which recurses viaprint_expr(e.value, level, flags)with the incoming level unchanged. Soconst enum E { X = -5 }; -E.Xreachesprint_number(-5, Exponentiation)and still prints- -5 /* X */. After this PR the two inlining paths disagree: same-file emits- -5, cross-module emits-(-5).Scope
The RHS of
**is not affected:binary_check_and_preparesetsright_level = e_level.sub(1) = Level::Multiplyfor BinPow (lib.rs:1494;**is right-associative so onlyleft_levelis raised back toExponentiation), so2 ** E.Negstill reachesprint_inlined_enumatLevel::Multiplyand prints2 ** -5on both paths — the PR description's "the right operand is unaffected" claim holds.Impact and fix
Purely cosmetic: the extra parentheses are always valid JS, and this only affects the narrow case of a cross-module negative enum value used as the operand of a prefix unary or
await. It's a byte or two of unnecessary output plus a minor inconsistency between the two inlining paths.If you want to tighten it, the simplest change is to condition the bump on the value actually being negative —
if level == Level::Exponentiation && num.is_sign_negative()— since a non-negative number never needs wrapping at any level and a negative one only needs it in the LHS-of-**position (where it's mandatory) and now also under prefix-unary/await (where it's redundant but harmless). Alternatively, thread an explicit flag from the BinPow left-operand position instead of inferring it fromlevel. Either way, not worth blocking on.
|
CI on b87a5cb is green for the printer/bundler changes; both new test files pass on every lane that ran them. The red lanes are unrelated to this diff:
All three are reported for main-break triage. Ready for a maintainer. |
There was a problem hiding this comment.
LGTM — targeted precedence fix in the BinPow arm with tests covering both inlining paths.
What was reviewed:
- Traced
EDot/EIndexprinting (lib.rs:3349-3454):levelis only consumed on theprint_inlined_enumpath, so theLevel::Callbump underts_enums.is_some()is a no-op for non-inlined member expressions —obj.b ** 2still prints without parens. unwrap_inlined()returns the wrappedExprby value (Expr: Copy), so the existingEUnary/EAwait/ENumberarms behave identically for non-inlined LHS.- Tests exercise
EDot+EIndex, negative/positive/string, RHS unaffected, minified variant, and the no-over-wrap assertion under prefix unary; thenew Function(out)loop guards syntactic validity across the matrix.
The pre-existing EImportIdentifier → void 0 ** 2 sibling I flagged earlier is acknowledged and deferred to a follow-up; not blocking.
Extended reasoning...
Overview
Fixes a printer bug where an inlined negative TypeScript enum value on the LHS of ** emits -5 ** 2 (a SyntaxError — the ES grammar requires an UpdateExpression on the left of **, and -5 is a UnaryExpression). Two substitution paths were bypassing the existing ENumber level bump: same-file EInlinedEnum wrapping, and cross-module EDot/EIndex → print_inlined_enum. The fix is 7 lines in binary_check_and_prepare's BinPow arm: unwrap EInlinedEnum before matching, and bump left_level to Call for EDot/EIndex when options.ts_enums is populated.
Security risks
None. Pure output-precedence adjustment in the JS printer; no untrusted-input handling, allocation, or FFI changes.
Level of scrutiny
Moderate — js_printer/lib.rs is a hot shared path, so the main risk is over-parenthesization regressions. I verified the key claim in the PR description: the EDot (lib.rs:3349-3400) and EIndex (lib.rs:3401-3454) printers only consult the incoming level on the print_inlined_enum branch; the non-inlined path prints target.prop / target[idx] without any level.gte(...) wrap check. So bumping left_level to Call when ts_enums.is_some() cannot add parens to obj.b ** 2. print_number at Level::Call (≥ Prefix) wraps negatives and neg-infinity but not non-negatives, matching the test's not.toContain("(5)"). Expr is #[derive(Copy)], so e.left.unwrap_inlined().data is sound and the existing match arms are unchanged for non-enum LHS.
Other factors
Test coverage is thorough for a fix of this size: transpiler tests hit the EInlinedEnum path via EDot and EIndex, positive/negative/string values, the RHS (unaffected), and validate every emitted form via new Function; bundler tests hit the cross-module print_inlined_enum path plain and minified, run the output for correctness (25 25 25 0.03125), and lock in the no-over-wrap-under-prefix-unary property (- -5 /* Neg */, not -(-5)). The author already responded to the comment-cop feedback (comment trimmed to one line in b87a5cb) and to my earlier over-wrap concern (fix moved from print_inlined_enum into the BinPow arm in ebfbc19, so it only fires when EDot/EIndex is the direct LHS of **). The pre-existing EImportIdentifier-with-Missing-status sibling I flagged is narrow, warned-about by the linker, and reasonably deferred. All inline threads are resolved.
|
@robobun is this exactly what esbuild does? |
|
No, esbuild has the same bug. Its (checked against 0.21.5 and 0.28.1; I didn't find an open esbuild issue for it) The mechanism here is the same as esbuild's existing arms (bump |
|
This still reproduces on 1.4.2 and the current canary (1.4.3-canary.1+f42e98025): I arrived at the same fix independently before finding this PR. The branch is |
|
Since #41186 (1.4.1) the same output also comes from the locals of I applied the printer hunk of this PR on 158ff6c and ran 10 forms (static, |
import.meta.main node and the left operand of ** by level
#33447
Repro
Bun.Transpiler({loader: "ts"}).transformSync(...)andbun buildboth emit:which is a
SyntaxError(aUnaryExpressionis not a valid left operand of**).The cross-module bundler path has the same problem:
Cause
binary_check_and_preparebumpsleft_leveltoCallwhen the left operand of**is anENumber, soprint_numberparenthesizes a negative value. Two paths bypass that bump and reachprint_numberatLevel::Exponentiation, which does not wrap negatives:E.XtoEInlinedEnum(ENumber(-5)). The BinPow left-operand match had noEInlinedEnumarm, soleft_levelstayed atExponentiation.EDot/EIndexover anEImportIdentifier. The printer substitutes the enum value viaprint_inlined_enumat print time, callingprint_number(num, level)with the incomingExponentiationlevel.Fix
Both handled in the BinPow arm of
binary_check_and_prepare:EInlinedEnumat its wrapped value before the left-operand match.left_leveltoCallforEDot/EIndexwhenoptions.ts_enumsis populated.EDot/EIndexprinting only uses the incoming level on theprint_inlined_enumpath, so the bump is a no-op for a member expression that is not inlined (obj.b ** 2still prints without parentheses).Non-negative numbers and string enum values were already valid on the left of
**and continue to print without extra parentheses. The right operand is unaffected. A cross-module inlined enum under a prefix unary (-E.Neg) still prints as- -5, not-(-5), since the bump is scoped to the BinPow left operand.Verification
New tests in
test/bundler/transpiler/transpiler.test.js(same-fileEInlinedEnumpath) andtest/bundler/esbuild/ts.test.ts(cross-moduleprint_inlined_enumpath, plain and minified, plus the prefix-unary no-over-wrap assertion) fail against the unfixed build with theUnexpected **syntax error and pass with the fix.[review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file