Incorrect output for #private with nested class in ESNext

[DanielRosenwasser]

Found from #41773 (comment)

// @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 @robpalme

[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

[a-tarasyuk]

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

[a-tarasyuk]

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;

[sandersn]

I like @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.

[dragomirtitian]

@sandersn I would like to pick this up if that's ok. Just to clear on the statement of work:

1. If target:esnext,then useDefineForClassFields: true will now be the default.
2. If target:esnext, useDefineForClassFields: true we emit correct code so no further action is needed (ex)
3. 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."