In JS, fix crash with in a file with conflicting ES2015/commonjs exports (microsoft#24960)

sandersn · web-flow · commit a56b272d3891 · 2018-06-14T11:18:23.000-07:00
* fix crash with conflicting ES2015/commonjs modules

* Refactor based on PR comments
diff --git a/src/compiler/binder.ts b/src/compiler/binder.ts
@@ -2305,18 +2305,22 @@ namespace ts {
         }
 
         function setCommonJsModuleIndicator(node: Node) {
+            if (file.externalModuleIndicator) {
+                return false;
+            }
             if (!file.commonJsModuleIndicator) {
                 file.commonJsModuleIndicator = node;
-                if (!file.externalModuleIndicator) {
-                    bindSourceFileAsExternalModule();
-                }
+                bindSourceFileAsExternalModule();
             }
+            return true;
         }
 
         function bindExportsPropertyAssignment(node: BinaryExpression) {
             // When we create a property via 'exports.foo = bar', the 'exports.foo' property access
             // expression is the declaration
-            setCommonJsModuleIndicator(node);
+            if (!setCommonJsModuleIndicator(node)) {
+                return;
+            }
             const lhs = node.left as PropertyAccessEntityNameExpression;
             const symbol = forEachIdentifierInEntityName(lhs.expression, /*parent*/ undefined, (id, symbol) => {
                 if (symbol) {
@@ -2337,15 +2341,15 @@ namespace ts {
             // is still pointing to 'module.exports'.
             // We do not want to consider this as 'export=' since a module can have only one of these.
             // Similarly we do not want to treat 'module.exports = exports' as an 'export='.
+            if (!setCommonJsModuleIndicator(node)) {
+                return;
+            }
             const assignedExpression = getRightMostAssignedExpression(node.right);
             if (isEmptyObjectLiteral(assignedExpression) || container === file && isExportsOrModuleExportsOrAlias(file, assignedExpression)) {
-                // Mark it as a module in case there are no other exports in the file
-                setCommonJsModuleIndicator(node);
                 return;
             }
 
             // 'module.exports = expr' assignment
-            setCommonJsModuleIndicator(node);
             const flags = exportAssignmentIsAlias(node)
                 ? SymbolFlags.Alias // An export= with an EntityNameExpression or a ClassExpression exports all meanings of that identifier or class
                 : SymbolFlags.Property | SymbolFlags.ExportValue | SymbolFlags.ValueModule;
diff --git a/tests/baselines/reference/conflictingCommonJSES2015Exports.symbols b/tests/baselines/reference/conflictingCommonJSES2015Exports.symbols
@@ -0,0 +1,18 @@
+=== tests/cases/conformance/salsa/bug24934.js ===
+export function abc(a, b, c) { return 5; }
+>abc : Symbol(abc, Decl(bug24934.js, 0, 0))
+>a : Symbol(a, Decl(bug24934.js, 0, 20))
+>b : Symbol(b, Decl(bug24934.js, 0, 22))
+>c : Symbol(c, Decl(bug24934.js, 0, 25))
+
+module.exports = { abc };
+>module : Symbol(module)
+>abc : Symbol(abc, Decl(bug24934.js, 1, 18))
+
+=== tests/cases/conformance/salsa/use.js ===
+import { abc } from './bug24934';
+>abc : Symbol(abc, Decl(use.js, 0, 8))
+
+abc(1, 2, 3);
+>abc : Symbol(abc, Decl(use.js, 0, 8))
+
diff --git a/tests/baselines/reference/conflictingCommonJSES2015Exports.types b/tests/baselines/reference/conflictingCommonJSES2015Exports.types
@@ -0,0 +1,27 @@
+=== tests/cases/conformance/salsa/bug24934.js ===
+export function abc(a, b, c) { return 5; }
+>abc : (a: any, b: any, c: any) => number
+>a : any
+>b : any
+>c : any
+>5 : 5
+
+module.exports = { abc };
+>module.exports = { abc } : { [x: string]: any; abc: (a: any, b: any, c: any) => number; }
+>module.exports : any
+>module : any
+>exports : any
+>{ abc } : { [x: string]: any; abc: (a: any, b: any, c: any) => number; }
+>abc : (a: any, b: any, c: any) => number
+
+=== tests/cases/conformance/salsa/use.js ===
+import { abc } from './bug24934';
+>abc : (a: any, b: any, c: any) => number
+
+abc(1, 2, 3);
+>abc(1, 2, 3) : number
+>abc : (a: any, b: any, c: any) => number
+>1 : 1
+>2 : 2
+>3 : 3
+
diff --git a/tests/cases/conformance/salsa/conflictingCommonJSES2015Exports.ts b/tests/cases/conformance/salsa/conflictingCommonJSES2015Exports.ts
@@ -0,0 +1,9 @@
+// @checkJs: true
+// @allowJS: true
+// @noEmit: true
+// @Filename: bug24934.js
+export function abc(a, b, c) { return 5; }
+module.exports = { abc };
+// @Filename: use.js
+import { abc } from './bug24934';
+abc(1, 2, 3);
