Merge branch 'fix-if-statements'

# Conflicts: # CHANGELOG.md
rollup · Apr 23, 2018 · e5df762 · e5df762
2 parents 481c47f + d21b935
commit e5df762
Show file tree

Hide file tree

Showing 27 changed files with 375 additions and 268 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,10 @@
 * Improve handling of external modules imported as both default and named imports ([#2136](https://github.com/rollup/rollup/pull/2136))
 * Fix wrong file name in error message ([#2137](https://github.com/rollup/rollup/pull/2137))
 
+## 0.58.2
+*2018-04-23*
+* Fix rendering of certain statically resolvable if statements ([#2146](https://github.com/rollup/rollup/pull/2146))
+
 ## 0.58.1
 *2018-04-18*
 * Fix comment detection ([#2129](https://github.com/rollup/rollup/pull/2129))

diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "rollup",
-  "version": "0.58.1",
+  "version": "0.58.2",
   "description": "Next-generation ES6 module bundler",
   "main": "dist/rollup.js",
   "module": "dist/rollup.es.js",

diff --git a/src/ast/nodes/ConditionalExpression.ts b/src/ast/nodes/ConditionalExpression.ts
@@ -23,62 +23,73 @@ export default class ConditionalExpression extends NodeBase {
 		callback: ForEachReturnExpressionCallback,
 		options: ExecutionPathOptions
 	) {
-		if (this.hasUnknownTestValue) {
+		const testValue = this.hasUnknownTestValue ? UNKNOWN_VALUE : this.getTestValue();
+		if (testValue === UNKNOWN_VALUE || testValue) {
 			this.consequent.forEachReturnExpressionWhenCalledAtPath(path, callOptions, callback, options);
+		}
+		if (testValue === UNKNOWN_VALUE || !testValue) {
 			this.alternate.forEachReturnExpressionWhenCalledAtPath(path, callOptions, callback, options);
-		} else {
-			this.forEachRelevantBranch(node =>
-				node.forEachReturnExpressionWhenCalledAtPath(path, callOptions, callback, options)
-			);
 		}
 	}
 
 	getValue(): any {
-		const testValue = this.test.getValue();
+		const testValue = this.hasUnknownTestValue ? UNKNOWN_VALUE : this.getTestValue();
 		if (testValue === UNKNOWN_VALUE) return UNKNOWN_VALUE;
 		return testValue ? this.consequent.getValue() : this.alternate.getValue();
 	}
 
 	hasEffects(options: ExecutionPathOptions): boolean {
-		return (
-			this.test.hasEffects(options) ||
-			(this.hasUnknownTestValue
-				? this.consequent.hasEffects(options) || this.alternate.hasEffects(options)
-				: this.someRelevantBranch(node => node.hasEffects(options)))
-		);
+		if (this.test.hasEffects(options)) return true;
+		const testValue = this.hasUnknownTestValue ? UNKNOWN_VALUE : this.getTestValue();
+		if (testValue === UNKNOWN_VALUE) {
+			return this.consequent.hasEffects(options) || this.alternate.hasEffects(options);
+		}
+		return testValue ? this.consequent.hasEffects(options) : this.alternate.hasEffects(options);
 	}
 
 	hasEffectsWhenAccessedAtPath(path: ObjectPath, options: ExecutionPathOptions): boolean {
-		return (
-			path.length > 0 &&
-			(this.hasUnknownTestValue
-				? this.consequent.hasEffectsWhenAccessedAtPath(path, options) ||
-				  this.alternate.hasEffectsWhenAccessedAtPath(path, options)
-				: this.someRelevantBranch(node => node.hasEffectsWhenAccessedAtPath(path, options)))
-		);
+		if (path.length === 0) return false;
+		const testValue = this.hasUnknownTestValue ? UNKNOWN_VALUE : this.getTestValue();
+		if (testValue === UNKNOWN_VALUE) {
+			return (
+				this.consequent.hasEffectsWhenAccessedAtPath(path, options) ||
+				this.alternate.hasEffectsWhenAccessedAtPath(path, options)
+			);
+		}
+		return testValue
+			? this.consequent.hasEffectsWhenAccessedAtPath(path, options)
+			: this.alternate.hasEffectsWhenAccessedAtPath(path, options);
 	}
 
 	hasEffectsWhenAssignedAtPath(path: ObjectPath, options: ExecutionPathOptions): boolean {
-		return (
-			path.length === 0 ||
-			(this.hasUnknownTestValue
-				? this.consequent.hasEffectsWhenAssignedAtPath(path, options) ||
-				  this.alternate.hasEffectsWhenAssignedAtPath(path, options)
-				: this.someRelevantBranch(node => node.hasEffectsWhenAssignedAtPath(path, options)))
-		);
+		if (path.length === 0) return true;
+		const testValue = this.hasUnknownTestValue ? UNKNOWN_VALUE : this.getTestValue();
+		if (testValue === UNKNOWN_VALUE) {
+			return (
+				this.consequent.hasEffectsWhenAssignedAtPath(path, options) ||
+				this.alternate.hasEffectsWhenAssignedAtPath(path, options)
+			);
+		}
+		return testValue
+			? this.consequent.hasEffectsWhenAssignedAtPath(path, options)
+			: this.alternate.hasEffectsWhenAssignedAtPath(path, options);
 	}
 
 	hasEffectsWhenCalledAtPath(
 		path: ObjectPath,
 		callOptions: CallOptions,
 		options: ExecutionPathOptions
 	): boolean {
-		return this.hasUnknownTestValue
-			? this.consequent.hasEffectsWhenCalledAtPath(path, callOptions, options) ||
-					this.alternate.hasEffectsWhenCalledAtPath(path, callOptions, options)
-			: this.someRelevantBranch(node =>
-					node.hasEffectsWhenCalledAtPath(path, callOptions, options)
-			  );
+		const testValue = this.hasUnknownTestValue ? UNKNOWN_VALUE : this.getTestValue();
+		if (testValue === UNKNOWN_VALUE) {
+			return (
+				this.consequent.hasEffectsWhenCalledAtPath(path, callOptions, options) ||
+				this.alternate.hasEffectsWhenCalledAtPath(path, callOptions, options)
+			);
+		}
+		return testValue
+			? this.consequent.hasEffectsWhenCalledAtPath(path, callOptions, options)
+			: this.alternate.hasEffectsWhenCalledAtPath(path, callOptions, options);
 	}
 
 	initialise() {
@@ -88,7 +99,7 @@ export default class ConditionalExpression extends NodeBase {
 
 	include() {
 		this.included = true;
-		const testValue = this.test.getValue();
+		const testValue = this.hasUnknownTestValue ? UNKNOWN_VALUE : this.getTestValue();
 		if (testValue === UNKNOWN_VALUE || this.test.shouldBeIncluded()) {
 			this.test.include();
 			this.consequent.include();
@@ -102,11 +113,12 @@ export default class ConditionalExpression extends NodeBase {
 
 	reassignPath(path: ObjectPath, options: ExecutionPathOptions) {
 		if (path.length > 0) {
-			if (this.hasUnknownTestValue) {
+			const testValue = this.hasUnknownTestValue ? UNKNOWN_VALUE : this.getTestValue();
+			if (testValue === UNKNOWN_VALUE || testValue) {
 				this.consequent.reassignPath(path, options);
+			}
+			if (testValue === UNKNOWN_VALUE || !testValue) {
 				this.alternate.reassignPath(path, options);
-			} else {
-				this.forEachRelevantBranch(node => node.reassignPath(path, options));
 			}
 		}
 	}
@@ -116,18 +128,18 @@ export default class ConditionalExpression extends NodeBase {
 		options: RenderOptions,
 		{ renderedParentType, isCalleeOfRenderedParent }: NodeRenderOptions = BLANK
 	) {
-		if (!this.context.treeshake || this.test.included) {
-			super.render(code, options);
-		} else {
-			const branchToRetain = this.consequent.included ? this.consequent : this.alternate;
-			code.remove(this.start, branchToRetain.start);
-			code.remove(branchToRetain.end, this.end);
-			branchToRetain.render(code, options, {
+		if (!this.test.included) {
+			const singleRetainedBranch = this.consequent.included ? this.consequent : this.alternate;
+			code.remove(this.start, singleRetainedBranch.start);
+			code.remove(singleRetainedBranch.end, this.end);
+			singleRetainedBranch.render(code, options, {
 				renderedParentType: renderedParentType || this.parent.type,
 				isCalleeOfRenderedParent: renderedParentType
 					? isCalleeOfRenderedParent
 					: (<CallExpression>this.parent).callee === this
 			});
+		} else {
+			super.render(code, options);
 		}
 	}
 
@@ -137,43 +149,44 @@ export default class ConditionalExpression extends NodeBase {
 		predicateFunction: SomeReturnExpressionCallback,
 		options: ExecutionPathOptions
 	): boolean {
-		return this.hasUnknownTestValue
+		const testValue = this.hasUnknownTestValue ? UNKNOWN_VALUE : this.getTestValue();
+		if (testValue === UNKNOWN_VALUE) {
+			return (
+				this.consequent.someReturnExpressionWhenCalledAtPath(
+					path,
+					callOptions,
+					predicateFunction,
+					options
+				) ||
+				this.alternate.someReturnExpressionWhenCalledAtPath(
+					path,
+					callOptions,
+					predicateFunction,
+					options
+				)
+			);
+		}
+		return testValue
 			? this.consequent.someReturnExpressionWhenCalledAtPath(
 					path,
 					callOptions,
 					predicateFunction,
 					options
-			  ) ||
-					this.alternate.someReturnExpressionWhenCalledAtPath(
-						path,
-						callOptions,
-						predicateFunction,
-						options
-					)
-			: this.someRelevantBranch(node =>
-					node.someReturnExpressionWhenCalledAtPath(path, callOptions, predicateFunction, options)
+			  )
+			: this.alternate.someReturnExpressionWhenCalledAtPath(
+					path,
+					callOptions,
+					predicateFunction,
+					options
 			  );
 	}
 
-	private forEachRelevantBranch(callback: (node: ExpressionNode) => void) {
-		const testValue = this.test.getValue();
-		if (testValue === UNKNOWN_VALUE) {
-			this.hasUnknownTestValue = true;
-			callback(this.consequent);
-			callback(this.alternate);
-		} else if (testValue) {
-			callback(this.consequent);
-		} else {
-			callback(this.alternate);
-		}
-	}
-
-	private someRelevantBranch(predicateFunction: (node: ExpressionNode) => boolean): boolean {
-		const testValue = this.test.getValue();
-		if (testValue === UNKNOWN_VALUE) {
+	private getTestValue() {
+		if (this.hasUnknownTestValue) return UNKNOWN_VALUE;
+		const value = this.test.getValue();
+		if (value === UNKNOWN_VALUE) {
 			this.hasUnknownTestValue = true;
-			return predicateFunction(this.consequent) || predicateFunction(this.alternate);
 		}
-		return testValue ? predicateFunction(this.consequent) : predicateFunction(this.alternate);
+		return value;
 	}
 }
diff --git a/src/ast/nodes/IfStatement.ts b/src/ast/nodes/IfStatement.ts
@@ -14,27 +14,32 @@ export default class IfStatement extends StatementBase {
 	private hasUnknownTestValue: boolean;
 
 	hasEffects(options: ExecutionPathOptions): boolean {
-		return (
-			this.test.hasEffects(options) ||
-			(this.hasUnknownTestValue
-				? this.consequent.hasEffects(options) ||
-				  (this.alternate !== null && this.alternate.hasEffects(options))
-				: this.someRelevantBranch(node => node.hasEffects(options)))
-		);
+		if (this.test.hasEffects(options)) return true;
+		const testValue = this.hasUnknownTestValue ? UNKNOWN_VALUE : this.getTestValue();
+		if (testValue === UNKNOWN_VALUE) {
+			return (
+				this.consequent.hasEffects(options) ||
+				(this.alternate !== null && this.alternate.hasEffects(options))
+			);
+		}
+		return testValue
+			? this.consequent.hasEffects(options)
+			: this.alternate !== null && this.alternate.hasEffects(options);
 	}
 
 	include() {
 		this.included = true;
-		const testValue = this.test.getValue();
+		const testValue = this.hasUnknownTestValue ? UNKNOWN_VALUE : this.getTestValue();
 		if (testValue === UNKNOWN_VALUE || this.test.shouldBeIncluded()) {
 			this.test.include();
 		}
-		if (testValue === UNKNOWN_VALUE) {
-			this.consequent.include();
-			if (this.alternate !== null) this.alternate.include();
-		} else if (testValue) {
+		if ((testValue === UNKNOWN_VALUE || testValue) && this.consequent.shouldBeIncluded()) {
 			this.consequent.include();
-		} else if (this.alternate !== null) {
+		}
+		if (
+			this.alternate !== null &&
+			((testValue === UNKNOWN_VALUE || !testValue) && this.alternate.shouldBeIncluded())
+		) {
 			this.alternate.include();
 		}
 	}
@@ -45,33 +50,43 @@ export default class IfStatement extends StatementBase {
 	}
 
 	render(code: MagicString, options: RenderOptions) {
-		const testValue = this.test.getValue();
+		// Note that unknown test values are always included
+		const testValue = this.hasUnknownTestValue ? UNKNOWN_VALUE : this.test.getValue();
 		if (
-			!this.context.treeshake ||
-			this.test.included ||
-			(testValue ? this.alternate !== null && this.alternate.included : this.consequent.included)
+			!this.test.included &&
+			(testValue ? this.alternate === null || !this.alternate.included : !this.consequent.included)
 		) {
-			super.render(code, options);
+			const singleRetainedBranch = testValue ? this.consequent : this.alternate;
+			code.remove(this.start, singleRetainedBranch.start);
+			code.remove(singleRetainedBranch.end, this.end);
+			singleRetainedBranch.render(code, options);
 		} else {
-			// if test is not included, it is impossible that alternate===null even though it is the retained branch
-			const branchToRetain = testValue ? this.consequent : this.alternate;
-			code.remove(this.start, branchToRetain.start);
-			code.remove(branchToRetain.end, this.end);
-			branchToRetain.render(code, options);
+			if (this.test.included) {
+				this.test.render(code, options);
+			} else {
+				code.overwrite(this.test.start, this.test.end, testValue ? 'true' : 'false');
+			}
+			if (this.consequent.included) {
+				this.consequent.render(code, options);
+			} else {
+				code.overwrite(this.consequent.start, this.consequent.end, ';');
+			}
+			if (this.alternate !== null) {
+				if (this.alternate.included) {
+					this.alternate.render(code, options);
+				} else {
+					code.remove(this.consequent.end, this.alternate.end);
+				}
+			}
 		}
 	}
 
-	private someRelevantBranch(predicateFunction: (node: StatementNode) => boolean): boolean {
-		const testValue = this.test.getValue();
-		if (testValue === UNKNOWN_VALUE) {
+	private getTestValue() {
+		if (this.hasUnknownTestValue) return UNKNOWN_VALUE;
+		const value = this.test.getValue();
+		if (value === UNKNOWN_VALUE) {
 			this.hasUnknownTestValue = true;
-			return (
-				predicateFunction(this.consequent) ||
-				(this.alternate !== null && predicateFunction(this.alternate))
-			);
 		}
-		return testValue
-			? predicateFunction(this.consequent)
-			: this.alternate !== null && predicateFunction(this.alternate);
+		return value;
 	}
 }