Skip to content

Commit 08c364d

Browse files
author
Andy
authored
fixUnusedIdentifier: Don't delete node whose ancestor was already deleted (#24207)
1 parent c25b02f commit 08c364d

File tree

4 files changed

+79
-36
lines changed

4 files changed

+79
-36
lines changed

‎src/services/codefixes/fixUnusedIdentifier.ts

+64-35
Original file line numberDiff line numberDiff line change
@@ -19,15 +19,15 @@ namespace ts.codefix {
1919
const changes = textChanges.ChangeTracker.with(context, t => t.deleteNode(sourceFile, importDecl));
2020
return [createCodeFixAction(fixName, changes, [Diagnostics.Remove_import_from_0, showModuleSpecifier(importDecl)], fixIdDelete, Diagnostics.Delete_all_unused_declarations)];
2121
}
22-
const delDestructure = textChanges.ChangeTracker.with(context, t => tryDeleteFullDestructure(t, sourceFile, context.span.start));
22+
const delDestructure = textChanges.ChangeTracker.with(context, t => tryDeleteFullDestructure(t, sourceFile, context.span.start, /*deleted*/ undefined));
2323
if (delDestructure.length) {
2424
return [createCodeFixAction(fixName, delDestructure, Diagnostics.Remove_destructuring, fixIdDelete, Diagnostics.Delete_all_unused_declarations)];
2525
}
2626

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

30-
const deletion = textChanges.ChangeTracker.with(context, t => tryDeleteDeclaration(t, sourceFile, token));
30+
const deletion = textChanges.ChangeTracker.with(context, t => tryDeleteDeclaration(t, sourceFile, token, /*deleted*/ undefined));
3131
if (deletion.length) {
3232
result.push(createCodeFixAction(fixName, deletion, [Diagnostics.Remove_declaration_for_Colon_0, token.getText(sourceFile)], fixIdDelete, Diagnostics.Delete_all_unused_declarations));
3333
}
@@ -40,30 +40,37 @@ namespace ts.codefix {
4040
return result;
4141
},
4242
fixIds: [fixIdPrefix, fixIdDelete],
43-
getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, diag) => {
44-
const { sourceFile } = context;
45-
const token = findPrecedingToken(textSpanEnd(diag), diag.file!);
46-
switch (context.fixId) {
47-
case fixIdPrefix:
48-
if (isIdentifier(token) && canPrefix(token)) {
49-
tryPrefixDeclaration(changes, diag.code, sourceFile, token);
50-
}
51-
break;
52-
case fixIdDelete:
53-
const importDecl = tryGetFullImport(diag.file!, diag.start!);
54-
if (importDecl) {
55-
changes.deleteNode(sourceFile, importDecl);
56-
}
57-
else {
58-
if (!tryDeleteFullDestructure(changes, sourceFile, diag.start!)) {
59-
tryDeleteDeclaration(changes, sourceFile, token);
43+
getAllCodeActions: context => {
44+
// Track a set of deleted nodes that may be ancestors of other marked for deletion -- only delete the ancestors.
45+
const deleted = new NodeSet();
46+
return codeFixAll(context, errorCodes, (changes, diag) => {
47+
const { sourceFile } = context;
48+
const token = findPrecedingToken(textSpanEnd(diag), diag.file!);
49+
switch (context.fixId) {
50+
case fixIdPrefix:
51+
if (isIdentifier(token) && canPrefix(token)) {
52+
tryPrefixDeclaration(changes, diag.code, sourceFile, token);
6053
}
61-
}
62-
break;
63-
default:
64-
Debug.fail(JSON.stringify(context.fixId));
65-
}
66-
}),
54+
break;
55+
case fixIdDelete:
56+
// Ignore if this range was already deleted.
57+
if (deleted.some(d => rangeContainsPosition(d, diag.start!))) break;
58+
59+
const importDecl = tryGetFullImport(diag.file!, diag.start!);
60+
if (importDecl) {
61+
changes.deleteNode(sourceFile, importDecl);
62+
}
63+
else {
64+
if (!tryDeleteFullDestructure(changes, sourceFile, diag.start!, deleted)) {
65+
tryDeleteDeclaration(changes, sourceFile, token, deleted);
66+
}
67+
}
68+
break;
69+
default:
70+
Debug.fail(JSON.stringify(context.fixId));
71+
}
72+
});
73+
},
6774
});
6875

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

