From 8c01efba04a3ee3a1fa0aec6658afb5884b9e3a4 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders Date: Wed, 3 Aug 2016 10:33:10 -0700 Subject: [PATCH 1/8] Allow JS multiple declarations of ctor properties When a property is declared in the constructor and on the prototype of an ES6 class, the property's symbol is discarded in favour of the method's symbol. That because the usual use for this pattern is to bind an instance function: `this.m = this.m.bind(this)`. In this case the type you want really is the method's type. --- src/compiler/binder.ts | 58 +++++++++++-------- src/compiler/types.ts | 1 + .../reference/multipleDeclarations.js | 32 +++++++++- .../reference/multipleDeclarations.symbols | 47 ++++++++++++++- .../reference/multipleDeclarations.types | 57 +++++++++++++++++- .../conformance/salsa/multipleDeclarations.ts | 17 +++++- 6 files changed, 183 insertions(+), 29 deletions(-) diff --git a/src/compiler/binder.ts b/src/compiler/binder.ts index 8d8666a67abac..6f0def32702d3 100644 --- a/src/compiler/binder.ts +++ b/src/compiler/binder.ts @@ -298,8 +298,10 @@ namespace ts { const name = isDefaultExport && parent ? "default" : getDeclarationName(node); let symbol: Symbol; - if (name !== undefined) { - + if (name === undefined) { + symbol = createSymbol(SymbolFlags.None, "__missing"); + } + else { // Check and see if the symbol table already has a symbol with this name. If not, // create a new symbol with this name and add it to the table. Note that we don't // give the new symbol any flags *yet*. This ensures that it will not conflict @@ -326,34 +328,38 @@ namespace ts { classifiableNames[name] = name; } - if (symbol.flags & excludes) { - if (node.name) { - node.name.parent = node; + else if (symbol.flags & excludes) { + if (symbol.isDiscardable) { + // Javascript constructor-declared symbols can be discarded in favor of + // prototype symbols like methods. + symbol = symbolTable[name] = createSymbol(SymbolFlags.None, name); } + else { + if (node.name) { + node.name.parent = node; + } - // Report errors every position with duplicate declaration - // Report errors on previous encountered declarations - let message = symbol.flags & SymbolFlags.BlockScopedVariable - ? Diagnostics.Cannot_redeclare_block_scoped_variable_0 - : Diagnostics.Duplicate_identifier_0; + // Report errors every position with duplicate declaration + // Report errors on previous encountered declarations + let message = symbol.flags & SymbolFlags.BlockScopedVariable + ? Diagnostics.Cannot_redeclare_block_scoped_variable_0 + : Diagnostics.Duplicate_identifier_0; - forEach(symbol.declarations, declaration => { - if (declaration.flags & NodeFlags.Default) { - message = Diagnostics.A_module_cannot_have_multiple_default_exports; - } - }); + forEach(symbol.declarations, declaration => { + if (declaration.flags & NodeFlags.Default) { + message = Diagnostics.A_module_cannot_have_multiple_default_exports; + } + }); - forEach(symbol.declarations, declaration => { - file.bindDiagnostics.push(createDiagnosticForNode(declaration.name || declaration, message, getDisplayName(declaration))); - }); - file.bindDiagnostics.push(createDiagnosticForNode(node.name || node, message, getDisplayName(node))); + forEach(symbol.declarations, declaration => { + file.bindDiagnostics.push(createDiagnosticForNode(declaration.name || declaration, message, getDisplayName(declaration))); + }); + file.bindDiagnostics.push(createDiagnosticForNode(node.name || node, message, getDisplayName(node))); - symbol = createSymbol(SymbolFlags.None, name); + symbol = createSymbol(SymbolFlags.None, name); + } } } - else { - symbol = createSymbol(SymbolFlags.None, "__missing"); - } addDeclarationToSymbol(symbol, node, includes); symbol.parent = parent; @@ -1967,7 +1973,7 @@ namespace ts { } function bindThisPropertyAssignment(node: BinaryExpression) { - // Declare a 'member' in case it turns out the container was an ES5 class or ES6 constructor + // Declare a 'member' if the container is an ES5 class or ES6 constructor let assignee: Node; if (container.kind === SyntaxKind.FunctionDeclaration || container.kind === SyntaxKind.FunctionExpression) { assignee = container; @@ -1980,7 +1986,9 @@ namespace ts { } assignee.symbol.members = assignee.symbol.members || {}; // It's acceptable for multiple 'this' assignments of the same identifier to occur - declareSymbol(assignee.symbol.members, assignee.symbol, node, SymbolFlags.Property, SymbolFlags.PropertyExcludes & ~SymbolFlags.Property); + // AND it can be overwritten by subsequent method declarations + let symbol = declareSymbol(assignee.symbol.members, assignee.symbol, node, SymbolFlags.Property, SymbolFlags.PropertyExcludes & ~SymbolFlags.Property); + symbol.isDiscardable = true; } function bindPrototypePropertyAssignment(node: BinaryExpression) { diff --git a/src/compiler/types.ts b/src/compiler/types.ts index 072209aa49640..4c08d7a487c41 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -2137,6 +2137,7 @@ namespace ts { /* @internal */ exportSymbol?: Symbol; // Exported symbol associated with this symbol /* @internal */ constEnumOnlyModule?: boolean; // True if module contains only const enums or other modules with only const enums /* @internal */ isReferenced?: boolean; // True if the symbol is referenced elsewhere + /* @internal */ isDiscardable?: boolean;// True if a Javascript class property can be overwritten by a method } /* @internal */ diff --git a/tests/baselines/reference/multipleDeclarations.js b/tests/baselines/reference/multipleDeclarations.js index 5f5ac29e90964..e5aae416663c4 100644 --- a/tests/baselines/reference/multipleDeclarations.js +++ b/tests/baselines/reference/multipleDeclarations.js @@ -5,7 +5,22 @@ function C() { } C.prototype.m = function() { this.nothing(); -}; +} + +class X { + constructor() { + this.m = this.m.bind(this); + this.mistake = 'frankly, complete nonsense'; + } + m() { + } + mistake() { + } +} +let x = new X(); +X.prototype.mistake = false; +x.m(); +x.mistake; //// [output.js] @@ -15,3 +30,18 @@ function C() { C.prototype.m = function () { this.nothing(); }; +var X = (function () { + function X() { + this.m = this.m.bind(this); + this.mistake = 'frankly, complete nonsense'; + } + X.prototype.m = function () { + }; + X.prototype.mistake = function () { + }; + return X; +}()); +var x = new X(); +X.prototype.mistake = false; +x.m(); +x.mistake; diff --git a/tests/baselines/reference/multipleDeclarations.symbols b/tests/baselines/reference/multipleDeclarations.symbols index a256943dd506c..7e4bcd53c8a99 100644 --- a/tests/baselines/reference/multipleDeclarations.symbols +++ b/tests/baselines/reference/multipleDeclarations.symbols @@ -14,6 +14,51 @@ C.prototype.m = function() { this.nothing(); >this : Symbol(C, Decl(input.js, 0, 0)) +} + +class X { +>X : Symbol(X, Decl(input.js, 6, 1)) + + constructor() { + this.m = this.m.bind(this); +>this.m : Symbol(X.m, Decl(input.js, 12, 5)) +>this : Symbol(X, Decl(input.js, 6, 1)) +>m : Symbol(X.m, Decl(input.js, 9, 19)) +>this.m.bind : Symbol(Function.bind, Decl(lib.d.ts, --, --)) +>this.m : Symbol(X.m, Decl(input.js, 12, 5)) +>this : Symbol(X, Decl(input.js, 6, 1)) +>m : Symbol(X.m, Decl(input.js, 12, 5)) +>bind : Symbol(Function.bind, Decl(lib.d.ts, --, --)) +>this : Symbol(X, Decl(input.js, 6, 1)) + + this.mistake = 'frankly, complete nonsense'; +>this.mistake : Symbol(X.mistake, Decl(input.js, 14, 5)) +>this : Symbol(X, Decl(input.js, 6, 1)) +>mistake : Symbol(X.mistake, Decl(input.js, 10, 35)) + } + m() { +>m : Symbol(X.m, Decl(input.js, 12, 5)) + } + mistake() { +>mistake : Symbol(X.mistake, Decl(input.js, 14, 5)) + } +} +let x = new X(); +>x : Symbol(x, Decl(input.js, 18, 3)) +>X : Symbol(X, Decl(input.js, 6, 1)) + +X.prototype.mistake = false; +>X.prototype.mistake : Symbol(X.mistake, Decl(input.js, 14, 5)) +>X : Symbol(X, Decl(input.js, 6, 1)) +>prototype : Symbol(X.prototype) + +x.m(); +>x.m : Symbol(X.m, Decl(input.js, 12, 5)) +>x : Symbol(x, Decl(input.js, 18, 3)) +>m : Symbol(X.m, Decl(input.js, 12, 5)) -}; +x.mistake; +>x.mistake : Symbol(X.mistake, Decl(input.js, 14, 5)) +>x : Symbol(x, Decl(input.js, 18, 3)) +>mistake : Symbol(X.mistake, Decl(input.js, 14, 5)) diff --git a/tests/baselines/reference/multipleDeclarations.types b/tests/baselines/reference/multipleDeclarations.types index 7c0a3de70c9c6..2a7cac18205ab 100644 --- a/tests/baselines/reference/multipleDeclarations.types +++ b/tests/baselines/reference/multipleDeclarations.types @@ -24,6 +24,61 @@ C.prototype.m = function() { >this.nothing : any >this : { m: () => void; } >nothing : any +} + +class X { +>X : X + + constructor() { + this.m = this.m.bind(this); +>this.m = this.m.bind(this) : any +>this.m : () => void +>this : this +>m : () => void +>this.m.bind(this) : any +>this.m.bind : (this: Function, thisArg: any, ...argArray: any[]) => any +>this.m : () => void +>this : this +>m : () => void +>bind : (this: Function, thisArg: any, ...argArray: any[]) => any +>this : this + + this.mistake = 'frankly, complete nonsense'; +>this.mistake = 'frankly, complete nonsense' : string +>this.mistake : () => void +>this : this +>mistake : () => void +>'frankly, complete nonsense' : string + } + m() { +>m : () => void + } + mistake() { +>mistake : () => void + } +} +let x = new X(); +>x : X +>new X() : X +>X : typeof X + +X.prototype.mistake = false; +>X.prototype.mistake = false : boolean +>X.prototype.mistake : () => void +>X.prototype : X +>X : typeof X +>prototype : X +>mistake : () => void +>false : boolean + +x.m(); +>x.m() : void +>x.m : () => void +>x : X +>m : () => void -}; +x.mistake; +>x.mistake : () => void +>x : X +>mistake : () => void diff --git a/tests/cases/conformance/salsa/multipleDeclarations.ts b/tests/cases/conformance/salsa/multipleDeclarations.ts index 6899be2ec89a2..9ead1eeb0a5e9 100644 --- a/tests/cases/conformance/salsa/multipleDeclarations.ts +++ b/tests/cases/conformance/salsa/multipleDeclarations.ts @@ -7,4 +7,19 @@ function C() { } C.prototype.m = function() { this.nothing(); -}; +} + +class X { + constructor() { + this.m = this.m.bind(this); + this.mistake = 'frankly, complete nonsense'; + } + m() { + } + mistake() { + } +} +let x = new X(); +X.prototype.mistake = false; +x.m(); +x.mistake; From 72057500b5220b28e637a6a31acb86f4f4273393 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders Date: Wed, 3 Aug 2016 16:10:14 -0700 Subject: [PATCH 2/8] Test that declares conflicting method first --- .../conformance/salsa/multipleDeclarations.ts | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/tests/cases/conformance/salsa/multipleDeclarations.ts b/tests/cases/conformance/salsa/multipleDeclarations.ts index 9ead1eeb0a5e9..ba2e14e11e934 100644 --- a/tests/cases/conformance/salsa/multipleDeclarations.ts +++ b/tests/cases/conformance/salsa/multipleDeclarations.ts @@ -1,14 +1,12 @@ // @filename: input.js // @out: output.js // @allowJs: true - function C() { this.m = null; } C.prototype.m = function() { this.nothing(); } - class X { constructor() { this.m = this.m.bind(this); @@ -23,3 +21,17 @@ let x = new X(); X.prototype.mistake = false; x.m(); x.mistake; +class Y { + mistake() { + } + m() { + } + constructor() { + this.m = this.m.bind(this); + this.mistake = 'even more nonsense'; + } +} +Y.prototype.mistake = true; +let y = new Y(); +y.m(); +y.mistake(); From 798be6f4f977dfbc0e46353e56b28b59c123d1d6 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders Date: Thu, 4 Aug 2016 15:17:08 -0700 Subject: [PATCH 3/8] Add new test baseline and delete else in binder The extra `else` caused a ton of test failures! --- src/compiler/binder.ts | 2 +- .../reference/multipleDeclarations.js | 31 +++++- .../reference/multipleDeclarations.symbols | 98 +++++++++++++------ .../reference/multipleDeclarations.types | 59 ++++++++++- 4 files changed, 157 insertions(+), 33 deletions(-) diff --git a/src/compiler/binder.ts b/src/compiler/binder.ts index 6f0def32702d3..5e2cd3f868084 100644 --- a/src/compiler/binder.ts +++ b/src/compiler/binder.ts @@ -328,7 +328,7 @@ namespace ts { classifiableNames[name] = name; } - else if (symbol.flags & excludes) { + if (symbol.flags & excludes) { if (symbol.isDiscardable) { // Javascript constructor-declared symbols can be discarded in favor of // prototype symbols like methods. diff --git a/tests/baselines/reference/multipleDeclarations.js b/tests/baselines/reference/multipleDeclarations.js index e5aae416663c4..7fb7e0bca026f 100644 --- a/tests/baselines/reference/multipleDeclarations.js +++ b/tests/baselines/reference/multipleDeclarations.js @@ -1,12 +1,10 @@ //// [input.js] - function C() { this.m = null; } C.prototype.m = function() { this.nothing(); } - class X { constructor() { this.m = this.m.bind(this); @@ -21,6 +19,20 @@ let x = new X(); X.prototype.mistake = false; x.m(); x.mistake; +class Y { + mistake() { + } + m() { + } + constructor() { + this.m = this.m.bind(this); + this.mistake = 'even more nonsense'; + } +} +Y.prototype.mistake = true; +let y = new Y(); +y.m(); +y.mistake(); //// [output.js] @@ -45,3 +57,18 @@ var x = new X(); X.prototype.mistake = false; x.m(); x.mistake; +var Y = (function () { + function Y() { + this.m = this.m.bind(this); + this.mistake = 'even more nonsense'; + } + Y.prototype.mistake = function () { + }; + Y.prototype.m = function () { + }; + return Y; +}()); +Y.prototype.mistake = true; +var y = new Y(); +y.m(); +y.mistake(); diff --git a/tests/baselines/reference/multipleDeclarations.symbols b/tests/baselines/reference/multipleDeclarations.symbols index 7e4bcd53c8a99..9e49c2fc00cc4 100644 --- a/tests/baselines/reference/multipleDeclarations.symbols +++ b/tests/baselines/reference/multipleDeclarations.symbols @@ -1,64 +1,106 @@ === tests/cases/conformance/salsa/input.js === - function C() { >C : Symbol(C, Decl(input.js, 0, 0)) this.m = null; ->m : Symbol(C.m, Decl(input.js, 1, 14), Decl(input.js, 3, 1)) +>m : Symbol(C.m, Decl(input.js, 0, 14), Decl(input.js, 2, 1)) } C.prototype.m = function() { ->C.prototype : Symbol(C.m, Decl(input.js, 1, 14), Decl(input.js, 3, 1)) +>C.prototype : Symbol(C.m, Decl(input.js, 0, 14), Decl(input.js, 2, 1)) >C : Symbol(C, Decl(input.js, 0, 0)) >prototype : Symbol(Function.prototype, Decl(lib.d.ts, --, --)) ->m : Symbol(C.m, Decl(input.js, 1, 14), Decl(input.js, 3, 1)) +>m : Symbol(C.m, Decl(input.js, 0, 14), Decl(input.js, 2, 1)) this.nothing(); >this : Symbol(C, Decl(input.js, 0, 0)) } - class X { ->X : Symbol(X, Decl(input.js, 6, 1)) +>X : Symbol(X, Decl(input.js, 5, 1)) constructor() { this.m = this.m.bind(this); ->this.m : Symbol(X.m, Decl(input.js, 12, 5)) ->this : Symbol(X, Decl(input.js, 6, 1)) ->m : Symbol(X.m, Decl(input.js, 9, 19)) +>this.m : Symbol(X.m, Decl(input.js, 10, 5)) +>this : Symbol(X, Decl(input.js, 5, 1)) +>m : Symbol(X.m, Decl(input.js, 7, 19)) >this.m.bind : Symbol(Function.bind, Decl(lib.d.ts, --, --)) ->this.m : Symbol(X.m, Decl(input.js, 12, 5)) ->this : Symbol(X, Decl(input.js, 6, 1)) ->m : Symbol(X.m, Decl(input.js, 12, 5)) +>this.m : Symbol(X.m, Decl(input.js, 10, 5)) +>this : Symbol(X, Decl(input.js, 5, 1)) +>m : Symbol(X.m, Decl(input.js, 10, 5)) >bind : Symbol(Function.bind, Decl(lib.d.ts, --, --)) ->this : Symbol(X, Decl(input.js, 6, 1)) +>this : Symbol(X, Decl(input.js, 5, 1)) this.mistake = 'frankly, complete nonsense'; ->this.mistake : Symbol(X.mistake, Decl(input.js, 14, 5)) ->this : Symbol(X, Decl(input.js, 6, 1)) ->mistake : Symbol(X.mistake, Decl(input.js, 10, 35)) +>this.mistake : Symbol(X.mistake, Decl(input.js, 12, 5)) +>this : Symbol(X, Decl(input.js, 5, 1)) +>mistake : Symbol(X.mistake, Decl(input.js, 8, 35)) } m() { ->m : Symbol(X.m, Decl(input.js, 12, 5)) +>m : Symbol(X.m, Decl(input.js, 10, 5)) } mistake() { ->mistake : Symbol(X.mistake, Decl(input.js, 14, 5)) +>mistake : Symbol(X.mistake, Decl(input.js, 12, 5)) } } let x = new X(); ->x : Symbol(x, Decl(input.js, 18, 3)) ->X : Symbol(X, Decl(input.js, 6, 1)) +>x : Symbol(x, Decl(input.js, 16, 3)) +>X : Symbol(X, Decl(input.js, 5, 1)) X.prototype.mistake = false; ->X.prototype.mistake : Symbol(X.mistake, Decl(input.js, 14, 5)) ->X : Symbol(X, Decl(input.js, 6, 1)) +>X.prototype.mistake : Symbol(X.mistake, Decl(input.js, 12, 5)) +>X : Symbol(X, Decl(input.js, 5, 1)) >prototype : Symbol(X.prototype) x.m(); ->x.m : Symbol(X.m, Decl(input.js, 12, 5)) ->x : Symbol(x, Decl(input.js, 18, 3)) ->m : Symbol(X.m, Decl(input.js, 12, 5)) +>x.m : Symbol(X.m, Decl(input.js, 10, 5)) +>x : Symbol(x, Decl(input.js, 16, 3)) +>m : Symbol(X.m, Decl(input.js, 10, 5)) x.mistake; ->x.mistake : Symbol(X.mistake, Decl(input.js, 14, 5)) ->x : Symbol(x, Decl(input.js, 18, 3)) ->mistake : Symbol(X.mistake, Decl(input.js, 14, 5)) +>x.mistake : Symbol(X.mistake, Decl(input.js, 12, 5)) +>x : Symbol(x, Decl(input.js, 16, 3)) +>mistake : Symbol(X.mistake, Decl(input.js, 12, 5)) + +class Y { +>Y : Symbol(Y, Decl(input.js, 19, 10)) + + mistake() { +>mistake : Symbol(Y.mistake, Decl(input.js, 20, 9), Decl(input.js, 26, 35)) + } + m() { +>m : Symbol(Y.m, Decl(input.js, 22, 5), Decl(input.js, 25, 19)) + } + constructor() { + this.m = this.m.bind(this); +>this.m : Symbol(Y.m, Decl(input.js, 22, 5), Decl(input.js, 25, 19)) +>this : Symbol(Y, Decl(input.js, 19, 10)) +>m : Symbol(Y.m, Decl(input.js, 22, 5), Decl(input.js, 25, 19)) +>this.m : Symbol(Y.m, Decl(input.js, 22, 5), Decl(input.js, 25, 19)) +>this : Symbol(Y, Decl(input.js, 19, 10)) +>m : Symbol(Y.m, Decl(input.js, 22, 5), Decl(input.js, 25, 19)) +>this : Symbol(Y, Decl(input.js, 19, 10)) + + this.mistake = 'even more nonsense'; +>this.mistake : Symbol(Y.mistake, Decl(input.js, 20, 9), Decl(input.js, 26, 35)) +>this : Symbol(Y, Decl(input.js, 19, 10)) +>mistake : Symbol(Y.mistake, Decl(input.js, 20, 9), Decl(input.js, 26, 35)) + } +} +Y.prototype.mistake = true; +>Y.prototype.mistake : Symbol(Y.mistake, Decl(input.js, 20, 9), Decl(input.js, 26, 35)) +>Y : Symbol(Y, Decl(input.js, 19, 10)) +>prototype : Symbol(Y.prototype) + +let y = new Y(); +>y : Symbol(y, Decl(input.js, 31, 3)) +>Y : Symbol(Y, Decl(input.js, 19, 10)) + +y.m(); +>y.m : Symbol(Y.m, Decl(input.js, 22, 5), Decl(input.js, 25, 19)) +>y : Symbol(y, Decl(input.js, 31, 3)) +>m : Symbol(Y.m, Decl(input.js, 22, 5), Decl(input.js, 25, 19)) + +y.mistake(); +>y.mistake : Symbol(Y.mistake, Decl(input.js, 20, 9), Decl(input.js, 26, 35)) +>y : Symbol(y, Decl(input.js, 31, 3)) +>mistake : Symbol(Y.mistake, Decl(input.js, 20, 9), Decl(input.js, 26, 35)) diff --git a/tests/baselines/reference/multipleDeclarations.types b/tests/baselines/reference/multipleDeclarations.types index 2a7cac18205ab..900d03195d486 100644 --- a/tests/baselines/reference/multipleDeclarations.types +++ b/tests/baselines/reference/multipleDeclarations.types @@ -1,5 +1,4 @@ === tests/cases/conformance/salsa/input.js === - function C() { >C : () => void @@ -25,7 +24,6 @@ C.prototype.m = function() { >this : { m: () => void; } >nothing : any } - class X { >X : X @@ -82,3 +80,60 @@ x.mistake; >x : X >mistake : () => void +class Y { +>Y : Y + + mistake() { +>mistake : any + } + m() { +>m : any + } + constructor() { + this.m = this.m.bind(this); +>this.m = this.m.bind(this) : any +>this.m : any +>this : this +>m : any +>this.m.bind(this) : any +>this.m.bind : any +>this.m : any +>this : this +>m : any +>bind : any +>this : this + + this.mistake = 'even more nonsense'; +>this.mistake = 'even more nonsense' : string +>this.mistake : any +>this : this +>mistake : any +>'even more nonsense' : string + } +} +Y.prototype.mistake = true; +>Y.prototype.mistake = true : boolean +>Y.prototype.mistake : any +>Y.prototype : Y +>Y : typeof Y +>prototype : Y +>mistake : any +>true : boolean + +let y = new Y(); +>y : Y +>new Y() : Y +>Y : typeof Y + +y.m(); +>y.m() : any +>y.m : any +>y : Y +>m : any + +y.mistake(); +>y.mistake() : any +>y.mistake : any +>y : Y +>mistake : any + From 8f638f7ecdc154c00c2be11a55971a7d13ee5757 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders Date: Fri, 5 Aug 2016 09:58:30 -0700 Subject: [PATCH 4/8] Fix lint --- src/compiler/binder.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/compiler/binder.ts b/src/compiler/binder.ts index 5e2cd3f868084..b771a3b67be14 100644 --- a/src/compiler/binder.ts +++ b/src/compiler/binder.ts @@ -1987,7 +1987,7 @@ namespace ts { assignee.symbol.members = assignee.symbol.members || {}; // It's acceptable for multiple 'this' assignments of the same identifier to occur // AND it can be overwritten by subsequent method declarations - let symbol = declareSymbol(assignee.symbol.members, assignee.symbol, node, SymbolFlags.Property, SymbolFlags.PropertyExcludes & ~SymbolFlags.Property); + const symbol = declareSymbol(assignee.symbol.members, assignee.symbol, node, SymbolFlags.Property, SymbolFlags.PropertyExcludes & ~SymbolFlags.Property); symbol.isDiscardable = true; } From cabd276ddcfe849f59b1ccfebc24b0d798dee062 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders Date: Fri, 5 Aug 2016 10:28:03 -0700 Subject: [PATCH 5/8] Fix more lint --- src/compiler/types.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/compiler/types.ts b/src/compiler/types.ts index 4c08d7a487c41..f5cee2f8f8f29 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -2137,7 +2137,7 @@ namespace ts { /* @internal */ exportSymbol?: Symbol; // Exported symbol associated with this symbol /* @internal */ constEnumOnlyModule?: boolean; // True if module contains only const enums or other modules with only const enums /* @internal */ isReferenced?: boolean; // True if the symbol is referenced elsewhere - /* @internal */ isDiscardable?: boolean;// True if a Javascript class property can be overwritten by a method + /* @internal */ isDiscardable?: boolean; // True if a Javascript class property can be overwritten by a method } /* @internal */ From 2845d2f8b8c2c476085eda9abe083000047d70c4 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders Date: Mon, 8 Aug 2016 09:04:46 -0700 Subject: [PATCH 6/8] Improve naming and documentation from PR --- src/compiler/binder.ts | 9 +++++++-- src/compiler/types.ts | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/compiler/binder.ts b/src/compiler/binder.ts index b771a3b67be14..849f503ef85e9 100644 --- a/src/compiler/binder.ts +++ b/src/compiler/binder.ts @@ -313,6 +313,11 @@ namespace ts { // declaration we have for this symbol, and then create a new symbol for this // declaration. // + // Note that when properties declared in Javascript constructors + // (marked by isReplaceableByMethod) conflict with another symbol, the property loses. + // Always. This allows the common Javascript pattern of overwriting a prototype method + // with an bound instance method of the same type: `this.method = this.method.bind(this)` + // // If we created a new symbol, either because we didn't have a symbol with this name // in the symbol table, or we conflicted with an existing symbol, then just add this // node as the sole declaration of the new symbol. @@ -329,7 +334,7 @@ namespace ts { } if (symbol.flags & excludes) { - if (symbol.isDiscardable) { + if (symbol.isReplaceableByMethod) { // Javascript constructor-declared symbols can be discarded in favor of // prototype symbols like methods. symbol = symbolTable[name] = createSymbol(SymbolFlags.None, name); @@ -1988,7 +1993,7 @@ namespace ts { // It's acceptable for multiple 'this' assignments of the same identifier to occur // AND it can be overwritten by subsequent method declarations const symbol = declareSymbol(assignee.symbol.members, assignee.symbol, node, SymbolFlags.Property, SymbolFlags.PropertyExcludes & ~SymbolFlags.Property); - symbol.isDiscardable = true; + symbol.isReplaceableByMethod = true; } function bindPrototypePropertyAssignment(node: BinaryExpression) { diff --git a/src/compiler/types.ts b/src/compiler/types.ts index f5cee2f8f8f29..70bd3c3c07035 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -2137,7 +2137,7 @@ namespace ts { /* @internal */ exportSymbol?: Symbol; // Exported symbol associated with this symbol /* @internal */ constEnumOnlyModule?: boolean; // True if module contains only const enums or other modules with only const enums /* @internal */ isReferenced?: boolean; // True if the symbol is referenced elsewhere - /* @internal */ isDiscardable?: boolean; // True if a Javascript class property can be overwritten by a method + /* @internal */ isReplaceableByMethod?: boolean; // Can this Javascript class property be replaced by a method symbol? } /* @internal */ From 07a8f31a2d294fb7d9aca3fe937cab0bb5345e21 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders Date: Wed, 17 Aug 2016 10:43:02 -0700 Subject: [PATCH 7/8] Correctly merge bindThisPropertyAssignment Also simply it considerably after noticing that it's *only* called for Javascript files, so there was a lot of dead code for TS cases that never happened. --- src/compiler/binder.ts | 34 +++++++++++++--------------------- 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/src/compiler/binder.ts b/src/compiler/binder.ts index ee82a88d7a57a..5aaf6f5018399 100644 --- a/src/compiler/binder.ts +++ b/src/compiler/binder.ts @@ -1977,33 +1977,25 @@ namespace ts { } function bindThisPropertyAssignment(node: BinaryExpression) { + Debug.assert(isInJavaScriptFile(node)); // Declare a 'member' if the container is an ES5 class or ES6 constructor - let assignee: Node; if (container.kind === SyntaxKind.FunctionDeclaration || container.kind === SyntaxKind.FunctionExpression) { - assignee = container; + container.symbol.members = container.symbol.members || createMap(); + // 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); } else if (container.kind === SyntaxKind.Constructor) { - if (isInJavaScriptFile(node)) { - // this.foo assignment in a JavaScript class - // Bind this property to the containing class - const saveContainer = container; - container = container.parent; - bindPropertyOrMethodOrAccessor(node, SymbolFlags.Property, SymbolFlags.None); - container = saveContainer; - return; - } - else { - assignee = container.parent; + // this.foo assignment in a JavaScript class + // Bind this property to the containing class + const saveContainer = container; + container = container.parent; + // AND it can be overwritten by subsequent method declarations + const symbol = bindPropertyOrMethodOrAccessor(node, SymbolFlags.Property, SymbolFlags.None); + if (symbol) { + (symbol as Symbol).isReplaceableByMethod = true; } + container = saveContainer; } - else { - return; - } - assignee.symbol.members = assignee.symbol.members || createMap(); - // It's acceptable for multiple 'this' assignments of the same identifier to occur - // AND it can be overwritten by subsequent method declarations - const symbol = declareSymbol(assignee.symbol.members, assignee.symbol, node, SymbolFlags.Property, SymbolFlags.PropertyExcludes & ~SymbolFlags.Property); - symbol.isReplaceableByMethod = true; } function bindPrototypePropertyAssignment(node: BinaryExpression) { From c73efe2fb629bcb3a283eaf7979d7070705546cc Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders Date: Wed, 17 Aug 2016 10:45:35 -0700 Subject: [PATCH 8/8] Fix comment --- src/compiler/binder.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/compiler/binder.ts b/src/compiler/binder.ts index 5aaf6f5018399..d8017d601adb0 100644 --- a/src/compiler/binder.ts +++ b/src/compiler/binder.ts @@ -1989,9 +1989,9 @@ namespace ts { // Bind this property to the containing class const saveContainer = container; container = container.parent; - // AND it can be overwritten by subsequent method declarations const symbol = bindPropertyOrMethodOrAccessor(node, SymbolFlags.Property, SymbolFlags.None); if (symbol) { + // constructor-declared symbols can be overwritten by subsequent method declarations (symbol as Symbol).isReplaceableByMethod = true; } container = saveContainer;