Skip to content

fix(es/decorators): delay 2022 decorator initializers after private fields#11847

Merged
kdy1 merged 15 commits into
mainfrom
kdy1/fix-11846-decorator-init-order
May 23, 2026
Merged

fix(es/decorators): delay 2022 decorator initializers after private fields#11847
kdy1 merged 15 commits into
mainfrom
kdy1/fix-11846-decorator-init-order

Conversation

@kdy1
Copy link
Copy Markdown
Member

@kdy1 kdy1 commented May 10, 2026

Description:

Fixes a 2022-03 decorator initialization ordering bug where _initProto(this) could run before decorated private instance field storage was initialized. In that ordering, addInitializer callbacks using context.access.get(this) could throw TypeError: attempted to get private field on non-instance.

This tracks 2022-03 decorated instance field initializer ids and only injects _initProto into a safe later public field initializer. If decorated private storage has to be initialized first and no safe field exists, _initProto falls back to constructor injection so class field lowering runs it after instance storage setup.

Added an exec regression fixture for the issue case with a decorated public field followed by decorated private and public fields.

BREAKING CHANGE:

None.

Related issue (if exists):

Fixes #11846

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 10, 2026

🦋 Changeset detected

Latest commit: 24b3862

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Copy Markdown
Contributor

Binary Sizes

File Size
swc.linux-x64-gnu.node 27M (27778376 bytes)

Commit: 3a67b6a

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented May 10, 2026

Merging this PR will improve performance by 2.81%

⚡ 3 improved benchmarks
✅ 183 untouched benchmarks
⏩ 123 skipped benchmarks1

Performance Changes

Benchmark BASE HEAD Efficiency
es/large/all/es2016 114.4 ms 112.1 ms +2.09%
es/large/all/es2020 110.4 ms 108.1 ms +2.19%
es/large/parser 36.3 ms 34.8 ms +4.17%

Tip

Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.


Comparing kdy1/fix-11846-decorator-init-order (24b3862) with main (a3cfbd7)2

Open in CodSpeed

Footnotes

  1. 123 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

  2. No successful run was found on main (1e98e97) during the generation of this report, so a3cfbd7 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@kdy1 kdy1 marked this pull request as ready for review May 22, 2026 10:38
@kdy1 kdy1 requested a review from a team as a code owner May 22, 2026 10:38
kodiakhq[bot]
kodiakhq Bot previously approved these changes May 22, 2026
@kdy1 kdy1 changed the title fix(es): delay 2022 decorator initializers after private fields fix(es/decorators): delay 2022 decorator initializers after private fields May 22, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Code Review

Thanks for tackling this — the ordering bug in #11846 is genuinely tricky and the new policy is more principled than the old heuristic. A few observations:

Correctness — looks good

The shift from "private-prop followed by getter ⇒ safe" to "any decorated instance field init blocks injection until after the last decorated private init" is a real improvement. The previous heuristic at decorator_impl.rs:2391+ was guessing at intent based on neighboring shape; the new approach uses ground-truth from the decorated_instance_field_inits set populated during child visitation. The save/restore at decorator_impl.rs:2394 / :2547 correctly handles nested classes inside field initializers.

