fixUnusedIdentifier: Handle destructure with all bindings unused (#23805)

* fixUnusedIdentifier: Handle destructure with all bindings unused

* Add parameters test

* Add test for 'for' loop
This commit is contained in:
Andy 2018-05-08 13:33:55 -07:00 committed by GitHub
parent 556c316fed
commit 5725428f2d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
16 changed files with 332 additions and 80 deletions

View File

@ -22352,10 +22352,6 @@ namespace ts {
function checkUnusedIdentifiers(potentiallyUnusedIdentifiers: ReadonlyArray<PotentiallyUnusedIdentifier>, addDiagnostic: AddUnusedDiagnostic) {
for (const node of potentiallyUnusedIdentifiers) {
switch (node.kind) {
case SyntaxKind.SourceFile:
case SyntaxKind.ModuleDeclaration:
checkUnusedModuleMembers(node, addDiagnostic);
break;
case SyntaxKind.ClassDeclaration:
case SyntaxKind.ClassExpression:
checkUnusedClassMembers(node, addDiagnostic);
@ -22364,6 +22360,8 @@ namespace ts {
case SyntaxKind.InterfaceDeclaration:
checkUnusedTypeParameters(node, addDiagnostic);
break;
case SyntaxKind.SourceFile:
case SyntaxKind.ModuleDeclaration:
case SyntaxKind.Block:
case SyntaxKind.CaseBlock:
case SyntaxKind.ForStatement:
@ -22397,35 +22395,6 @@ namespace ts {
}
}
function checkUnusedLocalsAndParameters(node: Node, addDiagnostic: AddUnusedDiagnostic): void {
if (!(node.flags & NodeFlags.Ambient)) {
node.locals.forEach(local => {
// If it's purely a type parameter, ignore, will be checked in `checkUnusedTypeParameters`.
// If it's a type parameter merged with a parameter, check if the parameter-side is used.
if (local.flags & SymbolFlags.TypeParameter ? (local.flags & SymbolFlags.Variable && !(local.isReferenced & SymbolFlags.Variable)) : !local.isReferenced) {
if (local.valueDeclaration && getRootDeclaration(local.valueDeclaration).kind === SyntaxKind.Parameter) {
const parameter = <ParameterDeclaration>getRootDeclaration(local.valueDeclaration);
const name = getNameOfDeclaration(local.valueDeclaration);
if (!isParameterPropertyDeclaration(parameter) && !parameterIsThisKeyword(parameter) && !parameterNameStartsWithUnderscore(name)) {
addDiagnostic(UnusedKind.Parameter, createDiagnosticForNode(name, Diagnostics._0_is_declared_but_its_value_is_never_read, symbolName(local)));
}
}
else {
forEach(local.declarations, d => errorUnusedLocal(d, symbolName(local), addDiagnostic));
}
}
});
}
}
function isRemovedPropertyFromObjectSpread(node: Node) {
if (isBindingElement(node) && isObjectBindingPattern(node.parent)) {
const lastElement = lastOrUndefined(node.parent.elements);
return lastElement !== node && !!lastElement.dotDotDotToken;
}
return false;
}
function errorUnusedLocal(declaration: Declaration, name: string, addDiagnostic: AddUnusedDiagnostic) {
const node = getNameOfDeclaration(declaration) || declaration;
if (isIdentifierThatStartsWithUnderScore(node)) {
@ -22436,10 +22405,8 @@ namespace ts {
}
}
if (!isRemovedPropertyFromObjectSpread(node.kind === SyntaxKind.Identifier ? node.parent : node)) {
const message = isTypeDeclaration(declaration) ? Diagnostics._0_is_declared_but_never_used : Diagnostics._0_is_declared_but_its_value_is_never_read;
addDiagnostic(UnusedKind.Local, createDiagnosticForNodeSpan(getSourceFileOfNode(declaration), declaration, node, message, name));
}
const message = isTypeDeclaration(declaration) ? Diagnostics._0_is_declared_but_never_used : Diagnostics._0_is_declared_but_its_value_is_never_read;
addDiagnostic(UnusedKind.Local, createDiagnosticForNodeSpan(getSourceFileOfNode(declaration), declaration, node, message, name));
}
function parameterNameStartsWithUnderscore(parameterName: DeclarationName) {
@ -22501,44 +22468,86 @@ namespace ts {
}
}
function checkUnusedModuleMembers(node: ModuleDeclaration | SourceFile, addDiagnostic: AddUnusedDiagnostic): void {
if (!(node.flags & NodeFlags.Ambient)) {
// 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)) {
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]]);
function addToGroup<K, V>(map: Map<[K, V[]]>, key: K, value: V, getKey: (key: K) => number | string): void {
const keyString = String(getKey(key));
const group = map.get(keyString);
if (group) {
group[1].push(value);
}
else {
map.set(keyString, [key, [value]]);
}
}
function tryGetRootParameterDeclaration(node: Node): ParameterDeclaration | undefined {
return tryCast(getRootDeclaration(node), isParameter);
}
function checkUnusedLocalsAndParameters(nodeWithLocals: Node, addDiagnostic: AddUnusedDiagnostic): void {
if (nodeWithLocals.flags & NodeFlags.Ambient) return;
// 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[]]>();
const unusedDestructures = createMap<[ObjectBindingPattern, BindingElement[]]>();
nodeWithLocals.locals.forEach(local => {
// If it's purely a type parameter, ignore, will be checked in `checkUnusedTypeParameters`.
// If it's a type parameter merged with a parameter, check if the parameter-side is used.
if (local.flags & SymbolFlags.TypeParameter ? !(local.flags & SymbolFlags.Variable && !(local.isReferenced & SymbolFlags.Variable)) : local.isReferenced || local.exportSymbol) {
return;
}
for (const declaration of local.declarations) {
if (isAmbientModule(declaration)) continue;
if (isImportedDeclaration(declaration)) {
addToGroup(unusedImports, importClauseFromImported(declaration), declaration, getNodeId);
}
else if (isBindingElement(declaration) && isObjectBindingPattern(declaration.parent)) {
// In `{ a, ...b }, `a` is considered used since it removes a property from `b`. `b` may still be unused though.
const lastElement = last(declaration.parent.elements);
if (declaration === lastElement || !last(declaration.parent.elements).dotDotDotToken) {
addToGroup(unusedDestructures, declaration.parent, declaration, getNodeId);
}
}
else {
const parameter = local.valueDeclaration && tryGetRootParameterDeclaration(local.valueDeclaration);
if (parameter) {
const name = getNameOfDeclaration(local.valueDeclaration);
if (!isParameterPropertyDeclaration(parameter) && !parameterIsThisKeyword(parameter) && !parameterNameStartsWithUnderscore(name)) {
addDiagnostic(UnusedKind.Parameter, createDiagnosticForNode(name, Diagnostics._0_is_declared_but_its_value_is_never_read, symbolName(local)));
}
}
else {
errorUnusedLocal(declaration, symbolName(local), addDiagnostic);
}
}
});
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), addDiagnostic);
}
});
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), addDiagnostic);
}
else if (unuseds.length === 1) {
addDiagnostic(UnusedKind.Local, createDiagnosticForNode(importDecl, Diagnostics._0_is_declared_but_its_value_is_never_read, idText(first(unuseds).name)));
}
else {
addDiagnostic(UnusedKind.Local, createDiagnosticForNode(importDecl, Diagnostics.All_imports_in_import_declaration_are_unused));
}
});
unusedDestructures.forEach(([bindingPattern, bindingElements]) => {
const kind = tryGetRootParameterDeclaration(bindingPattern.parent) ? UnusedKind.Parameter : UnusedKind.Local;
if (!bindingPattern.elements.every(e => contains(bindingElements, e))) {
for (const e of bindingElements) {
addDiagnostic(kind, createDiagnosticForNode(e, Diagnostics._0_is_declared_but_its_value_is_never_read, getBindingElementNameText(e)));
}
else if (unuseds.length === 1) {
addDiagnostic(UnusedKind.Local, createDiagnosticForNode(importDecl, Diagnostics._0_is_declared_but_its_value_is_never_read, idText(first(unuseds).name)));
}
else {
addDiagnostic(UnusedKind.Local, createDiagnosticForNode(importDecl, Diagnostics.All_imports_in_import_declaration_are_unused, showModuleSpecifier(importDecl)));
}
});
}
}
else if (bindingElements.length === 1) {
addDiagnostic(kind, createDiagnosticForNode(bindingPattern, Diagnostics._0_is_declared_but_its_value_is_never_read, getBindingElementNameText(first(bindingElements))));
}
else {
addDiagnostic(kind, createDiagnosticForNode(bindingPattern, Diagnostics.All_destructured_elements_are_unused));
}
});
}
type ImportedDeclaration = ImportClause | ImportSpecifier | NamespaceImport;

