Emit defineProperty calls before param prop assignments

[sandersn]

Note that I restricted this to when --useDefineForClassFields is true. Nothing changes when it's off. I think this is the correct fix for a patch release.

However, in principal there's nothing wrong with moving parameter property initialisation after property declaration initialisation. It would be Extremely Bad and Wrong to rely on this working:

class C {
  p = this.q // q is already initialised here!
  constructor(public q: number) { }
}

But today it emits the below code, and probably somebody relies on it without knowing:

class C {
    constructor(q) {
        this.q = q;
        this.p = p;
    }
}

Fixes #34942

[ajafff]

probably somebody relies on it

👋 a lot of code in my current project relies on this behavior.
IIRC the order is documented somewhere so changing it would be a huge breaking change.

[sandersn]

@ajafff could it be in the spec? I'll have to take a look. If that's the case the long-term fix would need to be an interleaving of parameter property defineProperty/assignments, all happening before the defineProperty calls for other property declarations.

[sandersn]

Or, after discussing with @RyanCavanaugh, removing the assignments and using the parameter as the initial value of the defineProperty call.

[Jessidhia]

That'd work as a quick fix for now, but I'd like to raise an issue with the semantics of parameter properties to begin with. The way it's specified for TypeScript is insufficiently clear, and the most direct interpretation of it actually means it should, in your first class C snippet, raise a "use before initialization" error in p = this.q with --strictNullChecks.

I wrote this in a bug report that got closed as a duplicate:

This depends on how the parameter property is going to be interpreted. If it's still going to be interpreted as the same as writing an assignment in the constructor's body, then it needs to be reordered to happen after all of the Object.defineProperty initializers (as they technically run before super() returns, or before the constructor block begins evaluating for non-subclassing classes), and bar's initializer should detect this.foo as used before initialization.

If they're going to be "as powerful as" actual class fields, then constructor parameter properties must be hoisted higher than any actual class field initializers. Even with this latter interpretation, which is almost what happened, the this.foo = foo assignment is incorrect and should be done either immediately after the Object.defineProperty for foo runs, or the Object.defineProperty should use the constructor parameter as value initializer.

But for the spec issue itself and how to handle it going forward, should I open a new issue? This is important for @babel/transform-typescript purposes as well, and if you opt for the "backwards compatible" way we run into a conflict with the ecma262 initialization order.

[sandersn]

@Jessidhia I think the right thing is to have Object.defineProperty use the constructor parameter as the value initializer. I think this resolves the conflict with ecma262 initialisation order.

Although this is complex enough that perhaps we should have a new issue to find out if we need changes to accomodate the class fields proposal.

[sandersn]

@rbuckton mind taking a look at this? This is the fix we talked about yesterday

@Jessidhia We decided that we won't make changes to parameter properties unless the TC39 committee considers a similar feature. The feature as specified is kind of weird and fits badly with the ECMAScript spec, but changing it will certainly break people. I don't think it's worthwhile to improve it unless we're forced to.

[ajafff]

We decided that we won't make changes to parameter properties unless the TC39 committee considers a similar feature. The feature as specified is kind of weird and fits badly with the ECMAScript spec, but changing it will certainly break people. I don't think it's worthwhile to improve it unless we're forced to.

Does that mean you always need to use Object.defineProperty for classes with parameter properties even with useDefineForClassFields: true, target: esnext? As far as I understand the spec, TypeScript is not able to place the parameter property initialisation before initialisation of (native) class fields.

[DanielRosenwasser]

🔔 reviewers can we get some eyes on this so we can get 3.7.3 out sooner?

[DanielRosenwasser]

@typescript-bot cherry-pick this to release-3.7

[typescript-bot]

Hey @DanielRosenwasser, I've opened #35242 for you.

[rbuckton]

You can just merge this with definePropertyES5.ts (and rename it?) and use this:

// @target: es5, esnext

[sandersn]

@typescript-bot cherry-pick this into release-3.7

[typescript-bot]

Hey @sandersn, I've opened #35303 for you.