fixUnusedIdentifier: Remove arguments corresponding to unused parameters #25011
Conversation
| @@ -82,12 +86,16 @@ namespace ts.codefix { | |||
| }, | |||
| }); | |||
|
|
|||
| function hasDeletedAncestor(deletedAncestors: ReadonlyNodeSet, node: Node): boolean { | |||
There was a problem hiding this comment.
can you add a test where the ranges are nested:
function f(a: any, unused: any) { a; }
f(1, {
prop: f(2, [
f(3, f(4, undefined))
])
});There was a problem hiding this comment.
or even more interesting if the first one is the nested reference:
function f(a: any, unused: any) { a; }
function g(a: any, unused: any) { a; }
g(1, {
prop: f(2, [
g(3, f(4, undefined))
])
});There was a problem hiding this comment.
After testing more, looks like the previous strategy wouldn't work because it relied on us deleting higher-leve nodes before lower-level ones. Changed this to just collect a list of deletions and do them all at the end, when we can know what all the high-level deletions are.
|
@mhegazy Could you re-review? |
| @@ -11624,6 +11633,15 @@ declare namespace ts.codefix { | |||
| } | |||
| declare namespace ts.codefix { | |||
| } | |||
| declare namespace ts.codefix { | |||
| class Deleter { | |||
There was a problem hiding this comment.
where is this defined? i can not see to find it in this PR?
There was a problem hiding this comment.
Bottom of fixUnusedIdentifier.ts. It's internal but those are currently being included in the API (#24966).
There was a problem hiding this comment.
thanks! can we move this to the change tracker instead?
| if (delDestructure.length) { | ||
| return [createCodeFixAction(fixName, delDestructure, Diagnostics.Remove_destructuring, fixIdDelete, Diagnostics.Delete_all_unused_declarations)]; | ||
| } | ||
| const delVar = textChanges.ChangeTracker.with(context, t => tryDeleteFullVariableStatement(t, sourceFile, startToken, /*deleted*/ undefined)); | ||
| const delVar = Deleter.with(context, d => tryDeleteFullVariableStatement(sourceFile, token, d)); |
There was a problem hiding this comment.
can we do this in the change tracker instead? the change tracker will track the changes, and will only do them at the end, so we can do the filtering of deleted ranges then instead of having a special deleter object.
ghost
merged commit 
ghost
mentioned this pull request
Fixes #24789