From 6da374cea2ad72b3a6f729d0e89a532170cf0858 Mon Sep 17 00:00:00 2001
From: Rich-Harris <richard.a.harris@gmail.com>
Date: Sat, 10 Sep 2016 16:49:37 -0400
Subject: [PATCH] preserve effects in for-of and for-in statements. fixes #870

---
 src/ast/nodes/ExpressionStatement.js          | 15 +------
 src/ast/nodes/ForInStatement.js               | 13 ++----
 src/ast/nodes/ForOfStatement.js               | 13 ++----
 src/ast/nodes/IfStatement.js                  |  4 +-
 src/ast/nodes/ThrowStatement.js               |  7 ++++
 src/ast/nodes/index.js                        |  2 +
 src/ast/nodes/shared/Statement.js             | 16 ++++++++
 src/ast/nodes/shared/assignTo.js              | 29 +++++++++++++
 .../_config.js                                |  3 ++
 .../_expected/amd.js                          | 28 +++++++++++++
 .../_expected/cjs.js                          | 26 ++++++++++++
 .../_expected/es.js                           | 24 +++++++++++
 .../_expected/iife.js                         | 29 +++++++++++++
 .../_expected/umd.js                          | 32 +++++++++++++++
 .../main.js                                   | 41 +++++++++++++++++++
 .../effect-in-for-of-loop/_expected/amd.js    | 17 ++++----
 .../effect-in-for-of-loop/_expected/cjs.js    | 17 ++++----
 .../effect-in-for-of-loop/_expected/es.js     | 17 ++++----
 .../effect-in-for-of-loop/_expected/iife.js   | 17 ++++----
 .../effect-in-for-of-loop/_expected/umd.js    | 17 ++++----
 test/form/effect-in-for-of-loop/main.js       | 28 +++++++------
 21 files changed, 309 insertions(+), 86 deletions(-)
 create mode 100644 src/ast/nodes/ThrowStatement.js
 create mode 100644 src/ast/nodes/shared/Statement.js
 create mode 100644 src/ast/nodes/shared/assignTo.js
 create mode 100644 test/form/effect-in-for-of-loop-in-functions/_config.js
 create mode 100644 test/form/effect-in-for-of-loop-in-functions/_expected/amd.js
 create mode 100644 test/form/effect-in-for-of-loop-in-functions/_expected/cjs.js
 create mode 100644 test/form/effect-in-for-of-loop-in-functions/_expected/es.js
 create mode 100644 test/form/effect-in-for-of-loop-in-functions/_expected/iife.js
 create mode 100644 test/form/effect-in-for-of-loop-in-functions/_expected/umd.js
 create mode 100644 test/form/effect-in-for-of-loop-in-functions/main.js

diff --git a/src/ast/nodes/ExpressionStatement.js b/src/ast/nodes/ExpressionStatement.js
index c5c3255..237441a 100644
--- a/src/ast/nodes/ExpressionStatement.js
+++ b/src/ast/nodes/ExpressionStatement.js
@@ -1,16 +1,5 @@
-import Node from '../Node.js';
+import Statement from './shared/Statement.js';
 
