Skip to content

fixUnusedIdentifier: Don't delete node whose ancestor was already deleted #24207

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
1 commit merged into from
May 17, 2018
Merged

fixUnusedIdentifier: Don't delete node whose ancestor was already deleted #24207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 64 additions & 35 deletions src/services/codefixes/fixUnusedIdentifier.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,15 @@ 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, context.span.start));
const delDestructure = textChanges.ChangeTracker.with(context, t => tryDeleteFullDestructure(t, sourceFile, context.span.start, /*deleted*/ undefined));
if (delDestructure.length) {
return [createCodeFixAction(fixName, delDestructure, Diagnostics.Remove_destructuring, fixIdDelete, Diagnostics.Delete_all_unused_declarations)];
}

const token = getToken(sourceFile, textSpanEnd(context.span));
const result: CodeFixAction[] = [];

const deletion = textChanges.ChangeTracker.with(context, t => tryDeleteDeclaration(t, sourceFile, token));
const deletion = textChanges.ChangeTracker.with(context, t => tryDeleteDeclaration(t, sourceFile, token, /*deleted*/ undefined));
if (deletion.length) {
result.push(createCodeFixAction(fixName, deletion, [Diagnostics.Remove_declaration_for_Colon_0, token.getText(sourceFile)], fixIdDelete, Diagnostics.Delete_all_unused_declarations));
}
Expand All @@ -40,30 +40,37 @@ namespace ts.codefix {
return result;
},
fixIds: [fixIdPrefix, fixIdDelete],
getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, diag) => {
const { sourceFile } = context;
const token = findPrecedingToken(textSpanEnd(diag), diag.file!);
switch (context.fixId) {
case fixIdPrefix:
if (isIdentifier(token) && canPrefix(token)) {
tryPrefixDeclaration(changes, diag.code, sourceFile, token);
}
break;
case fixIdDelete:
const importDecl = tryGetFullImport(diag.file!, diag.start!);
if (importDecl) {
changes.deleteNode(sourceFile, importDecl);
}
else {
if (!tryDeleteFullDestructure(changes, sourceFile, diag.start!)) {
tryDeleteDeclaration(changes, sourceFile, token);
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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we do this in the chanetracker instead, and have deleteRange filter out deletions if they are already subsumed with other ones?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that indicates a genuine error though, that we might want to know about. There's also one case where we replaceNode instead that would presumably still be seen as an error by ChangeTracker.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so every quick fix/refactor that needs to remove things will need to do this book keeping?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, this is the only code fix that should potentially delete a node and one of its children. In other cases that would be a bug that we would want to catch..

return codeFixAll(context, errorCodes, (changes, diag) => {
const { sourceFile } = context;
const token = findPrecedingToken(textSpanEnd(diag), diag.file!);
switch (context.fixId) {
case fixIdPrefix:
if (isIdentifier(token) && canPrefix(token)) {
tryPrefixDeclaration(changes, diag.code, sourceFile, token);
}
}
break;
default:
Debug.fail(JSON.stringify(context.fixId));
}
}),
break;
case fixIdDelete:
// Ignore if this range was already deleted.
if (deleted.some(d => rangeContainsPosition(d, diag.start!))) break;

const importDecl = tryGetFullImport(diag.file!, diag.start!);
if (importDecl) {
changes.deleteNode(sourceFile, importDecl);
}
else {
if (!tryDeleteFullDestructure(changes, sourceFile, diag.start!, deleted)) {
tryDeleteDeclaration(changes, sourceFile, token, deleted);
}
}
break;
default:
Debug.fail(JSON.stringify(context.fixId));
}
});
},
});

// Sometimes the diagnostic span is an entire ImportDeclaration, so we should remove the whole thing.
Expand All @@ -72,18 +79,20 @@ namespace ts.codefix {
return startToken.kind === SyntaxKind.ImportKeyword ? tryCast(startToken.parent, isImportDeclaration) : undefined;
}

