the tests all pass! but the code is a mess

src/Declaration.js

@@ -31,8 +31,9 @@ export default class Declaration {
 		if ( reference.isReassignment ) this.isReassigned = true;
 	}
 
-	hasSideEffect () {
+	testForSideEffects ( strongDependencies ) {
-		return testForSideEffects( this.functionBody, this.statement.scope, this.statement );
+		if ( !this.statement ) return;
+		return testForSideEffects( this.functionBody, this.statement.scope, this.statement, strongDependencies );
 	}
 
 	render ( es6 ) {
@@ -77,13 +78,13 @@ export class SyntheticDefaultDeclaration {
 		this.original = declaration;
 	}
 
-	hasSideEffect () {
+	testForSideEffects ( strongDependencies ) {
 		if ( this.original ) {
-			return this.original.hasSideEffect();
+			return this.original.testForSideEffects( strongDependencies );
 		}
 
 		if ( /FunctionExpression/.test( this.node.declaration.type ) ) {
-			return testForSideEffects( this.node.declaration.body, this.statement.scope, this.statement );
+			return testForSideEffects( this.node.declaration.body, this.statement.scope, this.statement, strongDependencies );
 		}
 	}
 

src/Module.js

@@ -49,6 +49,8 @@ export default class Module {
 
 		this.declarations = blank();
 		this.analyse();
+
+		this.strongDependencies = [];
 	}
 
 	addExport ( statement ) {
@@ -154,7 +156,7 @@ export default class Module {
 			if ( statement.isImportDeclaration ) this.addImport( statement );
 			else if ( statement.isExportDeclaration ) this.addExport( statement );
 
-			statement.analyse();
+			statement.firstPass();
 
 			statement.scope.eachDeclaration( ( name, declaration ) => {
 				this.declarations[ name ] = declaration;
@@ -246,23 +248,29 @@ export default class Module {
 			}
 		});
 
-		// identify strong dependencies to break ties in case of cycles
+		this.strongDependencies.forEach( module => {
-		this.statements.forEach( statement => {
+			if ( module !== this ) {
-			statement.references.forEach( reference => {
+				strongDependencies[ module.id ] = module;
-				const declaration = reference.declaration;
+			}
-
-				if ( declaration && declaration.statement ) {
-					const module = declaration.statement.module;
-					if ( module === this ) return;
-
-					// TODO disregard function declarations
-					if ( reference.isImmediatelyUsed ) {
-						strongDependencies[ module.id ] = module;
-					}
-				}
-			});
 		});
 
+		// // identify strong dependencies to break ties in case of cycles
+		// this.statements.forEach( statement => {
+		// 	statement.references.forEach( reference => {
+		// 		const declaration = reference.declaration;
+		//
+		// 		if ( declaration && declaration.statement ) {
+		// 			const module = declaration.statement.module;
+		// 			if ( module === this ) return;
+		//
+		// 			// TODO disregard function declarations
+		// 			if ( reference.isImmediatelyUsed ) {
+		// 				strongDependencies[ module.id ] = module;
+		// 			}
+		// 		}
+		// 	});
+		// });
+
 		return { strongDependencies, weakDependencies };
 	}
 
@@ -287,9 +295,14 @@ export default class Module {
 	}
 
 	markAllSideEffects () {
+		// console.group( this.id )
+
 		this.statements.forEach( statement => {
-			statement.markSideEffect();
+			statement.secondPass( this.strongDependencies );
 		});
+
+		// console.log( this.strongDependencies.map(m=>m.id) )
+		// console.groupEnd()
 	}
 
 	namespace () {

src/Statement.js

@@ -2,31 +2,11 @@ import { walk } from 'estree-walker';
 import Scope from './ast/Scope.js';
 import attachScopes from './ast/attachScopes.js';
 import modifierNodes from './ast/modifierNodes.js';
+import isFunctionDeclaration from './ast/isFunctionDeclaration.js';
+import isReference from './ast/isReference.js';
 import getLocation from './utils/getLocation.js';
 import testForSideEffects from './utils/testForSideEffects.js';
 
-function isReference ( node, parent ) {
-	if ( node.type === 'MemberExpression' ) {
-		return !node.computed && isReference( node.object, node );
-	}
-
-	if ( node.type === 'Identifier' ) {
-		// TODO is this right?
-		if ( parent.type === 'MemberExpression' ) return parent.computed || node === parent.object;
-
-		// disregard the `bar` in { bar: foo }
-		if ( parent.type === 'Property' && node !== parent.value ) return false;
-
-		// disregard the `bar` in `class Foo { bar () {...} }`
-		if ( parent.type === 'MethodDefinition' ) return false;
-
-		// disregard the `bar` in `export { foo as bar }`
-		if ( parent.type === 'ExportSpecifier' && node !== parent.local ) return;
-
-		return true;
-	}
-}
-
 class Reference {
 	constructor ( node, scope, statement ) {
 		this.node = node;
@@ -69,9 +49,12 @@ export default class Statement {
 		this.isImportDeclaration = node.type === 'ImportDeclaration';
 		this.isExportDeclaration = /^Export/.test( node.type );
 		this.isReexportDeclaration = this.isExportDeclaration && !!node.source;
+
+		this.isFunctionDeclaration = isFunctionDeclaration( node ) ||
+			this.isExportDeclaration && isFunctionDeclaration( node.declaration );
 	}
 
-	analyse () {
+	firstPass () {
 		if ( this.isImportDeclaration ) return; // nothing to analyse
 
 		// attach scopes
@@ -173,13 +156,27 @@ export default class Statement {
 		});
 	}
 
-	markSideEffect () {
+	secondPass ( strongDependencies ) {
-		if ( this.isIncluded ) return;
+		// console.group( 'second pass: %s', this.toString() )
+		// console.log( 'this.isIncluded', this.isIncluded )
+		// console.log( 'this.isImportDeclaration', this.isImportDeclaration )
+		// console.log( 'this.isFunctionDeclaration', this.isFunctionDeclaration )
 
-		if ( testForSideEffects( this.node, this.scope, this ) ) {
+		if ( this.didSecondPassAlready || this.isImportDeclaration || this.isFunctionDeclaration ) {
+			// console.log( '>>> skipping' )
+			// console.groupEnd()
+			return;
+		}
+
+		this.didSecondPassAlready = true;
+
+		if ( testForSideEffects( this.node, this.scope, this, strongDependencies ) ) {
 			this.mark();
+			// console.groupEnd()
 			return true;
 		}
+
+		// console.groupEnd()
 	}
 
 	source () {

src/ast/isFunctionDeclaration.js

@@ -0,0 +1,6 @@
+export default function isFunctionDeclaration ( node ) {
+	if ( !node ) return false;
+	
+	return node.type === 'FunctionDeclaration' ||
+		( node.type === 'VariableDeclaration' && node.init && /FunctionExpression/.test( node.init.type ) );
+}

src/ast/isReference.js

@@ -0,0 +1,21 @@
+export default function isReference ( node, parent ) {
+	if ( node.type === 'MemberExpression' ) {
+		return !node.computed && isReference( node.object, node );
+	}
+
+	if ( node.type === 'Identifier' ) {
+		// TODO is this right?
+		if ( parent.type === 'MemberExpression' ) return parent.computed || node === parent.object;
+
+		// disregard the `bar` in { bar: foo }
+		if ( parent.type === 'Property' && node !== parent.value ) return false;
+
+		// disregard the `bar` in `class Foo { bar () {...} }`
+		if ( parent.type === 'MethodDefinition' ) return false;
+
+		// disregard the `bar` in `export { foo as bar }`
+		if ( parent.type === 'ExportSpecifier' && node !== parent.local ) return;
+
+		return true;
+	}
+}

src/utils/testForSideEffects.js

@@ -1,5 +1,7 @@
 import { walk } from 'estree-walker';
 import modifierNodes from '../ast/modifierNodes.js';
+import isFunctionDeclaration from '../ast/isFunctionDeclaration.js';
+import isReference from '../ast/isReference.js';
 import flatten from '../ast/flatten';
 
 let pureFunctions = {};
@@ -8,70 +10,75 @@ let pureFunctions = {};
 	'Object.keys'
 ].forEach( name => pureFunctions[ name ] = true );
 
-export default function testForSideEffects ( node, scope, statement ) {
+export default function testForSideEffects ( node, scope, statement, strongDependencies, force ) {
 	let hasSideEffect = false;
 
 	walk( node, {
-		enter ( node ) {
+		enter ( node, parent ) {
-			if ( hasSideEffect ) return this.skip();
+			if ( !force && /Function/.test( node.type ) ) return this.skip();
-			if ( /Function/.test( node.type ) ) return this.skip();
 
 			if ( node._scope ) scope = node._scope;
 
+			if ( isReference( node, parent ) ) {
+				const flattened = flatten( node );
+
+				if ( !scope.contains( flattened.name ) ) {
+					const declaration = statement.module.trace( flattened.name );
+					if ( declaration && !declaration.isExternal ) {
+						const module = declaration.module || declaration.statement.module; // TODO is this right?
+						if ( !~strongDependencies.indexOf( module ) ) strongDependencies.push( module );
+					}
+				}
+			}
+
 			// If this is a top-level call expression, or an assignment to a global,
 			// this statement will need to be marked
-			if ( node.type === 'NewExpression' ) {
+			else if ( node.type === 'NewExpression' ) {
 				hasSideEffect = true;
-				return this.skip();
 			}
 
-			if ( node.type === 'CallExpression' ) {
+			else if ( node.type === 'CallExpression' ) {
-				if ( node.callee.type === 'Identifier' && !scope.contains( node.callee.name ) ) {
+				if ( node.callee.type === 'Identifier' ) {
-					const declaration = statement.module.trace( node.callee.name );
+					const declaration = scope.findDeclaration( node.callee.name ) ||
+					                    statement.module.trace( node.callee.name );
 
 					if ( !declaration || declaration.isExternal ) {
 						// we're calling a global or an external function. Assume side-effects
 						hasSideEffect = true;
-						return this.skip();
 					}
 
 					// we're calling a function defined in this bundle
-					if ( declaration.hasSideEffect() ) {
+					else if ( declaration.testForSideEffects( strongDependencies ) ) {
 						hasSideEffect = true;
-						return this.skip();
 					}
 
 					// TODO does function mutate inputs that are needed?
-					return;
 				}
 
-				if ( node.callee.type === 'MemberExpression' ) {
+				else if ( node.callee.type === 'MemberExpression' ) {
 					const flattened = flatten( node.callee );
 
-					if ( !flattened ) {
+					if ( flattened ) {
+						// if we're calling e.g. Object.keys(thing), there are no side-effects
+						// TODO make pureFunctions configurable
+						const declaration = statement.module.trace( flattened.name );
+						if ( !!declaration || !pureFunctions[ flattened.keypath ] ) {
+							hasSideEffect = true;
+						}
+					} else {
 						// is not a keypath like `foo.bar.baz` – could be e.g.
 						// `(a || b).foo()`. Err on the side of caution
 						hasSideEffect = true;
-						return;
 					}
-
-					// if we're calling e.g. Object.keys(thing), there are no side-effects
-					// TODO make pureFunctions configurable
-					const declaration = statement.module.trace( flattened.name );
-					if ( !declaration && pureFunctions[ flattened.keypath ] ) return;
-
-					hasSideEffect = true;
-					return this.skip();
 				}
 
 				// otherwise we're probably dealing with a function expression
-				if ( testForSideEffects( node.callee, scope, statement ) ) {
+				else if ( testForSideEffects( node.callee, scope, statement, strongDependencies, true ) ) {
 					hasSideEffect = true;
-					return this.skip();
 				}
 			}
 
-			if ( node.type in modifierNodes ) {
+			else if ( node.type in modifierNodes ) {
 				let subject = node[ modifierNodes[ node.type ] ];
 				while ( subject.type === 'MemberExpression' ) subject = subject.object;
 
@@ -79,7 +86,6 @@ export default function testForSideEffects ( node, scope, statement ) {
 
 				if ( !declaration || declaration.isExternal || declaration.statement.isIncluded ) {
 					hasSideEffect = true;
-					return this.skip();
 				}
 			}
 		},