Fix prop inclusion with try-catch-deoptimization (#5842)

lukastaegert · web-flow · commit 15a6f73006c6 · 2025-02-14T10:28:42.000+01:00
* Test that argument inclusion works correctly when using try-catch-deopt

* Refine call argument inclusion

- When call arguments are included, do not call "include" on the "this" argument
  again, as this will already be included at the call site. This allows to make
  the inclusion logic more straightforward
- create shared helper to do that
- if a call/new/tagged template expression is called with recursive inclusion,
  do not specifically include the arguments. Instead, if an identifier is
  included recursively, then include an unknown path of the variable.

* Refine inclusion and deoptimization logic for reassigned member expressions
diff --git a/src/ast/nodes/BreakStatement.ts b/src/ast/nodes/BreakStatement.ts
@@ -1,7 +1,12 @@
 import { type HasEffectsContext, type InclusionContext } from '../ExecutionContext';
 import type Identifier from './Identifier';
 import type * as NodeType from './NodeType';
-import { doNotDeoptimize, onlyIncludeSelfNoDeoptimize, StatementBase } from './shared/Node';
+import {
+	doNotDeoptimize,
+	type IncludeChildren,
+	onlyIncludeSelfNoDeoptimize,
+	StatementBase
+} from './shared/Node';
 
 export default class BreakStatement extends StatementBase {
 	declare label: Identifier | null;
@@ -19,10 +24,10 @@ export default class BreakStatement extends StatementBase {
 		return false;
 	}
 
-	include(context: InclusionContext): void {
+	include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {
 		this.included = true;
 		if (this.label) {
-			this.label.include(context);
+			this.label.include(context, includeChildrenRecursively);
 			context.includedLabels.add(this.label.name);
 		} else {
 			context.hasBreak = true;
diff --git a/src/ast/nodes/CallExpression.ts b/src/ast/nodes/CallExpression.ts
@@ -114,11 +114,7 @@ export default class CallExpression
 	include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {
 		if (!this.included) this.includeNode(context);
 		if (includeChildrenRecursively) {
-			this.callee.include(context, true);
-			for (const argument of this.arguments) {
-				argument.includePath(UNKNOWN_PATH, context);
-				argument.include(context, true);
-			}
+			super.include(context, true);
 			if (
 				includeChildrenRecursively === INCLUDE_PARAMETERS &&
 				this.callee instanceof Identifier &&
@@ -127,16 +123,8 @@ export default class CallExpression
 				this.callee.variable.markCalledFromTryStatement();
 			}
 		} else {
-			// If the callee is a member expression and does not have a variable, its
-			// object will already be included via the first argument of the
-			// interaction in includeCallArguments. Including it again can lead to
-			// severe performance problems.
-			if (this.callee instanceof MemberExpression && !this.callee.variable) {
-				this.callee.property.include(context, false);
-			} else {
-				this.callee.include(context, false);
-			}
-			this.callee.includeCallArguments(context, this.interaction);
+			this.callee.include(context, false);
+			this.callee.includeCallArguments(this.interaction, context);
 		}
 	}
 
diff --git a/src/ast/nodes/ConditionalExpression.ts b/src/ast/nodes/ConditionalExpression.ts
@@ -164,13 +164,13 @@ export default class ConditionalExpression extends NodeBase implements Deoptimiz
 		}
 	}
 
-	includeCallArguments(context: InclusionContext, interaction: NodeInteractionCalled): void {
+	includeCallArguments(interaction: NodeInteractionCalled, context: InclusionContext): void {
 		const usedBranch = this.getUsedBranch();
 		if (usedBranch) {
-			usedBranch.includeCallArguments(context, interaction);
+			usedBranch.includeCallArguments(interaction, context);
 		} else {
-			this.consequent.includeCallArguments(context, interaction);
-			this.alternate.includeCallArguments(context, interaction);
+			this.consequent.includeCallArguments(interaction, context);
+			this.alternate.includeCallArguments(interaction, context);
 		}
 	}
 
diff --git a/src/ast/nodes/ContinueStatement.ts b/src/ast/nodes/ContinueStatement.ts
@@ -1,7 +1,12 @@
 import { type HasEffectsContext, type InclusionContext } from '../ExecutionContext';
 import type Identifier from './Identifier';
 import type * as NodeType from './NodeType';
-import { doNotDeoptimize, onlyIncludeSelfNoDeoptimize, StatementBase } from './shared/Node';
+import {
+	doNotDeoptimize,
+	type IncludeChildren,
+	onlyIncludeSelfNoDeoptimize,
+	StatementBase
+} from './shared/Node';
 
 export default class ContinueStatement extends StatementBase {
 	declare label: Identifier | null;
@@ -19,10 +24,10 @@ export default class ContinueStatement extends StatementBase {
 		return false;
 	}
 
-	include(context: InclusionContext): void {
+	include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {
 		this.included = true;
 		if (this.label) {
-			this.label.include(context);
+			this.label.include(context, includeChildrenRecursively);
 			context.includedLabels.add(this.label.name);
 		} else {
 			context.hasContinue = true;
diff --git a/src/ast/nodes/LabeledStatement.ts b/src/ast/nodes/LabeledStatement.ts
@@ -44,7 +44,7 @@ export default class LabeledStatement extends StatementBase {
 		context.includedLabels = new Set<string>();
 		this.body.include(context, includeChildrenRecursively);
 		if (includeChildrenRecursively || context.includedLabels.has(this.label.name)) {
-			this.label.include(context);
+			this.label.include(context, includeChildrenRecursively);
 			context.includedLabels.delete(this.label.name);
 			context.brokenFlow = brokenFlow;
 		}
diff --git a/src/ast/nodes/MemberExpression.ts b/src/ast/nodes/MemberExpression.ts
@@ -44,6 +44,7 @@ import { getChainElementLiteralValueAtPath } from './shared/chainElements';
 import {
 	deoptimizeInteraction,
 	type ExpressionEntity,
+	includeInteraction,
 	type LiteralValueOrUnknown,
 	UNKNOWN_RETURN_EXPRESSION,
 	UnknownValue
@@ -375,6 +376,16 @@ export default class MemberExpression
 		}
 	}
 
+	includeNodeAsAssignmentTarget(context: InclusionContext) {
+		this.included = true;
+		if (!this.assignmentDeoptimized) this.applyAssignmentDeoptimization();
+		if (this.variable) {
+			this.scope.context.includeVariableInModule(this.variable, EMPTY_PATH, context);
+		} else if (!this.isUndefined) {
+			this.object.includePath([this.propertyKey], context);
+		}
+	}
+
 	includePath(path: ObjectPath, context: InclusionContext): void {
 		if (!this.included) this.includeNode(context);
 		if (this.variable) {
@@ -397,21 +408,17 @@ export default class MemberExpression
 		includeChildrenRecursively: IncludeChildren,
 		deoptimizeAccess: boolean
 	): void {
-		if (!this.assignmentDeoptimized) this.applyAssignmentDeoptimization();
-		if (deoptimizeAccess) {
-			this.include(context, includeChildrenRecursively);
-		} else {
-			if (!this.included) this.includeNode(context);
-			this.object.include(context, includeChildrenRecursively);
-			this.property.include(context, includeChildrenRecursively);
-		}
+		if (!this.included) this.includeNodeAsAssignmentTarget(context);
+		if (deoptimizeAccess && !this.deoptimized) this.applyDeoptimizations();
+		this.object.include(context, includeChildrenRecursively);
+		this.property.include(context, includeChildrenRecursively);
 	}
 
-	includeCallArguments(context: InclusionContext, interaction: NodeInteractionCalled): void {
+	includeCallArguments(interaction: NodeInteractionCalled, context: InclusionContext): void {
 		if (this.variable) {
-			this.variable.includeCallArguments(context, interaction);
+			this.variable.includeCallArguments(interaction, context);
 		} else {
-			super.includeCallArguments(context, interaction);
+			includeInteraction(interaction, context);
 		}
 	}
 
diff --git a/src/ast/nodes/NewExpression.ts b/src/ast/nodes/NewExpression.ts
@@ -42,13 +42,13 @@ export default class NewExpression extends NodeBase {
 	}
 
 	include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {
+		if (!this.included) this.includeNode(context);
 		if (includeChildrenRecursively) {
-			super.include(context, includeChildrenRecursively);
+			super.include(context, true);
 		} else {
-			if (!this.included) this.includeNode(context);
 			this.callee.include(context, false);
+			this.callee.includeCallArguments(this.interaction, context);
 		}
-		this.callee.includeCallArguments(context, this.interaction);
 	}
 
 	includeNode(context: InclusionContext) {
diff --git a/src/ast/nodes/TaggedTemplateExpression.ts b/src/ast/nodes/TaggedTemplateExpression.ts
@@ -48,16 +48,11 @@ export default class TaggedTemplateExpression extends CallExpressionBase {
 	include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {
 		if (!this.included) this.includeNode(context);
 		if (includeChildrenRecursively) {
-			super.include(context, includeChildrenRecursively);
+			super.include(context, true);
 		} else {
-			this.included = true;
-			this.tag.include(context, includeChildrenRecursively);
-			this.quasi.include(context, includeChildrenRecursively);
-		}
-		this.tag.includeCallArguments(context, this.interaction);
-		const [returnExpression] = this.getReturnExpression();
-		if (!returnExpression.included) {
-			returnExpression.include(context, false);
+			this.quasi.include(context, false);
+			this.tag.include(context, false);
+			this.tag.includeCallArguments(this.interaction, context);
 		}
 	}
 
diff --git a/src/ast/nodes/shared/ClassNode.ts b/src/ast/nodes/shared/ClassNode.ts
@@ -106,7 +106,7 @@ export default class ClassNode extends NodeBase implements DeoptimizableEntity {
 		for (const decorator of this.decorators) decorator.include(context, includeChildrenRecursively);
 		if (this.id) {
 			this.id.markDeclarationReached();
-			this.id.include(context);
+			this.id.include(context, includeChildrenRecursively);
 		}
 	}
 
diff --git a/src/ast/nodes/shared/Expression.ts b/src/ast/nodes/shared/Expression.ts
@@ -97,13 +97,8 @@ export class ExpressionEntity implements WritableEntity {
 	 * paths of the expression are included.
 	 * */
 
-	includeCallArguments(context: InclusionContext, interaction: NodeInteractionCalled): void {
-		for (const argument of interaction.args) {
-			if (argument) {
-				argument.includePath(UNKNOWN_PATH, context);
-				argument.include(context, false);
-			}
-		}
+	includeCallArguments(interaction: NodeInteractionCalled, context: InclusionContext): void {
+		includeInteraction(interaction, context);
 	}
 
 	shouldBeIncluded(_context: InclusionContext): boolean {
@@ -124,3 +119,16 @@ export const deoptimizeInteraction = (interaction: NodeInteraction) => {
 		argument?.deoptimizePath(UNKNOWN_PATH);
 	}
 };
+
+export const includeInteraction = ({ args }: NodeInteraction, context: InclusionContext) => {
+	// We do not re-include the "this" argument as we expect this is already
+	// re-included at the call site
+	args[0]?.includePath(UNKNOWN_PATH, context);
+	for (let argumentIndex = 1; argumentIndex < args.length; argumentIndex++) {
+		const argument = args[argumentIndex];
+		if (argument) {
+			argument.includePath(UNKNOWN_PATH, context);
+			argument.include(context, false);
+		}
+	}
+};
diff --git a/src/ast/nodes/shared/FunctionNode.ts b/src/ast/nodes/shared/FunctionNode.ts
@@ -32,7 +32,7 @@ export default class FunctionNode extends FunctionBase {
 		this.constructedEntity = new ObjectEntity(Object.create(null), OBJECT_PROTOTYPE);
 		// This makes sure that all deoptimizations of "this" are applied to the
 		// constructed entity.
-		this.scope.thisVariable.addArgumentValue(this.constructedEntity);
+		this.scope.thisVariable.addArgumentForDeoptimization(this.constructedEntity);
 	}
 
 	deoptimizeArgumentsOnInteractionAtPath(
@@ -43,7 +43,7 @@ export default class FunctionNode extends FunctionBase {
 		super.deoptimizeArgumentsOnInteractionAtPath(interaction, path, recursionTracker);
 		if (interaction.type === INTERACTION_CALLED && path.length === 0 && interaction.args[0]) {
 			// args[0] is the "this" argument
-			this.scope.thisVariable.addArgumentValue(interaction.args[0]);
+			this.scope.thisVariable.addArgumentForDeoptimization(interaction.args[0]);
 		}
 	}
 
@@ -102,7 +102,7 @@ export default class FunctionNode extends FunctionBase {
 
 	include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {
 		super.include(context, includeChildrenRecursively);
-		this.id?.include(context);
+		this.id?.include(context, includeChildrenRecursively);
 		const hasArguments = this.scope.argumentsVariable.included;
 		for (const parameter of this.params) {
 			if (!(parameter instanceof Identifier) || hasArguments) {
diff --git a/src/ast/nodes/shared/IdentifierBase.ts b/src/ast/nodes/shared/IdentifierBase.ts
@@ -12,14 +12,14 @@ import {
 	NODE_INTERACTION_UNKNOWN_ACCESS
 } from '../../NodeInteractions';
 import type { EntityPathTracker, ObjectPath } from '../../utils/PathTracker';
-import { EMPTY_PATH } from '../../utils/PathTracker';
+import { EMPTY_PATH, UNKNOWN_PATH } from '../../utils/PathTracker';
 import GlobalVariable from '../../variables/GlobalVariable';
 import LocalVariable from '../../variables/LocalVariable';
 import type Variable from '../../variables/Variable';
 import { Flag, isFlagSet, setFlag } from './BitFlags';
 import type { ExpressionEntity, LiteralValueOrUnknown } from './Expression';
 import { UNKNOWN_EXPRESSION } from './Expression';
-import { NodeBase } from './Node';
+import { type IncludeChildren, NodeBase } from './Node';
 
 const tdzVariableKinds = new Set(['class', 'const', 'let', 'var', 'using', 'await using']);
 
@@ -127,8 +127,11 @@ export default class IdentifierBase extends NodeBase {
 		}
 	}
 
-	include(context: InclusionContext): void {
+	include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {
 		if (!this.included) this.includeNode(context);
+		if (includeChildrenRecursively) {
+			this.variable?.includePath(UNKNOWN_PATH, context);
+		}
 	}
 
 	includeNode(context: InclusionContext) {
@@ -151,8 +154,8 @@ export default class IdentifierBase extends NodeBase {
 		}
 	}
 
-	includeCallArguments(context: InclusionContext, interaction: NodeInteractionCalled): void {
-		this.variable!.includeCallArguments(context, interaction);
+	includeCallArguments(interaction: NodeInteractionCalled, context: InclusionContext): void {
+		this.variable!.includeCallArguments(interaction, context);
 	}
 
 	isPossibleTDZ(): boolean {
diff --git a/src/ast/nodes/shared/MethodTypes.ts b/src/ast/nodes/shared/MethodTypes.ts
@@ -70,15 +70,13 @@ export class Method extends ExpressionEntity {
 
 	hasEffectsOnInteractionAtPath(
 		path: ObjectPath,
-		interaction: NodeInteraction,
+		{ args, type }: NodeInteraction,
 		context: HasEffectsContext
 	): boolean {
-		const { type } = interaction;
 		if (path.length > (type === INTERACTION_ACCESSED ? 1 : 0)) {
 			return true;
 		}
 		if (type === INTERACTION_CALLED) {
-			const { args } = interaction;
 			if (
 				this.description.mutatesSelfAsArray === true &&
 				args[0]?.hasEffectsOnInteractionAtPath(
diff --git a/src/ast/scopes/FunctionScope.ts b/src/ast/scopes/FunctionScope.ts
@@ -26,8 +26,8 @@ export default class FunctionScope extends ReturnValueScope {
 		return this;
 	}
 
-	includeCallArguments(context: InclusionContext, interaction: NodeInteractionCalled): void {
-		super.includeCallArguments(context, interaction);
+	includeCallArguments(interaction: NodeInteractionCalled, context: InclusionContext): void {
+		super.includeCallArguments(interaction, context);
 		if (this.argumentsVariable.included) {
 			const { args } = interaction;
 			for (let argumentIndex = 1; argumentIndex < args.length; argumentIndex++) {
diff --git a/src/ast/scopes/ParameterScope.ts b/src/ast/scopes/ParameterScope.ts
@@ -51,11 +51,10 @@ export default class ParameterScope extends ChildScope {
 		this.hasRest = hasRest;
 	}
 
-	includeCallArguments(context: InclusionContext, interaction: NodeInteractionCalled): void {
+	includeCallArguments({ args }: NodeInteractionCalled, context: InclusionContext): void {
 		let calledFromTryStatement = false;
 		let argumentIncluded = false;
 		const restParameter = this.hasRest && this.parameters[this.parameters.length - 1];
-		const { args } = interaction;
 		let lastExplicitlyIncludedIndex = args.length - 1;
 		// If there is a SpreadElement, we need to include all arguments after it
 		// because we no longer know which argument corresponds to which parameter.
diff --git a/src/ast/scopes/ReturnValueScope.ts b/src/ast/scopes/ReturnValueScope.ts
@@ -13,9 +13,8 @@ export default class ReturnValueScope extends ParameterScope {
 		this.returnExpressions.push(expression);
 	}
 
-	deoptimizeArgumentsOnCall(interaction: NodeInteractionCalled): void {
+	deoptimizeArgumentsOnCall({ args }: NodeInteractionCalled): void {
 		const { parameters } = this;
-		const { args } = interaction;
 		let position = 0;
 		for (; position < args.length - 1; position++) {
 			// Only the "this" argument arg[0] can be null
@@ -24,7 +23,9 @@ export default class ReturnValueScope extends ParameterScope {
 				// This deoptimizes the current and remaining parameters and arguments
 				for (; position < parameters.length; position++) {
 					args[position + 1]?.deoptimizePath(UNKNOWN_PATH);
-					parameters[position].forEach(variable => variable.markReassigned());
+					for (const variable of parameters[position]) {
+						variable.markReassigned();
+					}
 				}
 				break;
 			}
@@ -34,7 +35,7 @@ export default class ReturnValueScope extends ParameterScope {
 				const variables = parameters[position];
 				if (variables) {
 					for (const variable of variables) {
-						variable.addArgumentValue(argument);
+						variable.addArgumentForDeoptimization(argument);
 					}
 				}
 				this.addArgumentToBeDeoptimized(argument);
@@ -43,7 +44,7 @@ export default class ReturnValueScope extends ParameterScope {
 		const nonRestParameterLength = this.hasRest ? parameters.length - 1 : parameters.length;
 		for (; position < nonRestParameterLength; position++) {
 			for (const variable of parameters[position]) {
-				variable.addArgumentValue(UNDEFINED_EXPRESSION);
+				variable.addArgumentForDeoptimization(UNDEFINED_EXPRESSION);
 			}
 		}
 	}
diff --git a/src/ast/variables/LocalVariable.ts b/src/ast/variables/LocalVariable.ts
diff --git a/src/ast/variables/ParameterVariable.ts b/src/ast/variables/ParameterVariable.ts
diff --git a/test/function/samples/object-expression-treeshaking/try-statement-return-properties/_config.js b/test/function/samples/object-expression-treeshaking/try-statement-return-properties/_config.js
diff --git a/test/function/samples/object-expression-treeshaking/try-statement-return-properties/main.js b/test/function/samples/object-expression-treeshaking/try-statement-return-properties/main.js

Original file line number	Diff line number	Diff line change
`@@ -164,13 +164,13 @@ export default class ConditionalExpression extends NodeBase implements Deoptimiz`
`164`	`164`	`}`
`165`	`165`	`}`
`166`	`166`
`167`		`- includeCallArguments(context: InclusionContext, interaction: NodeInteractionCalled): void {`
	`167`	`+ includeCallArguments(interaction: NodeInteractionCalled, context: InclusionContext): void {`
`168`	`168`	`const usedBranch = this.getUsedBranch();`
`169`	`169`	`if (usedBranch) {`
`170`		`- usedBranch.includeCallArguments(context, interaction);`
	`170`	`+ usedBranch.includeCallArguments(interaction, context);`
`171`	`171`	`} else {`
`172`		`- this.consequent.includeCallArguments(context, interaction);`
`173`		`- this.alternate.includeCallArguments(context, interaction);`
	`172`	`+ this.consequent.includeCallArguments(interaction, context);`
	`173`	`+ this.alternate.includeCallArguments(interaction, context);`
`174`	`174`	`}`
`175`	`175`	`}`
`176`	`176`
Original file line number	Diff line number	Diff line change
`@@ -44,7 +44,7 @@ export default class LabeledStatement extends StatementBase {`
`44`	`44`	`context.includedLabels = new Set<string>();`
`45`	`45`	`this.body.include(context, includeChildrenRecursively);`
`46`	`46`	`if (includeChildrenRecursively \|\| context.includedLabels.has(this.label.name)) {`
`47`		`- this.label.include(context);`
	`47`	`+ this.label.include(context, includeChildrenRecursively);`
`48`	`48`	`context.includedLabels.delete(this.label.name);`
`49`	`49`	`context.brokenFlow = brokenFlow;`
`50`	`50`	`}`
Original file line number	Diff line number	Diff line change
`@@ -42,13 +42,13 @@ export default class NewExpression extends NodeBase {`
`42`	`42`	`}`
`43`	`43`
`44`	`44`	`include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {`
	`45`	`+ if (!this.included) this.includeNode(context);`
`45`	`46`	`if (includeChildrenRecursively) {`
`46`		`- super.include(context, includeChildrenRecursively);`
	`47`	`+ super.include(context, true);`
`47`	`48`	`} else {`
`48`		`- if (!this.included) this.includeNode(context);`
`49`	`49`	`this.callee.include(context, false);`
	`50`	`+ this.callee.includeCallArguments(this.interaction, context);`
`50`	`51`	`}`
`51`		`- this.callee.includeCallArguments(context, this.interaction);`
`52`	`52`	`}`
`53`	`53`
`54`	`54`	`includeNode(context: InclusionContext) {`
Original file line number	Diff line number	Diff line change
`@@ -106,7 +106,7 @@ export default class ClassNode extends NodeBase implements DeoptimizableEntity {`
`106`	`106`	`for (const decorator of this.decorators) decorator.include(context, includeChildrenRecursively);`
`107`	`107`	`if (this.id) {`
`108`	`108`	`this.id.markDeclarationReached();`
`109`		`- this.id.include(context);`
	`109`	`+ this.id.include(context, includeChildrenRecursively);`
`110`	`110`	`}`
`111`	`111`	`}`
`112`	`112`