improve renaming of variables in concatenated modules

more detailed tracking of conflicted references
to avoid renaming variables when possible

fixes #10168

lib/optimize/ConcatenatedModule.js

@@ -57,6 +57,13 @@ const propertyAccess = require("../util/propertyAccess");
  * @property {Dependency} dependency
  */
 
+/**
+ * @typedef {Object} Binding
+ * @property {ModuleInfo} info
+ * @property {string[]} ids
+ * @property {string[]} exportName
+ */
+
 /** @typedef {ConcatenatedModuleInfo | ExternalModuleInfo } ModuleInfo */
 
 /**
@@ -70,11 +77,12 @@ const propertyAccess = require("../util/propertyAccess");
  * @property {Iterable<string>} runtimeRequirements
  * @property {TODO} globalScope
  * @property {TODO} moduleScope
- * @property {TODO} internalNames
- * @property {Map<string | true, string>} exportMap
+ * @property {Map<string, string>} internalNames
+ * @property {Map<string, string>} exportMap
  * @property {Map<string, ReexportInfo>} reexportMap
  * @property {boolean} hasNamespaceObject
- * @property {TODO} namespaceObjectSource
+ * @property {string} namespaceObjectName
+ * @property {string} namespaceObjectSource
  */
 
 /**
@@ -162,7 +170,7 @@ const arrayEquals = (a, b) => {
  * @param {RequestShortener} requestShortener requestShortener
  * @param {RuntimeTemplate} runtimeTemplate runtimeTemplate
  * @param {boolean} strictHarmonyModule strictHarmonyModule
- * @returns {void}
+ * @returns {string} the name of the ns obj variable
  */
 const ensureNsObjSource = (
 	moduleGraph,
@@ -172,9 +180,9 @@ const ensureNsObjSource = (
 	runtimeTemplate,
 	strictHarmonyModule
 ) => {
+	const name = info.namespaceObjectName;
 	if (!info.hasNamespaceObject) {
 		info.hasNamespaceObject = true;
-		const name = info.exportMap.get(true);
 		const nsObj = [];
 		const exportsInfo = moduleGraph.getExportsInfo(info.module);
 		for (const exportInfo of exportsInfo.orderedExports) {
@@ -202,6 +210,7 @@ const ensureNsObjSource = (
 			","
 		)}\n});\n`;
 	}
+	return name;
 };
 
 /**
@@ -289,6 +298,81 @@ const getExternalImport = (
 	return reference;
 };
 
+/**
+ * @param {Set<ReexportInfo>} alreadyVisited alreadyVisited
+ * @param {RequestShortener} requestShortener the request shortener
+ * @param {ReexportInfo} reexport the current reexport
+ * @returns {void | never} throws error when circular
+ */
+const checkCircularReexport = (alreadyVisited, requestShortener, reexport) => {
+	if (alreadyVisited.has(reexport)) {
+		throw new Error(
+			`Circular reexports ${Array.from(
+				alreadyVisited,
+				e =>
+					`"${e.module.readableIdentifier(
+						requestShortener
+					)}".${e.exportName.join(".")}`
+			).join(" --> ")} -(circular)-> "${reexport.module.readableIdentifier(
+				requestShortener
+			)}".${reexport.exportName.join(".")}`
+		);
+	}
+	alreadyVisited.add(reexport);
+};
+
+/**
+ * @param {ModuleInfo} info module info
+ * @param {string[]} exportName exportName
+ * @param {Map<Module, ModuleInfo>} moduleToInfoMap moduleToInfoMap
+ * @param {RequestShortener} requestShortener the request shortener
+ * @param {Set<ReexportInfo>} alreadyVisited alreadyVisited
+ * @returns {Binding} the final variable
+ */
+const getFinalBinding = (
+	info,
+	exportName,
+	moduleToInfoMap,
+	requestShortener,
+	alreadyVisited = new Set()
+) => {
+	switch (info.type) {
+		case "concatenated": {
+			if (exportName.length === 0) {
+				return { info, ids: exportName, exportName };
+			}
+			const exportId = exportName[0];
+			const directExport = info.exportMap.get(exportId);
+			if (directExport) {
+				return {
+					info,
+					ids: [directExport, ...exportName.slice(1)],
+					exportName
+				};
+			}
+			const reexport = info.reexportMap.get(exportId);
+			if (reexport) {
+				checkCircularReexport(alreadyVisited, requestShortener, reexport);
+				const refInfo = moduleToInfoMap.get(reexport.module);
+				if (refInfo) {
+					// module is in the concatenation
+					return getFinalBinding(
+						refInfo,
+						[...reexport.exportName, ...exportName.slice(1)],
+						moduleToInfoMap,
+						requestShortener,
+						alreadyVisited
+					);
+				}
+			}
+			return { info, ids: null, exportName };
+		}
+		case "external": {
+			return { info, ids: exportName, exportName };
+		}
+	}
+};
+
 /**
  * @param {ModuleGraph} moduleGraph the module graph
  * @param {ModuleInfo} info module info
@@ -300,7 +384,6 @@ const getExternalImport = (
  * @param {boolean} callContext callContext
  * @param {boolean} strictHarmonyModule strictHarmonyModule
  * @param {boolean} asiSafe asiSafe
- * @param {Set<ReexportInfo>} alreadyVisited alreadyVisited
  * @returns {string} the final name
  */
 const getFinalName = (
@@ -313,13 +396,31 @@ const getFinalName = (
 	asCall,
 	callContext,
 	strictHarmonyModule,
-	asiSafe,
-	alreadyVisited = new Set()
+	asiSafe
 ) => {
-	switch (info.type) {
+	const binding = getFinalBinding(
+		info,
+		exportName,
+		moduleToInfoMap,
+		requestShortener
+	);
+	switch (binding.info.type) {
 		case "concatenated": {
-			if (exportName.length === 0) {
-				ensureNsObjSource(
+			const { info, ids, exportName } = binding;
+			if (!ids) {
+				const problem =
+					`Cannot get final name for export "${exportName}" in "${info.module.readableIdentifier(
+						requestShortener
+					)}"` +
+					` (known exports: ${Array.from(info.exportMap.keys()).join(" ")}, ` +
+					`known reexports: ${Array.from(info.reexportMap.keys()).join(" ")})`;
+				return `${Template.toNormalComment(problem)} undefined${propertyAccess(
+					exportName,
+					1
+				)}`;
+			}
+			if (ids.length === 0) {
+				return ensureNsObjSource(
 					moduleGraph,
 					info,
 					moduleToInfoMap,
@@ -327,84 +428,30 @@ const getFinalName = (
 					runtimeTemplate,
 					strictHarmonyModule
 				);
-				return info.internalNames.get(info.exportMap.get(true));
 			}
-			const exportId = exportName[0];
-			const directExport = info.exportMap.get(exportId);
+			const exportId = ids[0];
 			const exportsInfo = moduleGraph.getExportsInfo(info.module);
-			if (directExport) {
-				if (exportsInfo.isExportUsed(exportName) === UsageState.Unused) {
-					return `/* unused export */ undefined${propertyAccess(
-						exportName,
-						1
-					)}`;
-				}
-				const name = info.internalNames.get(directExport);
-				if (!name) {
-					throw new Error(
-						`The export "${directExport}" in "${info.module.readableIdentifier(
-							requestShortener
-						)}" has no internal name`
-					);
-				}
-				return `${name}${propertyAccess(exportName, 1)}`;
+			if (exportsInfo.isExportUsed(exportName) === UsageState.Unused) {
+				return `/* unused export */ undefined${propertyAccess(exportName, 1)}`;
 			}
-			const reexport = info.reexportMap.get(exportId);
-			if (reexport) {
-				if (alreadyVisited.has(reexport)) {
-					throw new Error(
-						`Circular reexports ${Array.from(
-							alreadyVisited,
-							e =>
-								`"${e.module.readableIdentifier(
-									requestShortener
-								)}".${e.exportName.join(".")}`
-						).join(
-							" --> "
-						)} -(circular)-> "${reexport.module.readableIdentifier(
-							requestShortener
-						)}".${reexport.exportName.join(".")}`
-					);
-				}
-				alreadyVisited.add(reexport);
-				const refInfo = moduleToInfoMap.get(reexport.module);
-				if (refInfo) {
-					// module is in the concatenation
-					return getFinalName(
-						moduleGraph,
-						refInfo,
-						[...reexport.exportName, ...exportName.slice(1)],
-						moduleToInfoMap,
-						requestShortener,
-						runtimeTemplate,
-						asCall,
-						callContext,
-						strictHarmonyModule,
-						asiSafe,
-						alreadyVisited
-					);
-				}
+			const name = info.internalNames.get(exportId);
+			if (!name) {
+				throw new Error(
+					`The export "${exportId}" in "${info.module.readableIdentifier(
+						requestShortener
+					)}" has no internal name`
+				);
 			}
-			const problem =
-				`Cannot get final name for export "${exportName}" in "${info.module.readableIdentifier(
-					requestShortener
-				)}"` +
-				` (known exports: ${Array.from(info.exportMap.keys())
-					.filter(name => name !== true)
-					.join(" ")}, ` +
-				`known reexports: ${Array.from(info.reexportMap.keys()).join(" ")})`;
-			return `${Template.toNormalComment(problem)} undefined${propertyAccess(
-				exportName,
-				1
-			)}`;
+			return `${name}${propertyAccess(exportName, 1)}`;
 		}
 		case "external": {
+			const { info, ids } = binding;
 			const importedModule = info.module;
 			return getExternalImport(
 				moduleGraph,
 				importedModule,
 				info,
-				exportName,
+				ids,
 				asCall,
 				callContext,
 				strictHarmonyModule,
@@ -531,8 +578,10 @@ const isModuleReference = name => {
 const matchModuleReference = (name, modulesWithInfo) => {
 	const match = MODULE_REFERENCE_REGEXP.exec(name);
 	if (!match) return null;
+	const index = +match[1];
 	return {
-		info: modulesWithInfo[+match[1]],
+		index,
+		info: modulesWithInfo[index],
 		ids:
 			match[2] === "ns"
 				? []
@@ -916,6 +965,18 @@ class ConcatenatedModule extends Module {
 		// List of all used names to avoid conflicts
 		const allUsedNames = new Set(RESERVED_NAMES);
 
+		// List of additional names in scope for module references
+		const usedNamesInScope = new Map();
+		const getUsedNamesInScopeSet = (module, id) => {
+			const key = `${module}-${id}`;
+			let set = usedNamesInScope.get(key);
+			if (set === undefined) {
+				set = new Set();
+				usedNamesInScope.set(key, set);
+			}
+			return set;
+		};
+
 		// Set of already checked scopes
 		const alreadyCheckedScopes = new Set();
 
@@ -946,21 +1007,34 @@ class ConcatenatedModule extends Module {
 					for (const reference of info.globalScope.through) {
 						const name = reference.identifier.name;
 						if (isModuleReference(name)) {
+							const match = matchModuleReference(name, modulesWithInfo);
+							if (!match || match.ids.length < 1) continue;
+							const binding = getFinalBinding(
+								match.info,
+								match.ids,
+								moduleToInfoMap,
+								requestShortener
+							);
+							if (!binding.ids) continue;
+							const usedNames = getUsedNamesInScopeSet(
+								binding.info.module.identifier(),
+								binding.info.type === "external"
+									? "external"
+									: binding.ids.length > 0
+									? binding.ids[0]
+									: ""
+							);
 							for (const expr of superClassExpressions) {
 								if (
 									expr.range[0] <= reference.identifier.range[0] &&
 									expr.range[1] >= reference.identifier.range[1]
 								) {
 									for (const variable of expr.variables) {
-										allUsedNames.add(variable.name);
+										usedNames.add(variable.name);
 									}
 								}
 							}
-							addScopeSymbols1(
-								reference.from,
-								allUsedNames,
-								alreadyCheckedScopes
-							);
+							addScopeSymbols1(reference.from, usedNames, alreadyCheckedScopes);
 						} else {
 							allUsedNames.add(name);
 						}
@@ -973,25 +1047,31 @@ class ConcatenatedModule extends Module {
 		for (const info of modulesWithInfo) {
 			switch (info.type) {
 				case "concatenated": {
+					const namespaceObjectUsedNames = getUsedNamesInScopeSet(
+						info.module.identifier(),
+						""
+					);
 					const namespaceObjectName = this.findNewName(
 						"namespaceObject",
 						allUsedNames,
-						null,
+						namespaceObjectUsedNames,
 						info.module.readableIdentifier(requestShortener)
 					);
 					allUsedNames.add(namespaceObjectName);
-					info.internalNames.set(namespaceObjectName, namespaceObjectName);
-					info.exportMap.set(true, namespaceObjectName);
+					info.namespaceObjectName = namespaceObjectName;
 					for (const variable of info.moduleScope.variables) {
 						const name = variable.name;
-						if (allUsedNames.has(name)) {
+						const usedNames = getUsedNamesInScopeSet(
+							info.module.identifier(),
+							name
+						);
+						if (allUsedNames.has(name) || usedNames.has(name)) {
 							const references = getAllReferences(variable);
-							const symbolsInReferences = new Set();
 							const alreadyCheckedInnerScopes = new Set();
 							for (const ref of references) {
 								addScopeSymbols2(
 									ref.from,
-									symbolsInReferences,
+									usedNames,
 									alreadyCheckedInnerScopes,
 									alreadyCheckedScopes
 								);
@@ -999,7 +1079,7 @@ class ConcatenatedModule extends Module {
 							const newName = this.findNewName(
 								name,
 								allUsedNames,
-								symbolsInReferences,
+								usedNames,
 								info.module.readableIdentifier(requestShortener)
 							);
 							allUsedNames.add(newName);
@@ -1030,10 +1110,14 @@ class ConcatenatedModule extends Module {
 					break;
 				}
 				case "external": {
+					const externalUsedNames = getUsedNamesInScopeSet(
+						info.module.identifier(),
+						"external"
+					);
 					const externalName = this.findNewName(
 						"",
 						allUsedNames,
-						null,
+						externalUsedNames,
 						info.module.readableIdentifier(requestShortener)
 					);
 					allUsedNames.add(externalName);
@@ -1046,7 +1130,7 @@ class ConcatenatedModule extends Module {
 						const externalNameInterop = this.findNewName(
 							"namespaceObject",
 							allUsedNames,
-							null,
+							externalUsedNames,
 							info.module.readableIdentifier(requestShortener)
 						);
 						allUsedNames.add(externalNameInterop);
@@ -1056,7 +1140,7 @@ class ConcatenatedModule extends Module {
 						const externalNameInterop = this.findNewName(
 							"default",
 							allUsedNames,
-							null,
+							externalUsedNames,
 							info.module.readableIdentifier(requestShortener)
 						);
 						allUsedNames.add(externalNameInterop);
@@ -1265,7 +1349,7 @@ class ConcatenatedModule extends Module {
 			let result;
 			switch (info.type) {
 				case "concatenated": {
-					/** @type {Map<string | true, string>} */
+					/** @type {Map<string, string>} */
 					const exportMap = new Map();
 					/** @type {Map<string, ReexportInfo>} */
 					const reexportMap = new Map();
@@ -1324,9 +1408,10 @@ class ConcatenatedModule extends Module {
 						globalScope: undefined,
 						moduleScope: undefined,
 						internalNames: new Map(),
-						exportMap: exportMap,
-						reexportMap: reexportMap,
+						exportMap,
+						reexportMap,
 						hasNamespaceObject: false,
+						namespaceObjectName: undefined,
 						namespaceObjectSource: null
 					};
 					break;
@@ -1744,7 +1829,7 @@ class HarmonyExportImportedSpecifierDependencyConcatenatedTemplate extends Depen
 					});
 				}
 				const map = new Map();
-				map.set(used, `/* concated reexport ${finalName} */ ${finalName}`);
+				map.set(used, `/* concated reexport */ ${finalName}`);
 				initFragments.push(
 					new HarmonyExportInitFragment(this.rootModule.exportsArgument, map)
 				);

test/configCases/concatenate-modules/rename-10168/A.js

@@ -0,0 +1 @@
+export class A {}

test/configCases/concatenate-modules/rename-10168/B.js

@@ -0,0 +1 @@
+export class B {}

test/configCases/concatenate-modules/rename-10168/C.js

@@ -0,0 +1,10 @@
+import { B as BB } from "./B";
+
+const X = 0;
+var Y = 0;
+
+export class C extends (function() {
+	var A = 0;
+	var B = 0;
+	return BB;
+})() {}

test/configCases/concatenate-modules/rename-10168/D.js

@@ -0,0 +1 @@
+export class D {}

test/configCases/concatenate-modules/rename-10168/E.js

@@ -0,0 +1,6 @@
+import { D as DD } from "./D";
+
+export class E extends (function() {
+	var D = 0;
+	return DD;
+})() {}

test/configCases/concatenate-modules/rename-10168/all.js

@@ -0,0 +1,6 @@
+import { A } from "./A";
+export { A };
+export { B } from "./B";
+export { C as CC } from "./C";
+export { D } from "./D";
+export { E } from "./E";

test/configCases/concatenate-modules/rename-10168/index.js

@@ -0,0 +1,12 @@
+import { A, B, CC, D, E } from "./all";
+
+require("./all");
+require("./D");
+
+it("should not rename classes unneccessary", () => {
+	expect(A.name).toBe("A");
+	expect(B.name).toBe("B_B");
+	expect(CC.name).toBe("C");
+	expect(D.name).toBe("D");
+	expect(E.name).toBe("E");
+});

test/configCases/concatenate-modules/rename-10168/webpack.config.js

@@ -0,0 +1,5 @@
+module.exports = {
+	optimization: {
+		concatenateModules: true
+	}
+};