For f.prototype.m = function() { this.x = 0; } make x a member of f, not of the function expression (#22643)

Author: Andy

src/compiler/binder.ts

@@ -119,6 +119,7 @@ namespace ts {
         let languageVersion: ScriptTarget;
         let parent: Node;
         let container: Node;
+        let containerContainer: Node; // Container one level up
         let blockScopeContainer: Node;
         let inferenceContainer: Node;
         let lastContainer: Node;
@@ -187,6 +188,7 @@ namespace ts {
             languageVersion = undefined;
             parent = undefined;
             container = undefined;
+            containerContainer = undefined;
             blockScopeContainer = undefined;
             inferenceContainer = undefined;
             lastContainer = undefined;
@@ -224,13 +226,7 @@ namespace ts {
             symbol.flags |= symbolFlags;
 
             node.symbol = symbol;
-
-            if (!symbol.declarations) {
-                symbol.declarations = [node];
-            }
-            else {
-                symbol.declarations.push(node);
-            }
+            symbol.declarations = append(symbol.declarations, node);
 
             if (symbolFlags & SymbolFlags.HasExports && !symbol.exports) {
                 symbol.exports = createSymbolTable();
@@ -486,8 +482,11 @@ namespace ts {
             // and block-container.  Then after we pop out of processing the children, we restore
             // these saved values.
             const saveContainer = container;
+            const saveContainerContainer = containerContainer;
             const savedBlockScopeContainer = blockScopeContainer;
 
+            containerContainer = container;
+
             // Depending on what kind of node this is, we may have to adjust the current container
             // and block-container.   If the current node is a container, then it is automatically
             // considered the current block-container as well.  Also, for containers that we know
@@ -580,7 +579,9 @@ namespace ts {
             else {
                 bindChildren(node);
             }
+
             container = saveContainer;
+            containerContainer = saveContainerContainer;
             blockScopeContainer = savedBlockScopeContainer;
         }
 
@@ -2327,14 +2328,23 @@ namespace ts {
 
         function bindThisPropertyAssignment(node: BinaryExpression | PropertyAccessExpression) {
             Debug.assert(isInJavaScriptFile(node));
-            const container = getThisContainer(node, /*includeArrowFunctions*/ false);
-            switch (container.kind) {
+            const thisContainer = getThisContainer(node, /*includeArrowFunctions*/ false);
+            switch (thisContainer.kind) {
                 case SyntaxKind.FunctionDeclaration:
                 case SyntaxKind.FunctionExpression:
+                    let constructorSymbol = thisContainer.symbol;
+                    // For `f.prototype.m = function() { this.x = 0; }`, `this.x = 0` should modify `f`'s members, not the function expression.
+                    if (isBinaryExpression(thisContainer.parent) && thisContainer.parent.operatorToken.kind === SyntaxKind.EqualsToken) {
+                        const l = thisContainer.parent.left;
+                        if (isPropertyAccessExpression(l) && isPropertyAccessExpression(l.expression) && l.expression.name.escapedText === "prototype" && isEntityNameExpression(l.expression.expression)) {
+                            constructorSymbol = getJSInitializerSymbolFromName(l.expression.expression, containerContainer);
+                        }
+                    }
+
                     // Declare a 'member' if the container is an ES5 class or ES6 constructor
-                    container.symbol.members = container.symbol.members || createSymbolTable();
+                    constructorSymbol.members = constructorSymbol.members || createSymbolTable();
                     // It's acceptable for multiple 'this' assignments of the same identifier to occur
-                    declareSymbol(container.symbol.members, container.symbol, node, SymbolFlags.Property, SymbolFlags.PropertyExcludes & ~SymbolFlags.Property);
+                    declareSymbol(constructorSymbol.members, constructorSymbol, node, SymbolFlags.Property, SymbolFlags.PropertyExcludes & ~SymbolFlags.Property);
                     break;
 
                 case SyntaxKind.Constructor:
@@ -2344,10 +2354,13 @@ namespace ts {
                 case SyntaxKind.SetAccessor:
                     // this.foo assignment in a JavaScript class
                     // Bind this property to the containing class
-                    const containingClass = container.parent;
-                    const symbolTable = hasModifier(container, ModifierFlags.Static) ? containingClass.symbol.exports : containingClass.symbol.members;
+                    const containingClass = thisContainer.parent;
+                    const symbolTable = hasModifier(thisContainer, ModifierFlags.Static) ? containingClass.symbol.exports : containingClass.symbol.members;
                     declareSymbol(symbolTable, containingClass.symbol, node, SymbolFlags.Property, SymbolFlags.None, /*isReplaceableByMethod*/ true);
                     break;
+
+                default:
+                    Debug.fail(Debug.showSyntaxKind(thisContainer));
             }
         }
 
@@ -2417,8 +2430,12 @@ namespace ts {
             bindPropertyAssignment(node.expression, node, /*isPrototypeProperty*/ false);
         }
 
+        function getJSInitializerSymbolFromName(name: EntityNameExpression, lookupContainer?: Node): Symbol {
+            return getJSInitializerSymbol(lookupSymbolForPropertyAccess(name, lookupContainer));
+        }
+
         function bindPropertyAssignment(name: EntityNameExpression, propertyAccess: PropertyAccessEntityNameExpression, isPrototypeProperty: boolean) {
-            let symbol = getJSInitializerSymbol(lookupSymbolForPropertyAccess(name));
+            let symbol = getJSInitializerSymbolFromName(name);
             let isToplevelNamespaceableInitializer: boolean;
             if (isBinaryExpression(propertyAccess.parent)) {
                 const isPrototypeAssignment = isPropertyAccessExpression(propertyAccess.parent.left) && propertyAccess.parent.left.name.escapedText === "prototype";
@@ -2458,9 +2475,9 @@ namespace ts {
             declareSymbol(symbolTable, symbol, propertyAccess, symbolFlags, symbolExcludes);
         }
 
-        function lookupSymbolForPropertyAccess(node: EntityNameExpression): Symbol | undefined {
+        function lookupSymbolForPropertyAccess(node: EntityNameExpression, lookupContainer: Node = container): Symbol | undefined {
             if (isIdentifier(node)) {
-                return lookupSymbolForNameWorker(container, node.escapedText);
+                return lookupSymbolForNameWorker(lookupContainer, node.escapedText);
             }
             else {
                 const symbol = getJSInitializerSymbol(lookupSymbolForPropertyAccess(node.expression));

src/compiler/types.ts

@@ -3324,7 +3324,7 @@ namespace ts {
         escapedName: __String;                  // Name of symbol
         declarations?: Declaration[];           // Declarations associated with this symbol
         valueDeclaration?: Declaration;         // First value declaration of the symbol
-        members?: SymbolTable;                  // Class, interface or literal instance members
+        members?: SymbolTable;                  // Class, interface or object literal instance members
         exports?: SymbolTable;                  // Module exports
         globalExports?: SymbolTable;            // Conditional global UMD exports
         /* @internal */ id?: number;            // Unique id (used to look up SymbolLinks)

tests/baselines/reference/constructorFunctions2.symbols

@@ -23,12 +23,29 @@ const B = function() { this.id = 1; }
 >B : Symbol(B, Decl(index.js, 3, 5))
 >id : Symbol(B.id, Decl(index.js, 3, 22))
 
-const b = new B().id;
->b : Symbol(b, Decl(index.js, 4, 5))
->new B().id : Symbol(B.id, Decl(index.js, 3, 22))
+B.prototype.m = function() { this.x = 2; }
+>B.prototype : Symbol(B.m, Decl(index.js, 3, 37))
 >B : Symbol(B, Decl(index.js, 3, 5))
+>prototype : Symbol(Function.prototype, Decl(lib.d.ts, --, --))
+>m : Symbol(B.m, Decl(index.js, 3, 37))
+>this.x : Symbol(B.x, Decl(index.js, 4, 28))
+>this : Symbol(B, Decl(index.js, 3, 9))
+>x : Symbol(B.x, Decl(index.js, 4, 28))
+
+const b = new B();
+>b : Symbol(b, Decl(index.js, 5, 5))
+>B : Symbol(B, Decl(index.js, 3, 5))
+
+b.id;
+>b.id : Symbol(B.id, Decl(index.js, 3, 22))
+>b : Symbol(b, Decl(index.js, 5, 5))
 >id : Symbol(B.id, Decl(index.js, 3, 22))
 
+b.x;
+>b.x : Symbol(B.x, Decl(index.js, 4, 28))
+>b : Symbol(b, Decl(index.js, 5, 5))
+>x : Symbol(B.x, Decl(index.js, 4, 28))
+
 === tests/cases/conformance/salsa/other.js ===
 function A() { this.id = 1; }
 >A : Symbol(A, Decl(other.js, 0, 0))

tests/baselines/reference/constructorFunctions2.types

@@ -30,13 +30,35 @@ const B = function() { this.id = 1; }
 >id : any
 >1 : 1
 
-const b = new B().id;
->b : number
->new B().id : number
->new B() : { id: number; }
+B.prototype.m = function() { this.x = 2; }
+>B.prototype.m = function() { this.x = 2; } : () => void
+>B.prototype.m : any
+>B.prototype : any
 >B : () => void
+>prototype : any
+>m : any
+>function() { this.x = 2; } : () => void
+>this.x = 2 : 2
+>this.x : number
+>this : { id: number; m: () => void; x: number; }
+>x : number
+>2 : 2
+
+const b = new B();
+>b : { id: number; m: () => void; x: number; }
+>new B() : { id: number; m: () => void; x: number; }
+>B : () => void
+
+b.id;
+>b.id : number
+>b : { id: number; m: () => void; x: number; }
 >id : number
 
+b.x;
+>b.x : number
+>b : { id: number; m: () => void; x: number; }
+>x : number
+
 === tests/cases/conformance/salsa/other.js ===
 function A() { this.id = 1; }
 >A : () => void

tests/cases/conformance/salsa/constructorFunctions2.ts

@@ -11,7 +11,10 @@ const A = require("./other");
 const a = new A().id;
 
 const B = function() { this.id = 1; }
-const b = new B().id;
+B.prototype.m = function() { this.x = 2; }
+const b = new B();
+b.id;
+b.x;
 
 // @filename: other.js
 function A() { this.id = 1; }

tests/cases/fourslash/convertFunctionToEs6ClassJsDoc.ts

@@ -23,7 +23,6 @@
 
 verify.codeFix({
     description: "Convert function to an ES2015 class",
-    index: 0, // TODO: GH#22240
     newFileContent:
 `class fn {
     constructor() {

tests/cases/fourslash/convertFunctionToEs6Class_falsePositive.ts

@@ -0,0 +1,26 @@
+/// <reference path='fourslash.ts' />
+
+// @allowJs: true
+// @Filename: /a.js
+////function [|f|]() {}
+////f.prototype.bar = [|function|](){
+////    this.x = 1;
+////};
+
+// Only a suggestion for `f`, not for the function expression. See GH#22240
+verify.getSuggestionDiagnostics([{
+    message: "This constructor function may be converted to a class declaration.",
+    code: 80002,
+}]);
+
+verify.codeFix({
+    description: "Convert function to an ES2015 class",
+    newFileContent:
+`class f {
+    constructor() { }
+    bar() {
+        this.x = 1;
+    }
+}
+`,
+});

tests/cases/fourslash/findAllRefsConstructorFunctions.ts

@@ -0,0 +1,14 @@
+/// <reference path="fourslash.ts" />
+
+// @allowJs: true
+
+// @Filename: /a.js
+////function f() {
+////    this.[|{| "isWriteAccess": true, "isDefinition": true |}x|] = 0;
+////}
+////f.prototype.setX = function() {
+////    this.[|{| "isWriteAccess": true, "isDefinition": true |}x|] = 1;
+////}
+////f.prototype.useX = function() { this.[|x|]; }
+
+verify.singleReferenceGroup("(property) f.x: number");