Fix values and types merging in JS module exports (microsoft#37896)

andrewbranch · web-flow · commit ce95d9ca6be1 · 2020-04-24T13:49:48.000-07:00
* Fix values and types merging in JS module exports

* Fix everything

* Share `setValueDeclaration` between binder (local merge) and checker (cross-file merge)

* Revert accidental changes to baselines

* Update baseline from master merge
diff --git a/src/compiler/binder.ts b/src/compiler/binder.ts
@@ -320,16 +320,6 @@ namespace ts {
             }
         }
 
-        function setValueDeclaration(symbol: Symbol, node: Declaration): void {
-            const { valueDeclaration } = symbol;
-            if (!valueDeclaration ||
-                (isAssignmentDeclaration(valueDeclaration) && !isAssignmentDeclaration(node)) ||
-                (valueDeclaration.kind !== node.kind && isEffectiveModuleDeclaration(valueDeclaration))) {
-                // other kinds of value declarations take precedence over modules and assignment declarations
-                symbol.valueDeclaration = node;
-            }
-        }
-
         // Should not be called on a declaration with a computed property name,
         // unless it is a well known Symbol.
         function getDeclarationName(node: Declaration): __String | undefined {
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
@@ -1115,12 +1115,8 @@ namespace ts {
                     target.constEnumOnlyModule = false;
                 }
                 target.flags |= source.flags;
-                if (source.valueDeclaration &&
-                    (!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;
+                if (source.valueDeclaration) {
+                    setValueDeclaration(target, source.valueDeclaration);
                 }
                 addRange(target.declarations, source.declarations);
                 if (source.members) {
@@ -7724,11 +7720,38 @@ namespace ts {
                     resolvedSymbol.exports = createSymbolTable();
                 }
                 (resolvedSymbol || symbol).exports!.forEach((s, name) => {
-                    if (members.has(name)) {
-                        const exportedMember = exportedType.members.get(name)!;
-                        const union = createSymbol(s.flags | exportedMember.flags, name);
-                        union.type = getUnionType([getTypeOfSymbol(s), getTypeOfSymbol(exportedMember)]);
-                        members.set(name, union);
+                    const exportedMember = members.get(name)!;
+                    if (exportedMember && exportedMember !== s) {
+                        if (s.flags & SymbolFlags.Value) {
+                            // If the member has an additional value-like declaration, union the types from the two declarations,
+                            // but issue an error if they occurred in two different files. The purpose is to support a JS file with
+                            // a pattern like:
+                            //
+                            // module.exports = { a: true };
+                            // module.exports.a = 3;
+                            //
+                            // but we may have a JS file with `module.exports = { a: true }` along with a TypeScript module augmentation
+                            // declaring an `export const a: number`. In that case, we issue a duplicate identifier error, because
+                            // it's unclear what that's supposed to mean, so it's probably a mistake.
+                            if (getSourceFileOfNode(s.valueDeclaration) !== getSourceFileOfNode(exportedMember.valueDeclaration)) {
+                                const unescapedName = unescapeLeadingUnderscores(s.escapedName);
+                                const exportedMemberName = tryCast(exportedMember.valueDeclaration, isNamedDeclaration)?.name || exportedMember.valueDeclaration;
+                                addRelatedInfo(
+                                    error(s.valueDeclaration, Diagnostics.Duplicate_identifier_0, unescapedName),
+                                    createDiagnosticForNode(exportedMemberName, Diagnostics._0_was_also_declared_here, unescapedName));
+                                addRelatedInfo(
+                                    error(exportedMemberName, Diagnostics.Duplicate_identifier_0, unescapedName),
+                                    createDiagnosticForNode(s.valueDeclaration, Diagnostics._0_was_also_declared_here, unescapedName));
+                            }
+                            const union = createSymbol(s.flags | exportedMember.flags, name);
+                            union.type = getUnionType([getTypeOfSymbol(s), getTypeOfSymbol(exportedMember)]);
+                            union.valueDeclaration = exportedMember.valueDeclaration;
+                            union.declarations = concatenate(exportedMember.declarations, s.declarations);
+                            members.set(name, union);
+                        }
+                        else {
+                            members.set(name, mergeSymbol(s, exportedMember));
+                        }
                     }
                     else {
                         members.set(name, s);
diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts
@@ -2314,6 +2314,17 @@ namespace ts {
             !!getJSDocTypeTag(expr.parent);
     }
 
+    export function setValueDeclaration(symbol: Symbol, node: Declaration): void {
+        const { valueDeclaration } = symbol;
+        if (!valueDeclaration ||
+            !(node.flags & NodeFlags.Ambient && !(valueDeclaration.flags & NodeFlags.Ambient)) &&
+            (isAssignmentDeclaration(valueDeclaration) && !isAssignmentDeclaration(node)) ||
+            (valueDeclaration.kind !== node.kind && isEffectiveModuleDeclaration(valueDeclaration))) {
+            // other kinds of value declarations take precedence over modules and assignment declarations
+            symbol.valueDeclaration = node;
+        }
+    }
+
     export function isFunctionSymbol(symbol: Symbol | undefined) {
         if (!symbol || !symbol.valueDeclaration) {
             return false;
diff --git a/tests/baselines/reference/jsDeclarationsClassExtendsVisibility.symbols b/tests/baselines/reference/jsDeclarationsClassExtendsVisibility.symbols
@@ -25,7 +25,7 @@ module.exports = Foo;
 >Foo : Symbol(Foo, Decl(cls.js, 4, 2))
 
 module.exports.Strings = Strings;
->module.exports.Strings : Symbol(Strings)
+>module.exports.Strings : Symbol(Strings, Decl(cls.js, 6, 21))
 >module.exports : Symbol(Strings, Decl(cls.js, 6, 21))
 >module : Symbol(module, Decl(cls.js, 5, 24))
 >exports : Symbol("tests/cases/conformance/jsdoc/declarations/cls", Decl(cls.js, 0, 0))
diff --git a/tests/baselines/reference/jsDeclarationsClassStatic.symbols b/tests/baselines/reference/jsDeclarationsClassStatic.symbols
@@ -34,7 +34,7 @@ module.exports = Handler;
 >Handler : Symbol(Handler, Decl(source.js, 0, 0), Decl(source.js, 7, 1))
 
 module.exports.Strings = Strings
->module.exports.Strings : Symbol(Strings)
+>module.exports.Strings : Symbol(Strings, Decl(source.js, 14, 25))
 >module.exports : Symbol(Strings, Decl(source.js, 14, 25))
 >module : Symbol(module, Decl(source.js, 12, 1))
 >exports : Symbol("tests/cases/conformance/jsdoc/declarations/source", Decl(source.js, 0, 0))
diff --git a/tests/baselines/reference/jsDeclarationsCrossfileMerge.symbols b/tests/baselines/reference/jsDeclarationsCrossfileMerge.symbols
@@ -13,7 +13,7 @@ module.exports = m.default;
 >default : Symbol(default, Decl(exporter.js, 0, 22))
 
 module.exports.memberName = "thing";
->module.exports.memberName : Symbol(memberName)
+>module.exports.memberName : Symbol(memberName, Decl(index.js, 2, 27))
 >module.exports : Symbol(memberName, Decl(index.js, 2, 27))
 >module : Symbol(module, Decl(index.js, 0, 32))
 >exports : Symbol("tests/cases/conformance/jsdoc/declarations/index", Decl(index.js, 0, 0))
diff --git a/tests/baselines/reference/jsDeclarationsExportAssignedClassExpressionAnonymousWithSub.symbols b/tests/baselines/reference/jsDeclarationsExportAssignedClassExpressionAnonymousWithSub.symbols
@@ -18,7 +18,7 @@ module.exports = class {
     }
 }
 module.exports.Sub = class {
->module.exports.Sub : Symbol(Sub)
+>module.exports.Sub : Symbol(Sub, Decl(index.js, 7, 1))
 >module.exports : Symbol(Sub, Decl(index.js, 7, 1))
 >module : Symbol(module, Decl(index.js, 0, 0), Decl(index.js, 10, 27))
 >exports : Symbol("tests/cases/conformance/jsdoc/declarations/index", Decl(index.js, 0, 0))
diff --git a/tests/baselines/reference/jsDeclarationsExportAssignedClassExpressionShadowing.symbols b/tests/baselines/reference/jsDeclarationsExportAssignedClassExpressionShadowing.symbols
@@ -27,7 +27,7 @@ module.exports = class Q {
     }
 }
 module.exports.Another = Q;
->module.exports.Another : Symbol(Another)
+>module.exports.Another : Symbol(Another, Decl(index.js, 10, 1))
 >module.exports : Symbol(Another, Decl(index.js, 10, 1))
 >module : Symbol(module, Decl(index.js, 5, 1))
 >exports : Symbol("tests/cases/conformance/jsdoc/declarations/index", Decl(index.js, 0, 0))
diff --git a/tests/baselines/reference/jsDeclarationsExportSubAssignments.symbols b/tests/baselines/reference/jsDeclarationsExportSubAssignments.symbols
@@ -19,7 +19,7 @@ module.exports = Foo;
 >Foo : Symbol(Foo, Decl(cls.js, 3, 2))
 
 module.exports.Strings = Strings;
->module.exports.Strings : Symbol(Strings)
+>module.exports.Strings : Symbol(Strings, Decl(cls.js, 5, 21))
 >module.exports : Symbol(Strings, Decl(cls.js, 5, 21))
 >module : Symbol(module, Decl(cls.js, 4, 12))
 >exports : Symbol("tests/cases/conformance/jsdoc/declarations/cls", Decl(cls.js, 0, 0))
diff --git a/tests/baselines/reference/jsExportMemberMergedWithModuleAugmentation.symbols b/tests/baselines/reference/jsExportMemberMergedWithModuleAugmentation.symbols
@@ -0,0 +1,43 @@
+=== /test.js ===
+class Abcde {
+>Abcde : Symbol(Abcde, Decl(test.js, 0, 0))
+
+  /** @type {string} */
+  x;
+>x : Symbol(Abcde.x, Decl(test.js, 0, 13))
+}
+
+module.exports = {
+>module.exports : Symbol("/test", Decl(test.js, 0, 0))
+>module : Symbol("/test.js", Decl(test.js, 3, 1), Decl(index.ts, 0, 31))
+>exports : Symbol("/test.js", Decl(test.js, 3, 1), Decl(index.ts, 0, 31))
+
+  Abcde
+>Abcde : Symbol(Abcde, Decl(test.js, 5, 18))
+
+};
+
+=== /index.ts ===
+import { Abcde } from "./test";
+>Abcde : Symbol(Abcde, Decl(index.ts, 0, 8))
+
+declare module "./test" {
+>"./test" : Symbol("/test.js", Decl(test.js, 3, 1), Decl(index.ts, 0, 31))
+
+  interface Abcde { b: string }
+>Abcde : Symbol(Abcde, Decl(index.ts, 2, 25), Decl(test.js, 5, 18))
+>b : Symbol(Abcde.b, Decl(index.ts, 3, 19))
+}
+
+new Abcde().x;
+>new Abcde().x : Symbol(Abcde.x, Decl(test.js, 0, 13))
+>Abcde : Symbol(Abcde, Decl(index.ts, 0, 8))
+>x : Symbol(Abcde.x, Decl(test.js, 0, 13))
+
+// Bug: the type meaning from /test.js does not
+// propagate through the object literal export.
+const x: Abcde = { b: "" };
+>x : Symbol(x, Decl(index.ts, 10, 5))
+>Abcde : Symbol(Abcde, Decl(index.ts, 0, 8))
+>b : Symbol(b, Decl(index.ts, 10, 18))
+
diff --git a/tests/baselines/reference/jsExportMemberMergedWithModuleAugmentation.types b/tests/baselines/reference/jsExportMemberMergedWithModuleAugmentation.types
@@ -0,0 +1,46 @@
+=== /test.js ===
+class Abcde {
+>Abcde : Abcde
+
+  /** @type {string} */
+  x;
+>x : string
+}
+
+module.exports = {
+>module.exports = {  Abcde} : { Abcde: typeof Abcde; }
+>module.exports : { Abcde: typeof Abcde; }
+>module : { "\"/test\"": { Abcde: typeof Abcde; }; }
+>exports : { Abcde: typeof Abcde; }
+>{  Abcde} : { Abcde: typeof Abcde; }
+
+  Abcde
+>Abcde : typeof Abcde
+
+};
+
+=== /index.ts ===
+import { Abcde } from "./test";
+>Abcde : typeof Abcde
+
+declare module "./test" {
+>"./test" : { Abcde: typeof Abcde; }
+
+  interface Abcde { b: string }
+>b : string
+}
+
+new Abcde().x;
+>new Abcde().x : string
+>new Abcde() : Abcde
+>Abcde : typeof Abcde
+>x : string
+
+// Bug: the type meaning from /test.js does not
+// propagate through the object literal export.
+const x: Abcde = { b: "" };
+>x : Abcde
+>{ b: "" } : { b: string; }
+>b : string
+>"" : ""
+
diff --git a/tests/baselines/reference/jsExportMemberMergedWithModuleAugmentation2.errors.txt b/tests/baselines/reference/jsExportMemberMergedWithModuleAugmentation2.errors.txt
@@ -0,0 +1,27 @@
+/index.ts(4,16): error TS2300: Duplicate identifier 'a'.
+/index.ts(7,3): error TS2339: Property 'toFixed' does not exist on type 'string'.
+/test.js(2,3): error TS2300: Duplicate identifier 'a'.
+
+
+==== /test.js (1 errors) ====
+    module.exports = {
+      a: "ok"
+      ~
+!!! error TS2300: Duplicate identifier 'a'.
+!!! related TS6203 /index.ts:4:16: 'a' was also declared here.
+    };
+    
+==== /index.ts (2 errors) ====
+    import { a } from "./test";
+    
+    declare module "./test" {
+      export const a: number;
+                   ~
+!!! error TS2300: Duplicate identifier 'a'.
+!!! related TS6203 /test.js:2:3: 'a' was also declared here.
+    }
+    
+    a.toFixed();
+      ~~~~~~~
+!!! error TS2339: Property 'toFixed' does not exist on type 'string'.
+    
diff --git a/tests/baselines/reference/jsExportMemberMergedWithModuleAugmentation2.symbols b/tests/baselines/reference/jsExportMemberMergedWithModuleAugmentation2.symbols
@@ -0,0 +1,25 @@
+=== /test.js ===
+module.exports = {
+>module.exports : Symbol("/test", Decl(test.js, 0, 0))
+>module : Symbol("/test.js", Decl(test.js, 0, 0), Decl(index.ts, 0, 27))
+>exports : Symbol("/test.js", Decl(test.js, 0, 0), Decl(index.ts, 0, 27))
+
+  a: "ok"
+>a : Symbol(a, Decl(test.js, 0, 18))
+
+};
+
+=== /index.ts ===
+import { a } from "./test";
+>a : Symbol(a, Decl(index.ts, 0, 8))
+
+declare module "./test" {
+>"./test" : Symbol("/test.js", Decl(test.js, 0, 0), Decl(index.ts, 0, 27))
+
+  export const a: number;
+>a : Symbol(a, Decl(index.ts, 3, 14))
+}
+
+a.toFixed();
+>a : Symbol(a, Decl(index.ts, 0, 8))
+
diff --git a/tests/baselines/reference/jsExportMemberMergedWithModuleAugmentation2.types b/tests/baselines/reference/jsExportMemberMergedWithModuleAugmentation2.types
@@ -0,0 +1,31 @@
+=== /test.js ===
+module.exports = {
+>module.exports = {  a: "ok"} : { a: string | number; }
+>module.exports : { a: string | number; }
+>module : { "\"/test\"": { a: string | number; }; }
+>exports : { a: string | number; }
+>{  a: "ok"} : { a: string; }
+
+  a: "ok"
+>a : string
+>"ok" : "ok"
+
+};
+
+=== /index.ts ===
+import { a } from "./test";
+>a : string
+
+declare module "./test" {
+>"./test" : { a: string | number; }
+
+  export const a: number;
+>a : number
+}
+
+a.toFixed();
+>a.toFixed() : any
+>a.toFixed : any
+>a : string
+>toFixed : any
+
diff --git a/tests/baselines/reference/moduleExportAlias2.symbols b/tests/baselines/reference/moduleExportAlias2.symbols
@@ -37,7 +37,7 @@ exports = module.exports = C
 >C : Symbol(C, Decl(semver.js, 2, 22))
 
 exports.f = n => n + 1
->exports.f : Symbol(f)
+>exports.f : Symbol(f, Decl(semver.js, 1, 28))
 >exports : Symbol(f, Decl(semver.js, 1, 28))
 >f : Symbol(f, Decl(semver.js, 1, 28))
 >n : Symbol(n, Decl(semver.js, 2, 11))
diff --git a/tests/baselines/reference/moduleExportAlias4.symbols b/tests/baselines/reference/moduleExportAlias4.symbols
@@ -12,7 +12,7 @@ module.exports = class C {}
 >C : Symbol(C, Decl(bug24024.js, 2, 16))
 
 module.exports.D = class D { }
->module.exports.D : Symbol(D)
+>module.exports.D : Symbol(D, Decl(bug24024.js, 2, 27))
 >module.exports : Symbol(D, Decl(bug24024.js, 2, 27))
 >module : Symbol(module, Decl(bug24024.js, 1, 31))
 >exports : Symbol("tests/cases/conformance/salsa/bug24024", Decl(bug24024.js, 0, 0))
diff --git a/tests/baselines/reference/moduleExportAliasImported.symbols b/tests/baselines/reference/moduleExportAliasImported.symbols
@@ -1,6 +1,6 @@
 === tests/cases/conformance/salsa/bug28014.js ===
 exports.version = 1
->exports.version : Symbol(version)
+>exports.version : Symbol(version, Decl(bug28014.js, 0, 0))
 >exports : Symbol(version, Decl(bug28014.js, 0, 0))
 >version : Symbol(version, Decl(bug28014.js, 0, 0))
 
diff --git a/tests/baselines/reference/moduleExportPropertyAssignmentDefault.symbols b/tests/baselines/reference/moduleExportPropertyAssignmentDefault.symbols
@@ -9,7 +9,7 @@ module.exports = axios // both assignments should be ok
 >axios : Symbol(axios, Decl(axios.js, 0, 3))
 
 module.exports.default = axios
->module.exports.default : Symbol(default)
+>module.exports.default : Symbol(default, Decl(axios.js, 1, 22))
 >module.exports : Symbol(default, Decl(axios.js, 1, 22))
 >module : Symbol(module, Decl(axios.js, 0, 14))
 >exports : Symbol("tests/cases/conformance/salsa/axios", Decl(axios.js, 0, 0))
diff --git a/tests/baselines/reference/moduleExportWithExportPropertyAssignment3.symbols b/tests/baselines/reference/moduleExportWithExportPropertyAssignment3.symbols
@@ -13,14 +13,14 @@ mod1.justExport.toFixed()
 >toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))
 
 mod1.bothBefore.toFixed() // error, 'toFixed' not on 'string | number'
->mod1.bothBefore : Symbol(bothBefore)
+>mod1.bothBefore : Symbol(bothBefore, Decl(mod1.js, 3, 18), Decl(mod1.js, 0, 0))
 >mod1 : Symbol(mod1, Decl(a.js, 1, 3))
->bothBefore : Symbol(bothBefore)
+>bothBefore : Symbol(bothBefore, Decl(mod1.js, 3, 18), Decl(mod1.js, 0, 0))
 
 mod1.bothAfter.toFixed() // error, 'toFixed' not on 'string | number'
->mod1.bothAfter : Symbol(bothAfter)
+>mod1.bothAfter : Symbol(bothAfter, Decl(mod1.js, 4, 18), Decl(mod1.js, 6, 1))
 >mod1 : Symbol(mod1, Decl(a.js, 1, 3))
->bothAfter : Symbol(bothAfter)
+>bothAfter : Symbol(bothAfter, Decl(mod1.js, 4, 18), Decl(mod1.js, 6, 1))
 
 mod1.justProperty.length
 >mod1.justProperty.length : Symbol(String.length, Decl(lib.es5.d.ts, --, --))
@@ -41,7 +41,7 @@ declare function require(name: string): any;
 === tests/cases/conformance/salsa/mod1.js ===
 /// <reference path='./requires.d.ts' />
 module.exports.bothBefore = 'string'
->module.exports.bothBefore : Symbol(bothBefore)
+>module.exports.bothBefore : Symbol(bothBefore, Decl(mod1.js, 3, 18), Decl(mod1.js, 0, 0))
 >module.exports : Symbol(bothBefore, Decl(mod1.js, 0, 0))
 >module : Symbol(module, Decl(mod1.js, 0, 0))
 >exports : Symbol("tests/cases/conformance/salsa/mod1", Decl(mod1.js, 0, 0))
@@ -62,7 +62,7 @@ module.exports = {
 >bothAfter : Symbol(bothAfter, Decl(mod1.js, 4, 18))
 }
 module.exports.bothAfter = 'string'
->module.exports.bothAfter : Symbol(bothAfter)
+>module.exports.bothAfter : Symbol(bothAfter, Decl(mod1.js, 4, 18), Decl(mod1.js, 6, 1))
 >module.exports : Symbol(bothAfter, Decl(mod1.js, 6, 1))
 >module : Symbol(module, Decl(mod1.js, 0, 0))
 >exports : Symbol("tests/cases/conformance/salsa/mod1", Decl(mod1.js, 0, 0))
diff --git a/tests/baselines/reference/moduleExportWithExportPropertyAssignment4.symbols b/tests/baselines/reference/moduleExportWithExportPropertyAssignment4.symbols
diff --git a/tests/baselines/reference/typedefCrossModule2.types b/tests/baselines/reference/typedefCrossModule2.types
diff --git a/tests/baselines/reference/typedefCrossModule4.types b/tests/baselines/reference/typedefCrossModule4.types
diff --git a/tests/cases/compiler/jsExportMemberMergedWithModuleAugmentation.ts b/tests/cases/compiler/jsExportMemberMergedWithModuleAugmentation.ts
diff --git a/tests/cases/compiler/jsExportMemberMergedWithModuleAugmentation2.ts b/tests/cases/compiler/jsExportMemberMergedWithModuleAugmentation2.ts
diff --git a/tests/cases/fourslash/completionsImportModuleAugmentationWithJS.ts b/tests/cases/fourslash/completionsImportModuleAugmentationWithJS.ts

Original file line number	Diff line number	Diff line change
`@@ -18,7 +18,7 @@ module.exports = class {`
`18`	`18`	`}`
`19`	`19`	`}`
`20`	`20`	`module.exports.Sub = class {`
`21`		`->module.exports.Sub : Symbol(Sub)`
	`21`	`+>module.exports.Sub : Symbol(Sub, Decl(index.js, 7, 1))`
`22`	`22`	`>module.exports : Symbol(Sub, Decl(index.js, 7, 1))`
`23`	`23`	`>module : Symbol(module, Decl(index.js, 0, 0), Decl(index.js, 10, 27))`
`24`	`24`	`>exports : Symbol("tests/cases/conformance/jsdoc/declarations/index", Decl(index.js, 0, 0))`
Original file line number	Diff line number	Diff line change
`@@ -27,7 +27,7 @@ module.exports = class Q {`
`27`	`27`	`}`
`28`	`28`	`}`
`29`	`29`	`module.exports.Another = Q;`
`30`		`->module.exports.Another : Symbol(Another)`
	`30`	`+>module.exports.Another : Symbol(Another, Decl(index.js, 10, 1))`
`31`	`31`	`>module.exports : Symbol(Another, Decl(index.js, 10, 1))`
`32`	`32`	`>module : Symbol(module, Decl(index.js, 5, 1))`
`33`	`33`	`>exports : Symbol("tests/cases/conformance/jsdoc/declarations/index", Decl(index.js, 0, 0))`