maxmustermann/TypeScript
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Unused Identifier codefix better understands constructors and methods (#41555)

Browse source 
* Unuse Identifier codefix understands constructors

Previously, it did not look for `super()` and `new this()` calls when
determining whether a constructor parameter could be deleted.

* better names, fix off-by-1 bug

* Codefix understands super methods now too

This unifies the code, changing it considerably.
This commit is contained in:
Nathan Shively-Sanders 2020-11-18 11:19:43 -08:00 committed by GitHub
parent 6643d97385
commit d3abd35428
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 147 additions and 50 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

55
src/services/codefixes/fixUnusedIdentifier.ts
View file

@ -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, <VariableDeclarationList>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)) {

32
src/services/findAllReferences.ts
View file

@ -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<T>(symbol: Symbol, propertyName: string, checker: TypeChecker, cb: (symbol: Symbol) => T | undefined): T | undefined {
const seen = new Map<string, 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));
}));
}
}
interface RelatedSymbol {
readonly symbol: Symbol;
readonly kind: NodeEntryKind | undefined;

32
src/services/utilities.ts
View file

@ -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<T>(symbol: Symbol, propertyName: string, checker: TypeChecker, cb: (symbol: Symbol) => T | undefined): T | undefined {
const seen = new Map<string, 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 function getParentNodeInSpan(node: Node | undefined, file: SourceFile, span: TextSpan): Node | undefined {
if (!node) return undefined;

43
tests/cases/fourslash/codeFixUnusedIdentifier_super.ts Normal file
View file

@ -0,0 +1,43 @@
/// <reference path='fourslash.ts' />
// @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);
}
}
`});

16
tests/cases/fourslash/codeFixUnusedIdentifier_super1.ts Normal file
View file

@ -0,0 +1,16 @@
/// <reference path='fourslash.ts' />
// @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" }]);

19
tests/cases/fourslash/codeFixUnusedIdentifier_super2.ts Normal file
View file

@ -0,0 +1,19 @@
/// <reference path='fourslash.ts' />
// @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" }
]);