fixUnusedIdentifier: Don't remove parameter in override or non-last parameter in callback

Author: Andy Hanson

src/services/codefixes/fixUnusedIdentifier.ts

@@ -13,21 +13,22 @@ namespace ts.codefix {
     registerCodeFix({
         errorCodes,
         getCodeActions(context) {
-            const { errorCode, sourceFile } = context;
+            const { errorCode, sourceFile, program } = context;
+            const checker = program.getTypeChecker();
             const importDecl = tryGetFullImport(sourceFile, context.span.start);
             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, context.span.start, /*deleted*/ undefined, checker));
             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[] = [];
 
-            const deletion = textChanges.ChangeTracker.with(context, t => tryDeleteDeclaration(t, sourceFile, token, /*deleted*/ undefined));
+            const deletion = textChanges.ChangeTracker.with(context, t => tryDeleteDeclaration(t, sourceFile, token, /*deleted*/ undefined, checker));
             if (deletion.length) {
                 result.push(createCodeFixAction(fixName, deletion, [Diagnostics.Remove_declaration_for_Colon_0, token.getText(sourceFile)], fixIdDelete, Diagnostics.Delete_all_unused_declarations));
             }
@@ -43,8 +44,9 @@ namespace ts.codefix {
         getAllCodeActions: context => {
             // Track a set of deleted nodes that may be ancestors of other marked for deletion -- only delete the ancestors.
             const deleted = new NodeSet();
+            const { sourceFile, program } = context;
+            const checker = program.getTypeChecker();
             return codeFixAll(context, errorCodes, (changes, diag) => {
-                const { sourceFile } = context;
                 const token = findPrecedingToken(textSpanEnd(diag), diag.file!);
                 switch (context.fixId) {
                     case fixIdPrefix:
@@ -61,8 +63,8 @@ namespace ts.codefix {
                             changes.deleteNode(sourceFile, importDecl);
                         }
                         else {
-                            if (!tryDeleteFullDestructure(changes, sourceFile, diag.start!, deleted)) {
-                                tryDeleteDeclaration(changes, sourceFile, token, deleted);
+                            if (!tryDeleteFullDestructure(changes, sourceFile, diag.start!, deleted, checker)) {
+                                tryDeleteDeclaration(changes, sourceFile, token, deleted, checker);
                             }
                         }
                         break;
@@ -79,7 +81,7 @@ namespace ts.codefix {
         return startToken.kind === SyntaxKind.ImportKeyword ? tryCast(startToken.parent, isImportDeclaration) : undefined;
     }
 
-    function tryDeleteFullDestructure(changes: textChanges.ChangeTracker, sourceFile: SourceFile, pos: number, deletedAncestors: NodeSet | undefined): boolean {
+    function tryDeleteFullDestructure(changes: textChanges.ChangeTracker, sourceFile: SourceFile, pos: number, deletedAncestors: NodeSet | undefined, checker: TypeChecker): boolean {
         const startToken = getTokenAtPosition(sourceFile, pos, /*includeJsDocComment*/ false);
         if (startToken.kind !== SyntaxKind.OpenBraceToken || !isObjectBindingPattern(startToken.parent)) return false;
         const decl = startToken.parent.parent;
@@ -88,6 +90,7 @@ namespace ts.codefix {
                 tryDeleteVariableDeclaration(changes, sourceFile, decl, deletedAncestors);
                 break;
             case SyntaxKind.Parameter:
+                if (!mayDeleteParameter(decl, checker)) break;
                 if (deletedAncestors) deletedAncestors.add(decl);
                 changes.deleteNodeInList(sourceFile, decl);
                 break;
@@ -130,10 +133,10 @@ namespace ts.codefix {
         return false;
     }
 
-    function tryDeleteDeclaration(changes: textChanges.ChangeTracker, sourceFile: SourceFile, token: Node, deletedAncestors: NodeSet | undefined): void {
+    function tryDeleteDeclaration(changes: textChanges.ChangeTracker, sourceFile: SourceFile, token: Node, deletedAncestors: NodeSet | undefined, checker: TypeChecker): void {
         switch (token.kind) {
             case SyntaxKind.Identifier:
-                tryDeleteIdentifier(changes, sourceFile, <Identifier>token, deletedAncestors);
+                tryDeleteIdentifier(changes, sourceFile, <Identifier>token, deletedAncestors, checker);
                 break;
             case SyntaxKind.PropertyDeclaration:
             case SyntaxKind.NamespaceImport:
@@ -156,7 +159,7 @@ namespace ts.codefix {
         }
     }
 
-    function tryDeleteIdentifier(changes: textChanges.ChangeTracker, sourceFile: SourceFile, identifier: Identifier, deletedAncestors: NodeSet | undefined): void {
+    function tryDeleteIdentifier(changes: textChanges.ChangeTracker, sourceFile: SourceFile, identifier: Identifier, deletedAncestors: NodeSet | undefined, checker: TypeChecker): void {
         const parent = identifier.parent;
         switch (parent.kind) {
             case SyntaxKind.VariableDeclaration:
@@ -180,11 +183,8 @@ namespace ts.codefix {
                 break;
 
             case SyntaxKind.Parameter:
+                if (!mayDeleteParameter(parent as ParameterDeclaration, checker)) break;
                 const oldFunction = parent.parent;
-                if (isSetAccessor(oldFunction)) {
-                    // Setter must have a parameter
-                    break;
-                }
 
                 if (isArrowFunction(oldFunction) && oldFunction.parameters.length === 1) {
                     // Lambdas with exactly one parameter are special because, after removal, there
@@ -334,6 +334,33 @@ namespace ts.codefix {
         }
     }
 
+    function mayDeleteParameter(p: ParameterDeclaration, checker: TypeChecker) {
+        const parent = p.parent;
+        switch (parent.kind) {
+            case SyntaxKind.FunctionDeclaration:
+                return true;
+
+            case SyntaxKind.FunctionExpression:
+            case SyntaxKind.ArrowFunction:
+                // Can't remove a non-last parameter in a callback. (Can if future parameters are also unused.)
+                const index = parent.parameters.indexOf(p);
+                Debug.assert(index !== -1);
+                return parent.parameters.slice(index).every(p => p.name.kind === SyntaxKind.Identifier && !p.symbol.isReferenced) || !checker.getContextualType(parent);
+
+            case SyntaxKind.SetAccessor:
+                // Setter must have a parameter
+                return false;
+
+            case SyntaxKind.MethodDeclaration:
+                // Don't remove a parameter if this overrides something
+                const symbol = checker.getSymbolAtLocation(parent.name);
+                return !isMemberSymbolInBaseType(symbol, checker);
+
+            default:
+                return Debug.failBadSyntaxKind(parent);
+        }
+    }
+
     class NodeSet {
         private map = createMap<Node>();
 

src/services/findAllReferences.ts

@@ -1398,34 +1398,6 @@ namespace ts.FindAllReferences.Core {
         }
     }
 
-    /**
-     * Find symbol of the given property-name and add the symbol to the given result array
-     * @param symbol a symbol to start searching for the given propertyName
-     * @param propertyName a name of property to search for
-     * @param result an array of symbol of found property symbols
-     * @param previousIterationSymbolsCache a cache of symbol from previous iterations of calling this function to prevent infinite revisiting of the same symbol.
-     *                                The value of previousIterationSymbol is undefined when the function is first called.
-     */
-    function getPropertySymbolsFromBaseTypes<T>(symbol: Symbol, propertyName: string, checker: TypeChecker, cb: (symbol: Symbol) => T | undefined): T | undefined {
-        const seen = createMap<true>();
-        return recur(symbol);
-
-        function recur(symbol: Symbol): T | undefined {
-            // Use `addToSeen` to ensure we don't infinitely recurse in this situation:
-            //      interface C extends C {
-            //          /*findRef*/propName: string;
-            //      }
-            if (!(symbol.flags & (SymbolFlags.Class | SymbolFlags.Interface)) || !addToSeen(seen, getSymbolId(symbol))) return;
-
-            return firstDefined(symbol.declarations, declaration => firstDefined(getAllSuperTypeNodes(declaration), typeReference => {
-                const type = checker.getTypeAtLocation(typeReference);
-                const propertySymbol = type && type.symbol && checker.getPropertyOfType(type, propertyName);
-                // Visit the typeReference as well to see if it directly or indirectly uses that property
-                return propertySymbol && (firstDefined(checker.getRootSymbols(propertySymbol), cb) || recur(type.symbol));
-            }));
-        }
-    }
-
     function getRelatedSymbol(search: Search, referenceSymbol: Symbol, referenceLocation: Node, state: State): Symbol | undefined {
         const { checker } = state;
         return forEachRelatedSymbol(referenceSymbol, referenceLocation, checker,

src/services/utilities.ts

@@ -1281,6 +1281,38 @@ namespace ts {
         return propSymbol;
     }
 
+    /**
+     * Find symbol of the given property-name and add the symbol to the given result array
+     * @param symbol a symbol to start searching for the given propertyName
+     * @param propertyName a name of property to search for
+     * @param result an array of symbol of found property symbols
+     * @param previousIterationSymbolsCache a cache of symbol from previous iterations of calling this function to prevent infinite revisiting of the same symbol.
+     *                                The value of previousIterationSymbol is undefined when the function is first called.
+     */
+    export function getPropertySymbolsFromBaseTypes<T>(symbol: Symbol, propertyName: string, checker: TypeChecker, cb: (symbol: Symbol) => T | undefined): T | undefined {
+        const seen = createMap<true>();
+        return recur(symbol);
+
+        function recur(symbol: Symbol): T | undefined {
+            // Use `addToSeen` to ensure we don't infinitely recurse in this situation:
+            //      interface C extends C {
+            //          /*findRef*/propName: string;
+            //      }
+            if (!(symbol.flags & (SymbolFlags.Class | SymbolFlags.Interface)) || !addToSeen(seen, getSymbolId(symbol))) return;
+
+            return firstDefined(symbol.declarations, declaration => firstDefined(getAllSuperTypeNodes(declaration), typeReference => {
+                const type = checker.getTypeAtLocation(typeReference);
+                const propertySymbol = type && type.symbol && checker.getPropertyOfType(type, propertyName);
+                // Visit the typeReference as well to see if it directly or indirectly uses that property
+                return propertySymbol && (firstDefined(checker.getRootSymbols(propertySymbol), cb) || recur(type.symbol));
+            }));
+        }
+    }
+
+    export function isMemberSymbolInBaseType(memberSymbol: Symbol, checker: TypeChecker): boolean {
+        return getPropertySymbolsFromBaseTypes(memberSymbol.parent, memberSymbol.name, checker, _ => true) || false;
+    }
+
     export class NodeSet {
         private map = createMap<Node>();
 

tests/cases/fourslash/codeFixUnusedIdentifier_all_delete.ts

@@ -7,12 +7,40 @@
 ////    const x = 0;
 ////}
 ////function g(a, b, c) { return a; }
+////
+////interface I {
+////    m(x: number): void;
+////}
+////
+////class C implements I {
+////    m(x: number): void {} // Does not remove 'x', which is inherited
+////    n(x: number): void {}
+////}
+////
+////declare function f(cb: (x: number, y: string) => void): void;
+////f((x, y) => {});
+////f((x, y) => { x; });
+////f((x, y) => { y; });
 
 verify.codeFixAll({
     fixId: "unusedIdentifier_delete",
     fixAllDescription: "Delete all unused declarations",
     newFileContent:
 `function f() {
 }
-function g(a) { return a; }`,
+function g(a) { return a; }
+
+interface I {
+    m(x: number): void;
+}
+
+class C implements I {
+    m(x: number): void {} // Does not remove 'x', which is inherited
+    n(): void {}
+}
+
+declare function f(cb: (x: number, y: string) => void): void;
+f(() => {});
+f((x) => { x; });
+f((x, y) => { y; });`,
 });

tests/cases/fourslash/codeFixUnusedIdentifier_parameterInCallback.ts

@@ -0,0 +1,9 @@
+/// <reference path='fourslash.ts' />
+
+// @noUnusedParameters: true
+
+////declare function f(cb: (x: number, y: string) => void): void;
+////f((x, y) => { y; });
+
+// No codefix to remove a non-last parameter
+verify.codeFixAvailable([{ description: "Prefix 'x' with an underscore" }]);

tests/cases/fourslash/codeFixUnusedIdentifier_parameterInOverride.ts

@@ -0,0 +1,14 @@
+/// <reference path='fourslash.ts' />
+
+// @noUnusedParameters: true
+
+////interface I {
+////    m(x: number): void;
+////}
+////
+////class C implements I {
+////    m(x: number): void {}
+////}
+
+// No codefix to remove the parameter, it's inherited
+verify.codeFixAvailable([{ description: "Prefix 'x' with an underscore" }]);