Merge pull request #94 from rollup/gh-91

Prevent function expressions being incorrectly renamed

src/Bundle.js

@@ -290,13 +290,13 @@ export default class Bundle {
 		//
 		// This doesn't apply if the bundle is exported as ES6!
 		let allBundleExports = blank();
-		let isVarDeclaration = blank();
+		let isReassignedVarDeclaration = blank();
 		let varExports = blank();
 		let getterExports = [];
 
 		this.orderedModules.forEach( module => {
-			module.varDeclarations.forEach( name => {
-				isVarDeclaration[ module.replacements[ name ] || name ] = true;
+			module.reassignments.forEach( name => {
+				isReassignedVarDeclaration[ module.replacements[ name ] || name ] = true;
 			});
 		});
 
@@ -306,7 +306,7 @@ export default class Bundle {
 				.forEach( name => {
 					const canonicalName = this.traceExport( this.entryModule, name );
 
-					if ( isVarDeclaration[ canonicalName ] ) {
+					if ( isReassignedVarDeclaration[ canonicalName ] ) {
 						varExports[ name ] = true;
 
 						// if the same binding is exported multiple ways, we need to

src/Module.js

@@ -63,7 +63,7 @@ export default class Module {
 
 		this.replacements = blank();
 
-		this.varDeclarations = [];
+		this.reassignments = [];
 
 		this.marked = blank();
 		this.definitions = blank();
@@ -206,20 +206,35 @@ export default class Module {
 				this.definitions[ name ] = statement;
 			});
 
-			statement.scope.varDeclarations.forEach( name => {
-				this.varDeclarations.push( name );
-			});
-
 			keys( statement.modifies ).forEach( name => {
 				( this.modifications[ name ] || ( this.modifications[ name ] = [] ) ).push( statement );
 			});
 		});
 
+		// discover variables that are reassigned inside function
+		// bodies, so we can keep bindings live, e.g.
+		//
+		//   export var count = 0;
+		//   export function incr () { count += 1 }
+		let reassigned = blank();
+		this.statements.forEach( statement => {
+			keys( statement.reassigns ).forEach( name => {
+				reassigned[ name ] = true;
+			});
+		});
+
 		// if names are referenced that are neither defined nor imported
 		// in this module, we assume that they're globals
 		this.statements.forEach( statement => {
 			if ( statement.isReexportDeclaration ) return;
 
+			// while we're here, mark reassignments
+			statement.scope.varDeclarations.forEach( name => {
+				if ( reassigned[ name ] && !~this.reassignments.indexOf( name ) ) {
+					this.reassignments.push( name );
+				}
+			});
+
 			keys( statement.dependsOn ).forEach( name => {
 				if ( !this.definitions[ name ] && !this.imports[ name ] ) {
 					this.bundle.assumedGlobals[ name ] = true;

src/Statement.js

@@ -29,6 +29,8 @@ export default class Statement {
 		this.dependsOn = blank();
 		this.stronglyDependsOn = blank();
 
+		this.reassigns = blank();
+
 		this.isIncluded = false;
 
 		this.isImportDeclaration = node.type === 'ImportDeclaration';
@@ -46,29 +48,23 @@ export default class Statement {
 				let newScope;
 
 				switch ( node.type ) {
-					case 'FunctionExpression':
 					case 'FunctionDeclaration':
-					case 'ArrowFunctionExpression':
-						if ( node.type === 'FunctionDeclaration' ) {
-							scope.addDeclaration( node.id.name, node, false );
-						}
-
-						newScope = new Scope({
-							parent: scope,
-							params: node.params, // TODO rest params?
-							block: false
-						});
-
-						// named function expressions - the name is considered
-						// part of the function's scope
-						if ( node.type === 'FunctionExpression' && node.id ) {
-							newScope.addDeclaration( node.id.name, node, false );
-						}
-
-						break;
+						scope.addDeclaration( node.id.name, node, false );
 
 					case 'BlockStatement':
-						if ( !/Function/.test( parent.type ) ) {
+						if ( parent && /Function/.test( parent.type ) ) {
+							newScope = new Scope({
+								parent: scope,
+								block: false,
+								params: parent.params
+							});
+
+							// named function expressions - the name is considered
+							// part of the function's scope
+							if ( parent.type === 'FunctionExpression' && parent.id ) {
+								newScope.addDeclaration( parent.id.name, parent, false );
+							}
+						} else {
 							newScope = new Scope({
 								parent: scope,
 								block: true
@@ -133,27 +129,19 @@ export default class Statement {
 		if ( !this.isImportDeclaration ) {
 			walk( this.node, {
 				enter: ( node, parent ) => {
-					if ( node._scope ) {
-						if ( !scope.isBlockScope ) {
-							if ( !isIife( node, parent ) ) readDepth += 1;
-							if ( isFunctionDeclaration( node, parent ) ) writeDepth += 1;
-						}
+					if ( isFunctionDeclaration( node, parent ) ) writeDepth += 1;
+					if ( /Function/.test( node.type ) && !isIife( node, parent ) ) readDepth += 1;
 
-						scope = node._scope;
-					}
+					if ( node._scope ) scope = node._scope;
 
 					this.checkForReads( scope, node, parent, !readDepth );
 					this.checkForWrites( scope, node, writeDepth );
 				},
 				leave: ( node, parent ) => {
-					if ( node._scope ) {
-						if ( !scope.isBlockScope ) {
-							if ( !isIife( node, parent ) ) readDepth -= 1;
-							if ( isFunctionDeclaration( node, parent ) ) writeDepth -= 1;
-						}
+					if ( isFunctionDeclaration( node, parent ) ) writeDepth -= 1;
+					if ( /Function/.test( node.type ) && !isIife( node, parent ) ) readDepth -= 1;
 
-						scope = scope.parent;
-					}
+					if ( node._scope ) scope = scope.parent;
 				}
 			});
 		}
@@ -226,6 +214,17 @@ export default class Statement {
 						this.module.exports.default.isModified = true;
 					}
 				}
+
+				// we track updates/reassignments to variables, to know whether we
+				// need to rewrite it later from `foo` to `exports.foo` to keep
+				// bindings live
+				if (
+					depth === 0 &&
+					writeDepth > 0 &&
+					!scope.contains( node.name )
+				) {
+					this.reassigns[ node.name ] = true;
+				}
 			}
 
 			// we only care about writes that happen a) at the top level,
@@ -348,11 +347,6 @@ export default class Statement {
 					let newNames = blank();
 					let hasReplacements;
 
-					// special case = function foo ( foo ) {...}
-					if ( node.id && names[ node.id.name ] && scope.declarations[ node.id.name ] ) {
-						magicString.overwrite( node.id.start, node.id.end, names[ node.id.name ] );
-					}
-
 					keys( names ).forEach( name => {
 						if ( !scope.declarations[ name ] ) {
 							newNames[ name ] = names[ name ];
@@ -393,6 +387,8 @@ export default class Statement {
 				if ( parent.type === 'MemberExpression' && !parent.computed && node !== parent.object ) return;
 				if ( parent.type === 'Property' && node !== parent.value ) return;
 				if ( parent.type === 'MethodDefinition' && node === parent.key ) return;
+				if ( parent.type === 'FunctionExpression' ) return;
+				if ( /Function/.test( parent.type ) && ~parent.params.indexOf( node ) ) return;
 				// TODO others...?
 
 				// all other identifiers should be overwritten

test/form/exported-empty-vars/_config.js

@@ -1,6 +0,0 @@
-module.exports = {
-	description: 'removes empty var declarations that are exported',
-	options: {
-		moduleName: 'myBundle'
-	}
-};

test/form/exported-empty-vars/_expected/amd.js

@@ -1,8 +0,0 @@
-define(['exports'], function (exports) { 'use strict';
-
-	exports.foo = 42;
-
-	exports.bar = 43;
-	exports.baz = 44;
-
-});

test/form/exported-empty-vars/_expected/cjs.js

@@ -1,6 +0,0 @@
-'use strict';
-
-exports.foo = 42;
-
-exports.bar = 43;
-exports.baz = 44;

test/form/exported-empty-vars/_expected/es6.js

@@ -1,9 +0,0 @@
-var foo;
-foo = 42;
-
-var bar;
-var baz;
-bar = 43;
-baz = 44;
-
-export { foo, bar, baz };

test/form/exported-empty-vars/_expected/iife.js

@@ -1,8 +0,0 @@
-(function (exports) { 'use strict';
-
-	exports.foo = 42;
-
-	exports.bar = 43;
-	exports.baz = 44;
-
-})((this.myBundle = {}));

test/form/exported-empty-vars/bar.js

@@ -1,6 +0,0 @@
-var bar, baz;
-
-bar = 43;
-baz = 44;
-
-export { bar, baz };

test/form/exported-empty-vars/foo.js

@@ -1,2 +0,0 @@
-export var foo;
-foo = 42;

test/form/exported-empty-vars/main.js

@@ -1,2 +0,0 @@
-export { foo } from './foo';
-export { bar, baz } from './bar';

test/form/exports-at-end-if-possible/_config.js

@@ -0,0 +1,7 @@
+module.exports = {
+	description: 'exports variables at end, if possible',
+	options: {
+		moduleName: 'myBundle'
+	},
+	// solo: true
+};

test/form/exports-at-end-if-possible/_expected/amd.js

@@ -0,0 +1,11 @@
+define(['exports'], function (exports) { 'use strict';
+
+	var FOO = 'foo';
+
+	console.log( FOO );
+	console.log( FOO );
+	console.log( FOO );
+
+	exports.FOO = FOO;
+
+});

test/form/exports-at-end-if-possible/_expected/cjs.js

@@ -0,0 +1,9 @@
+'use strict';
+
+var FOO = 'foo';
+
+console.log( FOO );
+console.log( FOO );
+console.log( FOO );
+
+exports.FOO = FOO;

test/form/exports-at-end-if-possible/_expected/es6.js

@@ -0,0 +1,7 @@
+var FOO = 'foo';
+
+console.log( FOO );
+console.log( FOO );
+console.log( FOO );
+
+export { FOO };

test/form/exports-at-end-if-possible/_expected/iife.js

@@ -0,0 +1,11 @@
+(function (exports) { 'use strict';
+
+	var FOO = 'foo';
+
+	console.log( FOO );
+	console.log( FOO );
+	console.log( FOO );
+
+	exports.FOO = FOO;
+
+})((this.myBundle = {}));

test/form/exported-empty-vars/_expected/umd.js → test/form/exports-at-end-if-possible/_expected/umd.js

@@ -4,9 +4,12 @@
 	factory((global.myBundle = {}));
 }(this, function (exports) { 'use strict';
 
-	exports.foo = 42;
+	var FOO = 'foo';
 
-	exports.bar = 43;
-	exports.baz = 44;
+	console.log( FOO );
+	console.log( FOO );
+	console.log( FOO );
+
+	exports.FOO = FOO;
 
 }));

test/form/exports-at-end-if-possible/main.js

@@ -0,0 +1,5 @@
+export var FOO = 'foo';
+
+console.log( FOO );
+console.log( FOO );
+console.log( FOO );

test/form/multiple-exports/_expected/amd.js

@@ -1,6 +1,9 @@
 define(['exports'], function (exports) { 'use strict';
 
-	exports.foo = 1;
-	exports.bar = 2;
+	var foo = 1;
+	var bar = 2;
+
+	exports.foo = foo;
+	exports.bar = bar;
 
 });

test/form/multiple-exports/_expected/cjs.js

@@ -1,4 +1,7 @@
 'use strict';
 
-exports.foo = 1;
-exports.bar = 2;
+var foo = 1;
+var bar = 2;
+
+exports.foo = foo;
+exports.bar = bar;

test/form/multiple-exports/_expected/iife.js

@@ -1,6 +1,9 @@
 (function (exports) { 'use strict';
 
-	exports.foo = 1;
-	exports.bar = 2;
+	var foo = 1;
+	var bar = 2;
+
+	exports.foo = foo;
+	exports.bar = bar;
 
 })((this.myBundle = {}));

test/form/multiple-exports/_expected/umd.js

@@ -4,7 +4,10 @@
 	factory((global.myBundle = {}));
 }(this, function (exports) { 'use strict';
 
-	exports.foo = 1;
-	exports.bar = 2;
+	var foo = 1;
+	var bar = 2;
+
+	exports.foo = foo;
+	exports.bar = bar;
 
 }));

test/form/preserves-comments-after-imports/_expected/amd.js

@@ -4,6 +4,8 @@ define(['exports'], function (exports) { 'use strict';
 	var number = 5;
 
 	/** A comment for obj */
-	exports.obj = { number };
+	var obj = { number };
+
+	exports.obj = obj;
 
 });

test/form/preserves-comments-after-imports/_expected/cjs.js

@@ -4,4 +4,6 @@
 var number = 5;
 
 /** A comment for obj */
-exports.obj = { number };
+var obj = { number };
+
+exports.obj = obj;

test/form/preserves-comments-after-imports/_expected/iife.js

@@ -4,6 +4,8 @@
 	var number = 5;
 
 	/** A comment for obj */
-	exports.obj = { number };
+	var obj = { number };
+
+	exports.obj = obj;
 
 })((this.myBundle = {}));

test/form/preserves-comments-after-imports/_expected/umd.js

@@ -8,6 +8,8 @@
 	var number = 5;
 
 	/** A comment for obj */
-	exports.obj = { number };
+	var obj = { number };
+
+	exports.obj = obj;
 
 }));

test/function/assignment-to-exports/_config.js

@@ -6,5 +6,6 @@ module.exports = {
 		assert.equal( exports.count, 0 );
 		exports.incr();
 		assert.equal( exports.count, 1 );
-	}
+	},
+	// solo: true
 };

test/function/functions-renamed-correctly/_config.js

@@ -0,0 +1,3 @@
+module.exports = {
+	description: 'renames function expression IDs correctly'
+};

test/function/functions-renamed-correctly/after.js

@@ -0,0 +1,5 @@
+function x () {
+	return 'after';
+}
+
+export { x as after };

test/function/functions-renamed-correctly/before.js

@@ -0,0 +1,5 @@
+function x () {
+	return 'before';
+}
+
+export { x as before };

test/function/functions-renamed-correctly/factorial.js

@@ -0,0 +1,7 @@
+var x = (function () {
+	return function x ( num ) {
+		return num <= 2 ? num : num * x( num - 1 );
+	};
+})();
+
+export { x };

test/function/functions-renamed-correctly/main.js

@@ -0,0 +1,7 @@
+import { before } from './before';
+import { x } from './factorial';
+import { after } from './after';
+
+before(); // before and after ensure x is renamed
+assert.equal( x( 5 ), 120 );
+after();