Allow function expression parameters to shadow the function id (#5262)

* Do not explicitly pass context when creating scopes * Allow function expression parameters to shadow their id * Describe dashes in PR template
rollup · Nov 21, 2023 · 2ebab0c · 2ebab0c
1 parent ce6f492
commit 2ebab0c
Show file tree

Hide file tree

Showing 23 changed files with 65 additions and 40 deletions.
diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md
@@ -36,10 +36,12 @@ List any relevant issue numbers:
 <!--
 If this PR resolves any issues, list them as
 
-  resolves #1234
+-  resolves #1234
 
 where 1234 is the issue number. This will help us with house-keeping as Github will automatically add a note to those issues stating that a potential fix exists. Once the PR is merged, Github will automatically close those issues.
 
+Starting each line with a dash "-" will cause GitHub to display the issue title inline.
+
 If an issue is only solved partially or is relevant in some other way, just list the number without "resolves".
 -->
 

diff --git a/src/ast/nodes/ArrowFunctionExpression.ts b/src/ast/nodes/ArrowFunctionExpression.ts
@@ -1,8 +1,8 @@
 import type { HasEffectsContext, InclusionContext } from '../ExecutionContext';
 import type { NodeInteraction } from '../NodeInteractions';
 import { INTERACTION_CALLED } from '../NodeInteractions';
+import type ChildScope from '../scopes/ChildScope';
 import ReturnValueScope from '../scopes/ReturnValueScope';
-import type Scope from '../scopes/Scope';
 import { type ObjectPath } from '../utils/PathTracker';
 import type BlockStatement from './BlockStatement';
 import Identifier from './Identifier';
@@ -21,8 +21,8 @@ export default class ArrowFunctionExpression extends FunctionBase {
 	declare type: NodeType.tArrowFunctionExpression;
 	protected objectEntity: ObjectEntity | null = null;
 
-	createScope(parentScope: Scope): void {
-		this.scope = new ReturnValueScope(parentScope, this.scope.context, false);
+	createScope(parentScope: ChildScope): void {
+		this.scope = new ReturnValueScope(parentScope, false);
 	}
 
 	hasEffects(): boolean {

diff --git a/src/ast/nodes/BlockStatement.ts b/src/ast/nodes/BlockStatement.ts
@@ -37,7 +37,7 @@ export default class BlockStatement extends StatementBase {
 	createScope(parentScope: ChildScope): void {
 		this.scope = (this.parent as Node).preventChildBlockScope
 			? (parentScope as ChildScope)
-			: new BlockScope(parentScope, this.scope.context);
+			: new BlockScope(parentScope);
 	}
 
 	hasEffects(context: HasEffectsContext): boolean {

diff --git a/src/ast/nodes/CatchClause.ts b/src/ast/nodes/CatchClause.ts
@@ -15,7 +15,7 @@ export default class CatchClause extends NodeBase {
 	declare type: NodeType.tCatchClause;
 
 	createScope(parentScope: ChildScope): void {
-		this.scope = new ParameterScope(parentScope, this.scope.context, true);
+		this.scope = new ParameterScope(parentScope, true);
 	}
 
 	parseNode(esTreeNode: GenericEsTreeNode): void {

diff --git a/src/ast/nodes/ClassBody.ts b/src/ast/nodes/ClassBody.ts
@@ -14,7 +14,7 @@ export default class ClassBody extends NodeBase {
 	declare type: NodeType.tClassBody;
 
 	createScope(parentScope: ChildScope): void {
-		this.scope = new ClassBodyScope(parentScope, this.parent as ClassNode, this.scope.context);
+		this.scope = new ClassBodyScope(parentScope, this.parent as ClassNode);
 	}
 
 	include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {

diff --git a/src/ast/nodes/ForInStatement.ts b/src/ast/nodes/ForInStatement.ts
@@ -24,7 +24,7 @@ export default class ForInStatement extends StatementBase {
 	declare type: NodeType.tForInStatement;
 
 	createScope(parentScope: ChildScope): void {
-		this.scope = new BlockScope(parentScope, this.scope.context);
+		this.scope = new BlockScope(parentScope);
 	}
 
 	hasEffects(context: HasEffectsContext): boolean {

diff --git a/src/ast/nodes/ForOfStatement.ts b/src/ast/nodes/ForOfStatement.ts
@@ -32,7 +32,7 @@ export default class ForOfStatement extends StatementBase {
 	}
 
 	createScope(parentScope: ChildScope): void {
-		this.scope = new BlockScope(parentScope, this.scope.context);
+		this.scope = new BlockScope(parentScope);
 	}
 
 	hasEffects(): boolean {

diff --git a/src/ast/nodes/ForStatement.ts b/src/ast/nodes/ForStatement.ts
@@ -21,7 +21,7 @@ export default class ForStatement extends StatementBase {
 	declare update: ExpressionNode | null;
 
 	createScope(parentScope: ChildScope): void {
-		this.scope = new BlockScope(parentScope, this.scope.context);
+		this.scope = new BlockScope(parentScope);
 	}
 
 	hasEffects(context: HasEffectsContext): boolean {

diff --git a/src/ast/nodes/FunctionExpression.ts b/src/ast/nodes/FunctionExpression.ts
@@ -1,11 +1,27 @@
 import type MagicString from 'magic-string';
 import { BLANK } from '../../utils/blank';
 import type { NodeRenderOptions, RenderOptions } from '../../utils/renderHelpers';
+import ChildScope from '../scopes/ChildScope';
+import type { IdentifierWithVariable } from './Identifier';
+import Identifier from './Identifier';
 import * as NodeType from './NodeType';
 import FunctionNode from './shared/FunctionNode';
+import type { GenericEsTreeNode } from './shared/Node';
 
 export default class FunctionExpression extends FunctionNode {
 	declare type: NodeType.tFunctionExpression;
+	declare idScope: ChildScope;
+
+	createScope(parentScope: ChildScope) {
+		super.createScope((this.idScope = new ChildScope(parentScope, parentScope.context)));
+	}
+
+	parseNode(esTreeNode: GenericEsTreeNode) {
+		if (esTreeNode.id !== null) {
+			this.id = new Identifier(esTreeNode.id, this, this.idScope) as IdentifierWithVariable;
+		}
+		super.parseNode(esTreeNode);
+	}
 
 	render(
 		code: MagicString,

diff --git a/src/ast/nodes/IfStatement.ts b/src/ast/nodes/IfStatement.ts
@@ -67,14 +67,14 @@ export default class IfStatement extends StatementBase implements DeoptimizableE
 	}
 
 	parseNode(esTreeNode: GenericEsTreeNode): void {
-		this.consequentScope = new TrackingScope(this.scope, this.scope.context);
+		this.consequentScope = new TrackingScope(this.scope);
 		this.consequent = new (this.scope.context.getNodeConstructor(esTreeNode.consequent.type))(
 			esTreeNode.consequent,
 			this,
 			this.consequentScope
 		);
 		if (esTreeNode.alternate) {
-			this.alternateScope = new TrackingScope(this.scope, this.scope.context);
+			this.alternateScope = new TrackingScope(this.scope);
 			this.alternate = new (this.scope.context.getNodeConstructor(esTreeNode.alternate.type))(
 				esTreeNode.alternate,
 				this,

diff --git a/src/ast/nodes/StaticBlock.ts b/src/ast/nodes/StaticBlock.ts
@@ -15,7 +15,7 @@ export default class StaticBlock extends StatementBase {
 	declare type: NodeType.tStaticBlock;
 
 	createScope(parentScope: ChildScope): void {
-		this.scope = new BlockScope(parentScope, this.scope.context);
+		this.scope = new BlockScope(parentScope);
 	}
 
 	hasEffects(context: HasEffectsContext): boolean {

diff --git a/src/ast/nodes/SwitchStatement.ts b/src/ast/nodes/SwitchStatement.ts
@@ -22,7 +22,7 @@ export default class SwitchStatement extends StatementBase {
 
 	createScope(parentScope: ChildScope): void {
 		this.parentScope = parentScope;
-		this.scope = new BlockScope(parentScope, this.scope.context);
+		this.scope = new BlockScope(parentScope);
 	}
 
 	hasEffects(context: HasEffectsContext): boolean {

diff --git a/src/ast/nodes/shared/ClassNode.ts b/src/ast/nodes/shared/ClassNode.ts
@@ -30,7 +30,7 @@ export default class ClassNode extends NodeBase implements DeoptimizableEntity {
 	private objectEntity: ObjectEntity | null = null;
 
 	createScope(parentScope: ChildScope): void {
-		this.scope = new ChildScope(parentScope, this.scope.context);
+		this.scope = new ChildScope(parentScope, parentScope.context);
 	}
 
 	deoptimizeArgumentsOnInteractionAtPath(

diff --git a/src/ast/nodes/shared/FunctionNode.ts b/src/ast/nodes/shared/FunctionNode.ts
@@ -1,6 +1,7 @@
 import { type HasEffectsContext, type InclusionContext } from '../../ExecutionContext';
 import type { NodeInteraction } from '../../NodeInteractions';
 import { INTERACTION_CALLED } from '../../NodeInteractions';
+import type ChildScope from '../../scopes/ChildScope';
 import FunctionScope from '../../scopes/FunctionScope';
 import type { ObjectPath, PathTracker } from '../../utils/PathTracker';
 import type BlockStatement from '../BlockStatement';
@@ -23,8 +24,8 @@ export default class FunctionNode extends FunctionBase {
 	protected objectEntity: ObjectEntity | null = null;
 	private declare constructedEntity: ObjectEntity;
 
-	createScope(parentScope: FunctionScope): void {
-		this.scope = new FunctionScope(parentScope, this.scope.context);
+	createScope(parentScope: ChildScope): void {
+		this.scope = new FunctionScope(parentScope);
 		this.constructedEntity = new ObjectEntity(Object.create(null), OBJECT_PROTOTYPE);
 		// This makes sure that all deoptimizations of "this" are applied to the
 		// constructed entity.

diff --git a/src/ast/scopes/BlockScope.ts b/src/ast/scopes/BlockScope.ts
@@ -7,6 +7,10 @@ import type LocalVariable from '../variables/LocalVariable';
 import ChildScope from './ChildScope';
 
 export default class BlockScope extends ChildScope {
+	constructor(parent: ChildScope) {
+		super(parent, parent.context);
+	}
+
 	addDeclaration(
 		identifier: Identifier,
 		context: AstContext,

diff --git a/src/ast/scopes/CatchBodyScope.ts b/src/ast/scopes/CatchBodyScope.ts
@@ -10,11 +10,8 @@ import ChildScope from './ChildScope';
 import type ParameterScope from './ParameterScope';
 
 export default class CatchBodyScope extends ChildScope {
-	constructor(
-		readonly parent: ParameterScope,
-		readonly context: AstContext
-	) {
-		super(parent, context);
+	constructor(readonly parent: ParameterScope) {
+		super(parent, parent.context);
 	}
 
 	addDeclaration(

diff --git a/src/ast/scopes/ClassBodyScope.ts b/src/ast/scopes/ClassBodyScope.ts
@@ -1,16 +1,15 @@
-import type { AstContext } from '../../Module';
 import type ClassNode from '../nodes/shared/ClassNode';
 import { VariableKind } from '../nodes/shared/VariableKinds';
 import LocalVariable from '../variables/LocalVariable';
 import ThisVariable from '../variables/ThisVariable';
 import ChildScope from './ChildScope';
-import type Scope from './Scope';
 
 export default class ClassBodyScope extends ChildScope {
 	readonly instanceScope: ChildScope;
 	readonly thisVariable: LocalVariable;
 
-	constructor(parent: Scope, classNode: ClassNode, context: AstContext) {
+	constructor(parent: ChildScope, classNode: ClassNode) {
+		const { context } = parent;
 		super(parent, context);
 		this.variables.set(
 			'this',

diff --git a/src/ast/scopes/FunctionBodyScope.ts b/src/ast/scopes/FunctionBodyScope.ts
@@ -8,11 +8,8 @@ import ChildScope from './ChildScope';
 import type ParameterScope from './ParameterScope';
 
 export default class FunctionBodyScope extends ChildScope {
-	constructor(
-		readonly parent: ParameterScope,
-		readonly context: AstContext
-	) {
-		super(parent, context);
+	constructor(parent: ParameterScope) {
+		super(parent, parent.context);
 	}
 
 	// There is stuff that is only allowed in function scopes, i.e. functions can

diff --git a/src/ast/scopes/FunctionScope.ts b/src/ast/scopes/FunctionScope.ts
@@ -1,4 +1,3 @@
-import type { AstContext } from '../../Module';
 import type { InclusionContext } from '../ExecutionContext';
 import type SpreadElement from '../nodes/SpreadElement';
 import type { ExpressionEntity } from '../nodes/shared/Expression';
@@ -11,8 +10,9 @@ export default class FunctionScope extends ReturnValueScope {
 	readonly argumentsVariable: ArgumentsVariable;
 	readonly thisVariable: ThisVariable;
 
-	constructor(parent: ChildScope, context: AstContext) {
-		super(parent, context, false);
+	constructor(parent: ChildScope) {
+		const { context } = parent;
+		super(parent, false);
 		this.variables.set('arguments', (this.argumentsVariable = new ArgumentsVariable(context)));
 		this.variables.set('this', (this.thisVariable = new ThisVariable(context)));
 	}

diff --git a/src/ast/scopes/ParameterScope.ts b/src/ast/scopes/ParameterScope.ts
@@ -1,4 +1,3 @@
-import type { AstContext } from '../../Module';
 import { logDuplicateArgumentNameError } from '../../utils/logs';
 import type { InclusionContext } from '../ExecutionContext';
 import type Identifier from '../nodes/Identifier';
@@ -8,19 +7,16 @@ import ParameterVariable from '../variables/ParameterVariable';
 import CatchBodyScope from './CatchBodyScope';
 import ChildScope from './ChildScope';
 import FunctionBodyScope from './FunctionBodyScope';
-import type Scope from './Scope';
 
 export default class ParameterScope extends ChildScope {
 	readonly bodyScope: ChildScope;
 	parameters: readonly ParameterVariable[][] = [];
 
 	private hasRest = false;
 
-	constructor(parent: Scope, context: AstContext, isCatchScope: boolean) {
-		super(parent, context);
-		this.bodyScope = isCatchScope
-			? new CatchBodyScope(this, context)
-			: new FunctionBodyScope(this, context);
+	constructor(parent: ChildScope, isCatchScope: boolean) {
+		super(parent, parent.context);
+		this.bodyScope = isCatchScope ? new CatchBodyScope(this) : new FunctionBodyScope(this);
 	}
 
 	/**

diff --git a/test/form/samples/function-id-as-parameter/_config.js b/test/form/samples/function-id-as-parameter/_config.js
@@ -0,0 +1,3 @@
+module.exports = defineTest({
+	description: 'allows function expression parameters to shadow their id'
+});
diff --git a/test/form/samples/function-id-as-parameter/_expected.js b/test/form/samples/function-id-as-parameter/_expected.js
@@ -0,0 +1,5 @@
+const foo = function foo(foo) {
+	console.log(foo);
+};
+
+foo(42);
diff --git a/test/form/samples/function-id-as-parameter/main.js b/test/form/samples/function-id-as-parameter/main.js
@@ -0,0 +1,5 @@
+const foo = function foo(foo) {
+	console.log(foo);
+}
+
+foo(42);