View File

@ -3551,6 +3551,11 @@
"category": "Message",
"code": 6197
},
"All destructured elements are unused.": {
"category": "Error",
"code": 6198,
"reportsUnnecessary": true
},
"Projects to reference": {
"category": "Message",
@ -3985,6 +3990,10 @@
"category": "Message",
"code": 90008
},
"Remove destructuring": {
"category": "Message",
"code": 90009
},
"Import '{0}' from module \"{1}\"": {
"category": "Message",
"code": 90013

View File

@ -8,6 +8,7 @@ namespace ts.codefix {
Diagnostics._0_is_declared_but_never_used.code,
Diagnostics.Property_0_is_declared_but_its_value_is_never_read.code,
Diagnostics.All_imports_in_import_declaration_are_unused.code,
Diagnostics.All_destructured_elements_are_unused.code,
];
registerCodeFix({
errorCodes,
@ -18,6 +19,10 @@ namespace ts.codefix {
const changes = textChanges.ChangeTracker.with(context, t => t.deleteNode(sourceFile, importDecl));
return [createCodeFixAction(fixName, changes, [Diagnostics.Remove_import_from_0, showModuleSpecifier(importDecl)], fixIdDelete, Diagnostics.Delete_all_unused_declarations)];
}
const delDestructure = textChanges.ChangeTracker.with(context, t => tryDeleteFullDestructure(t, sourceFile, context.span.start));
if (delDestructure.length) {
return [createCodeFixAction(fixName, delDestructure, Diagnostics.Remove_destructuring, fixIdDelete, Diagnostics.Delete_all_unused_declarations)];
}
const token = getToken(sourceFile, textSpanEnd(context.span));
const result: CodeFixAction[] = [];
@ -50,7 +55,9 @@ namespace ts.codefix {
changes.deleteNode(sourceFile, importDecl);
}
else {
tryDeleteDeclaration(changes, sourceFile, token);
if (!tryDeleteFullDestructure(changes, sourceFile, diag.start!)) {
tryDeleteDeclaration(changes, sourceFile, token);
}
}
break;
default:
@ -65,6 +72,26 @@ namespace ts.codefix {
return startToken.kind === SyntaxKind.ImportKeyword ? tryCast(startToken.parent, isImportDeclaration) : undefined;
}
function tryDeleteFullDestructure(changes: textChanges.ChangeTracker, sourceFile: SourceFile, pos: number): boolean {
const startToken = getTokenAtPosition(sourceFile, pos, /*includeJsDocComment*/ false);
if (startToken.kind !== SyntaxKind.OpenBraceToken || !isObjectBindingPattern(startToken.parent)) return false;
const decl = startToken.parent.parent;
switch (decl.kind) {
case SyntaxKind.VariableDeclaration:
tryDeleteVariableDeclaration(changes, sourceFile, decl);
break;
case SyntaxKind.Parameter:
changes.deleteNodeInList(sourceFile, decl);
break;
case SyntaxKind.BindingElement:
changes.deleteNode(sourceFile, decl);
break;
default:
return Debug.assertNever(decl);
}
return true;
}
function getToken(sourceFile: SourceFile, pos: number): Node {
const token = findPrecedingToken(pos, sourceFile);
// this handles var ["computed"] = 12;

View File

@ -0,0 +1,35 @@
tests/cases/compiler/unusedDestructuring.ts(3,7): error TS6198: All destructured elements are unused.
tests/cases/compiler/unusedDestructuring.ts(4,9): error TS6133: 'c' is declared but its value is never read.
tests/cases/compiler/unusedDestructuring.ts(6,7): error TS6133: 'e' is declared but its value is never read.
tests/cases/compiler/unusedDestructuring.ts(8,1): error TS6133: 'f' is declared but its value is never read.
tests/cases/compiler/unusedDestructuring.ts(8,12): error TS6198: All destructured elements are unused.
tests/cases/compiler/unusedDestructuring.ts(8,24): error TS6133: 'c' is declared but its value is never read.
tests/cases/compiler/unusedDestructuring.ts(8,32): error TS6133: 'e' is declared but its value is never read.
==== tests/cases/compiler/unusedDestructuring.ts (7 errors) ====
export {};
declare const o: any;
const { a, b } = o;
~~~~~~~~
!!! error TS6198: All destructured elements are unused.
const { c, d } = o;
~
!!! error TS6133: 'c' is declared but its value is never read.
d;
const { e } = o;
~~~~~
!!! error TS6133: 'e' is declared but its value is never read.
function f({ a, b }, { c, d }, { e }) {
~~~~~~~~~~
!!! error TS6133: 'f' is declared but its value is never read.
~~~~~~~~
!!! error TS6198: All destructured elements are unused.
~
!!! error TS6133: 'c' is declared but its value is never read.
~~~~~
!!! error TS6133: 'e' is declared but its value is never read.
d;
}

View File

@ -0,0 +1,26 @@
//// [unusedDestructuring.ts]
export {};
declare const o: any;
const { a, b } = o;
const { c, d } = o;
d;
const { e } = o;
function f({ a, b }, { c, d }, { e }) {
d;
}
//// [unusedDestructuring.js]
"use strict";
exports.__esModule = true;
var a = o.a, b = o.b;
var c = o.c, d = o.d;
d;
var e = o.e;
function f(_a, _b, _c) {
var a = _a.a, b = _a.b;
var c = _b.c, d = _b.d;
var e = _c.e;
d;
}

View File

@ -0,0 +1,34 @@
=== tests/cases/compiler/unusedDestructuring.ts ===
export {};
declare const o: any;
>o : Symbol(o, Decl(unusedDestructuring.ts, 1, 13))
const { a, b } = o;
>a : Symbol(a, Decl(unusedDestructuring.ts, 2, 7))
>b : Symbol(b, Decl(unusedDestructuring.ts, 2, 10))
>o : Symbol(o, Decl(unusedDestructuring.ts, 1, 13))
const { c, d } = o;
>c : Symbol(c, Decl(unusedDestructuring.ts, 3, 7))
>d : Symbol(d, Decl(unusedDestructuring.ts, 3, 10))
>o : Symbol(o, Decl(unusedDestructuring.ts, 1, 13))
d;
>d : Symbol(d, Decl(unusedDestructuring.ts, 3, 10))
const { e } = o;
>e : Symbol(e, Decl(unusedDestructuring.ts, 5, 7))
>o : Symbol(o, Decl(unusedDestructuring.ts, 1, 13))
function f({ a, b }, { c, d }, { e }) {
>f : Symbol(f, Decl(unusedDestructuring.ts, 5, 16))
>a : Symbol(a, Decl(unusedDestructuring.ts, 7, 12))
>b : Symbol(b, Decl(unusedDestructuring.ts, 7, 15))
>c : Symbol(c, Decl(unusedDestructuring.ts, 7, 22))
>d : Symbol(d, Decl(unusedDestructuring.ts, 7, 25))
>e : Symbol(e, Decl(unusedDestructuring.ts, 7, 32))
d;
>d : Symbol(d, Decl(unusedDestructuring.ts, 7, 25))
}

View File

@ -0,0 +1,34 @@
=== tests/cases/compiler/unusedDestructuring.ts ===
export {};
declare const o: any;
>o : any
const { a, b } = o;
>a : any
>b : any
>o : any
const { c, d } = o;
>c : any
>d : any
>o : any
d;
>d : any
const { e } = o;
>e : any
>o : any
function f({ a, b }, { c, d }, { e }) {
>f : ({ a, b }: { a: any; b: any; }, { c, d }: { c: any; d: any; }, { e }: { e: any; }) => void
>a : any
>b : any
>c : any
>d : any
>e : any
d;
>d : any
}

View File

@ -1,5 +1,5 @@
tests/cases/compiler/unusedDestructuringParameters.ts(1,13): error TS6133: 'a' is declared but its value is never read.
tests/cases/compiler/unusedDestructuringParameters.ts(3,14): error TS6133: 'a' is declared but its value is never read.
tests/cases/compiler/unusedDestructuringParameters.ts(3,13): error TS6133: 'a' is declared but its value is never read.
==== tests/cases/compiler/unusedDestructuringParameters.ts (2 errors) ====
@ -8,7 +8,7 @@ tests/cases/compiler/unusedDestructuringParameters.ts(3,14): error TS6133: 'a' i
!!! error TS6133: 'a' is declared but its value is never read.
f([1]);
const f2 = ({a}) => { };
~
~~~
!!! error TS6133: 'a' is declared but its value is never read.
f2({ a: 10 });
const f3 = ([_]) => { };

View File

@ -1,5 +1,5 @@
tests/cases/compiler/unusedLocalsAndObjectSpread.ts(20,15): error TS6133: 'bar' is declared but its value is never read.
tests/cases/compiler/unusedLocalsAndObjectSpread.ts(27,18): error TS6133: 'bar' is declared but its value is never read.
tests/cases/compiler/unusedLocalsAndObjectSpread.ts(20,18): error TS6133: 'bar' is declared but its value is never read.
tests/cases/compiler/unusedLocalsAndObjectSpread.ts(27,21): error TS6133: 'bar' is declared but its value is never read.
==== tests/cases/compiler/unusedLocalsAndObjectSpread.ts (2 errors) ====
@ -23,7 +23,7 @@ tests/cases/compiler/unusedLocalsAndObjectSpread.ts(27,18): error TS6133: 'bar'
const foo = { a: 1, b: 2 };
// 'a' is declared but never used
const {a, ...bar} = foo; // bar should be unused
~~~~~~
~~~
!!! error TS6133: 'bar' is declared but its value is never read.
//console.log(bar);
}
@ -32,7 +32,7 @@ tests/cases/compiler/unusedLocalsAndObjectSpread.ts(27,18): error TS6133: 'bar'
const foo = { a: 1, b: 2 };
// '_' is declared but never used
const {a: _, ...bar} = foo; // bar should be unused
~~~~~~
~~~
!!! error TS6133: 'bar' is declared but its value is never read.
//console.log(bar);
}

View File

@ -1,6 +1,6 @@
tests/cases/compiler/unusedLocalsAndObjectSpread2.ts(5,3): error TS6133: 'rest' is declared but its value is never read.
tests/cases/compiler/unusedLocalsAndObjectSpread2.ts(5,6): error TS6133: 'rest' is declared but its value is never read.
tests/cases/compiler/unusedLocalsAndObjectSpread2.ts(8,1): error TS6133: 'foo' is declared but its value is never read.
tests/cases/compiler/unusedLocalsAndObjectSpread2.ts(12,9): error TS6133: 'rest' is declared but its value is never read.
tests/cases/compiler/unusedLocalsAndObjectSpread2.ts(12,12): error TS6133: 'rest' is declared but its value is never read.
==== tests/cases/compiler/unusedLocalsAndObjectSpread2.ts (3 errors) ====
@ -9,7 +9,7 @@ tests/cases/compiler/unusedLocalsAndObjectSpread2.ts(12,9): error TS6133: 'rest'
children, // here!
active: _a, // here!
...rest
~~~~~~~
~~~~
!!! error TS6133: 'rest' is declared but its value is never read.
} = props;
@ -20,7 +20,7 @@ tests/cases/compiler/unusedLocalsAndObjectSpread2.ts(12,9): error TS6133: 'rest'
children,
active: _a,
...rest
~~~~~~~
~~~~
!!! error TS6133: 'rest' is declared but its value is never read.
} = props;
}

