account for side-effects in call expression arguments, and functions passed to functions (#786)

src/ast/nodes/CallExpression.js

@@ -31,7 +31,15 @@ export default class CallExpression extends Node {
 	}
 
 	hasEffects ( scope ) {
-		return callHasEffects( scope, this.callee, false );
+		if ( callHasEffects( scope, this.callee, false ) ) return true;
+
+		for ( let i = 0; i < this.arguments.length; i += 1 ) {
+			const arg = this.arguments[i];
+			if ( arg.hasEffects( scope ) ) return true;
+
+			// if a function is passed to a function, assume it is called
+			if ( callHasEffects( scope, arg, false ) ) return true;
+		}
 	}
 
 	initialise ( scope ) {

src/ast/nodes/NewExpression.js

@@ -3,6 +3,14 @@ import callHasEffects from './shared/callHasEffects.js';
 
 export default class NewExpression extends Node {
 	hasEffects ( scope ) {
-		return callHasEffects( scope, this.callee, true );
+		if ( callHasEffects( scope, this.callee, true ) ) return true;
+
+		for ( let i = 0; i < this.arguments.length; i += 1 ) {
+			const arg = this.arguments[i];
+			if ( arg.hasEffects( scope ) ) return true;
+
+			// if a function is passed to a function, assume it is called
+			if ( callHasEffects( scope, arg, false ) ) return true;
+		}
 	}
 }

src/ast/nodes/ReturnStatement.js

@@ -1,7 +0,0 @@
-import Node from '../Node.js';
-
-export default class ReturnStatement extends Node {
-	// hasEffects () {
-	// 	return true;
-	// }
-}

src/ast/nodes/index.js

@@ -25,7 +25,6 @@ import LogicalExpression from './LogicalExpression.js';
 import MemberExpression from './MemberExpression.js';
 import NewExpression from './NewExpression.js';
 import ObjectExpression from './ObjectExpression.js';
-import ReturnStatement from './ReturnStatement.js';
 import Statement from './shared/Statement.js';
 import TemplateLiteral from './TemplateLiteral.js';
 import ThisExpression from './ThisExpression.js';
@@ -64,7 +63,6 @@ export default {
 	MemberExpression,
 	NewExpression,
 	ObjectExpression,
-	ReturnStatement,
 	SwitchStatement: Statement,
 	TemplateLiteral,
 	ThisExpression,

src/ast/nodes/shared/callHasEffects.js

@@ -1,7 +1,7 @@
 import isReference from 'is-reference';
 import flatten from '../../utils/flatten.js';
 import pureFunctions from './pureFunctions.js';
-import { UNKNOWN } from '../../values.js';
+import * as values from '../../values.js';
 
 const currentlyCalling = new Set();
 
@@ -61,10 +61,12 @@ function fnHasEffects ( fn, isNew ) {
 }
 
 export default function callHasEffects ( scope, callee, isNew ) {
-	const values = new Set([ callee ]);
+	const nodes = new Set([ callee ]);
 
-	for ( const node of values ) {
-		if ( node === UNKNOWN ) return true; // err on side of caution
+	for ( const node of nodes ) {
+		// err on side of caution
+		// TODO refine this
+		if ( node === values.UNKNOWN || node === values.FUNCTION || node === values.OBJECT || node === values.ARRAY ) return true;
 
 		if ( /Function/.test( node.type ) ) {
 			if ( fnHasEffects( node, isNew && isES5Function( node ) ) ) return true;
@@ -84,18 +86,15 @@ export default function callHasEffects ( scope, callee, isNew ) {
 
 			else {
 				if ( node.declaration ) {
-					node.declaration.gatherPossibleValues( values );
+					node.declaration.gatherPossibleValues( nodes );
 				} else {
 					return true;
 				}
 			}
 		}
 
-		else {
-			if ( !node.gatherPossibleValues ) {
-				throw new Error( 'TODO' );
-			}
-			node.gatherPossibleValues( values );
+		else if ( node.gatherPossibleValues ) {
+			node.gatherPossibleValues( nodes );
 		}
 	}
 

test/form/side-effect-t/_config.js

@@ -0,0 +1,6 @@
+module.exports = {
+	description: 'preserves side-effects inside promises (#786)',
+	options: {
+		moduleName: 'myBundle'
+	}
+};

test/form/side-effect-t/_expected/amd.js

@@ -0,0 +1,32 @@
+define(['exports'], function (exports) { 'use strict';
+
+	function x () {
+		return new Promise( ( resolve, reject ) => {
+			console.log( 'this is a side-effect' );
+			resolve();
+		});
+	}
+
+	x();
+
+	function promiseCallback ( resolve, reject ) {
+		console.log( 'this is a side-effect' );
+		resolve();
+	}
+
+	function y () {
+		return new Promise( promiseCallback );
+	}
+
+	y();
+
+	function z ( x ) {
+		// this function has no side-effects, so should be excluded...
+	}
+
+	exports.a = 1;
+	z( exports.a += 1 ); // ...unless the call expression statement has its own side-effects
+
+	Object.defineProperty(exports, '__esModule', { value: true });
+
+});

test/form/side-effect-t/_expected/cjs.js

@@ -0,0 +1,30 @@
+'use strict';
+
+Object.defineProperty(exports, '__esModule', { value: true });
+
+function x () {
+	return new Promise( ( resolve, reject ) => {
+		console.log( 'this is a side-effect' );
+		resolve();
+	});
+}
+
+x();
+
+function promiseCallback ( resolve, reject ) {
+	console.log( 'this is a side-effect' );
+	resolve();
+}
+
+function y () {
+	return new Promise( promiseCallback );
+}
+
+y();
+
+function z ( x ) {
+	// this function has no side-effects, so should be excluded...
+}
+
+exports.a = 1;
+z( exports.a += 1 ); // ...unless the call expression statement has its own side-effects

test/form/side-effect-t/_expected/es.js

@@ -0,0 +1,28 @@
+function x () {
+	return new Promise( ( resolve, reject ) => {
+		console.log( 'this is a side-effect' );
+		resolve();
+	});
+}
+
+x();
+
+function promiseCallback ( resolve, reject ) {
+	console.log( 'this is a side-effect' );
+	resolve();
+}
+
+function y () {
+	return new Promise( promiseCallback );
+}
+
+y();
+
+function z ( x ) {
+	// this function has no side-effects, so should be excluded...
+}
+
+let a = 1;
+z( a += 1 ); // ...unless the call expression statement has its own side-effects
+
+export { a };

test/form/side-effect-t/_expected/iife.js

@@ -0,0 +1,31 @@
+(function (exports) {
+	'use strict';
+
+	function x () {
+		return new Promise( ( resolve, reject ) => {
+			console.log( 'this is a side-effect' );
+			resolve();
+		});
+	}
+
+	x();
+
+	function promiseCallback ( resolve, reject ) {
+		console.log( 'this is a side-effect' );
+		resolve();
+	}
+
+	function y () {
+		return new Promise( promiseCallback );
+	}
+
+	y();
+
+	function z ( x ) {
+		// this function has no side-effects, so should be excluded...
+	}
+
+	exports.a = 1;
+	z( exports.a += 1 ); // ...unless the call expression statement has its own side-effects
+
+}((this.myBundle = this.myBundle || {})));

test/form/side-effect-t/_expected/umd.js

@@ -0,0 +1,36 @@
+(function (global, factory) {
+	typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports) :
+	typeof define === 'function' && define.amd ? define(['exports'], factory) :
+	(factory((global.myBundle = global.myBundle || {})));
+}(this, (function (exports) { 'use strict';
+
+	function x () {
+		return new Promise( ( resolve, reject ) => {
+			console.log( 'this is a side-effect' );
+			resolve();
+		});
+	}
+
+	x();
+
+	function promiseCallback ( resolve, reject ) {
+		console.log( 'this is a side-effect' );
+		resolve();
+	}
+
+	function y () {
+		return new Promise( promiseCallback );
+	}
+
+	y();
+
+	function z ( x ) {
+		// this function has no side-effects, so should be excluded...
+	}
+
+	exports.a = 1;
+	z( exports.a += 1 ); // ...unless the call expression statement has its own side-effects
+
+	Object.defineProperty(exports, '__esModule', { value: true });
+
+})));

test/form/side-effect-t/main.js

@@ -0,0 +1,28 @@
+function x () {
+	return new Promise( ( resolve, reject ) => {
+		console.log( 'this is a side-effect' );
+		resolve();
+	});
+}
+
+x();
+
+function promiseCallback ( resolve, reject ) {
+	console.log( 'this is a side-effect' );
+	resolve();
+}
+
+function y () {
+	return new Promise( promiseCallback );
+}
+
+y();
+
+function z ( x ) {
+	// this function has no side-effects, so should be excluded...
+}
+
+let a = 1;
+z( a += 1 ); // ...unless the call expression statement has its own side-effects
+
+export { a };