Invalid sequence expression simplification (#4110)

* Add parentheses if necessary around sequence expression leading elements * Use correct this for tagged templates in simplified sequence expressions * Improve coverage
rollup · May 28, 2021 · ecb5b16 · ecb5b16
1 parent 700b2e0
commit ecb5b16
Show file tree

Hide file tree

Showing 11 changed files with 103 additions and 55 deletions.
diff --git a/src/ast/nodes/CallExpression.ts b/src/ast/nodes/CallExpression.ts
@@ -247,11 +247,10 @@ export default class CallExpression extends NodeBase implements DeoptimizableEnt
 		{ renderedParentType, renderedSurroundingElement }: NodeRenderOptions = BLANK
 	): void {
 		const surroundingELement = renderedParentType || renderedSurroundingElement;
-		this.callee.render(
-			code,
-			options,
-			surroundingELement ? { renderedSurroundingElement: surroundingELement } : BLANK
-		);
+		this.callee.render(code, options, {
+			isCalleeOfRenderedParent: true,
+			renderedSurroundingElement: surroundingELement
+		});
 		if (this.arguments.length > 0) {
 			if (this.arguments[this.arguments.length - 1].included) {
 				for (const arg of this.arguments) {

diff --git a/src/ast/nodes/ConditionalExpression.ts b/src/ast/nodes/ConditionalExpression.ts
@@ -19,7 +19,6 @@ import {
 	SHARED_RECURSION_TRACKER,
 	UNKNOWN_PATH
 } from '../utils/PathTracker';
-import CallExpression from './CallExpression';
 import * as NodeType from './NodeType';
 import SpreadElement from './SpreadElement';
 import { ExpressionEntity, LiteralValueOrUnknown, UnknownValue } from './shared/Expression';
@@ -180,7 +179,12 @@ export default class ConditionalExpression extends NodeBase implements Deoptimiz
 	render(
 		code: MagicString,
 		options: RenderOptions,
-		{ isCalleeOfRenderedParent, renderedParentType, preventASI }: NodeRenderOptions = BLANK
+		{
+			isCalleeOfRenderedParent,
+			preventASI,
+			renderedParentType,
+			renderedSurroundingElement
+		}: NodeRenderOptions = BLANK
 	): void {
 		const usedBranch = this.getUsedBranch();
 		if (!this.test.included) {
@@ -200,15 +204,15 @@ export default class ConditionalExpression extends NodeBase implements Deoptimiz
 			}
 			removeAnnotations(this, code);
 			usedBranch!.render(code, options, {
-				isCalleeOfRenderedParent: renderedParentType
-					? isCalleeOfRenderedParent
-					: (this.parent as CallExpression).callee === this,
+				isCalleeOfRenderedParent,
 				preventASI: true,
-				renderedParentType: renderedParentType || this.parent.type
+				...(renderedSurroundingElement
+					? { renderedSurroundingElement }
+					: { renderedParentType: renderedParentType || this.parent.type })
 			});
 		} else {
 			this.test.render(code, options, {
-				renderedSurroundingElement: renderedParentType
+				renderedSurroundingElement: renderedParentType || renderedSurroundingElement
 			});
 			this.consequent.render(code, options);
 			this.alternate.render(code, options);

diff --git a/src/ast/nodes/LogicalExpression.ts b/src/ast/nodes/LogicalExpression.ts
@@ -19,7 +19,6 @@ import {
 	SHARED_RECURSION_TRACKER,
 	UNKNOWN_PATH
 } from '../utils/PathTracker';
-import CallExpression from './CallExpression';
 import * as NodeType from './NodeType';
 import { ExpressionEntity, LiteralValueOrUnknown, UnknownValue } from './shared/Expression';
 import { MultiExpression } from './shared/MultiExpression';
@@ -190,11 +189,11 @@ export default class LogicalExpression extends NodeBase implements Deoptimizable
 			}
 			removeAnnotations(this, code);
 			this.getUsedBranch()!.render(code, options, {
-				isCalleeOfRenderedParent: renderedParentType
-					? isCalleeOfRenderedParent
-					: (this.parent as CallExpression).callee === this,
+				isCalleeOfRenderedParent,
 				preventASI,
-				renderedParentType: renderedParentType || this.parent.type
+				...(renderedSurroundingElement
+					? { renderedSurroundingElement }
+					: { renderedParentType: renderedParentType || this.parent.type })
 			});
 		} else {
 			this.left.render(code, options, {

diff --git a/src/ast/nodes/MemberExpression.ts b/src/ast/nodes/MemberExpression.ts
@@ -281,17 +281,15 @@ export default class MemberExpression extends NodeBase implements DeoptimizableE
 			renderedSurroundingElement
 		}: NodeRenderOptions = BLANK
 	): void {
-		const isCalleeOfDifferentParent =
-			renderedParentType === NodeType.CallExpression && isCalleeOfRenderedParent;
 		if (this.variable || this.replacement) {
 			let replacement = this.variable ? this.variable.getName() : this.replacement;
-			if (isCalleeOfDifferentParent) replacement = '0, ' + replacement;
+			if (renderedParentType && isCalleeOfRenderedParent) replacement = '0, ' + replacement;
 			code.overwrite(this.start, this.end, replacement!, {
 				contentOnly: true,
 				storeName: true
 			});
 		} else {
-			if (isCalleeOfDifferentParent) {
+			if (renderedParentType && isCalleeOfRenderedParent) {
 				code.appendRight(this.start, '0, ');
 			}
 			const surroundingElement = renderedParentType || renderedSurroundingElement;

diff --git a/src/ast/nodes/SequenceExpression.ts b/src/ast/nodes/SequenceExpression.ts
@@ -10,9 +10,8 @@ import { treeshakeNode } from '../../utils/treeshakeNode';
 import { CallOptions } from '../CallOptions';
 import { DeoptimizableEntity } from '../DeoptimizableEntity';
 import { HasEffectsContext, InclusionContext } from '../ExecutionContext';
-import { EVENT_CALLED, NodeEvent } from '../NodeEvents';
+import { NodeEvent } from '../NodeEvents';
 import { ObjectPath, PathTracker } from '../utils/PathTracker';
-import CallExpression from './CallExpression';
 import ExpressionStatement from './ExpressionStatement';
 import * as NodeType from './NodeType';
 import { ExpressionEntity, LiteralValueOrUnknown } from './shared/Expression';
@@ -23,7 +22,7 @@ export default class SequenceExpression extends NodeBase {
 	type!: NodeType.tSequenceExpression;
 
 	deoptimizePath(path: ObjectPath): void {
-		if (path.length > 0) this.expressions[this.expressions.length - 1].deoptimizePath(path);
+		this.expressions[this.expressions.length - 1].deoptimizePath(path);
 	}
 
 	deoptimizeThisOnEventAtPath(
@@ -32,14 +31,12 @@ export default class SequenceExpression extends NodeBase {
 		thisParameter: ExpressionEntity,
 		recursionTracker: PathTracker
 	): void {
-		if (event === EVENT_CALLED || path.length > 0) {
-			this.expressions[this.expressions.length - 1].deoptimizeThisOnEventAtPath(
-				event,
-				path,
-				thisParameter,
-				recursionTracker
-			);
-		}
+		this.expressions[this.expressions.length - 1].deoptimizeThisOnEventAtPath(
+			event,
+			path,
+			thisParameter,
+			recursionTracker
+		);
 	}
 
 	getLiteralValueAtPath(
@@ -69,9 +66,9 @@ export default class SequenceExpression extends NodeBase {
 	}
 
 	hasEffectsWhenAssignedAtPath(path: ObjectPath, context: HasEffectsContext): boolean {
-		return (
-			path.length === 0 ||
-			this.expressions[this.expressions.length - 1].hasEffectsWhenAssignedAtPath(path, context)
+		return this.expressions[this.expressions.length - 1].hasEffectsWhenAssignedAtPath(
+			path,
+			context
 		);
 	}
 
@@ -123,13 +120,17 @@ export default class SequenceExpression extends NodeBase {
 			if (includedNodes === 1 && preventASI) {
 				removeLineBreaks(code, start, node.start);
 			}
-			if (node === lastNode && includedNodes === 1) {
-				node.render(code, options, {
-					isCalleeOfRenderedParent: renderedParentType
-						? isCalleeOfRenderedParent
-						: (this.parent as CallExpression).callee === this,
-					renderedParentType: renderedParentType || this.parent.type
-				});
+			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
+					});
+				}
 			} else {
 				node.render(code, options);
 			}

diff --git a/src/ast/nodes/TaggedTemplateExpression.ts b/src/ast/nodes/TaggedTemplateExpression.ts
@@ -1,3 +1,5 @@
+import MagicString from 'magic-string';
+import { RenderOptions } from '../../utils/renderHelpers';
 import { CallOptions, NO_ARGS } from '../CallOptions';
 import { HasEffectsContext } from '../ExecutionContext';
 import { EMPTY_PATH } from '../utils/PathTracker';
@@ -28,17 +30,6 @@ export default class TaggedTemplateExpression extends NodeBase {
 					this.start
 				);
 			}
-
-			if (name === 'eval') {
-				this.context.warn(
-					{
-						code: 'EVAL',
-						message: `Use of eval is strongly discouraged, as it poses security risks and may cause issues with minification`,
-						url: 'https://rollupjs.org/guide/en/#avoiding-eval'
-					},
-					this.start
-				);
-			}
 		}
 	}
 
@@ -56,4 +47,9 @@ export default class TaggedTemplateExpression extends NodeBase {
 			withNew: false
 		};
 	}
+
+	render(code: MagicString, options: RenderOptions): void {
+		this.tag.render(code, options, { isCalleeOfRenderedParent: true });
+		this.quasi.render(code, options);
+	}
 }
diff --git a/test/form/samples/conditional-expression/_expected.js b/test/form/samples/conditional-expression/_expected.js
@@ -10,8 +10,8 @@ unknownValue ? 1 : foo();
 
 // known side-effect
 foo() ;
-(function () {this.x = 1;} )();
+((function () {this.x = 1;}) )();
 (() => () => console.log( 'effect' ) )()();
 foo();
-(function () {this.x = 1;})();
+((function () {this.x = 1;}))();
 (() => () => console.log( 'effect' ))()();
diff --git a/test/function/samples/sequence-expressions-template-tag/_config.js b/test/function/samples/sequence-expressions-template-tag/_config.js
@@ -0,0 +1,3 @@
+module.exports = {
+	description: 'uses corrrect "this" for tagged template expressions in simplified sequences'
+};
diff --git a/test/function/samples/sequence-expressions-template-tag/main.js b/test/function/samples/sequence-expressions-template-tag/main.js
@@ -0,0 +1,13 @@
+let o = {
+	f() {
+		assert.ok(this !== o);
+	}
+};
+(1, o.f)();
+(1, o.f)``;
+
+(true && o.f)();
+(true && o.f)``;
+
+(true ? o.f : false)();
+(true ? o.f : false)``;
diff --git a/test/function/samples/simplify-sequence-expressions/_config.js b/test/function/samples/simplify-sequence-expressions/_config.js
@@ -0,0 +1,4 @@
+module.exports = {
+	description:
+		'correctly simplify sequence expressions if the new leading element would require parentheses'
+};
diff --git a/test/function/samples/simplify-sequence-expressions/main.js b/test/function/samples/simplify-sequence-expressions/main.js
@@ -0,0 +1,31 @@
+let value = 0;
+
+1, function(){ value = 1 }();
+assert.strictEqual(value, 1);
+
+1, function(){ value = 2 }(), 2;
+assert.strictEqual(value, 2);
+
+1, {foo: value = 3 };
+assert.strictEqual(value, 3);
+
+1, {foo: value = 4 }, 2;
+assert.strictEqual(value, 4);
+
+1, true ? function(){ value = 5 }() : false;
+assert.strictEqual(value, 5);
+
+1, true ? function(){ value = 6 }() : false, 2;
+assert.strictEqual(value, 6);
+
+1, function(){ value = 7; return true }() ? true : false;
+assert.strictEqual(value, 7);
+
+1, function(){ value = 8; return true }() ? true : false, 2;
+assert.strictEqual(value, 8);
+
+1, false || function(){ value = 9 }();
+assert.strictEqual(value, 9);
+
+1, false || function(){ value = 10 }(), 2;
+assert.strictEqual(value, 10);