Skip to content

Commit 4462c15

Browse files
sandersnsandersn
authored
Correctly track thisContainer for this-property-assignments in JS nested containers (microsoft#22779)
* Track thisContainer for this-property-assignments in JS Previously it would update on every block, not just those that could bind `this`. In addition, if the constructor symbol still can't be found, then no binding happens. This is usually OK because people don't add new properties in methods too often. * Update additional baselines * Add lib:dom to new test * Address PR comments * Correct new name for saveThisParentContainer
1 parent f3a1f16 commit 4462c15

12 files changed

+302
-11
lines changed

‎src/compiler/binder.ts

+14-11
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ namespace ts {
118118
let languageVersion: ScriptTarget;
119119
let parent: Node;
120120
let container: Node;
121-
let containerContainer: Node; // Container one level up
121+
let thisParentContainer: Node; // Container one level up
122122
let blockScopeContainer: Node;
123123
let lastContainer: Node;
124124
let seenThisKeyword: boolean;
@@ -186,7 +186,7 @@ namespace ts {
186186
languageVersion = undefined;
187187
parent = undefined;
188188
container = undefined;
189-
containerContainer = undefined;
189+
thisParentContainer = undefined;
190190
blockScopeContainer = undefined;
191191
lastContainer = undefined;
192192
seenThisKeyword = false;
@@ -479,11 +479,9 @@ namespace ts {
479479
// and block-container. Then after we pop out of processing the children, we restore
480480
// these saved values.
481481
const saveContainer = container;
482-
const saveContainerContainer = containerContainer;
482+
const saveThisParentContainer = thisParentContainer;
483483
const savedBlockScopeContainer = blockScopeContainer;
484484

485-
containerContainer = container;
486-
487485
// Depending on what kind of node this is, we may have to adjust the current container
488486
// and block-container. If the current node is a container, then it is automatically
489487
// considered the current block-container as well. Also, for containers that we know
@@ -502,6 +500,9 @@ namespace ts {
502500
// for it. We must clear this so we don't accidentally move any stale data forward from
503501
// a previous compilation.
504502
if (containerFlags & ContainerFlags.IsContainer) {
503+
if (node.kind !== SyntaxKind.ArrowFunction) {
504+
thisParentContainer = container;
505+
}
505506
container = blockScopeContainer = node;
506507
if (containerFlags & ContainerFlags.HasLocals) {
507508
container.locals = createSymbolTable();
@@ -571,7 +572,7 @@ namespace ts {
571572
}
572573

573574
container = saveContainer;
574-
containerContainer = saveContainerContainer;
575+
thisParentContainer = saveThisParentContainer;
575576
blockScopeContainer = savedBlockScopeContainer;
576577
}
577578

@@ -2338,14 +2339,16 @@ namespace ts {
23382339
if (isBinaryExpression(thisContainer.parent) && thisContainer.parent.operatorToken.kind === SyntaxKind.EqualsToken) {
23392340
const l = thisContainer.parent.left;
23402341
if (isPropertyAccessEntityNameExpression(l) && isPrototypeAccess(l.expression)) {
2341-
constructorSymbol = getJSInitializerSymbolFromName(l.expression.expression, containerContainer);
2342+
constructorSymbol = getJSInitializerSymbolFromName(l.expression.expression, thisParentContainer);
23422343
}
23432344
}
23442345

2345-
// Declare a 'member' if the container is an ES5 class or ES6 constructor
2346-
constructorSymbol.members = constructorSymbol.members || createSymbolTable();
2347-
// It's acceptable for multiple 'this' assignments of the same identifier to occur
2348-
declareSymbol(constructorSymbol.members, constructorSymbol, node, SymbolFlags.Property, SymbolFlags.PropertyExcludes & ~SymbolFlags.Property);
2346+
if (constructorSymbol) {
2347+
// Declare a 'member' if the container is an ES5 class or ES6 constructor
2348+
constructorSymbol.members = constructorSymbol.members || createSymbolTable();
2349+
// It's acceptable for multiple 'this' assignments of the same identifier to occur
2350+
declareSymbol(constructorSymbol.members, constructorSymbol, node, SymbolFlags.Property, SymbolFlags.PropertyExcludes & ~SymbolFlags.Property);
2351+
}
23492352
break;
23502353

23512354
case SyntaxKind.Constructor:

‎tests/baselines/reference/typeFromPropertyAssignment20.symbols

+35
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
=== tests/cases/conformance/salsa/bluebird.js ===
2+
!function outer (f) { return f }(
3+
>outer : Symbol(outer, Decl(bluebird.js, 0, 1))
4+
>f : Symbol(f, Decl(bluebird.js, 0, 17))
5+
>f : Symbol(f, Decl(bluebird.js, 0, 17))
6+
7+
function inner () {
8+
>inner : Symbol(inner, Decl(bluebird.js, 0, 33))
9+
10+
function Async() {
11+
>Async : Symbol(Async, Decl(bluebird.js, 1, 23))
12+
13+
this._trampolineEnabled = true;
14+
>_trampolineEnabled : Symbol(Async._trampolineEnabled, Decl(bluebird.js, 2, 26), Decl(bluebird.js, 7, 20))
15+
}
16+
17+
Async.prototype.disableTrampolineIfNecessary = function dtin(b) {
18+
>Async.prototype : Symbol(Async.disableTrampolineIfNecessary, Decl(bluebird.js, 4, 9))
19+
>Async : Symbol(Async, Decl(bluebird.js, 1, 23))
20+
>prototype : Symbol(Function.prototype, Decl(lib.d.ts, --, --))
21+
>disableTrampolineIfNecessary : Symbol(Async.disableTrampolineIfNecessary, Decl(bluebird.js, 4, 9))
22+
>dtin : Symbol(dtin, Decl(bluebird.js, 6, 54))
23+
>b : Symbol(b, Decl(bluebird.js, 6, 69))
24+
25+
if (b) {
26+
>b : Symbol(b, Decl(bluebird.js, 6, 69))
27+
28+
this._trampolineEnabled = false;
29+
>this._trampolineEnabled : Symbol(Async._trampolineEnabled, Decl(bluebird.js, 2, 26), Decl(bluebird.js, 7, 20))
30+
>this : Symbol(Async, Decl(bluebird.js, 1, 23))
31+
>_trampolineEnabled : Symbol(Async._trampolineEnabled, Decl(bluebird.js, 2, 26), Decl(bluebird.js, 7, 20))
32+
}
33+
};
34+
})
35+

‎tests/baselines/reference/typeFromPropertyAssignment20.types

+48
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
=== tests/cases/conformance/salsa/bluebird.js ===
2+
!function outer (f) { return f }(
3+
>!function outer (f) { return f }( function inner () { function Async() { this._trampolineEnabled = true; } Async.prototype.disableTrampolineIfNecessary = function dtin(b) { if (b) { this._trampolineEnabled = false; } }; }) : boolean
4+
>function outer (f) { return f }( function inner () { function Async() { this._trampolineEnabled = true; } Async.prototype.disableTrampolineIfNecessary = function dtin(b) { if (b) { this._trampolineEnabled = false; } }; }) : () => void
5+
>function outer (f) { return f } : (f: () => void) => () => void
6+
>outer : (f: () => void) => () => void
7+
>f : () => void
8+
>f : () => void
9+
10+
function inner () {
11+
>function inner () { function Async() { this._trampolineEnabled = true; } Async.prototype.disableTrampolineIfNecessary = function dtin(b) { if (b) { this._trampolineEnabled = false; } }; } : () => void
12+
>inner : () => void
13+
14+
function Async() {
15+
>Async : () => void
16+
17+
this._trampolineEnabled = true;
18+
>this._trampolineEnabled = true : true
19+
>this._trampolineEnabled : any
20+
>this : any
21+
>_trampolineEnabled : any
22+
>true : true
23+
}
24+
25+
Async.prototype.disableTrampolineIfNecessary = function dtin(b) {
26+
>Async.prototype.disableTrampolineIfNecessary = function dtin(b) { if (b) { this._trampolineEnabled = false; } } : (b: any) => void
27+
>Async.prototype.disableTrampolineIfNecessary : any
28+
>Async.prototype : any
29+
>Async : () => void
30+
>prototype : any
31+
>disableTrampolineIfNecessary : any
32+
>function dtin(b) { if (b) { this._trampolineEnabled = false; } } : (b: any) => void
33+
>dtin : (b: any) => void
34+
>b : any
35+
36+
if (b) {
37+
>b : any
38+
39+
this._trampolineEnabled = false;
40+
>this._trampolineEnabled = false : false
41+
>this._trampolineEnabled : boolean
42+
>this : { _trampolineEnabled: boolean; disableTrampolineIfNecessary: (b: any) => void; }
43+
>_trampolineEnabled : boolean
44+
>false : false
45+
}
46+
};
47+
})
48+

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

+14
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
tests/cases/conformance/salsa/chrome-devtools-DOMExtension.js(2,17): error TS2339: Property 'removeChildren' does not exist on type 'Event'.
2+
tests/cases/conformance/salsa/chrome-devtools-DOMExtension.js(3,10): error TS2339: Property 'textContent' does not exist on type 'Event'.
3+
4+
5+
==== tests/cases/conformance/salsa/chrome-devtools-DOMExtension.js (2 errors) ====
6+
// Extend that DOM! (doesn't work, but shouldn't crash)
7+
Event.prototype.removeChildren = function () {
8+
~~~~~~~~~~~~~~
9+
!!! error TS2339: Property 'removeChildren' does not exist on type 'Event'.
10+
this.textContent = 'nope, not going to happen'
11+
~~~~~~~~~~~
12+
!!! error TS2339: Property 'textContent' does not exist on type 'Event'.
13+
}
14+

‎tests/baselines/reference/typeFromPropertyAssignment21.symbols

+11
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
=== tests/cases/conformance/salsa/chrome-devtools-DOMExtension.js ===
2+
// Extend that DOM! (doesn't work, but shouldn't crash)
3+
Event.prototype.removeChildren = function () {
4+
>Event.prototype : Symbol(prototype, Decl(lib.dom.d.ts, --, --))
5+
>Event : Symbol(Event, Decl(lib.dom.d.ts, --, --), Decl(lib.dom.d.ts, --, --))
6+
>prototype : Symbol(prototype, Decl(lib.dom.d.ts, --, --))
7+
8+
this.textContent = 'nope, not going to happen'
9+
>this : Symbol(Event, Decl(lib.dom.d.ts, --, --), Decl(lib.dom.d.ts, --, --))
10+
}
11+

‎tests/baselines/reference/typeFromPropertyAssignment21.types

+19
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
=== tests/cases/conformance/salsa/chrome-devtools-DOMExtension.js ===
2+
// Extend that DOM! (doesn't work, but shouldn't crash)
3+
Event.prototype.removeChildren = function () {
4+
>Event.prototype.removeChildren = function () { this.textContent = 'nope, not going to happen'} : () => void
5+
>Event.prototype.removeChildren : any
6+
>Event.prototype : Event
7+
>Event : { new (typeArg: string, eventInitDict?: EventInit): Event; prototype: Event; readonly AT_TARGET: number; readonly BUBBLING_PHASE: number; readonly CAPTURING_PHASE: number; readonly NONE: number; }
8+
>prototype : Event
9+
>removeChildren : any
10+
>function () { this.textContent = 'nope, not going to happen'} : () => void
11+
12+
this.textContent = 'nope, not going to happen'
13+
>this.textContent = 'nope, not going to happen' : "nope, not going to happen"
14+
>this.textContent : any
15+
>this : Event
16+
>textContent : any
17+
>'nope, not going to happen' : "nope, not going to happen"
18+
}
19+

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

+21
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
tests/cases/conformance/salsa/npm-install.js(12,1): error TS2322: Type 'string | number' is not assignable to type 'number | undefined'.
2+
Type 'string' is not assignable to type 'number | undefined'.
3+
4+
5+
==== tests/cases/conformance/salsa/npm-install.js (1 errors) ====
6+
function Installer () {
7+
this.args = 0
8+
}
9+
Installer.prototype.loadArgMetadata = function (next) {
10+
// ArrowFunction isn't treated as a this-container
11+
(args) => {
12+
this.args = 'hi'
13+
this.newProperty = 1
14+
}
15+
}
16+
var i = new Installer()
17+
i.newProperty = i.args // error, number | string =/> number | undefined
18+
~~~~~~~~~~~~~
19+
!!! error TS2322: Type 'string | number' is not assignable to type 'number | undefined'.
20+
!!! error TS2322: Type 'string' is not assignable to type 'number | undefined'.
21+

‎tests/baselines/reference/typeFromPropertyAssignment22.symbols

+41
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
=== tests/cases/conformance/salsa/npm-install.js ===
2+
function Installer () {
3+
>Installer : Symbol(Installer, Decl(npm-install.js, 0, 0))
4+
5+
this.args = 0
6+
>args : Symbol(Installer.args, Decl(npm-install.js, 0, 23), Decl(npm-install.js, 5, 15))
7+
}
8+
Installer.prototype.loadArgMetadata = function (next) {
9+
>Installer.prototype : Symbol(Installer.loadArgMetadata, Decl(npm-install.js, 2, 1))
10+
>Installer : Symbol(Installer, Decl(npm-install.js, 0, 0))
11+
>prototype : Symbol(Function.prototype, Decl(lib.d.ts, --, --))
12+
>loadArgMetadata : Symbol(Installer.loadArgMetadata, Decl(npm-install.js, 2, 1))
13+
>next : Symbol(next, Decl(npm-install.js, 3, 48))
14+
15+
// ArrowFunction isn't treated as a this-container
16+
(args) => {
17+
>args : Symbol(args, Decl(npm-install.js, 5, 5))
18+
19+
this.args = 'hi'
20+
>this.args : Symbol(Installer.args, Decl(npm-install.js, 0, 23), Decl(npm-install.js, 5, 15))
21+
>this : Symbol(Installer, Decl(npm-install.js, 0, 0))
22+
>args : Symbol(Installer.args, Decl(npm-install.js, 0, 23), Decl(npm-install.js, 5, 15))
23+
24+
this.newProperty = 1
25+
>this.newProperty : Symbol(Installer.newProperty, Decl(npm-install.js, 6, 24))
26+
>this : Symbol(Installer, Decl(npm-install.js, 0, 0))
27+
>newProperty : Symbol(Installer.newProperty, Decl(npm-install.js, 6, 24))
28+
}
29+
}
30+
var i = new Installer()
31+
>i : Symbol(i, Decl(npm-install.js, 10, 3))
32+
>Installer : Symbol(Installer, Decl(npm-install.js, 0, 0))
33+
34+
i.newProperty = i.args // error, number | string =/> number | undefined
35+
>i.newProperty : Symbol(Installer.newProperty, Decl(npm-install.js, 6, 24))
36+
>i : Symbol(i, Decl(npm-install.js, 10, 3))
37+
>newProperty : Symbol(Installer.newProperty, Decl(npm-install.js, 6, 24))
38+
>i.args : Symbol(Installer.args, Decl(npm-install.js, 0, 23), Decl(npm-install.js, 5, 15))
39+
>i : Symbol(i, Decl(npm-install.js, 10, 3))
40+
>args : Symbol(Installer.args, Decl(npm-install.js, 0, 23), Decl(npm-install.js, 5, 15))
41+

‎tests/baselines/reference/typeFromPropertyAssignment22.types

+55
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
=== tests/cases/conformance/salsa/npm-install.js ===
2+
function Installer () {
3+
>Installer : () => void
4+
5+
this.args = 0
6+
>this.args = 0 : 0
7+
>this.args : any
8+
>this : any
9+
>args : any
10+
>0 : 0
11+
}
12+
Installer.prototype.loadArgMetadata = function (next) {
13+
>Installer.prototype.loadArgMetadata = function (next) { // ArrowFunction isn't treated as a this-container (args) => { this.args = 'hi' this.newProperty = 1 }} : (next: any) => void
14+
>Installer.prototype.loadArgMetadata : any
15+
>Installer.prototype : any
16+
>Installer : () => void
17+
>prototype : any
18+
>loadArgMetadata : any
19+
>function (next) { // ArrowFunction isn't treated as a this-container (args) => { this.args = 'hi' this.newProperty = 1 }} : (next: any) => void
20+
>next : any
21+
22+
// ArrowFunction isn't treated as a this-container
23+
(args) => {
24+
>(args) => { this.args = 'hi' this.newProperty = 1 } : (args: any) => void
25+
>args : any
26+
27+
this.args = 'hi'
28+
>this.args = 'hi' : "hi"
29+
>this.args : string | number
30+
>this : { args: string | number; loadArgMetadata: (next: any) => void; newProperty: number | undefined; }
31+
>args : string | number
32+
>'hi' : "hi"
33+
34+
this.newProperty = 1
35+
>this.newProperty = 1 : 1
36+
>this.newProperty : number | undefined
37+
>this : { args: string | number; loadArgMetadata: (next: any) => void; newProperty: number | undefined; }
38+
>newProperty : number | undefined
39+
>1 : 1
40+
}
41+
}
42+
var i = new Installer()
43+
>i : { args: string | number; loadArgMetadata: (next: any) => void; newProperty: number | undefined; }
44+
>new Installer() : { args: string | number; loadArgMetadata: (next: any) => void; newProperty: number | undefined; }
45+
>Installer : () => void
46+
47+
i.newProperty = i.args // error, number | string =/> number | undefined
48+
>i.newProperty = i.args : string | number
49+
>i.newProperty : number | undefined
50+
>i : { args: string | number; loadArgMetadata: (next: any) => void; newProperty: number | undefined; }
51+
>newProperty : number | undefined
52+
>i.args : string | number
53+
>i : { args: string | number; loadArgMetadata: (next: any) => void; newProperty: number | undefined; }
54+
>args : string | number
55+

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

+17
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
// @allowJs: true
2+
// @checkJs: true
3+
// @noEmit: true
4+
// @Filename: bluebird.js
5+
6+
!function outer (f) { return f }(
7+
function inner () {
8+
function Async() {
9+
this._trampolineEnabled = true;
10+
}
11+
12+
Async.prototype.disableTrampolineIfNecessary = function dtin(b) {
13+
if (b) {
14+
this._trampolineEnabled = false;
15+
}
16+
};
17+
})

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

+10
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
// @allowJs: true
2+
// @checkJs: true
3+
// @noEmit: true
4+
// @lib: dom, es5
5+
// @Filename: chrome-devtools-DOMExtension.js
6+
7+
// Extend that DOM! (doesn't work, but shouldn't crash)
8+
Event.prototype.removeChildren = function () {
9+
this.textContent = 'nope, not going to happen'
10+
}

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

+17
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
// @allowJs: true
2+
// @checkJs: true
3+
// @noEmit: true
4+
// @strictNullChecks: true
5+
// @Filename: npm-install.js
6+
function Installer () {
7+
this.args = 0
8+
}
9+
Installer.prototype.loadArgMetadata = function (next) {
10+
// ArrowFunction isn't treated as a this-container
11+
(args) => {
12+
this.args = 'hi'
13+
this.newProperty = 1
14+
}
15+
}
16+
var i = new Installer()
17+
i.newProperty = i.args // error, number | string =/> number | undefined

0 commit comments

Comments
 (0)