Merge pull request #917 from rollup/gh-870

Preserve effects in for-of and for-in statements

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 );
-	}
 }

src/ast/nodes/ForInStatement.js

@@ -0,0 +1,10 @@
+import Statement from './shared/Statement.js';
+import assignTo from './shared/assignTo.js';
+import { STRING } from '../values.js';
+
+export default class ForInStatement extends Statement {
+	initialise ( scope ) {
+		super.initialise( scope );
+		assignTo( this.left, scope, STRING );
+	}
+}

src/ast/nodes/ForOfStatement.js

@@ -0,0 +1,10 @@
+import Statement from './shared/Statement.js';
+import assignTo from './shared/assignTo.js';
+import { UNKNOWN } from '../values.js';
+
+export default class ForOfStatement extends Statement {
+	initialise ( scope ) {
+		super.initialise( scope );
+		assignTo( this.left, scope, UNKNOWN );
+	}
+}

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();
 

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...
+	}
+}

src/ast/nodes/index.js

@@ -11,6 +11,8 @@ import ExportAllDeclaration from './ExportAllDeclaration.js';
 import ExportDefaultDeclaration from './ExportDefaultDeclaration.js';
 import ExportNamedDeclaration from './ExportNamedDeclaration.js';
 import ExpressionStatement from './ExpressionStatement.js';
+import ForInStatement from './ForInStatement.js';
+import ForOfStatement from './ForOfStatement.js';
 import FunctionDeclaration from './FunctionDeclaration.js';
 import FunctionExpression from './FunctionExpression.js';
 import Identifier from './Identifier.js';
@@ -24,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';
@@ -43,6 +46,8 @@ export default {
 	ExportDefaultDeclaration,
 	ExportNamedDeclaration,
 	ExpressionStatement,
+	ForInStatement,
+	ForOfStatement,
 	FunctionDeclaration,
 	FunctionExpression,
 	Identifier,
@@ -56,6 +61,7 @@ export default {
 	ReturnStatement,
 	TemplateLiteral,
 	ThisExpression,
+	ThrowStatement,
 	UnaryExpression,
 	UpdateExpression,
 	VariableDeclarator,

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 );
+	}
+}

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 );
+				}
+			}
+		}
+	}
+}

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)'
+}

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' }
+	]);
+
+});

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' }
+]);

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' }
+]);

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' }
+	]);
+
+}());

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' }
+	]);
+
+})));

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' }
+]);

test/form/effect-in-for-of-loop/_config.js

@@ -0,0 +1,3 @@
+module.exports = {
+	description: 'includes effects in for-of loop (#870)'
+}

test/form/effect-in-for-of-loop/_expected/amd.js

@@ -0,0 +1,20 @@
+define(function () { 'use strict';
+
+	const items = [{}, {}, {}];
+
+	for ( const a of items.children ) {
+		a.foo = 'a';
+	}
+
+	let c;
+	for ( c of items.children ) {
+		c.bar = 'c';
+	}
+
+	assert.deepEqual( items, [
+		{ foo: 'a', bar: 'c' },
+		{ foo: 'a', bar: 'c' },
+		{ foo: 'a', bar: 'c' }
+	]);
+
+});

test/form/effect-in-for-of-loop/_expected/cjs.js

@@ -0,0 +1,18 @@
+'use strict';
+
+const items = [{}, {}, {}];
+
+for ( const a of items.children ) {
+	a.foo = 'a';
+}
+
+let c;
+for ( c of items.children ) {
+	c.bar = 'c';
+}
+
+assert.deepEqual( items, [
+	{ foo: 'a', bar: 'c' },
+	{ foo: 'a', bar: 'c' },
+	{ foo: 'a', bar: 'c' }
+]);

test/form/effect-in-for-of-loop/_expected/es.js

@@ -0,0 +1,16 @@
+const items = [{}, {}, {}];
+
+for ( const a of items.children ) {
+	a.foo = 'a';
+}
+
+let c;
+for ( c of items.children ) {
+	c.bar = 'c';
+}
+
+assert.deepEqual( items, [
+	{ foo: 'a', bar: 'c' },
+	{ foo: 'a', bar: 'c' },
+	{ foo: 'a', bar: 'c' }
+]);

test/form/effect-in-for-of-loop/_expected/iife.js

@@ -0,0 +1,21 @@
+(function () {
+	'use strict';
+
+	const items = [{}, {}, {}];
+
+	for ( const a of items.children ) {
+		a.foo = 'a';
+	}
+
+	let c;
+	for ( c of items.children ) {
+		c.bar = 'c';
+	}
+
+	assert.deepEqual( items, [
+		{ foo: 'a', bar: 'c' },
+		{ foo: 'a', bar: 'c' },
+		{ foo: 'a', bar: 'c' }
+	]);
+
+}());

test/form/effect-in-for-of-loop/_expected/umd.js

@@ -0,0 +1,24 @@
+(function (global, factory) {
+	typeof exports === 'object' && typeof module !== 'undefined' ? factory() :
+	typeof define === 'function' && define.amd ? define(factory) :
+	(factory());
+}(this, (function () { 'use strict';
+
+	const items = [{}, {}, {}];
+
+	for ( const a of items.children ) {
+		a.foo = 'a';
+	}
+
+	let c;
+	for ( c of items.children ) {
+		c.bar = 'c';
+	}
+
+	assert.deepEqual( items, [
+		{ foo: 'a', bar: 'c' },
+		{ foo: 'a', bar: 'c' },
+		{ foo: 'a', bar: 'c' }
+	]);
+
+})));

test/form/effect-in-for-of-loop/main.js

@@ -0,0 +1,25 @@
+const items = [{}, {}, {}];
+
+for ( const a of items.children ) {
+	a.foo = 'a';
+}
+
+for ( const b of items.children ) {
+	// do nothing
+}
+
+let c;
+for ( c of items.children ) {
+	c.bar = 'c';
+}
+
+let d;
+for ( d of items.children ) {
+	// do nothing
+}
+
+assert.deepEqual( items, [
+	{ foo: 'a', bar: 'c' },
+	{ foo: 'a', bar: 'c' },
+	{ foo: 'a', bar: 'c' }
+]);