From 4584d6d47095bf2a53f6c1724da48a2ebd09b160 Mon Sep 17 00:00:00 2001 From: Alex T Date: Wed, 9 Sep 2020 18:23:53 +0300 Subject: [PATCH] fix(39676): skip removing unused parameters for functions used as callback references (#40299) --- src/services/codefixes/fixUnusedIdentifier.ts | 49 +++++++++++-------- .../codeFixUnusedIdentifier_all_delete.ts | 18 +++++++ ...ixUnusedIdentifier_parameter_callback1.ts} | 0 ...FixUnusedIdentifier_parameter_callback2.ts | 12 +++++ 4 files changed, 59 insertions(+), 20 deletions(-) rename tests/cases/fourslash/{codeFixUnusedIdentifier_parameter_callback.ts => codeFixUnusedIdentifier_parameter_callback1.ts} (100%) create mode 100644 tests/cases/fourslash/codeFixUnusedIdentifier_parameter_callback2.ts diff --git a/src/services/codefixes/fixUnusedIdentifier.ts b/src/services/codefixes/fixUnusedIdentifier.ts index 9467b6a9b0a..72ab6a957b5 100644 --- a/src/services/codefixes/fixUnusedIdentifier.ts +++ b/src/services/codefixes/fixUnusedIdentifier.ts @@ -224,12 +224,10 @@ namespace ts.codefix { } function tryDeleteParameter(changes: textChanges.ChangeTracker, sourceFile: SourceFile, p: ParameterDeclaration, checker: TypeChecker, sourceFiles: readonly SourceFile[], isFixAll = false): void { - if (mayDeleteParameter(p, checker, 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); - }); + if (mayDeleteParameter(checker, sourceFile, p, 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)); } else { changes.delete(sourceFile, p); @@ -238,29 +236,26 @@ namespace ts.codefix { } } - function mayDeleteParameter(p: ParameterDeclaration, checker: TypeChecker, isFixAll: boolean): boolean { - const { parent } = p; + function mayDeleteParameter(checker: TypeChecker, sourceFile: SourceFile, parameter: ParameterDeclaration, isFixAll: boolean): boolean { + const { parent } = parameter; 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: return true; - - case SyntaxKind.FunctionExpression: - case SyntaxKind.ArrowFunction: { - // Can't remove a non-last parameter in a callback. 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, "The parameter should already be in the list"); - return isFixAll - ? parameters.slice(index + 1).every(p => p.name.kind === SyntaxKind.Identifier && !p.symbol.isReferenced) - : index === parameters.length - 1; + case SyntaxKind.FunctionDeclaration: { + if (parent.name && isCallbackLike(checker, sourceFile, parent.name)) { + return isLastParameter(parent, parameter, isFixAll); + } + return true; } + case SyntaxKind.FunctionExpression: + case SyntaxKind.ArrowFunction: + // Can't remove a non-last parameter in a callback. Can remove a parameter in code-fix-all if future parameters are also unused. + return isLastParameter(parent, parameter, isFixAll); case SyntaxKind.SetAccessor: // Setter must have a parameter @@ -279,4 +274,18 @@ namespace ts.codefix { } }); } + + 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); + } + + function isLastParameter(func: FunctionLikeDeclaration, parameter: ParameterDeclaration, isFixAll: boolean): boolean { + const parameters = func.parameters; + const index = parameters.indexOf(parameter); + Debug.assert(index !== -1, "The parameter should already be in the list"); + return isFixAll ? + parameters.slice(index + 1).every(p => isIdentifier(p.name) && !p.symbol.isReferenced) : + index === parameters.length - 1; + } } diff --git a/tests/cases/fourslash/codeFixUnusedIdentifier_all_delete.ts b/tests/cases/fourslash/codeFixUnusedIdentifier_all_delete.ts index 92585714eec..ecdf9a98609 100644 --- a/tests/cases/fourslash/codeFixUnusedIdentifier_all_delete.ts +++ b/tests/cases/fourslash/codeFixUnusedIdentifier_all_delete.ts @@ -31,6 +31,15 @@ ////takesCb((x, y) => { x; }); ////takesCb((x, y) => { y; }); //// +////function fn1(x: number, y: string): void {} +////takesCb(fn1); +//// +////function fn2(x: number, y: string): void { x; } +////takesCb(fn2); +//// +////function fn3(x: number, y: string): void { y; } +////takesCb(fn3); +//// ////x => { //// const y = 0; ////}; @@ -76,6 +85,15 @@ takesCb(() => {}); takesCb((x) => { x; }); takesCb((x, y) => { y; }); +function fn1(): void {} +takesCb(fn1); + +function fn2(x: number): void { x; } +takesCb(fn2); + +function fn3(x: number, y: string): void { y; } +takesCb(fn3); + () => { }; diff --git a/tests/cases/fourslash/codeFixUnusedIdentifier_parameter_callback.ts b/tests/cases/fourslash/codeFixUnusedIdentifier_parameter_callback1.ts similarity index 100% rename from tests/cases/fourslash/codeFixUnusedIdentifier_parameter_callback.ts rename to tests/cases/fourslash/codeFixUnusedIdentifier_parameter_callback1.ts diff --git a/tests/cases/fourslash/codeFixUnusedIdentifier_parameter_callback2.ts b/tests/cases/fourslash/codeFixUnusedIdentifier_parameter_callback2.ts new file mode 100644 index 00000000000..b6cba7e3210 --- /dev/null +++ b/tests/cases/fourslash/codeFixUnusedIdentifier_parameter_callback2.ts @@ -0,0 +1,12 @@ +/// + +// @noUnusedParameters: true + +////declare function foo(cb: (x: number, y: string) => void): void; +////function fn(x: number, y: number): any { +//// return y; +////} +////foo(fn); + +// No codefix to remove a non-last parameter in a callback +verify.codeFixAvailable([{ description: "Prefix 'x' with an underscore" }]);