From bda91408ddb9915cb6d1acba94cbb93b21314af7 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Wed, 7 Mar 2018 11:46:06 -0800 Subject: [PATCH 1/3] When every import is unused, error on the entire import declaration --- src/compiler/checker.ts | 37 +++++++++++++-- src/compiler/diagnosticMessages.json | 4 ++ src/compiler/utilities.ts | 6 ++- src/services/codefixes/fixUnusedIdentifier.ts | 25 ++++++++-- .../reference/extendsUntypedModule.errors.txt | 6 +-- .../reference/unusedImports1.errors.txt | 6 +-- .../reference/unusedImports12.errors.txt | 23 ++++----- .../reference/unusedImports2.errors.txt | 6 +-- .../reference/unusedImports6.errors.txt | 6 +-- .../reference/unusedImports7.errors.txt | 6 +-- ...Imports_entireImportDeclaration.errors.txt | 36 ++++++++++++++ .../unusedImports_entireImportDeclaration.js | 34 ++++++++++++++ ...sedImports_entireImportDeclaration.symbols | 45 ++++++++++++++++++ ...nusedImports_entireImportDeclaration.types | 47 +++++++++++++++++++ .../unusedImports_entireImportDeclaration.ts | 17 +++++++ ...unusedImportsFS_entireImportDeclaration.ts | 10 ++++ 16 files changed, 277 insertions(+), 37 deletions(-) create mode 100644 tests/baselines/reference/unusedImports_entireImportDeclaration.errors.txt create mode 100644 tests/baselines/reference/unusedImports_entireImportDeclaration.js create mode 100644 tests/baselines/reference/unusedImports_entireImportDeclaration.symbols create mode 100644 tests/baselines/reference/unusedImports_entireImportDeclaration.types create mode 100644 tests/cases/compiler/unusedImports_entireImportDeclaration.ts create mode 100644 tests/cases/fourslash/unusedImportsFS_entireImportDeclaration.ts diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 2d9d0d189449d..5a7c2af4c7b95 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -21682,18 +21682,45 @@ namespace ts { function checkUnusedModuleMembers(node: ModuleDeclaration | SourceFile): void { if (compilerOptions.noUnusedLocals && !(node.flags & NodeFlags.Ambient)) { + const unusedImports: 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)) { + unusedImports.push(declaration); + } + else { + errorUnusedLocal(declaration, symbolName(local)); } } }); + + while (unusedImports.length) { + const decl = unusedImports.pop()!; + const importClause = decl.kind === SyntaxKind.ImportClause ? decl : decl.kind === SyntaxKind.NamespaceImport ? decl.parent : decl.parent.parent; + if (!forEachImportedDeclaration(importClause, d => d !== decl && !contains(unusedImports, d))) { + forEachImportedDeclaration(importClause, d => unorderedRemoveItem(unusedImports, d)); + error(importClause.parent, Diagnostics.No_import_of_0_is_used, showModuleSpecifier(importClause.parent)); + } + else { + errorUnusedLocal(decl, idText(decl.name)); + } + } } } + 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 forEachImportedDeclaration(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 index d8545e015290d..056d9694461ab 100644 --- a/src/compiler/diagnosticMessages.json +++ b/src/compiler/diagnosticMessages.json @@ -3480,6 +3480,10 @@ "category": "Message", "code": 6191 }, + "No import of '{0}' is used.": { + "category": "Error", + "code": 6192 + }, "Variable '{0}' implicitly has an '{1}' type.": { "category": "Error", "code": 7005 diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index b6099c3959cb5..2135351aa2a6c 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -1199,7 +1199,7 @@ namespace ts { && (node).expression.kind === SyntaxKind.ThisKeyword; } - export function getEntityNameFromTypeNode(node: TypeNode): EntityNameOrEntityNameExpression { + export function getEntityNameFromTypeNode(node: TypeNode): EntityNameOrEntityNameExpression { switch (node.kind) { case SyntaxKind.TypeReference: return (node).typeName; @@ -3723,6 +3723,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 index de053c2fe8f3b..669f4dfcc223e 100644 --- a/src/services/codefixes/fixUnusedIdentifier.ts +++ b/src/services/codefixes/fixUnusedIdentifier.ts @@ -5,12 +5,21 @@ 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.No_import_of_0_is_used.code, ]; registerCodeFix({ errorCodes, getCodeActions(context) { - const { sourceFile } = context; + const { errorCode, sourceFile } = context; const token = getToken(sourceFile, context.span.start); + + if (errorCode === Diagnostics.No_import_of_0_is_used.code) { + const decl = getImportDeclarationAtErrorLocation(token); + const description = formatStringFromArgs(getLocaleSpecificMessage(Diagnostics.Remove_declaration_for_Colon_0), [showModuleSpecifier(decl)]); + const changes = textChanges.ChangeTracker.with(context, t => t.deleteNode(sourceFile, decl)); + return [{ description, changes, fixId: fixIdDelete }]; + } + const result: CodeFixAction[] = []; const deletion = textChanges.ChangeTracker.with(context, t => tryDeleteDeclaration(t, sourceFile, token)); @@ -19,7 +28,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,7 +47,12 @@ namespace ts.codefix { } break; case fixIdDelete: - tryDeleteDeclaration(changes, sourceFile, token); + if (diag.code === Diagnostics.No_import_of_0_is_used.code) { + changes.deleteNode(sourceFile, getImportDeclarationAtErrorLocation(token)); + } + else { + tryDeleteDeclaration(changes, sourceFile, token); + } break; default: Debug.fail(JSON.stringify(context.fixId)); @@ -46,6 +60,11 @@ namespace ts.codefix { }), }); + function getImportDeclarationAtErrorLocation(token: Node): ImportDeclaration { + Debug.assert(token.kind === SyntaxKind.ImportKeyword); + return cast(token.parent, isImportDeclaration); + } + function getToken(sourceFile: SourceFile, pos: number): Node { const token = getTokenAtPosition(sourceFile, pos, /*includeJsDocComment*/ false); // this handles var ["computed"] = 12; diff --git a/tests/baselines/reference/extendsUntypedModule.errors.txt b/tests/baselines/reference/extendsUntypedModule.errors.txt index 8df9e8a1c3671..77f75f410bb08 100644 --- a/tests/baselines/reference/extendsUntypedModule.errors.txt +++ b/tests/baselines/reference/extendsUntypedModule.errors.txt @@ -1,11 +1,11 @@ -/a.ts(2,8): error TS6133: 'Bar' is declared but its value is never read. +/a.ts(2,1): error TS6192: No import of 'bar' is used. ==== /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. + ~~~~~~~~~~~~~~~~~~~~~~ +!!! error TS6192: No import of 'bar' is used. export class A extends Foo { } ==== /node_modules/foo/index.js (0 errors) ==== diff --git a/tests/baselines/reference/unusedImports1.errors.txt b/tests/baselines/reference/unusedImports1.errors.txt index 36963513146a9..f7729b9530992 100644 --- 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 TS6192: No import of './file1' is used. ==== 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. \ No newline at end of file + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +!!! error TS6192: No import of './file1' is used. \ No newline at end of file diff --git a/tests/baselines/reference/unusedImports12.errors.txt b/tests/baselines/reference/unusedImports12.errors.txt index 56df4edc5e3d7..b3d57b4fdc78d 100644 --- a/tests/baselines/reference/unusedImports12.errors.txt +++ b/tests/baselines/reference/unusedImports12.errors.txt @@ -1,22 +1,19 @@ -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,23): error TS6133: 'M' is declared but its value is never read. -tests/cases/compiler/a.ts(3,13): error TS6133: 'ns' is declared but its value is never read. +tests/cases/compiler/a.ts(1,1): error TS6192: No import of './b' is used. +tests/cases/compiler/a.ts(2,1): error TS6192: No import of './b' is used. +tests/cases/compiler/a.ts(3,1): error TS6192: No import of './b' is used. tests/cases/compiler/a.ts(4,8): 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. + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +!!! error TS6192: No import of './b' is used. 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: No import of './b' is used. import * as ns from './b'; - ~~ -!!! error TS6133: 'ns' is declared but its value is never read. + ~~~~~~~~~~~~~~~~~~~~~~~~~~ +!!! error TS6192: No import of './b' is used. import r = require("./b"); ~ !!! error TS6133: 'r' is declared but its value is never read. diff --git a/tests/baselines/reference/unusedImports2.errors.txt b/tests/baselines/reference/unusedImports2.errors.txt index c037eea2f512b..e2d5d788be08c 100644 --- 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 TS6192: No import of './file1' is used. ==== tests/cases/compiler/file1.ts (0 errors) ==== @@ -13,8 +13,8 @@ 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. + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +!!! error TS6192: No import of './file1' is used. var x = new Calculator(); x.handleChar(); \ No newline at end of file diff --git a/tests/baselines/reference/unusedImports6.errors.txt b/tests/baselines/reference/unusedImports6.errors.txt index 28b2d1a2875c1..286fdc32dc417 100644 --- 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 TS6192: No import of './file1' is used. ==== tests/cases/compiler/file1.ts (0 errors) ==== @@ -16,8 +16,8 @@ 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. + ~~~~~~~~~~~~~~~~~~~~~~~ +!!! error TS6192: No import of './file1' is used. diff --git a/tests/baselines/reference/unusedImports7.errors.txt b/tests/baselines/reference/unusedImports7.errors.txt index 9482a2c6f479b..e6d484f346eeb 100644 --- a/tests/baselines/reference/unusedImports7.errors.txt +++ b/tests/baselines/reference/unusedImports7.errors.txt @@ -1,4 +1,4 @@ -tests/cases/compiler/file2.ts(1,13): error TS6133: 'n' is declared but its value is never read. +tests/cases/compiler/file2.ts(1,1): error TS6192: No import of './file1' is used. ==== tests/cases/compiler/file1.ts (0 errors) ==== @@ -16,7 +16,7 @@ tests/cases/compiler/file2.ts(1,13): 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. + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +!!! error TS6192: No import of './file1' is used. \ No newline at end of file diff --git a/tests/baselines/reference/unusedImports_entireImportDeclaration.errors.txt b/tests/baselines/reference/unusedImports_entireImportDeclaration.errors.txt new file mode 100644 index 0000000000000..2444ffe99df03 --- /dev/null +++ b/tests/baselines/reference/unusedImports_entireImportDeclaration.errors.txt @@ -0,0 +1,36 @@ +/b.ts(1,1): error TS6192: No import of './a' is used. +/b.ts(2,1): error TS6192: No import of './a' is used. +/b.ts(4,19): error TS6133: 'a2' is declared but its value is never read. +/b.ts(4,28): error TS6133: 'b2' is declared but its value is never read. +/b.ts(6,17): 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: No import of './a' is used. + import d2, * as ns from "./a"; + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +!!! error TS6192: No import of './a' is used. + + 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; + \ No newline at end of file diff --git a/tests/baselines/reference/unusedImports_entireImportDeclaration.js b/tests/baselines/reference/unusedImports_entireImportDeclaration.js new file mode 100644 index 0000000000000..9bc77f50f7a6c --- /dev/null +++ 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 new file mode 100644 index 0000000000000..bd02fe90992b4 --- /dev/null +++ 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 new file mode 100644 index 0000000000000..82cacfd53146a --- /dev/null +++ b/tests/baselines/reference/unusedImports_entireImportDeclaration.types @@ -0,0 +1,47 @@ +=== /a.ts === +export const a = 0; +>a : 0 +>0 : 0 + +export const b = 0; +>b : 0 +>0 : 0 + +export default 0; + +=== /b.ts === +import d1, { a as a1, b as b1 } from "./a"; +>d1 : 0 +>a : 0 +>a1 : 0 +>b : 0 +>b1 : 0 + +import d2, * as ns from "./a"; +>d2 : 0 +>ns : typeof ns + +import d3, { a as a2, b as b2 } from "./a"; +>d3 : 0 +>a : 0 +>a2 : 0 +>b : 0 +>b2 : 0 + +d3; +>d3 : 0 + +import d4, * as ns2 from "./a"; +>d4 : 0 +>ns2 : typeof ns + +d4; +>d4 : 0 + +import d5, * as ns3 from "./a"; +>d5 : 0 +>ns3 : typeof ns + +ns3; +>ns3 : typeof ns + diff --git a/tests/cases/compiler/unusedImports_entireImportDeclaration.ts b/tests/cases/compiler/unusedImports_entireImportDeclaration.ts new file mode 100644 index 0000000000000..fa7fcb7f59632 --- /dev/null +++ b/tests/cases/compiler/unusedImports_entireImportDeclaration.ts @@ -0,0 +1,17 @@ +// @noUnusedLocals: true + +// @Filename: /a.ts +export const a = 0; +export const b = 0; +export default 0; + +// @Filename: /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; diff --git a/tests/cases/fourslash/unusedImportsFS_entireImportDeclaration.ts b/tests/cases/fourslash/unusedImportsFS_entireImportDeclaration.ts new file mode 100644 index 0000000000000..468b479002076 --- /dev/null +++ b/tests/cases/fourslash/unusedImportsFS_entireImportDeclaration.ts @@ -0,0 +1,10 @@ +/// + +// @noUnusedLocals: true +// @Filename: /a.ts +////import a, { b } from "mod"; + +verify.codeFix({ + description: "Remove declaration for: 'mod'", + newFileContent: "", +}); From dffc496e2f97b9df37c1e74b18d7aba357cacf75 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Wed, 7 Mar 2018 14:11:02 -0800 Subject: [PATCH 2/3] Code review --- src/compiler/checker.ts | 25 +++++++++++-------- src/compiler/diagnosticMessages.json | 6 ++++- src/services/codefixes/fixUnusedIdentifier.ts | 18 ++++++------- src/services/textChanges.ts | 2 +- .../reference/extendsUntypedModule.errors.txt | 4 +-- .../reference/unusedImports1.errors.txt | 4 +-- .../reference/unusedImports12.errors.txt | 12 ++++----- .../reference/unusedImports2.errors.txt | 4 +-- .../reference/unusedImports6.errors.txt | 4 +-- .../reference/unusedImports7.errors.txt | 4 +-- ...Imports_entireImportDeclaration.errors.txt | 8 +++--- ...unusedImportsFS_entireImportDeclaration.ts | 7 +++--- 12 files changed, 52 insertions(+), 46 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 5a7c2af4c7b95..2b3c76b26e75d 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -21682,13 +21682,13 @@ namespace ts { function checkUnusedModuleMembers(node: ModuleDeclaration | SourceFile): void { if (compilerOptions.noUnusedLocals && !(node.flags & NodeFlags.Ambient)) { - const unusedImports: ImportedDeclaration[] = []; + const unusedImports = createMultiMap(); node.locals.forEach(local => { if (local.isReferenced || local.exportSymbol) return; for (const declaration of local.declarations) { if (isAmbientModule(declaration)) continue; if (isImportedDeclaration(declaration)) { - unusedImports.push(declaration); + unusedImports.add(String(getNodeId(importClauseFromImported(declaration))), declaration); } else { errorUnusedLocal(declaration, symbolName(local)); @@ -21696,17 +21696,19 @@ namespace ts { } }); - while (unusedImports.length) { - const decl = unusedImports.pop()!; - const importClause = decl.kind === SyntaxKind.ImportClause ? decl : decl.kind === SyntaxKind.NamespaceImport ? decl.parent : decl.parent.parent; - if (!forEachImportedDeclaration(importClause, d => d !== decl && !contains(unusedImports, d))) { - forEachImportedDeclaration(importClause, d => unorderedRemoveItem(unusedImports, d)); - error(importClause.parent, Diagnostics.No_import_of_0_is_used, showModuleSpecifier(importClause.parent)); + unusedImports.forEach(unuseds => { + const importClause = importClauseFromImported(first(unuseds)); // others will have the same clause + 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 { - errorUnusedLocal(decl, idText(decl.name)); + error(importDecl, Diagnostics.All_imports_in_import_declaration_are_unused, showModuleSpecifier(importDecl)); } - } + }); } } @@ -21714,6 +21716,9 @@ namespace ts { 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(importClause: ImportClause, cb: (im: ImportedDeclaration) => T | undefined): T | undefined { const { name: defaultName, namedBindings } = importClause; diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json index 056d9694461ab..9f95a66711b75 100644 --- a/src/compiler/diagnosticMessages.json +++ b/src/compiler/diagnosticMessages.json @@ -3480,7 +3480,7 @@ "category": "Message", "code": 6191 }, - "No import of '{0}' is used.": { + "All imports in import declaration are unused.": { "category": "Error", "code": 6192 }, @@ -3855,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/services/codefixes/fixUnusedIdentifier.ts b/src/services/codefixes/fixUnusedIdentifier.ts index 669f4dfcc223e..d9e10eae5ded1 100644 --- a/src/services/codefixes/fixUnusedIdentifier.ts +++ b/src/services/codefixes/fixUnusedIdentifier.ts @@ -5,7 +5,7 @@ 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.No_import_of_0_is_used.code, + Diagnostics.All_imports_in_import_declaration_are_unused.code, ]; registerCodeFix({ errorCodes, @@ -13,9 +13,10 @@ namespace ts.codefix { const { errorCode, sourceFile } = context; const token = getToken(sourceFile, context.span.start); - if (errorCode === Diagnostics.No_import_of_0_is_used.code) { - const decl = getImportDeclarationAtErrorLocation(token); - const description = formatStringFromArgs(getLocaleSpecificMessage(Diagnostics.Remove_declaration_for_Colon_0), [showModuleSpecifier(decl)]); + // For single imports, we will use the whole span but still use "'{0}' is declared but its value is never read." + if (token.kind === SyntaxKind.ImportKeyword) { + const decl = cast(token.parent, isImportDeclaration); + const description = formatStringFromArgs(getLocaleSpecificMessage(Diagnostics.Remove_import_from_0), [showModuleSpecifier(decl)]); const changes = textChanges.ChangeTracker.with(context, t => t.deleteNode(sourceFile, decl)); return [{ description, changes, fixId: fixIdDelete }]; } @@ -47,8 +48,8 @@ namespace ts.codefix { } break; case fixIdDelete: - if (diag.code === Diagnostics.No_import_of_0_is_used.code) { - changes.deleteNode(sourceFile, getImportDeclarationAtErrorLocation(token)); + if (token.kind === SyntaxKind.ImportKeyword) { + changes.deleteNode(sourceFile, cast(token.parent, isImportDeclaration)); } else { tryDeleteDeclaration(changes, sourceFile, token); @@ -60,11 +61,6 @@ namespace ts.codefix { }), }); - function getImportDeclarationAtErrorLocation(token: Node): ImportDeclaration { - Debug.assert(token.kind === SyntaxKind.ImportKeyword); - return cast(token.parent, isImportDeclaration); - } - function getToken(sourceFile: SourceFile, pos: number): Node { const token = getTokenAtPosition(sourceFile, pos, /*includeJsDocComment*/ false); // this handles var ["computed"] = 12; diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 0e75158c4d1c3..8121576a02994 100644 --- 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 index 77f75f410bb08..aded38736e831 100644 --- a/tests/baselines/reference/extendsUntypedModule.errors.txt +++ b/tests/baselines/reference/extendsUntypedModule.errors.txt @@ -1,11 +1,11 @@ -/a.ts(2,1): error TS6192: No import of 'bar' is used. +/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 TS6192: No import of 'bar' is used. +!!! error TS6133: 'Bar' is declared but its value is never read. export class A extends Foo { } ==== /node_modules/foo/index.js (0 errors) ==== diff --git a/tests/baselines/reference/unusedImports1.errors.txt b/tests/baselines/reference/unusedImports1.errors.txt index f7729b9530992..d329b1e89a88c 100644 --- a/tests/baselines/reference/unusedImports1.errors.txt +++ b/tests/baselines/reference/unusedImports1.errors.txt @@ -1,4 +1,4 @@ -tests/cases/compiler/file2.ts(1,1): error TS6192: No import of './file1' is used. +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) ==== @@ -9,4 +9,4 @@ tests/cases/compiler/file2.ts(1,1): error TS6192: No import of './file1' is used ==== tests/cases/compiler/file2.ts (1 errors) ==== import {Calculator} from "./file1" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -!!! error TS6192: No import of './file1' is used. \ No newline at end of file +!!! error TS6133: 'Calculator' is declared but its value is never read. \ No newline at end of file diff --git a/tests/baselines/reference/unusedImports12.errors.txt b/tests/baselines/reference/unusedImports12.errors.txt index b3d57b4fdc78d..e3b6da00db879 100644 --- a/tests/baselines/reference/unusedImports12.errors.txt +++ b/tests/baselines/reference/unusedImports12.errors.txt @@ -1,19 +1,19 @@ -tests/cases/compiler/a.ts(1,1): error TS6192: No import of './b' is used. -tests/cases/compiler/a.ts(2,1): error TS6192: No import of './b' is used. -tests/cases/compiler/a.ts(3,1): error TS6192: No import of './b' is used. +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,8): error TS6133: 'r' is declared but its value is never read. ==== tests/cases/compiler/a.ts (4 errors) ==== import { Member } from './b'; ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -!!! error TS6192: No import of './b' is used. +!!! error TS6133: 'Member' is declared but its value is never read. import d, { Member as M } from './b'; ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -!!! error TS6192: No import of './b' is used. +!!! error TS6192: All imports in import declaration are unused. import * as ns from './b'; ~~~~~~~~~~~~~~~~~~~~~~~~~~ -!!! error TS6192: No import of './b' is used. +!!! error TS6133: 'ns' is declared but its value is never read. import r = require("./b"); ~ !!! error TS6133: 'r' is declared but its value is never read. diff --git a/tests/baselines/reference/unusedImports2.errors.txt b/tests/baselines/reference/unusedImports2.errors.txt index e2d5d788be08c..65d067f34f1c7 100644 --- a/tests/baselines/reference/unusedImports2.errors.txt +++ b/tests/baselines/reference/unusedImports2.errors.txt @@ -1,4 +1,4 @@ -tests/cases/compiler/file2.ts(2,1): error TS6192: No import of './file1' is used. +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) ==== @@ -14,7 +14,7 @@ tests/cases/compiler/file2.ts(2,1): error TS6192: No import of './file1' is used import {Calculator} from "./file1" import {test} from "./file1" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -!!! error TS6192: No import of './file1' is used. +!!! error TS6133: 'test' is declared but its value is never read. var x = new Calculator(); x.handleChar(); \ No newline at end of file diff --git a/tests/baselines/reference/unusedImports6.errors.txt b/tests/baselines/reference/unusedImports6.errors.txt index 286fdc32dc417..a211dec9b7024 100644 --- a/tests/baselines/reference/unusedImports6.errors.txt +++ b/tests/baselines/reference/unusedImports6.errors.txt @@ -1,4 +1,4 @@ -tests/cases/compiler/file2.ts(1,1): error TS6192: No import of './file1' is used. +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) ==== @@ -17,7 +17,7 @@ tests/cases/compiler/file2.ts(1,1): error TS6192: No import of './file1' is used ==== tests/cases/compiler/file2.ts (1 errors) ==== import d from "./file1" ~~~~~~~~~~~~~~~~~~~~~~~ -!!! error TS6192: No import of './file1' is used. +!!! 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 index e6d484f346eeb..52411e3403a7a 100644 --- a/tests/baselines/reference/unusedImports7.errors.txt +++ b/tests/baselines/reference/unusedImports7.errors.txt @@ -1,4 +1,4 @@ -tests/cases/compiler/file2.ts(1,1): error TS6192: No import of './file1' is used. +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) ==== @@ -17,6 +17,6 @@ tests/cases/compiler/file2.ts(1,1): error TS6192: No import of './file1' is used ==== tests/cases/compiler/file2.ts (1 errors) ==== import * as n from "./file1" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -!!! error TS6192: No import of './file1' is used. +!!! error TS6133: 'n' is declared but its value is never read. \ No newline at end of file diff --git a/tests/baselines/reference/unusedImports_entireImportDeclaration.errors.txt b/tests/baselines/reference/unusedImports_entireImportDeclaration.errors.txt index 2444ffe99df03..d0418446be3ec 100644 --- a/tests/baselines/reference/unusedImports_entireImportDeclaration.errors.txt +++ b/tests/baselines/reference/unusedImports_entireImportDeclaration.errors.txt @@ -1,5 +1,5 @@ -/b.ts(1,1): error TS6192: No import of './a' is used. -/b.ts(2,1): error TS6192: No import of './a' is used. +/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,19): error TS6133: 'a2' is declared but its value is never read. /b.ts(4,28): error TS6133: 'b2' is declared but its value is never read. /b.ts(6,17): error TS6133: 'ns2' is declared but its value is never read. @@ -14,10 +14,10 @@ ==== /b.ts (6 errors) ==== import d1, { a as a1, b as b1 } from "./a"; ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -!!! error TS6192: No import of './a' is used. +!!! error TS6192: All imports in import declaration are unused. import d2, * as ns from "./a"; ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -!!! error TS6192: No import of './a' is used. +!!! error TS6192: All imports in import declaration are unused. import d3, { a as a2, b as b2 } from "./a"; ~~ diff --git a/tests/cases/fourslash/unusedImportsFS_entireImportDeclaration.ts b/tests/cases/fourslash/unusedImportsFS_entireImportDeclaration.ts index 468b479002076..a75ef81b1ed39 100644 --- a/tests/cases/fourslash/unusedImportsFS_entireImportDeclaration.ts +++ b/tests/cases/fourslash/unusedImportsFS_entireImportDeclaration.ts @@ -2,9 +2,10 @@ // @noUnusedLocals: true // @Filename: /a.ts -////import a, { b } from "mod"; +////// leading trivia +////import a, { b } from "mod"; // trailing trivia verify.codeFix({ - description: "Remove declaration for: 'mod'", - newFileContent: "", + description: "Remove import from 'mod'", + newFileContent: " // trailing trivia", }); From d0e288f6af16f7ffadaf789b0b58fe5aba54a6e3 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Wed, 7 Mar 2018 14:32:48 -0800 Subject: [PATCH 3/3] Store key in map value --- src/compiler/checker.ts | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 024b06027a764..6d8d084acc580 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -21682,13 +21682,22 @@ namespace ts { function checkUnusedModuleMembers(node: ModuleDeclaration | SourceFile): void { if (compilerOptions.noUnusedLocals && !(node.flags & NodeFlags.Ambient)) { - const unusedImports = createMultiMap(); + // 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) return; for (const declaration of local.declarations) { if (isAmbientModule(declaration)) continue; if (isImportedDeclaration(declaration)) { - unusedImports.add(String(getNodeId(importClauseFromImported(declaration))), 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)); @@ -21696,8 +21705,7 @@ namespace ts { } }); - unusedImports.forEach(unuseds => { - const importClause = importClauseFromImported(first(unuseds)); // others will have the same clause + 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));