fix(es/decorators): delay 2022 decorator initializers after private fields#11847
Conversation
🦋 Changeset detectedLatest 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 |
Binary Sizes
Commit: 3a67b6a |
Merging this PR will improve performance by 2.81%
Performance Changes
Tip Curious why this is faster? Comment Comparing Footnotes
|
Code ReviewThanks 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 goodThe 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 The asymmetry between Auto-accessor regression checkThe old code intentionally allowed injection before private-prop-followed-by-getter (i.e., the desugared form of Concerns
Style / conventions
PerformanceNegligible — one extra hashset per class scope, O(n) scan over class members. Fine. SecurityNo 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. |
|
Re-reviewed at HEAD |
Code ReviewThanks 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 Correctness
Code quality / style
Test coverage
PerformanceThe added work per class is bounded by SecurityNo security implications — purely emitted-code ordering. Minor
|
ReviewTargeted fix that correctly addresses the ordering issue: Looks good
Subtle behavior changes worth being aware of
Nits
Verification suggestionWorth running the full Coverage gap (optional)The fixture is for the exact Overall: the fix is correctly scoped and the logic holds up under the cases I worked through. LGTM modulo the comment/wording nits above. |
There was a problem hiding this comment.
💡 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".
ReviewNice targeted fix — the bug analysis is solid and the helper-based factoring ( Things to consider1. Regression coverage only covers the to-es2015 path. The PR adds 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 - a = (_initProto(this), _init_a(this));
+ constructor(){ _initProto(this); }
+ a = _init_a(this);This is consistent (a decorated public field is also "unsafe" by 3. Detection relies on textual pattern match of
4. Minor:
5. Comment in
The "after that point" reads as if the constraint is positional, but Verification
Nothing security- or performance-critical here; mostly suggesting one more fixture and a couple of doc-comment touch-ups. |
There was a problem hiding this comment.
💡 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".
Code ReviewSolid fix for the issue #11846 ordering bug. The reasoning behind the placement of Correctness concerns1. Reliance on
Consider tracking synthesized accessor backing fields in an explicit 2. Adjacency-based detection of accessor pairs — The check looks at members at 3. Semantic divergence in initializer ordering
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
Test coverage
Performance / securityNo concerns. The added pass-local state is small, no Overall: the approach is reasonable and the bug is real. The main blocker before merge is double-checking the spec-vs-Babel ordering for |
There was a problem hiding this comment.
💡 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".
PR ReviewReviewed the changes for the 2022-03 decorator SummaryThe fix correctly addresses the bug where What looks good
Questions / suggestions
Testing
PerformanceThe added scans are O(n) per class body and only fire on 2022-03 emit, so impact is negligible. SecurityNo 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. |
There was a problem hiding this comment.
💡 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".
ReviewThanks for digging into #11846. The root cause analysis is solid — moving each field decorator's A few observations: Helper ABI break vs. changeset level
({ e: [_init_a, _init_b, _initProto] } = _apply_decs_2203_r(...));With the new helper that array is now Unrelated CI changes bundled in
Code quality nits in
|
SummaryThe decorator fix itself looks correct: aligning 2022-03 field initializer ordering with 2023-11 by emitting per-field That said, I have a few concrete suggestions before merging. Scope: unrelated CI changes bundled inThe PR title and changelog are about decorator init ordering, but it also reworks
If splitting is impractical, please at least call out the CI changes explicitly in the PR description.
|
Code ReviewThanks for taking on the #11846 ordering bug. The structural redesign — giving each field decorator its own 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
|
PR ReviewThanks for tackling #11846. The fix correctly addresses the root cause: in 2022-03 mode, field-decorator Things to consider1. Behavioral change for static fields is broader than the PR description suggests. Before this PR, every static-field That means a user-supplied
2. Missing exec coverage for the snapshot fixture.
3. Derived-class injection point is correct but subtle — worth a comment. When 4. Duplicated guards in the two
5. At 6. Unrelated CI changes bundled in. The Things that look good
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. |
**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
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,addInitializercallbacks usingcontext.access.get(this)could throwTypeError: attempted to get private field on non-instance.This tracks 2022-03 decorated instance field initializer ids and only injects
_initProtointo a safe later public field initializer. If decorated private storage has to be initialized first and no safe field exists,_initProtofalls 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