Incorrect output for #private with nested class in ESNext

Rob Palmer (@robpalme)

// @target: esnext

class A {
    #str = 'pr Str'
    constructor() {
        new A.B(this)
    }
    private static B = class {
        constructor(private a: A) {
            console.log(this.a.#str)
        }
    }
}
test: {
    new A
}

Currently, this is emitted as

"use strict";
class A {
    constructor() {
        this.#str = 'pr Str';
        new A.B(this);
    }
    #str;
}
A.B = class {
    constructor(a) {
        this.a = a;
        console.log(this.a.#str);
    }
};
test: {
    new A;
}

However, #str has been orphaned and this code will have errors.

CC Rob Palmer (@robpalme)

Thanks for highlighting this. I played around and reduced it to a slightly simpler case in the playground.

// @target: esnext

class A {
    #x : undefined;
    static B = class {
        constructor() {
            new A().#x;
        }
    }
}

...produces the following incorrect ESnext emit...

class A {
    #x;
}
A.B = class {
    constructor() {
        new A().#x;
    }
};

...whereas desired ESnext emit should retain the static field initialization inside the class...

class A {
    #x;
    static B = class {
        constructor() {
            new A().#x;
        }
    }
}

The bug does not occur when down-levelling because access to the private field happily goes via the module-scoped WeakMap.
The bug does not occur when enabling useDefineForClassFields because the static field initialization remains inside the class.

Making the static field assignment work without useDefineForClassFields might be possible if we sneakily export a getter.

class A {
    #x;
    static __bornToDie = t=>t.#x;
}
var __get_A_X = A.__bornToDie;
delete A.__bornToDie;
A.B = class {
    constructor() {
        __get_A_X(new A());
    }
};

But this feels like a lot of work for the matter at hand. Maybe the simplest solution would be to make target:ESnext imply useDefineForClassFields:true to side-step the problem entirely. Alternatively, an easy/quick solution to achieve correctness would be to degrade the target:ESnext && useDefineForClassFields:false case to use down-levelling.

Until this is fixed, users of target:ESNext can workaround this by enabling useDefineForClassFields

Daniel Rosenwasser (@DanielRosenwasser)

Daniel Rosenwasser (@DanielRosenwasser) I suppose static methods/properties should be transformed because the proposal is at Stage 3. Am I right?

Why I asked this question, maybe it doesn't make sense, https://github.com/tc39/proposal-class-fields is at Stage 3 too, and TS emits Private fields without polyfills. Example

class A {
    #x = undefined;
}


// .js
"use strict";
class A {
    constructor() {
        this.#x = undefined;
    }
    #x;
}

However, TS downgrades static methods/properties. Example

class Foo {
  static x = 1;
}

// .js
"use strict";
class Foo {
}
Foo.x = 1;

Rob Palmer (@robpalme)

I like Rob Palmer (@robpalme)'s idea of ESNext implying useDefineForClassFields; the emit is simpler and spec-standard class properties are widely enough supported that people overusing target:esnext [1] are unlikely to run into problems.

For the explicit combination of (1) target:esnext (2) useDefineForClassFields: false (3) nested static class with private reference, we should add an error that says "you can't do this, change useDefineForClassFields: true, or target an earlier version of ES."

[1] for example, somebody who just wants to use every feature in JS without the compiler doing any downlevelling, but who hasn't thought too much about which browser will.

Nathan Shively-Sanders (@sandersn)

Nathan Shively-Sanders (@sandersn) I would like to pick this up if that's ok. Just to clear on the statement of work:

If target:esnext,then useDefineForClassFields: true will now be the default.
If target:esnext, useDefineForClassFields: true we emit correct code so no further action is needed (ex)
If target:esnext, useDefineForClassFields: false and we encounter a usage of a #private field in a static member we emit an error like "You can't do this, change useDefineForClassFields: true, or target an earlier version of ES."

Merge remote-tracking branch 'remotes/origin/master' into microsoftGH…

I would strongly discourage using delete, as that is a performance cliff in V8 and other engines and implementers have strongly advised not to do this.

One approach I've seen is to leverage a computed property name, if possible:

var B_1;
class A {
    #x;
    static [(B_1 = class {
        constructor() {
            new A().#x;
        }
    }, "B"]() {}
}
A.B = B_1;

This, of course, doesn't work if the class has no non-private members, in which case we might have to add an unused member, as you propose, but don't delete it.

var <temp variables>;
class A {
  #x;
  // NOTE: __tsfieldinit__ would be an optimistic unique name
  // NOTE: __tsfieldinit__ is a placeholder. It won't actually be evaluated.
  static [(<inlined expressions>, "__tsfieldinit__")]() {} 
}
<static assignments>
A.__tsfieldinit__ = void 0; // mark undefined, but otherwise leave as is. `delete` is terrible for performance...

NOTE: we could possibly abuse name in this case..., though I'm not sure that's much better:

var <temp variables>;
class A {
  #x;
  static [(<inlined expressions>, "name")]() {}
}
Object.defineProperty(A, "name", { value: "A", writable: false }); // fix up A.name
<static assignments>;

Rob Palmer (@robpalme)

I like Rob Palmer (@robpalme)'s idea of ESNext implying useDefineForClassFields; the emit is simpler and spec-standard class properties are widely enough supported that people overusing target:esnext [1] are unlikely to run into problems.

The downside of this is that it could be considered silent breaking change (if you previously used --target:esnext on its own, your class field emit changes without notification, which can lead to subtle breaks). If we were going to make a change, I'd suggest that --target esnext should make you explicitly define your choice for --useDefineForClassFields if you use class fields in your code (assuming you haven't set --noEmit), at least for a few releases (or a future TS 5.x) to make people aware of the change.

We do have other cases where we downlevel things like destructuring to the ES5 form even when targeting ES2015, particularly in the module transforms, which is why destructuring.ts is separate from the es2015.ts transform. We could consider forcing the downlevel private field emit when we encounter the combination.

Navigation Menu

Repository navigation

Incorrect output for #private with nested class in ESNext #41788

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Incorrect output for #private with nested class in ESNext #41788

Description

Activity

robpalme commented on Dec 5, 2020

a-tarasyuk commented on Dec 7, 2020

a-tarasyuk commented on Dec 9, 2020

sandersn commented on Jan 15, 2021

dragomirtitian commented on Jan 22, 2021

rbuckton commented on Mar 18, 2021

rbuckton commented on Mar 18, 2021

rbuckton commented on Mar 18, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions