Skip to content

fix(transformer/typescript): preserve execution order for accessor with useDefineForClassFields: false#21369

Merged
graphite-app[bot] merged 1 commit intomainfrom
fix/transformer-accessor-use-define-for-class-fields
Apr 15, 2026
Merged

fix(transformer/typescript): preserve execution order for accessor with useDefineForClassFields: false#21369
graphite-app[bot] merged 1 commit intomainfrom
fix/transformer-accessor-use-define-for-class-fields

Conversation

@Dunqing
Copy link
Copy Markdown
Member

@Dunqing Dunqing commented Apr 13, 2026

Summary

When useDefineForClassFields: false (setPublicClassFields assumption), the TypeScript transform moves public/TS-private field initializers into the constructor. However, private field initializers (including accessor backing fields from the legacy decorator transform) were left as class field declarations, causing them to execute before the constructor body. This broke execution order when the initializer depended on other fields that were already moved to the constructor.

Input:

class Hello {
    private input = { foo };
    accessor util = this.input.foo();
}

Before (broken): this.input.foo() runs before this.input = { foo }

class Hello {
    constructor() { this.input = { foo }; }
    #_util_accessor_storage = this.input.foo(); // ← runs BEFORE constructor
}

After (fixed): all instance initializers in the constructor, in source order

class Hello {
    constructor() {
        this.input = { foo };
        this.#_util_accessor_storage = this.input.foo();
    }
    #_util_accessor_storage;
}

Approach

