Skip to content

fix(transformer/typescript): emit class fields for parameter properties#21831

Closed
Dunqing wants to merge 1 commit intomainfrom
fix/transformer-typescript-parameter-property-fields
Closed

fix(transformer/typescript): emit class fields for parameter properties#21831
Dunqing wants to merge 1 commit intomainfrom
fix/transformer-typescript-parameter-property-fields

Conversation

@Dunqing
Copy link
Copy Markdown
Member

@Dunqing Dunqing commented Apr 27, 2026

Summary

Fixes a long-standing inconsistency in the TypeScript transformer's handling of constructor parameter properties under the documented default (useDefineForClassFields: true per compiler_assumptions.rs:137-139).

The transformer already emitted TS-true-aligned output for declared fields (kept boom: number; as boom;) but silently dropped the field declaration for parameter properties, so downstream tools that re-analyze the transformed AST saw the property only as a this.* write, not as a declared class field.

// input
class Foo {
  boom: number;                  // declared field
  constructor(public foo) {}     // parameter property
}
// before — inconsistent
class Foo {
  boom;                          // ✅ kept (TS-true)
  constructor(foo) {             // ❌ missing `foo;` (TS-false-style for params only)
    this.foo = foo;
  }
}

// after — consistent
class Foo {
  foo;                           // ✅ now matches `boom;`
  boom;
  constructor(foo) {
    this.foo = foo;
  }
}

This is not a policy change — the documented default has always been useDefineForClassFields: true (set both set_public_class_fields and remove_class_fields_without_initializer to true to opt into false semantics). The fix just closes the missing case for parameter properties.

