From b624d674a93c5186f72b052bd7a636f7f0b5f367 Mon Sep 17 00:00:00 2001
From: Rich-Harris <richard.a.harris@gmail.com>
Date: Thu, 22 Dec 2016 13:17:19 -0500
Subject: [PATCH] remove, and warn about, unused imports from external modules
 (#595)

---
 src/Bundle.js                                 | 25 +++++++++++++++++++
 src/Declaration.js                            |  4 +--
 src/finalisers/cjs.js                         |  7 +++++-
 src/finalisers/es.js                          |  1 +
 test/form/prefer-const/_expected/amd.js       |  3 ++-
 test/form/prefer-const/_expected/cjs.js       |  3 +--
 test/form/prefer-const/_expected/es.js        |  5 ++--
 test/form/prefer-const/_expected/iife.js      |  5 ++--
 test/form/prefer-const/_expected/umd.js       |  9 ++++---
 test/form/prefer-const/main.js                |  6 ++---
 test/form/unused-import/_config.js            |  5 ++++
 test/form/unused-import/_expected/amd.js      |  5 ++++
 test/form/unused-import/_expected/cjs.js      |  3 +++
 test/form/unused-import/_expected/es.js       |  1 +
 test/form/unused-import/_expected/iife.js     |  6 +++++
 test/form/unused-import/_expected/umd.js      |  9 +++++++
 test/form/unused-import/main.js               |  5 ++++
 .../function/class-methods-not-renamed/foo.js |  2 +-
 .../_config.js                                |  1 +
 .../main.js                                   |  3 ++-
 test/function/unused-import/_config.js        | 12 +++++++++
 test/function/unused-import/main.js           |  5 ++++
 22 files changed, 104 insertions(+), 21 deletions(-)
 create mode 100644 test/form/unused-import/_config.js
 create mode 100644 test/form/unused-import/_expected/amd.js
 create mode 100644 test/form/unused-import/_expected/cjs.js
 create mode 100644 test/form/unused-import/_expected/es.js
 create mode 100644 test/form/unused-import/_expected/iife.js
 create mode 100644 test/form/unused-import/_expected/umd.js
 create mode 100644 test/form/unused-import/main.js
 create mode 100644 test/function/unused-import/_config.js
 create mode 100644 test/function/unused-import/main.js

diff --git a/src/Bundle.js b/src/Bundle.js
index d98db39..ba2b4a5 100644
--- a/src/Bundle.js
+++ b/src/Bundle.js
@@ -175,6 +175,21 @@ export default class Bundle {
 
 				timeStart( 'phase 4' );
 
+				// while we're here, check for unused external imports
+				this.externalModules.forEach( module => {
+					const unused = Object.keys( module.declarations )
+						.filter( name => name !== '*' )
+						.filter( name => !module.declarations[ name ].activated );
+
+					if ( unused.length === 0 ) return;
+
+					const names = unused.length === 1 ?
+						`'${unused[0]}' is` :
+						`${unused.slice( 0, -1 ).map( name => `'${name}'` ).join( ', ' )} and '${unused.pop()}' are`;
+
+					this.onwarn( `${names} imported from external module '${module.id}' but never used` );
+				});
+
 				this.orderedModules = this.sort();
 				this.deconflict();
 
@@ -329,6 +344,16 @@ export default class Bundle {
 							this.externalModules.push( module );
 							this.moduleById.set( externalId, module );
 						}
+
+						const externalModule = this.moduleById.get( externalId );
+
+						// add external declarations so we can detect which are never used
+						Object.keys( module.imports ).forEach( name => {
+							const importDeclaration = module.imports[ name ];
+							if ( importDeclaration.source !== source ) return;
+
+							externalModule.traceExport( importDeclaration.name );
+						});
 					} else {
 						if ( resolvedId === module.id ) {
 							throw new Error( `A module cannot import itself (${resolvedId})` );
diff --git a/src/Declaration.js b/src/Declaration.js
index 88f5553..04670d3 100644
--- a/src/Declaration.js
+++ b/src/Declaration.js
@@ -99,13 +99,13 @@ export class ExternalDeclaration {
 		this.safeName = null;
 		this.isExternal = true;
 
-		this.activated = true;
+		this.activated = false;
 
 		this.isNamespace = name === '*';
 	}
 
 	activate () {
-		// noop
+		this.activated = true;
 	}
 
 	addReference ( reference ) {
diff --git a/src/finalisers/cjs.js b/src/finalisers/cjs.js
index 17f846f..17036e9 100644
--- a/src/finalisers/cjs.js
+++ b/src/finalisers/cjs.js
@@ -28,7 +28,12 @@ export default function cjs ( bundle, magicString, { exportMode, intro, outro },
 
 				return `${varOrConst} ${module.name} = _interopDefault(require('${module.path}'));`;
 			} else {
-				return `${varOrConst} ${module.name} = require('${module.path}');`;
+				const activated = Object.keys( module.declarations )
+					.filter( name => module.declarations[ name ].activated );
+
+				return activated.length ?
+					`${varOrConst} ${module.name} = require('${module.path}');` :
+					`require('${module.path}');`;
 			}
 		})
 		.join( '\n' );
diff --git a/src/finalisers/es.js b/src/finalisers/es.js
index e962845..b2123bb 100644
--- a/src/finalisers/es.js
+++ b/src/finalisers/es.js
@@ -11,6 +11,7 @@ export default function es ( bundle, magicString, { intro, outro } ) {
 			const specifiersList = [specifiers];
 			const importedNames = keys( module.declarations )
 				.filter( name => name !== '*' && name !== 'default' )
+				.filter( name => module.declarations[ name ].activated )
 				.map( name => {
 					const declaration = module.declarations[ name ];
 
diff --git a/test/form/prefer-const/_expected/amd.js b/test/form/prefer-const/_expected/amd.js
index a89dacd..c75359e 100644
--- a/test/form/prefer-const/_expected/amd.js
+++ b/test/form/prefer-const/_expected/amd.js
@@ -1,4 +1,4 @@
-define(['external', 'other', 'another'], function (external, other, another) { 'use strict';
+define(['other'], function (other) { 'use strict';
 
 	const a = 1;
 	const b = 2;
@@ -10,6 +10,7 @@ define(['external', 'other', 'another'], function (external, other, another) { '
 	});
 
 	console.log( Object.keys( namespace ) );
+	console.log( other.name );
 
 	const main = 42;
 
diff --git a/test/form/prefer-const/_expected/cjs.js b/test/form/prefer-const/_expected/cjs.js
index 5c2e4da..12a94be 100644
--- a/test/form/prefer-const/_expected/cjs.js
+++ b/test/form/prefer-const/_expected/cjs.js
@@ -1,8 +1,6 @@
 'use strict';
 
-const external = require('external');
 const other = require('other');
-const another = require('another');
 
 const a = 1;
 const b = 2;
@@ -14,6 +12,7 @@ const namespace = Object.freeze({
 });
 
 console.log( Object.keys( namespace ) );
+console.log( other.name );
 
 const main = 42;
 
diff --git a/test/form/prefer-const/_expected/es.js b/test/form/prefer-const/_expected/es.js
index 8f5a5c9..cc8162a 100644
--- a/test/form/prefer-const/_expected/es.js
+++ b/test/form/prefer-const/_expected/es.js
@@ -1,6 +1,4 @@
-import 'external';
-import 'other';
-import 'another';
+import { name } from 'other';
 
 const a = 1;
 const b = 2;
@@ -12,6 +10,7 @@ const namespace = Object.freeze({
 });
 
 console.log( Object.keys( namespace ) );
+console.log( name );
 
 const main = 42;
 
diff --git a/test/form/prefer-const/_expected/iife.js b/test/form/prefer-const/_expected/iife.js
index 9c66729..9e167a6 100644
--- a/test/form/prefer-const/_expected/iife.js
+++ b/test/form/prefer-const/_expected/iife.js
@@ -1,4 +1,4 @@
-const myBundle = (function (external,other,another) {
+const myBundle = (function (other) {
 	'use strict';
 
 	const a = 1;
@@ -11,9 +11,10 @@ const myBundle = (function (external,other,another) {
 	});
 
 	console.log( Object.keys( namespace ) );
+	console.log( other.name );
 
 	const main = 42;
 
 	return main;
 
-}(external,other,another));
+}(other));
diff --git a/test/form/prefer-const/_expected/umd.js b/test/form/prefer-const/_expected/umd.js
index 0ec9750..172cd79 100644
--- a/test/form/prefer-const/_expected/umd.js
+++ b/test/form/prefer-const/_expected/umd.js
@@ -1,8 +1,8 @@
 (function (global, factory) {
-	typeof exports === 'object' && typeof module !== 'undefined' ? module.exports = factory(require('external'), require('other'), require('another')) :
-	typeof define === 'function' && define.amd ? define(['external', 'other', 'another'], factory) :
-	(global.myBundle = factory(global.external,global.other,global.another));
-}(this, (function (external,other,another) { 'use strict';
+	typeof exports === 'object' && typeof module !== 'undefined' ? module.exports = factory(require('other')) :
+	typeof define === 'function' && define.amd ? define(['other'], factory) :
+	(global.myBundle = factory(global.other));
+}(this, (function (other) { 'use strict';
 
 	const a = 1;
 	const b = 2;
@@ -14,6 +14,7 @@
 	});
 
 	console.log( Object.keys( namespace ) );
+	console.log( other.name );
 
 	const main = 42;
 
diff --git a/test/form/prefer-const/main.js b/test/form/prefer-const/main.js
index e8cc563..9686046 100644
--- a/test/form/prefer-const/main.js
+++ b/test/form/prefer-const/main.js
@@ -1,9 +1,7 @@
-import external from 'external';
-import a from 'other';
-import { b } from 'other';
-import { another } from 'another';
+import { name } from 'other';
 import * as namespace from './namespace.js';
 
 console.log( Object.keys( namespace ) );
+console.log( name );
 
 export default 42;
diff --git a/test/form/unused-import/_config.js b/test/form/unused-import/_config.js
new file mode 100644
index 0000000..ddcd500
--- /dev/null
+++ b/test/form/unused-import/_config.js
@@ -0,0 +1,5 @@
+const assert = require( 'assert' );
+
+module.exports = {
+	description: 'excludes unused imports ([#595])'
+};
diff --git a/test/form/unused-import/_expected/amd.js b/test/form/unused-import/_expected/amd.js
new file mode 100644
index 0000000..cf0fc03
--- /dev/null
+++ b/test/form/unused-import/_expected/amd.js
@@ -0,0 +1,5 @@
+define(['external'], function (external) { 'use strict';
+
+
+
+});
diff --git a/test/form/unused-import/_expected/cjs.js b/test/form/unused-import/_expected/cjs.js
new file mode 100644
index 0000000..406151a
--- /dev/null
+++ b/test/form/unused-import/_expected/cjs.js
@@ -0,0 +1,3 @@
+'use strict';
+
+require('external');
diff --git a/test/form/unused-import/_expected/es.js b/test/form/unused-import/_expected/es.js
new file mode 100644
index 0000000..b2bc48d
--- /dev/null
+++ b/test/form/unused-import/_expected/es.js
@@ -0,0 +1 @@
+import 'external';
diff --git a/test/form/unused-import/_expected/iife.js b/test/form/unused-import/_expected/iife.js
new file mode 100644
index 0000000..143e690
--- /dev/null
+++ b/test/form/unused-import/_expected/iife.js
@@ -0,0 +1,6 @@
+(function (external) {
+	'use strict';
+
+
+
+}(external));
diff --git a/test/form/unused-import/_expected/umd.js b/test/form/unused-import/_expected/umd.js
new file mode 100644
index 0000000..6155f5f
--- /dev/null
+++ b/test/form/unused-import/_expected/umd.js
@@ -0,0 +1,9 @@
+(function (global, factory) {
+	typeof exports === 'object' && typeof module !== 'undefined' ? factory(require('external')) :
+	typeof define === 'function' && define.amd ? define(['external'], factory) :
+	(factory(global.external));
+}(this, (function (external) { 'use strict';
+
+
+
+})));
diff --git a/test/form/unused-import/main.js b/test/form/unused-import/main.js
new file mode 100644
index 0000000..ce3e0cb
--- /dev/null
+++ b/test/form/unused-import/main.js
@@ -0,0 +1,5 @@
+import { unused } from 'external';
+
+function alsoUnused () {
+	unused();
+}
diff --git a/test/function/class-methods-not-renamed/foo.js b/test/function/class-methods-not-renamed/foo.js
index f87c828..c6433ae 100644
--- a/test/function/class-methods-not-renamed/foo.js
+++ b/test/function/class-methods-not-renamed/foo.js
@@ -2,7 +2,7 @@ import { bar } from 'path'; // path, so the test doesn't fail for unrelated reas
 
 export default class Foo {
 	bar () {
-		if ( false ) return bar();
+		if ( Math.random() < 0 ) return bar();
 		return true;
 	}
 }
diff --git a/test/function/preserves-function-expression-names/_config.js b/test/function/preserves-function-expression-names/_config.js
index 044005a..14d8d3f 100644
--- a/test/function/preserves-function-expression-names/_config.js
+++ b/test/function/preserves-function-expression-names/_config.js
@@ -7,5 +7,6 @@ module.exports = {
 	},
 	exports ( exports ) {
 		assert.equal( exports.x.name, 'basename' );
+		assert.equal( exports.y, 'somefile.txt' );
 	}
 };
diff --git a/test/function/preserves-function-expression-names/main.js b/test/function/preserves-function-expression-names/main.js
index 49282a6..b651cd0 100644
--- a/test/function/preserves-function-expression-names/main.js
+++ b/test/function/preserves-function-expression-names/main.js
@@ -1,5 +1,6 @@
 import { basename } from 'path';
 
 var x = function basename () {};
+var y = basename( 'path/to/somefile.txt' );
 
-export { x };
+export { x, y };
diff --git a/test/function/unused-import/_config.js b/test/function/unused-import/_config.js
new file mode 100644
index 0000000..6515768
--- /dev/null
+++ b/test/function/unused-import/_config.js
@@ -0,0 +1,12 @@
+const assert = require( 'assert' );
+
+module.exports = {
+	description: 'warns on unused imports ([#595])',
+	warnings: warnings => {
+		assert.deepEqual( warnings, [
+			`Treating 'external' as external dependency`,
+			`'unused', 'notused' and 'neverused' are imported from external module 'external' but never used`,
+			`Generated an empty bundle`
+		]);
+	}
+};
diff --git a/test/function/unused-import/main.js b/test/function/unused-import/main.js
new file mode 100644
index 0000000..86039f6
--- /dev/null
+++ b/test/function/unused-import/main.js
@@ -0,0 +1,5 @@
+import { unused, notused, neverused as willnotuse } from 'external';
+
+function alsoUnused () {
+	unused();
+}