"Delete all unused declarations" on TS imports removes code in files

[oguimbal]

• VSCode Version: 1.31.1
• OS Version: Windows 10 (not relevant)

Steps to Reproduce:

I feel that something is terribly wrong with the current behaviour of "Delete all unused declarations" codefix.

It is suggested to remove an unused import. In this context, its implicit meaning is (for me) something like "Delete all unused imports ".

But the actual behaviour can be quite dangerous, as it can remove actual code in the file body (which is definitely not something I would expect when refactoring dependencies):

We experienced this in my team recently (and deleted parts of code that should not have been deleted), and I dont understand how this can be the default behaviour.

NB: I'm not suggesting to fix the "Delete all unused declaration" codefix, rather to use another codefix when refactoring the depenency context... so I was not sure if this issue has to be posted on the Typescript repository, or here.

Does this issue occur when all extensions are disabled?: Yes

[mjbvz]

Moving upstream for more feedback.

The feature is working as currently designed. Separating removal of unused imports and other unused declarations was previously discussed in #21376 which has gotten thumbs up in the past year

[Kingwl]

Is that means we need a new code fix called remove all unused import?

[oguimbal]

Or to fix the current codefix so it does not remove any code that might have side-effects. Only declarations (which should be the default behaviour of this fix, if you ask me... the current implementation is very dangerous when using "fix all occurences").

i.e. let unusedVar = someMethod() should be transformed to someMethod().

But I guess that would raise the question: what should be done with expressions that would be raising no-unused-expression, like let unused = x.y (getters are methods that may have side-effects, after all), or worse: let unused = method() + 42.

[hbenl]

The feature is working as currently designed.

Seriously? I would not expect an automatic refactoring method to break my code quite so brazenly. Nor can I imagine that anyone else would expect that. I would expect an automatic refactoring to always try to be on the safe side.

i.e. let unusedVar = someMethod() should be transformed to someMethod().

That's exactly what I would have expected.

So please change this behavior. The refactoring methods in Typescript are incredibly useful for cleaning up old javascript code (and there's a lot of that out there), but I have to be able to rely on them to be reasonably safe.

Note that the other issue discussed here (separating the removal of all unused imports from the removal of all unused declarations) is also a very good idea, but totally independent of the issue of the removed code (and won't fix that issue).

[two-bridges]

The feature is working as currently designed

So, I'm literally staring at code changes staged for production and testing reveals nothing is saving to the database... Turns out we used 'Delete Unused Declarations' to clean up some code.... then I googled, then I found this thread.

[ricklove]

This has bit me many times now. It seems that it used to just remove imports and maybe was changed to remove all code that isn't used: Including any functions I was currently working on and forgot to put an export because I haven't called that function yet...

I would love a remove all unused imports but remove all unused declarations is unsafe.

It's one of those 'too good to be true' deals where the scammer ends up stealing your kidneys. 🤣