diff --git a/src/services/codefixes/fixUnusedIdentifier.ts b/src/services/codefixes/fixUnusedIdentifier.ts index 4bb8282e6e..a41a08420b 100644 --- a/src/services/codefixes/fixUnusedIdentifier.ts +++ b/src/services/codefixes/fixUnusedIdentifier.ts @@ -18,7 +18,7 @@ namespace ts.codefix { registerCodeFix({ errorCodes, getCodeActions(context) { - const { errorCode, sourceFile, program } = context; + const { errorCode, sourceFile, program, cancellationToken } = context; const checker = program.getTypeChecker(); const sourceFiles = program.getSourceFiles(); const token = getTokenAtPosition(sourceFile, context.span.start); @@ -36,7 +36,7 @@ namespace ts.codefix { return [createCodeFixAction(fixName, changes, [Diagnostics.Remove_import_from_0, showModuleSpecifier(importDecl)], fixIdDeleteImports, Diagnostics.Delete_all_unused_imports)]; } else if (isImport(token)) { - const deletion = textChanges.ChangeTracker.with(context, t => tryDeleteDeclaration(sourceFile, token, t, checker, sourceFiles, /*isFixAll*/ false)); + const deletion = textChanges.ChangeTracker.with(context, t => tryDeleteDeclaration(sourceFile, token, t, checker, sourceFiles, program, cancellationToken, /*isFixAll*/ false)); if (deletion.length) { return [createCodeFixAction(fixName, deletion, [Diagnostics.Remove_unused_declaration_for_Colon_0, token.getText(sourceFile)], fixIdDeleteImports, Diagnostics.Delete_all_unused_imports)]; } @@ -75,7 +75,7 @@ namespace ts.codefix { } else { const deletion = textChanges.ChangeTracker.with(context, t => - tryDeleteDeclaration(sourceFile, token, t, checker, sourceFiles, /*isFixAll*/ false)); + tryDeleteDeclaration(sourceFile, token, t, checker, sourceFiles, program, cancellationToken, /*isFixAll*/ false)); if (deletion.length) { const name = isComputedPropertyName(token.parent) ? token.parent : token; result.push(createDeleteFix(deletion, [Diagnostics.Remove_unused_declaration_for_Colon_0, name.getText(sourceFile)])); @@ -91,7 +91,7 @@ namespace ts.codefix { }, fixIds: [fixIdPrefix, fixIdDelete, fixIdDeleteImports, fixIdInfer], getAllCodeActions: context => { - const { sourceFile, program } = context; + const { sourceFile, program, cancellationToken } = context; const checker = program.getTypeChecker(); const sourceFiles = program.getSourceFiles(); return codeFixAll(context, errorCodes, (changes, diag) => { @@ -106,7 +106,7 @@ namespace ts.codefix { changes.delete(sourceFile, importDecl); } else if (isImport(token)) { - tryDeleteDeclaration(sourceFile, token, changes, checker, sourceFiles, /*isFixAll*/ true); + tryDeleteDeclaration(sourceFile, token, changes, checker, sourceFiles, program, cancellationToken, /*isFixAll*/ true); } break; } @@ -132,7 +132,7 @@ namespace ts.codefix { deleteEntireVariableStatement(changes, sourceFile, token.parent); } else { - tryDeleteDeclaration(sourceFile, token, changes, checker, sourceFiles, /*isFixAll*/ true); + tryDeleteDeclaration(sourceFile, token, changes, checker, sourceFiles, program, cancellationToken, /*isFixAll*/ true); } break; } @@ -217,8 +217,8 @@ namespace ts.codefix { return false; } - function tryDeleteDeclaration(sourceFile: SourceFile, token: Node, changes: textChanges.ChangeTracker, checker: TypeChecker, sourceFiles: readonly SourceFile[], isFixAll: boolean) { - tryDeleteDeclarationWorker(token, changes, sourceFile, checker, sourceFiles, isFixAll); + function tryDeleteDeclaration(sourceFile: SourceFile, token: Node, changes: textChanges.ChangeTracker, checker: TypeChecker, sourceFiles: readonly SourceFile[], program: Program, cancellationToken: CancellationToken, isFixAll: boolean) { + tryDeleteDeclarationWorker(token, changes, sourceFile, checker, sourceFiles, program, cancellationToken, isFixAll); if (isIdentifier(token)) deleteAssignments(changes, sourceFile, token, checker); } @@ -231,18 +231,18 @@ namespace ts.codefix { }); } - function tryDeleteDeclarationWorker(token: Node, changes: textChanges.ChangeTracker, sourceFile: SourceFile, checker: TypeChecker, sourceFiles: readonly SourceFile[], isFixAll: boolean): void { + function tryDeleteDeclarationWorker(token: Node, changes: textChanges.ChangeTracker, sourceFile: SourceFile, checker: TypeChecker, sourceFiles: readonly SourceFile[], program: Program, cancellationToken: CancellationToken, isFixAll: boolean): void { const { parent } = token; if (isParameter(parent)) { - tryDeleteParameter(changes, sourceFile, parent, checker, sourceFiles, isFixAll); + tryDeleteParameter(changes, sourceFile, parent, checker, sourceFiles, program, cancellationToken, isFixAll); } else { changes.delete(sourceFile, isImportClause(parent) ? token : isComputedPropertyName(parent) ? parent.parent : parent); } } - function tryDeleteParameter(changes: textChanges.ChangeTracker, sourceFile: SourceFile, p: ParameterDeclaration, checker: TypeChecker, sourceFiles: readonly SourceFile[], isFixAll = false): void { - if (mayDeleteParameter(checker, sourceFile, p, isFixAll)) { + function tryDeleteParameter(changes: textChanges.ChangeTracker, sourceFile: SourceFile, p: ParameterDeclaration, checker: TypeChecker, sourceFiles: readonly SourceFile[], program: Program, cancellationToken: CancellationToken, isFixAll = false): void { + if (mayDeleteParameter(checker, sourceFile, p, sourceFiles, program, cancellationToken, 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)); @@ -254,15 +254,36 @@ namespace ts.codefix { } } - function mayDeleteParameter(checker: TypeChecker, sourceFile: SourceFile, parameter: ParameterDeclaration, isFixAll: boolean): boolean { + function mayDeleteParameter(checker: TypeChecker, sourceFile: SourceFile, parameter: ParameterDeclaration, sourceFiles: readonly SourceFile[], program: Program, cancellationToken: CancellationToken, 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: + const index = parent.parameters.indexOf(parameter); + const referent = isMethodDeclaration(parent) ? parent.name : parent; + const entries = FindAllReferences.Core.getReferencedSymbolsForNode(parent.pos, referent, program, sourceFiles, cancellationToken); + if (entries) { + for (const entry of entries) { + for (const reference of entry.references) { + if (reference.kind === FindAllReferences.EntryKind.Node) { + // argument in super(...) + const isSuperCall = isSuperKeyword(reference.node) + && isCallExpression(reference.node.parent) + && reference.node.parent.arguments.length > index; + // argument in super.m(...) + const isSuperMethodCall = isPropertyAccessExpression(reference.node.parent) + && isSuperKeyword(reference.node.parent.expression) + && isCallExpression(reference.node.parent.parent) + && reference.node.parent.parent.arguments.length > index; + // parameter in overridden or overriding method + const isOverriddenMethod = (isMethodDeclaration(reference.node.parent) || isMethodSignature(reference.node.parent)) + && reference.node.parent !== parameter.parent + && reference.node.parent.parameters.length > index; + if (isSuperCall || isSuperMethodCall || isOverriddenMethod) return false; + } + } + } + } return true; case SyntaxKind.FunctionDeclaration: { if (parent.name && isCallbackLike(checker, sourceFile, parent.name)) { diff --git a/src/services/findAllReferences.ts b/src/services/findAllReferences.ts index 953fafe3c9..d7741036b4 100644 --- a/src/services/findAllReferences.ts +++ b/src/services/findAllReferences.ts @@ -616,7 +616,8 @@ namespace ts.FindAllReferences { } const checker = program.getTypeChecker(); - const symbol = checker.getSymbolAtLocation(node); + // constructors should use the class symbol, detected by name, if present + const symbol = checker.getSymbolAtLocation(isConstructorDeclaration(node) && node.parent.name || node); // Could not find a symbol e.g. unknown identifier if (!symbol) { @@ -874,6 +875,7 @@ namespace ts.FindAllReferences { function getSpecialSearchKind(node: Node): SpecialSearchKind { switch (node.kind) { + case SyntaxKind.Constructor: case SyntaxKind.ConstructorKeyword: return SpecialSearchKind.Constructor; case SyntaxKind.Identifier: @@ -2055,6 +2057,34 @@ namespace ts.FindAllReferences { } } + /** + * 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 = new Map(); + 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)); + })); + } + } + interface RelatedSymbol { readonly symbol: Symbol; readonly kind: NodeEntryKind | undefined; diff --git a/src/services/utilities.ts b/src/services/utilities.ts index ead7655e46..0a5af0a517 100644 --- a/src/services/utilities.ts +++ b/src/services/utilities.ts @@ -1894,38 +1894,6 @@ namespace ts { return typeOfPattern && checker.getPropertyOfType(typeOfPattern, bindingElement.name.text); } - /** - * 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 = new Map(); - 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 function getParentNodeInSpan(node: Node | undefined, file: SourceFile, span: TextSpan): Node | undefined { if (!node) return undefined; diff --git a/tests/cases/fourslash/codeFixUnusedIdentifier_super.ts b/tests/cases/fourslash/codeFixUnusedIdentifier_super.ts new file mode 100644 index 0000000000..e4ecaf13ab --- /dev/null +++ b/tests/cases/fourslash/codeFixUnusedIdentifier_super.ts @@ -0,0 +1,43 @@ +/// + +// @noUnusedParameters: true + + +//// class B { +//// fromBase(x: number) { return x } +//// } +//// +//// class C extends B { +//// fromDerived(keep1: number, remove1: number) {} +//// fromBase(keep2: number, remove2: any) {} +//// m(keep3: number, remove3: number) {} +//// } +//// +//// class D extends C { +//// fromDerived(x: number) { return x } +//// caller() { +//// super.m(1); +//// } +//// } +//// + +verify.codeFixAll({ + fixId: "unusedIdentifier_delete", + fixAllDescription: ts.Diagnostics.Delete_all_unused_declarations.message, + newFileContent: `class B { + fromBase(x: number) { return x } +} + +class C extends B { + fromDerived(keep1: number) {} + fromBase(keep2: number) {} + m(keep3: number) {} +} + +class D extends C { + fromDerived(x: number) { return x } + caller() { + super.m(1); + } +} +`}); diff --git a/tests/cases/fourslash/codeFixUnusedIdentifier_super1.ts b/tests/cases/fourslash/codeFixUnusedIdentifier_super1.ts new file mode 100644 index 0000000000..7047175ffd --- /dev/null +++ b/tests/cases/fourslash/codeFixUnusedIdentifier_super1.ts @@ -0,0 +1,16 @@ +/// + +// @noUnusedParameters: true + +//// class Base { +//// constructor(x: number) {} // Remove unused parameter +//// } +//// +//// class Derived extends Base { +//// constructor(x: number) { +//// super(x); +//// } +//// } + +// No codefix to remove a non-last parameter in a callback +verify.codeFixAvailable([{ description: "Prefix 'x' with an underscore" }]); diff --git a/tests/cases/fourslash/codeFixUnusedIdentifier_super2.ts b/tests/cases/fourslash/codeFixUnusedIdentifier_super2.ts new file mode 100644 index 0000000000..8bf80e4f55 --- /dev/null +++ b/tests/cases/fourslash/codeFixUnusedIdentifier_super2.ts @@ -0,0 +1,19 @@ +/// + +// @noUnusedParameters: true + +//// class Base { +//// constructor(x: number) {} // Remove unused parameter +//// } +//// +//// class Derived extends Base { +//// constructor() { +//// super(); +//// } +//// } + +// No codefix to remove a non-last parameter in a callback +verify.codeFixAvailable([ + { description: "Remove unused declaration for: 'x'" }, + { description: "Prefix 'x' with an underscore" } +]);