diff --git a/src/services/codefixes/fixUnusedIdentifier.ts b/src/services/codefixes/fixUnusedIdentifier.ts index e2ef6e9ec3c..abff1a5a02f 100644 --- a/src/services/codefixes/fixUnusedIdentifier.ts +++ b/src/services/codefixes/fixUnusedIdentifier.ts @@ -14,7 +14,8 @@ 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); @@ -22,7 +23,7 @@ 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, 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,7 +87,7 @@ 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) { @@ -92,6 +95,7 @@ namespace ts.codefix { 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, token, deletedAncestors); + tryDeleteIdentifier(changes, sourceFile, 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); + } + } } diff --git a/src/services/findAllReferences.ts b/src/services/findAllReferences.ts index cc13cfd28c4..f00046bd4ea 100644 --- a/src/services/findAllReferences.ts +++ b/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(symbol: Symbol, propertyName: string, checker: TypeChecker, cb: (symbol: Symbol) => T | undefined): T | undefined { - const seen = createMap(); - 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, diff --git a/src/services/utilities.ts b/src/services/utilities.ts index de577ec5946..85afcf9d7bf 100644 --- a/src/services/utilities.ts +++ b/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(symbol: Symbol, propertyName: string, checker: TypeChecker, cb: (symbol: Symbol) => T | undefined): T | undefined { + const seen = createMap(); + 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(); diff --git a/tests/cases/fourslash/codeFixUnusedIdentifier_all_delete.ts b/tests/cases/fourslash/codeFixUnusedIdentifier_all_delete.ts index 56558c3765d..c4c64c1da43 100644 --- a/tests/cases/fourslash/codeFixUnusedIdentifier_all_delete.ts +++ b/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 (; ;) {}`, diff --git a/tests/cases/fourslash/codeFixUnusedIdentifier_destructure_allUnused_all.ts b/tests/cases/fourslash/codeFixUnusedIdentifier_destructure_allUnused_all.ts index af27091788d..c9d4f62f2db 100644 --- a/tests/cases/fourslash/codeFixUnusedIdentifier_destructure_allUnused_all.ts +++ b/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; ////} diff --git a/tests/cases/fourslash/codeFixUnusedIdentifier_parameterInCallback.ts b/tests/cases/fourslash/codeFixUnusedIdentifier_parameterInCallback.ts new file mode 100644 index 00000000000..5fe369da48f --- /dev/null +++ b/tests/cases/fourslash/codeFixUnusedIdentifier_parameterInCallback.ts @@ -0,0 +1,9 @@ +/// + +// @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" }]); diff --git a/tests/cases/fourslash/codeFixUnusedIdentifier_parameterInOverride.ts b/tests/cases/fourslash/codeFixUnusedIdentifier_parameterInOverride.ts new file mode 100644 index 00000000000..35aff860958 --- /dev/null +++ b/tests/cases/fourslash/codeFixUnusedIdentifier_parameterInOverride.ts @@ -0,0 +1,14 @@ +/// + +// @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" }]); diff --git a/tests/cases/fourslash/unusedParameterInConstructor1.ts b/tests/cases/fourslash/unusedParameterInConstructor1.ts deleted file mode 100644 index 92e0e389708..00000000000 --- a/tests/cases/fourslash/unusedParameterInConstructor1.ts +++ /dev/null @@ -1,12 +0,0 @@ -/// - -// @noUnusedLocals: true -//// class C1 { -//// [|constructor(private p1: string, public p2: boolean, public p3: any, p5)|] { p5; } -//// } - -verify.codeFix({ - description: "Remove declaration for: 'p1'", - index: 0, - newRangeContent: "constructor(public p2: boolean, public p3: any, p5)", -}); diff --git a/tests/cases/fourslash/unusedParameterInConstructor1AddUnderscore.ts b/tests/cases/fourslash/unusedParameterInConstructor1AddUnderscore.ts index 28a84d6498b..c10a2f0d6a1 100644 --- a/tests/cases/fourslash/unusedParameterInConstructor1AddUnderscore.ts +++ b/tests/cases/fourslash/unusedParameterInConstructor1AddUnderscore.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; } diff --git a/tests/cases/fourslash/unusedParameterInConstructor2.ts b/tests/cases/fourslash/unusedParameterInConstructor2.ts deleted file mode 100644 index 9771d202a35..00000000000 --- a/tests/cases/fourslash/unusedParameterInConstructor2.ts +++ /dev/null @@ -1,12 +0,0 @@ -/// - -// @noUnusedLocals: true -//// class C1 { -//// [|constructor(public p1: string, private p2: boolean, public p3: any, p5)|] { p5; } -//// } - -verify.codeFix({ - description: "Remove declaration for: 'p2'", - index: 0, - newRangeContent: "constructor(public p1: string, public p3: any, p5)", -}); diff --git a/tests/cases/fourslash/unusedParameterInConstructor3.ts b/tests/cases/fourslash/unusedParameterInConstructor3.ts deleted file mode 100644 index b45f4d2b85f..00000000000 --- a/tests/cases/fourslash/unusedParameterInConstructor3.ts +++ /dev/null @@ -1,12 +0,0 @@ -/// - -// @noUnusedLocals: true -//// class C1 { -//// [|constructor(public p1: string, public p2: boolean, private p3: any, p5)|] { p5; } -//// } - -verify.codeFix({ - description: "Remove declaration for: 'p3'", - index: 0, - newRangeContent: "constructor(public p1: string, public p2: boolean, p5)", -}); diff --git a/tests/cases/fourslash/unusedParameterInConstructor4.ts b/tests/cases/fourslash/unusedParameterInConstructor4.ts deleted file mode 100644 index d63ceaa3bcc..00000000000 --- a/tests/cases/fourslash/unusedParameterInConstructor4.ts +++ /dev/null @@ -1,12 +0,0 @@ -/// - -// @noUnusedLocals: true -//// class C1 { -//// [|constructor(private readonly p2: boolean, p5)|] { p5; } -//// } - -verify.codeFix({ - description: "Remove declaration for: 'p2'", - index: 0, - newRangeContent: "constructor(p5)", -}); diff --git a/tests/cases/fourslash/unusedParameterInFunction3.ts b/tests/cases/fourslash/unusedParameterInFunction3.ts deleted file mode 100644 index 5bf2fc94519..00000000000 --- a/tests/cases/fourslash/unusedParameterInFunction3.ts +++ /dev/null @@ -1,12 +0,0 @@ -/// - -// @noUnusedParameters: true -////function [|greeter(x,y)|] { -//// y++; -////} - -verify.codeFix({ - description: "Remove declaration for: 'x'", - index: 0, - newRangeContent: "greeter(y)", -}); diff --git a/tests/cases/fourslash/unusedParameterInFunction4.ts b/tests/cases/fourslash/unusedParameterInFunction4.ts deleted file mode 100644 index 59ed0119402..00000000000 --- a/tests/cases/fourslash/unusedParameterInFunction4.ts +++ /dev/null @@ -1,12 +0,0 @@ -/// - -// @noUnusedParameters: true -////[|function greeter(x,y,z) |] { -//// use(x, z); -////} - -verify.codeFix({ - description: "Remove declaration for: 'y'", - index: 0, - newRangeContent: "function greeter(x,z) ", -}); diff --git a/tests/cases/fourslash/unusedParameterInLambda4.ts b/tests/cases/fourslash/unusedParameterInLambda4.ts deleted file mode 100644 index 015f081891a..00000000000 --- a/tests/cases/fourslash/unusedParameterInLambda4.ts +++ /dev/null @@ -1,11 +0,0 @@ -/// - -// @noUnusedLocals: true -// @noUnusedParameters: true -////[|/*~a*/(/*~b*/x/*~c*/,/*~d*/y/*~e*/)/*~f*/ => /*~g*/y/*~h*/|] - -verify.codeFix({ - description: "Remove declaration for: 'x'", - index: 0, - newRangeContent: "/*~a*/(/*~d*/y/*~e*/)/*~f*/ => /*~g*/y/*~h*/", -});