Skip to content

fixUnusedIdentifier: If every VariableDeclaration is unused, remove the VariableStatement #24231

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
1 commit merged into from
May 21, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
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
71 changes: 47 additions & 24 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22617,14 +22617,6 @@ namespace ts {

function errorUnusedLocal(declaration: Declaration, name: string, addDiagnostic: AddUnusedDiagnostic) {
const node = getNameOfDeclaration(declaration) || declaration;
if (isIdentifierThatStartsWithUnderScore(node)) {
const declaration = getRootDeclaration(node.parent);
if ((declaration.kind === SyntaxKind.VariableDeclaration && isForInOrOfStatement(declaration.parent.parent)) ||
declaration.kind === SyntaxKind.TypeParameter) {
return;
}
}

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));
}
Expand Down Expand Up @@ -22709,6 +22701,7 @@ namespace ts {
// 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[]]>();
const unusedVariables = createMap<[VariableDeclarationList, VariableDeclaration[]]>();
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.
Expand All @@ -22728,6 +22721,11 @@ namespace ts {
addToGroup(unusedDestructures, declaration.parent, declaration, getNodeId);
}
}
else if (isVariableDeclaration(declaration)) {
if (!isIdentifierThatStartsWithUnderScore(declaration.name) || !isForInOrOfStatement(declaration.parent.parent)) {
addToGroup(unusedVariables, declaration.parent, declaration, getNodeId);
}
}
else {
const parameter = local.valueDeclaration && tryGetRootParameterDeclaration(local.valueDeclaration);
if (parameter) {
Expand All @@ -22744,32 +22742,63 @@ namespace ts {
});
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)));
const nDeclarations = (importClause.name ? 1 : 0) +
(importClause.namedBindings ?
(importClause.namedBindings.kind === SyntaxKind.NamespaceImport ? 1 : importClause.namedBindings.elements.length)
: 0);
if (nDeclarations === unuseds.length) {
addDiagnostic(UnusedKind.Local, unuseds.length === 1
? createDiagnosticForNode(importDecl, Diagnostics._0_is_declared_but_its_value_is_never_read, idText(first(unuseds).name))
: createDiagnosticForNode(importDecl, Diagnostics.All_imports_in_import_declaration_are_unused));
}
else {
addDiagnostic(UnusedKind.Local, createDiagnosticForNode(importDecl, Diagnostics.All_imports_in_import_declaration_are_unused));
for (const unused of unuseds) errorUnusedLocal(unused, idText(unused.name), addDiagnostic);
}
});
unusedDestructures.forEach(([bindingPattern, bindingElements]) => {
const kind = tryGetRootParameterDeclaration(bindingPattern.parent) ? UnusedKind.Parameter : UnusedKind.Local;
if (!bindingPattern.elements.every(e => contains(bindingElements, e))) {
if (bindingPattern.elements.length === bindingElements.length) {
if (bindingElements.length === 1 && bindingPattern.parent.kind === SyntaxKind.VariableDeclaration && bindingPattern.parent.parent.kind === SyntaxKind.VariableDeclarationList) {
addToGroup(unusedVariables, bindingPattern.parent.parent, bindingPattern.parent, getNodeId);
}
else {
addDiagnostic(kind, bindingElements.length === 1
? createDiagnosticForNode(bindingPattern, Diagnostics._0_is_declared_but_its_value_is_never_read, idText(cast(first(bindingElements).name, isIdentifier)))
: createDiagnosticForNode(bindingPattern, Diagnostics.All_destructured_elements_are_unused));
}
}
else {
for (const e of bindingElements) {
addDiagnostic(kind, createDiagnosticForNode(e, Diagnostics._0_is_declared_but_its_value_is_never_read, idText(cast(e.name, isIdentifier))));
}
}
else if (bindingElements.length === 1) {
addDiagnostic(kind, createDiagnosticForNode(bindingPattern, Diagnostics._0_is_declared_but_its_value_is_never_read, idText(cast(first(bindingElements).name, isIdentifier))));
});
unusedVariables.forEach(([declarationList, declarations]) => {
if (declarationList.declarations.length === declarations.length) {
addDiagnostic(UnusedKind.Local, declarations.length === 1
? createDiagnosticForNode(first(declarations).name, Diagnostics._0_is_declared_but_its_value_is_never_read, bindingNameText(first(declarations).name))
: createDiagnosticForNode(declarationList.parent.kind === SyntaxKind.VariableStatement ? declarationList.parent : declarationList, Diagnostics.All_variables_are_unused));
}
else {
addDiagnostic(kind, createDiagnosticForNode(bindingPattern, Diagnostics.All_destructured_elements_are_unused));
for (const decl of declarations) {
addDiagnostic(UnusedKind.Local, createDiagnosticForNode(decl, Diagnostics._0_is_declared_but_its_value_is_never_read, idText(cast(decl.name, isIdentifier))));
}
}
});
}

function bindingNameText(name: BindingName): string {
switch (name.kind) {
case SyntaxKind.Identifier:
return idText(name);
case SyntaxKind.ArrayBindingPattern:
case SyntaxKind.ObjectBindingPattern:
return bindingNameText(cast(first(name.elements), isBindingElement).name);
default:
return Debug.assertNever(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;
Expand All @@ -22778,12 +22807,6 @@ namespace ts {
return decl.kind === SyntaxKind.ImportClause ? decl : decl.kind === SyntaxKind.NamespaceImport ? decl.parent : decl.parent.parent;
}

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
9 changes: 9 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -3556,6 +3556,11 @@
"code": 6198,
"reportsUnnecessary": true
},
"All variables are unused.": {
"category": "Error",
"code": 6199,
"reportsUnnecessary": true
},

"Projects to reference": {
"category": "Message",
Expand Down Expand Up @@ -3994,6 +3999,10 @@
"category": "Message",
"code": 90009
},
"Remove variable statement": {
"category": "Message",
"code": 90010
},
"Import '{0}' from module \"{1}\"": {
"category": "Message",
"code": 90013
Expand Down
38 changes: 26 additions & 12 deletions src/services/codefixes/fixUnusedIdentifier.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,27 @@ namespace ts.codefix {
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,
Diagnostics.All_variables_are_unused.code,
];
registerCodeFix({
errorCodes,
getCodeActions(context) {
const { errorCode, sourceFile } = context;
const importDecl = tryGetFullImport(sourceFile, context.span.start);
const startToken = getTokenAtPosition(sourceFile, context.span.start, /*includeJsDocComment*/ false);

const importDecl = tryGetFullImport(startToken);
if (importDecl) {
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, /*deleted*/ undefined));
const delDestructure = textChanges.ChangeTracker.with(context, t => tryDeleteFullDestructure(t, sourceFile, startToken, /*deleted*/ undefined));
if (delDestructure.length) {
return [createCodeFixAction(fixName, delDestructure, Diagnostics.Remove_destructuring, fixIdDelete, Diagnostics.Delete_all_unused_declarations)];
}
const delVar = textChanges.ChangeTracker.with(context, t => tryDeleteFullVariableStatement(t, sourceFile, startToken, /*deleted*/ undefined));
if (delVar.length) {
return [createCodeFixAction(fixName, delDestructure, Diagnostics.Remove_variable_statement, fixIdDelete, Diagnostics.Delete_all_unused_declarations)];
}

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

const importDecl = tryGetFullImport(diag.file!, diag.start!);
const importDecl = tryGetFullImport(startToken);
if (importDecl) {
changes.deleteNode(sourceFile, importDecl);
}
else {
if (!tryDeleteFullDestructure(changes, sourceFile, diag.start!, deleted)) {
tryDeleteDeclaration(changes, sourceFile, token, deleted);
}
else if (!tryDeleteFullDestructure(changes, sourceFile, startToken, deleted) && !tryDeleteFullVariableStatement(changes, sourceFile, startToken, deleted)) {
tryDeleteDeclaration(changes, sourceFile, token, deleted);
}
break;
default:
Expand All @@ -74,15 +80,13 @@ namespace ts.codefix {
});

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

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

function tryDeleteFullVariableStatement(changes: textChanges.ChangeTracker, sourceFile: SourceFile, startToken: Node, deletedAncestors: NodeSet | undefined) {
const declarationList = tryCast(startToken.parent, isVariableDeclarationList);
if (declarationList && declarationList.getChildren(sourceFile)[0] === startToken) {
if (deletedAncestors) deletedAncestors.add(declarationList);
changes.deleteNode(sourceFile, declarationList.parent.kind === SyntaxKind.VariableStatement ? declarationList.parent : declarationList);
return true;
}
return false;
}

function getToken(sourceFile: SourceFile, pos: number): Node {
const token = findPrecedingToken(pos, sourceFile, /*startNode*/ undefined, /*includeJsDoc*/ true);
// this handles var ["computed"] = 12;
Expand Down
14 changes: 9 additions & 5 deletions tests/baselines/reference/unusedDestructuring.errors.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,14 @@ tests/cases/compiler/unusedDestructuring.ts(3,7): error TS6198: All destructured
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(7,7): error TS6133: 'g' is declared but its value is never read.
tests/cases/compiler/unusedDestructuring.ts(9,1): error TS6133: 'f' is declared but its value is never read.
tests/cases/compiler/unusedDestructuring.ts(9,12): error TS6198: All destructured elements are unused.
tests/cases/compiler/unusedDestructuring.ts(9,24): error TS6133: 'c' is declared but its value is never read.
tests/cases/compiler/unusedDestructuring.ts(9,32): error TS6133: 'e' is declared but its value is never read.
tests/cases/compiler/unusedDestructuring.ts(8,1): error TS6199: All variables are unused.
tests/cases/compiler/unusedDestructuring.ts(10,1): error TS6133: 'f' is declared but its value is never read.
tests/cases/compiler/unusedDestructuring.ts(10,12): error TS6198: All destructured elements are unused.
tests/cases/compiler/unusedDestructuring.ts(10,24): error TS6133: 'c' is declared but its value is never read.
tests/cases/compiler/unusedDestructuring.ts(10,32): error TS6133: 'e' is declared but its value is never read.


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

function f({ a, b }, { c, d }, { e }) {
~~~~~~~~~~
Expand Down
2 changes: 2 additions & 0 deletions tests/baselines/reference/unusedDestructuring.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const { c, d } = o;
d;
const { e } = o;
const { f: g } = o;
const { h } = o, { i } = o;

function f({ a, b }, { c, d }, { e }) {
d;
Expand All @@ -20,6 +21,7 @@ var c = o.c, d = o.d;
d;
var e = o.e;
var g = o.f;
var h = o.h, i = o.i;
function f(_a, _b, _c) {
var a = _a.a, b = _a.b;
var c = _b.c, d = _b.d;
Expand Down
20 changes: 13 additions & 7 deletions tests/baselines/reference/unusedDestructuring.symbols
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,21 @@ const { f: g } = o;
>g : Symbol(g, Decl(unusedDestructuring.ts, 6, 7))
>o : Symbol(o, Decl(unusedDestructuring.ts, 1, 13))

const { h } = o, { i } = o;
>h : Symbol(h, Decl(unusedDestructuring.ts, 7, 7))
>o : Symbol(o, Decl(unusedDestructuring.ts, 1, 13))
>i : Symbol(i, Decl(unusedDestructuring.ts, 7, 18))
>o : Symbol(o, Decl(unusedDestructuring.ts, 1, 13))

function f({ a, b }, { c, d }, { e }) {
>f : Symbol(f, Decl(unusedDestructuring.ts, 6, 19))
>a : Symbol(a, Decl(unusedDestructuring.ts, 8, 12))
>b : Symbol(b, Decl(unusedDestructuring.ts, 8, 15))
>c : Symbol(c, Decl(unusedDestructuring.ts, 8, 22))
>d : Symbol(d, Decl(unusedDestructuring.ts, 8, 25))
>e : Symbol(e, Decl(unusedDestructuring.ts, 8, 32))
>f : Symbol(f, Decl(unusedDestructuring.ts, 7, 27))
>a : Symbol(a, Decl(unusedDestructuring.ts, 9, 12))
>b : Symbol(b, Decl(unusedDestructuring.ts, 9, 15))
>c : Symbol(c, Decl(unusedDestructuring.ts, 9, 22))
>d : Symbol(d, Decl(unusedDestructuring.ts, 9, 25))
>e : Symbol(e, Decl(unusedDestructuring.ts, 9, 32))

d;
>d : Symbol(d, Decl(unusedDestructuring.ts, 8, 25))
>d : Symbol(d, Decl(unusedDestructuring.ts, 9, 25))
}

6 changes: 6 additions & 0 deletions tests/baselines/reference/unusedDestructuring.types
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,12 @@ const { f: g } = o;
>g : any
>o : any

const { h } = o, { i } = o;
>h : any
>o : any
>i : 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
Expand Down
11 changes: 4 additions & 7 deletions tests/baselines/reference/unusedLocalsInMethod2.errors.txt
Original file line number Diff line number Diff line change
@@ -1,15 +1,12 @@
tests/cases/compiler/unusedLocalsInMethod2.ts(3,13): error TS6133: 'x' is declared but its value is never read.
tests/cases/compiler/unusedLocalsInMethod2.ts(3,16): error TS6133: 'y' is declared but its value is never read.
tests/cases/compiler/unusedLocalsInMethod2.ts(3,9): error TS6199: All variables are unused.


==== tests/cases/compiler/unusedLocalsInMethod2.ts (2 errors) ====
==== tests/cases/compiler/unusedLocalsInMethod2.ts (1 errors) ====
class greeter {
public function1() {
var x, y = 10;
~
!!! error TS6133: 'x' is declared but its value is never read.
~
!!! error TS6133: 'y' is declared but its value is never read.
~~~~~~~~~~~~~~
!!! error TS6199: All variables are unused.
y++;
}
}
Loading