Repository navigation
Conversation
|
Warning Review limit reached
Next review available in: 4 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 (7)
Comment |
|
Status: ready for review. Reproduced on bun 1.4.0 with the class in the PR description ( Since opening: CI: the new tests and every suite touched by this change pass on all lanes in the last two builds (97028, 97301). The remaining red entries are pre-existing intermittent failures unrelated to this diff and already reported separately ( |
…ator lowering Standard decorator lowering moves private methods, getters and setters out of the class body into plain function expressions once the class has a decorated member. A super property access in one of those bodies was copied verbatim, and super is a syntax error outside a class body, so the whole module failed to load. The lowering now rewrites those accesses before extracting the body: reads become __superGet(home, this, key), calls keep their receiver through .call(this) (optional calls through ?.call), plain assignments become __superSet, and every other assignable use (compound and logical assignment, ++/--, destructuring targets, for-in/of heads) goes through the accessor returned by __superWrapper so the operator keeps its native semantics. A tagged template binds the receiver. The walk stops at non-arrow functions and at the members of nested classes, which have their own super. home is the original class, captured into a temporary by a static block emitted as the first element of the class body (or its prototype for instance members), so the lookup starts at the method's [[HomeObject]] even when a class decorator later replaces the class, and it is available to static initializers that call the method while the class is still being defined. The temporary and the static block are only emitted when a lowered body actually uses super. The three helpers are added to runtime.js and to the bun:wrap import table.
4703d3c to
f2ebbb2
Compare
|
Updated 11:05 PM PT - Aug 14th, 2026
❌ @robobun, your commit 375ad3e has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38769That installs a local version of the PR into your bun-38769 --bun |
A chunk's isolated hash covers the part ranges of every file in the chunk, including the runtime module, so the statements added to runtime.js move the hashed output names, etags and debug ids these tests pin even though the emitted code is unchanged.
delete of a super reference throws a ReferenceError after evaluating the key and without reading the property. Routing it through __superGet ran the inherited getter and returned true instead. The access-form test now covers it, and the chunk hashes pinned by the bundler tests move again with the added runtime statement.
There was a problem hiding this comment.
I reviewed this PR and the earlier delete super.x finding has been addressed; the follow-up run found no further bugs. Given the size of the new lowering pass (~400 lines of AST rewriting covering every super access form, plus new traversal positions that also affect the existing ReplaceThis/ReplaceRef walkers) and its overlap with #38730/#31930/#31922/#38731, a human look is still worthwhile.
Checked and ruled out: object-literal methods and nested class members correctly keep their own super (the EFunction/rewrite_class arms stop the walk; covered by the objectLiteral/nestedClass cases in the access-form test); the Imports::ALL_SORTED/ALL_SORTED_INDEX tables are consistent with the four appended helpers (guarded by the existing all_sorted_matches_zig_comptime test); the for-in/of head rewrite writes through the arena pointer the same way the sibling ECall/EBinary arms do.
Extended reasoning...
Overview
This PR fixes a SyntaxError: super is not valid in this context when standard (TC39) decorator lowering extracts private method/getter/setter bodies out of the class body. It adds a RewriteKind::LowerSuper pass to the existing relocation rewriter in src/js_parser/lower/lower_decorators.rs (~400 new lines), four new runtime helpers (__superGet/__superSet/__superWrapper/__superDelete) in src/runtime.js, registers them in src/ast/runtime.rs, adds 11 new tests (~350 lines) in es-decorators.test.ts, and updates three bundler tests whose pinned chunk hashes moved because runtime.js gained statements.
Security risks
None identified. This is transpiler output-shape work; the new runtime helpers are thin wrappers over Reflect.get/Reflect.set/Object.getPrototypeOf with no untrusted-input parsing, filesystem, network, or auth surface.
Level of scrutiny
High. The change is in a hot transpiler path (decorator lowering runs on every class with a standard decorator), implements spec-mandated semantics across many syntactic forms (value position, call, optional call, simple/compound/logical assignment, pre/post inc/dec, destructuring targets, spread/rest, for-in/of heads, tagged templates, delete), and extends the shared rewrite_expr/rewrite_stmts walker with new traversal positions (rewrite_args, rewrite_binding, rewrite_class, for-head, catch binding) that also apply to the pre-existing ReplaceThis/ReplaceRef kinds. The PR description itself calls out non-obvious design choices (capturing the class via static { _home = this } rather than using the class binding or the _base temporary, following the prototype at access time, ordering the capture block first) and their interaction with class-decorator replacement and TDZ — exactly the kind of decisions a maintainer should sign off on.
Other factors
- The one issue found in the earlier review pass (
delete super.xlowering todelete __superGet(...)) was fixed in 0b1496b with a__superDeletehelper and a Native/Lowered comparison test case; that thread is resolved. - The comment-cop bot's paragraph-length-comment complaints were addressed in a19bd59/58e857ae (all resolved).
- Test coverage is thorough: the "every access form behaves like the native class" test runs the same member set through a native and a lowered class and asserts identical JSON output plus a documented literal, which is a strong differential check. The emitted-shape test asserts
_homeis only introduced when needed and that in-body methods keep nativesuper. - Four open PRs (#38730, #38731, #31930, #31922) touch the same file; the description explains the intended interaction, but merge order and folding are a human call.
- The
runtime.rsindex tables (ALL,ALL_SORTED,ALL_SORTED_INDEX,field,field_mut) were all updated consistently and are guarded by the existing unit test.
This is well-executed and well-tested, but not the kind of simple/mechanical change the auto-approval bar is meant for.
There was a problem hiding this comment.
I re-reviewed after the delete super.x fix in 0b1496b and didn't find any further issues — the __superDelete helper and the added Native/Lowered deleteSuper case look right. Given the scope (~400 lines of new AST-rewriting logic in the transpiler, an extended traversal that also affects the existing ReplaceThis/ReplaceRef passes, and the stated overlap with #38730/#31930/#31922/#38731), a human look would still be worthwhile.
What was reviewed:
lower_super_in_exprhandling of every access form (get/set/call/optional call/compound and logical assignment/update/destructure target/for-in-of head/tagged template/delete) against the Native/Lowered comparison test.- Traversal boundaries: object-literal methods and nested-class members correctly stop the
LowerSuperwalk (theirEFunctionvalue /enter_members = falsegates it), whileextendsand computed keys are entered. _homecapture via the leadingstatic { _home = this }block, only emitted when a lowered body usessuper;Imports::ALL/ALL_SORTED/ALL_SORTED_INDEXstay consistent (the unit test inruntime.rscovers this).- The three pinned-hash test updates are the expected consequence of adding statements to
runtime.js.
Extended reasoning...
Overview
This PR fixes a SyntaxError when TC39 standard-decorator lowering extracts a private method (or getter/setter) that uses super.x out of its class body into a plain function expression. It adds a RewriteKind::LowerSuper pass to the existing relocation rewriter in src/js_parser/lower/lower_decorators.rs that rewrites every super access form to calls into four new bun:wrap helpers (__superGet/__superSet/__superWrapper/__superDelete), and captures the original class via a static { _home = this } block. The traversal itself is extended to walk into arrow/function args, binding defaults, computed keys, await/yield/import() operands, for-in/of heads, catch bindings, and nested-class extends/computed keys — extensions that also apply to the pre-existing ReplaceThis/ReplaceRef kinds. src/ast/runtime.rs and src/runtime.js register the four helpers. Eleven new tests in es-decorators.test.ts cover the repro, decorated/undecorated/static variants, no-extends, class-decorator replacement, per-evaluation classes, nested-class scoping, and a comprehensive Native-vs-Lowered access-form comparison. Three bundler tests that pin chunk hashes/etags/debugIds are updated because runtime.js gained statements.
Prior finding addressed
My earlier review flagged that delete super.x fell through to a plain __superGet read (running the inherited getter and returning true) instead of throwing ReferenceError. Commit 0b1496b adds a UnDelete arm that emits __superDelete(key), and the access-form test now asserts ["ReferenceError", 1, 0] (key evaluated once, getter never read) for both the native and lowered classes. Verified in the current diff.
Security risks
None identified. This is a compile-time AST transformation of user source; the runtime helpers are thin wrappers over Reflect.get/Reflect.set/Object.getPrototypeOf and a ReferenceError throw, matching native super semantics. No untrusted-input parsing, auth, crypto, or filesystem paths are involved.
Level of scrutiny
High. This is ~400 lines of new logic in the JavaScript transpiler — a code path every user's decorated class flows through — implementing spec-mandated [[HomeObject]] semantics across a large syntactic surface. The traversal extensions also change what the existing ReplaceThis/ReplaceRef passes visit (parameter defaults, destructuring defaults, computed keys, for-heads, catch bindings), which the PR description says was previously skipped; that widening is plausibly a fix but is a behavior change beyond the headline bug. Correctness here depends on precise AST-shape reasoning (e.g., that object-literal methods and nested-class members are represented as EFunction/gated by enter_members, so the walk stops at their boundary) that benefits from a maintainer familiar with the AST invariants.
Other factors
- The PR explicitly overlaps with four other open PRs on the same file (#38730 rewrites
superin relocated static blocks, #31930 renames lowering temporaries, #31922/#38731 touchlower_impl); merge order and foldingLowerSuperinto #38730's static-block sites is a coordination question for a human. - Test coverage is thorough: the Native-vs-Lowered comparison is a strong invariant test, and the shape-assertion test confirms
_homeis only emitted when needed and in-body methods keep nativesuper. The two CI failures reported by robobun (setInterval.test.js,html-rewriter-leak.test.tson x64-asan) are unrelated to this change. - The comment-cop bot flags are all resolved (comments were shortened in a19bd59/58e857ae).
Given the size, the spec subtlety, and the cross-PR coordination, I'm deferring rather than approving.
|
#38730 (the static block / static initializer sites) is now stacked on this branch instead of carrying its own rewrite: it calls this PR's |
|
Status against #40833, which rewrites the standard decorator lowering: since e7eeb9e that branch routes One case still differs from the native class there, and this PR handles it: Leaving this open for the |
|
Status against main after #40833 merged (a22b2aa). The merged revision differs from the one my previous status comment measured: it dropped the I ran this PR's test block against a debug build of main at 09bb546. The block is
Two behavioral tests fail on main:
So #40833 supersedes part of this PR. The remaining bug on main is |
Problem
super.xfails to load:SyntaxError: super is not valid in this context.Decorated private methods (@dec #m() { return super.x }) and static private methods fail the same way.lower_implinsrc/js_parser/lower/lower_decorators.rsmoves private method bodies out of the class body. Undecorated ones become_m_fn = function () { ... }emitted before the class (thelower_all_privatepath), decorated ones are passed to__decorateElementas a function argument. The function value was copied verbatim, andsuper.xis only valid inside a class body or object literal method, so the printed module is a syntax error (bun build --no-bundleshows_im_fn = function() { return super.greet(); }at module level).Fix
RewriteKind::LowerSuperpass to the relocation rewriter already used forthisin extracted static blocks, and runs it over the parameters and body of every function the two extraction sites take out of the class:super.x,super[k]in value position become__superGet(home, this, key)super.f(...)becomes__superGet(...).call(this, ...);super.f?.(...)keeps its short-circuit as?.call(this, ...)super.x = vbecomes__superSet(home, this, key, v)(evaluates tov)+=and the logical assignments,++/--, destructuring assignment targets,for (super.x of ...)heads) becomes__superWrapper(home, this, key)._, an object whose_accessor reads and writes through the two helpers. The operator itself is left in place, so it keeps its native semantics (ToNumeric, BigInt, return value, short-circuiting) and the key expression is evaluated exactly once without temporariessuper.tag`...`becomes__superGet(...).bind(this)`...`delete super.xbecomes__superDelete(key), which evaluates the key and then throws a ReferenceError without reading the property, as the native form doeshomeis a new temporary holding the class, bound bystatic { _home = this; }emitted as the first element of the class body, and used as_homefor static members and_home.prototypefor instance members. Both the temporary and the block are only emitted when a lowered body actually contains asuperaccess.super.xin a method meansReflect.get(HomeObject.[[GetPrototypeOf]](), "x", this), where HomeObject is the class for static members andC.prototypeotherwise;__superGet(home, obj, key)isReflect.get(Object.getPrototypeOf(home), key, obj), i.e. the same operation with the same receiver, so inherited getters, setters and methods see the rightthis, andsuper.x = vwith no inherited setter defines the property on the instance like native code does. These are the helpers esbuild uses for the same lowering.__superSetthrows whenReflect.setreports failure because class bodies are strict code, where the native assignment throws too.__superGet(C.prototype, ...)) or the_basetemporary: class decorators reassign the binding, and with a replacing decorator (return class extends cls {...}) a lookup through the replacement's prototype finds the original class's own methods instead of the parent's (esbuild 0.25 has this bug); the binding is also in its TDZ while the body's static initializers run.Object.getPrototypeOf(_home)instead follows the class's actual prototype chain at access time, also covers classes withoutextends(Object.prototype / Function.prototype) and laterObject.setPrototypeOfre-parenting without special cases, and the leading static block makes it available to static initializers that call a lowered static method while the class is still being defined._homeis declared the same way as the_m_fntemporaries that reference it, so it has exactly their lifetime.await/yield/import()operands and theextendsclause and computed keys of nested classes, and stops at non-arrow functions, object literal methods and the members of nested classes, which have their ownsuper. The traversal positions added to the shared walker (rewrite_args,rewrite_binding,rewrite_class, for-in/of heads, catch bindings) apply to the existingReplaceThis/ReplaceRefkinds too, which previously skipped them.__superGet,__superSet,__superWrapperand__superDeleteare added tosrc/runtime.jsand to thebun:wrapimport table insrc/ast/runtime.rs(appended, so existing indices are unchanged).html-import-manifest.test.ts,bundler_promiseall_deadcode.test.tsandcyclic-imports-async-bundler.test.jspin output hashes, etags and debug ids. A chunk's isolated hash includes the part ranges of every file in the chunk, the runtime module among them (LinkerContext::generate_isolated_hash), so the statements added to runtime.js move those values even though the emitted code is byte-for-byte unchanged (checked by diffing the client bundle of the manifest test against the released binary's). Earlier runtime.js additions needed the same updates (feat(transpiler): implement TC39 standard ES decorators lowering #26436, Reduce the number of closures in generated bundler code #27022).test/bundler/transpiler/es-decorators.test.ts, newsuper property access in lowered private methodsblock (11 tests): the repro, decorated private method/getter/setter/static method, noextendsclause, a class that runs the same members natively and lowered and asserts identical output for every access form above,deleteincluded (that program also prints the same thing under node with the decorator removed), tag receiver, class decorator replacing a declaration and an expression, a static initializer calling a lowered static method during class definition, per-evaluation classes in a function, a nested class keeping its ownsuper, and the emitted shape (capture present only when used, in-body methods untouched). All 11 fail on the released binary with the SyntaxError above and pass with this change.es-decorators-esbuild(esbuild's 147 conformance tests),decorators,decorator-metadata,bundler_decorator_metadata,ts-use-define-for-class-fields,bundler_edgecase,bundler_regressions,bundler_minify,bundler_cjs,esbuild/{ts,lower,default},transpiler.test.js,regression/issue/27575,test/internal/source-lints: pass.bun run,bun build,--minify,--target=bun,--format=cjsand--format=iife;cargo clippy -p bun_js_parser -p bun_astis clean.lower_implrelocates code out of the class body at five places. This PR adds the mechanism and applies it to the two that extract private method bodies. Lower super in static blocks and static initializers relocated by decorator lowering (stacked on #38769) #38730 is now stacked on this branch and applies the sameSuperLowering(followed by the existingReplaceThis) to the other three, relocated static blocks and static field/accessor initializers, with its own tests; it should land after this one. The equivalent site in the legacy experimentalDecorators lowering (p.rs, decorated static members moved after the class) has the same defect and is tracked separately. Give decorator lowering temporaries file-unique names #31930 gives lowering temporaries unique names;_homeis created like the temporaries it is used alongside and is covered by that change the same way. Substitute this and inner class name in relocated static initializers during decorator lowering #31922 and Let a decorated class body observe the class its decorators return #38731 touch the same function (textual overlap only).Background
__decorateElement(...)calls that apply the decorators. Private members cannot be reached by code emitted outside the class body, so once any member is decorated, every private member is lowered to WeakSet/WeakMap storage (lower_all_private), and private method bodies become module-level function expressions that the in-body code calls through__privateMethod(this, _m, _m_fn).call(this).C.prototypeotherwise).super.xlooksxup on that object's prototype, at access time, and uses the method's ownthisas the receiver for getters, setters and the call.Reflect.get/Reflect.setare the built-ins that take an explicit receiver, which is why the helpers are written in terms of them.rewrite_expr/rewrite_stmts, parameterized byRewriteKind) is the lowering's walker for code it moves somewherethisorsuperno longer means the same thing;ReplaceThisis what relocated static blocks already used.bun:wrapis the module the transpiler imports runtime helpers from (src/runtime.js, bundled into the runtime module forbun build);Imports::ALLinsrc/ast/runtime.rsis the list of namescall_runtimeis allowed to emit, so new helpers have to be registered there as well.Emitted code for the repro
Other forms, from the access-form test: