js_printer: wrap cross-module enum inlined as a delete operand when non-finite - #36744
js_printer: wrap cross-module enum inlined as a delete operand when non-finite#36744robobun wants to merge 5 commits into
Conversation
…-finite delete E.N across modules (E a TS enum, N = NaN or Infinity) bundled to 'delete NaN' / 'delete Infinity': a strict-mode SyntaxError in the emitted ESM bundle. The source is an EDot so WAS_ORIGINALLY_DELETE_OF_IDENTIFIER_OR_PROPERTY_ACCESS is set and the EUnary '(0, ...)' re-wrap is skipped; the EDot print arm then replaces the property access with the constant via try_to_get_imported_enum_value. Thread an IsDeleteTarget ExprFlag from the EUnary delete arm (only when the parse-time flag is set, so it never reaches compound operands that forward flags to children) into the EDot/EIndex print arms, and have print_inlined_enum wrap non-finite numbers in '(0, ...)'. Finite numbers and strings are left as 'delete 42' / 'delete "s"' since those are valid strict-mode expressions.
WalkthroughThe PR adds bundler tests for cross-module enum values in ChangesDelete-target enum inlining
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted fix for a real bundler correctness bug (delete NaN strict-mode SyntaxError from print-time enum inlining).
What was reviewed:
- Flag propagation:
IsDeleteTargetis only set whenWAS_ORIGINALLY_DELETE_OF_IDENTIFIER_OR_PROPERTY_ACCESSis set, so it never reaches compound operands;EDotmasks flags before printinge.target(line 3283) andEIndexexplicitly removes it, so no leakage into subexpressions. print_inlined_enumwrap: level lowered toCommainside(0, ...)soprint_numberdoesn't double-paren-Infinity; comment placement stays inside the wrap.- Tests exercise both
EDot/EIndex, all three non-finite values, finite/string (asserted NOT wrapped), and minified output;run:executes the bundle so a residualdelete NaNwould SyntaxError and fail.
Extended reasoning...
Overview
Adds ExprFlag::IsDeleteTarget to src/js_printer/lib.rs, threads it from the EUnary delete arm into the EDot/EIndex print arms, and gives print_inlined_enum a new is_delete_target: bool parameter that wraps non-finite numbers in (0, ...). Two new itBundled tests in test/bundler/esbuild/ts.test.ts.
Security risks
None. Bundler output-correctness change; no untrusted-input handling, no allocation, no FFI.
Level of scrutiny
Medium — print_expr is on the hot path for all bundler output, but the added work is a couple of bitset contains/remove calls and a bool parameter. I traced flag lifetime through both arms: in EDot the flag is captured before the early-return inlining path and then stripped by the existing flags &= HasNonOptionalChainParent | ForbidCall mask before e.target is printed; in EIndex it is explicitly removed before e.target/e.index are printed. The gating on the parse-time WAS_ORIGINALLY_DELETE_OF_IDENTIFIER_OR_PROPERTY_ACCESS flag guarantees the operand was an identifier/property-access in source, so the flag cannot land on an EIf/EBinary that would forward it to a non-delete-target child. Optional-chain accesses (delete E?.N) skip the inlining branch entirely (e.optional_chain.is_none()), so no interaction there.
The wrap itself mirrors the existing (0, ...) pattern already used in the EUnary arm (b"(0," + print_space() + inner at Level::Comma + b")"). Placing the closing paren after the /* name */ comment matches the whitespace-minified assertion (delete(0,NaN) — comment suppressed under minify_whitespace).
Other factors
The PR description is unusually precise about why #36740 (guard on the unset-flag path) and #36734 (visitor-time delete_target for same-module EInlinedEnum) do not cover this print-time EImportIdentifier path — I verified that reasoning against the code. The test matrix hits every branch of the wrap predicate (NaN / +Inf / -Inf wrapped; finite 42 and string "s" explicitly asserted unwrapped) across both E.N and E["N"], and the run: stdout check would fail on the unfixed build via SyntaxError, so the tests are non-vacuous. The choice to wrap rather than suppress inlining is justified (semantics preserved: delete (0, NaN) → true, matching delete on a configurable enum member) and avoids forcing the linker to retain the enum object.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/bundler/esbuild/ts.test.ts`:
- Around line 2239-2243: Shorten or remove the paragraph comment above the
cross-module enum inlining test. Keep only a concise statement of the
non-obvious behavior being tested, relying on the test name and assertions for
the specific NaN/Infinity cases; do not describe printer control flow or
implementation details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7697f0af-afca-4062-8358-69777eb80943
📒 Files selected for processing (2)
src/js_printer/lib.rstest/bundler/esbuild/ts.test.ts
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js_printer/lib.rs:4033-4040— The sameWAS_ORIGINALLY_DELETE_OF_IDENTIFIER_OR_PROPERTY_ACCESS→ strict-modedelete <id>explanation is repeated across theExprFlag::IsDeleteTargetdoc comment (1245-1249), this 8-line block, and the 5-line block inprint_inlined_enum(6196-6200) — the comment-cop bot flagged all of them. Consider keeping the shared mechanism once on the enum-variant doc comment and trimming each call site to its unique bit: here, just the one-liner about gating on the parse-time flag so it never reachesEIf/EBinarychildren; inprint_inlined_enum, just thedelete <value>→truesemantics.Extended reasoning...
What
This PR adds three multi-line comment blocks that each restate the same core invariant:
ExprFlag::IsDeleteTargetdoc comment (lib.rs:1245-1249): the operand of adeletewhose source form was an identifier/property access; a print-time rewrite that surfaces a bare identifier (cross-module enum inlining toNaN/Infinity) must keep it a value.EUnaryarm (lib.rs:4033-4040): the operand was a property access in source, so the(0, ...)re-wrap above is skipped; cross-module enum inlining can still replace it withNaN/Infinity(a strict-modedelete <id>SyntaxError); tell the operand it's the delete target so the inline path can wrap itself.print_inlined_enum(lib.rs:6196-6200):delete E.NsetsWAS_ORIGINALLY_DELETE_OF_IDENTIFIER_OR_PROPERTY_ACCESS, so theEUnaryarm does not re-wrap; inlining a non-finite number would printdelete NaN— a strict-mode SyntaxError; wrap in(0, ...).
All three narrate the same causal chain: parse-time flag set →
EUnaryskips the re-wrap → print-time enum inlining emitsNaN/Infinity→ strict-modedelete <id>SyntaxError. The repo'sgithub-actionscomment-cop bot flagged every one of these locations on the PR timeline with the CLAUDE.md rule-13 quote ("If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code").Why it applies
REVIEW.md's comment rule is explicit: "Comments carry only durable non-obvious content… No narrating what the code does, no bug history — that belongs in the PR description." CLAUDE.md rule 14 adds: "Is this information the next Claude would spend multiple tool calls trying to understand? If the answer isn't clearly yes, the code comment is noise — delete it." And
src/CLAUDE.mdsays outright: "Avoid adding comments where not necessary."The invariant itself is non-obvious and worth documenting once — a reader landing on
print_inlined_enumwith no context would not immediately guess whyis_delete_target && !n.is_finite()triggers a(0, ...)wrap. But once theExprFlag::IsDeleteTargetdoc comment states the contract ("a print-time rewrite that would surface a bare identifier here must keep it a value, not a Reference"), the call sites are just honoring that contract, and re-explaining the wholeWAS_ORIGINALLY_...→EUnaryre-wrap-skip → SyntaxError chain at each site is narration the PR description already covers in full.Step-by-step
- A reader hits
value_flags.insert(ExprFlag::IsDeleteTarget)at line 4041 and wants to know what the flag means. - They jump-to-definition → land on the enum variant at 1245-1249, which already says: direct operand of a
deletewhose source was an identifier/property access; print-time rewrites that would surface a bare identifier (e.g. cross-module enum inlining toNaN/Infinity) must keep it a value. - That's the whole mechanism. The 8-line block at 4033-4040 re-tells the same story, adding only one non-derivable fact: gating on the parse-time flag keeps this from reaching compound operands (
EIf/EBinary) that forwardflagsto children which are not themselves delete targets. - Similarly at 6196-6200: the only fact not already on the enum doc is the semantics justification —
delete <value>evaluates totrue, matching the source semantics (enum members are configurable).
Suggested trim
Keep the
ExprFlag::IsDeleteTargetdoc comment as the canonical explanation, then:- 4033-4040 → one line:
// Gate on the parse-time flag so IsDeleteTarget never reaches EIf/EBinary children via forwarded flags. - 6196-6200 → one line:
// delete (0, NaN) evaluates to true, matchingdelete E.Non a configurable member. - The 2-line comment at 3316-3317 (
EIndex) is already terse and site-specific; it can stay.
Impact
None on behavior — this is comment hygiene only. It's flagged as a nit because the code is correct and the explanation is useful; it's just stated three times where once (plus site-specific one-liners) would do, and the repo's own automation already called it out.
There was a problem hiding this comment.
LGTM — targeted printer fix for a strict-mode delete <identifier> SyntaxError from cross-module enum inlining.
What was reviewed:
- Flag propagation:
IsDeleteTargetis only set when the parse-timeWAS_ORIGINALLY_DELETE_OF_IDENTIFIER_OR_PROPERTY_ACCESSflag is set, so it never reaches compound operands;EDotclears it via theflags &= ...mask before recursing into the target, andEIndexremoves it explicitly. - Both call sites of
print_inlined_enumupdated; wrap emits(0, X /* name */)withlevel = Commaand closes after the comment, matching the asserted output in both normal and minified modes. - Tests cover
EDotandEIndexpaths, all three non-finite values, finite/string (left unwrapped), plus arunassertion so the bundle actually executes.
Extended reasoning...
Overview
Fixes a bundler codegen bug where cross-module TS enum inlining at print time emits delete NaN / delete Infinity — a strict-mode SyntaxError in the ESM output. Changes are confined to src/js_printer/lib.rs (~35 lines: one new ExprFlag variant, threading it from the EUnary delete arm into the EDot/EIndex arms, and a small wrap in print_inlined_enum) plus two new itBundled tests in test/bundler/esbuild/ts.test.ts.
Security risks
None. Pure print-time output shaping; no untrusted input parsing, no allocations, no FFI.
Level of scrutiny
Moderate — bundler output correctness matters, but this is a narrow, well-understood edge case (delete on an inlined non-finite enum member). The mechanism is precise: IsDeleteTarget is set only when op == UnDelete and the parse-time flag confirms the original operand was a property access, so the operand at print time is exactly the EDot/EIndex node — the flag cannot leak to unrelated children. In EDot the subsequent flags &= HasNonOptionalChainParent | ForbidCall mask clears it before recursing into the target; in EIndex it's removed explicitly before both the target and index are printed. The wrap uses Level::Comma inside the parens, which is correct for (0, X).
Other factors
- Only two callers of
print_inlined_enumexist and both are updated. - Tests exercise both the
EDot(E.N) andEIndex(E["N"]) inlining paths, all non-finite values (NaN, ±Infinity), and confirm finite numbers / strings are left unwrapped (delete 42/delete "s"are valid). The minified variant checks whitespace-free output. Both testsrunthe bundle and assert[true,...], so a SyntaxError would fail loudly. - All prior bot comments (comment-cop, CodeRabbit) were about comment length and are resolved; the one remaining source comment is a single line stating the invariant.
- The PR description explains why the sibling fixes in #36740 (unset-flag path) and #36734 (visit-time
EInlinedEnum) don't cover this print-timeEImportIdentifierpath — the reasoning checks out against the code.
|
Diff is ready: the two new Build 87497 (final): all
Earlier builds 87409/87436 had every job expire with no agents available. |
|
Updated 10:19 PM PT - Aug 1st, 2026
❌ @robobun, your commit a05e926 has 2 failures in
UnsupportedThe requested configuration is currently not supported. Please check the documentation for supported configurations.c264b60a-68b2-4810-94b6-798cd9cef044
UnsupportedThe requested configuration is currently not supported. Please check the documentation for supported configurations.957f77bb-c21a-443b-930a-0bb6686d78ac
🧪 To try this PR locally: bunx bun-pr 36744That installs a local version of the PR into your bun-36744 --bun |
1 participant
Problem
Cross-module TS enum inlining happens at print time in the
EDot/EIndexarms viatry_to_get_imported_enum_value. The source isdelete E.N(anEDot), soWAS_ORIGINALLY_DELETE_OF_IDENTIFIER_OR_PROPERTY_ACCESSis set and the(0, ...)re-wrap in theEUnaryarm is skipped; theEDotarm then replaces the property access withNaN/Infinity, which are identifier references and therefore a strict-modedelete <id>SyntaxError in the emitted ESM bundle. esbuild has the same bug.Related but distinct:
(0, ...)re-wrap when the parse-time flag is unset (operand was not a property access in source) by extendingis_identifier_or_numeric_constant_or_property_access. That guard is short-circuited here because the flag is set.p.delete_targetin the visitor so same-module enum references (which becomeEInlinedEnumat visit time) are not inlined when they are the delete target. That does not reach this path:try_to_get_imported_enum_valuefires at print time after the visit pass, keyed onEImportIdentifier.Fix
Add
ExprFlag::IsDeleteTargetand set it when printing the operand ofdeletewhose parse-time flag is set (so it never reaches compound operands likeEIf/EBinarythat forwardflagsto children which are not themselves delete targets).print_inlined_enumwraps in(0, ...)whenis_delete_targetand the value is a non-finite number. Finite numbers and strings are left as-is (delete 42/delete "s"are valid strict-mode expressions that already evaluate totrue).delete (0, NaN)evaluates totrue, matching the source semantics: enum members are own configurable data properties, sodelete E.Nreturnstrue. Keeping the property access instead of inlining would require the linker to retain the enum object for this one use; the wrap preserves the return value without that coupling and is consistent with the existing semantic model for enum inlining.Verification
$ bun bd test test/bundler/esbuild/ts.test.ts -t EnumCrossModuleInliningDeleteTarget # 2 new tests, fail on mainFull
esbuild/ts.test.ts(59 pass),bundler_edgecase.test.ts(117 pass),bundler_minify.test.ts(42 pass),transpiler.test.js(183 pass),esbuild/default.test.ts(151 pass),esbuild/dce.test.ts(78 pass) unchanged.[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file