Skip to content

Commit b9f6056

Browse files
author
Andy
authored
For f.prototype.m = function() { this.x = 0; } make x a member of f, not of the function expression (microsoft#22643)
1 parent d4788f5 commit b9f6056

8 files changed

+124
-26
lines changed

‎src/compiler/binder.ts

+33-16
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,7 @@ namespace ts {
119119
let languageVersion: ScriptTarget;
120120
let parent: Node;
121121
let container: Node;
122+
let containerContainer: Node; // Container one level up
122123
let blockScopeContainer: Node;
123124
let inferenceContainer: Node;
124125
let lastContainer: Node;
@@ -187,6 +188,7 @@ namespace ts {
187188
languageVersion = undefined;
188189
parent = undefined;
189190
container = undefined;
191+
containerContainer = undefined;
190192
blockScopeContainer = undefined;
191193
inferenceContainer = undefined;
192194
lastContainer = undefined;
@@ -224,13 +226,7 @@ namespace ts {
224226
symbol.flags |= symbolFlags;
225227

226228
node.symbol = symbol;
227-
228-
if (!symbol.declarations) {
229-
symbol.declarations = [node];
230-
}
231-
else {
232-
symbol.declarations.push(node);
233-
}
229+
symbol.declarations = append(symbol.declarations, node);
234230

235231
if (symbolFlags & SymbolFlags.HasExports && !symbol.exports) {
236232
symbol.exports = createSymbolTable();
@@ -486,8 +482,11 @@ namespace ts {
486482
// and block-container. Then after we pop out of processing the children, we restore
487483
// these saved values.
488484
const saveContainer = container;
485+
const saveContainerContainer = containerContainer;
489486
const savedBlockScopeContainer = blockScopeContainer;
490487

488+
containerContainer = container;
489+
491490
// Depending on what kind of node this is, we may have to adjust the current container
492491
// and block-container. If the current node is a container, then it is automatically
493492
// considered the current block-container as well. Also, for containers that we know
@@ -580,7 +579,9 @@ namespace ts {
580579
else {
581580
bindChildren(node);
582581
}
582+
583583
container = saveContainer;
584+
containerContainer = saveContainerContainer;
584585
blockScopeContainer = savedBlockScopeContainer;
585586
}
586587

@@ -2327,14 +2328,23 @@ namespace ts {
23272328

23282329
function bindThisPropertyAssignment(node: BinaryExpression | PropertyAccessExpression) {
23292330
Debug.assert(isInJavaScriptFile(node));
2330-
const container = getThisContainer(node, /*includeArrowFunctions*/ false);
2331-
switch (container.kind) {
2331+
const thisContainer = getThisContainer(node, /*includeArrowFunctions*/ false);
2332+
switch (thisContainer.kind) {
23322333
case SyntaxKind.FunctionDeclaration:
23332334
case SyntaxKind.FunctionExpression:
2335+
let constructorSymbol = thisContainer.symbol;
2336+
// For `f.prototype.m = function() { this.x = 0; }`, `this.x = 0` should modify `f`'s members, not the function expression.
2337+
if (isBinaryExpression(thisContainer.parent) && thisContainer.parent.operatorToken.kind === SyntaxKind.EqualsToken) {
2338+
const l = thisContainer.parent.left;
2339+
if (isPropertyAccessExpression(l) && isPropertyAccessExpression(l.expression) && l.expression.name.escapedText === "prototype" && isEntityNameExpression(l.expression.expression)) {
2340+
constructorSymbol = getJSInitializerSymbolFromName(l.expression.expression, containerContainer);
2341+
}
2342+
}
2343+
23342344
// Declare a 'member' if the container is an ES5 class or ES6 constructor
2335-
container.symbol.members = container.symbol.members || createSymbolTable();
2345+
constructorSymbol.members = constructorSymbol.members || createSymbolTable();
23362346
// It's acceptable for multiple 'this' assignments of the same identifier to occur
2337-
declareSymbol(container.symbol.members, container.symbol, node, SymbolFlags.Property, SymbolFlags.PropertyExcludes & ~SymbolFlags.Property);
2347+
declareSymbol(constructorSymbol.members, constructorSymbol, node, SymbolFlags.Property, SymbolFlags.PropertyExcludes & ~SymbolFlags.Property);
23382348
break;
23392349

23402350
case SyntaxKind.Constructor:
@@ -2344,10 +2354,13 @@ namespace ts {
23442354
case SyntaxKind.SetAccessor:
23452355
// this.foo assignment in a JavaScript class
23462356
// Bind this property to the containing class
2347-
const containingClass = container.parent;
2348-
const symbolTable = hasModifier(container, ModifierFlags.Static) ? containingClass.symbol.exports : containingClass.symbol.members;
2357+
const containingClass = thisContainer.parent;
2358+
const symbolTable = hasModifier(thisContainer, ModifierFlags.Static) ? containingClass.symbol.exports : containingClass.symbol.members;
23492359
declareSymbol(symbolTable, containingClass.symbol, node, SymbolFlags.Property, SymbolFlags.None, /*isReplaceableByMethod*/ true);
23502360
break;
2361+
2362+
default:
2363+
Debug.fail(Debug.showSyntaxKind(thisContainer));
23512364
}
23522365
}
23532366

@@ -2417,8 +2430,12 @@ namespace ts {
24172430
bindPropertyAssignment(node.expression, node, /*isPrototypeProperty*/ false);
24182431
}
24192432

2433+
function getJSInitializerSymbolFromName(name: EntityNameExpression, lookupContainer?: Node): Symbol {
2434+
return getJSInitializerSymbol(lookupSymbolForPropertyAccess(name, lookupContainer));
2435+
}
2436+
24202437
function bindPropertyAssignment(name: EntityNameExpression, propertyAccess: PropertyAccessEntityNameExpression, isPrototypeProperty: boolean) {
2421-
let symbol = getJSInitializerSymbol(lookupSymbolForPropertyAccess(name));
2438+
let symbol = getJSInitializerSymbolFromName(name);
24222439
let isToplevelNamespaceableInitializer: boolean;
24232440
if (isBinaryExpression(propertyAccess.parent)) {
24242441
const isPrototypeAssignment = isPropertyAccessExpression(propertyAccess.parent.left) && propertyAccess.parent.left.name.escapedText === "prototype";
@@ -2458,9 +2475,9 @@ namespace ts {
24582475
declareSymbol(symbolTable, symbol, propertyAccess, symbolFlags, symbolExcludes);
24592476
}
24602477

2461-
function lookupSymbolForPropertyAccess(node: EntityNameExpression): Symbol | undefined {
2478+
function lookupSymbolForPropertyAccess(node: EntityNameExpression, lookupContainer: Node = container): Symbol | undefined {
24622479
if (isIdentifier(node)) {
2463-
return lookupSymbolForNameWorker(container, node.escapedText);
2480+
return lookupSymbolForNameWorker(lookupContainer, node.escapedText);
24642481
}
24652482
else {
24662483
const symbol = getJSInitializerSymbol(lookupSymbolForPropertyAccess(node.expression));

‎src/compiler/types.ts

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

‎tests/baselines/reference/constructorFunctions2.symbols

+20-3
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,29 @@ const B = function() { this.id = 1; }
2323
>B : Symbol(B, Decl(index.js, 3, 5))
2424
>id : Symbol(B.id, Decl(index.js, 3, 22))
2525

26-
const b = new B().id;
27-
>b : Symbol(b, Decl(index.js, 4, 5))
28-
>new B().id : Symbol(B.id, Decl(index.js, 3, 22))
26+
B.prototype.m = function() { this.x = 2; }
27+
>B.prototype : Symbol(B.m, Decl(index.js, 3, 37))
2928
>B : Symbol(B, Decl(index.js, 3, 5))
29+
>prototype : Symbol(Function.prototype, Decl(lib.d.ts, --, --))
30+
>m : Symbol(B.m, Decl(index.js, 3, 37))
31+
>this.x : Symbol(B.x, Decl(index.js, 4, 28))
32+
>this : Symbol(B, Decl(index.js, 3, 9))
33+
>x : Symbol(B.x, Decl(index.js, 4, 28))
34+
35+
const b = new B();
36+
>b : Symbol(b, Decl(index.js, 5, 5))
37+
>B : Symbol(B, Decl(index.js, 3, 5))
38+
39+
b.id;
40+
>b.id : Symbol(B.id, Decl(index.js, 3, 22))
41+
>b : Symbol(b, Decl(index.js, 5, 5))
3042
>id : Symbol(B.id, Decl(index.js, 3, 22))
3143

44+
b.x;
45+
>b.x : Symbol(B.x, Decl(index.js, 4, 28))
46+
>b : Symbol(b, Decl(index.js, 5, 5))
47+
>x : Symbol(B.x, Decl(index.js, 4, 28))
48+
3249
=== tests/cases/conformance/salsa/other.js ===
3350
function A() { this.id = 1; }
3451
>A : Symbol(A, Decl(other.js, 0, 0))

‎tests/baselines/reference/constructorFunctions2.types

+26-4
Original file line numberDiff line numberDiff line change
@@ -30,13 +30,35 @@ const B = function() { this.id = 1; }
3030
>id : any
3131
>1 : 1
3232

33-
const b = new B().id;
34-
>b : number
35-
>new B().id : number
36-
>new B() : { id: number; }
33+
B.prototype.m = function() { this.x = 2; }
34+
>B.prototype.m = function() { this.x = 2; } : () => void
35+
>B.prototype.m : any
36+
>B.prototype : any
3737
>B : () => void
38+
>prototype : any
39+
>m : any
40+
>function() { this.x = 2; } : () => void
41+
>this.x = 2 : 2
42+
>this.x : number
43+
>this : { id: number; m: () => void; x: number; }
44+
>x : number
45+
>2 : 2
46+
47+
const b = new B();
48+
>b : { id: number; m: () => void; x: number; }
49+
>new B() : { id: number; m: () => void; x: number; }
50+
>B : () => void
51+
52+
b.id;
53+
>b.id : number
54+
>b : { id: number; m: () => void; x: number; }
3855
>id : number
3956

57+
b.x;
58+
>b.x : number
59+
>b : { id: number; m: () => void; x: number; }
60+
>x : number
61+
4062
=== tests/cases/conformance/salsa/other.js ===
4163
function A() { this.id = 1; }
4264
>A : () => void

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

+4-1
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,10 @@ const A = require("./other");
1111
const a = new A().id;
1212

1313
const B = function() { this.id = 1; }
14-
const b = new B().id;
14+
B.prototype.m = function() { this.x = 2; }
15+
const b = new B();
16+
b.id;
17+
b.x;
1518

1619
// @filename: other.js
1720
function A() { this.id = 1; }

‎tests/cases/fourslash/convertFunctionToEs6ClassJsDoc.ts

-1
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323

2424
verify.codeFix({
2525
description: "Convert function to an ES2015 class",
26-
index: 0, // TODO: GH#22240
2726
newFileContent:
2827
`class fn {
2928
constructor() {

‎tests/cases/fourslash/convertFunctionToEs6Class_falsePositive.ts

+26
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
// @allowJs: true
4+
// @Filename: /a.js
5+
////function [|f|]() {}
6+
////f.prototype.bar = [|function|](){
7+
//// this.x = 1;
8+
////};
9+
10+
// Only a suggestion for `f`, not for the function expression. See GH#22240
11+
verify.getSuggestionDiagnostics([{
12+
message: "This constructor function may be converted to a class declaration.",
13+
code: 80002,
14+
}]);
15+
16+
verify.codeFix({
17+
description: "Convert function to an ES2015 class",
18+
newFileContent:
19+
`class f {
20+
constructor() { }
21+
bar() {
22+
this.x = 1;
23+
}
24+
}
25+
`,
26+
});

‎tests/cases/fourslash/findAllRefsConstructorFunctions.ts

+14
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
// @allowJs: true
4+
5+
// @Filename: /a.js
6+
////function f() {
7+
//// this.[|{| "isWriteAccess": true, "isDefinition": true |}x|] = 0;
8+
////}
9+
////f.prototype.setX = function() {
10+
//// this.[|{| "isWriteAccess": true, "isDefinition": true |}x|] = 1;
11+
////}
12+
////f.prototype.useX = function() { this.[|x|]; }
13+
14+
verify.singleReferenceGroup("(property) f.x: number");

0 commit comments

Comments
 (0)