Prevent class ids from shadowing other variables (#4697)

* Create test * Avoid conflicts between class names and local variables * Improve coverage
rollup · Nov 1, 2022 · 465d239 · 465d239
1 parent ab36666
commit 465d239
Show file tree

Hide file tree

Showing 12 changed files with 100 additions and 17 deletions.
diff --git a/src/ast/nodes/ClassDeclaration.ts b/src/ast/nodes/ClassDeclaration.ts
@@ -51,4 +51,17 @@ export default class ClassDeclaration extends ClassNode {
 		}
 		super.render(code, options);
 	}
+
+	protected applyDeoptimizations(): void {
+		super.applyDeoptimizations();
+		const { id, scope } = this;
+		if (id) {
+			const { name, variable } = id;
+			for (const accessedVariable of scope.accessedOutsideVariables.values()) {
+				if (accessedVariable !== variable) {
+					accessedVariable.forbidName(name);
+				}
+			}
+		}
+	}
 }
diff --git a/src/ast/nodes/VariableDeclarator.ts b/src/ast/nodes/VariableDeclarator.ts
@@ -29,17 +29,20 @@ export default class VariableDeclarator extends NodeBase {
 	}
 
 	hasEffects(context: HasEffectsContext): boolean {
+		if (!this.deoptimized) this.applyDeoptimizations();
 		const initEffect = this.init?.hasEffects(context);
 		this.id.markDeclarationReached();
 		return initEffect || this.id.hasEffects(context);
 	}
 
 	include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {
+		const { deoptimized, id, init } = this;
+		if (!deoptimized) this.applyDeoptimizations();
 		this.included = true;
-		this.init?.include(context, includeChildrenRecursively);
-		this.id.markDeclarationReached();
-		if (includeChildrenRecursively || this.id.shouldBeIncluded(context)) {
-			this.id.include(context, includeChildrenRecursively);
+		init?.include(context, includeChildrenRecursively);
+		id.markDeclarationReached();
+		if (includeChildrenRecursively || id.shouldBeIncluded(context)) {
+			id.include(context, includeChildrenRecursively);
 		}
 	}
 
@@ -76,5 +79,16 @@ export default class VariableDeclarator extends NodeBase {
 		}
 	}
 
-	protected applyDeoptimizations() {}
+	protected applyDeoptimizations() {
+		this.deoptimized = true;
+		const { id, init } = this;
+		if (init && id instanceof Identifier && init instanceof ClassExpression && !init.id) {
+			const { name, variable } = id;
+			for (const accessedVariable of init.scope.accessedOutsideVariables.values()) {
+				if (accessedVariable !== variable) {
+					accessedVariable.forbidName(name);
+				}
+			}
+		}
+	}
 }
diff --git a/src/ast/scopes/ChildScope.ts b/src/ast/scopes/ChildScope.ts
@@ -90,7 +90,7 @@ export default class ChildScope extends Scope {
 		}
 		for (const [name, variable] of this.variables) {
 			if (variable.included || variable.alwaysRendered) {
-				variable.setRenderNames(null, getSafeName(name, usedNames));
+				variable.setRenderNames(null, getSafeName(name, usedNames, variable.forbiddenNames));
 			}
 		}
 		for (const scope of this.children) {

diff --git a/src/ast/variables/Variable.ts b/src/ast/variables/Variable.ts
@@ -9,6 +9,7 @@ import type { ObjectPath } from '../utils/PathTracker';
 
 export default class Variable extends ExpressionEntity {
 	alwaysRendered = false;
+	forbiddenNames: Set<string> | null = null;
 	initReached = false;
 	isId = false;
 	// both NamespaceVariable and ExternalVariable can be namespaces
@@ -29,6 +30,14 @@ export default class Variable extends ExpressionEntity {
 	 */
 	addReference(_identifier: Identifier): void {}
 
+	/**
+	 * Prevent this variable from being renamed to this name to avoid name
+	 * collisions
+	 */
+	forbidName(name: string) {
+		(this.forbiddenNames ||= new Set()).add(name);
+	}
+
 	getBaseVariableName(): string {
 		return this.renderBaseName || this.renderName || this.name;
 	}

diff --git a/src/utils/deconflictChunk.ts b/src/utils/deconflictChunk.ts
@@ -99,7 +99,7 @@ function deconflictImportsEsmOrSystem(
 	// This is needed for namespace reexports
 	for (const dependency of dependenciesToBeDeconflicted.dependencies) {
 		if (preserveModules || dependency instanceof ExternalChunk) {
-			dependency.variableName = getSafeName(dependency.suggestedVariableName, usedNames);
+			dependency.variableName = getSafeName(dependency.suggestedVariableName, usedNames, null);
 		}
 	}
 	for (const variable of imports) {
@@ -122,15 +122,16 @@ function deconflictImportsEsmOrSystem(
 					)
 						? module.suggestedVariableName + '__default'
 						: module.suggestedVariableName,
-					usedNames
+					usedNames,
+					variable.forbiddenNames
 				)
 			);
 		} else {
-			variable.setRenderNames(null, getSafeName(name, usedNames));
+			variable.setRenderNames(null, getSafeName(name, usedNames, variable.forbiddenNames));
 		}
 	}
 	for (const variable of syntheticExports) {
-		variable.setRenderNames(null, getSafeName(variable.name, usedNames));
+		variable.setRenderNames(null, getSafeName(variable.name, usedNames, variable.forbiddenNames));
 	}
 }
 
