Do not create invalid code when simplifying object pattern assignments (

#4204) * Do not create invalid code when simplifying object pattern assignments * Use renderedSurroundingElement as sole reason for adding parentheses
rollup · Aug 10, 2021 · 4224666 · 4224666
1 parent 2a097a8
commit 4224666
Show file tree

Hide file tree

Showing 16 changed files with 65 additions and 59 deletions.
diff --git a/src/ast/nodes/AssignmentExpression.ts b/src/ast/nodes/AssignmentExpression.ts
@@ -17,6 +17,7 @@ import { EMPTY_PATH, ObjectPath, UNKNOWN_PATH } from '../utils/PathTracker';
 import Variable from '../variables/Variable';
 import Identifier from './Identifier';
 import * as NodeType from './NodeType';
+import ObjectPattern from './ObjectPattern';
 import { ExpressionNode, IncludeChildren, NodeBase } from './shared/Node';
 import { PatternNode } from './shared/Pattern';
 
@@ -88,7 +89,8 @@ export default class AssignmentExpression extends NodeBase {
 				removeLineBreaks(code, inclusionStart, this.right.start);
 			}
 			this.right.render(code, options, {
-				renderedParentType: renderedParentType || renderedSurroundingElement || this.parent.type
+				renderedParentType: renderedParentType || this.parent.type,
+				renderedSurroundingElement: renderedSurroundingElement || this.parent.type
 			});
 		}
 		if (options.format === 'system') {
@@ -108,6 +110,7 @@ export default class AssignmentExpression extends NodeBase {
 							options
 						);
 					}
+					return;
 				}
 			} else {
 				const systemPatternExports: Variable[] = [];
@@ -117,13 +120,23 @@ export default class AssignmentExpression extends NodeBase {
 						systemPatternExports,
 						this.start,
 						this.end,
-						(renderedParentType || renderedSurroundingElement) === NodeType.ExpressionStatement,
+						renderedSurroundingElement === NodeType.ExpressionStatement,
 						code,
 						options
 					);
+					return;
 				}
 			}
 		}
+		if (
+			this.left.included &&
+			this.left instanceof ObjectPattern &&
+			(renderedSurroundingElement === NodeType.ExpressionStatement ||
+				renderedSurroundingElement === NodeType.ArrowFunctionExpression)
+		) {
+			code.appendRight(this.start, '(');
+			code.prependLeft(this.end, ')');
+		}
 	}
 
 	protected applyDeoptimizations(): void {

diff --git a/src/ast/nodes/BinaryExpression.ts b/src/ast/nodes/BinaryExpression.ts
@@ -89,11 +89,9 @@ export default class BinaryExpression extends NodeBase implements DeoptimizableE
 	render(
 		code: MagicString,
 		options: RenderOptions,
-		{ renderedParentType, renderedSurroundingElement }: NodeRenderOptions = BLANK
+		{ renderedSurroundingElement }: NodeRenderOptions = BLANK
 	): void {
-		this.left.render(code, options, {
-			renderedSurroundingElement: renderedParentType || renderedSurroundingElement
-		});
+		this.left.render(code, options, { renderedSurroundingElement });
 		this.right.render(code, options);
 	}
 }
