useDefineWithClassFields should use-before-init error when class property initializer refers to parameter property

[dilame]

Bug Report

This bug exists in my project, not only playground, but playground seem to also have bugs, because i can't even seem to reliably reproduce it in the playground – sometimes it does not react on Target change. But right now i have an opened playground tab with this bug.

I will duplicate the MRE code right here as a text.
This is the source code

class Helper {
    create(): boolean {
        return true
    }
}

export class Broken {

    constructor(readonly facade: Helper) {
        console.log(this.bug)
    }
    bug = this.facade.create()

}

new Broken(new Helper)

This is the compiled code with target: ES2022 and module: CommonJS

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.Broken = void 0;
class Helper {
    create() {
        return true;
    }
}
class Broken {
    facade;
    constructor(facade) {
        this.facade = facade;
        console.log(this.bug);
    }
    bug = this.facade.create();
}
exports.Broken = Broken;
new Broken(new Helper);

If run this JS – we will get TypeError

Cannot read properties of undefined (reading 'create') 

⏯ Playground Link

Playground link with relevant code

I'm also sharing the playground link, but i assume it could produce the right code for you if you open it first time.
To reliable reproduce – change module to Node16 and then to CommonJS. You should see this result

[DanielRosenwasser]

I believe that this is due to #36425

@sandersn why don't we give an error for referencing a parameter property in a field initializer when useDefineForClassFields is set?

[dilame]

Is target: ES2022 supposed to produce such code? If i downgrade to target: ES2021 the problem disappears

[DanielRosenwasser]

In newer targets, useDefineForClassFields is set to true because it's spec-compliant. To be honest though, even I'm somewhat surprised that we changed the initialization ordering.

[fatcerberus]

even I'm somewhat surprised that we changed the initialization ordering

It’s impossible for facade to be initialized before bug in this code unless the compiler transpiles bug to a constructor-initialized property (as it does pre-ES2022).

[RyanCavanaugh]

The code generation here is as-expected; the bug is the lack of error message to flag the use-before-init.

[fatcerberus]

this.facade = facade;

Not directly related to the issue but if target is ES2022 (which implies useDefineForClassFields: true) then why isn't this being initialized with defineProperty? Are parameter properties exempt?

[RyanCavanaugh]

Parameter properties are considered to be sugar for a this.e = e assignment in the constructor.

[dilame]

The code generation here is as-expected

Doesn't it reduces readability? I was so happy TypeScript handles initialization ordering in constructor for me.
Now i will be forced to rewrite all such parts in the whole project with less accurate code. At least, as i understand, it requires 2 lines of code instead of 1. One line with field type declaration, and second line with value assignment in constructor.

[rbuckton]

It looks like our expectation (prior to useDefineForClassFields), was that parameter property initializers are assigned before instance initializers, as evidenced by the emit when useDefineForClassFields is false:

    constructor(facade) {
        this.facade = facade;
        this.bug = this.facade.create();
        console.log(this.bug);
    }

I would contend that the issue is our emit when useDefineForClassFields is true isn't correctly moving the field initializers into the constructor. Really, the emit should be this:

    facade;
    bug;
    constructor(facade) {
        this.facade = facade;
        this.bug = this.facade.create();
        console.log(this.bug);
    }

[AlCalzone]

FWIW, babel also produces code where this is a runtime error.
If this is the desired initialization order, then it should be a compiler error.

[Azbesciak]

Hi, anything guys? I also faced that, to say more we have ngrx code, which declares multiple effects in a class based on received in the constructor values, but these declared properties are not declared in the constructor. That is overkill.

From other fields, like kotlin or scala - you have a constructor, and later you can use these values from the constructor, to create other properties.

That should be obvious, and IMO it was for many users before ES2022.