Fix cross-file merge of assignment decl valueDeclaration (#26918)

* 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. * Update baselines * Add a fix for #27099 Makes commonjs merge with globals when appropriate. * Add a separate jsGlobalAugmentations table Instead of trying to filter these augmentations out of the normal symbol table of commonjs modules.
microsoft · Sep 16, 2018 · 394ee31 · 394ee31
1 parent 70ce7ab
commit 394ee31
Show file tree

Hide file tree

Showing 11 changed files with 203 additions and 6 deletions.
diff --git 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
@@ -850,7 +850,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;
@@ -28611,6 +28611,9 @@ namespace ts {
                 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
@@ -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<string>; // Map from a string to an interned string
         /* @internal */ nodeCount: number;

diff --git a/tests/baselines/reference/globalMergeWithCommonJSAssignmentDeclaration.errors.txt 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';
+
+
diff --git a/tests/baselines/reference/globalMergeWithCommonJSAssignmentDeclaration.symbols 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(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(lib.dom.d.ts, --, --), 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
@@ -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/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 : {}
 >{} : {}
 
diff --git a/tests/baselines/reference/typeFromPropertyAssignment35.symbols 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
@@ -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/globalMergeWithCommonJSAssignmentDeclaration.ts 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';
+
diff --git a/tests/cases/conformance/salsa/typeFromPropertyAssignment35.ts 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;
+