Typescript: Delete unused declarations codefix should offer "Delete unused elements of only this declaration" too

[qinxiaoxian]

Issue Type: Bug

When I click 'Delete all unused declarations', my function with three parameters, the second of which was not used, was removed. This is a serious error, it should not be removed, because it makes my logic wrong.

VS Code version: Code 1.35.1 (c7d83e57cd18f18026a8162d042843bda1bcf21f, 2019-06-12T14:30:02.622Z)
OS version: Windows_NT x64 10.0.17763

System Info

|Item|Value|
|---|---|
|CPUs|Intel(R) Core(TM) i5-8600K CPU @ 3.60GHz (6 x 3600)|
|GPU Status|2d_canvas: enabled
checker_imaging: disabled_off
flash_3d: enabled
flash_stage3d: enabled
flash_stage3d_baseline: enabled
gpu_compositing: enabled
multiple_raster_threads: enabled_on
native_gpu_memory_buffers: disabled_software
rasterization: enabled
surface_synchronization: enabled_on
video_decode: enabled
webgl: enabled
webgl2: enabled|
|Load (avg)|undefined|
|Memory (System)|23.86GB (17.18GB free)|
|Process Argv||
|Screen Reader|no|
|VM|0%|

Extensions (33)

Extension|Author (truncated)|Version
---|---|---
rainbow-brackets|2gu|0.0.6
vscode-file-peek|abi|1.0.1
vscode-browser-preview|auc|0.5.9
path-intellisense|chr|1.4.2
angular-schematics|cyr|1.23.0
vscode-eslint|dba|1.9.0
githistory|don|0.4.6
gitlens|eam|9.8.2
EditorConfig|Edi|0.13.0
tslint|eg2|1.0.43
vscode-npm-script|eg2|0.3.7
tortoise-svn|fan|0.1.1
auto-rename-tag|for|0.1.0
align-mode|Gru|0.0.16
svgeditor|hen|2.9.0
git-graph|mhu|1.9.0
code-beautifier|mic|2.2.1
Angular-BeastCode|Mik|8.0.10
vscode-postgresql|ms-|0.2.0
powershell|ms-|2019.5.0
debugger-for-chrome|msj|4.11.6
sqltools|mtx|0.19.6
angular-console|nrw|8.0.5
material-icon-theme|PKi|3.8.0
vscode-yaml|red|0.4.1
angular-follow-selector|san|1.2.0
open-in-browser|tec|2.0.0
pdf|tom|0.5.0
non-breaking-space-highlighter|vik|0.0.3
vscodeintellicode|Vis|1.1.7
vscode-icons|vsc|8.8.0
vscode-todo-highlight|way|1.0.4
html-css-class-completion|Zig|1.19.0

(1 theme extensions excluded)

[vscodebot[bot]]

(Experimental duplicate detection)
Thanks for submitting this issue. Please also check if it is already covered by an existing one, like:

• "Delete all unused declarations" on TS imports removes code in files (#69964)
  

[qinxiaoxian]

[qinxiaoxian]

It's a serious bug!!!

Before delete unused declaration

After delete unused declaration

[mjbvz]

Example code:

export function foo(a, b, c) { return a + c; }

const x = 1;

Triggering Remove all unused declarations on x removes parameter b too

[fatcerberus]

Wow, that's, um... yeah. That's pretty bad. Automated deletion of unused declarations shouldn't change the meaning of the code, and definitely shouldn't be deleting live code!

[AnyhowStep]

I guess the second example is a "bug" only because that expression has side effects.

Since TS can't know if a function call/await has side effects or not, it should just leave all such expressions intact, even if it means potentially leaving useless getX(); statements all over.

const unusedA = fooWithSideEffect();
const unusedB = fooWithoutSideEffect();

After removing unused declarations,

fooWithSideEffect();
fooWithoutSideEffect();

[qinxiaoxian]

Why is this problem moved to TypeScript? I write JS code. I feel that this problem is the problem of VSCode itself. Will this problem be solved?

[fatcerberus]

Why is this problem moved to TypeScript?

Because the TypeScript language service handles IntelliSense and refactorings for both JS and TS in VSCode.

[oguimbal]

You should NEVER use 'Delete all unused declaration' ... it is dangerous (it can break your code silently - i.e. without breaking compilation). #30260

[sandersn]

It sounds like there are 5 tasks here between this bug and #30260. Does this sound like an accurate breakdown?

1. Add "Remove all unused imports" codefix.
2. Make "Remove all unused declarations" conservative with respect to side effects. Briefly, do not delete initialisers of declarations.
3. Add "Remove [all] unused expression[s]" codefix.
4. Make "Remove all unused declarations" skip non-trailing parameters.
5. Make sure that removing a single unused non-trailing parameter either (1) does not work or (2) only works when all arguments have a non-void/undefined type.

I need to review the original design before commenting any more, especially for (2) and (5). We originally left (3) for linters, but we now have less certain codefixes that spatter suggestions everywhere, so I think it's a good candidate.

[sandersn]

I think, together, #41105 and #41168 fix the important problems with this codefix. The first separates fix-all-imports from other declarations, and the second stops the codefix from deleting any parameter that is ever provided with an argument. It also skips variable declarations with initialisers when in fix-all mode.

Notably, neither PR addresses @mjbvz's example:

export function foo(a, b, c) { return a + c; }

const x = 1;

b is still removed during a fix-all. I think this is OK for a locally-scoped function, and not terrible for an exported or global function.