Preserve optional chain short-circuiting through lowered private members in decorated classes#31914
Preserve optional chain short-circuiting through lowered private members in decorated classes#31914robobun wants to merge 3 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 5 minutes and 44 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 (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
CI note: the failing lane is #31820 pins the test to @angular/cli@21.1.3 and will clear this for the whole fleet. The decorator lowering change here is unrelated to that lane; all decorator and transpiler suites pass locally (es-decorators 44, es-decorators-esbuild 147, decorators 22, decorator-metadata 5, transpiler 188). |
There was a problem hiding this comment.
All three issues I flagged earlier are addressed and covered by tests, and this pass found nothing new — but ~400 lines of new optional-chain flattening in the decorator lowerer (this-binding, evaluate-once, super, delete) is subtle enough that a human should give it a once-over before merge.
Extended reasoning...
Overview
This PR adds optional-chain flattening to rewrite_private_accesses_in_expr in src/js_parser/lower/lower_decorators.rs so that ?. short-circuiting survives when a private member access in a decorated class is rewritten into __privateGet/__privateMethod helper calls. It introduces ~400 lines of new AST transformation: segment detection (optional_chain_needs_private_lowering), a flattener (lower_optional_chain_with_private) that rebuilds links bottom-up over a hoisted == null ? void 0 : ... test, callee handling for ?.() (rewrite_optional_callee) with receiver capture, a .call(recv) retargeter, temp-ref capture/declaration plumbing, and special cases for super callees and delete. Eleven new runtime tests in es-decorators.test.ts cover nullish/non-nullish paths, this preservation, evaluate-once, multi-segment chains, class expressions, instance methods/getters, super, the deeper-private-in-callee shape, and delete.
Prior review rounds
I raised three issues on earlier revisions, all confirmed and fixed with regression tests: (1) super.m?.().#priv emitting (_chain = super) — a SyntaxError; (2) o?.#f.b?.() losing the this binding when the lowered private is deeper in the callee chain; (3) delete o?.#f.x becoming a silent no-op because delete of a ternary value isn't a Reference. The current diff handles each of these in the way suggested, and the new tests assert the corrected behavior. This run's bug hunt found nothing further.
Security risks
None. This is a pure JS-to-JS lowering correctness fix in the transpiler; no auth, I/O, permissions, or untrusted-input parsing surface is touched.
Level of scrutiny
High. This is correctness-critical transpiler output: a mistake here silently changes the runtime semantics of user code (wrong this, double-evaluation, dropped delete, lost short-circuit) rather than failing loudly. The transformation interacts with several spec subtleties — Reference vs Value semantics, MakeSuperPropertyReference, OptionalChain evaluation order, strict-mode delete — and the fact that three real edge-case regressions surfaced across two review rounds confirms the surface is subtle. The test coverage is solid (11 targeted runtime tests plus a 37-case Node differential per the description), but the combinatorics of chain shapes are large enough that a maintainer familiar with lower_decorators.rs and the esbuild reference implementation should sanity-check the overall approach.
Other factors
The one CI failure (bunx.test.ts / Angular CLI Node-version gate) is unrelated and tracked separately. No CODEOWNERS entry covers this path. All prior inline threads are resolved.
When standard decorator lowering rewrites private member accesses into __privateGet/__privateMethod helper calls, the ?. tests on EDot/EIndex/ECall nodes in the chain were discarded, so a nullish base reached the helper and threw instead of short-circuiting to undefined. Hoist the chain's nullish tests out before rewriting: o?.a.#m() now lowers to o == null ? void 0 : __privateGet(_a = o.a, _m).call(_a), with receivers, callees, and effectful bases captured in temporaries that are declared alongside the other lowering variables. Fixes #31910
Bare super cannot be captured in a temporary; super.m?.() now tests the callee value and calls it with the current this.
An optional call whose callee chain contains a lowered private deeper in (o?.#f.b?.()) now routes through the callee rewrite so the receiver is captured for .call() instead of leaving a native ?.() on a ternary value. delete of a chain containing a lowered private pushes the delete into the non-null branch (o == null ? true : delete __privateGet(o, _f).x) so the property reference survives; delete of a ternary value is a silent no-op.
robobun
force-pushed
the
farm/c0a9217a/fix-decorator-optional-chain
branch
from
1bc7036 to
1d0b18d
Compare
There was a problem hiding this comment.
All three issues I flagged earlier are addressed and covered by tests, and I didn't find anything further — but ~380 lines of new optional-chain flattening logic in the decorator lowering pass is intricate enough that a human should sign off.
Extended reasoning...
Overview
This PR adds roughly 380 lines of new Rust to src/js_parser/lower/lower_decorators.rs, introducing five new helper functions (capture_expr_for_reuse, lowered_private_index_info, optional_chain_needs_private_lowering, retarget_call_through_dot_call, rewrite_optional_callee, lower_optional_chain_with_private) and a pre-pass in rewrite_private_accesses_in_expr that detects optional-chain segments containing lowered private accesses and flattens them into base == null ? void 0 : ... form so the ?. short-circuit survives the __privateGet/__privateMethod rewrite. It also adds ~220 lines of runtime tests in es-decorators.test.ts (11 new test cases) plus a small refactor folding the existing receiver-capture match into the new capture_expr_for_reuse helper.
Security risks
None. This is a transpiler AST transformation; it doesn't touch auth, crypto, network, filesystem, or permission boundaries. The only blast radius is correctness of emitted JavaScript for decorated classes.
Level of scrutiny
High. This is core transpiler output-correctness code with many interacting edge cases: this-binding preservation across .call() rewrites, super property callees (which can't be captured in a temp), delete of a flattened chain (must keep a Reference on the non-null branch), multi-segment chains where each ?. keeps its own test, optional calls whose callee is itself a chain with a private deeper in, evaluate-once semantics for effectful bases and computed keys, and temp-variable declaration placement (per-function-body vs. class-level). During review I found three separate regressions in earlier revisions (_chain = super SyntaxError, lost this for o?.#f.b?.(), and delete becoming a no-op), all of which the author confirmed and fixed with added tests — but the fact that three real bugs surfaced in successive passes is itself a signal that this transformation is subtle.
Other factors
- All three of my prior inline comments are resolved, the fixes match the suggested shapes, and each has a dedicated runtime test in the new suite.
- The bug-hunting system found nothing on the current revision.
- Test coverage is good: 11 new runtime tests exercise nullish/non-nullish paths,
thiscapture, evaluate-once, multi-segment chains,super,delete, class expressions, and instance methods/getters; the PR description also references a 37-case Node differential fixture. - The one CI failure (
bunx.test.tsAngular CLI Node-version check) is unrelated and tracked separately. - The change is not mechanical — it's a hand-written lowering algorithm with mutual recursion between
lower_optional_chain_with_private,rewrite_optional_callee, andrewrite_private_accesses_in_expr. Given the size and the demonstrated edge-case density, I'd rather a human familiar with the parser/lowering code give it a final look than auto-approve.
1 participant
What does this PR do?
Fixes #31910. Optional chains that pass through a lowered private member access in a decorated class lose their short-circuiting: the
?.nullish check is dropped by the rewrite, so the__privateGet/__privateMethodhelper is called withundefinedand throwsTypeError: Cannot read from private fieldinstead of the chain evaluating toundefined.Cause
rewrite_private_accesses_in_exprinsrc/js_parser/lower/lower_decorators.rsrewritesEIndex/ECallnodes with private identifiers into helper calls without looking at theoptional_chainflags, emittingwhere
o?.Fooevaluates toundefinedwithout short-circuiting the enclosing call.Fix
Before rewriting, detect optional-chain segments whose spine contains a lowered private access (or whose
?.()callee ends in one) and flatten the segment, hoisting the nullish test out, the way esbuild's decorator lowering does:varstatement emitted with the other lowering variables (WeakMaps,_dec,_init), covering both class statements and class expressions.callee?.()segments preserve thethisbinding of the callee's member access via.call(recv), including across nested segments (o?.a.#m?.()calls#mwiththis === o.a; esbuild dropsthishere, Node keeps it, this PR matches Node).?.syntax.deleteof a chain containing a lowered private pushes thedeleteinto the non-null branch (o == null ? true : delete __privateGet(o, _f).x) so the property reference survives.super.m?.()/super[k]?.()callees stay intact (baresupercannot be captured in a temporary) and call with the currentthis.?.in a multi-segment chain keeps its own test; inner segments are lowered recursively only when needed.The lowered output matches esbuild's decorator lowering shape for the common forms (
o?.#f,o?.a.#f,o?.a.#m(),o.#m?.(),g()?.a.#f,o?.a?.b.#f, ...).Verification
New tests in
test/bundler/transpiler/es-decorators.test.ts("optional chains through lowered private members"): nullish short-circuit for field gets / method calls / direct accesses, non-null paths with exact values andthis, optional calls of private members, multi-segment chains, evaluate-once + laziness of bases and computed keys,?.()at segment start with a private above it, decorated class expressions, and private instance methods/getters.bun bd test test/bundler/transpiler/es-decorators.test.ts: 43 pass with the fix; 7 of the 8 new tests fail withUSE_SYSTEM_BUN=1(the remaining one guards non-null path values, which already worked). Also ranes-decorators-esbuild.test.ts(147),decorators.test.ts(22),decorator-metadata.test.ts(5),bundler_decorator_metadata.test.ts(2), andtranspiler.test.js(188): no regressions.A 37-case differential fixture (every chain form above, nullish at each position plus hit paths, with side-effect counters) produces byte-identical output between Node (undecorated, native semantics) and this branch's lowering; 18 of the 37 cases throw
TypeErroron the unfixed build.Rebased on main after #31426 (evaluate private method call receivers once) merged; it touches the same function. Resolution: kept main's temp-declaration machinery (
drain_capture_temp_declsand the per-function-body declarations, which also make the temps reentrancy-safe) and dropped this PR's duplicate class-level drain block, and the receiver capture in the non-chainECallarm now goes through this PR'scapture_expr_for_reusehelper instead of an identical inline match. Both PRs' test suites pass together (54 tests ines-decorators.test.ts).