Added skipDestructiveCodeActions argument to organize imports server command

[JoshuaKGoldberg]

Fixes #43051

[DanielRosenwasser]

I think this is probably okay, but part of me feels like there's a difference between a user-specified import cleanup and one that was triggered incidentally by organize-imports-on-save.

[JoshuaKGoldberg]

Err, does such a preference exist in the services API? I don't see anything in src/compiler/types.ts but could add it that consideration if available.

[andrewbranch]

The combination of codefixes on save and save on blur is so horrifying to me; to pick up on Daniel’s point, I think it merits a broader discussion about what kinds of actions should be allowed through that sequence. My feeling is we should never run anything that deletes code without an explicit user action, and blurring your editor tab doesn’t count as an explicit user action.

[jessetrinity]

Doesn't "do X on save" require that the use take explicit action in the first place (add the configuration to vscode settings.json)?

[andrewbranch]

Yeah, but I’m talking about direct synchronous action. I’m not saying “this is dangerous so it needs to be opt-in”; I’m saying “we should not have a feature that deletes code when users aren’t looking, no matter how badly they think they want it.”

[andrewbranch]

And to clarify, I’m not talking about codefixes-on-save alone. That’s ok because the direct causal action is Ctrl+S. If it messes up, you’re actively observing the mistake and you can undo it. The problem is save-on-blur triggering codefixes-on-save. Because in that case, the you’re explicitly moving your attention away from the place where we’re performing the edits. Anything done via that chain reaction needs to be totally non-destructive (e.g., formatting).

[jessetrinity]

Thanks for the clarification. Does this make sense as a user preference, something like "remove unused imports when organizing imports" (only for the explicitly requested case)?

[jessetrinity]

@JoshuaKGoldberg we discussed this a bit with VSCode and we think it makes sense to add a user preference to control if code is deleted on save.

I propose allowDestructiveCodeActions?: boolean. Default would be true to keep the current behavior. 'false' would short circuit in the same way as your syntax fix. VSCode could decide whether to pass true or false based on how the code action is triggered.

The syntax error would then be checked after this option.

[JoshuaKGoldberg]

Super. Is that something I'd be well positioned to fix? I'm wary of taking on big projects that are still in early stages cross-team as an external contributor, but if you have a solid vision I could contribute to that'd make me very happy.

[jessetrinity]

The work on our (TypeScript) side should be pretty simple. You would just need to add the option to the existing UserPreferences in src\compiler\types.ts and src\server\protocol.ts. organizeImports already takes a UserPreferences argument, so you would just need to then check the new option in the code removal logic. I think you would also need to change the fourslash method to test the new behavior.

You don't necessarily need to make changes on the vscode side to get this change in but you're welcome to try. If you're not comfortable doing that we can just let them know about the new option. I'm not familiar enough with the vscode side logic to give you pointers anyway 😅.

[andrewbranch]

Note that UserPreferences are changed in a separate Configure request, so I’m not sure that (by itself) actually plays well with the idea of letting VS Code change it on the fly depending on how a command is triggered. We might need to add it as an argument to other commands.

[jessetrinity]

Oh right. So for this particular issue, OrganizeImportsRequestArgs.

[JoshuaKGoldberg]

Thanks! 3a61904 adds the option here. I've dabbled in VS Code before; it'd be fun to try to add it there too. 😄

[JoshuaKGoldberg]

Btw, I realized that 3a61904 (which added the allowDestructiveCodeActions argument) is a breaking change by nesting the scope part of the args. So in 7fd22f8 1637e1a I switched the change to be an extension, not a nesting.

[andrewbranch]

Thanks @JoshuaKGoldberg! Let’s get a review from @jessetrinity too.

[jessetrinity]

nit: a word

[jessetrinity]

Looks good. We should make a separate issue for adding a UserPreference for cases in which the command is explicitly requested.

[andrewbranch]

Why didn’t @typescript-bot issue the pinging-everyone-because-you-changed-protocol.ts comment?

[amcasey]

Seems like a breaking change to the protocol? Am I missing something?

[amcasey]

To confirm, we expect the editor to want to make this decision per-request and that's why it's not a UserPreference?

[sheetalkamat]

Also i would name this flag as skipDestructiveCodeActions so that you would need to check only for truthiness instead of defaults when not set..

[jessetrinity]

To confirm, we expect the editor to want to make this decision per-request and that's why it's not a UserPreference?

Yes, the editor would need to specify the option based on whether the command was sent as a part of compileOnSave or similar.

[JoshuaKGoldberg]

dacfd7e renames to skipDestructiveCodeActions and defaults it to falsy.

[amcasey]

Btw, I realized that 3a61904 (which added the allowDestructiveCodeActions argument) is a breaking change by nesting the scope part of the args. So in 7fd22f8 1637e1a I switched the change to be an extension, not a nesting.

I'm definitely missing something in this comment.

[amcasey]

I'd like to get a second opinion on the flag name, but LGTM otherwise.

[amcasey]

I don't think we have an actual convention, but it looks like we have a pattern of using positive names and, if necessary, treating undefined as true. e.g. UserPreferences.includeAutomaticOptionalChainCompletions or CompilerOptions.AllowUnreachableCode. I think @andrewbranch has also found this frustrating and might have suggestions?

[andrewbranch]

No, I don’t really have a preference here. This looks fine to me.