Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
4 commits merged into from
Mar 7, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 32 additions & 5 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I find the forEach code below a little hard to follow. Another way to express this might be to build a multi-map from import clauses to the unused declarations they contain. Then, for each import clause in the map, you could either report a single diagnostic or a list of diagnostics according to whether or not the import clause contained a declaration not in the corresponding unused list. I don't feel strong about this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, helped me remove the count variable.

}
else {
errorUnusedLocal(declaration, symbolName(local));
}
}
});

while (unusedImports.length) {
const decl = unusedImports.pop()!;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why recompute this? Isn't it the map key?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately we don't support non-string keys for maps for compatibility with ES5. See shimMap.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could always make the map value be a pair though...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must have been thinking of another context. This makes sense, given that limitation.

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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. if there is only one import e.g. default or one named import, the other error is superior. i would suggests such extending the span and using the error message with he name of the import.

  2. I think the new error message is inaccurate. we just know that the import declaration is not needed, but there may be other import declarations for the same module that are still in use. so possibly remove the name of the module from the error message and say Import declaration unused or all imports in import declaration are unused

}
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<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) {
Expand Down
4 changes: 4 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 5 additions & 1 deletion src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1199,7 +1199,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;
Expand Down Expand Up @@ -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 {
Expand Down
25 changes: 22 additions & 3 deletions src/services/codefixes/fixUnusedIdentifier.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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 });
Expand All @@ -38,14 +47,24 @@ 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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which adjacent trivia does this delete?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it removes leading but not trailing trivia by default.

}
else {
tryDeleteDeclaration(changes, sourceFile, token);
}
break;
default:
Debug.fail(JSON.stringify(context.fixId));
}
}),
});

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;
Expand Down
6 changes: 3 additions & 3 deletions tests/baselines/reference/extendsUntypedModule.errors.txt
Original file line number Diff line number Diff line change
@@ -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) ====
Expand Down
6 changes: 3 additions & 3 deletions tests/baselines/reference/unusedImports1.errors.txt
Original file line number Diff line number Diff line change
@@ -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) ====
Expand All @@ -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.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!!! error TS6192: No import of './file1' is used.
23 changes: 10 additions & 13 deletions tests/baselines/reference/unusedImports12.errors.txt
Original file line number Diff line number Diff line change
@@ -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.
Expand Down
6 changes: 3 additions & 3 deletions tests/baselines/reference/unusedImports2.errors.txt
Original file line number Diff line number Diff line change
@@ -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) ====
Expand All @@ -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();
6 changes: 3 additions & 3 deletions tests/baselines/reference/unusedImports6.errors.txt
Original file line number Diff line number Diff line change
@@ -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) ====
Expand All @@ -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.



Expand Down
6 changes: 3 additions & 3 deletions tests/baselines/reference/unusedImports7.errors.txt
Original file line number Diff line number Diff line change
@@ -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) ====
Expand All @@ -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.


Original file line number Diff line number Diff line change
@@ -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;

34 changes: 34 additions & 0 deletions tests/baselines/reference/unusedImports_entireImportDeclaration.js
Original file line number Diff line number Diff line change
@@ -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;
Original file line number Diff line number Diff line change
@@ -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))

Loading