Avoid name conflicts when inlining dynamic imports nested in functions (

#3256)

* Avoid name conflicts when inlining dynamic imports nested in functions

* Deduplicate code

* Remove unneeded checks

src/ast/nodes/ImportExpression.ts

@@ -12,11 +12,11 @@ interface DynamicImportMechanism {
 }
 
 export default class Import extends NodeBase {
+	inlineNamespace?: NamespaceVariable;
 	source!: ExpressionNode;
 	type!: NodeType.tImportExpression;
 
 	private exportMode: 'none' | 'named' | 'default' | 'auto' = 'auto';
-	private inlineNamespace?: NamespaceVariable;
 
 	hasEffects(): boolean {
 		return true;
@@ -26,6 +26,7 @@ export default class Import extends NodeBase {
 		if (!this.included) {
 			this.included = true;
 			this.context.includeDynamicImport(this);
+			this.scope.addAccessedDynamicImport(this);
 		}
 		this.source.include(context, includeChildrenRecursively);
 	}

src/ast/scopes/ChildScope.ts

@@ -1,9 +1,11 @@
 import { getSafeName } from '../../utils/safeName';
+import ImportExpression from '../nodes/ImportExpression';
 import { ExpressionEntity } from '../nodes/shared/Expression';
 import Variable from '../variables/Variable';
 import Scope from './Scope';
 
 export default class ChildScope extends Scope {
+	accessedDynamicImports?: Set<ImportExpression>;
 	accessedGlobalVariablesByFormat?: Map<string, Set<string>>;
 	accessedOutsideVariables = new Map<string, Variable>();
 	parent: Scope;
@@ -14,11 +16,18 @@ export default class ChildScope extends Scope {
 		parent.children.push(this);
 	}
 
-	addAccessedGlobalsByFormat(globalsByFormat: { [format: string]: string[] }) {
-		let accessedGlobalVariablesByFormat = this.accessedGlobalVariablesByFormat;
-		if (!accessedGlobalVariablesByFormat) {
-			accessedGlobalVariablesByFormat = this.accessedGlobalVariablesByFormat = new Map();
+	addAccessedDynamicImport(importExpression: ImportExpression) {
+		(this.accessedDynamicImports || (this.accessedDynamicImports = new Set())).add(
+			importExpression
+		);
+		if (this.parent instanceof ChildScope) {
+			this.parent.addAccessedDynamicImport(importExpression);
 		}
+	}
+
+	addAccessedGlobalsByFormat(globalsByFormat: { [format: string]: string[] }) {
+		const accessedGlobalVariablesByFormat =
+			this.accessedGlobalVariablesByFormat || (this.accessedGlobalVariablesByFormat = new Map());
 		for (const format of Object.keys(globalsByFormat)) {
 			let accessedGlobalVariables = accessedGlobalVariablesByFormat.get(format);
 			if (!accessedGlobalVariables) {
@@ -36,21 +45,14 @@ export default class ChildScope extends Scope {
 
 	addNamespaceMemberAccess(name: string, variable: Variable) {
 		this.accessedOutsideVariables.set(name, variable);
-		if (this.parent instanceof ChildScope) {
-			this.parent.addNamespaceMemberAccess(name, variable);
-		}
+		(this.parent as ChildScope).addNamespaceMemberAccess(name, variable);
 	}
 
 	addReturnExpression(expression: ExpressionEntity) {
 		this.parent instanceof ChildScope && this.parent.addReturnExpression(expression);
 	}
 
-	contains(name: string): boolean {
-		return this.variables.has(name) || this.parent.contains(name);
-	}
-
-	deconflict(format: string) {
-		const usedNames = new Set<string>();
+	addUsedOutsideNames(usedNames: Set<string>, format: string): void {
 		for (const variable of this.accessedOutsideVariables.values()) {
 			if (variable.included) {
 				usedNames.add(variable.getBaseVariableName());
@@ -66,6 +68,22 @@ export default class ChildScope extends Scope {
 				usedNames.add(name);
 			}
 		}
+	}
+
+	contains(name: string): boolean {
+		return this.variables.has(name) || this.parent.contains(name);
+	}
+
+	deconflict(format: string) {
+		const usedNames = new Set<string>();
+		this.addUsedOutsideNames(usedNames, format);
+		if (this.accessedDynamicImports) {
+			for (const importExpression of this.accessedDynamicImports) {
+				if (importExpression.inlineNamespace) {
+					usedNames.add(importExpression.inlineNamespace.getBaseVariableName());
+				}
+			}
+		}
 		for (const [name, variable] of this.variables) {
 			if (variable.included || variable.alwaysRendered) {
 				variable.setSafeName(getSafeName(name, usedNames));
@@ -77,7 +95,7 @@ export default class ChildScope extends Scope {
 	}
 
 	findLexicalBoundary(): ChildScope {
-		return this.parent instanceof ChildScope ? this.parent.findLexicalBoundary() : this;
+		return (this.parent as ChildScope).findLexicalBoundary();
 	}
 
 	findVariable(name: string): Variable {

src/utils/deconflictChunk.ts

@@ -31,7 +31,9 @@ export function deconflictChunk(
 	interop: boolean,
 	preserveModules: boolean
 ) {
-	addUsedGlobalNames(usedNames, modules, format);
+	for (const module of modules) {
+		module.scope.addUsedOutsideNames(usedNames, format);
+	}
 	deconflictTopLevelVariables(usedNames, modules);
 	DECONFLICT_IMPORTED_VARIABLES_BY_FORMAT[format](
 		usedNames,
@@ -46,25 +48,6 @@ export function deconflictChunk(
 	}
 }
 
-function addUsedGlobalNames(usedNames: Set<string>, modules: Module[], format: string) {
-	for (const module of modules) {
-		const moduleScope = module.scope;
-		for (const [name, variable] of moduleScope.accessedOutsideVariables) {
-			if (variable.included) {
-				usedNames.add(name);
-			}
-		}
-		const accessedGlobalVariables =
-			moduleScope.accessedGlobalVariablesByFormat &&
-			moduleScope.accessedGlobalVariablesByFormat.get(format);
-		if (accessedGlobalVariables) {
-			for (const name of accessedGlobalVariables) {
-				usedNames.add(name);
-			}
-		}
-	}
-}
-
 function deconflictImportsEsm(
 	usedNames: Set<string>,
 	imports: Set<Variable>,
@@ -114,9 +97,10 @@ function deconflictImportsOther(
 			if (chunk.exportMode === 'default' || (preserveModules && variable.isNamespace)) {
 				variable.setRenderNames(null, chunk.variableName);
 			} else {
-				variable.setRenderNames(chunk.variableName, chunk.getVariableExportName(variable) as
-					| string
-					| null);
+				variable.setRenderNames(
+					chunk.variableName,
+					chunk.getVariableExportName(variable) as string | null
+				);
 			}
 		}
 	}

test/form/samples/nested-inlined-dynamic-import/_config.js

@@ -0,0 +1,6 @@
+module.exports = {
+	description: 'deconflicts variables when nested dynamic imports are inlined',
+	options: {
+		inlineDynamicImports: true
+	}
+};

test/form/samples/nested-inlined-dynamic-import/_expected/amd.js

@@ -0,0 +1,18 @@
+define(function () { 'use strict';
+
+	async function main() {
+		const foo$1 = 1;
+		const ns = await Promise.resolve().then(function () { return foo; });
+		console.log(ns.value + foo$1);
+	}
+
+	main();
+
+	const value = 42;
+
+	var foo = /*#__PURE__*/Object.freeze({
+		__proto__: null,
+		value: value
+	});
+
+});

test/form/samples/nested-inlined-dynamic-import/_expected/cjs.js

@@ -0,0 +1,16 @@
+'use strict';
+
+async function main() {
+	const foo$1 = 1;
+	const ns = await Promise.resolve().then(function () { return foo; });
+	console.log(ns.value + foo$1);
+}
+
+main();
+
+const value = 42;
+
+var foo = /*#__PURE__*/Object.freeze({
+	__proto__: null,
+	value: value
+});

test/form/samples/nested-inlined-dynamic-import/_expected/es.js

@@ -0,0 +1,14 @@
+async function main() {
+	const foo$1 = 1;
+	const ns = await Promise.resolve().then(function () { return foo; });
+	console.log(ns.value + foo$1);
+}
+
+main();
+
+const value = 42;
+
+var foo = /*#__PURE__*/Object.freeze({
+	__proto__: null,
+	value: value
+});

test/form/samples/nested-inlined-dynamic-import/_expected/iife.js

@@ -0,0 +1,19 @@
+(function () {
+	'use strict';
+
+	async function main() {
+		const foo$1 = 1;
+		const ns = await Promise.resolve().then(function () { return foo; });
+		console.log(ns.value + foo$1);
+	}
+
+	main();
+
+	const value = 42;
+
+	var foo = /*#__PURE__*/Object.freeze({
+		__proto__: null,
+		value: value
+	});
+
+}());

test/form/samples/nested-inlined-dynamic-import/_expected/system.js

@@ -0,0 +1,23 @@
+System.register([], function () {
+	'use strict';
+	return {
+		execute: function () {
+
+			async function main() {
+				const foo$1 = 1;
+				const ns = await Promise.resolve().then(function () { return foo; });
+				console.log(ns.value + foo$1);
+			}
+
+			main();
+
+			const value = 42;
+
+			var foo = /*#__PURE__*/Object.freeze({
+				__proto__: null,
+				value: value
+			});
+
+		}
+	};
+});

test/form/samples/nested-inlined-dynamic-import/_expected/umd.js

@@ -0,0 +1,21 @@
+(function (factory) {
+	typeof define === 'function' && define.amd ? define(factory) :
+	factory();
+}((function () { 'use strict';
+
+	async function main() {
+		const foo$1 = 1;
+		const ns = await Promise.resolve().then(function () { return foo; });
+		console.log(ns.value + foo$1);
+	}
+
+	main();
+
+	const value = 42;
+
+	var foo = /*#__PURE__*/Object.freeze({
+		__proto__: null,
+		value: value
+	});
+
+})));

test/form/samples/nested-inlined-dynamic-import/foo.js

@@ -0,0 +1 @@
+export const value = 42;

test/form/samples/nested-inlined-dynamic-import/main.js

@@ -0,0 +1,7 @@
+async function main() {
+	const foo = 1;
+	const ns = await import('./foo.js');
+	console.log(ns.value + foo);
+}
+
+main();

test/function/samples/nested-inlined-dynamic-import-1/_config.js

@@ -0,0 +1,12 @@
+const assert = require('assert');
+
+module.exports = {
+	description:
+		'deconflicts variables when nested dynamic imports are inlined via inlineDynamicImports',
+	options: {
+		inlineDynamicImports: true
+	},
+	exports(exports) {
+		return exports().then(result => assert.strictEqual(result, 43));
+	}
+};

test/function/samples/nested-inlined-dynamic-import-1/foo.js

@@ -0,0 +1 @@
+export const value = 42;

test/function/samples/nested-inlined-dynamic-import-1/main.js

@@ -0,0 +1,4 @@
+export default () => {
+	const foo = 1;
+	return import('./foo.js').then(ns => ns.value + foo);
+};

test/function/samples/nested-inlined-dynamic-import-2/_config.js

@@ -0,0 +1,16 @@
+const assert = require('assert');
+
+module.exports = {
+	description: 'deconflicts variables when nested dynamic imports are inlined',
+	warnings: [
+		{
+			code: 'CIRCULAR_DEPENDENCY',
+			cycle: ['lib1.js', 'lib2.js', 'lib1.js'],
+			importer: 'lib1.js',
+			message: 'Circular dependency: lib1.js -> lib2.js -> lib1.js'
+		}
+	],
+	exports(exports) {
+		return exports().then(result => assert.strictEqual(result, 43));
+	}
+};

test/function/samples/nested-inlined-dynamic-import-2/lib1.js

@@ -0,0 +1,6 @@
+import './lib2.js';
+
+export default () => {
+	const lib2 = 1;
+	return import('./lib2.js').then(ns => ns.foo + lib2);
+};

test/function/samples/nested-inlined-dynamic-import-2/lib2.js

@@ -0,0 +1,3 @@
+import './lib1.js';
+
+export const foo = 42;

test/function/samples/nested-inlined-dynamic-import-2/main.js

@@ -0,0 +1 @@
+export {default} from './lib1.js';