@@ -145,20 +146,21 @@ function deconflictImportsOther(
 	externalChunkByModule: ReadonlyMap<ExternalModule, ExternalChunk>
 ): void {
 	for (const chunk of dependencies) {
-		chunk.variableName = getSafeName(chunk.suggestedVariableName, usedNames);
+		chunk.variableName = getSafeName(chunk.suggestedVariableName, usedNames, null);
 	}
 	for (const chunk of deconflictedNamespace) {
 		chunk.namespaceVariableName = getSafeName(
 			`${chunk.suggestedVariableName}__namespace`,
-			usedNames
+			usedNames,
+			null
 		);
 	}
 	for (const externalModule of deconflictedDefault) {
 		externalModule.defaultVariableName =
 			deconflictedNamespace.has(externalModule) &&
 			canDefaultBeTakenFromNamespace(interop(externalModule.id), externalLiveBindings)
 				? externalModule.namespaceVariableName
-				: getSafeName(`${externalModule.suggestedVariableName}__default`, usedNames);
+				: getSafeName(`${externalModule.suggestedVariableName}__default`, usedNames, null);
 	}
 	for (const variable of imports) {
 		const module = variable.module;
@@ -220,12 +222,18 @@ function deconflictTopLevelVariables(
 					(variable instanceof ExportDefaultVariable && variable.getOriginalVariable() !== variable)
 				)
 			) {
-				variable.setRenderNames(null, getSafeName(variable.name, usedNames));
+				variable.setRenderNames(
+					null,
+					getSafeName(variable.name, usedNames, variable.forbiddenNames)
+				);
 			}
 		}
 		if (includedNamespaces.has(module)) {
 			const namespace = module.namespace;
-			namespace.setRenderNames(null, getSafeName(namespace.name, usedNames));
+			namespace.setRenderNames(
+				null,
+				getSafeName(namespace.name, usedNames, namespace.forbiddenNames)
+			);
 		}
 	}
 }
diff --git a/src/utils/safeName.ts b/src/utils/safeName.ts
@@ -1,10 +1,14 @@
 import RESERVED_NAMES from './RESERVED_NAMES';
 import { toBase64 } from './base64';
 
-export function getSafeName(baseName: string, usedNames: Set<string>): string {
+export function getSafeName(
+	baseName: string,
+	usedNames: Set<string>,
+	forbiddenNames: Set<string> | null
+): string {
 	let safeName = baseName;
 	let count = 1;
-	while (usedNames.has(safeName) || RESERVED_NAMES.has(safeName)) {
+	while (usedNames.has(safeName) || RESERVED_NAMES.has(safeName) || forbiddenNames?.has(safeName)) {
 		safeName = `${baseName}$${toBase64(count++)}`;
 	}
 	usedNames.add(safeName);

diff --git a/test/function/samples/class-name-conflict2/_config.js b/test/function/samples/class-name-conflict2/_config.js
@@ -0,0 +1,3 @@
+module.exports = {
+	description: 'does not shadow variables when preserving class names'
+};
diff --git a/test/function/samples/class-name-conflict2/bar.js b/test/function/samples/class-name-conflict2/bar.js
@@ -0,0 +1,3 @@
+export class bar {
+	static base = true;
+}
diff --git a/test/function/samples/class-name-conflict2/declaration.js b/test/function/samples/class-name-conflict2/declaration.js
@@ -0,0 +1,12 @@
+import { foo as foo$ } from './foo.js';
+
+{
+	class foo extends foo$ {
+		static test() {
+			assert.ok(foo.base);
+		}
+	}
+
+	assert.strictEqual(foo.name, 'foo');
+	foo.test();
+}
diff --git a/test/function/samples/class-name-conflict2/expression.js b/test/function/samples/class-name-conflict2/expression.js
@@ -0,0 +1,12 @@
+import { bar as bar$ } from './bar.js';
+
+{
+	let bar = class extends bar$ {
+		static test() {
+			assert.ok(bar.base);
+		}
+	};
+
+	assert.strictEqual(bar.name, 'bar');
+	bar.test();
+}
diff --git a/test/function/samples/class-name-conflict2/foo.js b/test/function/samples/class-name-conflict2/foo.js
@@ -0,0 +1,3 @@
+export class foo {
+	static base = true;
+}
diff --git a/test/function/samples/class-name-conflict2/main.js b/test/function/samples/class-name-conflict2/main.js
@@ -0,0 +1,2 @@
+import './expression';
+import './declaration';