View File

@ -2,11 +2,12 @@ tests/cases/compiler/unusedParametersWithUnderscore.ts(1,12): error TS6133: 'a'
tests/cases/compiler/unusedParametersWithUnderscore.ts(1,19): error TS6133: 'c' is declared but its value is never read.
tests/cases/compiler/unusedParametersWithUnderscore.ts(1,27): error TS6133: 'd' is declared but its value is never read.
tests/cases/compiler/unusedParametersWithUnderscore.ts(1,29): error TS6133: 'e___' is declared but its value is never read.
tests/cases/compiler/unusedParametersWithUnderscore.ts(5,13): error TS6198: All destructured elements are unused.
tests/cases/compiler/unusedParametersWithUnderscore.ts(11,16): error TS6133: 'arg' is declared but its value is never read.
tests/cases/compiler/unusedParametersWithUnderscore.ts(17,13): error TS6133: 'arg' is declared but its value is never read.
==== tests/cases/compiler/unusedParametersWithUnderscore.ts (6 errors) ====
==== tests/cases/compiler/unusedParametersWithUnderscore.ts (7 errors) ====
function f(a, _b, c, ___, d,e___, _f) {
~
!!! error TS6133: 'a' is declared but its value is never read.
@ -20,6 +21,8 @@ tests/cases/compiler/unusedParametersWithUnderscore.ts(17,13): error TS6133: 'ar
function f2({_a, __b}) {
~~~~~~~~~
!!! error TS6198: All destructured elements are unused.
}
function f3([_a, ,__b]) {

View File

@ -0,0 +1,13 @@
// @noUnusedLocals: true
// @noUnusedParameters: true
export {};
declare const o: any;
const { a, b } = o;
const { c, d } = o;
d;
const { e } = o;
function f({ a, b }, { c, d }, { e }) {
d;
}

View File

@ -0,0 +1,14 @@
/// <reference path='fourslash.ts' />
// @noUnusedLocals: true
// @noUnusedParameters: true
////export {};
////const { x, y } = o;
verify.codeFix({
description: "Remove destructuring",
newFileContent:
`export {};
`,
});

View File

@ -0,0 +1,22 @@
/// <reference path='fourslash.ts' />
// @noUnusedLocals: true
// @noUnusedParameters: true
////const { x, y } = o;
////const { a, b } = o;
////a;
////export function f({ x, y }, { a, b }) {
//// a;
////}
verify.codeFixAll({
fixId: "unusedIdentifier_delete",
fixAllDescription: "Delete all unused declarations",
newFileContent:
`const { a } = o;
a;
export function f({ a }) {
a;
}`,
});

View File

@ -0,0 +1,12 @@
/// <reference path='fourslash.ts' />
// @noUnusedLocals: true
// @noUnusedParameters: true
////for (const { x } of o) {}
verify.codeFix({
description: "Remove destructuring",
newFileContent:
`for (const {} of o) {}`,
});

View File

@ -0,0 +1,14 @@
/// <reference path='fourslash.ts' />
// @noUnusedLocals: true
// @noUnusedParameters: true
////export {};
////const { x: { a, b } } = o;
verify.codeFix({
description: "Remove destructuring",
newFileContent:
`export {};
const { } = o;`,
});