Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent class ids from shadowing other variables #4697

Merged
merged 3 commits into from Nov 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/ast/nodes/ClassDeclaration.ts
Expand Up @@ -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);
}
}
}
}
}
24 changes: 19 additions & 5 deletions src/ast/nodes/VariableDeclarator.ts
Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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);
}
}
}
}
}
2 changes: 1 addition & 1 deletion src/ast/scopes/ChildScope.ts
Expand Up @@ -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) {
Expand Down
9 changes: 9 additions & 0 deletions src/ast/variables/Variable.ts
Expand Up @@ -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
Expand All @@ -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;
}
Expand Down
26 changes: 17 additions & 9 deletions src/utils/deconflictChunk.ts
Expand Up @@ -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) {
Expand All @@ -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));
}
}

Expand All @@ -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;
Expand Down Expand Up @@ -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)
);
}
}
}
8 changes: 6 additions & 2 deletions 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);
Expand Down
3 changes: 3 additions & 0 deletions test/function/samples/class-name-conflict2/_config.js
@@ -0,0 +1,3 @@
module.exports = {
description: 'does not shadow variables when preserving class names'
};
3 changes: 3 additions & 0 deletions test/function/samples/class-name-conflict2/bar.js
@@ -0,0 +1,3 @@
export class bar {
static base = true;
}
12 changes: 12 additions & 0 deletions 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();
}
12 changes: 12 additions & 0 deletions 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();
}
3 changes: 3 additions & 0 deletions test/function/samples/class-name-conflict2/foo.js
@@ -0,0 +1,3 @@
export class foo {
static base = true;
}
2 changes: 2 additions & 0 deletions test/function/samples/class-name-conflict2/main.js
@@ -0,0 +1,2 @@
import './expression';
import './declaration';