Skip to content

Commit 1074819

Browse files
sandersnsandersn
authored
Js constructor function fixes (microsoft#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
1 parent ab8233c commit 1074819

7 files changed

+245
-16
lines changed

‎src/compiler/checker.ts

+12-3
Original file line numberDiff line numberDiff line change
@@ -4259,7 +4259,14 @@ namespace ts {
42594259
}
42604260

42614261
if (isPropertyAccessExpression(expression.left) && expression.left.expression.kind === SyntaxKind.ThisKeyword) {
4262-
if (getThisContainer(expression, /*includeArrowFunctions*/ false).kind === SyntaxKind.Constructor) {
4262+
const thisContainer = getThisContainer(expression, /*includeArrowFunctions*/ false);
4263+
const isPrototypeProperty = isBinaryExpression(thisContainer.parent) &&
4264+
getSpecialPropertyAssignmentKind(thisContainer.parent) === SpecialPropertyAssignmentKind.PrototypeProperty;
4265+
// Properties defined in a constructor (or javascript constructor function) don't get undefined added.
4266+
// Function expressions that are assigned to the prototype count as methods.
4267+
if (thisContainer.kind === SyntaxKind.Constructor ||
4268+
thisContainer.kind === SyntaxKind.FunctionDeclaration ||
4269+
(thisContainer.kind === SyntaxKind.FunctionExpression && !isPrototypeProperty)) {
42634270
definedInConstructor = true;
42644271
}
42654272
else {
@@ -18566,7 +18573,7 @@ namespace ts {
1856618573
return true;
1856718574
}
1856818575

18569-
/** NOTE: Return value of `[]` means a different thing than `undefined`. `[]` means return `void`, `undefined` means return `never`. */
18576+
/** NOTE: Return value of `[]` means a different thing than `undefined`. `[]` means func returns `void`, `undefined` means it returns `never`. */
1857018577
function checkAndAggregateReturnExpressionTypes(func: FunctionLikeDeclaration, checkMode: CheckMode): Type[] | undefined {
1857118578
const functionFlags = getFunctionFlags(func);
1857218579
const aggregatedTypes: Type[] = [];
@@ -18595,7 +18602,9 @@ namespace ts {
1859518602
if (aggregatedTypes.length === 0 && !hasReturnWithNoExpression && (hasReturnOfTypeNever || mayReturnNever(func))) {
1859618603
return undefined;
1859718604
}
18598-
if (strictNullChecks && aggregatedTypes.length && hasReturnWithNoExpression) {
18605+
if (strictNullChecks && aggregatedTypes.length && hasReturnWithNoExpression &&
18606+
!(isJavaScriptConstructor(func) && aggregatedTypes.some(t => t.symbol === func.symbol))) {
18607+
// Javascript "callable constructors", containing eg `if (!(this instanceof A)) return new A()` should not add undefined
1859918608
pushIfUnique(aggregatedTypes, undefinedType);
1860018609
}
1860118610
return aggregatedTypes;

‎tests/baselines/reference/constructorFunctionsStrict.errors.txt

+28
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
tests/cases/conformance/salsa/a.js(9,1): error TS2322: Type 'undefined' is not assignable to type 'number'.
2+
3+
4+
==== tests/cases/conformance/salsa/a.js (1 errors) ====
5+
/** @param {number} x */
6+
function C(x) {
7+
this.x = x
8+
}
9+
C.prototype.m = function() {
10+
this.y = 12
11+
}
12+
var c = new C(1)
13+
c.x = undefined // should error
14+
~~~
15+
!!! error TS2322: Type 'undefined' is not assignable to type 'number'.
16+
c.y = undefined // ok
17+
18+
/** @param {number} x */
19+
function A(x) {
20+
if (!(this instanceof A)) {
21+
return new A(x)
22+
}
23+
this.x = x
24+
}
25+
var k = A(1)
26+
var j = new A(2)
27+
k.x === j.x
28+

‎tests/baselines/reference/constructorFunctionsStrict.symbols

+69
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
=== tests/cases/conformance/salsa/a.js ===
2+
/** @param {number} x */
3+
function C(x) {
4+
>C : Symbol(C, Decl(a.js, 0, 0))
5+
>x : Symbol(x, Decl(a.js, 1, 11))
6+
7+
this.x = x
8+
>x : Symbol(C.x, Decl(a.js, 1, 15))
9+
>x : Symbol(x, Decl(a.js, 1, 11))
10+
}
11+
C.prototype.m = function() {
12+
>C.prototype : Symbol(C.m, Decl(a.js, 3, 1))
13+
>C : Symbol(C, Decl(a.js, 0, 0))
14+
>prototype : Symbol(Function.prototype, Decl(lib.d.ts, --, --))
15+
>m : Symbol(C.m, Decl(a.js, 3, 1))
16+
17+
this.y = 12
18+
>this.y : Symbol(C.y, Decl(a.js, 4, 28))
19+
>this : Symbol(C, Decl(a.js, 0, 0))
20+
>y : Symbol(C.y, Decl(a.js, 4, 28))
21+
}
22+
var c = new C(1)
23+
>c : Symbol(c, Decl(a.js, 7, 3))
24+
>C : Symbol(C, Decl(a.js, 0, 0))
25+
26+
c.x = undefined // should error
27+
>c.x : Symbol(C.x, Decl(a.js, 1, 15))
28+
>c : Symbol(c, Decl(a.js, 7, 3))
29+
>x : Symbol(C.x, Decl(a.js, 1, 15))
30+
>undefined : Symbol(undefined)
31+
32+
c.y = undefined // ok
33+
>c.y : Symbol(C.y, Decl(a.js, 4, 28))
34+
>c : Symbol(c, Decl(a.js, 7, 3))
35+
>y : Symbol(C.y, Decl(a.js, 4, 28))
36+
>undefined : Symbol(undefined)
37+
38+
/** @param {number} x */
39+
function A(x) {
40+
>A : Symbol(A, Decl(a.js, 9, 15))
41+
>x : Symbol(x, Decl(a.js, 12, 11))
42+
43+
if (!(this instanceof A)) {
44+
>A : Symbol(A, Decl(a.js, 9, 15))
45+
46+
return new A(x)
47+
>A : Symbol(A, Decl(a.js, 9, 15))
48+
>x : Symbol(x, Decl(a.js, 12, 11))
49+
}
50+
this.x = x
51+
>x : Symbol(A.x, Decl(a.js, 15, 5))
52+
>x : Symbol(x, Decl(a.js, 12, 11))
53+
}
54+
var k = A(1)
55+
>k : Symbol(k, Decl(a.js, 18, 3))
56+
>A : Symbol(A, Decl(a.js, 9, 15))
57+
58+
var j = new A(2)
59+
>j : Symbol(j, Decl(a.js, 19, 3))
60+
>A : Symbol(A, Decl(a.js, 9, 15))
61+
62+
k.x === j.x
63+
>k.x : Symbol(A.x, Decl(a.js, 15, 5))
64+
>k : Symbol(k, Decl(a.js, 18, 3))
65+
>x : Symbol(A.x, Decl(a.js, 15, 5))
66+
>j.x : Symbol(A.x, Decl(a.js, 15, 5))
67+
>j : Symbol(j, Decl(a.js, 19, 3))
68+
>x : Symbol(A.x, Decl(a.js, 15, 5))
69+

‎tests/baselines/reference/constructorFunctionsStrict.types

+94
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
=== tests/cases/conformance/salsa/a.js ===
2+
/** @param {number} x */
3+
function C(x) {
4+
>C : (x: number) => void
5+
>x : number
6+
7+
this.x = x
8+
>this.x = x : number
9+
>this.x : any
10+
>this : any
11+
>x : any
12+
>x : number
13+
}
14+
C.prototype.m = function() {
15+
>C.prototype.m = function() { this.y = 12} : () => void
16+
>C.prototype.m : any
17+
>C.prototype : any
18+
>C : (x: number) => void
19+
>prototype : any
20+
>m : any
21+
>function() { this.y = 12} : () => void
22+
23+
this.y = 12
24+
>this.y = 12 : 12
25+
>this.y : number | undefined
26+
>this : { x: number; m: () => void; y: number | undefined; }
27+
>y : number | undefined
28+
>12 : 12
29+
}
30+
var c = new C(1)
31+
>c : { x: number; m: () => void; y: number | undefined; }
32+
>new C(1) : { x: number; m: () => void; y: number | undefined; }
33+
>C : (x: number) => void
34+
>1 : 1
35+
36+
c.x = undefined // should error
37+
>c.x = undefined : undefined
38+
>c.x : number
39+
>c : { x: number; m: () => void; y: number | undefined; }
40+
>x : number
41+
>undefined : undefined
42+
43+
c.y = undefined // ok
44+
>c.y = undefined : undefined
45+
>c.y : number | undefined
46+
>c : { x: number; m: () => void; y: number | undefined; }
47+
>y : number | undefined
48+
>undefined : undefined
49+
50+
/** @param {number} x */
51+
function A(x) {
52+
>A : (x: number) => typeof A
53+
>x : number
54+
55+
if (!(this instanceof A)) {
56+
>!(this instanceof A) : boolean
57+
>(this instanceof A) : boolean
58+
>this instanceof A : boolean
59+
>this : any
60+
>A : (x: number) => typeof A
61+
62+
return new A(x)
63+
>new A(x) : { x: number; }
64+
>A : (x: number) => typeof A
65+
>x : number
66+
}
67+
this.x = x
68+
>this.x = x : number
69+
>this.x : any
70+
>this : any
71+
>x : any
72+
>x : number
73+
}
74+
var k = A(1)
75+
>k : { x: number; }
76+
>A(1) : { x: number; }
77+
>A : (x: number) => typeof A
78+
>1 : 1
79+
80+
var j = new A(2)
81+
>j : { x: number; }
82+
>new A(2) : { x: number; }
83+
>A : (x: number) => typeof A
84+
>2 : 2
85+
86+
k.x === j.x
87+
>k.x === j.x : boolean
88+
>k.x : number
89+
>k : { x: number; }
90+
>x : number
91+
>j.x : number
92+
>j : { x: number; }
93+
>x : number
94+

‎tests/baselines/reference/jsdocTypeTagCast.errors.txt

+6-6
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,10 @@ tests/cases/conformance/jsdoc/b.js(51,17): error TS2352: Type 'SomeDerived' cann
77
Property 'q' is missing in type 'SomeDerived'.
88
tests/cases/conformance/jsdoc/b.js(52,17): error TS2352: Type 'SomeBase' cannot be converted to type 'SomeOther'.
99
Property 'q' is missing in type 'SomeBase'.
10-
tests/cases/conformance/jsdoc/b.js(58,1): error TS2322: Type '{ p: string | number | undefined; }' is not assignable to type 'SomeBase'.
10+
tests/cases/conformance/jsdoc/b.js(58,1): error TS2322: Type '{ p: string | number; }' is not assignable to type 'SomeBase'.
1111
Types of property 'p' are incompatible.
12-
Type 'string | number | undefined' is not assignable to type 'number'.
13-
Type 'undefined' is not assignable to type 'number'.
12+
Type 'string | number' is not assignable to type 'number'.
13+
Type 'string' is not assignable to type 'number'.
1414
tests/cases/conformance/jsdoc/b.js(66,8): error TS2352: Type 'boolean' cannot be converted to type 'string | number'.
1515
tests/cases/conformance/jsdoc/b.js(66,15): error TS2304: Cannot find name 'numOrStr'.
1616
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
9797

9898
someBase = someFakeClass; // Error
9999
~~~~~~~~
100-
!!! error TS2322: Type '{ p: string | number | undefined; }' is not assignable to type 'SomeBase'.
100+
!!! error TS2322: Type '{ p: string | number; }' is not assignable to type 'SomeBase'.
101101
!!! error TS2322: Types of property 'p' are incompatible.
102-
!!! error TS2322: Type 'string | number | undefined' is not assignable to type 'number'.
103-
!!! error TS2322: Type 'undefined' is not assignable to type 'number'.
102+
!!! error TS2322: Type 'string | number' is not assignable to type 'number'.
103+
!!! error TS2322: Type 'string' is not assignable to type 'number'.
104104
someBase = /** @type {SomeBase} */(someFakeClass);
105105

106106
// Type assertion cannot be a type-predicate type

‎tests/baselines/reference/jsdocTypeTagCast.types

+7-7
Original file line numberDiff line numberDiff line change
@@ -108,8 +108,8 @@ var someOther = new SomeOther();
108108
>SomeOther : typeof SomeOther
109109

110110
var someFakeClass = new SomeFakeClass();
111-
>someFakeClass : { p: string | number | undefined; }
112-
>new SomeFakeClass() : { p: string | number | undefined; }
111+
>someFakeClass : { p: string | number; }
112+
>new SomeFakeClass() : { p: string | number; }
113113
>SomeFakeClass : () => void
114114

115115
someBase = /** @type {SomeBase} */(someDerived);
@@ -168,24 +168,24 @@ someOther = /** @type {SomeOther} */(someOther);
168168

169169
someFakeClass = someBase;
170170
>someFakeClass = someBase : SomeBase
171-
>someFakeClass : { p: string | number | undefined; }
171+
>someFakeClass : { p: string | number; }
172172
>someBase : SomeBase
173173

174174
someFakeClass = someDerived;
175175
>someFakeClass = someDerived : SomeDerived
176-
>someFakeClass : { p: string | number | undefined; }
176+
>someFakeClass : { p: string | number; }
177177
>someDerived : SomeDerived
178178

179179
someBase = someFakeClass; // Error
180-
>someBase = someFakeClass : { p: string | number | undefined; }
180+
>someBase = someFakeClass : { p: string | number; }
181181
>someBase : SomeBase
182-
>someFakeClass : { p: string | number | undefined; }
182+
>someFakeClass : { p: string | number; }
183183

184184
someBase = /** @type {SomeBase} */(someFakeClass);
185185
>someBase = /** @type {SomeBase} */(someFakeClass) : SomeBase
186186
>someBase : SomeBase
187187
>(someFakeClass) : SomeBase
188-
>someFakeClass : { p: string | number | undefined; }
188+
>someFakeClass : { p: string | number; }
189189

190190
// Type assertion cannot be a type-predicate type
191191
/** @type {number | string} */

‎tests/cases/conformance/salsa/constructorFunctionsStrict.ts

+29
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
// @noEmit: true
2+
// @allowJs: true
3+
// @checkJs: true
4+
// @strict: true
5+
// @noImplicitThis: false
6+
// @Filename: a.js
7+
8+
9+
/** @param {number} x */
10+
function C(x) {
11+
this.x = x
12+
}
13+
C.prototype.m = function() {
14+
this.y = 12
15+
}
16+
var c = new C(1)
17+
c.x = undefined // should error
18+
c.y = undefined // ok
19+
20+
/** @param {number} x */
21+
function A(x) {
22+
if (!(this instanceof A)) {
23+
return new A(x)
24+
}
25+
this.x = x
26+
}
27+
var k = A(1)
28+
var j = new A(2)
29+
k.x === j.x

0 commit comments

Comments
 (0)