Codefix for removing Unused Identifiers by paulvanbrenk · Pull Request #11546 · microsoft/TypeScript

paulvanbrenk · 2016-10-12T02:53:18Z

Quick fix for removing unused variables and unused locals.

Current known limitations:

no support for the spread operator
no support for the for ... in loop

mhegazy · 2016-10-12T22:13:26Z

src/services/codefixes/unusedIdentifierFixes.ts

+            const start = context.span.start;
+            const token = getTokenAtPosition(sourceFile, start);
+
+            if (token.kind === ts.SyntaxKind.Identifier) {


please refactor this into a switch statement.

mhegazy · 2016-10-12T22:24:16Z

src/services/codefixes/unusedIdentifierFixes.ts

+                    return createCodeFix("{}", token.parent.pos, token.parent.end - token.parent.pos);
+                }
+
+                if (token.parent.kind === SyntaxKind.ImportEqualsDeclaration) {


what does this mean. this is import m = require("mod");, you want to remove the whole declaration, and not just the identifier.

Also, please add a test for this.

This is import a = A

Tests are here, changed this to remove the whole line.

mhegazy · 2016-10-12T22:32:19Z

src/services/codefixes/unusedIdentifierFixes.ts

+            }
+
+            if (token.kind === SyntaxKind.AsteriskToken && token.parent.kind === SyntaxKind.NamespaceImport) {
+                return createCodeFix("{}", token.parent.pos, token.parent.end - token.parent.pos);


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;

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.

mhegazy · 2016-10-12T22:40:59Z

tests/cases/fourslash/unusedParameterInConstructor1.ts

+////    [|constructor(private p1: string, public p2: boolean, public p3: any, p5)|] { p5; }
+//// }
+
+verify.codeFixAtPosition("constructor(public p2: boolean, public p3: any, p5)");


please add another test with private readonly and another one with private is not in the first position.

mhegazy · 2016-10-12T22:51:16Z

src/services/codefixes/unusedIdentifierFixes.ts

+                    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);


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.

mhegazy · 2016-10-12T22:54:02Z

src/services/codefixes/unusedIdentifierFixes.ts

+                }
+
+                if (token.parent.kind === SyntaxKind.TypeParameter) {
+                    const typeParameters = (<ClassDeclaration>token.parent.parent).typeParameters;


why ClassDeclaraton? that can be function, interface declaration, method, type alias, arrow function, etc.. please add tests for these.

Used DeclWIthTypeParam, tests were already there.

mhegazy · 2016-10-12T22:54:35Z

src/services/codefixes/unusedIdentifierFixes.ts

+                }
+
+                if (token.parent.kind === ts.SyntaxKind.Parameter) {
+                    const functionDeclaration = <FunctionDeclaration>token.parent.parent;


this is the same logic as above, please merge these two branches.

mhegazy · 2016-10-12T23:00:05Z

src/services/codefixes/unusedIdentifierFixes.ts

+                    token.parent.kind === SyntaxKind.MethodDeclaration ||
+                    token.parent.kind === SyntaxKind.ModuleDeclaration ||
+                    token.parent.kind === SyntaxKind.PropertyDeclaration ||
+                    token.parent.kind === SyntaxKind.ArrowFunction) {


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); }

Also please add handeling for computed properties with non-identifier names:

class C { private ["string"] (){} private [Symbol.Iterator]() {} }

mhegazy · 2016-10-13T20:59:12Z

src/services/codefixes/unusedIdentifierFixes.ts

+                    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);


this is not valid for..in statement. we should switch it x to _x and add special casing for _x to be not show the error. alternatively do nothing here, which would be my recommendation.

Doing nothing, modified test to make sure.

mhegazy · 2016-10-13T21:03:43Z

src/services/codefixes/unusedIdentifierFixes.ts

+                    }
+                    else {
+                        const variableStatement = <VariableStatement>token.parent.parent.parent;
+                        if (variableStatement.declarationList.declarations.length === 1) {


consider sharing the logic here with the other list removal cases.

mhegazy · 2016-10-13T21:06:36Z

src/services/codefixes/unusedIdentifierFixes.ts

+                    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) {


consider merging this into a list removal logic for all of the cases below.

mhegazy · 2016-10-13T21:07:15Z

src/services/codefixes/unusedIdentifierFixes.ts

+                            }
+                        }
+                    }
+                }


what about binding patterns? can you add tests for these cases. both for parameters and variable declarations.

mhegazy · 2016-10-13T21:09:15Z

src/services/codefixes/unusedIdentifierFixes.ts

