Skip to content

Commit 5725428

Browse files
author
Andy
authored
fixUnusedIdentifier: Handle destructure with all bindings unused (microsoft#23805)
* fixUnusedIdentifier: Handle destructure with all bindings unused * Add parameters test * Add test for 'for' loop
1 parent 556c316 commit 5725428

16 files changed

+332
-80
lines changed

src/compiler/checker.ts

+77-68
Original file line numberDiff line numberDiff line change
@@ -22352,10 +22352,6 @@ namespace ts {
2235222352
function checkUnusedIdentifiers(potentiallyUnusedIdentifiers: ReadonlyArray<PotentiallyUnusedIdentifier>, addDiagnostic: AddUnusedDiagnostic) {
2235322353
for (const node of potentiallyUnusedIdentifiers) {
2235422354
switch (node.kind) {
22355-
case SyntaxKind.SourceFile:
22356-
case SyntaxKind.ModuleDeclaration:
22357-
checkUnusedModuleMembers(node, addDiagnostic);
22358-
break;
2235922355
case SyntaxKind.ClassDeclaration:
2236022356
case SyntaxKind.ClassExpression:
2236122357
checkUnusedClassMembers(node, addDiagnostic);
@@ -22364,6 +22360,8 @@ namespace ts {
2236422360
case SyntaxKind.InterfaceDeclaration:
2236522361
checkUnusedTypeParameters(node, addDiagnostic);
2236622362
break;
22363+
case SyntaxKind.SourceFile:
22364+
case SyntaxKind.ModuleDeclaration:
2236722365
case SyntaxKind.Block:
2236822366
case SyntaxKind.CaseBlock:
2236922367
case SyntaxKind.ForStatement:
@@ -22397,35 +22395,6 @@ namespace ts {
2239722395
}
2239822396
}
2239922397

22400-
function checkUnusedLocalsAndParameters(node: Node, addDiagnostic: AddUnusedDiagnostic): void {
22401-
if (!(node.flags & NodeFlags.Ambient)) {
22402-
node.locals.forEach(local => {
22403-
// If it's purely a type parameter, ignore, will be checked in `checkUnusedTypeParameters`.
22404-
// If it's a type parameter merged with a parameter, check if the parameter-side is used.
22405-
if (local.flags & SymbolFlags.TypeParameter ? (local.flags & SymbolFlags.Variable && !(local.isReferenced & SymbolFlags.Variable)) : !local.isReferenced) {
22406-
if (local.valueDeclaration && getRootDeclaration(local.valueDeclaration).kind === SyntaxKind.Parameter) {
22407-
const parameter = <ParameterDeclaration>getRootDeclaration(local.valueDeclaration);
22408-
const name = getNameOfDeclaration(local.valueDeclaration);
22409-
if (!isParameterPropertyDeclaration(parameter) && !parameterIsThisKeyword(parameter) && !parameterNameStartsWithUnderscore(name)) {
22410-
addDiagnostic(UnusedKind.Parameter, createDiagnosticForNode(name, Diagnostics._0_is_declared_but_its_value_is_never_read, symbolName(local)));
22411-
}
22412-
}
22413-
else {
22414-
forEach(local.declarations, d => errorUnusedLocal(d, symbolName(local), addDiagnostic));
22415-
}
22416-
}
22417-
});
22418-
}
22419-
}
22420-
22421-
function isRemovedPropertyFromObjectSpread(node: Node) {
22422-
if (isBindingElement(node) && isObjectBindingPattern(node.parent)) {
22423-
const lastElement = lastOrUndefined(node.parent.elements);
22424-
return lastElement !== node && !!lastElement.dotDotDotToken;
22425-
}
22426-
return false;
22427-
}
22428-
2242922398
function errorUnusedLocal(declaration: Declaration, name: string, addDiagnostic: AddUnusedDiagnostic) {
2243022399
const node = getNameOfDeclaration(declaration) || declaration;
2243122400
if (isIdentifierThatStartsWithUnderScore(node)) {
@@ -22436,10 +22405,8 @@ namespace ts {
2243622405
}
2243722406
}
2243822407

22439-
if (!isRemovedPropertyFromObjectSpread(node.kind === SyntaxKind.Identifier ? node.parent : node)) {
22440-
const message = isTypeDeclaration(declaration) ? Diagnostics._0_is_declared_but_never_used : Diagnostics._0_is_declared_but_its_value_is_never_read;
22441-
addDiagnostic(UnusedKind.Local, createDiagnosticForNodeSpan(getSourceFileOfNode(declaration), declaration, node, message, name));
22442-
}
22408+
const message = isTypeDeclaration(declaration) ? Diagnostics._0_is_declared_but_never_used : Diagnostics._0_is_declared_but_its_value_is_never_read;
22409+
addDiagnostic(UnusedKind.Local, createDiagnosticForNodeSpan(getSourceFileOfNode(declaration), declaration, node, message, name));
2244322410
}
2244422411

2244522412
function parameterNameStartsWithUnderscore(parameterName: DeclarationName) {
@@ -22501,44 +22468,86 @@ namespace ts {
2250122468
}
2250222469
}
2250322470

22504-
function checkUnusedModuleMembers(node: ModuleDeclaration | SourceFile, addDiagnostic: AddUnusedDiagnostic): void {
22505-
if (!(node.flags & NodeFlags.Ambient)) {
22506-
// 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.
22507-
const unusedImports = createMap<[ImportClause, ImportedDeclaration[]]>();
22508-
node.locals.forEach(local => {
22509-
if (local.isReferenced || local.exportSymbol) return;
22510-
for (const declaration of local.declarations) {
22511-
if (isAmbientModule(declaration)) continue;
22512-
if (isImportedDeclaration(declaration)) {
22513-
const importClause = importClauseFromImported(declaration);
22514-
const key = String(getNodeId(importClause));
22515-
const group = unusedImports.get(key);
22516-
if (group) {
22517-
group[1].push(declaration);
22518-
}
22519-
else {
22520-
unusedImports.set(key, [importClause, [declaration]]);
22471+
function addToGroup<K, V>(map: Map<[K, V[]]>, key: K, value: V, getKey: (key: K) => number | string): void {
22472+
const keyString = String(getKey(key));
22473+
const group = map.get(keyString);
22474+
if (group) {
22475+
group[1].push(value);
22476+
}
22477+
else {
22478+
map.set(keyString, [key, [value]]);
22479+
}
22480+
}
22481+
22482+
function tryGetRootParameterDeclaration(node: Node): ParameterDeclaration | undefined {
22483+
return tryCast(getRootDeclaration(node), isParameter);
22484+
}
22485+
22486+
function checkUnusedLocalsAndParameters(nodeWithLocals: Node, addDiagnostic: AddUnusedDiagnostic): void {
22487+
if (nodeWithLocals.flags & NodeFlags.Ambient) return;
22488+
22489+
// 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.
22490+
const unusedImports = createMap<[ImportClause, ImportedDeclaration[]]>();
22491+
const unusedDestructures = createMap<[ObjectBindingPattern, BindingElement[]]>();
22492+
nodeWithLocals.locals.forEach(local => {
22493+
// If it's purely a type parameter, ignore, will be checked in `checkUnusedTypeParameters`.
22494+
// If it's a type parameter merged with a parameter, check if the parameter-side is used.
22495+
if (local.flags & SymbolFlags.TypeParameter ? !(local.flags & SymbolFlags.Variable && !(local.isReferenced & SymbolFlags.Variable)) : local.isReferenced || local.exportSymbol) {
22496+
return;
22497+
}
22498+
22499+
for (const declaration of local.declarations) {
22500+
if (isAmbientModule(declaration)) continue;
22501+
if (isImportedDeclaration(declaration)) {
22502+
addToGroup(unusedImports, importClauseFromImported(declaration), declaration, getNodeId);
22503+
}
22504+
else if (isBindingElement(declaration) && isObjectBindingPattern(declaration.parent)) {
22505+
// In `{ a, ...b }, `a` is considered used since it removes a property from `b`. `b` may still be unused though.
22506+
const lastElement = last(declaration.parent.elements);
22507+
if (declaration === lastElement || !last(declaration.parent.elements).dotDotDotToken) {
22508+
addToGroup(unusedDestructures, declaration.parent, declaration, getNodeId);
22509+
}
22510+
}
22511+
else {
22512+
const parameter = local.valueDeclaration && tryGetRootParameterDeclaration(local.valueDeclaration);
22513+
if (parameter) {
22514+
const name = getNameOfDeclaration(local.valueDeclaration);
22515+
if (!isParameterPropertyDeclaration(parameter) && !parameterIsThisKeyword(parameter) && !parameterNameStartsWithUnderscore(name)) {
22516+
addDiagnostic(UnusedKind.Parameter, createDiagnosticForNode(name, Diagnostics._0_is_declared_but_its_value_is_never_read, symbolName(local)));
2252122517
}
2252222518
}
2252322519
else {
2252422520
errorUnusedLocal(declaration, symbolName(local), addDiagnostic);
2252522521
}
2252622522
}
22527-
});
22528-
22529-
unusedImports.forEach(([importClause, unuseds]) => {
22530-
const importDecl = importClause.parent;
22531-
if (forEachImportedDeclaration(importClause, d => !contains(unuseds, d))) {
22532-
for (const unused of unuseds) errorUnusedLocal(unused, idText(unused.name), addDiagnostic);
22533-
}
22534-
else if (unuseds.length === 1) {
22535-
addDiagnostic(UnusedKind.Local, createDiagnosticForNode(importDecl, Diagnostics._0_is_declared_but_its_value_is_never_read, idText(first(unuseds).name)));
22536-
}
22537-
else {
22538-
addDiagnostic(UnusedKind.Local, createDiagnosticForNode(importDecl, Diagnostics.All_imports_in_import_declaration_are_unused, showModuleSpecifier(importDecl)));
22523+
}
22524+
});
22525+
unusedImports.forEach(([importClause, unuseds]) => {
22526+
const importDecl = importClause.parent;
22527+
if (forEachImportedDeclaration(importClause, d => !contains(unuseds, d))) {
22528+
for (const unused of unuseds) errorUnusedLocal(unused, idText(unused.name), addDiagnostic);
22529+
}
22530+
else if (unuseds.length === 1) {
22531+
addDiagnostic(UnusedKind.Local, createDiagnosticForNode(importDecl, Diagnostics._0_is_declared_but_its_value_is_never_read, idText(first(unuseds).name)));
22532+
}
22533+
else {
22534+
addDiagnostic(UnusedKind.Local, createDiagnosticForNode(importDecl, Diagnostics.All_imports_in_import_declaration_are_unused));
22535+
}
22536+
});
22537+
unusedDestructures.forEach(([bindingPattern, bindingElements]) => {
22538+
const kind = tryGetRootParameterDeclaration(bindingPattern.parent) ? UnusedKind.Parameter : UnusedKind.Local;
22539+
if (!bindingPattern.elements.every(e => contains(bindingElements, e))) {
22540+
for (const e of bindingElements) {
22541+
addDiagnostic(kind, createDiagnosticForNode(e, Diagnostics._0_is_declared_but_its_value_is_never_read, getBindingElementNameText(e)));
2253922542
}
22540-
});
22541-
}
22543+
}
22544+
else if (bindingElements.length === 1) {
22545+
addDiagnostic(kind, createDiagnosticForNode(bindingPattern, Diagnostics._0_is_declared_but_its_value_is_never_read, getBindingElementNameText(first(bindingElements))));
22546+
}
22547+
else {
22548+
addDiagnostic(kind, createDiagnosticForNode(bindingPattern, Diagnostics.All_destructured_elements_are_unused));
22549+
}
22550+
});
2254222551
}
2254322552

2254422553
type ImportedDeclaration = ImportClause | ImportSpecifier | NamespaceImport;

src/compiler/diagnosticMessages.json

+9
Original file line numberDiff line numberDiff line change
@@ -3551,6 +3551,11 @@
35513551
"category": "Message",
35523552
"code": 6197
35533553
},
3554+
"All destructured elements are unused.": {
3555+
"category": "Error",
3556+
"code": 6198,
3557+
"reportsUnnecessary": true
3558+
},
35543559

35553560
"Projects to reference": {
35563561
"category": "Message",
@@ -3985,6 +3990,10 @@
39853990
"category": "Message",
39863991
"code": 90008
39873992
},
3993+
"Remove destructuring": {
3994+
"category": "Message",
3995+
"code": 90009
3996+
},
39883997
"Import '{0}' from module \"{1}\"": {
39893998
"category": "Message",
39903999
"code": 90013

src/services/codefixes/fixUnusedIdentifier.ts

+28-1
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ namespace ts.codefix {
88
Diagnostics._0_is_declared_but_never_used.code,
99
Diagnostics.Property_0_is_declared_but_its_value_is_never_read.code,
1010
Diagnostics.All_imports_in_import_declaration_are_unused.code,
11+
Diagnostics.All_destructured_elements_are_unused.code,
1112
];
1213
registerCodeFix({
1314
errorCodes,
@@ -18,6 +19,10 @@ namespace ts.codefix {
1819
const changes = textChanges.ChangeTracker.with(context, t => t.deleteNode(sourceFile, importDecl));
1920
return [createCodeFixAction(fixName, changes, [Diagnostics.Remove_import_from_0, showModuleSpecifier(importDecl)], fixIdDelete, Diagnostics.Delete_all_unused_declarations)];
2021
}
22+
const delDestructure = textChanges.ChangeTracker.with(context, t => tryDeleteFullDestructure(t, sourceFile, context.span.start));
23+
if (delDestructure.length) {
24+
return [createCodeFixAction(fixName, delDestructure, Diagnostics.Remove_destructuring, fixIdDelete, Diagnostics.Delete_all_unused_declarations)];
25+
}
2126

2227
const token = getToken(sourceFile, textSpanEnd(context.span));
2328
const result: CodeFixAction[] = [];
@@ -50,7 +55,9 @@ namespace ts.codefix {
5055
changes.deleteNode(sourceFile, importDecl);
5156
}
5257
else {
53-
tryDeleteDeclaration(changes, sourceFile, token);
58+
if (!tryDeleteFullDestructure(changes, sourceFile, diag.start!)) {
59+
tryDeleteDeclaration(changes, sourceFile, token);
60+
}
5461
}
5562
break;
5663
default:
@@ -65,6 +72,26 @@ namespace ts.codefix {
6572
return startToken.kind === SyntaxKind.ImportKeyword ? tryCast(startToken.parent, isImportDeclaration) : undefined;
6673
}
6774

75+
function tryDeleteFullDestructure(changes: textChanges.ChangeTracker, sourceFile: SourceFile, pos: number): boolean {
76+
const startToken = getTokenAtPosition(sourceFile, pos, /*includeJsDocComment*/ false);
77+
if (startToken.kind !== SyntaxKind.OpenBraceToken || !isObjectBindingPattern(startToken.parent)) return false;
78+
const decl = startToken.parent.parent;
79+
switch (decl.kind) {
80+
case SyntaxKind.VariableDeclaration:
81+
tryDeleteVariableDeclaration(changes, sourceFile, decl);
82+
break;
83+
case SyntaxKind.Parameter:
84+
changes.deleteNodeInList(sourceFile, decl);
85+
break;
86+
case SyntaxKind.BindingElement:
87+
changes.deleteNode(sourceFile, decl);
88+
break;
89+
default:
90+
return Debug.assertNever(decl);
91+
}
92+
return true;
93+
}
94+
6895
function getToken(sourceFile: SourceFile, pos: number): Node {
6996
const token = findPrecedingToken(pos, sourceFile);
7097
// this handles var ["computed"] = 12;
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
tests/cases/compiler/unusedDestructuring.ts(3,7): error TS6198: All destructured elements are unused.
2+
tests/cases/compiler/unusedDestructuring.ts(4,9): error TS6133: 'c' is declared but its value is never read.
3+
tests/cases/compiler/unusedDestructuring.ts(6,7): error TS6133: 'e' is declared but its value is never read.
4+
tests/cases/compiler/unusedDestructuring.ts(8,1): error TS6133: 'f' is declared but its value is never read.
5+
tests/cases/compiler/unusedDestructuring.ts(8,12): error TS6198: All destructured elements are unused.
6+
tests/cases/compiler/unusedDestructuring.ts(8,24): error TS6133: 'c' is declared but its value is never read.
7+
tests/cases/compiler/unusedDestructuring.ts(8,32): error TS6133: 'e' is declared but its value is never read.
8+
9+
10+
==== tests/cases/compiler/unusedDestructuring.ts (7 errors) ====
11+
export {};
12+
declare const o: any;
13+
const { a, b } = o;
14+
~~~~~~~~
15+
!!! error TS6198: All destructured elements are unused.
16+
const { c, d } = o;
17+
~
18+
!!! error TS6133: 'c' is declared but its value is never read.
19+
d;
20+
const { e } = o;
21+
~~~~~
22+
!!! error TS6133: 'e' is declared but its value is never read.
23+
24+
function f({ a, b }, { c, d }, { e }) {
25+
~~~~~~~~~~
26+
!!! error TS6133: 'f' is declared but its value is never read.
27+
~~~~~~~~
28+
!!! error TS6198: All destructured elements are unused.
29+
~
30+
!!! error TS6133: 'c' is declared but its value is never read.
31+
~~~~~
32+
!!! error TS6133: 'e' is declared but its value is never read.
33+
d;
34+
}
35+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
//// [unusedDestructuring.ts]
2+
export {};
3+
declare const o: any;
4+
const { a, b } = o;
5+
const { c, d } = o;
6+
d;
7+
const { e } = o;
8+
9+
function f({ a, b }, { c, d }, { e }) {
10+
d;
11+
}
12+
13+
14+
//// [unusedDestructuring.js]
15+
"use strict";
16+
exports.__esModule = true;
17+
var a = o.a, b = o.b;
18+
var c = o.c, d = o.d;
19+
d;
20+
var e = o.e;
21+
function f(_a, _b, _c) {
22+
var a = _a.a, b = _a.b;
23+
var c = _b.c, d = _b.d;
24+
var e = _c.e;
25+
d;
26+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
=== tests/cases/compiler/unusedDestructuring.ts ===
2+
export {};
3+
declare const o: any;
4+
>o : Symbol(o, Decl(unusedDestructuring.ts, 1, 13))
5+
6+
const { a, b } = o;
7+
>a : Symbol(a, Decl(unusedDestructuring.ts, 2, 7))
8+
>b : Symbol(b, Decl(unusedDestructuring.ts, 2, 10))
9+
>o : Symbol(o, Decl(unusedDestructuring.ts, 1, 13))
10+
11+
const { c, d } = o;
12+
>c : Symbol(c, Decl(unusedDestructuring.ts, 3, 7))
13+
>d : Symbol(d, Decl(unusedDestructuring.ts, 3, 10))
14+
>o : Symbol(o, Decl(unusedDestructuring.ts, 1, 13))
15+
16+
d;
17+
>d : Symbol(d, Decl(unusedDestructuring.ts, 3, 10))
18+
19+
const { e } = o;
20+
>e : Symbol(e, Decl(unusedDestructuring.ts, 5, 7))
21+
>o : Symbol(o, Decl(unusedDestructuring.ts, 1, 13))
22+
23+
function f({ a, b }, { c, d }, { e }) {
24+
>f : Symbol(f, Decl(unusedDestructuring.ts, 5, 16))
25+
>a : Symbol(a, Decl(unusedDestructuring.ts, 7, 12))
26+
>b : Symbol(b, Decl(unusedDestructuring.ts, 7, 15))
27+
>c : Symbol(c, Decl(unusedDestructuring.ts, 7, 22))
28+
>d : Symbol(d, Decl(unusedDestructuring.ts, 7, 25))
29+
>e : Symbol(e, Decl(unusedDestructuring.ts, 7, 32))
30+
31+
d;
32+
>d : Symbol(d, Decl(unusedDestructuring.ts, 7, 25))
33+
}
34+

0 commit comments

Comments
 (0)