function tryDeleteFullDestructure(changes: textChanges.ChangeTracker, sourceFile: SourceFile, pos: number): boolean {
function tryDeleteFullDestructure(changes: textChanges.ChangeTracker, sourceFile: SourceFile, pos: number, deletedAncestors: NodeSet | undefined): boolean {
const startToken = getTokenAtPosition(sourceFile, pos, /*includeJsDocComment*/ false);
if (startToken.kind !== SyntaxKind.OpenBraceToken || !isObjectBindingPattern(startToken.parent)) return false;
const decl = startToken.parent.parent;
switch (decl.kind) {
case SyntaxKind.VariableDeclaration:
tryDeleteVariableDeclaration(changes, sourceFile, decl);
tryDeleteVariableDeclaration(changes, sourceFile, decl, deletedAncestors);
break;
case SyntaxKind.Parameter:
if (deletedAncestors) deletedAncestors.add(decl);
changes.deleteNodeInList(sourceFile, decl);
break;
case SyntaxKind.BindingElement:
if (deletedAncestors) deletedAncestors.add(decl);
changes.deleteNode(sourceFile, decl);
break;
default:
Expand Down Expand Up @@ -121,34 +130,37 @@ namespace ts.codefix {
return false;
}

function tryDeleteDeclaration(changes: textChanges.ChangeTracker, sourceFile: SourceFile, token: Node): void {
function tryDeleteDeclaration(changes: textChanges.ChangeTracker, sourceFile: SourceFile, token: Node, deletedAncestors: NodeSet | undefined): void {
switch (token.kind) {
case SyntaxKind.Identifier:
tryDeleteIdentifier(changes, sourceFile, <Identifier>token);
tryDeleteIdentifier(changes, sourceFile, <Identifier>token, deletedAncestors);
break;
case SyntaxKind.PropertyDeclaration:
case SyntaxKind.NamespaceImport:
if (deletedAncestors) deletedAncestors.add(token.parent);
changes.deleteNode(sourceFile, token.parent);
break;
default:
tryDeleteDefault(changes, sourceFile, token);
tryDeleteDefault(changes, sourceFile, token, deletedAncestors);
}
}

function tryDeleteDefault(changes: textChanges.ChangeTracker, sourceFile: SourceFile, token: Node): void {
function tryDeleteDefault(changes: textChanges.ChangeTracker, sourceFile: SourceFile, token: Node, deletedAncestors: NodeSet | undefined): void {
if (isDeclarationName(token)) {
if (deletedAncestors) deletedAncestors.add(token.parent);
changes.deleteNode(sourceFile, token.parent);
}
else if (isLiteralComputedPropertyDeclarationName(token)) {
if (deletedAncestors) deletedAncestors.add(token.parent.parent);
changes.deleteNode(sourceFile, token.parent.parent);
}
}

function tryDeleteIdentifier(changes: textChanges.ChangeTracker, sourceFile: SourceFile, identifier: Identifier): void {
function tryDeleteIdentifier(changes: textChanges.ChangeTracker, sourceFile: SourceFile, identifier: Identifier, deletedAncestors: NodeSet | undefined): void {
const parent = identifier.parent;
switch (parent.kind) {
case SyntaxKind.VariableDeclaration:
tryDeleteVariableDeclaration(changes, sourceFile, <VariableDeclaration>parent);
tryDeleteVariableDeclaration(changes, sourceFile, <VariableDeclaration>parent, deletedAncestors);
break;

case SyntaxKind.TypeParameter:
Expand Down Expand Up @@ -255,7 +267,7 @@ namespace ts.codefix {
break;

default:
tryDeleteDefault(changes, sourceFile, identifier);
tryDeleteDefault(changes, sourceFile, identifier, deletedAncestors);
break;
}
}
Expand All @@ -280,15 +292,17 @@ namespace ts.codefix {
}

// token.parent is a variableDeclaration
function tryDeleteVariableDeclaration(changes: textChanges.ChangeTracker, sourceFile: SourceFile, varDecl: VariableDeclaration): void {
function tryDeleteVariableDeclaration(changes: textChanges.ChangeTracker, sourceFile: SourceFile, varDecl: VariableDeclaration, deletedAncestors: NodeSet | undefined): void {
switch (varDecl.parent.parent.kind) {
case SyntaxKind.ForStatement: {
const forStatement = varDecl.parent.parent;
const forInitializer = <VariableDeclarationList>forStatement.initializer;
if (forInitializer.declarations.length === 1) {
if (deletedAncestors) deletedAncestors.add(forInitializer);
changes.deleteNode(sourceFile, forInitializer);
}
else {
if (deletedAncestors) deletedAncestors.add(varDecl);
changes.deleteNodeInList(sourceFile, varDecl);
}
break;
Expand All @@ -298,6 +312,7 @@ namespace ts.codefix {
const forOfStatement = varDecl.parent.parent;
Debug.assert(forOfStatement.initializer.kind === SyntaxKind.VariableDeclarationList);
const forOfInitializer = <VariableDeclarationList>forOfStatement.initializer;
if (deletedAncestors) deletedAncestors.add(forOfInitializer.declarations[0]);
changes.replaceNode(sourceFile, forOfInitializer.declarations[0], createObjectLiteral());
break;

Expand All @@ -308,11 +323,25 @@ namespace ts.codefix {
default:
const variableStatement = varDecl.parent.parent;
if (variableStatement.declarationList.declarations.length === 1) {
if (deletedAncestors) deletedAncestors.add(variableStatement);
changes.deleteNode(sourceFile, variableStatement);
}
else {
if (deletedAncestors) deletedAncestors.add(varDecl);
changes.deleteNodeInList(sourceFile, varDecl);
}
}
}

class NodeSet {
private map = createMap<Node>();

add(node: Node): void {
this.map.set(String(getNodeId(node)), node);
}

some(pred: (node: Node) => boolean): boolean {
return forEachEntry(this.map, pred) || false;
}
}
}
2 changes: 1 addition & 1 deletion src/services/suggestionDiagnostics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ namespace ts {
}
}

return diags.concat(checker.getSuggestionDiagnostics(sourceFile));
return diags.concat(checker.getSuggestionDiagnostics(sourceFile)).sort((d1, d2) => d1.start - d2.start);
}

// convertToEs6Module only works on top-level, so don't trigger it if commonjs code only appears in nested scopes.
Expand Down
4 changes: 4 additions & 0 deletions src/services/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,10 @@ namespace ts {
return startEndContainsRange(r1.pos, r1.end, r2);
}

export function rangeContainsPosition(r: TextRange, pos: number): boolean {
return r.pos <= pos && pos <= r.end;
}

export function startEndContainsRange(start: number, end: number, range: TextRange): boolean {
return start <= range.pos && end >= range.end;
}
Expand Down
10 changes: 10 additions & 0 deletions tests/cases/fourslash/codeFixUnusedIdentifier_all_delete_paramInFunction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/// <reference path='fourslash.ts' />

////export {};
////function f(x) {}

verify.codeFixAll({
fixId: "unusedIdentifier_delete",
fixAllDescription: "Delete all unused declarations",
newFileContent: "export {};\n",
});