Gh 41788 incorrect output for esprivate with nested class in esnext (#…

…42663) * If target:esnext,then useDefineForClassFields: true will now be the default. * Added error if a private identifier is used in a static a initializer if target:ESNext and useDefineForClassFields:false. * Added test for new useDefineForClassFields default and error message. * Fixed tests after changing the default of useDefineForClassFields to true for target esnext * Fixed code review suggestions. * Updated error message. * Added missing static check for the containing property. Fixed other code review issues.
microsoft · Apr 7, 2021 · 2484210 · 2484210
1 parent 2bb54dc
commit 2484210
Show file tree

Hide file tree

Showing 64 changed files with 1,109 additions and 54 deletions.
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
@@ -346,6 +346,7 @@ namespace ts {
         const compilerOptions = host.getCompilerOptions();
         const languageVersion = getEmitScriptTarget(compilerOptions);
         const moduleKind = getEmitModuleKind(compilerOptions);
+        const useDefineForClassFields = getUseDefineForClassFields(compilerOptions);
         const allowSyntheticDefaultImports = getAllowSyntheticDefaultImports(compilerOptions);
         const strictNullChecks = getStrictOptionValue(compilerOptions, "strictNullChecks");
         const strictFunctionTypes = getStrictOptionValue(compilerOptions, "strictFunctionTypes");
@@ -1501,7 +1502,7 @@ namespace ts {
                 }
                 else if (isParameterPropertyDeclaration(declaration, declaration.parent)) {
                     // foo = this.bar is illegal in esnext+useDefineForClassFields when bar is a parameter property
-                    return !(compilerOptions.target === ScriptTarget.ESNext && !!compilerOptions.useDefineForClassFields
+                    return !(compilerOptions.target === ScriptTarget.ESNext && useDefineForClassFields
                              && getContainingClass(declaration) === getContainingClass(usage)
                              && isUsedInFunctionOrInstanceProperty(usage, declaration));
                 }
@@ -1532,7 +1533,7 @@ namespace ts {
                 return true;
             }
             if (isUsedInFunctionOrInstanceProperty(usage, declaration)) {
-                if (compilerOptions.target === ScriptTarget.ESNext && !!compilerOptions.useDefineForClassFields
+                if (compilerOptions.target === ScriptTarget.ESNext && useDefineForClassFields
                     && getContainingClass(declaration)
                     && (isPropertyDeclaration(declaration) || isParameterPropertyDeclaration(declaration, declaration.parent))) {
                     return !isPropertyImmediatelyReferencedWithinDeclaration(declaration, usage, /*stopAtAnyPropertyDeclaration*/ true);
@@ -1680,7 +1681,7 @@ namespace ts {
                     case SyntaxKind.PropertyDeclaration:
                         // static properties in classes introduce temporary variables
                         if (hasStaticModifier(node)) {
-                            return target < ScriptTarget.ESNext || !compilerOptions.useDefineForClassFields;
+                            return target < ScriptTarget.ESNext || !useDefineForClassFields;
                         }
                         return requiresScopeChangeWorker((node as PropertyDeclaration).name);
                     default:
@@ -2098,7 +2099,7 @@ namespace ts {
 
             // Perform extra checks only if error reporting was requested
             if (nameNotFoundMessage) {
-                if (propertyWithInvalidInitializer && !(compilerOptions.target === ScriptTarget.ESNext && compilerOptions.useDefineForClassFields)) {
+                if (propertyWithInvalidInitializer && !(compilerOptions.target === ScriptTarget.ESNext && useDefineForClassFields)) {
                     // We have a match, but the reference occurred within a property initializer and the identifier also binds
                     // to a local variable in the constructor where the code will be emitted. Note that this is actually allowed
                     // with ESNext+useDefineForClassFields because the scope semantics are different.
@@ -24260,7 +24261,7 @@ namespace ts {
                     break;
                 case SyntaxKind.PropertyDeclaration:
                 case SyntaxKind.PropertySignature:
-                    if (hasSyntacticModifier(container, ModifierFlags.Static) && !(compilerOptions.target === ScriptTarget.ESNext && compilerOptions.useDefineForClassFields)) {
+                    if (hasSyntacticModifier(container, ModifierFlags.Static) && !(compilerOptions.target === ScriptTarget.ESNext && useDefineForClassFields)) {
                         error(node, Diagnostics.this_cannot_be_referenced_in_a_static_property_initializer);
                         // do not return here so in case if lexical this is captured - it will be reflected in flags on NodeLinks
                     }
@@ -27057,6 +27058,30 @@ namespace ts {
                 if (assignmentKind && lexicallyScopedSymbol && lexicallyScopedSymbol.valueDeclaration && isMethodDeclaration(lexicallyScopedSymbol.valueDeclaration)) {
                     grammarErrorOnNode(right, Diagnostics.Cannot_assign_to_private_method_0_Private_methods_are_not_writable, idText(right));
                 }
+
+                if (lexicallyScopedSymbol?.valueDeclaration && (compilerOptions.target === ScriptTarget.ESNext && !useDefineForClassFields)) {
+                    const lexicalClass = getContainingClass(lexicallyScopedSymbol.valueDeclaration);
+                    const parentStaticFieldInitializer = findAncestor(node, (n) => {
+                        if (n === lexicalClass) return "quit";
+                        if (isPropertyDeclaration(n.parent) && hasStaticModifier(n.parent) && n.parent.initializer === n && n.parent.parent === lexicalClass) {
+                            return true;
+                        }
+                        return false;
+                    });
+                    if (parentStaticFieldInitializer) {
+                        const parentStaticFieldInitializerSymbol = getSymbolOfNode(parentStaticFieldInitializer.parent);
+                        Debug.assert(parentStaticFieldInitializerSymbol, "Initializer without declaration symbol");
+                        const diagnostic = error(node,
+                            Diagnostics.Property_0_may_not_be_used_in_a_static_property_s_initializer_in_the_same_class_when_target_is_esnext_and_useDefineForClassFields_is_false,
+                            symbolName(lexicallyScopedSymbol));
+                        addRelatedInfo(diagnostic,
+                            createDiagnosticForNode(parentStaticFieldInitializer.parent,
+                                Diagnostics.Initializer_for_property_0,
+                                symbolName(parentStaticFieldInitializerSymbol))
+                        );
+                    }
+                }
+
                 if (isAnyLike) {
                     if (lexicallyScopedSymbol) {
                         return apparentType;
@@ -33134,7 +33159,7 @@ namespace ts {
                     // - The constructor declares parameter properties
                     //   or the containing class declares instance member variables with initializers.
                     const superCallShouldBeFirst =
-                        (compilerOptions.target !== ScriptTarget.ESNext || !compilerOptions.useDefineForClassFields) &&
+                        (compilerOptions.target !== ScriptTarget.ESNext || !useDefineForClassFields) &&
                         (some((<ClassDeclaration>node.parent).members, isInstancePropertyWithInitializerOrPrivateIdentifierProperty) ||
                          some(node.parameters, p => hasSyntacticModifier(p, ModifierFlags.ParameterPropertyModifier)));
 
@@ -37142,7 +37167,7 @@ namespace ts {
                                 Diagnostics._0_is_defined_as_a_property_in_class_1_but_is_overridden_here_in_2_as_an_accessor;
                             error(getNameOfDeclaration(derived.valueDeclaration) || derived.valueDeclaration, errorMessage, symbolToString(base), typeToString(baseType), typeToString(type));
                         }
-                        else if (compilerOptions.useDefineForClassFields) {
+                        else if (useDefineForClassFields) {
                             const uninitialized = derived.declarations?.find(d => d.kind === SyntaxKind.PropertyDeclaration && !(d as PropertyDeclaration).initializer);
                             if (uninitialized
                                 && !(derived.flags & SymbolFlags.Transient)

diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json
@@ -3312,6 +3312,14 @@
         "category": "Error",
         "code": 2809
     },
+    "Property '{0}' may not be used in a static property's initializer in the same class when 'target' is 'esnext' and 'useDefineForClassFields' is 'false'.": {
+        "category": "Error",
+        "code": 2810
+    },
+    "Initializer for property '{0}'": {
+        "category": "Error",
+        "code": 2811
+    },
 
     "Import declaration '{0}' is using private name '{1}'.": {
         "category": "Error",

diff --git a/src/compiler/transformers/classFields.ts b/src/compiler/transformers/classFields.ts
@@ -108,6 +108,7 @@ namespace ts {
         const resolver = context.getEmitResolver();
         const compilerOptions = context.getCompilerOptions();
         const languageVersion = getEmitScriptTarget(compilerOptions);
+        const useDefineForClassFields = getUseDefineForClassFields(compilerOptions);
 
         const shouldTransformPrivateElements = languageVersion < ScriptTarget.ESNext;
 
@@ -138,7 +139,7 @@ namespace ts {
         function transformSourceFile(node: SourceFile) {
             const options = context.getCompilerOptions();
             if (node.isDeclarationFile
-                || options.useDefineForClassFields && options.target === ScriptTarget.ESNext) {
+                || useDefineForClassFields && options.target === ScriptTarget.ESNext) {
                 return node;
             }
             const visited = visitEachChild(node, visitor, context);
@@ -340,7 +341,7 @@ namespace ts {
             // Create a temporary variable to store a computed property name (if necessary).
             // If it's not inlineable, then we emit an expression after the class which assigns
             // the property name to the temporary variable.
-            const expr = getPropertyNameExpressionIfNeeded(node.name, !!node.initializer || !!context.getCompilerOptions().useDefineForClassFields);
+            const expr = getPropertyNameExpressionIfNeeded(node.name, !!node.initializer || useDefineForClassFields);
             if (expr && !isSimpleInlineableExpression(expr)) {
                 getPendingExpressions().push(expr);
             }
@@ -829,7 +830,7 @@ namespace ts {
             if (hasStaticModifier(member) || hasSyntacticModifier(getOriginalNode(member), ModifierFlags.Abstract)) {
                 return false;
             }
-            if (context.getCompilerOptions().useDefineForClassFields) {
+            if (useDefineForClassFields) {
                 // If we are using define semantics and targeting ESNext or higher,
                 // then we don't need to transform any class properties.
                 return languageVersion < ScriptTarget.ESNext;
@@ -865,7 +866,6 @@ namespace ts {
         }
 
         function transformConstructorBody(node: ClassDeclaration | ClassExpression, constructor: ConstructorDeclaration | undefined, isDerivedClass: boolean) {
-            const useDefineForClassFields = context.getCompilerOptions().useDefineForClassFields;
             let properties = getProperties(node, /*requireInitializer*/ false, /*isStatic*/ false);
             if (!useDefineForClassFields) {
                 properties = filter(properties, property => !!property.initializer || isPrivateIdentifier(property.name));
@@ -1000,7 +1000,7 @@ namespace ts {
          */
         function transformProperty(property: PropertyDeclaration, receiver: LeftHandSideExpression) {
             // We generate a name here in order to reuse the value cached by the relocated computed name expression (which uses the same generated name)
-            const emitAssignment = !context.getCompilerOptions().useDefineForClassFields;
+            const emitAssignment = !useDefineForClassFields;
             const propertyName = isComputedPropertyName(property.name) && !isSimpleInlineableExpression(property.name.expression)
                 ? factory.updateComputedPropertyName(property.name, factory.getGeneratedNameForNode(property.name))
                 : property.name;

diff --git a/src/compiler/transformers/es2015.ts b/src/compiler/transformers/es2015.ts
@@ -1626,7 +1626,7 @@ namespace ts {
             const memberFunction = transformFunctionLikeToExpression(member, /*location*/ member, /*name*/ undefined, container);
             const propertyName = visitNode(member.name, visitor, isPropertyName);
             let e: Expression;
-            if (!isPrivateIdentifier(propertyName) && context.getCompilerOptions().useDefineForClassFields) {
+            if (!isPrivateIdentifier(propertyName) && getUseDefineForClassFields(context.getCompilerOptions())) {
                 const name = isComputedPropertyName(propertyName) ? propertyName.expression
                     : isIdentifier(propertyName) ? factory.createStringLiteral(unescapeLeadingUnderscores(propertyName.escapedText))
                     : propertyName;

diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts
@@ -6071,6 +6071,10 @@ namespace ts {
         return compilerOptions.allowJs === undefined ? !!compilerOptions.checkJs : compilerOptions.allowJs;
     }
 
+    export function getUseDefineForClassFields(compilerOptions: CompilerOptions): boolean {
+        return compilerOptions.useDefineForClassFields === undefined ? compilerOptions.target === ScriptTarget.ESNext : compilerOptions.useDefineForClassFields;
+    }
+
     export function compilerOptionsAffectSemanticDiagnostics(newOptions: CompilerOptions, oldOptions: CompilerOptions): boolean {
         return oldOptions !== newOptions &&
             semanticDiagnosticsOptionDeclarations.some(option => !isJsonEqual(getCompilerOptionValue(oldOptions, option), getCompilerOptionValue(newOptions, option)));

diff --git a/src/testRunner/unittests/transform.ts b/src/testRunner/unittests/transform.ts
@@ -176,6 +176,7 @@ namespace ts {
                 compilerOptions: {
                     target: ScriptTarget.ESNext,
                     newLine: NewLineKind.CarriageReturnLineFeed,
+                    useDefineForClassFields: false,
                 }
             }).outputText;
         });

diff --git a/tests/baselines/reference/classWithStaticFieldInParameterBindingPattern(target=esnext).js b/tests/baselines/reference/classWithStaticFieldInParameterBindingPattern(target=esnext).js
@@ -4,7 +4,6 @@
 
 //// [classWithStaticFieldInParameterBindingPattern.js]
 // https://github.com/microsoft/TypeScript/issues/36295
-((_a) => { var _b; var { [(_b = class {
-    },
-    _b.x = 1,
-    _b).x]: b = "" } = _a; })();
+(({ [class {
+    static x = 1;
+}.x]: b = "" }) => { })();
diff --git a/tests/baselines/reference/classWithStaticFieldInParameterInitializer(target=esnext).js b/tests/baselines/reference/classWithStaticFieldInParameterInitializer(target=esnext).js
@@ -4,7 +4,6 @@
 
 //// [classWithStaticFieldInParameterInitializer.js]
 // https://github.com/microsoft/TypeScript/issues/36295
-((b) => { var _a; if (b === void 0) { b = (_a = class {
-    },
-    _a.x = 1,
-    _a); } })();
+((b = class {
+    static x = 1;
+}) => { })();
diff --git a/tests/baselines/reference/privateNameAndStaticInitializer(target=esnext).js b/tests/baselines/reference/privateNameAndStaticInitializer(target=esnext).js
@@ -9,11 +9,7 @@ class A {
 
 //// [privateNameAndStaticInitializer.js]
 class A {
-    constructor() {
-        this.#foo = 1;
-        this.#prop = 2;
-    }
-    #foo;
-    #prop;
+    #foo = 1;
+    static inst = new A();
+    #prop = 2;
 }
-A.inst = new A();
diff --git a/tests/baselines/reference/privateNameComputedPropertyName1(target=esnext).js b/tests/baselines/reference/privateNameComputedPropertyName1(target=esnext).js
@@ -39,18 +39,15 @@ new A().test();
 
 //// [privateNameComputedPropertyName1.js]
 class A {
+    #a = 'a';
+    #b;
+    #c = 'c';
+    #d;
+    #e = '';
     constructor() {
-        this.#a = 'a';
-        this.#c = 'c';
-        this.#e = '';
         this.#b = 'b';
         this.#d = 'd';
     }
-    #a;
-    #b;
-    #c;
-    #d;
-    #e;
     test() {
         const data = { a: 'a', b: 'b', c: 'c', d: 'd', e: 'e' };
         const { [this.#a]: a, [this.#b]: b, [this.#c]: c, [this.#d]: d, [this.#e = 'e']: e, } = data;

diff --git a/tests/baselines/reference/privateNameComputedPropertyName2(target=esnext).js b/tests/baselines/reference/privateNameComputedPropertyName2(target=esnext).js
@@ -12,10 +12,7 @@ console.log(getX(new A));
 //// [privateNameComputedPropertyName2.js]
 let getX;
 class A {
-    constructor() {
-        this.#x = 100;
-    }
-    #x;
+    #x = 100;
     [(getX = (a) => a.#x, "_")]() { }
 }
 console.log(getX(new A));
diff --git a/tests/baselines/reference/privateNameComputedPropertyName3(target=esnext).js b/tests/baselines/reference/privateNameComputedPropertyName3(target=esnext).js
@@ -26,17 +26,14 @@ console.log(new Foo("NAME").getValue(100));
 
 //// [privateNameComputedPropertyName3.js]
 class Foo {
+    #name;
     constructor(name) {
         this.#name = name;
     }
-    #name;
     getValue(x) {
         const obj = this;
         class Bar {
-            constructor() {
-                this.#y = 100;
-            }
-            #y;
+            #y = 100;
             [obj.#name]() {
                 return x + this.#y;
             }