Skip to content

Commit 802dc2b

Browse files
author
Andy
authored
fixUnusedIdentifier: If every VariableDeclaration is unused, remove the VariableStatement (microsoft#24231)
1 parent d2f6f6a commit 802dc2b

11 files changed

Lines changed: 129 additions & 63 deletions

‎src/compiler/checker.ts‎

Lines changed: 47 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -22620,14 +22620,6 @@ namespace ts {
2262022620

2262122621
function errorUnusedLocal(declaration: Declaration, name: string, addDiagnostic: AddUnusedDiagnostic) {
2262222622
const node = getNameOfDeclaration(declaration) || declaration;
22623-
if (isIdentifierThatStartsWithUnderScore(node)) {
22624-
const declaration = getRootDeclaration(node.parent);
22625-
if ((declaration.kind === SyntaxKind.VariableDeclaration && isForInOrOfStatement(declaration.parent.parent)) ||
22626-
declaration.kind === SyntaxKind.TypeParameter) {
22627-
return;
22628-
}
22629-
}
22630-
2263122623
const message = isTypeDeclaration(declaration) ? Diagnostics._0_is_declared_but_never_used : Diagnostics._0_is_declared_but_its_value_is_never_read;
2263222624
addDiagnostic(UnusedKind.Local, createDiagnosticForNodeSpan(getSourceFileOfNode(declaration), declaration, node, message, name));
2263322625
}
@@ -22712,6 +22704,7 @@ namespace ts {
2271222704
// 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.
2271322705
const unusedImports = createMap<[ImportClause, ImportedDeclaration[]]>();
2271422706
const unusedDestructures = createMap<[ObjectBindingPattern, BindingElement[]]>();
22707+
const unusedVariables = createMap<[VariableDeclarationList, VariableDeclaration[]]>();
2271522708
nodeWithLocals.locals.forEach(local => {
2271622709
// If it's purely a type parameter, ignore, will be checked in `checkUnusedTypeParameters`.
2271722710
// If it's a type parameter merged with a parameter, check if the parameter-side is used.
@@ -22731,6 +22724,11 @@ namespace ts {
2273122724
addToGroup(unusedDestructures, declaration.parent, declaration, getNodeId);
2273222725
}
2273322726
}
22727+
else if (isVariableDeclaration(declaration)) {
22728+
if (!isIdentifierThatStartsWithUnderScore(declaration.name) || !isForInOrOfStatement(declaration.parent.parent)) {
22729+
addToGroup(unusedVariables, declaration.parent, declaration, getNodeId);
22730+
}
22731+
}
2273422732
else {
2273522733
const parameter = local.valueDeclaration && tryGetRootParameterDeclaration(local.valueDeclaration);
2273622734
if (parameter) {
@@ -22747,32 +22745,63 @@ namespace ts {
2274722745
});
2274822746
unusedImports.forEach(([importClause, unuseds]) => {
2274922747
const importDecl = importClause.parent;
22750-
if (forEachImportedDeclaration(importClause, d => !contains(unuseds, d))) {
22751-
for (const unused of unuseds) errorUnusedLocal(unused, idText(unused.name), addDiagnostic);
22752-
}
22753-
else if (unuseds.length === 1) {
22754-
addDiagnostic(UnusedKind.Local, createDiagnosticForNode(importDecl, Diagnostics._0_is_declared_but_its_value_is_never_read, idText(first(unuseds).name)));
22748+
const nDeclarations = (importClause.name ? 1 : 0) +
22749+
(importClause.namedBindings ?
22750+
(importClause.namedBindings.kind === SyntaxKind.NamespaceImport ? 1 : importClause.namedBindings.elements.length)
22751+
: 0);
22752+
if (nDeclarations === unuseds.length) {
22753+
addDiagnostic(UnusedKind.Local, unuseds.length === 1
22754+
? createDiagnosticForNode(importDecl, Diagnostics._0_is_declared_but_its_value_is_never_read, idText(first(unuseds).name))
22755+
: createDiagnosticForNode(importDecl, Diagnostics.All_imports_in_import_declaration_are_unused));
2275522756
}
2275622757
else {
22757-
addDiagnostic(UnusedKind.Local, createDiagnosticForNode(importDecl, Diagnostics.All_imports_in_import_declaration_are_unused));
22758+
for (const unused of unuseds) errorUnusedLocal(unused, idText(unused.name), addDiagnostic);
2275822759
}
2275922760
});
2276022761
unusedDestructures.forEach(([bindingPattern, bindingElements]) => {
2276122762
const kind = tryGetRootParameterDeclaration(bindingPattern.parent) ? UnusedKind.Parameter : UnusedKind.Local;
22762-
if (!bindingPattern.elements.every(e => contains(bindingElements, e))) {
22763+
if (bindingPattern.elements.length === bindingElements.length) {
22764+
if (bindingElements.length === 1 && bindingPattern.parent.kind === SyntaxKind.VariableDeclaration && bindingPattern.parent.parent.kind === SyntaxKind.VariableDeclarationList) {
22765+
addToGroup(unusedVariables, bindingPattern.parent.parent, bindingPattern.parent, getNodeId);
22766+
}
22767+
else {
22768+
addDiagnostic(kind, bindingElements.length === 1
22769+
? createDiagnosticForNode(bindingPattern, Diagnostics._0_is_declared_but_its_value_is_never_read, idText(cast(first(bindingElements).name, isIdentifier)))
22770+
: createDiagnosticForNode(bindingPattern, Diagnostics.All_destructured_elements_are_unused));
22771+
}
22772+
}
22773+
else {
2276322774
for (const e of bindingElements) {
2276422775
addDiagnostic(kind, createDiagnosticForNode(e, Diagnostics._0_is_declared_but_its_value_is_never_read, idText(cast(e.name, isIdentifier))));
2276522776
}
2276622777
}
22767-
else if (bindingElements.length === 1) {
22768-
addDiagnostic(kind, createDiagnosticForNode(bindingPattern, Diagnostics._0_is_declared_but_its_value_is_never_read, idText(cast(first(bindingElements).name, isIdentifier))));
22778+
});
22779+
unusedVariables.forEach(([declarationList, declarations]) => {
22780+
if (declarationList.declarations.length === declarations.length) {
22781+
addDiagnostic(UnusedKind.Local, declarations.length === 1
22782+
? createDiagnosticForNode(first(declarations).name, Diagnostics._0_is_declared_but_its_value_is_never_read, bindingNameText(first(declarations).name))
22783+
: createDiagnosticForNode(declarationList.parent.kind === SyntaxKind.VariableStatement ? declarationList.parent : declarationList, Diagnostics.All_variables_are_unused));
2276922784
}
2277022785
else {
22771-
addDiagnostic(kind, createDiagnosticForNode(bindingPattern, Diagnostics.All_destructured_elements_are_unused));
22786+
for (const decl of declarations) {
22787+
addDiagnostic(UnusedKind.Local, createDiagnosticForNode(decl, Diagnostics._0_is_declared_but_its_value_is_never_read, idText(cast(decl.name, isIdentifier))));
22788+
}
2277222789
}
2277322790
});
2277422791
}
2277522792

22793+
function bindingNameText(name: BindingName): string {
22794+
switch (name.kind) {
22795+
case SyntaxKind.Identifier:
22796+
return idText(name);
22797+
case SyntaxKind.ArrayBindingPattern:
22798+
case SyntaxKind.ObjectBindingPattern:
22799+
return bindingNameText(cast(first(name.elements), isBindingElement).name);
22800+
default:
22801+
return Debug.assertNever(name);
22802+
}
22803+
}
22804+
2277622805
type ImportedDeclaration = ImportClause | ImportSpecifier | NamespaceImport;
2277722806
function isImportedDeclaration(node: Node): node is ImportedDeclaration {
2277822807
return node.kind === SyntaxKind.ImportClause || node.kind === SyntaxKind.ImportSpecifier || node.kind === SyntaxKind.NamespaceImport;
@@ -22781,12 +22810,6 @@ namespace ts {
2278122810
return decl.kind === SyntaxKind.ImportClause ? decl : decl.kind === SyntaxKind.NamespaceImport ? decl.parent : decl.parent.parent;
2278222811
}
2278322812

22784-
function forEachImportedDeclaration<T>(importClause: ImportClause, cb: (im: ImportedDeclaration) => T | undefined): T | undefined {
22785-
const { name: defaultName, namedBindings } = importClause;
22786-
return (defaultName && cb(importClause)) ||
22787-
namedBindings && (namedBindings.kind === SyntaxKind.NamespaceImport ? cb(namedBindings) : forEach(namedBindings.elements, cb));
22788-
}
22789-
2279022813
function checkBlock(node: Block) {
2279122814
// Grammar checking for SyntaxKind.Block
2279222815
if (node.kind === SyntaxKind.Block) {

‎src/compiler/diagnosticMessages.json‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3556,6 +3556,11 @@
35563556
"code": 6198,
35573557
"reportsUnnecessary": true
35583558
},
3559+
"All variables are unused.": {
3560+
"category": "Error",
3561+
"code": 6199,
3562+
"reportsUnnecessary": true
3563+
},
35593564

35603565
"Projects to reference": {
35613566
"category": "Message",
@@ -3994,6 +3999,10 @@
39943999
"category": "Message",
39954000
"code": 90009
39964001
},
4002+
"Remove variable statement": {
4003+
"category": "Message",
4004+
"code": 90010
4005+
},
39974006
"Import '{0}' from module \"{1}\"": {
39984007
"category": "Message",
39994008
"code": 90013

‎src/services/codefixes/fixUnusedIdentifier.ts‎

Lines changed: 26 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -9,20 +9,27 @@ namespace ts.codefix {
99
Diagnostics.Property_0_is_declared_but_its_value_is_never_read.code,
1010
Diagnostics.All_imports_in_import_declaration_are_unused.code,
1111
Diagnostics.All_destructured_elements_are_unused.code,
12+
Diagnostics.All_variables_are_unused.code,
1213
];
1314
registerCodeFix({
1415
errorCodes,
1516
getCodeActions(context) {
1617
const { errorCode, sourceFile } = context;
17-
const importDecl = tryGetFullImport(sourceFile, context.span.start);
18+
const startToken = getTokenAtPosition(sourceFile, context.span.start, /*includeJsDocComment*/ false);
19+
20+
const importDecl = tryGetFullImport(startToken);
1821
if (importDecl) {
1922
const changes = textChanges.ChangeTracker.with(context, t => t.deleteNode(sourceFile, importDecl));
2023
return [createCodeFixAction(fixName, changes, [Diagnostics.Remove_import_from_0, showModuleSpecifier(importDecl)], fixIdDelete, Diagnostics.Delete_all_unused_declarations)];
2124
}
22-
const delDestructure = textChanges.ChangeTracker.with(context, t => tryDeleteFullDestructure(t, sourceFile, context.span.start, /*deleted*/ undefined));
25+
const delDestructure = textChanges.ChangeTracker.with(context, t => tryDeleteFullDestructure(t, sourceFile, startToken, /*deleted*/ undefined));
2326
if (delDestructure.length) {
2427
return [createCodeFixAction(fixName, delDestructure, Diagnostics.Remove_destructuring, fixIdDelete, Diagnostics.Delete_all_unused_declarations)];
2528
}
29+
const delVar = textChanges.ChangeTracker.with(context, t => tryDeleteFullVariableStatement(t, sourceFile, startToken, /*deleted*/ undefined));
30+
if (delVar.length) {
31+
return [createCodeFixAction(fixName, delDestructure, Diagnostics.Remove_variable_statement, fixIdDelete, Diagnostics.Delete_all_unused_declarations)];
32+
}
2633

2734
const token = getToken(sourceFile, textSpanEnd(context.span));
2835
const result: CodeFixAction[] = [];
@@ -45,6 +52,7 @@ namespace ts.codefix {
4552
const deleted = new NodeSet();
4653
return codeFixAll(context, errorCodes, (changes, diag) => {
4754
const { sourceFile } = context;
55+
const startToken = getTokenAtPosition(sourceFile, diag.start, /*includeJsDocComment*/ false);
4856
const token = findPrecedingToken(textSpanEnd(diag), diag.file!);
4957
switch (context.fixId) {
5058
case fixIdPrefix:
@@ -56,14 +64,12 @@ namespace ts.codefix {
5664
// Ignore if this range was already deleted.
5765
if (deleted.some(d => rangeContainsPosition(d, diag.start!))) break;
5866

59-
const importDecl = tryGetFullImport(diag.file!, diag.start!);
67+
const importDecl = tryGetFullImport(startToken);
6068
if (importDecl) {
6169
changes.deleteNode(sourceFile, importDecl);
6270
}
63-
else {
64-
if (!tryDeleteFullDestructure(changes, sourceFile, diag.start!, deleted)) {
65-
tryDeleteDeclaration(changes, sourceFile, token, deleted);
66-
}
71+
else if (!tryDeleteFullDestructure(changes, sourceFile, startToken, deleted) && !tryDeleteFullVariableStatement(changes, sourceFile, startToken, deleted)) {
72+
tryDeleteDeclaration(changes, sourceFile, token, deleted);
6773
}
6874
break;
6975
default:
@@ -74,15 +80,13 @@ namespace ts.codefix {
7480
});
7581

7682
// Sometimes the diagnostic span is an entire ImportDeclaration, so we should remove the whole thing.
77-
function tryGetFullImport(sourceFile: SourceFile, pos: number): ImportDeclaration | undefined {
78-
const startToken = getTokenAtPosition(sourceFile, pos, /*includeJsDocComment*/ false);
83+
function tryGetFullImport(startToken: Node): ImportDeclaration | undefined {
7984
return startToken.kind === SyntaxKind.ImportKeyword ? tryCast(startToken.parent, isImportDeclaration) : undefined;
8085
}
8186

82-
function tryDeleteFullDestructure(changes: textChanges.ChangeTracker, sourceFile: SourceFile, pos: number, deletedAncestors: NodeSet | undefined): boolean {
83-
const startToken = getTokenAtPosition(sourceFile, pos, /*includeJsDocComment*/ false);
87+
function tryDeleteFullDestructure(changes: textChanges.ChangeTracker, sourceFile: SourceFile, startToken: Node, deletedAncestors: NodeSet | undefined): boolean {
8488
if (startToken.kind !== SyntaxKind.OpenBraceToken || !isObjectBindingPattern(startToken.parent)) return false;
85-
const decl = startToken.parent.parent;
89+
const decl = cast(startToken.parent, isObjectBindingPattern).parent;
8690
switch (decl.kind) {
8791
case SyntaxKind.VariableDeclaration:
8892
tryDeleteVariableDeclaration(changes, sourceFile, decl, deletedAncestors);
@@ -101,6 +105,16 @@ namespace ts.codefix {
101105
return true;
102106
}
103107

108+
function tryDeleteFullVariableStatement(changes: textChanges.ChangeTracker, sourceFile: SourceFile, startToken: Node, deletedAncestors: NodeSet | undefined) {
109+
const declarationList = tryCast(startToken.parent, isVariableDeclarationList);
110+
if (declarationList && declarationList.getChildren(sourceFile)[0] === startToken) {
111+
if (deletedAncestors) deletedAncestors.add(declarationList);
112+
changes.deleteNode(sourceFile, declarationList.parent.kind === SyntaxKind.VariableStatement ? declarationList.parent : declarationList);
113+
return true;
114+
}
115+
return false;
116+
}
117+
104118
function getToken(sourceFile: SourceFile, pos: number): Node {
105119
const token = findPrecedingToken(pos, sourceFile, /*startNode*/ undefined, /*includeJsDoc*/ true);
106120
// this handles var ["computed"] = 12;

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

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,14 @@ tests/cases/compiler/unusedDestructuring.ts(3,7): error TS6198: All destructured
22
tests/cases/compiler/unusedDestructuring.ts(4,9): error TS6133: 'c' is declared but its value is never read.
33
tests/cases/compiler/unusedDestructuring.ts(6,7): error TS6133: 'e' is declared but its value is never read.
44
tests/cases/compiler/unusedDestructuring.ts(7,7): error TS6133: 'g' is declared but its value is never read.
5-
tests/cases/compiler/unusedDestructuring.ts(9,1): error TS6133: 'f' is declared but its value is never read.
6-
tests/cases/compiler/unusedDestructuring.ts(9,12): error TS6198: All destructured elements are unused.
7-
tests/cases/compiler/unusedDestructuring.ts(9,24): error TS6133: 'c' is declared but its value is never read.
8-
tests/cases/compiler/unusedDestructuring.ts(9,32): error TS6133: 'e' is declared but its value is never read.
5+
tests/cases/compiler/unusedDestructuring.ts(8,1): error TS6199: All variables are unused.
6+
tests/cases/compiler/unusedDestructuring.ts(10,1): error TS6133: 'f' is declared but its value is never read.
7+
tests/cases/compiler/unusedDestructuring.ts(10,12): error TS6198: All destructured elements are unused.
8+
tests/cases/compiler/unusedDestructuring.ts(10,24): error TS6133: 'c' is declared but its value is never read.
9+
tests/cases/compiler/unusedDestructuring.ts(10,32): error TS6133: 'e' is declared but its value is never read.
910

1011

11-
==== tests/cases/compiler/unusedDestructuring.ts (8 errors) ====
12+
==== tests/cases/compiler/unusedDestructuring.ts (9 errors) ====
1213
export {};
1314
declare const o: any;
1415
const { a, b } = o;
@@ -24,6 +25,9 @@ tests/cases/compiler/unusedDestructuring.ts(9,32): error TS6133: 'e' is declared
2425
const { f: g } = o;
2526
~~~~~~~~
2627
!!! error TS6133: 'g' is declared but its value is never read.
28+
const { h } = o, { i } = o;
29+
~~~~~~~~~~~~~~~~~~~~~~~~~~~
30+
!!! error TS6199: All variables are unused.
2731

2832
function f({ a, b }, { c, d }, { e }) {
2933
~~~~~~~~~~

‎tests/baselines/reference/unusedDestructuring.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ const { c, d } = o;
66
d;
77
const { e } = o;
88
const { f: g } = o;
9+
const { h } = o, { i } = o;
910

1011
function f({ a, b }, { c, d }, { e }) {
1112
d;
@@ -20,6 +21,7 @@ var c = o.c, d = o.d;
2021
d;
2122
var e = o.e;
2223
var g = o.f;
24+
var h = o.h, i = o.i;
2325
function f(_a, _b, _c) {
2426
var a = _a.a, b = _a.b;
2527
var c = _b.c, d = _b.d;

‎tests/baselines/reference/unusedDestructuring.symbols‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -24,15 +24,21 @@ const { f: g } = o;
2424
>g : Symbol(g, Decl(unusedDestructuring.ts, 6, 7))
2525
>o : Symbol(o, Decl(unusedDestructuring.ts, 1, 13))
2626

27+
const { h } = o, { i } = o;
28+
>h : Symbol(h, Decl(unusedDestructuring.ts, 7, 7))
29+
>o : Symbol(o, Decl(unusedDestructuring.ts, 1, 13))
30+
>i : Symbol(i, Decl(unusedDestructuring.ts, 7, 18))
31+
>o : Symbol(o, Decl(unusedDestructuring.ts, 1, 13))
32+
2733
function f({ a, b }, { c, d }, { e }) {
28-
>f : Symbol(f, Decl(unusedDestructuring.ts, 6, 19))
29-
>a : Symbol(a, Decl(unusedDestructuring.ts, 8, 12))
30-
>b : Symbol(b, Decl(unusedDestructuring.ts, 8, 15))
31-
>c : Symbol(c, Decl(unusedDestructuring.ts, 8, 22))
32-
>d : Symbol(d, Decl(unusedDestructuring.ts, 8, 25))
33-
>e : Symbol(e, Decl(unusedDestructuring.ts, 8, 32))
34+
>f : Symbol(f, Decl(unusedDestructuring.ts, 7, 27))
35+
>a : Symbol(a, Decl(unusedDestructuring.ts, 9, 12))
36+
>b : Symbol(b, Decl(unusedDestructuring.ts, 9, 15))
37+
>c : Symbol(c, Decl(unusedDestructuring.ts, 9, 22))
38+
>d : Symbol(d, Decl(unusedDestructuring.ts, 9, 25))
39+
>e : Symbol(e, Decl(unusedDestructuring.ts, 9, 32))
3440

3541
d;
36-
>d : Symbol(d, Decl(unusedDestructuring.ts, 8, 25))
42+
>d : Symbol(d, Decl(unusedDestructuring.ts, 9, 25))
3743
}
3844

‎tests/baselines/reference/unusedDestructuring.types‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,12 @@ const { f: g } = o;
2525
>g : any
2626
>o : any
2727

28+
const { h } = o, { i } = o;
29+
>h : any
30+
>o : any
31+
>i : any
32+
>o : any
33+
2834
function f({ a, b }, { c, d }, { e }) {
2935
>f : ({ a, b }: { a: any; b: any; }, { c, d }: { c: any; d: any; }, { e }: { e: any; }) => void
3036
>a : any

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

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,12 @@
1-
tests/cases/compiler/unusedLocalsInMethod2.ts(3,13): error TS6133: 'x' is declared but its value is never read.
2-
tests/cases/compiler/unusedLocalsInMethod2.ts(3,16): error TS6133: 'y' is declared but its value is never read.
1+
tests/cases/compiler/unusedLocalsInMethod2.ts(3,9): error TS6199: All variables are unused.
32

43

5-
==== tests/cases/compiler/unusedLocalsInMethod2.ts (2 errors) ====
4+
==== tests/cases/compiler/unusedLocalsInMethod2.ts (1 errors) ====
65
class greeter {
76
public function1() {
87
var x, y = 10;
9-
~
10-
!!! error TS6133: 'x' is declared but its value is never read.
11-
~
12-
!!! error TS6133: 'y' is declared but its value is never read.
8+
~~~~~~~~~~~~~~
9+
!!! error TS6199: All variables are unused.
1310
y++;
1411
}
1512
}

0 commit comments

Comments
 (0)