Matches TypeScript's WillHoistInitializersToConstructor logic in shouldTransformAutoAccessorsInCurrentClass:

  • Pre-scan the class body to check if any non-private instance field has an initializer that will be hoisted
  • When hoisting occurs, include all instance fields (including # private) in the hoisting to preserve execution order
  • When no public fields are being hoisted, private fields stay as class field declarations

No changes needed to the decorator/legacy transform — the fix is entirely in transform_class_fields.

Fixes #21365

Test plan

  • Added test fixture: accessor-use-define-for-class-fields (with hoisted + non-hoisted cases)
  • Updated use-define-for-class-fields-without-class-properties expected output to match TypeScript behavior
  • cargo test -p oxc_transformer — all pass
  • cargo run -p oxc_transform_conformance — no regressions, previously failing test now passes

🤖 Generated with Claude Code

Copy link
Copy Markdown
Member Author

Dunqing commented Apr 13, 2026


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent changes, fast-track this PR to the front of the merge queue

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@github-actions github-actions Bot added A-transformer Area - Transformer / Transpiler C-bug Category - Bug labels Apr 13, 2026
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 13, 2026

Merging this PR will not alter performance

✅ 44 untouched benchmarks
⏩ 7 skipped benchmarks1


Comparing fix/transformer-accessor-use-define-for-class-fields (e645a94) with main (9fb2d9c)

Open in CodSpeed

Footnotes

  1. 7 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.

@Dunqing Dunqing changed the base branch from fix/allocator-clippy to graphite-base/21369 April 13, 2026 07:23
@Dunqing Dunqing force-pushed the graphite-base/21369 branch from 3619c61 to 345614b Compare April 13, 2026 07:23
@Dunqing Dunqing force-pushed the fix/transformer-accessor-use-define-for-class-fields branch from 22675c4 to 44345c0 Compare April 13, 2026 07:23
@Dunqing Dunqing changed the base branch from graphite-base/21369 to main April 13, 2026 07:23
@Dunqing Dunqing force-pushed the fix/transformer-accessor-use-define-for-class-fields branch from 44345c0 to 29902db Compare April 13, 2026 07:47
@github-actions github-actions Bot added the A-allocator Area - Allocator label Apr 13, 2026
@Dunqing Dunqing force-pushed the fix/transformer-accessor-use-define-for-class-fields branch 3 times, most recently from d975300 to e645a94 Compare April 14, 2026 06:40
@Dunqing Dunqing marked this pull request as ready for review April 14, 2026 07:48
@Dunqing Dunqing requested a review from overlookmotel as a code owner April 14, 2026 07:48
@Dunqing Dunqing requested a review from sapphi-red April 14, 2026 07:48
@Dunqing Dunqing changed the title fix(transformer): preserve execution order for accessor with useDefineForClassFields: false fix(transformer/transformer): preserve execution order for accessor with useDefineForClassFields: false Apr 14, 2026
@Dunqing Dunqing changed the title fix(transformer/transformer): preserve execution order for accessor with useDefineForClassFields: false fix(transformer/typescript): preserve execution order for accessor with useDefineForClassFields: false Apr 14, 2026
@Dunqing Dunqing added 0-merge Merge with Graphite Merge Queue and removed A-allocator Area - Allocator labels Apr 15, 2026
Copy link
Copy Markdown
Member Author

Dunqing commented Apr 15, 2026

Merge activity

…th `useDefineForClassFields: false` (#21369)

## Summary

When `useDefineForClassFields: false` (`setPublicClassFields` assumption), the TypeScript transform moves public/TS-private field initializers into the constructor. However, private field initializers (including `accessor` backing fields from the legacy decorator transform) were left as class field declarations, causing them to execute **before** the constructor body. This broke execution order when the initializer depended on other fields that were already moved to the constructor.

**Input:**
```ts
class Hello {
    private input = { foo };
    accessor util = this.input.foo();
}
```

**Before (broken):** `this.input.foo()` runs before `this.input = { foo }`
```js
class Hello {
    constructor() { this.input = { foo }; }
    #_util_accessor_storage = this.input.foo(); // ← runs BEFORE constructor
}
```

**After (fixed):** all instance initializers in the constructor, in source order
```js
class Hello {
    constructor() {
        this.input = { foo };
        this.#_util_accessor_storage = this.input.foo();
    }
    #_util_accessor_storage;
}
```

### Approach

Matches TypeScript's [`WillHoistInitializersToConstructor`](https://github.com/microsoft/TypeScript/blob/7b8cb3bdf82f400642b73173f941335775d6f730/src/compiler/transformers/classFields.ts#L339) logic in [`shouldTransformAutoAccessorsInCurrentClass`](https://github.com/microsoft/TypeScript/blob/7b8cb3bdf82f400642b73173f941335775d6f730/src/compiler/transformers/classFields.ts#L1069-L1073):

- Pre-scan the class body to check if any non-private instance field has an initializer that will be hoisted
- When hoisting occurs, include **all** instance fields (including `#` private) in the hoisting to preserve execution order
- When no public fields are being hoisted, private fields stay as class field declarations

No changes needed to the decorator/legacy transform — the fix is entirely in `transform_class_fields`.

Fixes #21365

## Test plan

- [x] Added test fixture: `accessor-use-define-for-class-fields` (with hoisted + non-hoisted cases)
- [x] Updated `use-define-for-class-fields-without-class-properties` expected output to match TypeScript behavior
- [x] `cargo test -p oxc_transformer` — all pass
- [x] `cargo run -p oxc_transform_conformance` — no regressions, previously failing test now passes

🤖 Generated with [Claude Code](https://claude.ai/code)
@graphite-app graphite-app Bot force-pushed the fix/transformer-accessor-use-define-for-class-fields branch from e645a94 to 4fb73a7 Compare April 15, 2026 01:22
@graphite-app graphite-app Bot merged commit 4fb73a7 into main Apr 15, 2026
26 checks passed
@graphite-app graphite-app Bot removed the 0-merge Merge with Graphite Merge Queue label Apr 15, 2026
@graphite-app graphite-app Bot deleted the fix/transformer-accessor-use-define-for-class-fields branch April 15, 2026 01:26
Dunqing added a commit that referenced this pull request Apr 16, 2026
### 💥 BREAKING CHANGES

- 24fb7eb allocator: [**BREAKING**] Rename `Box` and `Vec` methods
(#21395) (overlookmotel)

### 🚀 Features

- ce5072d parser: Support `turbopack` magic comments (#20803) (Kane
Wang)
- f5deb55 napi/transform: Expose `optimizeConstEnums` and
`optimizeEnums` options (#21388) (Dunqing)
- 24b03de data_structures: Introduce `NonNullConst` and `NonNullMut`
pointer types (#21425) (overlookmotel)

### 🐛 Bug Fixes

- d7a359a ecmascript: Treat update expressions as unconditionally
side-effectful (#21456) (Dunqing)
- 56af2f4 transformer/async-to-generator: Correct scope of inferred
named FE in async-to-generator (#21458) (Dunqing)
- b3ed467 minifier: Avoid illegal `var;` when folding unused arguments
copy loop (#21421) (fazba)
- b0e8f13 minifier: Preserve `var` inside `catch` with same-named
parameter (#21366) (Dunqing)
- 4fb73a7 transformer/typescript: Preserve execution order for accessor
with `useDefineForClassFields: false` (#21369) (Dunqing)

### ⚡ Performance

- da3cc16 parser: Refactor out `LexerContext` (#21275) (Ulrich Stark)

### 📚 Documentation

- c5b19bb allocator: Reformat comments in `Arena` (#21448)
(overlookmotel)
- 091e88e lexer: Update doc comment about perf benefit of reading
through references (#21423) (overlookmotel)
- 922cbee allocator: Remove references to "bump" from comments (#21397)
(overlookmotel)

Co-authored-by: Dunqing <29533304+Dunqing@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-transformer Area - Transformer / Transpiler C-bug Category - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

transformer: lowering accessor + useDefineForClassFields: false outputs code with incorrect execution order

2 participants