Call includeNode for self or children nodes in includeDestructuredIfNecessary (#6090)

TrickyPi · web-flow · commit 7fb50f9b31e4 · 2025-09-07T07:47:16.000+02:00
* Add test

* Call includeNode for self or children nodes in includeDestructuredIfNecessary

* Replace includeDestructuredIfNecessary with a stub in MemberExpression
diff --git a/src/ast/nodes/ArrayPattern.ts b/src/ast/nodes/ArrayPattern.ts
@@ -86,24 +86,19 @@ export default class ArrayPattern extends NodeBase implements DeclarationPattern
 	): boolean {
 		let included = false;
 		const includedPatternPath = getIncludedPatternPath(destructuredInitPath);
-		for (const element of this.elements) {
+		for (const element of [...this.elements].reverse()) {
 			if (element) {
-				element.included ||= included;
+				if (included && !element.included) {
+					element.includeNode(context);
+				}
 				included =
 					element.includeDestructuredIfNecessary(context, includedPatternPath, init) || included;
 			}
 		}
-		if (included) {
-			// This is necessary so that if any pattern element is included, all are
-			// included for proper deconflicting
-			for (const element of this.elements) {
-				if (element && !element.included) {
-					element.included = true;
-					element.includeDestructuredIfNecessary(context, includedPatternPath, init);
-				}
-			}
+		if (!this.included && included) {
+			this.includeNode(context);
 		}
-		return (this.included ||= included);
+		return this.included;
 	}
 
 	markDeclarationReached(): void {
diff --git a/src/ast/nodes/AssignmentPattern.ts b/src/ast/nodes/AssignmentPattern.ts
@@ -71,13 +71,16 @@ export default class AssignmentPattern extends NodeBase implements DeclarationPa
 		if ((included ||= this.right.shouldBeIncluded(context))) {
 			this.right.include(context, false);
 			if (!this.left.included) {
-				this.left.included = true;
+				this.left.includeNode(context);
 				// Unfortunately, we need to include the left side again now, so that
 				// any declared variables are properly included.
 				this.left.includeDestructuredIfNecessary(context, destructuredInitPath, init);
 			}
 		}
-		return (this.included = included);
+		if (!this.included && included) {
+			this.includeNode(context);
+		}
+		return this.included;
 	}
 
 	includeNode(context: InclusionContext) {
diff --git a/src/ast/nodes/Identifier.ts b/src/ast/nodes/Identifier.ts
@@ -115,8 +115,9 @@ export default class Identifier extends IdentifierBase implements DeclarationPat
 		}
 		const { propertyReadSideEffects } = this.scope.context.options
 			.treeshake as NormalizedTreeshakingOptions;
+		let included = this.included;
 		if (
-			(this.included ||=
+			(included ||=
 				destructuredInitPath.length > 0 &&
 				!context.brokenFlow &&
 				propertyReadSideEffects &&
@@ -131,9 +132,11 @@ export default class Identifier extends IdentifierBase implements DeclarationPat
 				this.scope.context.includeVariableInModule(this.variable, EMPTY_PATH, context);
 			}
 			init.includePath(destructuredInitPath, context);
-			return true;
 		}
-		return false;
+		if (!this.included && included) {
+			this.includeNode(context);
+		}
+		return this.included;
 	}
 
 	markDeclarationReached(): void {
diff --git a/src/ast/nodes/MemberExpression.ts b/src/ast/nodes/MemberExpression.ts
@@ -7,7 +7,7 @@ import { logIllegalImportReassignment, logMissingExport } from '../../utils/logs
 import type { NodeRenderOptions, RenderOptions } from '../../utils/renderHelpers';
 import type { DeoptimizableEntity } from '../DeoptimizableEntity';
 import type { HasEffectsContext, InclusionContext } from '../ExecutionContext';
-import { createHasEffectsContext, createInclusionContext } from '../ExecutionContext';
+import { createInclusionContext } from '../ExecutionContext';
 import type {
 	NodeInteraction,
 	NodeInteractionAccessed,
@@ -440,25 +440,14 @@ export default class MemberExpression
 		}
 	}
 
-	includeDestructuredIfNecessary(
-		context: InclusionContext,
-		destructuredInitPath: ObjectPath,
-		init: ExpressionEntity
-	): boolean {
-		if (
-			(this.included ||=
-				destructuredInitPath.length > 0 &&
-				!context.brokenFlow &&
-				init.hasEffectsOnInteractionAtPath(
-					destructuredInitPath,
-					NODE_INTERACTION_UNKNOWN_ACCESS,
-					createHasEffectsContext()
-				))
-		) {
-			init.include(context, false);
-			return true;
-		}
-		return false;
+	includeDestructuredIfNecessary(): boolean {
+		/* istanbul ignore next */
+		this.scope.context.error(
+			{
+				message: 'includeDestructuredIfNecessary is currently not supported for MemberExpressions'
+			},
+			this.start
+		);
 	}
 
 	initialise(): void {
diff --git a/src/ast/nodes/ObjectPattern.ts b/src/ast/nodes/ObjectPattern.ts
@@ -87,25 +87,23 @@ export default class ObjectPattern extends NodeBase implements DeclarationPatter
 		destructuredInitPath: ObjectPath,
 		init: ExpressionEntity
 	): boolean {
-		if (!this.properties.length) return false;
+		if (!this.properties.length) return this.included;
 
 		const lastProperty = this.properties.at(-1)!;
-		const lastPropertyIncluded = lastProperty.includeDestructuredIfNecessary(
-			context,
-			destructuredInitPath,
-			init
-		);
+		let included = lastProperty.includeDestructuredIfNecessary(context, destructuredInitPath, init);
 		const lastPropertyIsRestElement = lastProperty.type === NodeType.RestElement;
 
-		let included = lastPropertyIsRestElement ? lastPropertyIncluded : false;
 		for (const property of this.properties.slice(0, -1)) {
-			if (lastPropertyIsRestElement && lastPropertyIncluded) {
+			if (lastPropertyIsRestElement && included && !property.included) {
 				property.includeNode(context);
 			}
 			included =
 				property.includeDestructuredIfNecessary(context, destructuredInitPath, init) || included;
 		}
-		return (this.included ||= included);
+		if (!this.included && included) {
+			this.includeNode(context);
+		}
+		return this.included;
 	}
 
 	markDeclarationReached(): void {
diff --git a/src/ast/nodes/Property.ts b/src/ast/nodes/Property.ts
@@ -84,13 +84,16 @@ export default class Property extends MethodBase implements DeclarationPatternNo
 		if ((included ||= this.key.hasEffects(createHasEffectsContext()))) {
 			this.key.include(context, false);
 			if (!this.value.included) {
-				this.value.included = true;
+				this.value.includeNode(context);
 				// Unfortunately, we need to include the value again now, so that any
 				// declared variables are properly included.
 				(this.value as PatternNode).includeDestructuredIfNecessary(context, path, init);
 			}
 		}
-		return (this.included = included);
+		if (!this.included && included) {
+			this.includeNode(context);
+		}
+		return this.included;
 	}
 
 	include(context: InclusionContext, includeChildrenRecursively: IncludeChildren) {
diff --git a/src/ast/nodes/RestElement.ts b/src/ast/nodes/RestElement.ts
@@ -73,12 +73,15 @@ export default class RestElement extends NodeBase implements DeclarationPatternN
 		destructuredInitPath: ObjectPath,
 		init: ExpressionEntity
 	): boolean {
-		return (this.included =
-			this.argument.includeDestructuredIfNecessary(
-				context,
-				getIncludedPatternPath(destructuredInitPath),
-				init
-			) || this.included);
+		const included = this.argument.includeDestructuredIfNecessary(
+			context,
+			getIncludedPatternPath(destructuredInitPath),
+			init
+		);
+		if (!this.included && included) {
+			this.includeNode(context);
+		}
+		return this.included;
 	}
 
 	include(context: InclusionContext, includeChildrenRecursively: IncludeChildren) {
diff --git a/test/function/samples/keep-the-non-first-assignment-in-destructuring-unknown-array-variables/_config.js b/test/function/samples/keep-the-non-first-assignment-in-destructuring-unknown-array-variables/_config.js
@@ -0,0 +1,6 @@
+module.exports = defineTest({
+	description: 'Keep the non-first assignment in destructuring unknown array variables',
+	context: {
+		unknownVariable: []
+	}
+});
diff --git a/test/function/samples/keep-the-non-first-assignment-in-destructuring-unknown-array-variables/main.js b/test/function/samples/keep-the-non-first-assignment-in-destructuring-unknown-array-variables/main.js
@@ -0,0 +1,2 @@
+const [a, last2 = { foo: true }] = unknownVariable;
+assert.ok(last2.foo);

Original file line number	Diff line number	Diff line change
`@@ -71,13 +71,16 @@ export default class AssignmentPattern extends NodeBase implements DeclarationPa`
`71`	`71`	`if ((included \|\|= this.right.shouldBeIncluded(context))) {`
`72`	`72`	`this.right.include(context, false);`
`73`	`73`	`if (!this.left.included) {`
`74`		`- this.left.included = true;`
	`74`	`+ this.left.includeNode(context);`
`75`	`75`	`// Unfortunately, we need to include the left side again now, so that`
`76`	`76`	`// any declared variables are properly included.`
`77`	`77`	`this.left.includeDestructuredIfNecessary(context, destructuredInitPath, init);`
`78`	`78`	`}`
`79`	`79`	`}`
`80`		`- return (this.included = included);`
	`80`	`+ if (!this.included && included) {`
	`81`	`+ this.includeNode(context);`
	`82`	`+ }`
	`83`	`+ return this.included;`
`81`	`84`	`}`
`82`	`85`
`83`	`86`	`includeNode(context: InclusionContext) {`
Original file line number	Diff line number	Diff line change
`@@ -115,8 +115,9 @@ export default class Identifier extends IdentifierBase implements DeclarationPat`
`115`	`115`	`}`
`116`	`116`	`const { propertyReadSideEffects } = this.scope.context.options`
`117`	`117`	`.treeshake as NormalizedTreeshakingOptions;`
	`118`	`+ let included = this.included;`
`118`	`119`	`if (`
`119`		`- (this.included \|\|=`
	`120`	`+ (included \|\|=`
`120`	`121`	`destructuredInitPath.length > 0 &&`
`121`	`122`	`!context.brokenFlow &&`
`122`	`123`	`propertyReadSideEffects &&`
`@@ -131,9 +132,11 @@ export default class Identifier extends IdentifierBase implements DeclarationPat`
`131`	`132`	`this.scope.context.includeVariableInModule(this.variable, EMPTY_PATH, context);`
`132`	`133`	`}`
`133`	`134`	`init.includePath(destructuredInitPath, context);`
`134`		`- return true;`
`135`	`135`	`}`
`136`		`- return false;`
	`136`	`+ if (!this.included && included) {`
	`137`	`+ this.includeNode(context);`
	`138`	`+ }`
	`139`	`+ return this.included;`
`137`	`140`	`}`
`138`	`141`
`139`	`142`	`markDeclarationReached(): void {`
Original file line number	Diff line number	Diff line change
`@@ -84,13 +84,16 @@ export default class Property extends MethodBase implements DeclarationPatternNo`
`84`	`84`	`if ((included \|\|= this.key.hasEffects(createHasEffectsContext()))) {`
`85`	`85`	`this.key.include(context, false);`
`86`	`86`	`if (!this.value.included) {`
`87`		`- this.value.included = true;`
	`87`	`+ this.value.includeNode(context);`
`88`	`88`	`// Unfortunately, we need to include the value again now, so that any`
`89`	`89`	`// declared variables are properly included.`
`90`	`90`	`(this.value as PatternNode).includeDestructuredIfNecessary(context, path, init);`
`91`	`91`	`}`
`92`	`92`	`}`
`93`		`- return (this.included = included);`
	`93`	`+ if (!this.included && included) {`
	`94`	`+ this.includeNode(context);`
	`95`	`+ }`
	`96`	`+ return this.included;`
`94`	`97`	`}`
`95`	`98`
`96`	`99`	`include(context: InclusionContext, includeChildrenRecursively: IncludeChildren) {`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+const [a, last2 = { foo: true }] = unknownVariable;`
	`2`	`+assert.ok(last2.foo);`