fixUnusedIdentifier: Don't remove parameter in override or non-last parameter in callback (microsoft#24306)

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

* Only allow removing last parameters; don't care about contextual type

Author: Andy

src/services/codefixes/fixUnusedIdentifier.ts

@@ -14,15 +14,16 @@ namespace ts.codefix {
     registerCodeFix({
         errorCodes,
         getCodeActions(context) {
-            const { errorCode, sourceFile } = context;
+            const { errorCode, sourceFile, program } = context;
+            const checker = program.getTypeChecker();
             const startToken = getTokenAtPosition(sourceFile, context.span.start, /*includeJsDocComment*/ false);
 
             const importDecl = tryGetFullImport(startToken);
             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, startToken, /*deleted*/ undefined));
+            const delDestructure = textChanges.ChangeTracker.with(context, t => tryDeleteFullDestructure(t, sourceFile, startToken, /*deleted*/ undefined, checker, /*isFixAll*/ false));
             if (delDestructure.length) {
                 return [createCodeFixAction(fixName, delDestructure, Diagnostics.Remove_destructuring, fixIdDelete, Diagnostics.Delete_all_unused_declarations)];
             }
@@ -34,7 +35,7 @@ namespace ts.codefix {
             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, /*isFixAll*/ false));
             if (deletion.length) {
                 result.push(createCodeFixAction(fixName, deletion, [Diagnostics.Remove_declaration_for_Colon_0, token.getText(sourceFile)], fixIdDelete, Diagnostics.Delete_all_unused_declarations));
             }
@@ -50,8 +51,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 startToken = getTokenAtPosition(sourceFile, diag.start, /*includeJsDocComment*/ false);
                 const token = findPrecedingToken(textSpanEnd(diag), diag.file)!;
                 switch (context.fixId) {
@@ -68,8 +70,9 @@ namespace ts.codefix {
                         if (importDecl) {
                             changes.deleteNode(sourceFile, importDecl);
                         }
-                        else if (!tryDeleteFullDestructure(changes, sourceFile, startToken, deleted) && !tryDeleteFullVariableStatement(changes, sourceFile, startToken, deleted)) {
-                            tryDeleteDeclaration(changes, sourceFile, token, deleted);
+                        else if (!tryDeleteFullDestructure(changes, sourceFile, startToken, deleted, checker, /*isFixAll*/ true) &&
+                            !tryDeleteFullVariableStatement(changes, sourceFile, startToken, deleted)) {
+                            tryDeleteDeclaration(changes, sourceFile, token, deleted, checker, /*isFixAll*/ true);
                         }
                         break;
                     default:
@@ -84,14 +87,15 @@ namespace ts.codefix {
         return startToken.kind === SyntaxKind.ImportKeyword ? tryCast(startToken.parent, isImportDeclaration) : undefined;
     }
 
-    function tryDeleteFullDestructure(changes: textChanges.ChangeTracker, sourceFile: SourceFile, startToken: Node, deletedAncestors: NodeSet | undefined): boolean {
+    function tryDeleteFullDestructure(changes: textChanges.ChangeTracker, sourceFile: SourceFile, startToken: Node, deletedAncestors: NodeSet | undefined, checker: TypeChecker, isFixAll: boolean): boolean {
         if (startToken.kind !== SyntaxKind.OpenBraceToken || !isObjectBindingPattern(startToken.parent)) return false;
         const decl = cast(startToken.parent, isObjectBindingPattern).parent;
         switch (decl.kind) {
             case SyntaxKind.VariableDeclaration:
                 tryDeleteVariableDeclaration(changes, sourceFile, decl, deletedAncestors);
                 break;
             case SyntaxKind.Parameter:
+                if (!mayDeleteParameter(decl, checker, isFixAll)) break;
                 if (deletedAncestors) deletedAncestors.add(decl);
                 changes.deleteNodeInList(sourceFile, decl);
                 break;
@@ -144,10 +148,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, isFixAll: boolean): void {
         switch (token.kind) {
             case SyntaxKind.Identifier:
-                tryDeleteIdentifier(changes, sourceFile, <Identifier>token, deletedAncestors);
+                tryDeleteIdentifier(changes, sourceFile, <Identifier>token, deletedAncestors, checker, isFixAll);
                 break;
             case SyntaxKind.PropertyDeclaration:
             case SyntaxKind.NamespaceImport:
@@ -170,7 +174,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, isFixAll: boolean): void {
         const parent = identifier.parent;
         switch (parent.kind) {
             case SyntaxKind.VariableDeclaration:
@@ -194,11 +198,8 @@ namespace ts.codefix {
                 break;
 
             case SyntaxKind.Parameter:
+                if (!mayDeleteParameter(parent as ParameterDeclaration, checker, isFixAll)) 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
@@ -347,4 +348,35 @@ namespace ts.codefix {
                 }
         }
     }
+
+    function mayDeleteParameter(p: ParameterDeclaration, checker: TypeChecker, isFixAll: boolean) {
+        const parent = p.parent;
+        switch (parent.kind) {
+            case SyntaxKind.MethodDeclaration:
+                // Don't remove a parameter if this overrides something
+                const symbol = checker.getSymbolAtLocation(parent.name)!;
+                if (isMemberSymbolInBaseType(symbol, checker)) return false;
+                // falls through
+
+            case SyntaxKind.Constructor:
+            case SyntaxKind.FunctionDeclaration:
+            case SyntaxKind.FunctionExpression:
+            case SyntaxKind.ArrowFunction: {
+                // Can't remove a non-last parameter. Can remove a parameter in code-fix-all if future parameters are also unused.
+                const { parameters } = parent;
+                const index = parameters.indexOf(p);
+                Debug.assert(index !== -1);
+                return isFixAll
+                    ? parameters.slice(index + 1).every(p => p.name.kind === SyntaxKind.Identifier && !p.symbol.isReferenced)
+                    : index === parameters.length - 1;
+            }
+
+            case SyntaxKind.SetAccessor:
+                // Setter must have a parameter
+                return false;
+
+            default:
+                return Debug.failBadSyntaxKind(parent);
+        }
+    }
 }

src/services/findAllReferences.ts

@@ -1399,34 +1399,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

@@ -1293,6 +1293,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 type && 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,6 +7,21 @@
 ////    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; });
+////
 ////{
 ////    let a, b;
 ////}
@@ -19,6 +34,21 @@ verify.codeFixAll({
 `function f() {
 }
 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; });
+
 {
 }
 for (; ;) {}`,

tests/cases/fourslash/codeFixUnusedIdentifier_destructure_allUnused_all.ts

@@ -6,7 +6,7 @@
 ////const { x, y } = o;
 ////const { a, b } = o;
 ////a;
-////export function f({ x, y }, { a, b }) {
+////export function f({ a, b }, { x, y }) {
 ////    a;
 ////}
 

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

tests/cases/fourslash/unusedParameterInConstructor1.ts

@@ -7,7 +7,6 @@
 
 verify.codeFix({
     description: "Prefix 'p1' with an underscore",
-    index: 1,
     newFileContent:
 `class C1 {
     constructor(_p1: string, public p2: boolean, public p3: any, p5) { p5; }