Skip to content

Commit 1f7a509

Browse files
author
Andy
authored
When every import is unused, error on the entire import declaration (#22386)
* When every import is unused, error on the entire import declaration * Code review * Store key in map value
1 parent 963acb3 commit 1f7a509

17 files changed

+289
-30
lines changed

‎src/compiler/checker.ts

+44-4
Original file line numberDiff line numberDiff line change
@@ -21682,18 +21682,58 @@ namespace ts {
2168221682

2168321683
function checkUnusedModuleMembers(node: ModuleDeclaration | SourceFile): void {
2168421684
if (compilerOptions.noUnusedLocals && !(node.flags & NodeFlags.Ambient)) {
21685+
// 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.
21686+
const unusedImports = createMap<[ImportClause, ImportedDeclaration[]]>();
2168521687
node.locals.forEach(local => {
21686-
if (!local.isReferenced && !local.exportSymbol) {
21687-
for (const declaration of local.declarations) {
21688-
if (!isAmbientModule(declaration)) {
21689-
errorUnusedLocal(declaration, symbolName(local));
21688+
if (local.isReferenced || local.exportSymbol) return;
21689+
for (const declaration of local.declarations) {
21690+
if (isAmbientModule(declaration)) continue;
21691+
if (isImportedDeclaration(declaration)) {
21692+
const importClause = importClauseFromImported(declaration);
21693+
const key = String(getNodeId(importClause));
21694+
const group = unusedImports.get(key);
21695+
if (group) {
21696+
group[1].push(declaration);
2169021697
}
21698+
else {
21699+
unusedImports.set(key, [importClause, [declaration]]);
21700+
}
21701+
}
21702+
else {
21703+
errorUnusedLocal(declaration, symbolName(local));
2169121704
}
2169221705
}
2169321706
});
21707+
21708+
unusedImports.forEach(([importClause, unuseds]) => {
21709+
const importDecl = importClause.parent;
21710+
if (forEachImportedDeclaration(importClause, d => !contains(unuseds, d))) {
21711+
for (const unused of unuseds) errorUnusedLocal(unused, idText(unused.name));
21712+
}
21713+
else if (unuseds.length === 1) {
21714+
error(importDecl, Diagnostics._0_is_declared_but_its_value_is_never_read, idText(first(unuseds).name));
21715+
}
21716+
else {
21717+
error(importDecl, Diagnostics.All_imports_in_import_declaration_are_unused, showModuleSpecifier(importDecl));
21718+
}
21719+
});
2169421720
}
2169521721
}
2169621722

21723+
type ImportedDeclaration = ImportClause | ImportSpecifier | NamespaceImport;
21724+
function isImportedDeclaration(node: Node): node is ImportedDeclaration {
21725+
return node.kind === SyntaxKind.ImportClause || node.kind === SyntaxKind.ImportSpecifier || node.kind === SyntaxKind.NamespaceImport;
21726+
}
21727+
function importClauseFromImported(decl: ImportedDeclaration): ImportClause {
21728+
return decl.kind === SyntaxKind.ImportClause ? decl : decl.kind === SyntaxKind.NamespaceImport ? decl.parent : decl.parent.parent;
21729+
}
21730+
21731+
function forEachImportedDeclaration<T>(importClause: ImportClause, cb: (im: ImportedDeclaration) => T | undefined): T | undefined {
21732+
const { name: defaultName, namedBindings } = importClause;
21733+
return (defaultName && cb(importClause)) ||
21734+
namedBindings && (namedBindings.kind === SyntaxKind.NamespaceImport ? cb(namedBindings) : forEach(namedBindings.elements, cb));
21735+
}
21736+
2169721737
function checkBlock(node: Block) {
2169821738
// Grammar checking for SyntaxKind.Block
2169921739
if (node.kind === SyntaxKind.Block) {

‎src/compiler/diagnosticMessages.json

+8
Original file line numberDiff line numberDiff line change
@@ -3480,6 +3480,10 @@
34803480
"category": "Message",
34813481
"code": 6191
34823482
},
3483+
"All imports in import declaration are unused.": {
3484+
"category": "Error",
3485+
"code": 6192
3486+
},
34833487
"Variable '{0}' implicitly has an '{1}' type.": {
34843488
"category": "Error",
34853489
"code": 7005
@@ -3851,6 +3855,10 @@
38513855
"category": "Message",
38523856
"code": 90004
38533857
},
3858+
"Remove import from '{0}'": {
3859+
"category": "Message",
3860+
"code": 90005
3861+
},
38543862
"Implement interface '{0}'": {
38553863
"category": "Message",
38563864
"code": 90006

‎src/compiler/utilities.ts

+5-1
Original file line numberDiff line numberDiff line change
@@ -1204,7 +1204,7 @@ namespace ts {
12041204
&& (<PropertyAccessExpression | ElementAccessExpression>node).expression.kind === SyntaxKind.ThisKeyword;
12051205
}
12061206

1207-
export function getEntityNameFromTypeNode(node: TypeNode): EntityNameOrEntityNameExpression {
1207+
export function getEntityNameFromTypeNode(node: TypeNode): EntityNameOrEntityNameExpression {
12081208
switch (node.kind) {
12091209
case SyntaxKind.TypeReference:
12101210
return (<TypeReferenceNode>node).typeName;
@@ -3728,6 +3728,10 @@ namespace ts {
37283728
export function isUMDExportSymbol(symbol: Symbol) {
37293729
return symbol && symbol.declarations && symbol.declarations[0] && isNamespaceExportDeclaration(symbol.declarations[0]);
37303730
}
3731+
3732+
export function showModuleSpecifier({ moduleSpecifier }: ImportDeclaration): string {
3733+
return isStringLiteral(moduleSpecifier) ? moduleSpecifier.text : getTextOfNode(moduleSpecifier);
3734+
}
37313735
}
37323736

37333737
namespace ts {

‎src/services/codefixes/fixUnusedIdentifier.ts

+23-3
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,19 @@ namespace ts.codefix {
55
const errorCodes = [
66
Diagnostics._0_is_declared_but_its_value_is_never_read.code,
77
Diagnostics.Property_0_is_declared_but_its_value_is_never_read.code,
8+
Diagnostics.All_imports_in_import_declaration_are_unused.code,
89
];
910
registerCodeFix({
1011
errorCodes,
1112
getCodeActions(context) {
12-
const { sourceFile } = context;
13+
const { errorCode, sourceFile } = context;
14+
const importDecl = tryGetFullImport(sourceFile, context.span.start);
15+
if (importDecl) {
16+
const description = formatStringFromArgs(getLocaleSpecificMessage(Diagnostics.Remove_import_from_0), [showModuleSpecifier(importDecl)]);
17+
const changes = textChanges.ChangeTracker.with(context, t => t.deleteNode(sourceFile, importDecl));
18+
return [{ description, changes, fixId: fixIdDelete }];
19+
}
20+
1321
const token = getToken(sourceFile, textSpanEnd(context.span));
1422
const result: CodeFixAction[] = [];
1523

@@ -19,7 +27,7 @@ namespace ts.codefix {
1927
result.push({ description, changes: deletion, fixId: fixIdDelete });
2028
}
2129

22-
const prefix = textChanges.ChangeTracker.with(context, t => tryPrefixDeclaration(t, context.errorCode, sourceFile, token));
30+
const prefix = textChanges.ChangeTracker.with(context, t => tryPrefixDeclaration(t, errorCode, sourceFile, token));
2331
if (prefix.length) {
2432
const description = formatStringFromArgs(getLocaleSpecificMessage(Diagnostics.Prefix_0_with_an_underscore), [token.getText()]);
2533
result.push({ description, changes: prefix, fixId: fixIdPrefix });
@@ -38,14 +46,26 @@ namespace ts.codefix {
3846
}
3947
break;
4048
case fixIdDelete:
41-
tryDeleteDeclaration(changes, sourceFile, token);
49+
const importDecl = tryGetFullImport(diag.file!, diag.start!);
50+
if (importDecl) {
51+
changes.deleteNode(sourceFile, importDecl);
52+
}
53+
else {
54+
tryDeleteDeclaration(changes, sourceFile, token);
55+
}
4256
break;
4357
default:
4458
Debug.fail(JSON.stringify(context.fixId));
4559
}
4660
}),
4761
});
4862

63+
// Sometimes the diagnostic span is an entire ImportDeclaration, so we should remove the whole thing.
64+
function tryGetFullImport(sourceFile: SourceFile, pos: number): ImportDeclaration | undefined {
65+
const startToken = getTokenAtPosition(sourceFile, pos, /*includeJsDocComment*/ false);
66+
return startToken.kind === SyntaxKind.ImportKeyword ? tryCast(startToken.parent, isImportDeclaration) : undefined;
67+
}
68+
4969
function getToken(sourceFile: SourceFile, pos: number): Node {
5070
const token = findPrecedingToken(pos, sourceFile);
5171
// this handles var ["computed"] = 12;

‎src/services/textChanges.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ namespace ts.textChanges {
140140

141141
export function getAdjustedStartPosition(sourceFile: SourceFile, node: Node, options: ConfigurableStart, position: Position) {
142142
if (options.useNonAdjustedStartPosition) {
143-
return node.getStart();
143+
return node.getStart(sourceFile);
144144
}
145145
const fullStart = node.getFullStart();
146146
const start = node.getStart(sourceFile);

‎tests/baselines/reference/extendsUntypedModule.errors.txt

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
1-
/a.ts(2,8): error TS6133: 'Bar' is declared but its value is never read.
1+
/a.ts(2,1): error TS6133: 'Bar' is declared but its value is never read.
22

33

44
==== /a.ts (1 errors) ====
55
import Foo from "foo";
66
import Bar from "bar"; // error: unused
7-
~~~
7+
~~~~~~~~~~~~~~~~~~~~~~
88
!!! error TS6133: 'Bar' is declared but its value is never read.
99
export class A extends Foo { }
1010

‎tests/baselines/reference/unusedImports1.errors.txt

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
tests/cases/compiler/file2.ts(1,9): error TS6133: 'Calculator' is declared but its value is never read.
1+
tests/cases/compiler/file2.ts(1,1): error TS6133: 'Calculator' is declared but its value is never read.
22

33

44
==== 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
88

99
==== tests/cases/compiler/file2.ts (1 errors) ====
1010
import {Calculator} from "./file1"
11-
~~~~~~~~~~
11+
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1212
!!! error TS6133: 'Calculator' is declared but its value is never read.

‎tests/baselines/reference/unusedImports12.errors.txt

+8-11
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,18 @@
1-
tests/cases/compiler/a.ts(1,10): error TS6133: 'Member' is declared but its value is never read.
2-
tests/cases/compiler/a.ts(2,8): error TS6133: 'd' is declared but its value is never read.
3-
tests/cases/compiler/a.ts(2,13): error TS6133: 'M' is declared but its value is never read.
4-
tests/cases/compiler/a.ts(3,8): error TS6133: 'ns' is declared but its value is never read.
1+
tests/cases/compiler/a.ts(1,1): error TS6133: 'Member' is declared but its value is never read.
2+
tests/cases/compiler/a.ts(2,1): error TS6192: All imports in import declaration are unused.
3+
tests/cases/compiler/a.ts(3,1): error TS6133: 'ns' is declared but its value is never read.
54
tests/cases/compiler/a.ts(4,1): error TS6133: 'r' is declared but its value is never read.
65

76

8-
==== tests/cases/compiler/a.ts (5 errors) ====
7+
==== tests/cases/compiler/a.ts (4 errors) ====
98
import { Member } from './b';
10-
~~~~~~
9+
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1110
!!! error TS6133: 'Member' is declared but its value is never read.
1211
import d, { Member as M } from './b';
13-
~
14-
!!! error TS6133: 'd' is declared but its value is never read.
15-
~~~~~~~~~~~
16-
!!! error TS6133: 'M' is declared but its value is never read.
12+
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
13+
!!! error TS6192: All imports in import declaration are unused.
1714
import * as ns from './b';
18-
~~~~~~~
15+
~~~~~~~~~~~~~~~~~~~~~~~~~~
1916
!!! error TS6133: 'ns' is declared but its value is never read.
2017
import r = require("./b");
2118
~~~~~~~~

‎tests/baselines/reference/unusedImports2.errors.txt

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
tests/cases/compiler/file2.ts(2,9): error TS6133: 'test' is declared but its value is never read.
1+
tests/cases/compiler/file2.ts(2,1): error TS6133: 'test' is declared but its value is never read.
22

33

44
==== 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
1313
==== tests/cases/compiler/file2.ts (1 errors) ====
1414
import {Calculator} from "./file1"
1515
import {test} from "./file1"
16-
~~~~
16+
~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1717
!!! error TS6133: 'test' is declared but its value is never read.
1818

1919
var x = new Calculator();

‎tests/baselines/reference/unusedImports6.errors.txt

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
tests/cases/compiler/file2.ts(1,8): error TS6133: 'd' is declared but its value is never read.
1+
tests/cases/compiler/file2.ts(1,1): error TS6133: 'd' is declared but its value is never read.
22

33

44
==== 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
1616

1717
==== tests/cases/compiler/file2.ts (1 errors) ====
1818
import d from "./file1"
19-
~
19+
~~~~~~~~~~~~~~~~~~~~~~~
2020
!!! error TS6133: 'd' is declared but its value is never read.
2121

2222

‎tests/baselines/reference/unusedImports7.errors.txt

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
tests/cases/compiler/file2.ts(1,8): error TS6133: 'n' is declared but its value is never read.
1+
tests/cases/compiler/file2.ts(1,1): error TS6133: 'n' is declared but its value is never read.
22

33

44
==== 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
1616

1717
==== tests/cases/compiler/file2.ts (1 errors) ====
1818
import * as n from "./file1"
19-
~~~~~~
19+
~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2020
!!! error TS6133: 'n' is declared but its value is never read.
2121

2222

‎tests/baselines/reference/unusedImports_entireImportDeclaration.errors.txt

+36
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
/b.ts(1,1): error TS6192: All imports in import declaration are unused.
2+
/b.ts(2,1): error TS6192: All imports in import declaration are unused.
3+
/b.ts(4,14): error TS6133: 'a2' is declared but its value is never read.
4+
/b.ts(4,23): error TS6133: 'b2' is declared but its value is never read.
5+
/b.ts(6,12): error TS6133: 'ns2' is declared but its value is never read.
6+
/b.ts(8,8): error TS6133: 'd5' is declared but its value is never read.
7+
8+
9+
==== /a.ts (0 errors) ====
10+
export const a = 0;
11+
export const b = 0;
12+
export default 0;
13+
14+
==== /b.ts (6 errors) ====
15+
import d1, { a as a1, b as b1 } from "./a";
16+
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
17+
!!! error TS6192: All imports in import declaration are unused.
18+
import d2, * as ns from "./a";
19+
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
20+
!!! error TS6192: All imports in import declaration are unused.
21+
22+
import d3, { a as a2, b as b2 } from "./a";
23+
~~~~~~~
24+
!!! error TS6133: 'a2' is declared but its value is never read.
25+
~~~~~~~
26+
!!! error TS6133: 'b2' is declared but its value is never read.
27+
d3;
28+
import d4, * as ns2 from "./a";
29+
~~~~~~~~
30+
!!! error TS6133: 'ns2' is declared but its value is never read.
31+
d4;
32+
import d5, * as ns3 from "./a";
33+
~~
34+
!!! error TS6133: 'd5' is declared but its value is never read.
35+
ns3;
36+

‎tests/baselines/reference/unusedImports_entireImportDeclaration.js

+34
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
//// [tests/cases/compiler/unusedImports_entireImportDeclaration.ts] ////
2+
3+
//// [a.ts]
4+
export const a = 0;
5+
export const b = 0;
6+
export default 0;
7+
8+
//// [b.ts]
9+
import d1, { a as a1, b as b1 } from "./a";
10+
import d2, * as ns from "./a";
11+
12+
import d3, { a as a2, b as b2 } from "./a";
13+
d3;
14+
import d4, * as ns2 from "./a";
15+
d4;
16+
import d5, * as ns3 from "./a";
17+
ns3;
18+
19+
20+
//// [a.js]
21+
"use strict";
22+
exports.__esModule = true;
23+
exports.a = 0;
24+
exports.b = 0;
25+
exports["default"] = 0;
26+
//// [b.js]
27+
"use strict";
28+
exports.__esModule = true;
29+
var a_1 = require("./a");
30+
a_1["default"];
31+
var a_2 = require("./a");
32+
a_2["default"];
33+
var ns3 = require("./a");
34+
ns3;

‎tests/baselines/reference/unusedImports_entireImportDeclaration.symbols

+45
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
=== /a.ts ===
2+
export const a = 0;
3+
>a : Symbol(a, Decl(a.ts, 0, 12))
4+
5+
export const b = 0;
6+
>b : Symbol(b, Decl(a.ts, 1, 12))
7+
8+
export default 0;
9+
10+
=== /b.ts ===
11+
import d1, { a as a1, b as b1 } from "./a";
12+
>d1 : Symbol(d1, Decl(b.ts, 0, 6))
13+
>a : Symbol(a1, Decl(b.ts, 0, 12))
14+
>a1 : Symbol(a1, Decl(b.ts, 0, 12))
15+
>b : Symbol(b1, Decl(b.ts, 0, 21))
16+
>b1 : Symbol(b1, Decl(b.ts, 0, 21))
17+
18+
import d2, * as ns from "./a";
19+
>d2 : Symbol(d2, Decl(b.ts, 1, 6))
20+
>ns : Symbol(ns, Decl(b.ts, 1, 10))
21+
22+
import d3, { a as a2, b as b2 } from "./a";
23+
>d3 : Symbol(d3, Decl(b.ts, 3, 6))
24+
>a : Symbol(a2, Decl(b.ts, 3, 12))
25+
>a2 : Symbol(a2, Decl(b.ts, 3, 12))
26+
>b : Symbol(b2, Decl(b.ts, 3, 21))
27+
>b2 : Symbol(b2, Decl(b.ts, 3, 21))
28+
29+
d3;
30+
>d3 : Symbol(d3, Decl(b.ts, 3, 6))
31+
32+
import d4, * as ns2 from "./a";
33+
>d4 : Symbol(d4, Decl(b.ts, 5, 6))
34+
>ns2 : Symbol(ns2, Decl(b.ts, 5, 10))
35+
36+
d4;
37+
>d4 : Symbol(d4, Decl(b.ts, 5, 6))
38+
39+
import d5, * as ns3 from "./a";
40+
>d5 : Symbol(d5, Decl(b.ts, 7, 6))
41+
>ns3 : Symbol(ns3, Decl(b.ts, 7, 10))
42+
43+
ns3;
44+
>ns3 : Symbol(ns3, Decl(b.ts, 7, 10))
45+

0 commit comments

Comments
 (0)