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

* fixUnusedIdentifier: Handle destructure with all bindings unused

* Add parameters test

* Add test for 'for' loop

Author: Andy

src/compiler/checker.ts

@@ -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);
-                    }
-                    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)));
+                }
+            });
+            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 (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;

src/compiler/diagnosticMessages.json

@@ -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

src/services/codefixes/fixUnusedIdentifier.ts

@@ -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;

tests/baselines/reference/unusedDestructuring.errors.txt

@@ -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;
+    }
+    

tests/baselines/reference/unusedDestructuring.js

@@ -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;
+}

tests/baselines/reference/unusedDestructuring.symbols

@@ -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))
+}
+