-export default class ExpressionStatement extends Node {
-	render ( code, es ) {
-		if ( !this.module.bundle.treeshake || this.shouldInclude ) {
-			super.render( code, es );
-		} else {
-			code.remove( this.leadingCommentStart || this.start, this.next || this.end );
-		}
-	}
+export default class ExpressionStatement extends Statement {
 
-	run ( scope ) {
-		this.shouldInclude = true;
-		super.run( scope );
-	}
 }
diff --git a/src/ast/nodes/ForInStatement.js b/src/ast/nodes/ForInStatement.js
index 002dd0a..a59bfd2 100644
--- a/src/ast/nodes/ForInStatement.js
+++ b/src/ast/nodes/ForInStatement.js
@@ -1,15 +1,10 @@
-import Node from '../Node.js';
+import Statement from './shared/Statement.js';
+import assignTo from './shared/assignTo.js';
 import { STRING } from '../values.js';
 
-export default class ForInStatement extends Node {
+export default class ForInStatement extends Statement {
 	initialise ( scope ) {
 		super.initialise( scope );
-
-		// special case
-		if ( this.left.type === 'VariableDeclaration' ) {
-			for ( const proxy of this.left.declarations[0].proxies.values() ) {
-				proxy.possibleValues.add( STRING );
-			}
-		}
+		assignTo( this.left, scope, STRING );
 	}
 }
diff --git a/src/ast/nodes/ForOfStatement.js b/src/ast/nodes/ForOfStatement.js
index 201d491..9e663a0 100644
--- a/src/ast/nodes/ForOfStatement.js
+++ b/src/ast/nodes/ForOfStatement.js
@@ -1,15 +1,10 @@
-import Node from '../Node.js';
+import Statement from './shared/Statement.js';
+import assignTo from './shared/assignTo.js';
 import { UNKNOWN } from '../values.js';
 
-export default class ForOfStatement extends Node {
+export default class ForOfStatement extends Statement {
 	initialise ( scope ) {
 		super.initialise( scope );
-
-		// special case
-		if ( this.left.type === 'VariableDeclaration' ) {
-			for ( const proxy of this.left.declarations[0].proxies.values() ) {
-				proxy.possibleValues.add( UNKNOWN );
-			}
-		}
+		assignTo( this.left, scope, UNKNOWN );
 	}
 }
diff --git a/src/ast/nodes/IfStatement.js b/src/ast/nodes/IfStatement.js
index 1a5c184..9e6f0fc 100644
--- a/src/ast/nodes/IfStatement.js
+++ b/src/ast/nodes/IfStatement.js
@@ -1,8 +1,8 @@
-import Node from '../Node.js';
+import Statement from './shared/Statement.js';
 import { UNKNOWN } from '../values.js';
 
 // TODO DRY this out
-export default class IfStatement extends Node {
+export default class IfStatement extends Statement {
 	initialise ( scope ) {
 		this.testValue = this.test.getValue();
 
diff --git a/src/ast/nodes/ThrowStatement.js b/src/ast/nodes/ThrowStatement.js
new file mode 100644
index 0000000..4e9047b
--- /dev/null
+++ b/src/ast/nodes/ThrowStatement.js
@@ -0,0 +1,7 @@
+import Node from '../Node.js';
+
+export default class ThrowStatement extends Node {
+	hasEffects ( scope ) {
+		return scope.findLexicalBoundary().isModuleScope; // TODO should this just be `true`? probably...
+	}
+}
diff --git a/src/ast/nodes/index.js b/src/ast/nodes/index.js
index 3cca1ad..b5babe0 100644
--- a/src/ast/nodes/index.js
+++ b/src/ast/nodes/index.js
@@ -26,6 +26,7 @@ import ParenthesizedExpression from './ParenthesizedExpression.js';
 import ReturnStatement from './ReturnStatement.js';
 import TemplateLiteral from './TemplateLiteral.js';
 import ThisExpression from './ThisExpression.js';
+import ThrowStatement from './ThrowStatement.js';
 import UnaryExpression from './UnaryExpression.js';
 import UpdateExpression from './UpdateExpression.js';
 import VariableDeclarator from './VariableDeclarator.js';
@@ -60,6 +61,7 @@ export default {
 	ReturnStatement,
 	TemplateLiteral,
 	ThisExpression,
+	ThrowStatement,
 	UnaryExpression,
 	UpdateExpression,
 	VariableDeclarator,
diff --git a/src/ast/nodes/shared/Statement.js b/src/ast/nodes/shared/Statement.js
new file mode 100644
index 0000000..5667652
--- /dev/null
+++ b/src/ast/nodes/shared/Statement.js
@@ -0,0 +1,16 @@
+import Node from '../../Node.js';
+
+export default class Statement extends Node {
+	render ( code, es ) {
+		if ( !this.module.bundle.treeshake || this.shouldInclude ) {
+			super.render( code, es );
+		} else {
+			code.remove( this.leadingCommentStart || this.start, this.next || this.end );
+		}
+	}
+
+	run ( scope ) {
+		this.shouldInclude = true;
+		super.run( scope );
+	}
+}
diff --git a/src/ast/nodes/shared/assignTo.js b/src/ast/nodes/shared/assignTo.js
new file mode 100644
index 0000000..03512e1
--- /dev/null
+++ b/src/ast/nodes/shared/assignTo.js
@@ -0,0 +1,29 @@
+import extractNames from '../../utils/extractNames.js';
+
+export default function assignToForLoopLeft ( node, scope, value ) {
+	if ( node.type === 'VariableDeclaration' ) {
+		for ( const proxy of node.declarations[0].proxies.values() ) {
+			proxy.possibleValues.add( value );
+		}
+	}
+
+	else {
+		while ( node.type === 'ParenthesizedExpression' ) node = node.expression;
+
+		if ( node.type === 'MemberExpression' ) {
+			// apparently this is legal JavaScript? Though I don't know what
+			// kind of monster would write `for ( foo.bar of thing ) {...}`
+
+			// for now, do nothing, as I'm not sure anything needs to happen...
+		}
+
+		else {
+			for ( const name of extractNames( node ) ) {
+				const declaration = scope.findDeclaration( name );
+				if ( declaration.possibleValues ) {
+					declaration.possibleValues.add( value );
+				}
+			}
+		}
+	}
+}
diff --git a/test/form/effect-in-for-of-loop-in-functions/_config.js b/test/form/effect-in-for-of-loop-in-functions/_config.js
new file mode 100644
index 0000000..79abe74
--- /dev/null
+++ b/test/form/effect-in-for-of-loop-in-functions/_config.js
@@ -0,0 +1,3 @@
+module.exports = {
+	description: 'includes effects in for-of loop (#870)'
+}
diff --git a/test/form/effect-in-for-of-loop-in-functions/_expected/amd.js b/test/form/effect-in-for-of-loop-in-functions/_expected/amd.js
new file mode 100644
index 0000000..a7c1b48
--- /dev/null
+++ b/test/form/effect-in-for-of-loop-in-functions/_expected/amd.js
@@ -0,0 +1,28 @@
+define(function () { 'use strict';
+
+	const items = [{}, {}, {}];
+
+	function a () {
+		for ( const item of items.children ) {
+			item.foo = 'a';
+		}
+	}
+
+	a();
+
+	function c () {
+		let item;
+		for ( item of items.children ) {
+			item.bar = 'c';
+		}
+	}
+
+	c();
+
+	assert.deepEqual( items, [
+		{ foo: 'a', bar: 'c' },
+		{ foo: 'a', bar: 'c' },
+		{ foo: 'a', bar: 'c' }
+	]);
+
+});
diff --git a/test/form/effect-in-for-of-loop-in-functions/_expected/cjs.js b/test/form/effect-in-for-of-loop-in-functions/_expected/cjs.js
new file mode 100644
index 0000000..51c5f3b
--- /dev/null
+++ b/test/form/effect-in-for-of-loop-in-functions/_expected/cjs.js
@@ -0,0 +1,26 @@
+'use strict';
+
+const items = [{}, {}, {}];
+
+function a () {
+	for ( const item of items.children ) {
+		item.foo = 'a';
+	}
+}
+
+a();
+
+function c () {
+	let item;
+	for ( item of items.children ) {
+		item.bar = 'c';
+	}
+}
+
+c();
+
+assert.deepEqual( items, [
+	{ foo: 'a', bar: 'c' },
+	{ foo: 'a', bar: 'c' },
+	{ foo: 'a', bar: 'c' }
+]);
diff --git a/test/form/effect-in-for-of-loop-in-functions/_expected/es.js b/test/form/effect-in-for-of-loop-in-functions/_expected/es.js
new file mode 100644
index 0000000..0aafee1
--- /dev/null
+++ b/test/form/effect-in-for-of-loop-in-functions/_expected/es.js
@@ -0,0 +1,24 @@
+const items = [{}, {}, {}];
+
+function a () {
+	for ( const item of items.children ) {
+		item.foo = 'a';
+	}
+}
+
+a();
+
+function c () {
+	let item;
+	for ( item of items.children ) {
+		item.bar = 'c';
+	}
+}
+
+c();
+
+assert.deepEqual( items, [
+	{ foo: 'a', bar: 'c' },
+	{ foo: 'a', bar: 'c' },
+	{ foo: 'a', bar: 'c' }
+]);
diff --git a/test/form/effect-in-for-of-loop-in-functions/_expected/iife.js b/test/form/effect-in-for-of-loop-in-functions/_expected/iife.js
new file mode 100644
index 0000000..406ab3a
--- /dev/null
+++ b/test/form/effect-in-for-of-loop-in-functions/_expected/iife.js
@@ -0,0 +1,29 @@
+(function () {
+	'use strict';
+
+	const items = [{}, {}, {}];
+
+	function a () {
+		for ( const item of items.children ) {
+			item.foo = 'a';
+		}
+	}
+
+	a();
+
+	function c () {
+		let item;
+		for ( item of items.children ) {
+			item.bar = 'c';
+		}
+	}
+
+	c();
+
+	assert.deepEqual( items, [
+		{ foo: 'a', bar: 'c' },
+		{ foo: 'a', bar: 'c' },
+		{ foo: 'a', bar: 'c' }
+	]);
+
+}());
diff --git a/test/form/effect-in-for-of-loop-in-functions/_expected/umd.js b/test/form/effect-in-for-of-loop-in-functions/_expected/umd.js
new file mode 100644
index 0000000..33518c0
--- /dev/null
+++ b/test/form/effect-in-for-of-loop-in-functions/_expected/umd.js
@@ -0,0 +1,32 @@
+(function (global, factory) {
+	typeof exports === 'object' && typeof module !== 'undefined' ? factory() :
+	typeof define === 'function' && define.amd ? define(factory) :
+	(factory());
+}(this, (function () { 'use strict';
+
+	const items = [{}, {}, {}];
+
+	function a () {
+		for ( const item of items.children ) {
+			item.foo = 'a';
+		}
+	}
+
+	a();
+
+	function c () {
+		let item;
+		for ( item of items.children ) {
+			item.bar = 'c';
+		}
+	}
+
+	c();
+
+	assert.deepEqual( items, [
+		{ foo: 'a', bar: 'c' },
+		{ foo: 'a', bar: 'c' },
+		{ foo: 'a', bar: 'c' }
+	]);
+
+})));
diff --git a/test/form/effect-in-for-of-loop-in-functions/main.js b/test/form/effect-in-for-of-loop-in-functions/main.js
new file mode 100644
index 0000000..7ba1241
--- /dev/null
+++ b/test/form/effect-in-for-of-loop-in-functions/main.js
@@ -0,0 +1,41 @@
+const items = [{}, {}, {}];
+
+function a () {
+	for ( const item of items.children ) {
+		item.foo = 'a';
+	}
+}
+
+a();
+
+function b () {
+	for ( const item of items.children ) {
+		// do nothing
+	}
+}
+
+b();
+
+function c () {
+	let item;
+	for ( item of items.children ) {
+		item.bar = 'c';
+	}
+}
+
+c();
+
+function d () {
+	let item;
+	for ( item of items.children ) {
+		// do nothing
+	}
+}
+
+d();
+
+assert.deepEqual( items, [
+	{ foo: 'a', bar: 'c' },
+	{ foo: 'a', bar: 'c' },
+	{ foo: 'a', bar: 'c' }
+]);
diff --git a/test/form/effect-in-for-of-loop/_expected/amd.js b/test/form/effect-in-for-of-loop/_expected/amd.js
index 57223a0..c252e26 100644
--- a/test/form/effect-in-for-of-loop/_expected/amd.js
+++ b/test/form/effect-in-for-of-loop/_expected/amd.js
@@ -2,18 +2,19 @@ define(function () { 'use strict';
 
 	const items = [{}, {}, {}];
 
-	function x () {
-		for ( const item of items.children ) {
-			item.foo = 'bar';
-		}
+	for ( const a of items.children ) {
+		a.foo = 'a';
 	}
 
-	x();
+	let c;
+	for ( c of items.children ) {
+		c.bar = 'c';
+	}
 
 	assert.deepEqual( items, [
-		{ foo: 'bar' },
-		{ foo: 'bar' },
-		{ foo: 'bar' }
+		{ foo: 'a', bar: 'c' },
+		{ foo: 'a', bar: 'c' },
+		{ foo: 'a', bar: 'c' }
 	]);
 
 });
diff --git a/test/form/effect-in-for-of-loop/_expected/cjs.js b/test/form/effect-in-for-of-loop/_expected/cjs.js
index fd94a9d..197b73e 100644
--- a/test/form/effect-in-for-of-loop/_expected/cjs.js
+++ b/test/form/effect-in-for-of-loop/_expected/cjs.js
@@ -2,16 +2,17 @@
 
 const items = [{}, {}, {}];
 
-function x () {
-	for ( const item of items.children ) {
-		item.foo = 'bar';
-	}
+for ( const a of items.children ) {
+	a.foo = 'a';
 }
 
-x();
+let c;
+for ( c of items.children ) {
+	c.bar = 'c';
+}
 
 assert.deepEqual( items, [
-	{ foo: 'bar' },
-	{ foo: 'bar' },
-	{ foo: 'bar' }
+	{ foo: 'a', bar: 'c' },
+	{ foo: 'a', bar: 'c' },
+	{ foo: 'a', bar: 'c' }
 ]);
diff --git a/test/form/effect-in-for-of-loop/_expected/es.js b/test/form/effect-in-for-of-loop/_expected/es.js
index cabffc4..3869419 100644
--- a/test/form/effect-in-for-of-loop/_expected/es.js
+++ b/test/form/effect-in-for-of-loop/_expected/es.js
@@ -1,15 +1,16 @@
 const items = [{}, {}, {}];
 
-function x () {
-	for ( const item of items.children ) {
-		item.foo = 'bar';
-	}
+for ( const a of items.children ) {
+	a.foo = 'a';
 }
 
-x();
+let c;
+for ( c of items.children ) {
+	c.bar = 'c';
+}
 
 assert.deepEqual( items, [
-	{ foo: 'bar' },
-	{ foo: 'bar' },
-	{ foo: 'bar' }
+	{ foo: 'a', bar: 'c' },
+	{ foo: 'a', bar: 'c' },
+	{ foo: 'a', bar: 'c' }
 ]);
diff --git a/test/form/effect-in-for-of-loop/_expected/iife.js b/test/form/effect-in-for-of-loop/_expected/iife.js
index 96e91ea..7ca26c8 100644
--- a/test/form/effect-in-for-of-loop/_expected/iife.js
+++ b/test/form/effect-in-for-of-loop/_expected/iife.js
@@ -3,18 +3,19 @@
 
 	const items = [{}, {}, {}];
 
-	function x () {
-		for ( const item of items.children ) {
-			item.foo = 'bar';
-		}
+	for ( const a of items.children ) {
+		a.foo = 'a';
 	}
 
-	x();
+	let c;
+	for ( c of items.children ) {
+		c.bar = 'c';
+	}
 
 	assert.deepEqual( items, [
-		{ foo: 'bar' },
-		{ foo: 'bar' },
-		{ foo: 'bar' }
+		{ foo: 'a', bar: 'c' },
+		{ foo: 'a', bar: 'c' },
+		{ foo: 'a', bar: 'c' }
 	]);
 
 }());
diff --git a/test/form/effect-in-for-of-loop/_expected/umd.js b/test/form/effect-in-for-of-loop/_expected/umd.js
index 83a6d30..08ef206 100644
--- a/test/form/effect-in-for-of-loop/_expected/umd.js
+++ b/test/form/effect-in-for-of-loop/_expected/umd.js
@@ -6,18 +6,19 @@
 
 	const items = [{}, {}, {}];
 
-	function x () {
-		for ( const item of items.children ) {
-			item.foo = 'bar';
-		}
+	for ( const a of items.children ) {
+		a.foo = 'a';
 	}
 
-	x();
+	let c;
+	for ( c of items.children ) {
+		c.bar = 'c';
+	}
 
 	assert.deepEqual( items, [
-		{ foo: 'bar' },
-		{ foo: 'bar' },
-		{ foo: 'bar' }
+		{ foo: 'a', bar: 'c' },
+		{ foo: 'a', bar: 'c' },
+		{ foo: 'a', bar: 'c' }
 	]);
 
 })));
diff --git a/test/form/effect-in-for-of-loop/main.js b/test/form/effect-in-for-of-loop/main.js
index 401dcf2..122bf0f 100644
--- a/test/form/effect-in-for-of-loop/main.js
+++ b/test/form/effect-in-for-of-loop/main.js
@@ -1,23 +1,25 @@
 const items = [{}, {}, {}];
 
-function x () {
-	for ( const item of items.children ) {
-		item.foo = 'bar';
-	}
+for ( const a of items.children ) {
+	a.foo = 'a';
 }
 
-x();
+for ( const b of items.children ) {
+	// do nothing
+}
 
-function y () {
-	for ( const item of items.children ) {
-		// do nothing
-	}
+let c;
+for ( c of items.children ) {
+	c.bar = 'c';
 }
 
-y();
+let d;
+for ( d of items.children ) {
+	// do nothing
+}
 
 assert.deepEqual( items, [
-	{ foo: 'bar' },
-	{ foo: 'bar' },
-	{ foo: 'bar' }
+	{ foo: 'a', bar: 'c' },
+	{ foo: 'a', bar: 'c' },
+	{ foo: 'a', bar: 'c' }
 ]);