Repository navigation
Conversation
…ounted a read The printer replaced every `X.name` on an imported TypeScript enum with the member's value. The parser and the linker decide the same thing earlier (`record_import_property_use`), to know if `X` is still used, and the two decisions differed: - The parser does not count a write or a `delete` target as a read. The printer inlined it anyway, so `E.A = 5` printed as `1 /* A */ = 5`, a SyntaxError. - The parser reads the key of `E[Key.first]` through an inlined enum. The printer did not, so the linker dropped `E` and the printer still printed `E["A"]`, a ReferenceError. The printer now inlines only a node the parser marked (`is_import_property_use`), and it reads an index key the way the parser does. With `minify_syntax` the parser turns `E[key]` into `E.A` once the key folds to a string. That node had no mark. It now gets one, so such a read stays inlined and `E` can be removed.
|
Updated 3:46 AM PT - Sep 12th, 2026
❌ @robobun, your commit e7b7769 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 42454That installs a local version of the PR into your bun-42454 --bun |
|
Reproduced on 1.4.3-canary.1+4ff919377 with
The new cases in CI (build 114671): the diff is green. Both changed test files pass on every lane. Two tests are red, and neither uses the bundler:
Both are reported for main-break triage. The other 9 entries (inspector protocol, napi uv, webview, cpu-prof, expect-assertions, install registry, install patch, filesink, css color) passed on retry or alone. This PR is ready for a maintainer. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. WalkthroughThe change tracks import-property usage during index-to-dot conversion, limits imported enum substitution to tracked reads, and adds regression coverage for writes, deletes, computed indexes, minification, and cyclic module initialization. ChangesEnum import-property handling
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The enum access tracking and printer gating changes preserve writable targets while retaining valid read inlining coverage. 🚥 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 — the printer's inlining decision now matches the parser's use-count decision via the existing is_import_property_use flag, and the folded-index path is kept in sync.
What was reviewed:
- Traced
record_import_property_usein the new minify-fold branch: single decrement (early return prevents the non-minify call at L1057), short-circuits before mutation on optional chain / assign-target / delete. - Checked the
E::Indexprinter arm:unwrap_inlined()on the key mirrors the parser's read atvisit_expr.rs:1000, so parser and printer agree on which keys are inlinable. - Confirmed the one pessimization (post-visit nodes like React Compiler output lose the inline) stays runtime-correct because the base use was never decremented, so the enum object is kept.
- Tests cover the full assign-target matrix (simple/compound/++/destructure/rest/default/for-in/for-of/delete/logical), Dot and Index, identifier and non-identifier keys, minify on and off, and the cycle guard for the parser-side change.
Extended reasoning...
Overview
This PR fixes cross-module TypeScript enum inlining in the bundler so that only reads of E.member are replaced with the constant value; writes, compound assignments, destructuring targets, for-in/for-of targets, and delete targets keep the property access. The printer (src/js_printer/lib.rs, E::Dot and E::Index arms) now gates try_to_get_imported_enum_value behind the existing e.is_import_property_use flag, and the E::Index arm now calls unwrap_inlined() on the key so E[Key.first] (where Key.first was itself inlined) is recognized. The parser visitor (src/js_parser/visit/visit_expr.rs) now marks the E::Dot produced by the minify_syntax a["b"] → a.b fold via record_import_property_use, using the same hoisted IdentifierOpts as the non-fold branch. A doc comment on Dot::is_import_property_use is expanded, and six new bundler tests are added.
Security risks
None. This is a compiler/bundler code-generation change with no auth, crypto, filesystem, or network surface. The only user-observable effect is that previously-invalid emitted JS (1 = 10, delete NaN) now prints as the original property access.
Level of scrutiny
Moderate. The native change is ~30 lines across two files and reuses an existing, already-tested flag (is_import_property_use) that import_member_binding was already gated on. The key correctness invariant — "inline only where the parser decremented the base import's use count" — is exactly what the flag encodes, so the printer and linker cannot disagree about whether the enum object survives. I traced record_import_property_use to confirm the new call site in the minify-fold branch cannot double-decrement (the branch returns early before the existing call at L1057), short-circuits on optional_chain.is_some() before any state mutation, and returns false without side effects for assign/delete targets. The unwrap_inlined() addition in the E::Index printer arm mirrors line 1000 of visit_expr.rs, so the parser and printer read the same key.
Other factors
Test coverage is unusually thorough for a bundler fix: both itBundled and itElides variants, static import / import * / await import() / require() / .then(), every assign-target form including nested destructuring with defaults and rest, E::Dot and E::Index with identifier and non-identifier keys, minifySyntax both on and off, and a dedicated import-cycle case that guards the parser-side change in isolation. The one acknowledged trade-off (React Compiler-generated nodes lose the inline but stay runtime-correct because the enum object is retained) is a minor pessimization, not a correctness issue. No CODEOWNERS cover the changed paths, the bug hunt exited on dry_streak, and there are no outstanding reviewer objections.
This branch has not been deployed
1 participant
Problem
E[Key.first](E: enum from another file,Key: string enum in this file) bundles toE["A"]after the linker removedE:ReferenceError: E is not defined.tscaccepts this input.E.A = 5bundles to1 /* A */ = 5:SyntaxError: Left side of assignment is not a reference. So doE.B++,[E.B] = [7],delete E.N. This needs JS oras any. bundler: read import() / require() exports without a namespace object #41186 added therequire()/import()forms.print_expr(src/js_printer/lib.rs) picks theX.namenodes to inline on its own. The parser picks first (record_import_property_use,src/js_parser/p.rs). It unwraps an inlined key and, since bundler: bind property accesses on re-exported namespaces directly #41009, skips write anddeletetargets.Fix
is_import_property_use, and it unwraps the key the same way. A write ordeletetarget has no mark and prints as written. The parser counted it as a use ofE, so the enum object stays.--minify-syntaxthe parser foldsE[key]intoE.Aafter the visit (visit_expr.rs). That node now gets the mark, so the read stays inlined.Status.Busy, not1. It kept the enum object before too.test/bundler/esbuild/ts.test.ts(4 new cases),test/bundler/bundler_dynamic_import_dce.test.ts(2 new cases). 1.4.3-canary.1 fails 5 of 6. Other suites: see Notes.Background
ts_enumstable.E.Aprints as1 /* A */.record_import_property_usecounts a read ofX.nameas a use of the property, not ofX. The linker dropsXwhen every counted read is a known member.is_import_property_use(onE::Dot/E::Index) records that count.import_member_bindingalready needs it.Notes
is_import_property_usedoc comment, the--minify-syntaxmark (the review found the import-cycle case below), andEIndextargets plus non-identifier keys in the tests, so the minified variants fail before the fix too.ts.test.ts(EnumCrossModuleInliningFoldedIndexInCycle) passes on 1.4.3-canary.1. It guards the--minify-syntaxpath:LogLevel[process.env.NODE_ENV]in a file that runs before the enum's file. With only the printer change that bundle threwTypeError: undefined is not an object (evaluating 'e.production').record_import_property_useonly. It does not runmaybe_rewrite_property_accessthere, sons["a" + ""]on animport * as nsprints as before.require(),await import()and.then()forms. js_printer: parenthesize inlined negative enum value on LHS of ** #36133 fixes-1 /* A */ ** 2in all 10 forms. TheEImportIdentifierarm in js_printer: print0 / 0andvoid 0where a local binding orwithobject shadowsNaN/undefined#36124 fixesvoid 0 ** 2in all 8 forms. This PR does not touch those lines.delete NaNasdelete (0, NaN). It predates bundler: bind property accesses on re-exported namespaces directly #41009. With this changedelete E.Nprints as written, so that wrap is not needed.1 /* A */ = 5. ForE[Key.first]it keepsEand printsE["A" /* first */], which runs.record_import_property_usereturns false withoutbundle, andts_enumsis only set by the linker), so the transpiler cache version stays.esbuild/ts,esbuild/importstar,esbuild/dce,esbuild/default,esbuild/splitting,esbuild/lower,bundler_dynamic_import_dce(282),bundler_minify,bundler_edgecase,bundler_regressions,bundler_cjs,bundler_cjs2esm,bundler_barrel,bundler_splitting,bundler_promiseall_deadcode,bundler_jsx,transpiler/react-compiler,transpiler/transpiler.test.js.[human-review] gate passed · iteration 0 · 5 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