Repository navigation
Conversation
…ate` member Standard decorator lowering turns a `#private` name that has a decorated member into a WeakMap or WeakSet and rewrites each access. The template arm of `rewrite_private_accesses_in_expr` only recursed into the tag, so `this.#tag`x`` became `__privateMethod(this, _tag, _tag_fn)`x``. The tag was no longer a member access and the function ran with `this` undefined. The arm now rewrites such a tag to `__privateMethod(recv, ...).bind(recv)`. `.call` cannot be used here because the strings array of a tagged template is the one the engine keeps for the call site. The call arm and the template arm share one `capture_receiver` helper: `this` and an identifier are read twice, any other receiver goes through an `_obj` temporary so it is evaluated once. The lowered output changes, so the runtime transpiler cache version goes to 33.
|
Status: fix pushed, waiting for CI. How I reproduced it: the class below, run as function dec() { return (v, ctx) => {}; }
class A {
@dec() #tag(s, ...v) { return this?.n + ":" + s.raw.join("|") + v.join(","); }
@dec() static #stag(s) { return this === A; }
n = 7;
run() { return [this.#tag`a${1}b`, A.#stag`x`]; }
}
console.log(JSON.stringify(new A().run()));Test: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused lowering fix that mirrors the existing ECall arm, with the cache version bumped and thorough test coverage.
What was reviewed:
capture_receiveris a pure extraction of the previous inline block in theECallarm; that arm's output is unchanged.- The new
ETemplatepath emits.bind(recv)(not.call), which preserves per-call-site template-strings identity — the test asserts this directly (sameSiteSameStrings/otherSiteOtherStrings). - Non-matching tags still fall through to the recursive rewrite, and template parts are still walked;
receiverEvals: 9checks the temporary-capture path evaluates eachself()once per site. - Transpiler cache constant bumped to 33 as required for changed lowered output.
Extended reasoning...
Overview
The PR fixes ES-decorator lowering for tagged templates whose tag is a decorated #private member. Before, the ETemplate arm of rewrite_private_accesses_in_expr in src/js_parser/lower/lower_decorators.rs only recursed into the tag, producing a bare __privateGet(x, _m) value that lost its receiver, so the tag ran with this === undefined. The fix rewrites such tags to __privateGet(x, _m).bind(x) and factors the receiver-capture logic (identifier/this re-read; anything else through an _obj temporary) into a shared capture_receiver helper used by both the ECall and ETemplate arms. The runtime transpiler cache version in src/jsc/RuntimeTranspilerCache.rs is bumped 32→33, and test/bundler/transpiler/es-decorators.test.ts gains a privateTag section covering method/static/getter/field/accessor tags, chained and nested receivers, arrow bodies, a static {} block, single-evaluation of side-effecting receivers, and template-strings-array identity.
Security risks
None. This is AST-to-AST lowering of already-parsed source; no untrusted-length arithmetic, no filesystem/network I/O, no auth or crypto surface. The only user-observable effect is corrected this binding in emitted code.
Level of scrutiny
Moderate. Transpiler output correctness affects every user, so I read the surrounding ECall arm to confirm the extracted capture_receiver helper is byte-for-byte the previous inline logic (it is — the ECall arm's emitted code is unchanged). The new ETemplate path is a direct structural mirror of ECall with .bind in place of .call, which is the correct choice: .call(x, strings, ...) would destroy the engine's per-call-site frozen strings identity, whereas .bind keeps the tagged-template call form intact. The else branch preserves the prior recursive walk for non-private tags, and template parts are still visited afterward, so no existing rewrite is skipped. I hand-checked the test's expected receiverEvals: 9 (one static-block self() + four per run() × two runs) and the sites[] indexing for sameSiteSameStrings — both line up.
Other factors
REVIEW.md's specific requirements are all met: the duplicated receiver-capture block is extracted into a named helper and used at both sites; the transpiler cache format constant is bumped for changed cached output; the whole sibling class (all five private-member kinds, static and instance, plain/this/chained/nested receivers) is covered rather than just the repro; the test is added to the existing module file with a single structural toEqual on exact values derived from Node's undecorated output. No CODEOWNERS entry covers src/js_parser/. The PR description transparently documents the ordering interaction with two open PRs (cache version conflict, and the target_was_originally_property_access flag) and the forms deliberately left out — none of which affect the correctness of what's landed here. Exit reason was dry_streak with no findings.
|
The review found nothing to change, and no review threads are open. Buildkite build 115372 is still running. |
This branch has not been deployed
1 participant
Problem
#privatemember runs the function withthis === undefined.this.#tag`a${1}b`givesundefined:a|b1. Node gives7:a|b1without the decorator.ETemplatearm ofrewrite_private_accesses_in_expr(src/js_parser/lower/lower_decorators.rs:423). It only recursed into the tag, so the tag became the value__privateMethod(this, _tag, _tag_fn). That is not a member access, so it gets no receiver.Fix
__privateMethod(recv, _tag, _tag_fn).bind(recv), the shape esbuild emits..call(recv, strings)is not possible: the engine keeps one strings array per call site.ECallandETemplatearms share a newcapture_receiverhelper. A receiver other thanthisor an identifier goes through an_objtemporary, somake().#tag`x`evaluatesmake()once.test/bundler/transpiler/es-decorators.test.ts, newprivateTagsection (.js,.ts,bun build). The 3 new tests fail without the fix. Other suites: see Notes.Background
#privatename that has a decorated member with a WeakMap or WeakSet, so the runtime helpers can reach it. Eachx.#namebecomes a helper call such as__privateGet(x, _name).a.b`x`callsa.bwiththis = aonly when the tag is a member access. Its first argument is a frozen strings array, created once per call site.ECallarm already lowersx.#m(...)to__privateMethod(x, _m, _m_fn).call(x, ...).Notes
Repro (
bun tag.js). Before:["undefined:a|b1",false]. After, and node without the decorators:["7:a|b1",true].Lowered
run()after the change:The same path covers a decorated private method, static method, getter, field and auto-accessor that holds a function. The new test section checks each of them. It also checks an identifier receiver, a receiver with side effects (evaluated once), a receiver that is itself a lowered private read (
this.#me.#tag), a tagged template nested in a template part, an arrow function with an expression body, and a static block. It checks that one call site gets the same strings array on every evaluation. The expected values are what node prints for the same class without the decorators.Other probes against node, all equal with this change: a decorated static field initializer in a class expression, a chained template (
Foo.#curry`ab``), an async method, a generator, a parameter default, an arrow function inside a method, a computed object key, and a parenthesized optional chain (``(a?.#tag)p`` withaset and witha` null).capture_receiveris the receiver block of theECallarm, moved into a function. The output of theECallarm does not change. As before, the lowering reads an identifier receiver twice and does not capture it. A private getter that assigns to that identifier can see the difference. That is the same in both arms and this PR does not change it.Order against other open PRs:
#privatemember #42651 (assignment targets of a lowered#privatemember) also sets the cache version to 33. Its diff applies on top of this branch with one conflict, that line. The PR that lands second takes 34.thisundefined for call targets and template tags that a rewrite turned into a property access #40829 addstarget_was_originally_property_accesstoE::Call, and its printer prints(0, a.b)()for a property access target without the flag. The new.bindcall is anE::Callon a property access, like the.callin theECallarm that Keepthisundefined for call targets and template tags that a rewrite turned into a property access #40829 already marks. The PR that lands second must set the flag totrueon the.bindcall.Not in this PR:
new A.#make`j`()still throwsTypeError: function is not a constructor. The lowered tag contains a call, and the printer drops the parentheses that a call inside a template tag needs undernew. That printer bug is on main with and without this change (new (o.f().g`x`)()has it too). js_printer: keep the parentheses a new callee or template tag needs #41638 fixes it. With that printer change applied locally on top of this branch, the case printsnew (__privateMethod(A, _make, _make_fn).bind(A))`j`and matches node.import(this.#x)has no arm (EImport), so the private name stays in the output and the file fails to load. Optional chains through a lowered member are Optional chains through lowered private members do not short-circuit in decorated classes #31910._objof a tag in the same way as to the_objof a call.Suites run with the debug build:
test/bundler/transpiler/es-decorators.test.ts(421 pass),es-decorators-esbuild.test.ts(147 pass),decorators.test.ts(24 pass),decorator-metadata.test.ts(5 pass),test/cli/run/transpiler-cache.test.ts(20 pass),test/regression/issue/27526.test.ts,test/regression/issue/27575.test.ts.Self-reviewed: 3 changes requested, 3 made (the cache version, the shared helper, the notes above on order and on excluded forms).
[human-review] gate passed · iteration 0 · 3 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