diff --git a/src/ast/nodes/CallExpression.ts b/src/ast/nodes/CallExpression.ts
@@ -246,12 +246,11 @@ export default class CallExpression extends NodeBase implements DeoptimizableEnt
 	render(
 		code: MagicString,
 		options: RenderOptions,
-		{ renderedParentType, renderedSurroundingElement }: NodeRenderOptions = BLANK
+		{ renderedSurroundingElement }: NodeRenderOptions = BLANK
 	): void {
-		const surroundingELement = renderedParentType || renderedSurroundingElement;
 		this.callee.render(code, options, {
 			isCalleeOfRenderedParent: true,
-			renderedSurroundingElement: surroundingELement
+			renderedSurroundingElement
 		});
 		if (this.arguments.length > 0) {
 			if (this.arguments[this.arguments.length - 1].included) {

diff --git a/src/ast/nodes/ClassExpression.ts b/src/ast/nodes/ClassExpression.ts
@@ -10,11 +10,10 @@ export default class ClassExpression extends ClassNode {
 	render(
 		code: MagicString,
 		options: RenderOptions,
-		{ renderedParentType, renderedSurroundingElement }: NodeRenderOptions = BLANK
+		{ renderedSurroundingElement }: NodeRenderOptions = BLANK
 	): void {
 		super.render(code, options);
-		const surroundingElement = renderedParentType || renderedSurroundingElement;
-		if (surroundingElement === NodeType.ExpressionStatement) {
+		if (renderedSurroundingElement === NodeType.ExpressionStatement) {
 			code.appendRight(this.start, '(');
 			code.prependLeft(this.end, ')');
 		}

diff --git a/src/ast/nodes/ConditionalExpression.ts b/src/ast/nodes/ConditionalExpression.ts
@@ -206,14 +206,11 @@ export default class ConditionalExpression extends NodeBase implements Deoptimiz
 			usedBranch!.render(code, options, {
 				isCalleeOfRenderedParent,
 				preventASI: true,
-				...(renderedSurroundingElement
-					? { renderedSurroundingElement }
-					: { renderedParentType: renderedParentType || this.parent.type })
+				renderedParentType: renderedParentType || this.parent.type,
+				renderedSurroundingElement: renderedSurroundingElement || this.parent.type
 			});
 		} else {
-			this.test.render(code, options, {
-				renderedSurroundingElement: renderedParentType || renderedSurroundingElement
-			});
+			this.test.render(code, options, { renderedSurroundingElement });
 			this.consequent.render(code, options);
 			this.alternate.render(code, options);
 		}

diff --git a/src/ast/nodes/ExportDefaultDeclaration.ts b/src/ast/nodes/ExportDefaultDeclaration.ts
@@ -99,8 +99,7 @@ export default class ExportDefaultDeclaration extends NodeBase {
 		} else {
 			code.remove(this.start, declarationStart);
 			this.declaration.render(code, options, {
-				isCalleeOfRenderedParent: false,
-				renderedParentType: NodeType.ExpressionStatement
+				renderedSurroundingElement: NodeType.ExpressionStatement
 			});
 			if (code.original[this.end - 1] !== ';') {
 				code.appendLeft(this.end, ';');

diff --git a/src/ast/nodes/FunctionExpression.ts b/src/ast/nodes/FunctionExpression.ts
@@ -10,11 +10,10 @@ export default class FunctionExpression extends FunctionNode {
 	render(
 		code: MagicString,
 		options: RenderOptions,
-		{ renderedParentType, renderedSurroundingElement }: NodeRenderOptions = BLANK
+		{ renderedSurroundingElement }: NodeRenderOptions = BLANK
 	): void {
 		super.render(code, options);
-		const surroundingElement = renderedParentType || renderedSurroundingElement;
-		if (surroundingElement === NodeType.ExpressionStatement) {
+		if (renderedSurroundingElement === NodeType.ExpressionStatement) {
 			code.appendRight(this.start, '(');
 			code.prependLeft(this.end, ')');
 		}

diff --git a/src/ast/nodes/LogicalExpression.ts b/src/ast/nodes/LogicalExpression.ts
@@ -191,14 +191,13 @@ export default class LogicalExpression extends NodeBase implements Deoptimizable
 			this.getUsedBranch()!.render(code, options, {
 				isCalleeOfRenderedParent,
 				preventASI,
-				...(renderedSurroundingElement
-					? { renderedSurroundingElement }
-					: { renderedParentType: renderedParentType || this.parent.type })
+				renderedParentType: renderedParentType || this.parent.type,
+				renderedSurroundingElement: renderedSurroundingElement || this.parent.type
 			});
 		} else {
 			this.left.render(code, options, {
 				preventASI,
-				renderedSurroundingElement: renderedParentType || renderedSurroundingElement
+				renderedSurroundingElement
 			});
 			this.right.render(code, options);
 		}

diff --git a/src/ast/nodes/MemberExpression.ts b/src/ast/nodes/MemberExpression.ts
@@ -316,12 +316,7 @@ export default class MemberExpression extends NodeBase implements DeoptimizableE
 			if (renderedParentType && isCalleeOfRenderedParent) {
 				code.appendRight(this.start, '0, ');
 			}
-			const surroundingElement = renderedParentType || renderedSurroundingElement;
-			this.object.render(
-				code,
-				options,
-				surroundingElement ? { renderedSurroundingElement: surroundingElement } : BLANK
-			);
+			this.object.render(code, options, { renderedSurroundingElement });
 			this.property.render(code, options);
 		}
 	}

diff --git a/src/ast/nodes/ObjectExpression.ts b/src/ast/nodes/ObjectExpression.ts
@@ -90,13 +90,12 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE
 	render(
 		code: MagicString,
 		options: RenderOptions,
-		{ renderedParentType, renderedSurroundingElement }: NodeRenderOptions = BLANK
+		{ renderedSurroundingElement }: NodeRenderOptions = BLANK
 	): void {
 		super.render(code, options);
-		const surroundingElement = renderedParentType || renderedSurroundingElement;
 		if (
-			surroundingElement === NodeType.ExpressionStatement ||
-			surroundingElement === NodeType.ArrowFunctionExpression
+			renderedSurroundingElement === NodeType.ExpressionStatement ||
+			renderedSurroundingElement === NodeType.ArrowFunctionExpression
 		) {
 			code.appendRight(this.start, '(');
 			code.prependLeft(this.end, ')');

diff --git a/src/ast/nodes/SequenceExpression.ts b/src/ast/nodes/SequenceExpression.ts
@@ -121,16 +121,12 @@ export default class SequenceExpression extends NodeBase {
 				removeLineBreaks(code, start, node.start);
 			}
 			if (includedNodes === 1) {
-				if (node === lastNode) {
-					node.render(code, options, {
-						isCalleeOfRenderedParent,
-						renderedParentType: renderedParentType || this.parent.type
-					});
-				} else {
-					node.render(code, options, {
-						renderedSurroundingElement: renderedParentType || this.parent.type
-					});
-				}
+				const parentType = renderedParentType || this.parent.type;
+				node.render(code, options, {
+					isCalleeOfRenderedParent: isCalleeOfRenderedParent && node === lastNode,
+					renderedParentType: parentType,
+					renderedSurroundingElement: parentType
+				});
 			} else {
 				node.render(code, options);
 			}

diff --git a/src/ast/nodes/VariableDeclarator.ts b/src/ast/nodes/VariableDeclarator.ts
@@ -56,7 +56,7 @@ export default class VariableDeclarator extends NodeBase {
 			this.init.render(
 				code,
 				options,
-				renderId ? BLANK : { renderedParentType: NodeType.ExpressionStatement }
+				renderId ? BLANK : { renderedSurroundingElement: NodeType.ExpressionStatement }
 			);
 		} else if (
 			this.id instanceof Identifier &&

diff --git a/src/utils/renderHelpers.ts b/src/utils/renderHelpers.ts
@@ -23,8 +23,12 @@ export interface NodeRenderOptions {
 	isNoStatement?: boolean;
 	isShorthandProperty?: boolean;
 	preventASI?: boolean;
-	renderedParentType?: string; // also serves as a flag if the rendered parent is different from the actual parent
-	renderedSurroundingElement?: string; // same as parent type, but for changed non-direct parents that directly precede elements
+	/* Indicates if the direct parent of an element changed.
+	Necessary for determining the "this" context of callees. */
+	renderedParentType?: string;
+	/* Indicates if the parent or ancestor surrounding an element has changed and what it changed to.
+	Necessary for adding parentheses. */
+	renderedSurroundingElement?: string;
 	start?: number;
 }
 

diff --git a/test/function/samples/function-expressions-simplified-to-statement/main.js b/test/function/samples/function-expressions-simplified-to-statement/main.js
@@ -9,17 +9,17 @@ true ? function foo(x){
 	value = x;
 }("consequent") : 2;
 
-assert.equal(value, 'consequent');
+assert.strictEqual(value, 'consequent');
 
 foo("incorrect");
 
-assert.equal(value, 'foo');
+assert.strictEqual(value, 'foo');
 
 false ? null: function foo(x){
 	value = x;
 }("alternate");
 
-assert.equal(value, 'alternate');
+assert.strictEqual(value, 'alternate');
 
 // logical expression
 function bar(){
@@ -30,17 +30,17 @@ true && function bar(x){
 	value = x;
 }("and");
 
-assert.equal(value, 'and');
+assert.strictEqual(value, 'and');
 
 bar("incorrect");
 
-assert.equal(value, 'bar');
+assert.strictEqual(value, 'bar');
 
 false || function bar(x){
 	value = x;
 }("or");
 
-assert.equal(value, 'or');
+assert.strictEqual(value, 'or');
 
 // sequence expression
 function baz(){
@@ -50,9 +50,8 @@ function baz(){
 0, function baz(x){
 	value = x;
 }("comma");
-
-assert.equal(value, 'comma');
+assert.strictEqual(value, 'comma');
 
 baz("incorrect");
 
-assert.equal(value, 'baz');
+assert.strictEqual(value, 'baz');
diff --git a/test/function/samples/simplify-with-destructuring/_config.js b/test/function/samples/simplify-with-destructuring/_config.js
@@ -0,0 +1,3 @@
+module.exports = {
+	description: 'handle destructuring assignments in simplified sequence expressions'
+};
diff --git a/test/function/samples/simplify-with-destructuring/main.js b/test/function/samples/simplify-with-destructuring/main.js
@@ -0,0 +1,7 @@
+let foo, unused;
+null, { foo } = { foo: 'bar' };
+assert.strictEqual(foo, 'bar');
+
+const assign = () => unused = { foo } = { foo: 'baz' };
+assign();
+assert.strictEqual(foo, 'baz');