When every import is unused, error on the entire import declaration (#22386)

Andy · web-flow · commit 1f7a5097fabc · 2018-03-07T14:42:09.000-08:00
* When every import is unused, error on the entire import declaration

* Code review

* Store key in map value
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
@@ -21682,18 +21682,58 @@ namespace ts {
 
         function checkUnusedModuleMembers(node: ModuleDeclaration | SourceFile): void {
             if (compilerOptions.noUnusedLocals && !(node.flags & NodeFlags.Ambient)) {
+                // Ideally we could use the ImportClause directly as a key, but must wait until we have full ES6 maps. So must store key along with value.
+                const unusedImports = createMap<[ImportClause, ImportedDeclaration[]]>();
                 node.locals.forEach(local => {
-                    if (!local.isReferenced && !local.exportSymbol) {
-                        for (const declaration of local.declarations) {
-                            if (!isAmbientModule(declaration)) {
-                                errorUnusedLocal(declaration, symbolName(local));
+                    if (local.isReferenced || local.exportSymbol) return;
+                    for (const declaration of local.declarations) {
+                        if (isAmbientModule(declaration)) continue;
+                        if (isImportedDeclaration(declaration)) {
+                            const importClause = importClauseFromImported(declaration);
+                            const key = String(getNodeId(importClause));
+                            const group = unusedImports.get(key);
+                            if (group) {
+                                group[1].push(declaration);
                             }
+                            else {
+                                unusedImports.set(key, [importClause, [declaration]]);
+                            }
+                        }
+                        else {
+                            errorUnusedLocal(declaration, symbolName(local));
                         }
                     }
                 });
+
+                unusedImports.forEach(([importClause, unuseds]) => {
+                    const importDecl = importClause.parent;
+                    if (forEachImportedDeclaration(importClause, d => !contains(unuseds, d))) {
+                        for (const unused of unuseds) errorUnusedLocal(unused, idText(unused.name));
+                    }
+                    else if (unuseds.length === 1) {
+                        error(importDecl, Diagnostics._0_is_declared_but_its_value_is_never_read, idText(first(unuseds).name));
+                    }
+                    else {
+                        error(importDecl, Diagnostics.All_imports_in_import_declaration_are_unused, showModuleSpecifier(importDecl));
+                    }
+                });
             }
         }
 
+        type ImportedDeclaration = ImportClause | ImportSpecifier | NamespaceImport;
+        function isImportedDeclaration(node: Node): node is ImportedDeclaration {
+            return node.kind === SyntaxKind.ImportClause || node.kind === SyntaxKind.ImportSpecifier || node.kind === SyntaxKind.NamespaceImport;
+        }
+        function importClauseFromImported(decl: ImportedDeclaration): ImportClause {
+            return decl.kind === SyntaxKind.ImportClause ? decl : decl.kind === SyntaxKind.NamespaceImport ? decl.parent : decl.parent.parent;
+        }
+
+        function forEachImportedDeclaration<T>(importClause: ImportClause, cb: (im: ImportedDeclaration) => T | undefined): T | undefined {
+            const { name: defaultName, namedBindings } = importClause;
+            return (defaultName && cb(importClause)) ||
+                namedBindings && (namedBindings.kind === SyntaxKind.NamespaceImport ? cb(namedBindings) : forEach(namedBindings.elements, cb));
+        }
+
         function checkBlock(node: Block) {
             // Grammar checking for SyntaxKind.Block
             if (node.kind === SyntaxKind.Block) {
diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json
@@ -3480,6 +3480,10 @@
         "category": "Message",
         "code": 6191
     },
+    "All imports in import declaration are unused.": {
+        "category": "Error",
+        "code": 6192
+    },
     "Variable '{0}' implicitly has an '{1}' type.": {
         "category": "Error",
         "code": 7005
@@ -3851,6 +3855,10 @@
         "category": "Message",
         "code": 90004
     },
+    "Remove import from '{0}'": {
+        "category": "Message",
+        "code": 90005
+    },
     "Implement interface '{0}'": {
         "category": "Message",
         "code": 90006
diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts
@@ -1204,7 +1204,7 @@ namespace ts {
             && (<PropertyAccessExpression | ElementAccessExpression>node).expression.kind === SyntaxKind.ThisKeyword;
     }
 
-   export function getEntityNameFromTypeNode(node: TypeNode): EntityNameOrEntityNameExpression {
+    export function getEntityNameFromTypeNode(node: TypeNode): EntityNameOrEntityNameExpression {
         switch (node.kind) {
             case SyntaxKind.TypeReference:
                 return (<TypeReferenceNode>node).typeName;
@@ -3728,6 +3728,10 @@ namespace ts {
     export function isUMDExportSymbol(symbol: Symbol) {
         return symbol && symbol.declarations && symbol.declarations[0] && isNamespaceExportDeclaration(symbol.declarations[0]);
     }
+
+    export function showModuleSpecifier({ moduleSpecifier }: ImportDeclaration): string {
+        return isStringLiteral(moduleSpecifier) ? moduleSpecifier.text : getTextOfNode(moduleSpecifier);
+    }
 }
 
 namespace ts {
diff --git a/src/services/codefixes/fixUnusedIdentifier.ts b/src/services/codefixes/fixUnusedIdentifier.ts
@@ -5,11 +5,19 @@ namespace ts.codefix {
     const errorCodes = [
         Diagnostics._0_is_declared_but_its_value_is_never_read.code,
         Diagnostics.Property_0_is_declared_but_its_value_is_never_read.code,
+        Diagnostics.All_imports_in_import_declaration_are_unused.code,
     ];
     registerCodeFix({
         errorCodes,
         getCodeActions(context) {
-            const { sourceFile } = context;
+            const { errorCode, sourceFile } = context;
+            const importDecl = tryGetFullImport(sourceFile, context.span.start);
+            if (importDecl) {
+                const description = formatStringFromArgs(getLocaleSpecificMessage(Diagnostics.Remove_import_from_0), [showModuleSpecifier(importDecl)]);
+                const changes = textChanges.ChangeTracker.with(context, t => t.deleteNode(sourceFile, importDecl));
+                return [{ description, changes, fixId: fixIdDelete }];
+            }
+
             const token = getToken(sourceFile, textSpanEnd(context.span));
             const result: CodeFixAction[] = [];
 
@@ -19,7 +27,7 @@ namespace ts.codefix {
                 result.push({ description, changes: deletion, fixId: fixIdDelete });
             }
 
-            const prefix = textChanges.ChangeTracker.with(context, t => tryPrefixDeclaration(t, context.errorCode, sourceFile, token));
+            const prefix = textChanges.ChangeTracker.with(context, t => tryPrefixDeclaration(t, errorCode, sourceFile, token));
             if (prefix.length) {
                 const description = formatStringFromArgs(getLocaleSpecificMessage(Diagnostics.Prefix_0_with_an_underscore), [token.getText()]);
                 result.push({ description, changes: prefix, fixId: fixIdPrefix });
@@ -38,14 +46,26 @@ namespace ts.codefix {
                     }
                     break;
                 case fixIdDelete:
-                    tryDeleteDeclaration(changes, sourceFile, token);
+                    const importDecl = tryGetFullImport(diag.file!, diag.start!);
+                    if (importDecl) {
+                        changes.deleteNode(sourceFile, importDecl);
+                    }
+                    else {
+                        tryDeleteDeclaration(changes, sourceFile, token);
+                    }
                     break;
                 default:
                     Debug.fail(JSON.stringify(context.fixId));
             }
         }),
     });
 
+    // Sometimes the diagnostic span is an entire ImportDeclaration, so we should remove the whole thing.
+    function tryGetFullImport(sourceFile: SourceFile, pos: number): ImportDeclaration | undefined {
+        const startToken = getTokenAtPosition(sourceFile, pos, /*includeJsDocComment*/ false);
+        return startToken.kind === SyntaxKind.ImportKeyword ? tryCast(startToken.parent, isImportDeclaration) : undefined;
+    }
+
     function getToken(sourceFile: SourceFile, pos: number): Node {
         const token = findPrecedingToken(pos, sourceFile);
         // this handles var ["computed"] = 12;
diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts
@@ -140,7 +140,7 @@ namespace ts.textChanges {
 
     export function getAdjustedStartPosition(sourceFile: SourceFile, node: Node, options: ConfigurableStart, position: Position) {
         if (options.useNonAdjustedStartPosition) {
-            return node.getStart();
+            return node.getStart(sourceFile);
         }
         const fullStart = node.getFullStart();
         const start = node.getStart(sourceFile);
diff --git a/tests/baselines/reference/extendsUntypedModule.errors.txt b/tests/baselines/reference/extendsUntypedModule.errors.txt
@@ -1,10 +1,10 @@
-/a.ts(2,8): error TS6133: 'Bar' is declared but its value is never read.
+/a.ts(2,1): error TS6133: 'Bar' is declared but its value is never read.
 
 
 ==== /a.ts (1 errors) ====
     import Foo from "foo";
     import Bar from "bar"; // error: unused
-           ~~~
+    ~~~~~~~~~~~~~~~~~~~~~~
 !!! error TS6133: 'Bar' is declared but its value is never read.
     export class A extends Foo { }
     
diff --git a/tests/baselines/reference/unusedImports1.errors.txt b/tests/baselines/reference/unusedImports1.errors.txt
@@ -1,4 +1,4 @@
-tests/cases/compiler/file2.ts(1,9): error TS6133: 'Calculator' is declared but its value is never read.
+tests/cases/compiler/file2.ts(1,1): error TS6133: 'Calculator' is declared but its value is never read.
 
 
 ==== tests/cases/compiler/file1.ts (0 errors) ====
@@ -8,5 +8,5 @@ tests/cases/compiler/file2.ts(1,9): error TS6133: 'Calculator' is declared but i
     
 ==== tests/cases/compiler/file2.ts (1 errors) ====
     import {Calculator} from "./file1"
-            ~~~~~~~~~~
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 !!! error TS6133: 'Calculator' is declared but its value is never read.
diff --git a/tests/baselines/reference/unusedImports12.errors.txt b/tests/baselines/reference/unusedImports12.errors.txt
@@ -1,21 +1,18 @@
-tests/cases/compiler/a.ts(1,10): error TS6133: 'Member' is declared but its value is never read.
-tests/cases/compiler/a.ts(2,8): error TS6133: 'd' is declared but its value is never read.
-tests/cases/compiler/a.ts(2,13): error TS6133: 'M' is declared but its value is never read.
-tests/cases/compiler/a.ts(3,8): error TS6133: 'ns' is declared but its value is never read.
+tests/cases/compiler/a.ts(1,1): error TS6133: 'Member' is declared but its value is never read.
+tests/cases/compiler/a.ts(2,1): error TS6192: All imports in import declaration are unused.
+tests/cases/compiler/a.ts(3,1): error TS6133: 'ns' is declared but its value is never read.
 tests/cases/compiler/a.ts(4,1): error TS6133: 'r' is declared but its value is never read.
 
 
-==== tests/cases/compiler/a.ts (5 errors) ====
+==== tests/cases/compiler/a.ts (4 errors) ====
     import { Member } from './b';
-             ~~~~~~
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 !!! error TS6133: 'Member' is declared but its value is never read.
     import d, { Member as M } from './b';
-           ~
-!!! error TS6133: 'd' is declared but its value is never read.
-                ~~~~~~~~~~~
-!!! error TS6133: 'M' is declared but its value is never read.
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+!!! error TS6192: All imports in import declaration are unused.
     import * as ns from './b';
-           ~~~~~~~
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~
 !!! error TS6133: 'ns' is declared but its value is never read.
     import r = require("./b");
     ~~~~~~~~
diff --git a/tests/baselines/reference/unusedImports2.errors.txt b/tests/baselines/reference/unusedImports2.errors.txt
@@ -1,4 +1,4 @@
-tests/cases/compiler/file2.ts(2,9): error TS6133: 'test' is declared but its value is never read.
+tests/cases/compiler/file2.ts(2,1): error TS6133: 'test' is declared but its value is never read.
 
 
 ==== tests/cases/compiler/file1.ts (0 errors) ====
@@ -13,7 +13,7 @@ tests/cases/compiler/file2.ts(2,9): error TS6133: 'test' is declared but its val
 ==== tests/cases/compiler/file2.ts (1 errors) ====
     import {Calculator} from "./file1"
     import {test} from "./file1"
-            ~~~~
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 !!! error TS6133: 'test' is declared but its value is never read.
     
     var x = new Calculator();
diff --git a/tests/baselines/reference/unusedImports6.errors.txt b/tests/baselines/reference/unusedImports6.errors.txt
@@ -1,4 +1,4 @@
-tests/cases/compiler/file2.ts(1,8): error TS6133: 'd' is declared but its value is never read.
+tests/cases/compiler/file2.ts(1,1): error TS6133: 'd' is declared but its value is never read.
 
 
 ==== tests/cases/compiler/file1.ts (0 errors) ====
@@ -16,7 +16,7 @@ tests/cases/compiler/file2.ts(1,8): error TS6133: 'd' is declared but its value
     
 ==== tests/cases/compiler/file2.ts (1 errors) ====
     import d from "./file1"
-           ~
+    ~~~~~~~~~~~~~~~~~~~~~~~
 !!! error TS6133: 'd' is declared but its value is never read.
     
     
diff --git a/tests/baselines/reference/unusedImports7.errors.txt b/tests/baselines/reference/unusedImports7.errors.txt
@@ -1,4 +1,4 @@
-tests/cases/compiler/file2.ts(1,8): error TS6133: 'n' is declared but its value is never read.
+tests/cases/compiler/file2.ts(1,1): error TS6133: 'n' is declared but its value is never read.
 
 
 ==== tests/cases/compiler/file1.ts (0 errors) ====
@@ -16,7 +16,7 @@ tests/cases/compiler/file2.ts(1,8): error TS6133: 'n' is declared but its value
     
 ==== tests/cases/compiler/file2.ts (1 errors) ====
     import * as n from "./file1"
-           ~~~~~~
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 !!! error TS6133: 'n' is declared but its value is never read.
     
     
diff --git a/tests/baselines/reference/unusedImports_entireImportDeclaration.errors.txt b/tests/baselines/reference/unusedImports_entireImportDeclaration.errors.txt
@@ -0,0 +1,36 @@
+/b.ts(1,1): error TS6192: All imports in import declaration are unused.
+/b.ts(2,1): error TS6192: All imports in import declaration are unused.
+/b.ts(4,14): error TS6133: 'a2' is declared but its value is never read.
+/b.ts(4,23): error TS6133: 'b2' is declared but its value is never read.
+/b.ts(6,12): error TS6133: 'ns2' is declared but its value is never read.
+/b.ts(8,8): error TS6133: 'd5' is declared but its value is never read.
+
+
+==== /a.ts (0 errors) ====
+    export const a = 0;
+    export const b = 0;
+    export default 0;
+    
+==== /b.ts (6 errors) ====
+    import d1, { a as a1, b as b1 } from "./a";
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+!!! error TS6192: All imports in import declaration are unused.
+    import d2, * as ns from "./a";
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+!!! error TS6192: All imports in import declaration are unused.
+    
+    import d3, { a as a2, b as b2 } from "./a";
+                 ~~~~~~~
+!!! error TS6133: 'a2' is declared but its value is never read.
+                          ~~~~~~~
+!!! error TS6133: 'b2' is declared but its value is never read.
+    d3;
+    import d4, * as ns2 from "./a";
+               ~~~~~~~~
+!!! error TS6133: 'ns2' is declared but its value is never read.
+    d4;
+    import d5, * as ns3 from "./a";
+           ~~
+!!! error TS6133: 'd5' is declared but its value is never read.
+    ns3;
+    
diff --git a/tests/baselines/reference/unusedImports_entireImportDeclaration.js b/tests/baselines/reference/unusedImports_entireImportDeclaration.js
@@ -0,0 +1,34 @@
+//// [tests/cases/compiler/unusedImports_entireImportDeclaration.ts] ////
+
+//// [a.ts]
+export const a = 0;
+export const b = 0;
+export default 0;
+
+//// [b.ts]
+import d1, { a as a1, b as b1 } from "./a";
+import d2, * as ns from "./a";
+
+import d3, { a as a2, b as b2 } from "./a";
+d3;
+import d4, * as ns2 from "./a";
+d4;
+import d5, * as ns3 from "./a";
+ns3;
+
+
+//// [a.js]
+"use strict";
+exports.__esModule = true;
+exports.a = 0;
+exports.b = 0;
+exports["default"] = 0;
+//// [b.js]
+"use strict";
+exports.__esModule = true;
+var a_1 = require("./a");
+a_1["default"];
+var a_2 = require("./a");
+a_2["default"];
+var ns3 = require("./a");
+ns3;
diff --git a/tests/baselines/reference/unusedImports_entireImportDeclaration.symbols b/tests/baselines/reference/unusedImports_entireImportDeclaration.symbols
@@ -0,0 +1,45 @@
+=== /a.ts ===
+export const a = 0;
+>a : Symbol(a, Decl(a.ts, 0, 12))
+
+export const b = 0;
+>b : Symbol(b, Decl(a.ts, 1, 12))
+
+export default 0;
+
+=== /b.ts ===
+import d1, { a as a1, b as b1 } from "./a";
+>d1 : Symbol(d1, Decl(b.ts, 0, 6))
+>a : Symbol(a1, Decl(b.ts, 0, 12))
+>a1 : Symbol(a1, Decl(b.ts, 0, 12))
+>b : Symbol(b1, Decl(b.ts, 0, 21))
+>b1 : Symbol(b1, Decl(b.ts, 0, 21))
+
+import d2, * as ns from "./a";
+>d2 : Symbol(d2, Decl(b.ts, 1, 6))
+>ns : Symbol(ns, Decl(b.ts, 1, 10))
+
+import d3, { a as a2, b as b2 } from "./a";
+>d3 : Symbol(d3, Decl(b.ts, 3, 6))
+>a : Symbol(a2, Decl(b.ts, 3, 12))
+>a2 : Symbol(a2, Decl(b.ts, 3, 12))
+>b : Symbol(b2, Decl(b.ts, 3, 21))
+>b2 : Symbol(b2, Decl(b.ts, 3, 21))
+
+d3;
+>d3 : Symbol(d3, Decl(b.ts, 3, 6))
+
+import d4, * as ns2 from "./a";
+>d4 : Symbol(d4, Decl(b.ts, 5, 6))
+>ns2 : Symbol(ns2, Decl(b.ts, 5, 10))
+
+d4;
+>d4 : Symbol(d4, Decl(b.ts, 5, 6))
+
+import d5, * as ns3 from "./a";
+>d5 : Symbol(d5, Decl(b.ts, 7, 6))
+>ns3 : Symbol(ns3, Decl(b.ts, 7, 10))
+
+ns3;
+>ns3 : Symbol(ns3, Decl(b.ts, 7, 10))
+
diff --git a/tests/baselines/reference/unusedImports_entireImportDeclaration.types b/tests/baselines/reference/unusedImports_entireImportDeclaration.types
diff --git a/tests/cases/compiler/unusedImports_entireImportDeclaration.ts b/tests/cases/compiler/unusedImports_entireImportDeclaration.ts
diff --git a/tests/cases/fourslash/unusedImportsFS_entireImportDeclaration.ts b/tests/cases/fourslash/unusedImportsFS_entireImportDeclaration.ts

Original file line number	Diff line number	Diff line change
`@@ -1204,7 +1204,7 @@ namespace ts {`
`1204`	`1204`	`&& (<PropertyAccessExpression \| ElementAccessExpression>node).expression.kind === SyntaxKind.ThisKeyword;`
`1205`	`1205`	`}`
`1206`	`1206`
`1207`		`- export function getEntityNameFromTypeNode(node: TypeNode): EntityNameOrEntityNameExpression {`
	`1207`	`+ export function getEntityNameFromTypeNode(node: TypeNode): EntityNameOrEntityNameExpression {`
`1208`	`1208`	`switch (node.kind) {`
`1209`	`1209`	`case SyntaxKind.TypeReference:`
`1210`	`1210`	`return (<TypeReferenceNode>node).typeName;`
`@@ -3728,6 +3728,10 @@ namespace ts {`
`3728`	`3728`	`export function isUMDExportSymbol(symbol: Symbol) {`
`3729`	`3729`	`return symbol && symbol.declarations && symbol.declarations[0] && isNamespaceExportDeclaration(symbol.declarations[0]);`
`3730`	`3730`	`}`
	`3731`	`+`
	`3732`	`+ export function showModuleSpecifier({ moduleSpecifier }: ImportDeclaration): string {`
	`3733`	`+ return isStringLiteral(moduleSpecifier) ? moduleSpecifier.text : getTextOfNode(moduleSpecifier);`
	`3734`	`+ }`
`3731`	`3735`	`}`
`3732`	`3736`
`3733`	`3737`	`namespace ts {`
Original file line number	Diff line number	Diff line change
`@@ -140,7 +140,7 @@ namespace ts.textChanges {`
`140`	`140`
`141`	`141`	`export function getAdjustedStartPosition(sourceFile: SourceFile, node: Node, options: ConfigurableStart, position: Position) {`
`142`	`142`	`if (options.useNonAdjustedStartPosition) {`
`143`		`- return node.getStart();`
	`143`	`+ return node.getStart(sourceFile);`
`144`	`144`	`}`
`145`	`145`	`const fullStart = node.getFullStart();`
`146`	`146`	`const start = node.getStart(sourceFile);`