Implementation

  • Add add_parameter_property_fields to the enter_class path that runs when set_public_class_fields is false, mirroring the existing branch that calls transform_class_fields when it is true.
  • Skip parameter properties whose name matches an existing instance field on the same class (matches tsc's dedup). Static, private, and declare-only fields are different bindings, so they don't suppress the new field.
  • Early-bail before allocating the dedup set when the class has no parameter properties — keeps the transformer-allocations snapshot at baseline.

No new option is added; the existing set_public_class_fields assumption already maps cleanly to useDefineForClassFields (the playground's useDefineForClassFields: true already toggles set_public_class_fields = false).

Closes #21600
Closes #21831

Copy link
Copy Markdown
Member Author

Dunqing commented Apr 27, 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.

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 27, 2026

Merging this PR will not alter performance

✅ 44 untouched benchmarks
⏩ 7 skipped benchmarks1


Comparing fix/transformer-typescript-parameter-property-fields (bf76f6c) with main (64a8180)

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 force-pushed the fix/transformer-typescript-parameter-property-fields branch 3 times, most recently from 8751841 to 4a64d7e Compare April 27, 2026 08:30
@Dunqing
Copy link
Copy Markdown
Member Author

Dunqing commented Apr 27, 2026

Overrode two Babel test outputs to match oxc's new output. They are correct — the reason is that oxc defaults to useDefineForClassFields: true while Babel's transform-typescript plugin defaults to useDefineForClassFields: false, so the field declarations (x;, y;, z;) appear in oxc's output but not Babel's. Same pattern as the ~20 other overrides in tasks/transform_conformance/overrides/ where oxc deliberately differs from Babel.

@Dunqing Dunqing marked this pull request as ready for review April 27, 2026 09:25
@Dunqing Dunqing requested a review from overlookmotel as a code owner April 27, 2026 09:25
@Dunqing Dunqing requested review from sapphi-red and removed request for overlookmotel April 27, 2026 09:49
@Dunqing Dunqing marked this pull request as draft April 27, 2026 09:49
@Dunqing Dunqing force-pushed the fix/transformer-typescript-parameter-property-fields branch 2 times, most recently from c0fa4c3 to bf76f6c Compare April 28, 2026 08:44
@Dunqing Dunqing marked this pull request as ready for review April 28, 2026 08:48
@graphite-app graphite-app Bot added the 0-merge Merge with Graphite Merge Queue label Apr 30, 2026
@graphite-app
Copy link
Copy Markdown
Contributor

graphite-app Bot commented Apr 30, 2026

Merge activity

…es (#21831)

## Summary

Fixes a long-standing inconsistency in the TypeScript transformer's handling of constructor parameter properties under the documented default (`useDefineForClassFields: true` per [`compiler_assumptions.rs:137-139`](https://github.com/oxc-project/oxc/blob/main/crates/oxc_transformer/src/compiler_assumptions.rs#L137-L139)).

The transformer already emitted TS-true-aligned output for **declared** fields (kept `boom: number;` as `boom;`) but silently dropped the field declaration for **parameter properties**, so downstream tools that re-analyze the transformed AST saw the property only as a `this.*` write, not as a declared class field.

```ts
// input
class Foo {
  boom: number;                  // declared field
  constructor(public foo) {}     // parameter property
}
```

```js
// before — inconsistent
class Foo {
  boom;                          // ✅ kept (TS-true)
  constructor(foo) {             // ❌ missing `foo;` (TS-false-style for params only)
    this.foo = foo;
  }
}

// after — consistent
class Foo {
  foo;                           // ✅ now matches `boom;`
  boom;
  constructor(foo) {
    this.foo = foo;
  }
}
```

This is **not a policy change** — the documented default has always been `useDefineForClassFields: true` (set both `set_public_class_fields` and `remove_class_fields_without_initializer` to `true` to opt into `false` semantics). The fix just closes the missing case for parameter properties.

## Implementation

- Add `add_parameter_property_fields` to the `enter_class` path that runs when `set_public_class_fields` is `false`, mirroring the existing branch that calls `transform_class_fields` when it is `true`.
- Skip parameter properties whose name matches an existing instance field on the same class (matches tsc's dedup). Static, private, and `declare`-only fields are different bindings, so they don't suppress the new field.
- Early-bail before allocating the dedup set when the class has no parameter properties — keeps the transformer-allocations snapshot at baseline.

No new option is added; the existing `set_public_class_fields` assumption already maps cleanly to `useDefineForClassFields` (the playground's `useDefineForClassFields: true` already toggles `set_public_class_fields = false`).

Closes #21600
Closes #21831
@graphite-app graphite-app Bot force-pushed the fix/transformer-typescript-parameter-property-fields branch from bf76f6c to c59c199 Compare April 30, 2026 00:39
@graphite-app graphite-app Bot closed this in c59c199 Apr 30, 2026
@graphite-app graphite-app Bot removed the 0-merge Merge with Graphite Merge Queue label Apr 30, 2026
camc314 added a commit that referenced this pull request May 5, 2026
### 💥 BREAKING CHANGES

- 0ffbe0d allocator: [**BREAKING**] Remove `Allocator::end_ptr` method
(#21871) (overlookmotel)

### 🚀 Features

- 9593ec8 transformer/jsx: Add jsxDEV source metadata for fragments
(#21932) (Ido Rosenthal)

### 🐛 Bug Fixes

- 429deac napi/parser: Export `visitorKeys` from `wasm` entrypoint
(#21996) (NullVoxPopuli)
- e852911 codegen: Preserve legal comments orphaned by upstream passes
(#21575) (Dunqing)
- e3399ec transformer/class-properties: Preserve RHS in
logical-assignment to static private field (#21950) (Dunqing)
- c59c199 transformer/typescript: Emit class fields for parameter
properties (#21831) (Dunqing)
- aaabde4 parser: Attach legal comments to following token (#21670)
(Dunqing)

### ⚡ Performance

- 0bf0cb9 allocator: Per-platform `Arena::new_fixed_size`
implementations (#22088) (overlookmotel)

### 📚 Documentation

- 62ec410 allocator: Correct doc comment for `Allocator::from_raw_parts`
(#22093) (overlookmotel)
- 3e152c6 allocator: Correct typos in comments (#22092) (overlookmotel)
- e220855 allocator: Correct doc comment for `Allocator::set_cursor_ptr`
(#21866) (overlookmotel)

---------

Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
Co-authored-by: Cameron Clark <cameron.clark@hey.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

transformer: parameter properties should be affected by useDefineForClassFields

1 participant