From 1074819be3e52fea18acf1b5711980ea719e7d68 Mon Sep 17 00:00:00 2001
From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com>
Date: Tue, 20 Mar 2018 11:24:09 -0700
Subject: [PATCH] Js constructor function fixes (#22721)

* Do not add undefined for this assignments in functions

* Test:constructor functions with --strict

* First draft -- works, but needs a stricter check added

* Update baselines

* Make undefined-skip stricter and more efficient

Symbol-based now instead of syntactic

* Exclude prototype function assignments

* Add explanatory comment
---
 src/compiler/checker.ts                       | 15 ++-
 .../constructorFunctionsStrict.errors.txt     | 28 ++++++
 .../constructorFunctionsStrict.symbols        | 69 ++++++++++++++
 .../constructorFunctionsStrict.types          | 94 +++++++++++++++++++
 .../reference/jsdocTypeTagCast.errors.txt     | 12 +--
 .../reference/jsdocTypeTagCast.types          | 14 +--
 .../salsa/constructorFunctionsStrict.ts       | 29 ++++++
 7 files changed, 245 insertions(+), 16 deletions(-)
 create mode 100644 tests/baselines/reference/constructorFunctionsStrict.errors.txt
 create mode 100644 tests/baselines/reference/constructorFunctionsStrict.symbols
 create mode 100644 tests/baselines/reference/constructorFunctionsStrict.types
 create mode 100644 tests/cases/conformance/salsa/constructorFunctionsStrict.ts

diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
index 137964324d7..4c3a98b12dc 100644
--- a/src/compiler/checker.ts
+++ b/src/compiler/checker.ts
@@ -4259,7 +4259,14 @@ namespace ts {
                 }
 
                 if (isPropertyAccessExpression(expression.left) && expression.left.expression.kind === SyntaxKind.ThisKeyword) {
-                    if (getThisContainer(expression, /*includeArrowFunctions*/ false).kind === SyntaxKind.Constructor) {
+                    const thisContainer = getThisContainer(expression, /*includeArrowFunctions*/ false);
+                    const isPrototypeProperty = isBinaryExpression(thisContainer.parent) &&
+                        getSpecialPropertyAssignmentKind(thisContainer.parent) === SpecialPropertyAssignmentKind.PrototypeProperty;
+                    // Properties defined in a constructor (or javascript constructor function) don't get undefined added.
+                    // Function expressions that are assigned to the prototype count as methods.
+                    if (thisContainer.kind === SyntaxKind.Constructor ||
+                        thisContainer.kind === SyntaxKind.FunctionDeclaration ||
+                        (thisContainer.kind === SyntaxKind.FunctionExpression && !isPrototypeProperty)) {
                         definedInConstructor = true;
                     }
                     else {
@@ -18566,7 +18573,7 @@ namespace ts {
             return true;
         }
 
-        /** NOTE: Return value of `[]` means a different thing than `undefined`. `[]` means return `void`, `undefined` means return `never`. */
+        /** NOTE: Return value of `[]` means a different thing than `undefined`. `[]` means func returns `void`, `undefined` means it returns `never`. */
         function checkAndAggregateReturnExpressionTypes(func: FunctionLikeDeclaration, checkMode: CheckMode): Type[] | undefined {
             const functionFlags = getFunctionFlags(func);
             const aggregatedTypes: Type[] = [];
@@ -18595,7 +18602,9 @@ namespace ts {
             if (aggregatedTypes.length === 0 && !hasReturnWithNoExpression && (hasReturnOfTypeNever || mayReturnNever(func))) {
                 return undefined;
             }
-            if (strictNullChecks && aggregatedTypes.length && hasReturnWithNoExpression) {
+            if (strictNullChecks && aggregatedTypes.length && hasReturnWithNoExpression &&
+                !(isJavaScriptConstructor(func) && aggregatedTypes.some(t => t.symbol === func.symbol))) {
+                // Javascript "callable constructors", containing eg `if (!(this instanceof A)) return new A()` should not add undefined
                 pushIfUnique(aggregatedTypes, undefinedType);
             }
             return aggregatedTypes;
diff --git a/tests/baselines/reference/constructorFunctionsStrict.errors.txt b/tests/baselines/reference/constructorFunctionsStrict.errors.txt
new file mode 100644
index 00000000000..a0ca87a307c
--- /dev/null
+++ b/tests/baselines/reference/constructorFunctionsStrict.errors.txt
@@ -0,0 +1,28 @@
+tests/cases/conformance/salsa/a.js(9,1): error TS2322: Type 'undefined' is not assignable to type 'number'.
+
+
+==== tests/cases/conformance/salsa/a.js (1 errors) ====
+    /** @param {number} x */
+    function C(x) {
+        this.x = x
+    }
+    C.prototype.m = function() {
+        this.y = 12
+    }
+    var c = new C(1)
+    c.x = undefined // should error
+    ~~~
+!!! error TS2322: Type 'undefined' is not assignable to type 'number'.
+    c.y = undefined // ok
+    
+    /** @param {number} x */
+    function A(x) {
+        if (!(this instanceof A)) {
+            return new A(x)
+        }
+        this.x = x
+    }
+    var k = A(1)
+    var j = new A(2)
+    k.x === j.x
+    
\ No newline at end of file
diff --git a/tests/baselines/reference/constructorFunctionsStrict.symbols b/tests/baselines/reference/constructorFunctionsStrict.symbols
new file mode 100644
index 00000000000..c51f795d632
--- /dev/null
+++ b/tests/baselines/reference/constructorFunctionsStrict.symbols
@@ -0,0 +1,69 @@
+=== tests/cases/conformance/salsa/a.js ===
+/** @param {number} x */
+function C(x) {
+>C : Symbol(C, Decl(a.js, 0, 0))
+>x : Symbol(x, Decl(a.js, 1, 11))
+
+    this.x = x
+>x : Symbol(C.x, Decl(a.js, 1, 15))
+>x : Symbol(x, Decl(a.js, 1, 11))
+}
+C.prototype.m = function() {
+>C.prototype : Symbol(C.m, Decl(a.js, 3, 1))
+>C : Symbol(C, Decl(a.js, 0, 0))
+>prototype : Symbol(Function.prototype, Decl(lib.d.ts, --, --))
+>m : Symbol(C.m, Decl(a.js, 3, 1))
+
+    this.y = 12
+>this.y : Symbol(C.y, Decl(a.js, 4, 28))
+>this : Symbol(C, Decl(a.js, 0, 0))
+>y : Symbol(C.y, Decl(a.js, 4, 28))
+}
+var c = new C(1)
+>c : Symbol(c, Decl(a.js, 7, 3))
+>C : Symbol(C, Decl(a.js, 0, 0))
+
+c.x = undefined // should error
+>c.x : Symbol(C.x, Decl(a.js, 1, 15))
+>c : Symbol(c, Decl(a.js, 7, 3))
+>x : Symbol(C.x, Decl(a.js, 1, 15))
+>undefined : Symbol(undefined)
+
+c.y = undefined // ok
+>c.y : Symbol(C.y, Decl(a.js, 4, 28))
+>c : Symbol(c, Decl(a.js, 7, 3))
+>y : Symbol(C.y, Decl(a.js, 4, 28))
+>undefined : Symbol(undefined)
+
+/** @param {number} x */
+function A(x) {
+>A : Symbol(A, Decl(a.js, 9, 15))
+>x : Symbol(x, Decl(a.js, 12, 11))
+
+    if (!(this instanceof A)) {
+>A : Symbol(A, Decl(a.js, 9, 15))
+
+        return new A(x)
+>A : Symbol(A, Decl(a.js, 9, 15))
+>x : Symbol(x, Decl(a.js, 12, 11))
+    }
+    this.x = x
+>x : Symbol(A.x, Decl(a.js, 15, 5))
+>x : Symbol(x, Decl(a.js, 12, 11))
+}
+var k = A(1)
+>k : Symbol(k, Decl(a.js, 18, 3))
+>A : Symbol(A, Decl(a.js, 9, 15))
+
+var j = new A(2)
+>j : Symbol(j, Decl(a.js, 19, 3))
+>A : Symbol(A, Decl(a.js, 9, 15))
+
+k.x === j.x
+>k.x : Symbol(A.x, Decl(a.js, 15, 5))
+>k : Symbol(k, Decl(a.js, 18, 3))
+>x : Symbol(A.x, Decl(a.js, 15, 5))
+>j.x : Symbol(A.x, Decl(a.js, 15, 5))
+>j : Symbol(j, Decl(a.js, 19, 3))
+>x : Symbol(A.x, Decl(a.js, 15, 5))
+
diff --git a/tests/baselines/reference/constructorFunctionsStrict.types b/tests/baselines/reference/constructorFunctionsStrict.types
new file mode 100644
index 00000000000..745d55c5bfb
--- /dev/null
+++ b/tests/baselines/reference/constructorFunctionsStrict.types
@@ -0,0 +1,94 @@
+=== tests/cases/conformance/salsa/a.js ===
+/** @param {number} x */
+function C(x) {
+>C : (x: number) => void
+>x : number
+
+    this.x = x
+>this.x = x : number
+>this.x : any
+>this : any
+>x : any
+>x : number
+}
+C.prototype.m = function() {
+>C.prototype.m = function() {    this.y = 12} : () => void
+>C.prototype.m : any
+>C.prototype : any
+>C : (x: number) => void
+>prototype : any
+>m : any
+>function() {    this.y = 12} : () => void
+
+    this.y = 12
+>this.y = 12 : 12
+>this.y : number | undefined
+>this : { x: number; m: () => void; y: number | undefined; }
+>y : number | undefined
+>12 : 12
+}
+var c = new C(1)
+>c : { x: number; m: () => void; y: number | undefined; }
+>new C(1) : { x: number; m: () => void; y: number | undefined; }
+>C : (x: number) => void
+>1 : 1
+
+c.x = undefined // should error
+>c.x = undefined : undefined
+>c.x : number
+>c : { x: number; m: () => void; y: number | undefined; }
+>x : number
+>undefined : undefined
+
+c.y = undefined // ok
+>c.y = undefined : undefined
+>c.y : number | undefined
+>c : { x: number; m: () => void; y: number | undefined; }
+>y : number | undefined
+>undefined : undefined
+
+/** @param {number} x */
+function A(x) {
+>A : (x: number) => typeof A
+>x : number
+
+    if (!(this instanceof A)) {
+>!(this instanceof A) : boolean
+>(this instanceof A) : boolean
+>this instanceof A : boolean
+>this : any
+>A : (x: number) => typeof A
+
+        return new A(x)
+>new A(x) : { x: number; }
+>A : (x: number) => typeof A
+>x : number
+    }
+    this.x = x
+>this.x = x : number
+>this.x : any
+>this : any
+>x : any
+>x : number
+}
+var k = A(1)
+>k : { x: number; }
+>A(1) : { x: number; }
+>A : (x: number) => typeof A
+>1 : 1
+
+var j = new A(2)
+>j : { x: number; }
+>new A(2) : { x: number; }
+>A : (x: number) => typeof A
+>2 : 2
+
+k.x === j.x
+>k.x === j.x : boolean
+>k.x : number
+>k : { x: number; }
+>x : number
+>j.x : number
+>j : { x: number; }
+>x : number
+
diff --git a/tests/baselines/reference/jsdocTypeTagCast.errors.txt b/tests/baselines/reference/jsdocTypeTagCast.errors.txt
index f1f53ec1ecb..9120e570fd1 100644
--- a/tests/baselines/reference/jsdocTypeTagCast.errors.txt
+++ b/tests/baselines/reference/jsdocTypeTagCast.errors.txt
@@ -7,10 +7,10 @@ tests/cases/conformance/jsdoc/b.js(51,17): error TS2352: Type 'SomeDerived' cann
   Property 'q' is missing in type 'SomeDerived'.
 tests/cases/conformance/jsdoc/b.js(52,17): error TS2352: Type 'SomeBase' cannot be converted to type 'SomeOther'.
   Property 'q' is missing in type 'SomeBase'.
-tests/cases/conformance/jsdoc/b.js(58,1): error TS2322: Type '{ p: string | number | undefined; }' is not assignable to type 'SomeBase'.
+tests/cases/conformance/jsdoc/b.js(58,1): error TS2322: Type '{ p: string | number; }' is not assignable to type 'SomeBase'.
   Types of property 'p' are incompatible.
-    Type 'string | number | undefined' is not assignable to type 'number'.
-      Type 'undefined' is not assignable to type 'number'.
+    Type 'string | number' is not assignable to type 'number'.
+      Type 'string' is not assignable to type 'number'.
 tests/cases/conformance/jsdoc/b.js(66,8): error TS2352: Type 'boolean' cannot be converted to type 'string | number'.
 tests/cases/conformance/jsdoc/b.js(66,15): error TS2304: Cannot find name 'numOrStr'.
 tests/cases/conformance/jsdoc/b.js(66,24): error TS1005: '}' expected.
@@ -97,10 +97,10 @@ tests/cases/conformance/jsdoc/b.js(67,8): error TS2454: Variable 'numOrStr' is u
     
     someBase = someFakeClass; // Error
     ~~~~~~~~
-!!! error TS2322: Type '{ p: string | number | undefined; }' is not assignable to type 'SomeBase'.
+!!! error TS2322: Type '{ p: string | number; }' is not assignable to type 'SomeBase'.
 !!! error TS2322:   Types of property 'p' are incompatible.
-!!! error TS2322:     Type 'string | number | undefined' is not assignable to type 'number'.
-!!! error TS2322:       Type 'undefined' is not assignable to type 'number'.
+!!! error TS2322:     Type 'string | number' is not assignable to type 'number'.
+!!! error TS2322:       Type 'string' is not assignable to type 'number'.
     someBase = /** @type {SomeBase} */(someFakeClass);
     
     // Type assertion cannot be a type-predicate type
diff --git a/tests/baselines/reference/jsdocTypeTagCast.types b/tests/baselines/reference/jsdocTypeTagCast.types
index 1e1fe5d690c..a24d04148e3 100644
--- a/tests/baselines/reference/jsdocTypeTagCast.types
+++ b/tests/baselines/reference/jsdocTypeTagCast.types
@@ -108,8 +108,8 @@ var someOther = new SomeOther();
 >SomeOther : typeof SomeOther
 
 var someFakeClass = new SomeFakeClass();
->someFakeClass : { p: string | number | undefined; }
->new SomeFakeClass() : { p: string | number | undefined; }
+>someFakeClass : { p: string | number; }
+>new SomeFakeClass() : { p: string | number; }
 >SomeFakeClass : () => void
 
 someBase = /** @type {SomeBase} */(someDerived);
@@ -168,24 +168,24 @@ someOther = /** @type {SomeOther} */(someOther);
 
 someFakeClass = someBase;
 >someFakeClass = someBase : SomeBase
->someFakeClass : { p: string | number | undefined; }
+>someFakeClass : { p: string | number; }
 >someBase : SomeBase
 
 someFakeClass = someDerived;
 >someFakeClass = someDerived : SomeDerived
->someFakeClass : { p: string | number | undefined; }
+>someFakeClass : { p: string | number; }
 >someDerived : SomeDerived
 
 someBase = someFakeClass; // Error
->someBase = someFakeClass : { p: string | number | undefined; }
+>someBase = someFakeClass : { p: string | number; }
 >someBase : SomeBase
->someFakeClass : { p: string | number | undefined; }
+>someFakeClass : { p: string | number; }
 
 someBase = /** @type {SomeBase} */(someFakeClass);
 >someBase = /** @type {SomeBase} */(someFakeClass) : SomeBase
 >someBase : SomeBase
 >(someFakeClass) : SomeBase
->someFakeClass : { p: string | number | undefined; }
+>someFakeClass : { p: string | number; }
 
 // Type assertion cannot be a type-predicate type
 /** @type {number | string} */
diff --git a/tests/cases/conformance/salsa/constructorFunctionsStrict.ts b/tests/cases/conformance/salsa/constructorFunctionsStrict.ts
new file mode 100644
index 00000000000..33cfee4d846
--- /dev/null
+++ b/tests/cases/conformance/salsa/constructorFunctionsStrict.ts
@@ -0,0 +1,29 @@
+// @noEmit: true
+// @allowJs: true
+// @checkJs: true
+// @strict: true
+// @noImplicitThis: false
+// @Filename: a.js
+
+
+/** @param {number} x */
+function C(x) {
+    this.x = x
+}
+C.prototype.m = function() {
+    this.y = 12
+}
+var c = new C(1)
+c.x = undefined // should error
+c.y = undefined // ok
+
+/** @param {number} x */
+function A(x) {
+    if (!(this instanceof A)) {
+        return new A(x)
+    }
+    this.x = x
+}
+var k = A(1)
+var j = new A(2)
+k.x === j.x