The asymmetry between is_unsafe_2022_private_field_init (private-only, blocks) and can_inject_2022_init_proto_into_field (public ClassProp only, also skips decorated values) is justified: a private brand check throws TypeError, while a public field read returns undefined. So decorated public fields are skipped (their own value isn't installed yet at injection time) but don't block later non-decorated fields from being a target.

Auto-accessor regression check

The old code intentionally allowed injection before private-prop-followed-by-getter (i.e., the desugared form of accessor x). The new code marks the desugared private storage as unsafe too — but the auto-accessor handler at decorator_impl.rs:2864-2868 correctly inserts the init id into decorated_instance_field_inits so the logic stays consistent. For a class with only @dec accessor x = 1, we now fall back to constructor injection, which class-properties lowers into a constructor where this.#_x = _init_x(...) precedes _initProto(this). That's actually more correct than the old behavior (which would have invoked addInitializer callbacks against an uninitialized private storage).

Concerns

  1. Constructor-fallback ordering depends on class-properties insertion semantics. The PR description states that "class field lowering runs it after instance storage setup," and the new test confirms this for the bug case. But the entire correctness of the fallback hinges on class-properties inserting lowered field initializers before the existing constructor body (which contains the post-super _initProto(this)). If anyone later changes that ordering — or runs decorators with a different downstream transform — the fallback silently breaks. Worth either adding a comment near inject_after_super at decorator_impl.rs:2493 documenting this assumption, or covering it via a fixture test that explicitly exercises a constructor with pre-existing statements.

  2. Test coverage is thin. A single exec fixture covers the original bug, but several adjacent cases would harden the fix:

    • @dec accessor x = 1 with no other fields (auto-accessor + constructor fallback path).
    • Decorated private field in a derived class (validates inject_after_super placement when an actual super() call exists).
    • Decorated private field followed by a plain public field with a side-effecting initializer (proves the public field is chosen over constructor fallback and ordering with the side effect is preserved).
    • Decorated private field followed only by static fields / methods (forces constructor fallback in a non-derived class).
  3. Minor: comment readability. The new comment at decorator_impl.rs:2408-2414 says "We also avoid decorated public field initializers after that point because their own values are not installed yet." The phrase "after that point" is ambiguous — it actually means "we still skip any decorated public field as an injection target, regardless of position." Suggest rewording to e.g. "Decorated public field initializers are also unsuitable as injection targets, since their own _init_* call has not run when _initProto would fire."

  4. Minor: redundant is_2023_11() guard. is_2022_decorated_instance_field_init returns early if is_2023_11(), but its only callers (is_unsafe_2022_private_field_init, can_inject_2022_init_proto_into_field) are themselves only invoked from the else branch of if self.is_2023_11() at decorator_impl.rs:2417. The guard isn't wrong, just dead. Either drop it or document it as defensive.

Style / conventions

  • Matches the project's Cow<str>/&str Atom guidance (no concerns here — only Ids are stored).
  • Three identical if !self.is_2023_11() && !p.is_static { ... insert ... } blocks at :2864, :3307, :3618. A small helper like track_decorated_instance_field_init(&self, init: &Ident, is_static: bool) would centralize the policy and make it easier to keep in sync if the rule ever evolves.

Performance

Negligible — one extra hashset per class scope, O(n) scan over class members. Fine.

Security

No concerns.


Overall: solid fix with a sound model. The main thing I'd push on is broader fixture coverage for the auto-accessor and derived-class scenarios, since the policy change touches more shapes than just the one in #11846.

@kdy1 kdy1 requested a review from a team as a code owner May 22, 2026 10:44
kodiakhq[bot]
kodiakhq Bot previously approved these changes May 22, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Re-reviewed at HEAD 5423dff. The only changes since my prior review are a merge of main and the changeset file — decorator_impl.rs and the exec fixture are unchanged. That earlier review still stands; the main outstanding item is broader fixture coverage (auto-accessor-only class, derived class with super(), decorated private followed by side-effecting public, decorated private with only static/method members after it) to harden the policy change beyond the single bug repro.

@github-actions
Copy link
Copy Markdown
Contributor

Code Review

Thanks for the thorough fix and exec regression test. Overall the change is well-scoped and the new logic is easier to reason about than the previous should_inject matcher. A few notes below.

Correctness

  • _initProto placement looks correct. In the new test, transformation lands _initProto(this) at the end of the constructor body. That relies on the subsequent class-field lowering prepending field initializers ahead of the existing constructor statements — which is what happens today, but it would be worth a comment near the inject_after_super call site, since the ordering is implicit and a future change to that other pass could silently regress this fix.
  • Public auto-accessors are now treated as unsafe. Under the old code, the synthesized PrivateProp storage that backs @dec accessor x was used as an injection site (the comment said "we still allow private storage generated for public accessors"). That case has the same hazard as decorated #field: addInitializercontext.access.get(this) → getter → read of un-initialized private storage. The new logic correctly treats it as unsafe. Worth calling out in the PR description.
  • Narrow shape check in is_2022_decorated_instance_field_init. The matcher only recognizes Callee::Expr(Expr::Ident(_)). For 2022-03 that's exactly what field_init_call_expr produces (with init_extra == None), so it's safe today. If a future change ever wraps the call (e.g. seq, paren, parens-as-arg) the detection silently turns off and you fall back to "unsafe = false" / "injectable = true", which could regress quietly. Two small hardening options:
    • Use value.unwrap_call().is_some_and(|c| matches!(&c.callee, Callee::Expr(e) if matches!(&**e, Expr::Ident(i) if self.state.decorated_instance_field_inits.contains(&i.to_id())))) against a more permissive shape, or
    • Add a debug assertion that 2022-03 field values never have an outer wrapper.

Code quality / style

  • The two helpers is_unsafe_2022_private_field_init / can_inject_2022_init_proto_into_field invert each other on the public/private dimension. Consider folding them into a single classifier returning a small enum (Unsafe | Injectable | Skip) — the call-site in visit_mut_class would then be one match arm and the "scan for last unsafe, then scan forward" logic gets clearer.
  • The name is_unsafe_2022_private_field_init reads ambiguously next to Rust's unsafe keyword. Something like is_decorated_private_instance_field (or …_blocks_init_proto) would describe the predicate more directly.
  • Helper docs (/// …) on the two new methods would be a nice touch — they're not obvious from the names alone.

Test coverage

  • The new exec fixture covers the exact issue case.
  • Two cases I'd add or extend if cheap:
    1. Class with only decorated private fields (no trailing safe public field), to exercise the constructor-injection fallback in isolation. Today the new fixture exercises that path because the trailing effect field is also decorated, but a dedicated case documents intent.
    2. @dec accessor x; followed by another decorated public field, to lock in the "public auto-accessor storage counts as unsafe" behavior change.
  • Directory naming nit: existing siblings use a double-dash separator (2022-03-ordering--to-es2015). The new fixture uses a single dash (issue-11846-to-es2015). Not a correctness issue, but normalizing is one rename away.

Performance

The added work per class is bounded by O(body.len()) (one rposition plus a linear loop) plus a small FxHashSet. Negligible. Nothing to flag.

Security

No security implications — purely emitted-code ordering.

Minor

  • A short changeset entry would help downstream consumers see this in the release notes (the changeset bot already flagged this).
  • The state save/restore for decorated_instance_field_inits is correctly symmetric with the surrounding take()/restore pattern in visit_mut_class. 👍

@github-actions
Copy link
Copy Markdown
Contributor

Review

Targeted fix that correctly addresses the ordering issue: _initProto must run after any decorated private storage has been brand-installed and after decorated public field values have been written, so that addInitializer callbacks calling context.access.get(this) find a live element.

Looks good

  • Per-class state for decorated_instance_field_inits is properly saved/restored around visit_mut_class, so nested classes won't bleed init-ids into each other.
  • Population sites cover every place a 2022-03 decorated instance field init is created: the @accessor storage path (line 2864), visit_mut_class_prop (line 3307), and visit_mut_private_prop (line 3618). Each insert is correctly gated on !is_2023_11() and !is_static.
  • is_2022_decorated_instance_field_init matches the shape produced by field_init_call_expr for 2022-03 non-static fields (_init_X(this, value)). Since prepend_pending_instance_field_extras is a no-op when !is_2023_11() and init_extra is None in 2022-03, the field's outer expression is always a bare call — no false negatives from a wrapping from_exprs/wrap_init_with_extra.
  • Ident uniqueness: _init_<name> idents are created via private_ident! / apply_mark(Mark::new()), so distinct fields with the same name (e.g. effect and #effect in the fixture) produce distinct Ids in the FxHashSet. The set check is exact.
  • Regression fixture at tests/decorators/issue-11846-to-es2015/exec.js exercises the exact triple from the issue (decorated public → decorated private → decorated public) and asserts both initializer counts and the context.access.get(this) behavior inside addInitializer.

Subtle behavior changes worth being aware of

  1. No more injection into PrivateProp. The old code injected _initProto into a private storage when the next member was a getter (the @accessor case). The new can_inject_2022_init_proto_into_field only accepts ClassProp. That's the correct call — injecting into a private storage's value runs _initProto before the brand exists, which is itself the bug being fixed — but it means decorated @accessor-only classes will now always fall back to constructor injection. That's fine functionally (class-fields lowering hoists the field inits before the constructor-injected _initProto), just worth knowing in case anyone compares output diffs across versions.

  2. Reliance on class-fields lowering order. The fallback comment says: "class field lowering runs it after instance storage setup." This invariant is now load-bearing for the @Accessor and "no non-decorated field after the last unsafe private" cases. A short note in the source pointing at where class-properties moves field init before constructor-prefix statements would help future readers understand why the fallback is safe.

  3. A non-decorated public field positioned before the last unsafe private gets skipped. Example:

    unrelated = 5;           // safe target on its face
    @effect #effect = fn;    // unsafe private at index 1

    last_unsafe_private_init is 1, so unrelated (index 0) is also skipped and we fall through to constructor injection. That's the right call — injecting into unrelated would run _initProto before #effect storage exists — but it's a slightly non-obvious consequence of the i <= last guard. The current comment block above the loop covers the what but not this why; a one-liner ("fields before the last unsafe private are skipped because storage they precede isn't installed yet") would make the intent clearer.

Nits

  • The new doc comment on decorated_instance_field_inits:

    Running _initProto before them lets addInitializer callbacks observe an uninitialized element.

    Reads as "lets callbacks succeed in observing uninitialized state," but the intent is "would cause callbacks to throw on uninitialized state." Consider rephrasing to "would cause addInitializer callbacks to observe an uninitialized element."

  • is_unsafe_2022_private_field_init and can_inject_2022_init_proto_into_field are nicely symmetric helpers — both could be &self free functions that take &ClassState if you want them more clearly side-effect-free, but the current methods are fine.

Verification suggestion

Worth running the full swc_ecma_transforms_proposal test suite to confirm no existing 2022-03 decorator fixtures changed output — the PR adds files but doesn't modify any existing snapshots, which is plausible (most fixtures probably don't mix decorated public + decorated private), but worth a quick cargo test -p swc_ecma_transforms_proposal confirmation.

Coverage gap (optional)

The fixture is for the exact #11846 case. One more fixture covering @accessor + @effect-like decorator together (forcing the constructor fallback path) would lock in the new behavior in case the @Accessor lowering ever moves around again.

Overall: the fix is correctly scoped and the logic holds up under the cases I worked through. LGTM modulo the comment/wording nits above.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5423dffaa0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/swc_ecma_transforms_proposal/src/decorator_impl.rs Outdated
kodiakhq[bot]
kodiakhq Bot previously approved these changes May 22, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Review

Nice targeted fix — the bug analysis is solid and the helper-based factoring (is_2022_decorated_instance_field_init / is_unsafe_2022_private_field_init / can_inject_2022_init_proto_into_field) reads much better than the previous index/lookahead in visit_mut_class. State save/restore for decorated_instance_field_inits across nested classes is correctly handled in visit_mut_class (lines 2394 / 2547).

Things to consider

1. Regression coverage only covers the to-es2015 path.

The PR adds tests/decorators/issue-11846-to-es2015/ but no companion tests/decorators/issue-11846/. Every other 2022-03 fixture has both variants, and the change to visit_mut_class runs regardless of class-properties lowering — under non-es2015 the test would still validate execution ordering (fields then constructor body running _initProto). It's cheap insurance against regressing the non-lowered path, especially since output fixtures for that path now differ materially (see 2022-03-fields/public/output.js).

2. Semantic change is broader than the issue title implies.

The fix is described as "delay 2022 decorator initializers after private fields," but a side-effect is that classes with only decorated public fields (no decorated private storage at all) also get _initProto moved out of the first field initializer into a separate constructor. See e.g. tests/decorators/2022-03-fields/public/output.js:

-    a = (_initProto(this), _init_a(this));
+    constructor(){ _initProto(this); }
+    a = _init_a(this);

This is consistent (a decorated public field is also "unsafe" by can_inject_2022_init_proto_into_field because _init_a(this) itself hasn't run yet, and addInitializer callbacks via that decorator could attempt context.access.get(this)), but it changes ordering and bundle output for every existing 2022-03 user who has any decorated instance field. Worth calling out in the changeset entry so downstream consumers aren't surprised by diff churn in already-passing snapshots.

3. Detection relies on textual pattern match of _init_x(this, ...).

is_2022_decorated_instance_field_init matches Expr::Call where the callee is a bare Ident whose Id is in decorated_instance_field_inits. This works today because field_init_call_expr always produces exactly that shape, and the helper is only invoked after children have been visited. But if a later transform ever wraps the init call (e.g. for pending_instance_field_extras or compat lowering), the check silently returns false and _initProto may be re-injected ahead of an unsafe init. A short comment on is_2022_decorated_instance_field_init noting the structural assumption ("must run on prop values produced by field_init_call_expr before any wrapping") would help future maintainers.

4. Minor: last_unsafe_private_init is re-computed even on the no-unsafe-private path.

n.body.iter().rposition(...) runs unconditionally. For the common case (no decorated private instance fields), it walks the whole body returning None and then the inner loop still calls can_inject_2022_init_proto_into_field. Not a hot path, but you can early-return the original "first decorated-free public field" search when decorated_instance_field_inits is empty. Optional.

5. Comment in visit_mut_class (line 2408) could be slightly clearer.

"We also avoid decorated public field initializers after that point because their own values are not installed yet."

The "after that point" reads as if the constraint is positional, but can_inject_2022_init_proto_into_field rejects decorated public fields unconditionally, regardless of position. Suggest: "We also skip decorated public fields entirely, since their _init_x(this) value hasn't been computed when _initProto would observe it."

Verification

  • Verified decorated_instance_field_inits is populated at all three decorated-instance-field sites: visit_mut_class_prop (line 3307), visit_mut_private_prop (line 3618), and the auto-accessor branch in visit_mut_class_member (line 2864). Looks complete.
  • The fallback to ensure_constructor + inject_after_super is the right escape hatch when no safe public field exists.
  • Static fields are correctly excluded everywhere (is_static checks at each insertion site and in both unsafe/safe predicates).

Nothing security- or performance-critical here; mostly suggesting one more fixture and a couple of doc-comment touch-ups.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5ea9409a60

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/swc_ecma_transforms_proposal/src/decorator_impl.rs Outdated
kodiakhq[bot]
kodiakhq Bot previously approved these changes May 22, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Code Review

Solid fix for the issue #11846 ordering bug. The reasoning behind the placement of _initProto is sound and the regression test exec is well-targeted. Below are some concerns and suggestions.

Correctness concerns

1. Reliance on prop.span.is_dummy() to detect synthesized accessor backing storagedecorator_impl.rs:1182, 1209

is_2022_public_accessor_backing_field and is_2022_accessor_backing_field use !prop.span.is_dummy() to bail out (i.e., they require the span to be DUMMY_SP). This is fragile:

  • It assumes the auto-accessor lowering inside this same pass continues to produce DUMMY_SP-spanned PrivateProps.
  • A future change to that lowering (or any other pass that runs before, e.g., span propagation) could silently break the heuristic and reintroduce the bug, with no test failure.

Consider tracking synthesized accessor backing fields in an explicit FxHashSet<Id> (similar to decorated_instance_field_inits) when they are produced. That makes the relationship between producer and consumer explicit.

2. Adjacency-based detection of accessor pairsdecorator_impl.rs:1186-1195, 1213-1222

The check looks at members at index+1 and index+2 and assumes they are the getter/setter for the field at index. There is no key/name match between the PrivateProp and the getter/setter. If any pass ever reorders class members or inserts another field-like member between them, this check would silently misidentify members. Consider matching by key name as an extra guard.

3. Semantic divergence in initializer ordering

tests/decorators/2022-03-ordering/accessor-initializers-fields/exec.js (and the --to-es2015 mirror) expected order changes from [0..30) to [0..22) ++ [26,27,28,29,22,23,24,25]. That means accessor init effects (26–29) now run before the method/accessor addInitializer callbacks (22–25). Per the 2022-03 proposal, _initProto (which runs addInitializer callbacks for methods/accessors) should run after super() but before field initialization. The fix moves it past instance field init when any decorated private field is present, which is a deliberate but observable behavior change.

Worth confirming this matches Babel's current 2022-03 output (the repo otherwise tracks Babel's behavior closely for these fixtures). If Babel ships the old order, downstream users could observe ordering differences between Babel and SWC. At minimum, the changeset note could call this out.

Code-quality nits

  • is_2022_accessor_backing_field first calls is_2022_public_accessor_backing_field (which performs all the is_static/value.is_none()/span.is_dummy() checks again), then re-does those checks before the matches!. Factor out a small helper that returns the inner prop (or None) to avoid duplicating preconditions.
  • can_inject_2022_init_proto_into_field's PrivateProp branch returns true only when is_2022_public_accessor_backing_field holds — so the !prop.is_static && prop.value.is_some() guard is redundant (those are already enforced inside the helper). Drop them for clarity.
  • decorator_impl.rs:2515-2516 does (0..n.body.len()).rposition(...) and then the loop iterates for i in 0..n.body.len(), computing is_unsafe_2022_private_field_init once and can_inject_2022_init_proto_into_field once per index. The unsafe check internally calls the accessor-backing-field check, which is re-evaluated in can_inject_.... For typical class sizes this is fine, but if you want to be tidy, computing a single index first_safe_index and then a single linear scan would read more cleanly.
  • The doc comment block at decorator_impl.rs:2471-2477 is good. Consider also linking back to issue stage 3 decorator error execution #11846 from is_unsafe_2022_private_field_init so future maintainers find the rationale.

Test coverage

  • The new issue-11846-to-es2015 exec fixture is a good regression test. Consider adding a parallel non---to-es2015 variant under tests/decorators/issue-11846/ so both lowering paths are exercised (the bug surface includes both).
  • Recommend an additional exec fixture where the decorated private field is followed (not preceded) by a public decorated field — to verify last_unsafe_private_init finds the latest unsafe index and the public field still gets _init_a(this) without _initProto injection.
  • A negative test would be nice: a class with only a decorated public field and no decorated private fields should still inline _initProto into the field initializer (no constructor injection), to lock in the unchanged behavior.

Performance / security

No concerns. The added pass-local state is small, no unsafe, no new allocations on hot paths beyond a single FxHashSet per class scope (correctly saved/restored across nested classes).

Overall: the approach is reasonable and the bug is real. The main blocker before merge is double-checking the spec-vs-Babel ordering for accessor-initializers-fields and hardening the synthesized-field detection so it doesn't silently regress.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 418a967f9e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/swc_ecma_transforms_proposal/src/decorator_impl.rs Outdated
@github-actions
Copy link
Copy Markdown
Contributor

PR Review

Reviewed the changes for the 2022-03 decorator _initProto ordering fix (#11846).

Summary

The fix correctly addresses the bug where _initProto(this) could fire addInitializer callbacks before decorated private storage was set up, causing context.access.get(this) to throw on a private brand check. The new logic tracks decorated instance field initializer ids, finds the latest "unsafe" private decorated field, then defers _initProto past it (and any later decorated public fields whose initializers callers may try to read). When no safe field site remains, it falls back to constructor injection. Edge cases — no private decorated fields, isolated public decoration, and synthesized accessor backing storage — are handled correctly.

What looks good

  • Helper decomposition (is_2022_decorated_instance_field_init, is_2022_public_accessor_backing_field, is_2022_accessor_backing_field, is_unsafe_2022_private_field_init, is_2022_decorated_public_field_init, can_inject_2022_init_proto_into_field) is readable and keeps the post-visit injection loop concise.
  • decorated_instance_field_inits is properly saved/restored around nested classes via take(...) and re-assignment at the end of visit_mut_class — nested decorated classes won't bleed ids into each other.
  • The body is visited before the injection scan, so the id set is fully populated when is_unsafe_2022_private_field_init runs.
  • The new regression fixtures (issue-11846-public-fields, issue-11846-to-es2015) cover both pure-public and mixed public/private scenarios.

Questions / suggestions

  1. Spec compliance vs. practical fix (worth a second look). TC39 2022-03 places _initProto before field initialization. The new behavior delays it past instance field inits (visible in the 2022-03-ordering/accessor-initializers-fields expected output: addInitializer callbacks 22..25 now run after init callbacks 26..29). For @effect decorators relying on a constructed private brand this is required, but it does invert the documented ordering. Has this been cross-checked against Babel's output for the same input, or at least the TC39 reference tests? If Babel still emits the pre-field ordering, downstream users targeting both transpilers may see divergent behavior. Even if it matches Babel today, a note in the changeset or comment block would help future maintainers.

  2. Heuristic: prop.span.is_dummy() to identify synthesized accessor backing storage. This works because the synthesizer at decorator_impl.rs:2785 uses DUMMY_SP, but is implicit. A user-written accessor whose span got rewritten by an upstream pass would silently misclassify. Adding an explicit marker (e.g., a synthesized_accessor_backing: bool flag on ClassState, or comparing the key against the captured init id) would make this more robust. Not blocking, but a comment near the synthesizer pointing out the DUMMY_SP contract would help.

  3. Adjacency assumption in is_2022_public_accessor_backing_field / is_2022_accessor_backing_field. The check assumes the getter/setter sit at exactly index + 1 and index + 2. This holds today because the auto-accessor lowering pushes them together, but any future pass that interleaves another member (e.g., a debug stub) would break the heuristic. Worth a debug_assert! or a comment documenting the invariant.

  4. is_2022_decorated_public_field_init does not require a dummy span, unlike the private-backing helpers. That is correct — user-written ClassProps are what we want to match — but it's worth a sentence explaining why the asymmetry exists.

  5. Duplication between is_2022_public_accessor_backing_field and is_2022_accessor_backing_field. Both repeat prop.is_static || prop.value.is_none() || !prop.span.is_dummy(). Extracting a synthesized_instance_backing(prop) predicate would remove the duplication and centralize the heuristic from item 2.

  6. is_2022_decorated_instance_field_init only handles bare Expr::Call. For 2022-03 + non-static, prepend_pending_instance_field_extras is a no-op (it short-circuits on is_2023_11() and on static), so the field value is the raw call. That holds today, but if anyone later wraps these initializers in a Seq for 2022-03 the detection silently breaks. Either pattern-match through a Seq's last expression or assert/comment the invariant.

  7. Unrelated CI change. The wasm-pack install switch (curl-script → npm install -g wasm-pack) appears in the same PR. It's a sensible change, but mixing it with a semantically tricky decorator fix makes bisection harder; a separate PR would be cleaner. Not a blocker.

Testing

  • New fixtures look right and follow the suite naming conventions per crates/swc_ecma_transforms_proposal/AGENTS.md.
  • Recommend confirming cargo test -p swc_ecma_transforms_proposal passes locally without UPDATE=1 (per AGENTS.md) and that the two updated ordering exec fixtures genuinely reflect the desired runtime behavior rather than just the new emit.

Performance

The added scans are O(n) per class body and only fire on 2022-03 emit, so impact is negligible.

Security

No security implications spotted.

Overall: solid, well-tested fix. Main asks are spec/Babel parity confirmation and a couple of small clarifying comments around the dummy-span and adjacency heuristics.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 62692386da

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/swc_ecma_transforms_proposal/src/decorator_impl.rs Outdated
kodiakhq[bot]
kodiakhq Bot previously approved these changes May 23, 2026
kodiakhq[bot]
kodiakhq Bot previously approved these changes May 23, 2026
kodiakhq[bot]
kodiakhq Bot previously approved these changes May 23, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Review

Thanks for digging into #11846. The root cause analysis is solid — moving each field decorator's addInitializer callbacks into its own per-field _init_extra_* entry (instead of pooling them into protoInitializers and triggering them all via _initProto) is the right structural fix and gets us closer to the spec ordering. I traced through several of the updated snapshots and the new exec test, and the timing of _initProto vs each field's local extras lines up with what the proposal mandates.

A few observations:

Helper ABI break vs. changeset level

_apply_decs_2203_r now returns a different shape — every decorated field gains an _init_extra_* slot between its init and the next member's entry. Old transpiled output destructures the array positionally:

({ e: [_init_a, _init_b, _initProto] } = _apply_decs_2203_r(...));

With the new helper that array is now [_init_a, _init_extra_a, _init_b, _init_extra_b, _initProto], so the bindings silently land on the wrong functions at runtime. This is a hard ABI break for anyone who upgrades @swc/helpers without retranspiling. The changeset marks @swc/helpers as patch, but a runtime contract change of this kind probably deserves at least minor plus an explicit release-note callout that the helper version must move in lockstep with the transformer. Worth checking how previous 2022-03 helper layout changes were tagged in this repo.

Unrelated CI changes bundled in

.github/workflows/CI.yml switches the wasm-pack install path from the rustwasm shell installer to npm install -g wasm-pack in three jobs. The comment justifies it ("can crash on GitHub Windows runners"), but it's unrelated to the decorator fix and would be much easier to review/revert in isolation. Consider splitting it into its own PR — keeps the bisect history clean if Windows CI flakes later.

Code quality nits in decorator_impl.rs

  • is_2022_public_accessor_backing_field and is_2022_accessor_backing_field repeat the same prop.is_static || prop.value.is_none() || !prop.span.is_dummy() guard and the same matches! shape, differing only by Method vs PrivateMethod for the getter/setter pair. A small helper that takes a fn(&ClassMember) -> Option<(MethodKind, bool /* is_static */)> (or just an enum for the next-two-members shape) would collapse the duplication and make the intent clearer.
  • The doc comment on is_2022_accessor_backing_field ("Generated accessor storage should not block _initProto …") describes motivation but doesn't state what the function returns. Compare with the sibling is_2022_public_accessor_backing_field which is clearer. Worth restating in the form "Returns true if members[index] is private storage synthesized for a private auto-accessor."
  • can_inject_2022_init_proto_into_field is named clearly, but a one-line // comment summarizing why public ClassProp is unconditionally OK while PrivateProp requires the accessor-backing check would save a reader from having to reconstruct the reasoning.

Helper file readability

The old comments in applyMemberDecs (// initialize staticInitializers when we see a non-field static member) were dropped in the refactor. The new shape with a separate if (kind === 0 /* FIELD */) branch is more correct, but a brief comment explaining "field decorators get a local initializers array so each field's addInitializer callbacks are returned as a dedicated extra" would help future maintainers understand why FIELD now diverges from the shared protoInitializers / staticInitializers paths.

Minor: unconditional _init_extra_* allocation

Every decorated field now emits an _init_extra_* binding + destructure slot + helper push, even if no decorator on that field calls addInitializer. The helper always pushes a runner because initializers = [] is truthy. This is a small output-size cost on every decorated field in every project. Probably fine — the codegen-time "did any decorator call addInitializer?" check isn't feasible — but worth confirming you're OK with the bytes. (For comparison, the 2023-11 path already does this, so it's at least consistent.)

Constructor injection for derived classes

inject_after_super places leftover _init_extra_* calls in a sequence with super() ((super(), _init_extra_x(this))). That's correct because the spec runs InitializeInstanceElements between super() returning and the surrounding expression yielding this, so the extras correctly run after instance fields. Non-derived classes prepend to the ctor body, which also runs after implicit field initialization. Both paths check out, but worth noting — for derived classes — that the new exec test only exercises a non-derived class. An exec fixture with class B extends A { @effect #x = …; } would directly cover the inject_after_super ordering, which is the path most likely to regress silently if inject_after_super semantics ever change.

Test coverage

  • issue-11846-public-fields snapshot + issue-11846-to-es2015 exec is a good combination — snapshot pins the codegen shape, exec validates runtime semantics.
  • Consider adding an exec variant without --to-es2015 to cover the path where private fields stay as #name syntax (currently only snapshotted). The runtime semantics under V8's native private fields is where the original TypeError: attempted to get private field on non-instance showed up, so an exec test on the un-lowered output is the most direct regression net.
  • A class B extends A { @dec field = … } derived-class variant would also be worth adding given how much of the new logic hinges on inject_after_super placement.

Spec / behavior compatibility

Worth flagging in the release notes that this changes the observable execution order for all 2022-03 decorated fields, not just the buggy private-field case. The new ordering matches the spec (per-field addInitializer runs immediately after that field is set, before the next field's value is computed), but consumers who happened to rely on the old ordering — where all field addInitializers piled into the shared _initProto and fired before any field initializer — will see a change. Probably worth a one-line note in the changeset body.

Overall the fix looks correct and the test added directly targets the failure mode from #11846. Main asks: bump the helper changeset versioning, split the CI workflow change out, tighten the new docstrings, and add at least one derived-class exec test to lock down the inject_after_super path.

@github-actions
Copy link
Copy Markdown
Contributor

Summary

The decorator fix itself looks correct: aligning 2022-03 field initializer ordering with 2023-11 by emitting per-field _init_extra_* callbacks and reserving _initProto for method/accessor addInitializer work. The helper change (per-field initializers = [] followed by an extra entry pushed into ret for kind === 0) is consistent with what the Rust side now destructures, and the new exec regression test (issue-11846-to-es2015) directly reproduces the original context.access.get(this) failure scenario.

That said, I have a few concrete suggestions before merging.

Scope: unrelated CI changes bundled in

The PR title and changelog are about decorator init ordering, but it also reworks .github/workflows/CI.yml (submodule init split, wasm-pack switched to npm install -g) and .github/workflows/claude-code-review.yml (error handling around comment dismissal). These are reasonable changes, but they're orthogonal to the decorator fix. Splitting them into separate PRs would:

  • make bisecting easier if either change causes a regression (decorator output vs. CI infra),
  • avoid blocking the bugfix on CI debate (e.g., npm install -g wasm-pack vs. the rustwasm installer),
  • keep the changelog entry honest about what shipped.

If splitting is impractical, please at least call out the CI changes explicitly in the PR description.

crates/swc_ecma_transforms_proposal/src/decorator_impl.rs

is_2022_public_accessor_backing_field (line 1145) and is_2022_accessor_backing_field (line 1168):

  • The detection relies on prop.span.is_dummy() to distinguish synthesized auto-accessor backing storage from a source-written private field. That's correct today (see line 2698 where the synthesized PrivateProp is built with span: DUMMY_SP), but it's a load-bearing implicit invariant. A one-line comment in is_2022_public_accessor_backing_field like "synthesized backing storage uses DUMMY_SP — see auto-accessor lowering" would save the next maintainer a hunt. The current doc comments describe what the function detects but not why span.is_dummy() is a reliable signal.

  • is_2022_accessor_backing_field calls is_2022_public_accessor_backing_field and then re-runs the exact same PrivateProp / is_static / value.is_none() / span.is_dummy() checks before testing for a PrivateMethod pair. You can fold this into a single function that checks the prop once and then matches either (Method, Method) or (PrivateMethod, PrivateMethod) getter/setter pairs in one matches!. Same behavior, less duplication.

prepend_pending_instance_field_extras (line 781) and queue_pending_instance_field_extra (line 800):

Dropping the is_2023_11() gate is the right call now that 2022-03 uses the same ordering model, but the function names still hint that they're 2023-11–specific. Not a blocker; consider renaming or at least updating the doc comments next time this is touched.

Output shape

A few things to think about for follow-ups (not blockers):

  • Every decorated field now emits a _init_extra_* slot even when the decorator never calls addInitializer. For instance the new issue-11846-public-fields output adds three _init_extra_* variables for three decorated fields, two of which are never useful. That's unavoidable without runtime knowledge of decorator behavior, but it's a visible bundle-size hit for users with many decorated fields. Worth a note in the changelog so people aren't surprised.
  • Static fields now wrap each initializer in an IIFE ((()=>{ const _value = _init_a(this); _init_extra_a(this); return _value; })()) where instance fields use the more compact (prev_extras, init) chained pattern. The asymmetry is intentional (static fields don't have a constructor to flush remaining extras to), but it's a notable code-size delta vs. before. Future improvement: a sequence expression with a temp var _value declared once at the top of the static block could avoid per-field IIFEs.

Test coverage

  • issue-11846-to-es2015/exec.js is a great regression test — it actually exercises the brand check that triggered stage 3 decorator error execution #11846. Nice.
  • The companion issue-11846-public-fields/ test is snapshot-only. A modern-JS exec test (no --to-es2015) covering the same scenario would catch any future regression in the non-es2015 path without needing the class-properties downleveling. Recommend adding issue-11846/exec.js alongside.
  • The existing fixtures get a lot of churn (10+ files), and the new _init_extra_* injection points are visible across all of them. Worth a careful eyeball pass — particularly 2022-03-misc/all-decorators/output.js, which has the most complex ordering — to confirm the new sequence matches Babel reference output for the same input.

Runtime helper (_apply_decs_2203_r.js)

  • The change is minimal and symmetric (same edit in both crates/.../helpers/ and packages/helpers/esm/). Good.
  • pushInitializers(ret, initializers) pushes a function even when initializers is [] (since the truthiness check if (initializers) passes for empty arrays). That's deliberate so the destructuring pattern stays aligned, but it means every decorated field pays the cost of one extra function allocation and one extra ret.push regardless of whether addInitializer was ever called. Not a hot path, but worth knowing.
  • Minor: the comment-stripping in applyMemberDecs removed the // initialize staticInitializers when we see a non-field static member / // initialize protoInitializers ... comments. They were still accurate. Consider keeping them in the new structure.

Correctness questions

  • For derived classes, inject_after_super(ctor, pending_instance) (line 2496) injects the final field's _init_extra_* callbacks immediately after super(). That seems right, but no fixture in this PR covers a derived class with a tail _init_extra_* flush. If there's a tsc/babel-style fixture available, adding one would close the loop.
  • The 2022-03 spec orders field decorators' addInitializer callbacks to run after that field's value is assigned. The chosen lowering (run field N's extras at field N+1's init site, last field's extras in the constructor) is functionally equivalent but cognitively non-obvious. A short comment near queue_pending_instance_field_extra explaining this would help future readers.

Overall the fix is sound and the new exec test directly demonstrates correctness. Main asks before merge: (1) split or at least document the CI changes, (2) drop a comment near the span.is_dummy() check, (3) consider adding a modern-JS exec for the public-field scenario.

kodiakhq[bot]
kodiakhq Bot previously approved these changes May 23, 2026
@kdy1 kdy1 enabled auto-merge (squash) May 23, 2026 16:04
@github-actions
Copy link
Copy Markdown
Contributor

Code Review

Thanks for taking on the #11846 ordering bug. The structural redesign — giving each field decorator its own _init_extra_* runner and reserving _initProto for method/accessor addInitializer callbacks — is a real improvement over the previous shape-based heuristic and is much closer to the 2022-03 spec ordering. The new exec fixture directly reproduces the original TypeError: attempted to get private field on non-instance failure mode and asserts the right runtime behavior.

Multiple prior bot reviews already cover the high-level concerns well. Below I focus on the items I think still warrant attention before merge.

Runtime ABI break in _apply_decs_2203_r — versioning concern

The helper's returned array shape now changes: every decorated field gains an _init_extra_* slot between its init and the next entry. Old transpiled output destructures positionally:

({ e: [_init_a, _init_b, _initProto] } = _apply_decs_2203_r(...));

Run against the new helper, those bindings silently land on the wrong functions at runtime. This is a hard runtime contract change between @swc/core and @swc/helpers:

  • Users who upgrade only @swc/core (and pull older @swc/helpers from their lockfile) will emit the new destructure shape against the old helper → silent misalignment.
  • Users who upgrade only @swc/helpers against an older @swc/core → same problem in reverse.

The current changeset (crates/swc_ecma_transforms_base/src/helpers/_apply_decs_2203_r.js) bumps swc_core / swc_ecma_transforms_base / swc_ecma_transforms_proposal as patch, but the npm package @swc/helpers is not mentioned at all even though packages/helpers/esm/_apply_decs_2203_r.js was updated. Two things to consider:

  1. Add a changeset entry for @swc/helpers and bump it to at least minor — patch versioning doesn't communicate a runtime contract change.
  2. Document (in the changeset and/or release notes) the required peer-version pairing between @swc/core and @swc/helpers for projects using 2022-03 decorators.

Behavior change is broader than the issue title

The PR title and changeset describe "delay 2022 decorator initializers after private fields," but a side effect is that every 2022-03-decorated class with instance field decorators now emits a different shape (see 2022-03-fields/public/output.js, 2022-03-exported/member-decorator/output.mjs, etc.). The accessor-ordering exec test in 2022-03-ordering/accessor-initializers-fields will run accessor init callbacks before method addInitializer callbacks (26..29 then 22..25), which is the spec-correct behavior but a deliberate observable change from the previous emit. Worth calling out explicitly in the changeset body so downstream consumers aren't surprised.

Fragile prop.span.is_dummy() invariant

is_2022_public_accessor_backing_field (decorator_impl.rs:1145) and is_2022_accessor_backing_field (decorator_impl.rs:1168) gate on prop.span.is_dummy() to distinguish synthesized auto-accessor backing storage from a user-written private field. Today the synthesizer constructs the PrivateProp with DUMMY_SP, so this works — but the invariant is implicit and load-bearing:

  • A future pass that propagates a real span onto synthesized storage would silently flip these helpers to false, forcing constructor fallback (still correct, but losing the public-field injection target).
  • The predicate doesn't compare the getter/setter key against the storage key, so any future pass that reorders class members between index+1/index+2 would silently misidentify.

A short comment near the DUMMY_SP check (e.g., "synthesized accessor backing storage uses DUMMY_SP — see auto-accessor lowering at ...") would help, but tracking synthesized backing storage in an explicit FxHashSet<Id> at synthesis time would be more robust.

is_2022_accessor_backing_field duplicates checks already done in the public helper

is_2022_accessor_backing_field first calls is_2022_public_accessor_backing_field (which performs all of is_static / value.is_none() / span.is_dummy()), then re-does the same PrivateProp extraction and the same three checks before the PrivateMethod pair match. The two helpers can be folded into one that extracts the prop once and matches either (Method, Method) or (PrivateMethod, PrivateMethod) in a single matches!. Same behavior, less duplication, easier to keep in sync.

Similarly, in can_inject_2022_init_proto_into_field, the PrivateProp branch's !prop.is_static && prop.value.is_some() guard is redundant — those are already enforced inside is_2022_accessor_backing_field.

Constructor-fallback ordering depends on class-properties insertion semantics

The fallback's correctness — for the "no safe public field" case and for the per-class final _init_extra_* flush — hinges on proposal-class-properties inserting lowered field initializers before the existing constructor body. This holds today (verified via the new exec test) but isn't documented at the call sites in visit_mut_class. A one-line comment near decorator_impl.rs:2484 (ensure_constructor / inject_after_super for _initProto) and :2493 (the pending instance extras flush) would protect against silent regression if class-properties' insertion order is ever changed.

Helper file readability nit

The refactor in applyMemberDecs removed the still-accurate "initialize staticInitializers when we see a non-field static member" / "initialize protoInitializers when we see a non-field member" comments. The new shape is more correct, but a brief comment explaining "field decorators get a fresh local initializers array so each field's addInitializer callbacks are returned as a dedicated extra in ret" would help future maintainers understand why FIELD now diverges from the shared paths.

Test coverage gaps

The new issue-11846-to-es2015/exec.js covers the exact bug — great. A few adjacent shapes that exercise paths now in scope but not yet pinned by a fixture:

  1. Derived class (class B extends A { @effect #x = 1; }): the only validation of inject_after_super for the pending extras flush is via the implicit non-derived path. A derived-class exec would cover the inject_after_super(ctor, pending_instance) branch at decorator_impl.rs:2496.
  2. @dec accessor x = 1 standalone (no other fields): exercises the is_2022_public_accessor_backing_field branch in isolation and the constructor fallback through it.
  3. Decorated #field followed only by static fields / methods: forces the constructor-fallback path in a non-derived class.
  4. Modern-JS exec for the public-field case (no --to-es2015): the current issue-11846-public-fields is snapshot-only; native #name syntax is exactly where the original TypeError showed up in V8, so an exec test on the un-lowered output would be the most direct regression net.
  5. Directory naming: siblings use a double-dash separator (2022-03-ordering--to-es2015); the new fixture uses a single dash (issue-11846-to-es2015). One rename, optional.

Static-field IIFE per field

Each decorated static field now wraps the initializer in a fresh IIFE ((()=>{ const _value = _init_a(this); _init_extra_a(this); return _value; })()). The asymmetry with the instance-field (prev_extras, init) chained pattern is intentional (static fields lack a constructor sink for the final extra), but it's a visible code-size delta vs. before. A follow-up that uses a single sequence with a temp at the top of the synthesized static block could remove the per-field arrow allocation. Not a blocker.

Unrelated CI changes bundled in

.github/workflows/CI.yml (submodule init split, wasm-pack switched to npm install -g) and .github/workflows/claude-code-review.yml (error handling around comment dismissal) are reasonable but orthogonal to the decorator fix. Splitting them into separate PRs would:

  • make bisecting easier if either change causes a regression,
  • avoid blocking the bugfix on CI debate,
  • keep the changelog entry honest.

If splitting isn't practical, at least mention them in the PR description.

Minor

  • The doc comment on is_2022_public_accessor_backing_field says "this storage can host _initProto so method/accessor addInitializer callbacks still run before accessor init effects." — with the new model, _initProto carries only method/accessor inits (field-decorator addInitializers are now in _init_extra_*), so the phrasing slightly overstates what _initProto does today. Minor wording tweak would help.
  • Every decorated field allocates an _init_extra_* ident, var declaration, destructure slot, and helper push regardless of whether any decorator actually calls addInitializer. Unavoidable without runtime knowledge, but worth noting as a bundle-size cost in the release note.
  • _init_extra_* is allocated even when init.sym already has an _init_ prefix → identifiers like _init_extra__init_a (double underscore). Cosmetic only.

Performance

Negligible — one extra FxHashSet per class scope wasn't introduced (the design uses pending_instance_field_extra_inits which was already there). The O(body.len()) scan is fine.

Security

No concerns. Pure emitted-code ordering.


Overall: solid, well-motivated fix with the right structural direction. The main asks before merge are (1) the @swc/helpers versioning story, (2) a comment hardening the DUMMY_SP invariant (or replacing it with an explicit tracking set), and (3) at minimum a derived-class exec fixture to lock in the inject_after_super flush path beyond the one issue repro.

@github-actions
Copy link
Copy Markdown
Contributor

PR Review

Thanks for tackling #11846. The fix correctly addresses the root cause: in 2022-03 mode, field-decorator addInitializer callbacks need to run after their field's storage is initialized, not bundled into the shared _initProto. Moving them to per-field _init_extra_X initializers is the right modeling, and the helper change (pushInitializers(ret, initializers) right after each FIELD's init, plus initializers = [] per FIELD) matches what 2023-11 already did.

Things to consider

1. Behavioral change for static fields is broader than the PR description suggests.

Before this PR, every static-field addInitializer callback ran from a single _initStatic(Foo) call after all static fields had been initialized. After this PR, each static field's addInitializer runs immediately after that field's own initializer (inside the (()=>{ const _value = _init_a(Foo); _init_extra_a(Foo); return _value })() wrapper). See the new tests/decorators/2022-03-fields/static-private/output.js and static-public/output.js.

That means a user-supplied addInitializer on @dec static a can no longer observe Foo.b even if b is declared right after a — previously it could. The PR description only mentions the instance-field case from #11846, so it's worth either:

  • explicitly calling this out in the PR description / changeset (this is arguably a behavior fix, not just an internal refactor), or
  • adding an exec fixture in tests/decorators/2022-03-fields*/static-* that asserts the new per-field ordering, so the contract is documented.

2. Missing exec coverage for the snapshot fixture.

tests/decorators/issue-11846-public-fields/ has only input.js/output.js. The exec fixture (issue-11846-to-es2015/exec.js) is --to-es2015 only and uses effect/property decorators rather than the initializedBeforeFields shape from the original input. Consider adding an exec.js next to the snapshot that actually runs the input under 2022-03 (no es2015 lowering) and asserts instance.initializedBeforeFields === true — this both pins the bug as fixed end-to-end and locks in the snapshot's intent for future readers.

3. Derived-class injection point is correct but subtle — worth a comment.

When pending_instance_field_extra_inits still has items after visiting all members, they get injected via inject_after_super (derived) or prepended to the constructor body (non-derived). That happens to be correct because class field initializers run between super() and the rest of the constructor body in derived classes, and at the very start of the constructor for non-derived. But a one-line comment at decorator_impl.rs:2490-2509 explaining why injecting after super() still runs after field init would save the next reader from having to reason it out.

4. Duplicated guards in the two is_2022_..._accessor_backing_field helpers.

is_2022_public_accessor_backing_field and is_2022_accessor_backing_field both check prop.is_static || prop.value.is_none() || !prop.span.is_dummy() on the same PrivateProp. The second function calls the first, then re-extracts the prop and re-runs identical guards. Extracting that into a small is_synthetic_private_storage(&PrivateProp) -> bool (or just inlining the getter/setter shape check after a single guard) would tighten things up. Minor.

5. init_extra is still gated on is_2023_11() for AutoAccessor, but not for ClassProp/PrivateProp.

At decorator_impl.rs:2708-2710 and :2722-2724, auto-accessors only get init_extra in 2023-11 mode. That's actually correct, because auto-accessors use kind=1 (ACCESSOR) and the new helper's pushInitializers(ret, initializers) only fires for kind=0 (FIELD) — so an _init_extra slot would be absent from the helper's return for 2022-03 accessors. But this asymmetry between paths is non-obvious; a one-liner like // 2022-03 accessors stay on protoInitializers; only kind=0 FIELD gets a per-member extra would help.

6. Unrelated CI changes bundled in.

The .github/workflows/CI.yml changes (splitting submodule init out of actions/checkout, switching wasm-pack install from the shell installer to npm install -g wasm-pack) are sensible on their own — the rationale in the inline comments is good — but they're unrelated to the decorator fix. Bundling them risks reverting the bug fix if the CI changes turn out to need adjustment. Consider landing them separately. Same for the changeset: only swc_ecma_transforms_base (helper file) and swc_ecma_transforms_proposal (transform) actually changed; swc_core being bumped is fine but worth verifying.

Things that look good

  • Helper change (_apply_decs_2203_r.js / packages/helpers/esm/_apply_decs_2203_r.js) is kept in sync between the two copies, which is easy to miss.
  • The extra_stmts/pending_instance_field_extra_inits save-and-restore around visit_mut_class (:2395 and :2538) correctly handles nested class expressions — the valid-expression-formats fixture diff confirms it works on inner classes built inside method bodies.
  • LHS bucket ordering for 2022-03 (partition_2022_member_decs still partitioning non-field vs field, then chaining) is unchanged, and the helper's per-FIELD push happens at exactly the matching position in ret. The destructuring patterns in the updated snapshots line up.
  • No new clippy / fmt concerns at a glance; verifying with the standard cargo fmt --all / cargo clippy --all --all-targets -- -D warnings / cargo test -p swc_ecma_transforms_proposal before merge per repo conventions is sufficient.

Overall: the core fix is correct and the snapshot diffs are coherent. Main asks are (a) call out the static-field behavior change, (b) add a runtime exec fixture for the 2022-03 public-fields case, and (c) ideally split the CI changes into a separate PR.

@kdy1 kdy1 merged commit 3f1a4f5 into main May 23, 2026
98 of 100 checks passed
@kdy1 kdy1 deleted the kdy1/fix-11846-decorator-init-order branch May 23, 2026 16:33
@github-actions github-actions Bot added this to the Planned milestone May 23, 2026
kdy1 added a commit that referenced this pull request May 27, 2026
**Description:**

Reverts #11847 because the helper changes shipped in
`@swc/helpers@0.5.22` caused a decorator helper regression.

This restores the previous 2022-03 decorator initializer behavior,
removes the regression fixtures added by #11847, reverts the matching
external helper source, and adds a patch changeset for the touched Rust
crates.

`@swc/helpers@0.5.22` has been deprecated on npm with an upgrade note
for `0.5.23`.

Validation:

- `git submodule update --init --recursive`
- `cargo fmt --all`
- `cargo test -p swc_ecma_transforms_base`
- `cargo test -p swc_ecma_transforms_proposal`
- `cargo clippy --all --all-targets -- -D warnings`

`pnpm --filter @swc/helpers build` was also attempted, but local
`node_modules` is not installed in this checkout, so `zx` was
unavailable.

**BREAKING CHANGE:**

None.

**Related issue (if exists):**

Reverts #11847
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

stage 3 decorator error execution

1 participant