js_parser: give standard decorator lowering temporaries unique names - #38734
js_parser: give standard decorator lowering temporaries unique names#38734robobun wants to merge 4 commits into
Conversation
The temporaries the standard decorator / accessor lowering declares (_init, _dec, _<Class>, the _<name> WeakMap of every lowered member, ...) were created with fixed names. The runtime transpiler prints symbols by original name, so they collided with identifiers of the same name in the file and with each other, and they were never added to declared_symbols, so the bundler's top-level renamer did not rename them either. Register each temporary in the scope it hoists to and in declared_symbols so the renamers handle it, and when no renamer runs pick file-unique names after the visit pass, once every identifier the file uses has a symbol.
|
Warning Review limit reached
Next review available in: 24 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 (5)
Comment |
|
Status: ready for review (current revision ec51ea7). Reproduced on main (9805144) and on the released 1.4.0 with plain
The new tests in CI: build 97649 for the current head finished with 177 of 179 jobs passed and no test failures (the annotated ones are unrelated tests that passed on retry); the two |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a non-trivial change to the parser's decorator lowering (new post-visit naming pass, changed scope/declared_symbols registration for every temporary) and it supersedes #31930, a human look would still be worthwhile.
What was reviewed:
new_temp's hoisting-scope walk +declared_symbolsappend matches the existingusinglowering pattern atp.rs:9057-9075, and.expect("oom")is the local convention there.- The removed
expr_var_decls.pushfor_dec/_computedKeywas a genuine duplicate — Phase 8 already extracts everySLocalbinding frompre_eval_stmtsintoexpr_var_decls(lower_decorators.rs:2515-2531). name_decorator_tempsruns after allappend_partcalls and is a no-op on the bundle path (only populated when!will_use_renamer());accessor_storage_basegates onis_utf8() && is_identifier, fixing theaccessor "x y"case.
Extended reasoning...
Overview
Fixes name collisions in the standard-decorator / accessor lowering (src/js_parser/lower/lower_decorators.rs). The lowering emits temporaries (_init, _dec, _<name> WeakMaps, ...) next to each class; previously those were created via new_sym with fixed names and only registered in current_scope.generated, so under the no-op renamer (runtime transpiler / --no-bundle) two distinct symbols printed as one identifier, and under the bundler renamer top-level ones were never renamed at all. The fix introduces new_temp, which registers each temporary in the nearest var-hoisting scope and in Part.declared_symbols, and a post-visit name_decorator_temps pass that assigns file-unique names when no renamer will run. It also fixes accessor "x y" producing an invalid var _x y and removes a double-push of pre-eval bindings into expr_var_decls. New state on P (decorator_temp_refs), one new call in _parse, and 15 new tests across the bundler and runtime-transpiler suites.
Security risks
None. Purely codegen naming; no input parsing, allocation sizing, or trust-boundary changes.
Level of scrutiny
Medium-high. This is core JS parser / lowering that ships in every bun run and bun build. The change is well-reasoned and heavily tested, but it introduces a new post-visit naming pass and changes how every decorator temporary is registered with the scope tree and declared_symbols — which feeds both NumberRenamer and MinifyRenamer. That is exactly the kind of subtle interaction where a maintainer familiar with the renamer contracts should sign off. It also supersedes an alternative approach in #31930, which is a design call.
Other factors
The implementation mirrors an existing pattern precisely: the hoisting-scope walk + is_top_level + scope.generated/declared_symbols registration is byte-for-byte the shape used by the using lowering at p.rs:9057-9079, including the .expect("oom") convention. The removed expr_var_decls pushes were verified to be duplicates of Phase 8's SLocal-extraction loop. name_decorator_temps is a no-op on the bundle path (decorator_temp_refs is only populated when !will_use_renamer()), and its call site in _parse is after all append_part calls so the symbol table is complete when it iterates self.symbols. The test coverage is thorough (statement/expression mode, sibling classes, sibling blocks, minified, cross-file, non-identifier keys, late globals) and each new test asserts exact stdout.
…eiver temp Move the hoisting-scope walk plus declared_symbols registration onto P (var_hoisting_scope / declare_generated_binding) and use it from both the using lowering, which had its own copy, and the decorator lowering. Route the _obj temporary that captures a private call receiver through new_temp as well; it was still printed under a fixed name in bundles. The remaining fixed-name symbols take a 'static name so temporaries cannot bypass new_temp. Tests: method parameter shadowing a lowered member's storage, the receiver temporary, the nested class expression from #31929, the cross-file shapes from #30568, and decorated members in the accessor-only fixtures.
There was a problem hiding this comment.
I reviewed this PR and the automated bug-hunting pass found no issues. Because it reworks how the decorator lowering registers and names its generated bindings (new post-visit renaming pass, new declared_symbols/hoisting-scope registration shared with the using lowering, and a change to how the _obj receiver temp is declared), a human familiar with the parser/renamer architecture should still take a look.
What was reviewed:
new_temp/declare_generated_binding/var_hoisting_scope: registration in the hoisting scope +declared_symbolsmatches whatNumberRenamer/MinifyRenamerconsume; theusing-lowering refactor to use the same helper preserves the previous behavior.name_decorator_temps: reserving every symbol'soriginal_name(not just the hoisting scope's) is deliberate — nested-scope shadowing is covered by a test; the two-pass clear-then-reserve avoids self-collision.accessor_storage_base: checked thatis_identifiergating rules out thevar _x ycase and that non-UTF-8 / computed keys fall back to_accessor_storage.- The
_objreceiver temp switch fromgenerate_temp_reftonew_temp+ manualtemp_refs_to_declare.push— verifieddrain_capture_temp_declsstill declares it and it now participates in the collision-avoidance pass.
Extended reasoning...
Overview
This PR fixes name collisions in the standard-decorator / accessor lowering by (1) registering every lowering temporary in the enclosing var-hoisting scope and Part.declared_symbols so the bundler's renamers see them, and (2) adding a post-visit pass (name_decorator_temps) that assigns collision-free names when no renamer will run. It also extracts var_hoisting_scope / declare_generated_binding from LowerUsingDeclarationsContext into shared helpers, drops the ad-hoc per-class _dec2/_computedKey2/_accessor_storage0 counters, gates accessor-storage base names on is_identifier, and reroutes the _obj receiver-capture temp through the new mechanism. ~200 lines of Rust logic change plus ~370 lines of new tests across the runtime-transpiler and bundler suites.
Security risks
None. This is code-generation / symbol-naming logic in the parser; no untrusted-input parsing surface is added, no filesystem/network/crypto paths are touched.
Level of scrutiny
High. The decorator lowering is production code-generation on a hot path (_parse), and the fix threads through three interacting mechanisms (scope registration, declared_symbols, post-visit renaming) whose invariants ("top-level names come only from declared_symbols", "nested names come from scope.generated", "no renamer → original_name prints as-is") are documented but subtle. The using-lowering refactor is behavior-preserving but shares the new helper, so a regression there would affect an unrelated feature. This is well beyond a mechanical/config change and warrants a maintainer who knows the renamer contracts.
Other factors
- The comment-cop bot left 11 unresolved inline flags on the new doc comments. They read as genuine function documentation rather than workaround justifications, but they are outstanding automated feedback the author may want to address or dismiss.
- The last CI status comment (build #96808) reports failures on commit b04edc6; a follow-up commit a27d128 was pushed but no updated CI status is visible in the timeline.
- Test coverage is thorough (12 runtime-transpiler tests + 7 bundler tests covering user-name collisions, intra-class collisions, cross-file, sibling blocks ± minify, nested-scope shadowing, non-identifier keys, and the receiver-temp case), and the description states the existing decorator/transpiler/esbuild-port suites pass.
- No prior claude[bot] review on this PR.
|
Follow-up after a self-review of the first revision (a27d128, fd7e30e):
|
There was a problem hiding this comment.
I reviewed this and the automated bug hunt found nothing. Given the scope — a new post-visit naming pass, changes to how generated symbols land in scope.generated / Part.declared_symbols, and a refactor of the using lowering to share declare_generated_binding — a maintainer look would still be worthwhile, especially since this supersedes #31930 on approach.
What was reviewed:
new_temp/name_decorator_tempsflow: temporaries append top.declared_symbolsduring the visit insideappend_part(which clears at entry and collects at exit), so the part picks them up.- Removal of the Phase-2
expr_var_decls.pushfor_dec/_computedKey— Phase 8 already extracts the bindings frompre_eval_stmts, so the old code was declaring them twice. accessor_storage_basenow gates onis_identifier, soaccessor "x y"no longer producesvar _x y.- The
_objreceiver temp now goes throughnew_temp+temp_refs_to_declare, anddrain_capture_temp_declsstill declares it in the right body.
Extended reasoning...
Overview
Fixes standard-decorator/accessor lowering temporaries (_init, _dec, _<name> WeakMaps, etc.) colliding with each other and with user identifiers under both the no-renamer path (bun run, --no-bundle) and the bundler renamers. Touches src/js_parser/lower/lower_decorators.rs (replaces new_sym with new_temp for ~15 call sites, adds name_decorator_temps, accessor_storage_base, drops per-class counters), src/js_parser/p.rs (new decorator_temp_refs field, var_hoisting_scope, declare_generated_binding; refactors LowerUsingDeclarationsContext to use the new helper), and parse_entry.rs (one call after the visit pass). Adds 12 runtime tests and 7 bundler tests.
Security risks
None. Pure transpiler-output change; no I/O, network, auth, or user-input parsing surface added.
Level of scrutiny
High. Symbol naming and declared_symbols registration feed both bundler renamers and the printer directly; a mistake here miscompiles every file that uses a decorator or accessor. The change also touches the using lowering (behavior-preserving refactor to share declare_generated_binding), so two independent lowerings are affected. The design choice — a post-visit renaming pass over collected refs, rather than generate_temp_ref-style eager naming — is well-argued in the description (late-referenced globals, readable stack-trace names) but is exactly the kind of architectural decision a maintainer should sign off on, particularly since it supersedes the competing approach in #31930.
Other factors
- CI: the last robobun status is ❌ on b04edc6, two commits behind head. No green status is visible for fd7e30e.
- comment-cop has 7 open flags on the current head (doc comments on
new_temp,name_decorator_temps,declare_generated_binding, etc.). These read as necessary API documentation rather than workaround justifications, but a maintainer should confirm. - Test coverage is thorough: statement vs. expression classes, user-declared collisions, late globals, nested-scope shadowing, sibling classes, sibling blocks with and without
--minify-identifiers, cross-file bundling, non-identifier accessor keys, and the #31929 nested-static-initializer case. The description states all fail on the unfixed build. - Spot-checked: the removed Phase-2
expr_var_declspushes were genuinely duplicated by Phase 8'sSLocalwalk overpre_eval_stmts;append_partclearsdeclared_symbolsbefore visiting and collects after, sonew_temp's appends land in the right part;new_fixed_name_symcorrectly retains the three symbols whose original name is semantically meaningful (class.name, setter param,arguments).
…8316 and #29837 Adds the shapes from #31930 that were not covered yet: decorated field initializers in a base class and a subclass (#28010), two decorated classes in one bundled file (#28316), accessor storage in a base class and a subclass (#29837), the _m / _m_fn temporaries of decorated private methods in two classes, and a class statement named init, whose _init binding shares its base name with another class's initializer array.
|
Cross-checked this branch against the other open fixes for the same decorator lowering bugs (debug build of fd7e30e):
Pushed ec51ea7 with the test shapes from #31930 that were not covered yet: the #28010 field initializers and the #29837 accessor storage in a base class and a subclass, the |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It's a substantial change to the parser's symbol registration and adds a new post-visit naming pass, so a human look at the renamer/scope interactions would still be worthwhile.
Checked: name_decorator_temps clears the temps' own names before scanning p.symbols so they don't reserve themselves, and the suffix search skips existing _x2-style user names correctly. Phase 8 already pushes pre_eval_stmts bindings into expr_var_decls, so dropping the inline pushes for dec_ref/key_ref removes a duplicate rather than losing a declaration. The using refactor is behavior-equivalent (the dropped ensure_unused_capacity is replaced by per-item append().expect("oom")). accessor_storage_base now gates on is_identifier, closing the var _x y hole.
Extended reasoning...
Overview
This PR fixes standard-decorator lowering temporaries (_init, _dec, _<name> WeakMaps, _obj, etc.) that previously had fixed names and collided with user identifiers and with each other, both under bun run (no renamer) and bun build (renamer didn't see them). It touches lower_decorators.rs (~150 changed lines), adds two helpers on P (var_hoisting_scope, declare_generated_binding) that the using lowering now shares, adds a decorator_temp_refs field, and inserts a post-visit name_decorator_temps() call in parse_entry.rs. 20 new tests across the transpiler and bundler suites cover user-name collisions, intra-class collisions, cross-class/cross-file collisions, minified sibling blocks, and non-identifier accessor keys.
Security risks
None. This is code-generation naming; the inputs are already-parsed AST symbols and the output is identifier strings. No untrusted-size arithmetic, no filesystem/network paths.
Level of scrutiny
High. lower_decorators.rs runs on every file that uses standard decorators or accessor, and the change adds a new global naming pass that runs after the visit and mutates symbol.original_name for a subset of symbols. It also changes where generated bindings are registered (hoisting scope + declared_symbols instead of current_scope.generated alone), which interacts with NumberRenamer, MinifyRenamer, and per-part declared_symbols collection during tree-shaking. The using lowering refactor is behavior-preserving but touches working code. These are exactly the interactions the PR description spends most of its length justifying, and a maintainer who knows renameSymbolsInChunk should confirm the is_top_level / scope.generated registration is right for every renamer.
Other factors
The description is unusually thorough (mechanism, why the superseded #31930 was insufficient, which tests fail on which build), the comment-cop bot flags were addressed in fd7e30e, and the test matrix follows REVIEW.md's variant guidance (statement/expression, sibling blocks ± minify, cross-file, nested-in-static-initializer, non-identifier keys). No human reviewer has looked at it yet; CI build #97649 is in progress.
Supersedes #31930.
Fixes #31929
Fixes #28010
Fixes #28316
Fixes #29837
Problem
accessorlowering (src/js_parser/lower/lower_decorators.rs) declares temporaries next to the class:_init,_dec,_base,_class,_computedKey,let _<Class>, one_<name>WeakMap / WeakSet /_<name>_fnper lowered member, and_objfor a captured private call receiver. They were created with fixed names, so two different symbols routinely printed as the same identifier.bun run/bun build --no-bundleprint symbols under their original names (no renamer), so the temporaries collided with the file's own identifiers and with each other:const _init = 1; class A { @dec m() {} }fails to load:SyntaxError: Cannot declare a var variable that shadows a let/const/class variable: '_init'.(vardeclarations are silently rebound instead; a method parameter_valuenext to a#valuefield makes__privateSetreceive the parameter).class A { @dec accessor x = 1; #x = 2 }emitsvar _x = new WeakMaptwice and throwsTypeError: Cannot add the same private member more than once._init/_computedKey/_<name>, so a constructor that runs after the second class is evaluated reads the second class's data:class A { @dec [keyA] = 1 }produced{"b":1}, a base class and a subclass with a private field each threw, a class expression nested in another one's static initializer clobbered the outer chain (Decorator lowering temps (_class/_init/_dec) collide when a decorated class is nested inside another decorated class's static initializer #31929), etc. (TC39 Decorator Initializers Mismapped by Index in Subclasses #28010, Incorrect TypeScript transpilation with field decorators and two classes in the same file #28316, Class auto-accessor in subclass causes "Cannot add the same private member more than once" error #29837, and the closed duplicates Bug in downleveling of ES decorators #30326,addInitializerbug is mixing context for non-shared classes (Decorators) #30420, TC39 decorators: superclass @addInitializer never fires; last decorator's cb fires twice on subclass #31965).bun buildhas a renamer, but the temporaries only went intoscope.generated. The chunk renamer assigns top-level names fromPart.declared_symbols(src/bundler/linker_context/renameSymbolsInChunk.rs), so module-level temporaries were never renamed: the same collisions happened within a file and across the files of one bundle (Bundler Scope Hoisting Collision: Stage 3 Decorator accessor properties with identical names share the same WeakMap polyfill, causing "Cannot read from private field" #30568),_objshadowed a user's_obj, and a class expression inside a block registered itsvartemporaries in the block scope, so under--minify-identifierstwo sibling blocks received the same minified names.accessor "x y"emittedvar _x y = new WeakMap.Fix
P::declare_generated_binding(scope, ref)(src/js_parser/p.rs) registers a generated binding inscope.generatedand indeclared_symbolswith the matchingis_top_level;P::var_hoisting_scope()finds the scope an emittedvarbelongs to. Theusinglowering had exactly this code inline and now calls the helpers, so other lowerings that declare temporaries can share them.new_temp, which registers it with those helpers. That is all the renamers need:NumberRenamerrenames top-level symbols fromdeclared_symbolsand nested ones fromscope.generated; the minifier assigns nested slots per scope and top-level slots fromdeclared_symbols, and registering in the hoisting scope is what makes the sibling-block case distinct. The_objreceiver temporary goes throughnew_temptoo and is still declared bydrain_capture_temp_decls.!will_use_renamer(), the same splitgenerate_temp_refanddeclare_generated_symboluse), the temporaries are also collected andname_decorator_tempsnames them after the visit pass: the first request for a base keeps it, later ones getbase2,base3, ... (the renamer's convention), skipping every_-prefixed symbol in the file. It runs after the visit because references to undeclared globals only get a symbol when the visit reaches them, and it reserves the symbols of every scope, not just the hoisting scope's members, because the temporaries are read from nested code (constructors, generated accessors, methods) where a same-named parameter or local would shadow them.__bun_temp_ref_N$style names because lowered private methods are printed as_helper_fn = function () {...}, and that name is what shows up in stack traces. Converting the othergenerate_temp_refusers to this naming is left alone; it would change their output for no bug.arguments) take a'staticname throughnew_fixed_name_sym, so a temporary cannot bypassnew_temp; the inferred binding name of a lowered anonymous class expression is created inline and behaves as before._dec2/_computedKey2/_accessor_storage0counters are gone (both naming paths number the temporaries now),accessor_storage_baseonly derives_<key>from keys that are identifiers, and the expression-mode pre-eval temporaries are no longer pushed into the hoistedvartwice (var _class, _init, _dec, _dec). A file with one decorated class and none of these names in it keeps its current output.declared_symbolsregistration, but named them at lowering time and registered nested temporaries as top-level: built on top of main, the user-identifier, late-global, and minified sibling-block tests below fail on it, while all of its tests pass on this branch. Decorator lowering temps (_class/_init/_dec) collide when a decorated class is nested inside another decorated class's static initializer #31929's snippet as filed also references the outer class name from the relocated initializer, which is Substitute this and inner class name in relocated static initializers during decorator lowering #31922's bug; the naming half is what this PR fixes and tests.test/bundler/transpiler/es-decorators.test.ts, "lowering temporaries do not collide" (16 tests: file-declared names in statement and expression mode, globals referenced after the class, a method parameter shadowing a member's storage, the receiver temporary, two members of one class, two classes in one scope, computed keys read by constructors, a base class + subclass with private fields, the field initializers of TC39 Decorator Initializers Mismapped by Index in Subclasses #28010, the accessor storage of Class auto-accessor in subclass causes "Cannot add the same private member more than once" error #29837, decorated private methods of two classes, a class statement namedinit, class expressions in sibling blocks, the Decorator lowering temps (_class/_init/_dec) collide when a decorated class is nested inside another decorated class's static initializer #31929 nesting, non-identifier accessor keys). 15 fail on the unfixed build (the receiver-temporary case only fails in bundles); all pass with the fix.test/bundler/bundler_edgecase.test.ts,edgecase/DecoratorLowering*(9 tests: user names, one class, the two classes of Incorrect TypeScript transpilation with field decorators and two classes in the same file #28316 in one file,_initand accessor storage across files (Bundler Scope Hoisting Collision: Stage 3 Decorator accessor properties with identical names share the same WeakMap polyfill, causing "Cannot read from private field" #30568), the receiver temporary against a user's_obj, sibling blocks plain and--minify-identifiers, non-identifier key). All fail on the unfixed build, pass with the fix.usinglowering tests;cargo clippy -p bun_js_parserclean.bun runandbun build. Give decorator lowering temporaries file-unique names #31930's tests all pass on this branch; Lower accessor-only classes in place instead of relocating static elements #31926's do not (the Accessor-only class lowering relocates static initializers out of class scope (private name SyntaxError, wrong evaluation order) #31921 cases: private names and evaluation order in relocated static elements of accessor-only classes), so Lower accessor-only classes in place instead of relocating static elements #31926 stays open as a separate fix.Background
Symbolper binding; the printer asks a renamer for each symbol's name.bun buildusesNumberRenamer(appends numbers to resolve collisions) orMinifyRenamer; everything else usesNoOpRenamer, which printsSymbol.original_nameas is. That is why generated bindings on the non-bundle path have to carry collision-free names themselves (generate_temp_refdocuments this).scope.generatedis the list of parser-generated symbols belonging to a scope; renamers walk it for nested scopes.Part.declared_symbolslists the bindings each top-level part declares; it is the only source of names for the top level of an ESM file inNumberRenamer, and it also feeds the minifier's top-level slots.varemitted inside a block belongs to the enclosing function or module scope (Scope::kind_stops_hoisting), which is why the temporaries are registered there rather than incurrent_scope.__runInitializers(_init, ...)and assignthis[_computedKey], and lowered private members / accessors look their values up in the_<name>WeakMap. Any later class that reuses the name redirects those reads, which is why the symptoms range from syntax errors to silently wrong values.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bundler_edgecase.test.ts