Skip to content

Commit c59c199

Browse files
DunqingDunqing
committed
fix(transformer/typescript): emit class fields for parameter properties (#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
1 parent c731a8b commit c59c199

11 files changed

Lines changed: 221 additions & 10 deletions

File tree

  • crates/oxc_transformer/src/typescript
  • tasks/transform_conformance
    • overrides/babel-plugin-transform-typescript/test/fixtures/class
    • snapshots
    • tests
      • babel-plugin-transform-typescript/test/fixtures
      • legacy-decorators/test/fixtures/oxc/metadata/abstract-class

‎crates/oxc_transformer/src/typescript/class.rs‎

Lines changed: 130 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
1+
use rustc_hash::FxHashSet;
2+
13
use oxc_allocator::{TakeIn, Vec as ArenaVec};
2-
use oxc_ast::ast::*;
4+
use oxc_ast::{NONE, ast::*};
35
use oxc_semantic::{ScopeFlags, ScopeId};
46
use oxc_span::SPAN;
57
use oxc_str::Ident;
@@ -25,9 +27,11 @@ impl<'a> TypeScript<'a> {
2527
/// after the super call in the constructor body.
2628
///
2729
/// Same as `Self::convert_constructor_params` does, the reason why we still need that method because
28-
/// this method only calls when `set_public_class_fields` is `true` and `class_properties` plugin is
29-
/// disabled, otherwise the `convert_constructor_params` method will be called. Merging them together
30-
/// will increase unnecessary check when only transform constructor parameters.
30+
/// this method only runs when `set_public_class_fields` is `true` and the `class_properties` plugin
31+
/// is disabled. The other branches (`class_properties` enabled, or `set_public_class_fields: false`
32+
/// — see [`Self::add_parameter_property_fields`]) reach the parameter-property assignments via
33+
/// `convert_constructor_params` directly. Merging them together would add unnecessary checks for
34+
/// the param-only paths.
3135
///
3236
/// 2. Convert class fields to `this` assignments in the constructor body.
3337
///
@@ -231,6 +235,128 @@ impl<'a> TypeScript<'a> {
231235
}
232236
}
233237

238+
/// Insert an uninitialized class field declaration for each constructor parameter property.
239+
///
240+
/// Aligns with TypeScript's output under `useDefineForClassFields: true`, which is the
241+
/// default since TypeScript 4.0. Runs when [`crate::CompilerAssumptions::set_public_class_fields`]
242+
/// is `false` (the inverse of the `useDefineForClassFields: false` mapping).
243+
///
244+
/// Input:
245+
/// ```ts
246+
/// class C {
247+
/// constructor(public x = 1) {}
248+
/// }
249+
/// ```
250+
///
251+
/// Output:
252+
/// ```js
253+
/// class C {
254+
/// x;
255+
/// constructor(x = 1) {
256+
/// this.x = x;
257+
/// }
258+
/// }
259+
/// ```
260+
///
261+
/// Skips parameter properties whose name matches an existing instance field declaration on
262+
/// the same class (matching TypeScript's dedup). Only static-identifier instance fields
263+
/// count — static, private, computed, and `declare`-only fields don't suppress the new field.
264+
pub(super) fn add_parameter_property_fields(class: &mut Class<'a>, ctx: &TraverseCtx<'a>) {
265+
let Some(params) = Self::find_constructor_params(class) else { return };
266+
267+
// Bail before allocating when there are no parameter properties to emit.
268+
// Parameter properties are TS-specific and uncommon, so most classes hit this path.
269+
if !params.iter().any(FormalParameter::has_modifier) {
270+
return;
271+
}
272+
273+
let mut declared_names = Self::collect_instance_field_names(class);
274+
let fields = ctx.ast.vec_from_iter(
275+
params
276+
.iter()
277+
.filter(|param| param.has_modifier())
278+
.filter_map(|param| param.pattern.get_binding_identifier())
279+
.filter(|id| declared_names.insert(id.name))
280+
.map(|id| Self::build_uninitialized_field(id, ctx)),
281+
);
282+
283+
class.body.body.splice(0..0, fields);
284+
}
285+
286+
/// Returns the body-bearing constructor's parameter list, or `None` if the class has none.
287+
fn find_constructor_params<'b>(
288+
class: &'b Class<'a>,
289+
) -> Option<&'b ArenaVec<'a, FormalParameter<'a>>> {
290+
class.body.body.iter().find_map(|element| {
291+
if let ClassElement::MethodDefinition(method) = element
292+
&& method.kind.is_constructor()
293+
&& method.value.body.is_some()
294+
{
295+
Some(&method.value.params.items)
296+
} else {
297+
None
298+
}
299+
})
300+
}
301+
302+
/// Collect names of instance field declarations keyed by a static identifier.
303+
///
304+
/// Static, private, computed, and `declare`-only fields don't suppress a parameter-property
305+
/// field — they're different bindings or erased before emit. Plain instance-field collisions
306+
/// are a TS `Duplicate identifier` error, so this dedup is mostly defensive.
307+
fn collect_instance_field_names(class: &Class<'a>) -> FxHashSet<Ident<'a>> {
308+
class
309+
.body
310+
.body
311+
.iter()
312+
.filter_map(|element| match element {
313+
ClassElement::PropertyDefinition(prop)
314+
if !prop.r#static
315+
&& !prop.declare
316+
&& let PropertyKey::StaticIdentifier(ident) = &prop.key =>
317+
{
318+
Some(ident.name)
319+
}
320+
ClassElement::AccessorProperty(prop)
321+
if !prop.r#static
322+
&& let PropertyKey::StaticIdentifier(ident) = &prop.key =>
323+
{
324+
Some(ident.name)
325+
}
326+
_ => None,
327+
})
328+
.collect()
329+
}
330+
331+
/// Build `<id.name>;` — an uninitialized public class field declaration.
332+
///
333+
/// Safe to read `id.name` directly here — `add_parameter_property_fields` is gated on
334+
/// `!is_class_properties_plugin_enabled`, so no rename has happened.
335+
/// `convert_constructor_params` uses the span trick because it also runs alongside
336+
/// class-properties.
337+
fn build_uninitialized_field(
338+
id: &BindingIdentifier<'a>,
339+
ctx: &TraverseCtx<'a>,
340+
) -> ClassElement<'a> {
341+
let key = ctx.ast.property_key_static_identifier(id.span, id.name);
342+
ctx.ast.class_element_property_definition(
343+
id.span,
344+
PropertyDefinitionType::PropertyDefinition,
345+
ctx.ast.vec(),
346+
key,
347+
NONE,
348+
None,
349+
false,
350+
false,
351+
false,
352+
false,
353+
false,
354+
false,
355+
false,
356+
None,
357+
)
358+
}
359+
234360
pub(super) fn transform_class_on_exit(&self, class: &mut Class<'a>, ctx: &TraverseCtx<'a>) {
235361
if !self.remove_class_fields_without_initializer {
236362
return;

‎crates/oxc_transformer/src/typescript/mod.rs‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -120,8 +120,15 @@ impl<'a> Traverse<'a, TransformState<'a>> for TypeScript<'a> {
120120

121121
// Avoid converting class fields when class-properties plugin is enabled, that plugin has covered all
122122
// this transformation does.
123-
if !self.is_class_properties_plugin_enabled && self.set_public_class_fields {
124-
self.transform_class_fields(class, ctx);
123+
if !self.is_class_properties_plugin_enabled {
124+
if self.set_public_class_fields {
125+
self.transform_class_fields(class, ctx);
126+
} else {
127+
// Default oxc behavior matches `useDefineForClassFields: true`. Emit an uninitialized
128+
// field declaration for each constructor parameter property so downstream `[[Define]]`
129+
// semantics see the property, matching TypeScript's output.
130+
Self::add_parameter_property_fields(class, ctx);
131+
}
125132
}
126133
}
127134

‎tasks/transform_conformance/overrides/babel-plugin-transform-typescript/test/fixtures/class/parameter-properties-with-super/output.js‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
class C extends Object {
2+
x;
3+
constructor(x) {
4+
super();
5+
this.x = x;
6+
}
7+
}

‎tasks/transform_conformance/overrides/babel-plugin-transform-typescript/test/fixtures/class/parameter-properties/output.js‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
class C {
2+
x;
3+
y;
4+
z;
5+
constructor(x, y = 0, z = 0) {
6+
this.x = x;
7+
this.y = y;
8+
this.z = z;
9+
}
10+
}

‎tasks/transform_conformance/snapshots/oxc.snap.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
commit: c543b031
22

3-
Passed: 222/369
3+
Passed: 224/371
44

55
# All Passed:
66
* babel-plugin-transform-class-static-block
@@ -69,7 +69,7 @@ after transform: SymbolId(0): [ReferenceId(0), ReferenceId(2), ReferenceId(6), R
6969
rebuilt : SymbolId(0): [ReferenceId(0), ReferenceId(2), ReferenceId(6), ReferenceId(10)]
7070

7171

72-
# babel-plugin-transform-typescript (17/50)
72+
# babel-plugin-transform-typescript (19/52)
7373
* allow-declare-fields-false/input.ts
7474
Unresolved references mismatch:
7575
after transform: ["dce"]

‎tasks/transform_conformance/tests/babel-plugin-transform-typescript/test/fixtures/class-constructor-arguments-with-declared-fields/input.ts‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
// Static and private fields with the same name as a constructor parameter
2+
// property are different bindings, so the parameter property still emits its
3+
// own field declaration. (Same-name plain instance fields would be a
4+
// `Duplicate identifier` error in TypeScript, so dedup against those is
5+
// purely defensive.)
6+
export class WithStaticSameName {
7+
static x = 0;
8+
constructor(public x = 1) {}
9+
}
10+
11+
export class WithPrivateSameName {
12+
#x = 0;
13+
constructor(public x = 1) {}
14+
15+
read() {
16+
return this.#x;
17+
}
18+
}

‎tasks/transform_conformance/tests/babel-plugin-transform-typescript/test/fixtures/class-constructor-arguments-with-declared-fields/output.js‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
export class WithStaticSameName {
2+
x;
3+
static x = 0;
4+
constructor(x = 1) {
5+
this.x = x;
6+
}
7+
}
8+
export class WithPrivateSameName {
9+
x;
10+
#x = 0;
11+
constructor(x = 1) {
12+
this.x = x;
13+
}
14+
read() {
15+
return this.#x;
16+
}
17+
}

‎tasks/transform_conformance/tests/babel-plugin-transform-typescript/test/fixtures/class-constructor-arguments-with-default-value/input.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
// Repro of issue #21600: parameter property with default value should still
2+
// emit an uninitialized class field declaration to match TypeScript's
3+
// `useDefineForClassFields: true` output.
4+
class Foo {
5+
constructor(public b1 = 2.1) {
6+
}
7+
}

‎tasks/transform_conformance/tests/babel-plugin-transform-typescript/test/fixtures/class-constructor-arguments-with-default-value/output.js‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
class Foo {
2+
b1;
3+
constructor(b1 = 2.1) {
4+
this.b1 = b1;
5+
}
6+
}

‎tasks/transform_conformance/tests/babel-plugin-transform-typescript/test/fixtures/class-constructor-arguments/output.js‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,8 @@
11
class Foo {
2+
foo;
3+
bar;
4+
zoo;
5+
bang;
26
boom;
37
constructor(foo, bar, zoo, bang, too) {
48
this.foo = foo;
@@ -8,8 +12,12 @@ class Foo {
812
console.log(this.foo, this.bar, this.zoo, this.bang);
913
}
1014
}
11-
1215
class Bar extends Foo {
16+
foo;
17+
bar;
18+
zoo;
19+
bang;
20+
boom;
1321
constructor(foo, bar, zoo, bang, boom, too) {
1422
super(foo, bar, zoo, bang, too);
1523
this.foo = foo;
@@ -19,8 +27,12 @@ class Bar extends Foo {
1927
this.boom = boom;
2028
}
2129
}
22-
2330
class Baz extends Bar {
31+
foo;
32+
bar;
33+
zoo;
34+
bang;
35+
boom;
2436
constructor(foo, bar, zoo, bang, boom, too) {
2537
this.foo = foo;
2638
this.bar = bar;

0 commit comments

Comments
 (0)