From e948e6e365748e7d49ada60c041e3297eb1933a8 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Wed, 5 Sep 2018 13:00:57 -0700 Subject: [PATCH 1/4] Fix cross-file merge of assignment decl valueDeclaration Previously mergeSymbol in the checker always updated valueDeclaration if target.valueDeclaration was an assignment declaration. The binder only updates target.valueDeclaration if it is an assignment declaration and source.valueDeclaration is *not* an assignment declaration. Now the checker behaves the same way as the binder. --- src/compiler/checker.ts | 2 +- .../typeFromPropertyAssignment35.symbols | 51 +++++++++++++++++ .../typeFromPropertyAssignment35.types | 57 +++++++++++++++++++ .../salsa/typeFromPropertyAssignment35.ts | 22 +++++++ 4 files changed, 131 insertions(+), 1 deletion(-) create mode 100644 tests/baselines/reference/typeFromPropertyAssignment35.symbols create mode 100644 tests/baselines/reference/typeFromPropertyAssignment35.types create mode 100644 tests/cases/conformance/salsa/typeFromPropertyAssignment35.ts diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index d1a442e8eccc6..311aee96f7aca 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -837,7 +837,7 @@ namespace ts { target.flags |= source.flags; if (source.valueDeclaration && (!target.valueDeclaration || - isAssignmentDeclaration(target.valueDeclaration) || + isAssignmentDeclaration(target.valueDeclaration) && !isAssignmentDeclaration(source.valueDeclaration) || isEffectiveModuleDeclaration(target.valueDeclaration) && !isEffectiveModuleDeclaration(source.valueDeclaration))) { // other kinds of value declarations take precedence over modules and assignment declarations target.valueDeclaration = source.valueDeclaration; diff --git a/tests/baselines/reference/typeFromPropertyAssignment35.symbols b/tests/baselines/reference/typeFromPropertyAssignment35.symbols new file mode 100644 index 0000000000000..d651748d1cc42 --- /dev/null +++ b/tests/baselines/reference/typeFromPropertyAssignment35.symbols @@ -0,0 +1,51 @@ +=== tests/cases/conformance/salsa/bug26877.js === +/** @param {Emu.D} x */ +function ollKorrect(x) { +>ollKorrect : Symbol(ollKorrect, Decl(bug26877.js, 0, 0)) +>x : Symbol(x, Decl(bug26877.js, 1, 20)) + + x._model +>x._model : Symbol(D._model, Decl(bug26877.js, 7, 19)) +>x : Symbol(x, Decl(bug26877.js, 1, 20)) +>_model : Symbol(D._model, Decl(bug26877.js, 7, 19)) + + const y = new Emu.D() +>y : Symbol(y, Decl(bug26877.js, 3, 9)) +>Emu.D : Symbol(Emu.D, Decl(bug26877.js, 5, 1), Decl(second.js, 2, 4)) +>Emu : Symbol(Emu, Decl(bug26877.js, 5, 1), Decl(second.js, 0, 3), Decl(second.js, 0, 12)) +>D : Symbol(Emu.D, Decl(bug26877.js, 5, 1), Decl(second.js, 2, 4)) + + const z = Emu.D._wrapperInstance; +>z : Symbol(z, Decl(bug26877.js, 4, 9)) +>Emu.D._wrapperInstance : Symbol(Emu.D._wrapperInstance, Decl(second.js, 0, 12)) +>Emu.D : Symbol(Emu.D, Decl(bug26877.js, 5, 1), Decl(second.js, 2, 4)) +>Emu : Symbol(Emu, Decl(bug26877.js, 5, 1), Decl(second.js, 0, 3), Decl(second.js, 0, 12)) +>D : Symbol(Emu.D, Decl(bug26877.js, 5, 1), Decl(second.js, 2, 4)) +>_wrapperInstance : Symbol(Emu.D._wrapperInstance, Decl(second.js, 0, 12)) +} +Emu.D = class { +>Emu.D : Symbol(Emu.D, Decl(bug26877.js, 5, 1), Decl(second.js, 2, 4)) +>Emu : Symbol(Emu, Decl(bug26877.js, 5, 1), Decl(second.js, 0, 3), Decl(second.js, 0, 12)) +>D : Symbol(Emu.D, Decl(bug26877.js, 5, 1), Decl(second.js, 2, 4)) + + constructor() { + this._model = 1 +>this._model : Symbol(D._model, Decl(bug26877.js, 7, 19)) +>this : Symbol(D, Decl(bug26877.js, 6, 7)) +>_model : Symbol(D._model, Decl(bug26877.js, 7, 19)) + } +} + +=== tests/cases/conformance/salsa/second.js === +var Emu = {} +>Emu : Symbol(Emu, Decl(bug26877.js, 5, 1), Decl(second.js, 0, 3), Decl(second.js, 0, 12)) + +/** @type {string} */ +Emu.D._wrapperInstance; +>Emu.D._wrapperInstance : Symbol(Emu.D._wrapperInstance, Decl(second.js, 0, 12)) +>Emu.D : Symbol(Emu.D, Decl(bug26877.js, 5, 1), Decl(second.js, 2, 4)) +>Emu : Symbol(Emu, Decl(bug26877.js, 5, 1), Decl(second.js, 0, 3), Decl(second.js, 0, 12)) +>D : Symbol(Emu.D, Decl(bug26877.js, 5, 1), Decl(second.js, 2, 4)) +>_wrapperInstance : Symbol(Emu.D._wrapperInstance, Decl(second.js, 0, 12)) + + diff --git a/tests/baselines/reference/typeFromPropertyAssignment35.types b/tests/baselines/reference/typeFromPropertyAssignment35.types new file mode 100644 index 0000000000000..678adb1ba5346 --- /dev/null +++ b/tests/baselines/reference/typeFromPropertyAssignment35.types @@ -0,0 +1,57 @@ +=== tests/cases/conformance/salsa/bug26877.js === +/** @param {Emu.D} x */ +function ollKorrect(x) { +>ollKorrect : (x: D) => void +>x : D + + x._model +>x._model : number +>x : D +>_model : number + + const y = new Emu.D() +>y : D +>new Emu.D() : D +>Emu.D : typeof D +>Emu : typeof Emu +>D : typeof D + + const z = Emu.D._wrapperInstance; +>z : string +>Emu.D._wrapperInstance : string +>Emu.D : typeof D +>Emu : typeof Emu +>D : typeof D +>_wrapperInstance : string +} +Emu.D = class { +>Emu.D = class { constructor() { this._model = 1 }} : typeof D +>Emu.D : typeof D +>Emu : typeof Emu +>D : typeof D +>class { constructor() { this._model = 1 }} : typeof D + + constructor() { + this._model = 1 +>this._model = 1 : 1 +>this._model : number +>this : this +>_model : number +>1 : 1 + } +} + +=== tests/cases/conformance/salsa/second.js === +var Emu = {} +>Emu : typeof Emu +>{} : {} + +/** @type {string} */ +Emu.D._wrapperInstance; +>Emu.D._wrapperInstance : string +>Emu.D : typeof D +>Emu : typeof Emu +>D : typeof D +>_wrapperInstance : string + + diff --git a/tests/cases/conformance/salsa/typeFromPropertyAssignment35.ts b/tests/cases/conformance/salsa/typeFromPropertyAssignment35.ts new file mode 100644 index 0000000000000..952fdd7c9c02d --- /dev/null +++ b/tests/cases/conformance/salsa/typeFromPropertyAssignment35.ts @@ -0,0 +1,22 @@ +// @noEmit: true +// @allowJs: true +// @checkJs: true + +// @Filename: bug26877.js +/** @param {Emu.D} x */ +function ollKorrect(x) { + x._model + const y = new Emu.D() + const z = Emu.D._wrapperInstance; +} +Emu.D = class { + constructor() { + this._model = 1 + } +} + +// @Filename: second.js +var Emu = {} +/** @type {string} */ +Emu.D._wrapperInstance; + From a669d9720f2c42c6d1ff9641766ffb408caa6b6b Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Fri, 14 Sep 2018 10:45:30 -0700 Subject: [PATCH 2/4] Update baselines --- .../baselines/reference/jsContainerMergeJsContainer.types | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/baselines/reference/jsContainerMergeJsContainer.types b/tests/baselines/reference/jsContainerMergeJsContainer.types index 75d2152c57190..73e61aded0df9 100644 --- a/tests/baselines/reference/jsContainerMergeJsContainer.types +++ b/tests/baselines/reference/jsContainerMergeJsContainer.types @@ -6,18 +6,18 @@ const a = {}; a.d = function() {}; >a.d = function() {} : { (): void; prototype: {}; } ->a.d : typeof a.d +>a.d : { (): void; prototype: {}; } >a : typeof a ->d : typeof a.d +>d : { (): void; prototype: {}; } >function() {} : { (): void; prototype: {}; } === tests/cases/conformance/salsa/b.js === a.d.prototype = {}; >a.d.prototype = {} : {} >a.d.prototype : {} ->a.d : typeof a.d +>a.d : { (): void; prototype: {}; } >a : typeof a ->d : typeof a.d +>d : { (): void; prototype: {}; } >prototype : {} >{} : {} From cb9fefe761d29b3037874eba755f47e428c50420 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Fri, 14 Sep 2018 16:38:00 -0700 Subject: [PATCH 3/4] Add a fix for #27099 Makes commonjs merge with globals when appropriate. --- src/compiler/checker.ts | 17 +++++++++++++-- ...thCommonJSAssignmentDeclaration.errors.txt | 11 ++++++++++ ...eWithCommonJSAssignmentDeclaration.symbols | 17 +++++++++++++++ ...rgeWithCommonJSAssignmentDeclaration.types | 21 +++++++++++++++++++ ...lMergeWithCommonJSAssignmentDeclaration.ts | 8 +++++++ 5 files changed, 72 insertions(+), 2 deletions(-) create mode 100644 tests/baselines/reference/globalMergeWithCommonJSAssignmentDeclaration.errors.txt create mode 100644 tests/baselines/reference/globalMergeWithCommonJSAssignmentDeclaration.symbols create mode 100644 tests/baselines/reference/globalMergeWithCommonJSAssignmentDeclaration.types create mode 100644 tests/cases/conformance/salsa/globalMergeWithCommonJSAssignmentDeclaration.ts diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index c8ead8d75dfb2..fe824de75ad49 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -923,6 +923,14 @@ namespace ts { return combined; } + /** CommonJS is *almost* a module -- no merges -- except that assignment declarations need to merge with globals. */ + function mergeCommonJsSymbolTable(target: SymbolTable, source: SymbolTable) { + source.forEach((sourceSymbol, id) => { + if (target.has(id) && sourceSymbol.flags & SymbolFlags.Assignment) { + target.set(id, mergeSymbol(target.get(id)!, sourceSymbol)); + } + }); + } function mergeSymbolTable(target: SymbolTable, source: SymbolTable) { source.forEach((sourceSymbol, id) => { target.set(id, target.has(id) ? mergeSymbol(target.get(id)!, sourceSymbol) : sourceSymbol); @@ -28588,8 +28596,13 @@ namespace ts { if (file.redirectInfo) { continue; } - if (!isExternalOrCommonJsModule(file)) { - mergeSymbolTable(globals, file.locals!); + if (!isExternalModule(file)) { + if(file.commonJsModuleIndicator) { + mergeCommonJsSymbolTable(globals, file.locals!); + } + else { + mergeSymbolTable(globals, file.locals!); + } } if (file.patternAmbientModules && file.patternAmbientModules.length) { patternAmbientModules = concatenate(patternAmbientModules, file.patternAmbientModules); diff --git a/tests/baselines/reference/globalMergeWithCommonJSAssignmentDeclaration.errors.txt b/tests/baselines/reference/globalMergeWithCommonJSAssignmentDeclaration.errors.txt new file mode 100644 index 0000000000000..bc67e012a8618 --- /dev/null +++ b/tests/baselines/reference/globalMergeWithCommonJSAssignmentDeclaration.errors.txt @@ -0,0 +1,11 @@ +tests/cases/conformance/salsa/bug27099.js(1,1): error TS2322: Type '1' is not assignable to type 'string'. + + +==== tests/cases/conformance/salsa/bug27099.js (1 errors) ==== + window.name = 1; + ~~~~~~~~~~~ +!!! error TS2322: Type '1' is not assignable to type 'string'. + window.console; // should not have error: Property 'console' does not exist on type 'typeof window'. + module.exports = 'anything'; + + \ No newline at end of file diff --git a/tests/baselines/reference/globalMergeWithCommonJSAssignmentDeclaration.symbols b/tests/baselines/reference/globalMergeWithCommonJSAssignmentDeclaration.symbols new file mode 100644 index 0000000000000..078c790e02be4 --- /dev/null +++ b/tests/baselines/reference/globalMergeWithCommonJSAssignmentDeclaration.symbols @@ -0,0 +1,17 @@ +=== tests/cases/conformance/salsa/bug27099.js === +window.name = 1; +>window.name : Symbol(Window.name, Decl(lib.dom.d.ts, --, --)) +>window : Symbol(window, Decl(bug27099.js, 0, 0)) +>name : Symbol(Window.name, Decl(lib.dom.d.ts, --, --)) + +window.console; // should not have error: Property 'console' does not exist on type 'typeof window'. +>window.console : Symbol(WindowConsole.console, Decl(lib.dom.d.ts, --, --)) +>window : Symbol(window, Decl(bug27099.js, 0, 0)) +>console : Symbol(WindowConsole.console, Decl(lib.dom.d.ts, --, --)) + +module.exports = 'anything'; +>module.exports : Symbol("tests/cases/conformance/salsa/bug27099", Decl(bug27099.js, 0, 0)) +>module : Symbol(export=, Decl(bug27099.js, 1, 15)) +>exports : Symbol(export=, Decl(bug27099.js, 1, 15)) + + diff --git a/tests/baselines/reference/globalMergeWithCommonJSAssignmentDeclaration.types b/tests/baselines/reference/globalMergeWithCommonJSAssignmentDeclaration.types new file mode 100644 index 0000000000000..485c90f6100cf --- /dev/null +++ b/tests/baselines/reference/globalMergeWithCommonJSAssignmentDeclaration.types @@ -0,0 +1,21 @@ +=== tests/cases/conformance/salsa/bug27099.js === +window.name = 1; +>window.name = 1 : 1 +>window.name : string +>window : Window +>name : string +>1 : 1 + +window.console; // should not have error: Property 'console' does not exist on type 'typeof window'. +>window.console : Console +>window : Window +>console : Console + +module.exports = 'anything'; +>module.exports = 'anything' : string +>module.exports : string +>module : { "tests/cases/conformance/salsa/bug27099": string; } +>exports : string +>'anything' : "anything" + + diff --git a/tests/cases/conformance/salsa/globalMergeWithCommonJSAssignmentDeclaration.ts b/tests/cases/conformance/salsa/globalMergeWithCommonJSAssignmentDeclaration.ts new file mode 100644 index 0000000000000..f1435eea87e04 --- /dev/null +++ b/tests/cases/conformance/salsa/globalMergeWithCommonJSAssignmentDeclaration.ts @@ -0,0 +1,8 @@ +// @noEmit: true +// @allowJs: true +// @checkJs: true +// @Filename: bug27099.js +window.name = 1; +window.console; // should not have error: Property 'console' does not exist on type 'typeof window'. +module.exports = 'anything'; + From a6d77fa7b8ce8916299b1c0ee22a0d740cfef238 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Sat, 15 Sep 2018 09:34:56 -0700 Subject: [PATCH 4/4] Add a separate jsGlobalAugmentations table Instead of trying to filter these augmentations out of the normal symbol table of commonjs modules. --- src/compiler/binder.ts | 7 ++++++- src/compiler/checker.ts | 20 +++++-------------- src/compiler/types.ts | 2 ++ ...eWithCommonJSAssignmentDeclaration.symbols | 4 ++-- 4 files changed, 15 insertions(+), 18 deletions(-) diff --git a/src/compiler/binder.ts b/src/compiler/binder.ts index 81d4857ad073a..af3cf3b022dca 100644 --- a/src/compiler/binder.ts +++ b/src/compiler/binder.ts @@ -2532,7 +2532,9 @@ namespace ts { return symbol; } else { - return declareSymbol(parent ? parent.exports! : container.locals!, parent, id, flags, excludeFlags); + const table = parent ? parent.exports! : + file.jsGlobalAugmentations || (file.jsGlobalAugmentations = createSymbolTable()); + return declareSymbol(table, parent, id, flags, excludeFlags); } }); } @@ -2901,6 +2903,9 @@ namespace ts { if (local) { return local.exportSymbol || local; } + if (isSourceFile(container) && container.jsGlobalAugmentations && container.jsGlobalAugmentations.has(name)) { + return container.jsGlobalAugmentations.get(name); + } return container.symbol && container.symbol.exports && container.symbol.exports.get(name); } diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index fe824de75ad49..16227e28beb80 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -923,14 +923,6 @@ namespace ts { return combined; } - /** CommonJS is *almost* a module -- no merges -- except that assignment declarations need to merge with globals. */ - function mergeCommonJsSymbolTable(target: SymbolTable, source: SymbolTable) { - source.forEach((sourceSymbol, id) => { - if (target.has(id) && sourceSymbol.flags & SymbolFlags.Assignment) { - target.set(id, mergeSymbol(target.get(id)!, sourceSymbol)); - } - }); - } function mergeSymbolTable(target: SymbolTable, source: SymbolTable) { source.forEach((sourceSymbol, id) => { target.set(id, target.has(id) ? mergeSymbol(target.get(id)!, sourceSymbol) : sourceSymbol); @@ -28596,13 +28588,11 @@ namespace ts { if (file.redirectInfo) { continue; } - if (!isExternalModule(file)) { - if(file.commonJsModuleIndicator) { - mergeCommonJsSymbolTable(globals, file.locals!); - } - else { - mergeSymbolTable(globals, file.locals!); - } + if (!isExternalOrCommonJsModule(file)) { + mergeSymbolTable(globals, file.locals!); + } + if (file.jsGlobalAugmentations) { + mergeSymbolTable(globals, file.jsGlobalAugmentations); } if (file.patternAmbientModules && file.patternAmbientModules.length) { patternAmbientModules = concatenate(patternAmbientModules, file.patternAmbientModules); diff --git a/src/compiler/types.ts b/src/compiler/types.ts index b7e4ac12056c3..12d478b89baaa 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -2593,6 +2593,8 @@ namespace ts { /* @internal */ externalModuleIndicator?: Node; // The first node that causes this file to be a CommonJS module /* @internal */ commonJsModuleIndicator?: Node; + // JS identifier-declarations that are intended to merge with globals + /* @internal */ jsGlobalAugmentations?: SymbolTable; /* @internal */ identifiers: Map; // Map from a string to an interned string /* @internal */ nodeCount: number; diff --git a/tests/baselines/reference/globalMergeWithCommonJSAssignmentDeclaration.symbols b/tests/baselines/reference/globalMergeWithCommonJSAssignmentDeclaration.symbols index 078c790e02be4..dd9b8a1bab9cc 100644 --- a/tests/baselines/reference/globalMergeWithCommonJSAssignmentDeclaration.symbols +++ b/tests/baselines/reference/globalMergeWithCommonJSAssignmentDeclaration.symbols @@ -1,12 +1,12 @@ === tests/cases/conformance/salsa/bug27099.js === window.name = 1; >window.name : Symbol(Window.name, Decl(lib.dom.d.ts, --, --)) ->window : Symbol(window, Decl(bug27099.js, 0, 0)) +>window : Symbol(window, Decl(lib.dom.d.ts, --, --), Decl(bug27099.js, 0, 0)) >name : Symbol(Window.name, Decl(lib.dom.d.ts, --, --)) window.console; // should not have error: Property 'console' does not exist on type 'typeof window'. >window.console : Symbol(WindowConsole.console, Decl(lib.dom.d.ts, --, --)) ->window : Symbol(window, Decl(bug27099.js, 0, 0)) +>window : Symbol(window, Decl(lib.dom.d.ts, --, --), Decl(bug27099.js, 0, 0)) >console : Symbol(WindowConsole.console, Decl(lib.dom.d.ts, --, --)) module.exports = 'anything';