75-
function tryDeleteFullDestructure(changes: textChanges.ChangeTracker, sourceFile: SourceFile, pos: number): boolean {
82+
function tryDeleteFullDestructure(changes: textChanges.ChangeTracker, sourceFile: SourceFile, pos: number, deletedAncestors: NodeSet | undefined): boolean {
7683
const startToken = getTokenAtPosition(sourceFile, pos, /*includeJsDocComment*/ false);
7784
if (startToken.kind !== SyntaxKind.OpenBraceToken || !isObjectBindingPattern(startToken.parent)) return false;
7885
const decl = startToken.parent.parent;
7986
switch (decl.kind) {
8087
case SyntaxKind.VariableDeclaration:
81-
tryDeleteVariableDeclaration(changes, sourceFile, decl);
88+
tryDeleteVariableDeclaration(changes, sourceFile, decl, deletedAncestors);
8289
break;
8390
case SyntaxKind.Parameter:
91+
if (deletedAncestors) deletedAncestors.add(decl);
8492
changes.deleteNodeInList(sourceFile, decl);
8593
break;
8694
case SyntaxKind.BindingElement:
95+
if (deletedAncestors) deletedAncestors.add(decl);
8796
changes.deleteNode(sourceFile, decl);
8897
break;
8998
default:
@@ -121,34 +130,37 @@ namespace ts.codefix {
121130
return false;
122131
}
123132

124-
function tryDeleteDeclaration(changes: textChanges.ChangeTracker, sourceFile: SourceFile, token: Node): void {
133+
function tryDeleteDeclaration(changes: textChanges.ChangeTracker, sourceFile: SourceFile, token: Node, deletedAncestors: NodeSet | undefined): void {
125134
switch (token.kind) {
126135
case SyntaxKind.Identifier:
127-
tryDeleteIdentifier(changes, sourceFile, <Identifier>token);
136+
tryDeleteIdentifier(changes, sourceFile, <Identifier>token, deletedAncestors);
128137
break;
129138
case SyntaxKind.PropertyDeclaration:
130139
case SyntaxKind.NamespaceImport:
140+
if (deletedAncestors) deletedAncestors.add(token.parent);
131141
changes.deleteNode(sourceFile, token.parent);
132142
break;
133143
default:
134-
tryDeleteDefault(changes, sourceFile, token);
144+
tryDeleteDefault(changes, sourceFile, token, deletedAncestors);
135145
}
136146
}
137147

138-
function tryDeleteDefault(changes: textChanges.ChangeTracker, sourceFile: SourceFile, token: Node): void {
148+
function tryDeleteDefault(changes: textChanges.ChangeTracker, sourceFile: SourceFile, token: Node, deletedAncestors: NodeSet | undefined): void {
139149
if (isDeclarationName(token)) {
150+
if (deletedAncestors) deletedAncestors.add(token.parent);
140151
changes.deleteNode(sourceFile, token.parent);
141152
}
142153
else if (isLiteralComputedPropertyDeclarationName(token)) {
154+
if (deletedAncestors) deletedAncestors.add(token.parent.parent);
143155
changes.deleteNode(sourceFile, token.parent.parent);
144156
}
145157
}
146158

147-
function tryDeleteIdentifier(changes: textChanges.ChangeTracker, sourceFile: SourceFile, identifier: Identifier): void {
159+
function tryDeleteIdentifier(changes: textChanges.ChangeTracker, sourceFile: SourceFile, identifier: Identifier, deletedAncestors: NodeSet | undefined): void {
148160
const parent = identifier.parent;
149161
switch (parent.kind) {
150162
case SyntaxKind.VariableDeclaration:
151-
tryDeleteVariableDeclaration(changes, sourceFile, <VariableDeclaration>parent);
163+
tryDeleteVariableDeclaration(changes, sourceFile, <VariableDeclaration>parent, deletedAncestors);
152164
break;
153165

154166
case SyntaxKind.TypeParameter:
@@ -255,7 +267,7 @@ namespace ts.codefix {
255267
break;
256268

257269
default:
258-
tryDeleteDefault(changes, sourceFile, identifier);
270+
tryDeleteDefault(changes, sourceFile, identifier, deletedAncestors);
259271
break;
260272
}
261273
}
@@ -280,15 +292,17 @@ namespace ts.codefix {
280292
}
281293

282294
// token.parent is a variableDeclaration
283-
function tryDeleteVariableDeclaration(changes: textChanges.ChangeTracker, sourceFile: SourceFile, varDecl: VariableDeclaration): void {
295+
function tryDeleteVariableDeclaration(changes: textChanges.ChangeTracker, sourceFile: SourceFile, varDecl: VariableDeclaration, deletedAncestors: NodeSet | undefined): void {
284296
switch (varDecl.parent.parent.kind) {
285297
case SyntaxKind.ForStatement: {
286298
const forStatement = varDecl.parent.parent;
287299
const forInitializer = <VariableDeclarationList>forStatement.initializer;
288300
if (forInitializer.declarations.length === 1) {
301+
if (deletedAncestors) deletedAncestors.add(forInitializer);
289302
changes.deleteNode(sourceFile, forInitializer);
290303
}
291304
else {
305+
if (deletedAncestors) deletedAncestors.add(varDecl);
292306
changes.deleteNodeInList(sourceFile, varDecl);
293307
}
294308
break;
@@ -298,6 +312,7 @@ namespace ts.codefix {
298312
const forOfStatement = varDecl.parent.parent;
299313
Debug.assert(forOfStatement.initializer.kind === SyntaxKind.VariableDeclarationList);
300314
const forOfInitializer = <VariableDeclarationList>forOfStatement.initializer;
315+
if (deletedAncestors) deletedAncestors.add(forOfInitializer.declarations[0]);
301316
changes.replaceNode(sourceFile, forOfInitializer.declarations[0], createObjectLiteral());
302317
break;
303318

@@ -308,11 +323,25 @@ namespace ts.codefix {
308323
default:
309324
const variableStatement = varDecl.parent.parent;
310325
if (variableStatement.declarationList.declarations.length === 1) {
326+
if (deletedAncestors) deletedAncestors.add(variableStatement);
311327
changes.deleteNode(sourceFile, variableStatement);
312328
}
313329
else {
330+
if (deletedAncestors) deletedAncestors.add(varDecl);
314331
changes.deleteNodeInList(sourceFile, varDecl);
315332
}
316333
}
317334
}
335+
336+
class NodeSet {
337+
private map = createMap<Node>();
338+
339+
add(node: Node): void {
340+
this.map.set(String(getNodeId(node)), node);
341+
}
342+
343+
some(pred: (node: Node) => boolean): boolean {
344+
return forEachEntry(this.map, pred) || false;
345+
}
346+
}
318347
}

‎src/services/suggestionDiagnostics.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ namespace ts {
6060
}
6161
}
6262

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

6666
// convertToEs6Module only works on top-level, so don't trigger it if commonjs code only appears in nested scopes.

‎src/services/utilities.ts

+4
Original file line numberDiff line numberDiff line change
@@ -417,6 +417,10 @@ namespace ts {
417417
return startEndContainsRange(r1.pos, r1.end, r2);
418418
}
419419

420+
export function rangeContainsPosition(r: TextRange, pos: number): boolean {
421+
return r.pos <= pos && pos <= r.end;
422+
}
423+
420424
export function startEndContainsRange(start: number, end: number, range: TextRange): boolean {
421425
return start <= range.pos && end >= range.end;
422426
}

‎tests/cases/fourslash/codeFixUnusedIdentifier_all_delete_paramInFunction.ts

+10
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
////export {};
4+
////function f(x) {}
5+
6+
verify.codeFixAll({
7+
fixId: "unusedIdentifier_delete",
8+
fixAllDescription: "Delete all unused declarations",
9+
newFileContent: "export {};\n",
10+
});

0 commit comments

Comments
 (0)