+                    token.parent.kind === SyntaxKind.MethodDeclaration ||
+                    token.parent.kind === SyntaxKind.ModuleDeclaration ||
+                    token.parent.kind === SyntaxKind.PropertyDeclaration ||
+                    token.parent.kind === SyntaxKind.ArrowFunction) {


Also please add handeling for computed properties with non-identifier names:

class C { private ["string"] (){} private [Symbol.Iterator]() {} }

RyanCavanaugh · 2016-10-24T17:54:31Z

tests/cases/fourslash/unusedLocalsInFunction2.ts

+////    x++;
+////}
+
+verify.codeFixAtPosition("var x;", 6133);


No error codes in fourslash tests

mhegazy · 2016-10-26T03:23:03Z

src/services/codefixes/unusedIdentifierFixes.ts

+                                    break;
+
+                                case SyntaxKind.ForInStatement:
+                                    // There is no valid fix in the case of:


rename the variable to start with _.

Out of scope, will add for 2nd iteration of this fix

mhegazy · 2016-10-26T03:27:08Z

src/services/codefixes/unusedIdentifierFixes.ts

+                                    }
+                            }
+
+                        case SyntaxKind.FunctionDeclaration:


namespace, type alais, and enum as well

i would move this to the default clause and use isDeclarationName

mhegazy · 2016-10-26T03:34:37Z

src/services/codefixes/unusedIdentifierFixes.ts

+                        case SyntaxKind.ImportSpecifier:
+                            const namedImports = <NamedImports>token.parent.parent;
+                            const elements = namedImports.elements;
+                            if (elements.length === 1) {


if no other imports in this list we need to remove the whole module import.

mhegazy · 2016-10-26T03:35:48Z

src/services/codefixes/unusedIdentifierFixes.ts

+                    }
+                    break;
+
+                case SyntaxKind.PrivateKeyword:


When is this ever used?

mhegazy · 2016-10-26T03:41:38Z

src/services/codefixes/unusedIdentifierFixes.ts

+            const token = getTokenAtPosition(sourceFile, start);
+
+            switch (token.kind) {
+                case ts.SyntaxKind.Identifier:


binding patterns are not handled anywhere:

function f({a, b}) {}

outside of scope, I think we can do a better job when we add the change signature refactoring

mhegazy · 2016-10-26T03:42:48Z

src/services/codefixes/unusedIdentifierFixes.ts

+            const token = getTokenAtPosition(sourceFile, start);
+
+            switch (token.kind) {
+                case ts.SyntaxKind.Identifier:


Also please add handeling for computed properties with non-identifier names:

class C { private ["string"] (){} private [Symbol.Iterator]() {} }

mhegazy · 2016-10-26T03:50:09Z

tests/cases/fourslash/unusedImports12FS.ts

+
+// @noUnusedLocals: true
+// @Filename: file2.ts
+//// [| import f1, * as s from "./file1"; |]


Please add a test with both unused.

That generates 2 errors and 2 fixes. So not applicable

mhegazy · 2016-10-26T03:52:19Z

src/services/codefixes/unusedIdentifierFixes.ts

+                case SyntaxKind.PropertyDeclaration:
+                    return createCodeFix("", token.parent.pos, token.parent.end - token.parent.pos);
+
+                case SyntaxKind.AsteriskToken:


when do we get a * token?

Paul van Brenk added 2 commits October 11, 2016 15:04

Codefixes for unused variables, properties etc.

05ec512

Merge branch 'pvb/codeaction/api' into unusedidentifier

28c08fd

msftclas added the cla-not-required label Oct 12, 2016

mhegazy suggested changes Oct 13, 2016

View reviewed changes

Paul van Brenk added 3 commits October 21, 2016 14:56

Major refactoring after PR feedback

03a6eeb

Add tests for different constructor parameter cases.

79fa9db

Remove unused case, and fix for..in

1f94e14

RyanCavanaugh reviewed Oct 24, 2016

View reviewed changes

Paul van Brenk added 2 commits October 24, 2016 11:40

Fix 'import a = A' case

4097874

Simplifying handling import cases

d4b99d6

mhegazy suggested changes Oct 26, 2016

View reviewed changes

Paul van Brenk added 5 commits November 1, 2016 16:31

Add more testcases and simplify

b59714e

Removed unused, confusing cases

7196018

Handle most module cases

2f453ce

Clean up

40c0cbd

Remove error-codes from fourslash tests

78fdd44

mhegazy approved these changes Nov 14, 2016

View reviewed changes

Merge branch 'master' into unusedidentifier

1924298

paulvanbrenk merged commit 57feab3 into master Nov 15, 2016

paulvanbrenk deleted the unusedidentifier branch November 15, 2016 18:30

microsoft locked and limited conversation to collaborators Jun 19, 2018

Conversation

paulvanbrenk commented Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhegazy Oct 13, 2016 • edited by paulvanbrenk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulvanbrenk Oct 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhegazy Oct 13, 2016 • edited by paulvanbrenk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulvanbrenk commented Oct 12, 2016 •

edited

Loading

mhegazy Oct 13, 2016 •

edited by paulvanbrenk

Loading

paulvanbrenk Oct 21, 2016 •

edited

Loading

mhegazy Oct 13, 2016 •

edited by paulvanbrenk

Loading