-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Codefix for removing Unused Identifiers #11546
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
Changes from 2 commits
05ec512
28c08fd
03a6eeb
79fa9db
1f94e14
4097874
d4b99d6
b59714e
7196018
2f453ce
40c0cbd
78fdd44
1924298
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,2 @@ | ||
| ///<reference path='superFixes.ts' /> | ||
| ///<reference path='unusedIdentifierFixes.ts' /> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,153 @@ | ||
| /* @internal */ | ||
| namespace ts.codefix { | ||
| registerCodeFix({ | ||
| errorCodes: [ | ||
| Diagnostics._0_is_declared_but_never_used.code, | ||
| Diagnostics.Property_0_is_declared_but_never_used.code | ||
| ], | ||
| getCodeActions: (context: CodeFixContext) => { | ||
| const sourceFile = context.sourceFile; | ||
| const start = context.span.start; | ||
| const token = getTokenAtPosition(sourceFile, start); | ||
|
|
||
| if (token.kind === ts.SyntaxKind.Identifier) { | ||
| if (token.parent.kind === ts.SyntaxKind.VariableDeclaration) { | ||
| if (token.parent.parent.parent.kind === SyntaxKind.ForStatement) { | ||
| const forStatement = <ForStatement>token.parent.parent.parent; | ||
| const initializer = <VariableDeclarationList>forStatement.initializer; | ||
| if (initializer.declarations.length === 1) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider merging this into a list removal logic for all of the cases below.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| return createCodeFix("", initializer.pos, initializer.end - initializer.pos); | ||
| } | ||
| else { | ||
| if (initializer.declarations[0] === token.parent) { | ||
| return createCodeFix("", token.parent.pos, token.parent.end - token.parent.pos + 1); | ||
| } | ||
| else { | ||
| return createCodeFix("", token.parent.pos - 1, token.parent.end - token.parent.pos + 1); | ||
| } | ||
| } | ||
| } | ||
| else if (token.parent.parent.parent.kind === SyntaxKind.ForInStatement) { | ||
| const forInStatement = <ForInStatement>token.parent.parent.parent; | ||
| const initializer = <VariableDeclarationList>forInStatement.initializer; | ||
| return createCodeFix("{}", initializer.declarations[0].pos, initializer.declarations[0].end - initializer.declarations[0].pos); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is not valid
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doing nothing, modified test to make sure. |
||
| } | ||
| else if (token.parent.parent.parent.kind === SyntaxKind.ForOfStatement) { | ||
| const forOfStatement = <ForOfStatement>token.parent.parent.parent; | ||
| const initializer = <VariableDeclarationList>forOfStatement.initializer; | ||
| return createCodeFix("{}", initializer.declarations[0].pos, initializer.declarations[0].end - initializer.declarations[0].pos); | ||
| } | ||
| else if (token.parent.parent.kind === SyntaxKind.CatchClause) { | ||
| const catchClause = <CatchClause>token.parent.parent; | ||
| const parameter = catchClause.variableDeclaration.getChildren()[0]; | ||
| return createCodeFix("", parameter.pos, parameter.end - parameter.pos); | ||
| } | ||
| else { | ||
| const variableStatement = <VariableStatement>token.parent.parent.parent; | ||
| if (variableStatement.declarationList.declarations.length === 1) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider sharing the logic here with the other list removal cases.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| return createCodeFix("", variableStatement.pos, variableStatement.end - variableStatement.pos); | ||
| } | ||
| else { | ||
| const declarations = variableStatement.declarationList.declarations; | ||
| if (declarations[0].name === token) { | ||
| return createCodeFix("", token.parent.pos + 1, token.parent.end - token.parent.pos); | ||
| } | ||
| else { | ||
| return createCodeFix("", token.parent.pos - 1, token.parent.end - token.parent.pos + 1); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what about binding patterns? can you add tests for these cases. both for parameters and variable declarations. |
||
|
|
||
| if (token.parent.kind === SyntaxKind.FunctionDeclaration || | ||
| token.parent.kind === SyntaxKind.ClassDeclaration || | ||
| token.parent.kind === SyntaxKind.InterfaceDeclaration || | ||
| token.parent.kind === SyntaxKind.MethodDeclaration || | ||
| token.parent.kind === SyntaxKind.ModuleDeclaration || | ||
| token.parent.kind === SyntaxKind.PropertyDeclaration || | ||
| token.parent.kind === SyntaxKind.ArrowFunction) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how can an arrowFunction have an identifier as a name? consider changing this to: if (isDeclaration(token.parent) && (<Declaration>parent).name === token) {
return createCodeFix("", token.parent.pos, token.parent.end - token.parent.pos);
}
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also please add handeling for computed properties with non-identifier names: class C {
private ["string"] (){}
private [Symbol.Iterator]() {}
} |
||
| return createCodeFix("", token.parent.pos, token.parent.end - token.parent.pos); | ||
| } | ||
|
|
||
| if (token.parent.kind === SyntaxKind.TypeParameter) { | ||
| const typeParameters = (<ClassDeclaration>token.parent.parent).typeParameters; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why ClassDeclaraton? that can be function, interface declaration, method, type alias, arrow function, etc.. please add tests for these.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Used DeclWIthTypeParam, tests were already there. |
||
| if (typeParameters.length === 1) { | ||
| return createCodeFix("", token.parent.pos - 1, token.parent.end - token.parent.pos + 2); | ||
| } | ||
| else { | ||
| if (typeParameters[0] === token.parent) { | ||
| return createCodeFix("", token.parent.pos, token.parent.end - token.parent.pos + 1); | ||
| } | ||
| else { | ||
| return createCodeFix("", token.parent.pos - 1, token.parent.end - token.parent.pos + 1); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (token.parent.kind === ts.SyntaxKind.Parameter) { | ||
| const functionDeclaration = <FunctionDeclaration>token.parent.parent; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is the same logic as above, please merge these two branches.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| if (functionDeclaration.parameters.length === 1) { | ||
| return createCodeFix("", token.parent.pos, token.parent.end - token.parent.pos); | ||
| } | ||
| else { | ||
| if (functionDeclaration.parameters[0] === token.parent) { | ||
| return createCodeFix("", token.parent.pos, token.parent.end - token.parent.pos + 1); | ||
| } | ||
| else { | ||
| return createCodeFix("", token.parent.pos - 1, token.parent.end - token.parent.pos + 1); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (token.parent.kind === SyntaxKind.ImportSpecifier) { | ||
| const namedImports = <NamedImports>token.parent.parent; | ||
| const elements = namedImports.elements; | ||
| if (elements.length === 1) { | ||
| // Only 1 import and it is unused. So the entire line could be removed. | ||
| return createCodeFix("", token.parent.pos, token.parent.end - token.parent.pos); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is not removing the whole line? do you mean namedImports.parent? I think you need to handle all cases. i have listed them below, also please add tests for each combination. |
||
| } | ||
| else { | ||
| if (elements[0] === token.parent) { | ||
| return createCodeFix("", token.parent.pos, token.parent.end - token.parent.pos + 1); | ||
| } | ||
| else { | ||
| return createCodeFix("", token.parent.pos - 1, token.parent.end - token.parent.pos + 1); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (token.parent.parent.kind === SyntaxKind.ImportClause || token.parent.parent.kind === SyntaxKind.ImportDeclaration) { | ||
| return createCodeFix("{}", token.parent.pos, token.parent.end - token.parent.pos); | ||
| } | ||
|
|
||
| if (token.parent.kind === SyntaxKind.ImportEqualsDeclaration) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what does this mean. this is
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, please add a test for this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tests are here, changed this to remove the whole line. |
||
| return createCodeFix("{}", token.pos, token.end - token.pos); | ||
| } | ||
|
|
||
| if (token.parent.kind === SyntaxKind.EnumDeclaration) { | ||
| return createCodeFix("", token.parent.pos, token.parent.end - token.parent.pos); | ||
| } | ||
| } | ||
|
|
||
| if (token.kind === SyntaxKind.PrivateKeyword && token.parent.kind === SyntaxKind.PropertyDeclaration) { | ||
| return createCodeFix("", token.parent.pos, token.parent.end - token.parent.pos); | ||
| } | ||
|
|
||
| if (token.kind === SyntaxKind.AsteriskToken && token.parent.kind === SyntaxKind.NamespaceImport) { | ||
| return createCodeFix("{}", token.parent.pos, token.parent.end - token.parent.pos); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do not think this is correct either. you need to check if this is the only import for this module, remove it whole sale, if not, remove whole import declaration. also please add tests for the deffrent combinations of import: import * as ns from "mod";
import d from "mod"
import {a, b} from "mod" // where both are unused
import {a, b} from "mod" // where only one is unused
import d, * as ns from "mod"; // where both are unused
import d, * as ns from "mod"; // where one of them is unsued, but the other is
import d, {a, b} from "mod"; // where one of a,b, or d is unused, but other are not
import d, {a, b} from "mod"; // where both a and b are unused, but d is not
import d, {a, b} from "mod"; // where only d is unsued
import d, {a, b} from "mod"; // where all are unused
import m = require("mod");
import m = n.a;
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the cases where more than 1 import are unused, will result in 2 errors, and 2 fixes; so those would be identical to the single unused import. All other cases have test cases. |
||
| } | ||
|
|
||
| return undefined; | ||
|
|
||
| function createCodeFix(newText: string, start: number, length: number): CodeAction[] { | ||
| return [{ | ||
| description: getLocaleSpecificMessage(Diagnostics.Remove_unused_identifiers), | ||
| changes: [{ | ||
| fileName: sourceFile.fileName, | ||
| textChanges: [{ newText, span: { start, length } }] | ||
| }] | ||
| }]; | ||
| } | ||
| } | ||
| }); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| /// <reference path='fourslash.ts' /> | ||
|
|
||
| // @noUnusedLocals: true | ||
| //// [| namespace greeter { | ||
| //// class class1 { | ||
| //// } | ||
| //// } |] | ||
|
|
||
| verify.codeFixAtPosition(`namespace greeter { | ||
| }`); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| /// <reference path='fourslash.ts' /> | ||
|
|
||
| // @noUnusedLocals: true | ||
| //// [| namespace greeter { | ||
| //// export class class2 { | ||
| //// } | ||
| //// class class1 { | ||
| //// } | ||
| //// } |] | ||
|
|
||
| verify.codeFixAtPosition(`namespace greeter { | ||
| export class class2 { | ||
| } | ||
| }`); | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| /// <reference path='fourslash.ts' /> | ||
|
|
||
| // @noUnusedLocals: true | ||
| // @noUnusedParameters:true | ||
| //// [| namespace Validation { | ||
| //// class c1 { | ||
| //// | ||
| //// }/*1*/ | ||
| //// } |] | ||
|
|
||
| verify.codeFixAtPosition(`namespace Validation { | ||
| }`); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| /// <reference path='fourslash.ts' /> | ||
|
|
||
| // @noUnusedLocals: true | ||
| // @noUnusedParameters:true | ||
|
|
||
| //// [| namespace Validation { | ||
| //// class c1 { | ||
| //// | ||
| //// } | ||
| //// | ||
| //// export class c2 { | ||
| //// | ||
| //// } | ||
| //// } |] | ||
|
|
||
| verify.codeFixAtPosition(`namespace Validation { | ||
| export class c2 { | ||
| } | ||
| }`); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| /// <reference path='fourslash.ts' /> | ||
|
|
||
| // @noUnusedLocals: true | ||
| // @noUnusedParameters:true | ||
|
|
||
| //// [| namespace Validation { | ||
| //// class c1 { | ||
| //// | ||
| //// } | ||
| //// | ||
| //// export class c2 { | ||
| //// | ||
| //// } | ||
| //// | ||
| //// class c3 extends c1 { | ||
| //// | ||
| //// } | ||
| ////} |] | ||
|
|
||
| verify.codeFixAtPosition(`namespace Validation { | ||
| class c1 { | ||
| } | ||
|
|
||
| export class c2 { | ||
| } | ||
| }`); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| /// <reference path='fourslash.ts' /> | ||
|
|
||
| // @noUnusedLocals: true | ||
| // @noUnusedParameters:true | ||
| //// [| namespace Validation { | ||
| //// class c1 { | ||
| //// | ||
| //// } | ||
| //// | ||
| //// export class c2 { | ||
| //// | ||
| //// } | ||
| //// | ||
| //// class c3 { | ||
| //// public x: c1; | ||
| //// } | ||
| ////} |] | ||
|
|
||
| verify.codeFixAtPosition(`namespace Validation { | ||
| class c1 { | ||
|
|
||
| } | ||
|
|
||
| export class c2 { | ||
|
|
||
| } | ||
| }`); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| /// <reference path='fourslash.ts' /> | ||
|
|
||
| // @noUnusedLocals: true | ||
| //// [| function f1 () { | ||
| //// const x: string = "x"; | ||
| //// } |] | ||
|
|
||
| verify.codeFixAtPosition(`function f1 () { | ||
| }`); | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| /// <reference path='fourslash.ts' /> | ||
|
|
||
| // @noUnusedLocals: true | ||
| //// [| function f1 () { | ||
| //// enum Directions { Up, Down} | ||
| //// } |] | ||
|
|
||
| verify.codeFixAtPosition(`function f1 () { | ||
| } | ||
| `); | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| /// <reference path='fourslash.ts' /> | ||
|
|
||
| // @noUnusedLocals: true | ||
| //// [| namespace greeter { | ||
| //// function function1() { | ||
| //// }/*1*/ | ||
| //// } |] | ||
|
|
||
| verify.codeFixAtPosition(`namespace greeter { | ||
| }`); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| /// <reference path='fourslash.ts' /> | ||
|
|
||
| // @noUnusedLocals: true | ||
| //// [| namespace greeter { | ||
| //// export function function2() { | ||
| //// } | ||
| //// function function1() { | ||
| //// } | ||
| ////} |] | ||
|
|
||
| verify.codeFixAtPosition(`namespace greeter { | ||
| export function function2() { | ||
| } | ||
| }`); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| /// <reference path='fourslash.ts' /> | ||
|
|
||
| // @noUnusedLocals: true | ||
| // @noUnusedParameters:true | ||
|
|
||
| //// [| namespace Validation { | ||
| //// function function1() { | ||
| //// } | ||
| ////} |] | ||
|
|
||
| verify.codeFixAtPosition(`namespace Validation { | ||
| }`); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| /// <reference path='fourslash.ts' /> | ||
|
|
||
| // @noUnusedLocals: true | ||
| // @noUnusedParameters:true | ||
| //// [| namespace Validation { | ||
| //// var function1 = function() { | ||
| //// } | ||
| ////} |] | ||
|
|
||
| verify.codeFixAtPosition(`namespace Validation { | ||
| }`); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| /// <reference path='fourslash.ts' /> | ||
|
|
||
| // @noUnusedLocals: true | ||
| // @noUnusedParameters:true | ||
| ////namespace Validation { | ||
| //// var function1 = function() { | ||
| //// } | ||
| //// | ||
| //// export function function2() { | ||
| //// | ||
| //// } | ||
| //// | ||
| //// [| function function3() { | ||
| //// function1(); | ||
| //// } | ||
| //// | ||
| //// function function4() { | ||
| //// | ||
| //// } | ||
| //// | ||
| //// exp ort let a = function3; |] | ||
| ////} | ||
|
|
||
| verify.codeFixAtPosition(`function function3() { | ||
| function1(); | ||
| } | ||
|
|
||
| export let a = function3;`, 6133); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please refactor this into a switch statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done