Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

transformer: class constructor argument access modifier special behavior #4789

Closed
yyx990803 opened this issue Aug 9, 2024 · 8 comments · Fixed by #4808
Closed

transformer: class constructor argument access modifier special behavior #4789

yyx990803 opened this issue Aug 9, 2024 · 8 comments · Fixed by #4808
Assignees
Labels
A-transformer Area - Transformer / Transpiler C-bug Category - Bug

Comments

@yyx990803
Copy link

yyx990803 commented Aug 9, 2024

In TypeScript, the following class (case 1) reports a type error:

class Foo {
  x = this.foo // error: Property 'foo' is used before its initialization
  foo: any
  constructor(foo: any) {
    this.foo = foo
  }
}

Because it will be transformed to:

class Foo {
    constructor(foo) {
        this.x = this.foo; // <-- this.foo not assigned yet!
        this.foo = foo;
    }
}

TS Playground

However, if it uses access modifier on the constructor argument (case 2):

class Foo {
  x = this.foo
  constructor(public foo: any) {
    console.log(this.foo)
  }
}

It will work both in types and at runtime, because it will be transformed to:

class Foo {
    constructor(foo) {
        this.foo = foo; // <-- injected from argument
        this.x = this.foo; // <-- moved from field initializers
        console.log(this.foo);
    }
}

TS Playground

Notice how x = this.foo is moved into the constructor to be after the this.foo assignment.

Oxc's current behavior

Oxc currently transforms case 2 to:

class Foo {
  x = this.foo
  constructor(foo) {
    console.log(this.foo)
    this.foo = foo // <-- injected from argument
  }
}

Notice that both x = this.foo and console.log(this.foo) are executed before the this.foo assignment, both leading to runtime errors.

What needs to be fixed

  1. The generated assignment for constructor arguments with access modifiers should be injected to the top of the constructor, not the bottom.
  2. When constructor argument access modifiers are used, all class field initializers need to be moved into the constructor (so their initialization order is preserved), after the argument assignments, and before existing constructor code.

Related: Behavior with useDefineForClassFields: true

When useDefineForClassFields is set to true, case 2 will also throw a type error. And TS's transform out will become:

class Foo {
    foo;
    x = this.foo;
    constructor(foo) {
        this.foo = foo;
        console.log(this.foo);
    }
}

TS Playground

I'm not sure if oxc's TS transform currently takes this into account, but this is something we will need to consider.

@yyx990803 yyx990803 added C-bug Category - Bug A-transformer Area - Transformer / Transpiler labels Aug 9, 2024
@magic-akari
Copy link
Contributor

@Dunqing
Copy link
Member

Dunqing commented Aug 10, 2024

Thanks for the detailed issue!

The oxc transformer port from Babel, and the useDefineForClassFields: true is Babel's default behavior. so I think we don't need to do anything until microsoft/TypeScript#45995 is solved

@overlookmotel
Copy link
Contributor

overlookmotel commented Aug 10, 2024

#4808 fixes point (1) on the "What needs to be fixed" list above. But @Dunqing I'm not sure if we have more to do to satisfy point (2)?

overlookmotel pushed a commit that referenced this issue Aug 10, 2024
…uments with access modifiers should be injected to the top of the constructor (#4808)

fix: #4789
@Dunqing
Copy link
Member

Dunqing commented Aug 10, 2024

#4808 fixes point (1) on the "What needs to be fixed" list above. But @Dunqing I'm not sure if we have more to do to satisfy point (2)?

No, it doesn't. This one is handled correctly. After #4808 was merged, we can close this

@overlookmotel
Copy link
Contributor

#4808 fixes point (1) on the "What needs to be fixed" list above. But @Dunqing I'm not sure if we have more to do to satisfy point (2)?

No, it doesn't. This one is handled correctly. After #4808 was merged, we can close this

OK great. Do we have a test for the 2nd case TS Playground?

@robpalme
Copy link

It looks like you have this under control.

I wanted to mention that useDefineForClassFields: true is the modern default in TypeScript and aligns with JS semantics. It gets automatically enabled in TS when target: es2022 or target: esnext. Unfortunately in the TS playground UI, the config checkboxes are all independent, meaning selecting target: esnext does not immediately enable it, so this implication is not always obvious in the playground.

@Dunqing
Copy link
Member

Dunqing commented Aug 12, 2024

Thank you @robpalme explain this more!

@Dunqing
Copy link
Member

Dunqing commented Aug 12, 2024

OK great. Do we have a test for the 2nd case TS Playground?

If I understand correctly, the case should be like,
image

In the TypeScript, like this:
image

https://www.typescriptlang.org/play/?useDefineForClassFields=true&target=99#code/MYGwhgzhAEBiD29oG8BQ1rHgOwgFwCcBXYPeAgCgAciAjEAS2GgDcAuabIgW1oFMCAGmhUCDFmDx9OHLrwEBKFOgyYcEeCD4A6EPADmFPAAsGEbSwUqAvqmtA

The output has a difference with TypeScript. But it doesn’t matter, the output is consident with Babel

@Dunqing Dunqing closed this as completed Aug 12, 2024
github-merge-queue bot pushed a commit to rolldown/rolldown that referenced this issue Aug 12, 2024
### Description

A small update for

* oxc-project/oxc#4789
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
5 participants