Evaluate private method call receivers once in decorator lowering - #31426
Conversation
|
PR changed again? Review this PR in Change Stack to compare snapshots and stay oriented. Warning Review limit reached
More reviews will be available in 5 minutes and 54 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughCapture non-trivial private-call receivers into temporaries and hoist declarations into the correct scope; add compile-time and runtime tests verifying single evaluation and linear lowered output size. ChangesPrivate member call receiver safety
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found — the receiver-capture approach and temp-ref bookkeeping look correct and the tests cover the key paths well, but this is a transpiler-correctness change in a fairly intricate lowering pass, so it's worth a human pass.
Extended reasoning...
Overview
This PR fixes exponential output growth (and a receiver double-evaluation bug) in rewrite_private_accesses_in_expr within src/js_parser/lower/lower_decorators.rs. The lowering of recv.#m(args) → __privateGet(recv, _m).call(recv, args) previously inserted the same receiver AST node twice; the fix reuses this/identifier receivers via fresh nodes and captures any other receiver into a generate_temp_ref temporary (_obj = recv), with the temporaries declared in a var statement pushed to prefix_stmts just before Phase 8 output assembly. Four new tests in es-decorators.test.ts cover output-size linearity, single-evaluation semantics, chained optional calls, and the unchanged this/identifier fast paths.
Security risks
None. This is AST-lowering logic with no auth/crypto/permission/IO surface. The fix actually removes a fuzzer-found unbounded-memory DoS vector in Bun.Transpiler.
Level of scrutiny
Moderate-to-high. The transpiler is a production-critical correctness path — a regression here silently miscompiles user code. The change is small in line count but spans non-local state: a temp_refs_to_declare.len() snapshot is taken at the top of lower_standard_decorators and consumed/truncated ~1300 lines later, relying on the invariant that nothing else in this function pushes to temp_refs_to_declare (verified: line 772 is the only generate_temp_ref call in the file). Phase 8 correctly routes the new S::Local through expr_var_decls for the class-expression path.
Other factors
- Bug hunter found no issues.
- No CODEOWNERS for these paths.
use_refin theEIdentifierbranch records an extra usage for the second occurrence, which is semantically correct (the identifier really is referenced twice in the output).- The PR explicitly scopes out the pre-existing optional-chain short-circuit gap, which is reasonable.
- Test coverage is good and the description reports the broader decorator/transpiler suites pass, but given this is core transpiler output, a human reviewer familiar with the lowering phases should confirm the temp-ref scope/declaration placement is sound for all class forms (nested classes, class expressions inside decorated classes, etc.).
|
CI status for this PR — the two red builds are unrelated infra/test flake, not the diff:
The change itself is confined to the standard-decorator private-member lowering; the decorator/transpiler suites (including the new regression tests here) pass on every lane that executed them. I've used my one CI retrigger, so leaving it here — ready for maintainer review. |
|
Pushed regression tests for a second fuzzer-minimized variant of the same root cause: a |
e4dcfb8 to
36da04c
Compare
|
Rebased onto current main ( The Re-verified after rebase: all 41 tests in |
|
CI status after the rebase (build 59704, sha 36da04c, final):
Every red lane across this PR's builds has been a distinct unrelated infra issue (expired macOS agent → git SIGKILL in an install fixture → stale size baseline → gitlab.com outage). The diff itself is green everywhere it's been exercised. Ready for maintainer review/merge. |
When a class is lowered for standard decorators, `recv.#m(args)` was rewritten to `__privateGet(recv, _m).call(recv, args)`, duplicating the receiver expression. Side effects in the receiver ran twice, and for chained calls like `o.#m().#m().#m()` the duplication compounded, so printed output grew exponentially with chain length (a 44-link chain from the fuzzer made the printer allocate without bound). Reuse `this` and identifier receivers directly; capture any other receiver in a temporary (`__privateGet(_obj = recv, _m).call(_obj, args)`) declared alongside the other lowering variables.
…ic fields A fuzzer-minimized variant of the receiver-duplication blowup reaches the multi-GB range with only ~30 links: a `.#method()()` chain in a decorated static field initializer (ts loader). Cover it with a transpiled-size test (20 links must stay in the kilobytes) and a runtime test asserting each link's private method is evaluated exactly once.
36da04c to
3802549
Compare
|
Pushed 3802549 on top of a rebase onto current main (08226e2):
Re-verified on this branch: all 42 tests in Also filed #31910 for the pre-existing optional-chain short-circuit gap this PR's description mentions, and added repros to #28118 for the pre-existing missed rewrites in field initializers. Both predate and are untouched by this change. |
![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 `@src/js_parser/lower/lower_decorators.rs`:
- Around line 759-780: The code currently hoists a mutable temp (tmp_ref from
self.generate_temp_ref) into the outer lowering prelude via assign_to/use_ref so
every invocation shares one binding; change the non-identifier/this branch so
the temporary is allocated and consumed per-invocation (not a hoisted outer
binding): instead emit an inline/single-use temp expression for obj_expr (use a
freshly generated tmp_ref but make assign_to produce an immediate assignment
expression (or an expression-sequence) that scopes the temp to this call) and
pass that immediate write as get_obj and the corresponding read expression to
this_arg before calling self.private_get_expr; keep references to obj_expr,
tmp_ref, assign_to, use_ref, generate_temp_ref and private_get_expr to locate
and update the logic.
🪄 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: dbffbdec-6be5-4a84-8794-3a79c3a8897b
📒 Files selected for processing (2)
src/js_parser/lower/lower_decorators.rstest/bundler/transpiler/es-decorators.test.ts
A temp hoisted to class-statement scope is one shared binding across all invocations of a method. For getter/accessor-backed private calls, __privateGet runs the user getter between the temp write and the .call read, so a getter reentering the same call site overwrote the outer invocation's receiver. Declare temps created inside function/arrow bodies at the top of that body instead, matching where esbuild places them; sites outside function bodies run at most once per class evaluation and keep the hoisted declaration.
…ializer temps Deduplicate the two copies of the drain-temps-into-var-declaration block into drain_capture_temp_decls, used by both the function-body and the class-prelude placement. Extend the class expression test with a decorated instance field whose initializer has a complex receiver, so the hoisted-to-nearest-statement-list placement is exercised at runtime, and reword its comment to describe both placements.
Instance field initializers run per construction, not once per class evaluation; they share the hoisted binding the way esbuild's lowering does. Say that instead of over-claiming once-per-class-evaluation.
|
CI status (build 60965, sha 5025eae, final): 267 jobs passed. Every failing lane (debian/ubuntu/alpine/windows, x64/aarch64/baseline/asan) fails on exactly one file, All review feedback is addressed and every review thread is resolved. Ready for maintainer review; the bunx lanes clear once #31820 (or a rebase past it) lands. |
2 participants
What does this PR do?
Fixes unbounded memory growth in the transpiler (found by fuzzing) when a decorated class contains chained private method calls, plus a receiver double-evaluation bug in the same lowering.
Repro (fuzzer input, ~555 bytes — 5+ GB RSS and climbing before this change, 5 KB output in ~10 ms after):
Cause
rewrite_private_accesses_in_exprinsrc/js_parser/lower/lower_decorators.rslowersrecv.#m(args)to__privateGet(recv, _m).call(recv, args), inserting the same receiver expression twice (the two copies share AST nodes). Two consequences:.#m()call, each chain link doubles what the printer has to emit —?.Foo.#m()×44 is ~2^44 worth of text, so the printer allocates until it OOMs. Measured with the old lowering: a 16-link chain prints 2.9 MB, 18 links 11.8 MB, 20 links 47 MB (×4 per 2 links).getCounter().#m(42)calledgetCounter()two times.Fix
Evaluate the receiver exactly once:
thisand identifier receivers are reused directly (output unchanged for the commonthis.#m()/obj.#m()cases).__privateGet(_obj = recv, _m).call(_obj, args).Temporaries created inside method/function/arrow bodies are declared at the top of that body (
run(id) { var _obj; return __privateGet(_obj = make(id), ...).call(_obj); }), so each invocation gets a fresh binding; a binding shared across invocations could be clobbered when a getter-backed private call reenters the same site, since__privateGet(obj, member, getter)runs the user getter between the temp write and the.call(_obj)read. Temporaries created outside function bodies (field initializers, static blocks, decorate expressions, which run at most once per class evaluation) are declared alongside the other lowering variables (_dec, WeakMaps,_init), covering both class statements and class expressions. Both placements match where esbuild declares the corresponding_atemps.Output for the fuzz chain is now linear in chain length (n=44 → ~5 KB).
Note: optional-chain short-circuiting across a lowered private access (
o?.Foo.#m()whenois nullish) still throws like it did before this change; that is a separate, pre-existing gap in the decorator lowering, tracked in #31910, and is not affected by this PR.Verification
New tests in
test/bundler/transpiler/es-decorators.test.ts("private member calls in lowered classes"):?.Foo.#m()chain stays under 50 KB and reparses (old lowering: 47,186,639 bytes → fails).#method()()chains in decorated static field initializers stay linear (old lowering: ~64 MB for 20 links → fails)this/ identifier receivers keep working, decorated class expressions evaluate receivers oncebun bd test test/bundler/transpiler/es-decorators.test.ts— 43 pass with the fix; the size/evaluation tests above fail without it (USE_SYSTEM_BUN=1). Also ranes-decorators-esbuild.test.ts,decorators.test.ts,decorator-metadata.test.ts,bundler_decorator_metadata.test.ts, andtranspiler.test.js— no regressions (400+ tests), plus a minifiedBun.buildbundle to confirm the renamer keeps function-scoped temps collision-free.