Remove-unused-identifiers codefix skips assigned identifiers (#41168)

* Remove-all-unused-identifiers skips assigned identifiers

Previously, fixUnusedIdentifier worked the same in fix-all mode as for a
single fix: identifiers with assignments would be deleted:

```ts
function f(a) { }
f(1)
```

becomes

```ts
function f() { }
f()
```

But any kind of argument will be deleted, even one with side effects.
For a single codefix invocation, this is probably OK.
But for fix-all, this could lead to multiple changes
spread throughout a large file.

Now fix-all will only delete parameters and variable declarations with
no assignments:

```ts
function f(a) { }
function g(a) { }
f(1)
g
let x = 1
let y
```

becomes

```
function f(a) { }
function g() { }
f(1)
g
let x = 1
```

* Don't remove assigned parameters for single codefix either

* add optional parameter test case

* Skip initialised params and binding elements

Based on PR feedback from @amcasey

* fixAll removes unused binding patterns completely

* Fixes from comments

Thanks @amcasey for the thorough review

* fix trailing space lint

* correctly remove-all array binding

src/compiler/checker.ts

@@ -33401,7 +33401,7 @@ namespace ts {
         function checkUnusedLocalsAndParameters(nodeWithLocals: Node, addDiagnostic: AddUnusedDiagnostic): void {
             // 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 = new Map<string, [ImportClause, ImportedDeclaration[]]>();
-            const unusedDestructures = new Map<string, [ObjectBindingPattern, BindingElement[]]>();
+            const unusedDestructures = new Map<string, [BindingPattern, BindingElement[]]>();
             const unusedVariables = new Map<string, [VariableDeclarationList, VariableDeclaration[]]>();
             nodeWithLocals.locals!.forEach(local => {
                 // If it's purely a type parameter, ignore, will be checked in `checkUnusedTypeParameters`.
@@ -33433,7 +33433,12 @@ namespace ts {
                         const name = local.valueDeclaration && getNameOfDeclaration(local.valueDeclaration);
                         if (parameter && name) {
                             if (!isParameterPropertyDeclaration(parameter, parameter.parent) && !parameterIsThisKeyword(parameter) && !isIdentifierThatStartsWithUnderscore(name)) {
-                                addDiagnostic(parameter, UnusedKind.Parameter, createDiagnosticForNode(name, Diagnostics._0_is_declared_but_its_value_is_never_read, symbolName(local)));
+                                if (isBindingElement(declaration) && isArrayBindingPattern(declaration.parent)) {
+                                    addToGroup(unusedDestructures, declaration.parent, declaration, getNodeId);
+                                }
+                                else {
+                                    addDiagnostic(parameter, UnusedKind.Parameter, createDiagnosticForNode(name, Diagnostics._0_is_declared_but_its_value_is_never_read, symbolName(local)));
+                                }
                             }
                         }
                         else {

src/services/codefixes/fixUnusedIdentifier.ts

@@ -42,7 +42,7 @@ namespace ts.codefix {
                 }
             }
 
-            if (isObjectBindingPattern(token.parent)) {
+            if (isObjectBindingPattern(token.parent) || isArrayBindingPattern(token.parent)) {
                 if (isParameter(token.parent.parent)) {
                     const elements = token.parent.elements;
                     const diagnostic: [DiagnosticMessage, string] = [
@@ -51,7 +51,7 @@ namespace ts.codefix {
                     ];
                     return [
                         createDeleteFix(textChanges.ChangeTracker.with(context, t =>
-                            deleteDestructuringElements(t, sourceFile, <ObjectBindingPattern>token.parent)), diagnostic)
+                            deleteDestructuringElements(t, sourceFile, token.parent as ObjectBindingPattern | ArrayBindingPattern)), diagnostic)
                     ];
                 }
                 return [
@@ -121,13 +121,16 @@ namespace ts.codefix {
                             deleteTypeParameters(changes, sourceFile, token);
                         }
                         else if (isObjectBindingPattern(token.parent)) {
-                            if (isParameter(token.parent.parent)) {
-                                deleteDestructuringElements(changes, sourceFile, token.parent);
+                            if (token.parent.parent.initializer) {
+                                break;
                             }
-                            else {
+                            else if (!isParameter(token.parent.parent) || isNotProvidedArguments(token.parent.parent, checker, sourceFiles)) {
                                 changes.delete(sourceFile, token.parent.parent);
                             }
                         }
+                        else if (isArrayBindingPattern(token.parent.parent) && token.parent.parent.parent.initializer) {
+                            break;
+                        }
                         else if (canDeleteEntireVariableStatement(sourceFile, token)) {
                             deleteEntireVariableStatement(changes, sourceFile, <VariableDeclarationList>token.parent);
                         }
@@ -165,7 +168,7 @@ namespace ts.codefix {
             || token.kind === SyntaxKind.Identifier && (token.parent.kind === SyntaxKind.ImportSpecifier || token.parent.kind === SyntaxKind.ImportClause);
     }
 
-    // Sometimes the diagnostic span is an entire ImportDeclaration, so we should remove the whole thing.
+    /** Sometimes the diagnostic span is an entire ImportDeclaration, so we should remove the whole thing. */
     function tryGetFullImport(token: Node): ImportDeclaration | undefined {
         return token.kind === SyntaxKind.ImportKeyword ? tryCast(token.parent, isImportDeclaration) : undefined;
     }
@@ -178,7 +181,7 @@ namespace ts.codefix {
         changes.delete(sourceFile, node.parent.kind === SyntaxKind.VariableStatement ? node.parent : node);
     }
 
-    function deleteDestructuringElements(changes: textChanges.ChangeTracker, sourceFile: SourceFile, node: ObjectBindingPattern) {
+    function deleteDestructuringElements(changes: textChanges.ChangeTracker, sourceFile: SourceFile, node: ObjectBindingPattern | ArrayBindingPattern) {
         forEach(node.elements, n => changes.delete(sourceFile, n));
     }
 
@@ -219,16 +222,14 @@ namespace ts.codefix {
 
     function tryDeleteDeclaration(sourceFile: SourceFile, token: Node, changes: textChanges.ChangeTracker, checker: TypeChecker, sourceFiles: readonly SourceFile[], program: Program, cancellationToken: CancellationToken, isFixAll: boolean) {
         tryDeleteDeclarationWorker(token, changes, sourceFile, checker, sourceFiles, program, cancellationToken, isFixAll);
-        if (isIdentifier(token)) deleteAssignments(changes, sourceFile, token, checker);
-    }
-
-    function deleteAssignments(changes: textChanges.ChangeTracker, sourceFile: SourceFile, token: Identifier, checker: TypeChecker) {
-        FindAllReferences.Core.eachSymbolReferenceInFile(token, checker, sourceFile, (ref: Node) => {
-            if (isPropertyAccessExpression(ref.parent) && ref.parent.name === ref) ref = ref.parent;
-            if (isBinaryExpression(ref.parent) && isExpressionStatement(ref.parent.parent) && ref.parent.left === ref) {
-                changes.delete(sourceFile, ref.parent.parent);
-            }
-        });
+        if (isIdentifier(token)) {
+            FindAllReferences.Core.eachSymbolReferenceInFile(token, checker, sourceFile, (ref: Node) => {
+                if (isPropertyAccessExpression(ref.parent) && ref.parent.name === ref) ref = ref.parent;
+                if (!isFixAll && isBinaryExpression(ref.parent) && isExpressionStatement(ref.parent.parent) && ref.parent.left === ref) {
+                    changes.delete(sourceFile, ref.parent.parent);
+                }
+            });
+        }
     }
 
     function tryDeleteDeclarationWorker(token: Node, changes: textChanges.ChangeTracker, sourceFile: SourceFile, checker: TypeChecker, sourceFiles: readonly SourceFile[], program: Program, cancellationToken: CancellationToken, isFixAll: boolean): void {
@@ -236,24 +237,37 @@ namespace ts.codefix {
         if (isParameter(parent)) {
             tryDeleteParameter(changes, sourceFile, parent, checker, sourceFiles, program, cancellationToken, isFixAll);
         }
-        else {
+        else if (!isFixAll || !(isIdentifier(token) && FindAllReferences.Core.isSymbolReferencedInFile(token, checker, sourceFile))) {
             changes.delete(sourceFile, isImportClause(parent) ? token : isComputedPropertyName(parent) ? parent.parent : parent);
         }
     }
 
-    function tryDeleteParameter(changes: textChanges.ChangeTracker, sourceFile: SourceFile, p: ParameterDeclaration, checker: TypeChecker, sourceFiles: readonly SourceFile[], program: Program, cancellationToken: CancellationToken, isFixAll = false): void {
-        if (mayDeleteParameter(checker, sourceFile, p, sourceFiles, program, cancellationToken, isFixAll)) {
-            if (p.modifiers && p.modifiers.length > 0 &&
-                (!isIdentifier(p.name) || FindAllReferences.Core.isSymbolReferencedInFile(p.name, checker, sourceFile))) {
-                p.modifiers.forEach(modifier => changes.deleteModifier(sourceFile, modifier));
+    function tryDeleteParameter(
+        changes: textChanges.ChangeTracker,
+        sourceFile: SourceFile,
+        parameter: ParameterDeclaration,
+        checker: TypeChecker,
+        sourceFiles: readonly SourceFile[],
+        program: Program,
+        cancellationToken: CancellationToken,
+        isFixAll = false): void {
+        if (mayDeleteParameter(checker, sourceFile, parameter, sourceFiles, program, cancellationToken, isFixAll)) {
+            if (parameter.modifiers && parameter.modifiers.length > 0 &&
+                (!isIdentifier(parameter.name) || FindAllReferences.Core.isSymbolReferencedInFile(parameter.name, checker, sourceFile))) {
+                parameter.modifiers.forEach(modifier => changes.deleteModifier(sourceFile, modifier));
             }
-            else {
-                changes.delete(sourceFile, p);
-                deleteUnusedArguments(changes, sourceFile, p, sourceFiles, checker);
+            else if (!parameter.initializer && isNotProvidedArguments(parameter, checker, sourceFiles)) {
+                changes.delete(sourceFile, parameter);
             }
         }
     }
 
+    function isNotProvidedArguments(parameter: ParameterDeclaration, checker: TypeChecker, sourceFiles: readonly SourceFile[]) {
+        const index = parameter.parent.parameters.indexOf(parameter);
+        // Just in case the call didn't provide enough arguments.
+        return !FindAllReferences.Core.someSignatureUsage(parameter.parent, sourceFiles, checker, (_, call) => !call || call.arguments.length > index);
+    }
+
     function mayDeleteParameter(checker: TypeChecker, sourceFile: SourceFile, parameter: ParameterDeclaration, sourceFiles: readonly SourceFile[], program: Program, cancellationToken: CancellationToken, isFixAll: boolean): boolean {
         const { parent } = parameter;
         switch (parent.kind) {
@@ -305,15 +319,6 @@ namespace ts.codefix {
         }
     }
 
-    function deleteUnusedArguments(changes: textChanges.ChangeTracker, sourceFile: SourceFile, deletedParameter: ParameterDeclaration, sourceFiles: readonly SourceFile[], checker: TypeChecker): void {
-        FindAllReferences.Core.eachSignatureCall(deletedParameter.parent, sourceFiles, checker, call => {
-            const index = deletedParameter.parent.parameters.indexOf(deletedParameter);
-            if (call.arguments.length > index) { // Just in case the call didn't provide enough arguments.
-                changes.delete(sourceFile, call.arguments[index]);
-            }
-        });
-    }
-
     function isCallbackLike(checker: TypeChecker, sourceFile: SourceFile, name: Identifier): boolean {
         return !!FindAllReferences.Core.eachSymbolReferenceInFile(name, checker, sourceFile, reference =>
             isIdentifier(reference) && isCallExpression(reference.parent) && reference.parent.arguments.indexOf(reference) >= 0);

src/services/findAllReferences.ts

@@ -1240,8 +1240,13 @@ namespace ts.FindAllReferences {
             }
         }
 
-        export function eachSignatureCall(signature: SignatureDeclaration, sourceFiles: readonly SourceFile[], checker: TypeChecker, cb: (call: CallExpression) => void): void {
-            if (!signature.name || !isIdentifier(signature.name)) return;
+        export function someSignatureUsage(
+            signature: SignatureDeclaration,
+            sourceFiles: readonly SourceFile[],
+            checker: TypeChecker,
+            cb: (name: Identifier, call?: CallExpression) => boolean
+        ): boolean {
+            if (!signature.name || !isIdentifier(signature.name)) return false;
 
             const symbol = Debug.checkDefined(checker.getSymbolAtLocation(signature.name));
 
@@ -1249,14 +1254,16 @@ namespace ts.FindAllReferences {
                 for (const name of getPossibleSymbolReferenceNodes(sourceFile, symbol.name)) {
                     if (!isIdentifier(name) || name === signature.name || name.escapedText !== signature.name.escapedText) continue;
                     const called = climbPastPropertyAccess(name);
-                    const call = called.parent;
-                    if (!isCallExpression(call) || call.expression !== called) continue;
+                    const call = isCallExpression(called.parent) && called.parent.expression === called ? called.parent : undefined;
                     const referenceSymbol = checker.getSymbolAtLocation(name);
                     if (referenceSymbol && checker.getRootSymbols(referenceSymbol).some(s => s === symbol)) {
-                        cb(call);
+                        if (cb(name, call)) {
+                            return true;
+                        }
                     }
                 }
             }
+            return false;
         }
 
         function getPossibleSymbolReferenceNodes(sourceFile: SourceFile, symbolName: string, container: Node = sourceFile): readonly Node[] {

tests/baselines/reference/unusedDestructuringParameters.errors.txt

@@ -1,10 +1,10 @@
-tests/cases/compiler/unusedDestructuringParameters.ts(1,13): error TS6133: 'a' is declared but its value is never read.
+tests/cases/compiler/unusedDestructuringParameters.ts(1,12): 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) ====
     const f = ([a]) => { };
-                ~
+               ~~~
 !!! error TS6133: 'a' is declared but its value is never read.
     f([1]);
     const f2 = ({a}) => { };

tests/cases/fourslash/codeFixUnusedIdentifier_all_delete.ts

@@ -12,8 +12,9 @@
 ////function f(a, b) {
 ////    const x = 0;
 ////}
-////function g(a, b, c) { return a; }
-////f; g;
+////function g(a) { return a; }
+////function h(c) { return c; }
+////f(); g(); h();
 ////
 ////interface I {
 ////    m(x: number): void;
@@ -70,7 +71,8 @@ used1; used2;
 function f() {
 }
 function g(a) { return a; }
-f; g;
+function h(c) { return c; }
+f(); g(); h();
 
 interface I {
     m(x: number): void;
@@ -87,10 +89,10 @@ takesCb(() => {});
 takesCb((x) => { x; });
 takesCb((x, y) => { y; });
 
-function fn1(): void {}
+function fn1(x: number, y: string): void {}
 takesCb(fn1);
 
-function fn2(x: number): void { x; }
+function fn2(x: number, y: string): void { x; }
 takesCb(fn2);
 
 function fn3(x: number, y: string): void { y; }

tests/cases/fourslash/codeFixUnusedIdentifier_deleteWrite.ts

@@ -5,20 +5,10 @@
 
 ////let x = 0;
 ////x = 1;
-////
-////export class C {
-////    private p: number;
-////
-////    m() { this.p = 0; }
-////}
+////export {};
 
-verify.codeFixAll({
-    fixId: "unusedIdentifier_delete",
-    fixAllDescription: ts.Diagnostics.Delete_all_unused_declarations.message,
+verify.codeFix({
+    description: "Remove unused declaration for: 'x'",
     newFileContent:
-`
-export class C {
-
-    m() { }
-}`,
+`export {};`,
 });

tests/cases/fourslash/codeFixUnusedIdentifier_deleteWrite2.ts

@@ -0,0 +1,19 @@
+/// <reference path='fourslash.ts' />
+
+// @noLib: true
+// @noUnusedLocals: true
+
+////export class C {
+////    private p: number;
+////
+////    m() { this.p = 0; }
+////}
+
+verify.codeFix({
+    description: "Remove unused declaration for: 'p'",
+    newFileContent:
+`export class C {
+
+    m() { }
+}`,
+});

tests/cases/fourslash/codeFixUnusedIdentifier_destructure_allUnused_all.ts

@@ -3,20 +3,27 @@
 // @noUnusedLocals: true
 // @noUnusedParameters: true
 
-////const { x, y } = o;
-////const { a, b } = o;
-////a;
-////export function f({ a, b }, { x, y }) {
-////    a;
-////}
+//// const { x, y } = o;
+//// const { a, b } = o;
+//// a;
+//// export function f({ fa, fb }, { fx, fy }) {
+////     fb;
+//// }
+//// export function g([ ga, gb ], [ gary, gygax ]) {
+////     ga;
+//// }
 
 verify.codeFixAll({
     fixId: "unusedIdentifier_delete",
     fixAllDescription: ts.Diagnostics.Delete_all_unused_declarations.message,
     newFileContent:
-`const { a } = o;
+`const { x, y } = o;
+const { a } = o;
 a;
-export function f({ a }, {  }) {
-    a;
+export function f({ fb }) {
+    fb;
+}
+export function g([ ga ],) {
+    ga;
 }`,
 });

tests/cases/fourslash/codeFixUnusedIdentifier_destructure_partlyUnused.ts

@@ -57,19 +57,19 @@ verify.codeFixAll({
     x; z;
 }
 {
-    const [x] = o;
+    const [x, y] = o;
     x;
 }
 {
-    const [, y] = o;
+    const [x, y] = o;
     y;
 }
 {
-    const [, y] = o;
+    const [x, y, z] = o;
     y;
 }
 {
-    const [x,, z] = o;
+    const [x, y, z] = o;
     x; z;
 }`,
 });

tests/cases/fourslash/codeFixUnusedIdentifier_parameter.ts

@@ -0,0 +1,9 @@
+/// <reference path='fourslash.ts' />
+
+// @noUnusedLocals: true
+// @noUnusedParameters: true
+
+////function g(a, b) { b; }
+////g(1, 2);
+
+verify.not.codeFixAvailable("Remove unused declaration for: 'a'");

tests/cases/fourslash/codeFixUnusedIdentifier_parameter_all.ts

@@ -5,37 +5,75 @@
 
 ////function f(a, b, { x, y }) { b; }
 ////f(0, 1, { x: 1, y: 1 });
+////function g(a, b, { x, y }: { x: number, y: number }) { b; }
+////g();
+////function h(a, b?) { a; }
+////h(1);
+////function i(x = 1) { }
+////i();
+////
+////function container(o) {
+////    const { x, y } = { x: 1, y: 2 }
+////    const { a, b } = o
+////    const [ z, ka ] = [ 3, 4 ]
+////    const [ c, d ] = o
+////}
 ////
 ////class C {
 ////    m(a, b, c) { b; }
+////    n(a, b, c) { b; }
 ////}
 ////new C().m(0, 1, 2);
+////new C().n();
 ////
 ////// Test of deletedAncestors
 ////function a(a: any, unused: any) { a; }
 ////function b(a: any, unused: any) { a; }
+////function c(a: any, unused: any) { a; }
 ////
 ////b(1, {
 ////    prop: a(2, [
 ////        b(3, a(4, undefined)),
 ////    ]),
 ////});
+////b(1, { prop: c() });
 
 verify.codeFixAll({
     fixId: "unusedIdentifier_delete",
     fixAllDescription: ts.Diagnostics.Delete_all_unused_declarations.message,
     newFileContent:
-`function f(b, {  }) { b; }
-f(1, { x: 1, y: 1 });
+`function f(a, b, { x, y }) { b; }
+f(0, 1, { x: 1, y: 1 });
+function g(b) { b; }
+g();
+function h(a) { a; }
+h(1);
+function i(x = 1) { }
+i();
+
+function container(o) {
+    const { x, y } = { x: 1, y: 2 }
+    const { a, b } = o
+    const [ z, ka ] = [ 3, 4 ]
+    const [ c, d ] = o
+}
 
 class C {
-    m(b) { b; }
+    m(a, b, c) { b; }
+    n(b) { b; }
 }
-new C().m(1);
+new C().m(0, 1, 2);
+new C().n();
 
 // Test of deletedAncestors
-function a(a: any) { a; }
-function b(a: any) { a; }
+function a(a: any, unused: any) { a; }
+function b(a: any, unused: any) { a; }
+function c(a: any) { a; }
 
-b(1);`,
+b(1, {
+    prop: a(2, [
+        b(3, a(4, undefined)),
+    ]),
+});
+b(1, { prop: c() });`,
 });