CompletionItem commands no longer work if added during resolve (regression)

[DanTup]

This code produces a completion item and adds a command during resolve (which just shows an alert).

In the latest VS Code, this command does not fire when the completion item is selected, unless it is attached in the original provideCompletionItems call.

import * as vscode from 'vscode';

export function activate(context: vscode.ExtensionContext) {
	context.subscriptions.push(vscode.commands.registerCommand('vscode-repro-completion-commands.helloWorld', () => {
		vscode.window.showInformationMessage('Hello World from vscode-repro-completion-commands!');
	}));

	context.subscriptions.push(vscode.languages.registerCompletionItemProvider(
		{ scheme: 'file' },
		{
			provideCompletionItems(doc, pos, token) {
				const completionItem = new vscode.CompletionItem("ProcessResult");
				// Attaching the command here works:
				// completionItem.command = {
				// 	command: "vscode-repro-completion-commands.helloWorld",
				// 	title: "add imports",
				// };

				return new vscode.CompletionList(
					[completionItem],
					false,
				);
			},

			resolveCompletionItem(oldItem) {
				const completionItem = new vscode.CompletionItem("ProcessResult");
				// Attaching the command here does not work.
				completionItem.command = {
					command: "vscode-repro-completion-commands.helloWorld",
					title: "add imports",
				};

				return completionItem;
			}
		},
	));
}

[DanTup]

Note: If I mutate oldItem in resolve instead and attach command, then it works (but in my real project the LSP client is handling this so I can't make that change).

[jrieken]

We made the API implementation more strict and adhere to its documentation. During resolving no property that affects the insert behaviour should be changed. That's because there is no guarantee that resolve is called or awaited before inserting the completion. For the command specifically it will only be invoked when available at insert time and it will be ignored when resolve finishes after that.

@DanTup Is setting the command during provide an option for you? I would assume inserting the command id is a simple/lightweight operation that doesn't need to do anything. The heavy lifting can be done during the actual execution of the command

[DanTup]

For the command specifically it will only be invoked when available at insert time and it will be ignored when resolve finishes after that.

Not sure I undestand. Resolve is fully completing before I select the item.

FWIW, looking at my logs I think it broke in Insiders between May 24th and 26th.

Is setting the command during provide an option for you?

I'm not sure - I'm trying to delay as much computation to resolve because there can be huge numbers of results. I'd have to do some digging to see for sure - but I'm not sure I understand why this change. I'm happy for the command to not work if the resolve hasn't completed - but I don't see any benefit to not allowing it to be added during resolve at all.

Edit: Also, adding commands+args to thousands of items will increase the payload size :(

[jrieken]

Resolve is fully completing before I select the item.

There is no guarantee for this, if you select quickly, if your EH process is busy with something else then resolve won't be called or won't finish. In the suggest controller, inserting an item does not await the resolve-call before making the edit and before running the command. I know these delays are unlikely to happen on our beefy dev machines that run a single extension but the real life is different.

but I'm not sure I understand why this change

It's meant to reduce flakyness. Image a user that only gets the command to execute on every other completion. They cannot understand why that is.

[jrieken]

Just to explain why we cannot await resolve

• user triggers suggestion which is async
• after some time suggestion shows, resolve happens as items are focused
• user clicks on the 3rd item
• item is being inserted with resolve for that item not yet called
• user expects the suggestion to appear immediately, user want to continue typing immediately.

If we would await the resolve call users would be confused because nothing happens in the editor, also users can always continue to type making it very hard and confusing when the suggestion is actually inserted at a later point.

[DanTup]

There is no guarantee for this, if you select quickly,

Yep, I understand this - and this was already the case (and documented) for additionalTextEdits:

This function may fill in additionalTextEdits. However, that means an item might be inserted before resolving is done and in that case the editor will do a best effort to still apply those additional text edits.

That's absolutely fine, but I don't understand why command would be more restricted than additionalTextEdits - to me they're essentially the same thing (except additionalTextEdits doesn't support inserting into other files, which is why we have to use commands).

It's meant to reduce flakyness. Image a user that only gets the command to execute on every other completion. They cannot understand why that is.

But this is also the case for additionalTextEdits? As an extension author, I would rather have this flakiness (which no user has ever reported) than it being broken for 100% of users.

resolve seemed designed for things that are expensive and is documented as if it supports what we were using. Having to drop resolve and do more things up-front for potentially thousands of items feels like a step backwards (as well as a big breaking change).

If we would await the resolve call users would be confused because nothing happens in the editor

I'm definitely not asking for this. I'm asking for the previous behaviour, the same as is documented against resolve. If you have resolved in time, it works. If you have not, it does not work. It was always a best effort, and I think it's necessary given users expect every symbol (imported or not) to be in code completion. We can't really send megabytes of JSON to put commands and arguments on every item :-(

[jrieken]

Also see #183092 for more context. We have relaxed the rules for additionalTextEdits during the last 1.79 insider period (we track violating extension to come up with a plan) but we didn't expect this to cause issues with commands - because adding them is considered cheap

[DanTup]

we didn't expect this to cause issues with commands - because adding them is considered cheap

For me, the command is just the same as additionalTextEdits, because we're using it to add imports for "other files". Because of how the server currently works (which is something that could change, but would be much work and not ship for another SDK release) we just round-trip the edit inside the command. So computing the command arguments is the same as computing the edits.

Again - I really don't understand why it's acceptable for additionalTextEdits but not commands. If extension authors want (and can) to provide the command up-front, they could. Nothing is gained for them by changing this - but functionality is lost for those that can't easily do this (or that were just caught out and can't ship a fix quickly).

I feel like this change is going to result in implementing a parallel resolve via commands. We'll end up duplicating the data we need to locate a symbol that is in the data round-tripped for resolve in the command.arguments.

[DanTup]

Btw, it also seems odd that mutating the provided completion item during resolve works. I could patch that into the LSP middleware to "fix" my issue for now, but presumable it's a bug.

I do hope you'll reconsider this though (or at least give language servers/SDKs time to update). I feel like the issue it causes is far worse then the problem it's solving (like I said, I've had zero complaints about it not working, because realistically it's probably incredibly rare), and the refactor to work differently isn't trivial.

[jrieken]

Btw, it also seems odd that mutating the provided completion item during resolve works

Yeah, that's an oversight.

Again - I really don't understand why it's acceptable for additionalTextEdits but not commands.

That's because the command will not be executed after resolve but additionalTextEdits are attempted to be inserted. So, there is a guarantee for flakiness if you set command during resolve. Also we assume that setting the command during provide is cheap and easy. Please let me know why that wouldn't be the case

[DanTup]

That's because the command will not be executed after resolve but additionalTextEdits are attempted to be inserted. So, there is a guarantee for flakiness if you set command during resolve.

I'm not sure I understand the difference. Why wouldn't additionalTextEdits also be flaky?

Also we assume that setting the command during provide is cheap and easy. Please let me know why that wouldn't be the case

There are two parts to this.. One is that the command args are (currently) expensive to compute because they contain the edits. To our server, there is no distinction between edits for the same file or another file, so they are computed exactly the same (during resolve) and because of the API, we have to split them into those for the current file (to go into additionalTextEdits) and others (which we tag into command args).

To handle some edits in resolve and some in the command would involve doing the same work twice, and discarding some of the edits in each step.

Here's an example - our command just gives the edits back to the server, because we don't want to track state on the server and we don't want to re-compute something we just computed during resolve anyway:

"command": {
	"arguments": [
		{
			"edit": {
				"documentChanges": [
					{
						"edits": [
							{
								"newText": "import 'dart:io';\n\n",
								"range": {
								"end": { "character": 0, "line": 0 },
								"start": { "character": 0, "line": 0 }
								}
							}
						],
						"textDocument": {
							"uri": "file:///C:/Dev/Test%20Projects/test_failure_import_part/lib/part_wrapper.dart",
							"version": 1
						}
					}
				]
			}
		}
	],
	"command": "edit.sendWorkspaceEdit",
	"title": "Add import"
},

But there's a second part to this - payload size. The completion payload can already be massive because users expect every symbol (imported or not) to be listed in completion. VS Code taking over ranking during typing makes it difficult to use isIncomplete so we tend to send a larger list rather than smaller. So we try to move as much into resolve as we can (which appeared to be the recommendation until now). Duplicating the command and either the edits in arguments, or everything that was in data (to be used in resolve) so we can re-compute edits will make the payload much larger again.

I'm still not sure I understand the motivation for this... Were you getting bug reports about command not working if people typed and accepted fast? It feels like something is being taken away without any (obvious) advantage being given. Extensions/servers could already provide the commands up-front before, so if you want to avoid flakiness you can just provide recommendations and blame extensions if their completion is flaky (let's face it, we can write flaky completion regardless of the API :-))

All of the alternatives to what we currently have seem to have downsides (larger payloads, duplicated/wasted computation) and code completion no longer lives in many extensions, but in language servers that may be in SDKs that are on much slower release cycles than extensions.

[DanTup]

That's because the command will not be executed after resolve but additionalTextEdits are attempted to be inserted. So, there is a guarantee for flakiness if you set command during resolve.

I just realised what you meant here. When resolve completes, you will attempt to apply those edits, even if the insertion has already occurred.

However, the word attempt here makes me think this is not guaranteed to work, so it feels like this is just a different kind of flakiness, rather than no flakiness vs flakiness (and I'm still not certain why one is allowed and one is disallowed).

[jrieken]

One is that the command args are (currently) expensive to compute because they contain the edits.

I understand but the idea is to have a command that kicks of that computation, not a command that deals with the result. So, instead of attaching the "Add Import/edit.sendWorkspaceEdit"-command you can attach a "Post Insertion"-command that checks for import-edits and applies them. Think of the command as a post insertion that allows you do more work. This is how the built-in TypeScript does it

But there's a second part to this - payload size. The completion payload can already be massive because users expect every symbol

Usually the API implementation doesn't send command arguments back and forth. It creates an indirection command and the actual arguments stay inside the EH. Also a "Post Insertion" command might be relatively lightweight wrt arguments because it only needs to know the item it refers to.

Go forward, I see the following options

1. Allow to change/set the command during resolve
2. Dart utilises the loophole that still allows the command to be set
3. Dart sets the command during provide

Given option 2 (and 3) and given this is the only issue in the area it's unlikely that we do a recovery release for this

[DanTup]

I understand but the idea is to have a command that kicks of that computation, not a command that deals with the result.

Doing this would end up duplicating all the info in the LSP data field (which is used for resolve) into the command.arguments, which would add a lot of (duplicated) bloat to the payload. It feels like this is just implementing "another copy of resolve" which feels odd.

The reason we were attaching the edit as we do, is that we compute edits once regardless of whether they're from the same or other files, but VS Code/LSP requires we split them up. If there was something like additionalTextEdits that could take a WorkspaceEdit (that we could add during resolve), that would be much neater, but I don't know if it's something you'd consider?

Usually the API implementation doesn't send command arguments back and forth. It creates an indirection command and the actual arguments stay inside the EH

Sure, but when using an LSP server there's EH<->LSP Server streams too, so we'd still be serialising/sending/deserializing a lot of extra data here. I've tried hard to minimize this initial payload (leaving as much as possible to resolve) but this feels like it's going in the opposite direction.

Go forward, I see the following options

1. Allow to change/set the command during resolve
2. Dart utilises the loophole that still allows the command to be set
3. Dart sets the command during provide

Ofcourse, I think 1 is the best option :) I'm less bothered about a recovery release but more about the future generally. I can consider option 2 short-term (assuming I can do it via LSP middleware), but option 3 doesn't feel like a good solution to me (it feels like the drawbacks outweigh the benefit VS Code got from this change).

Would you consider option 1, an additional WorkspaceEdit that could be provided by resolve, or not "fixing" the loophole (although if going to leave the loophole, I feel like just supporting it better would be better).

Thanks!

[genlu]

This change breaks C# extension too (both the old OmniSharp-based server and new Roslyn server.) As we rely on resolving the command to handle items with slow-to-calculate edits. We have already presented our case in detail in this issue from two years ago. FYI @dibarbet

[jrieken]

As we rely on resolving the command to handle items with slow-to-calculate edits

@genlu Similar questions

• why don't you set the command during provide
• why don't you use additional text edits for those edits

---

Also FYI that we have added telemetry to know often an item is inserted without being resolved. The data isn't representative (because it's insiders only) but so far it seems that ~5% of all accepted items aren't resolved¹ and that this happens to ~10% of distinct machines.

¹the actual number might be higher because we don't count yet if an item needed resolving or not. For now 95% count as resolved irrespectively if they need resolving or not.

---

I will keep you posted as more data comes in. That will take a while because it needs stable bits to deliver these data points.

I haven't discard the idea of allowing to lazily resolve the command and to lazily execute it. However it will mean an extension can make no more assumptions about when the command runs. Today the order is (1) additional edits, (2) main edit, (3) command. Additional edits can be late/lazy but the editor with all its information does the apply attempt. And extension running a command will have a much harder time. By the time resolve is finished and by the time the command is being executed many things might have changed, like the inserted suggestion might been deleted again or the user is now typing in a different file etc pp

[DanTup]

I haven't discard the idea of allowing to lazily resolve the command and to lazily execute it.

FWIW, this was not specifically what I was asking for. I am completely happy for commands to not fire if resolve doesn't complete by the time the user chooses a completion (although, I also thought that is what additionalTextEdits did). The command is there to provide some convenience (automatically adding an import) and is only required for some edge cases (where imports aren't in the current file - those are handled by additionalTextEdits - because it's a "part" file).

This is what I believed the old behaviour was, and nobody had ever complained. If you select an completion quickly and were in a part file, the import might not be added. You can just use the quick-fix to add it, or delete it and re-select it less quickly. This was absolutely better than it flat out not working, and it's better than us having to do more expensive work for every (potentially thousands) of completion items to try and set the command up in advance.

By the time resolve is finished and by the time the command is being executed many things might have changed, like the inserted suggestion might been deleted again

I'm not sure I understand how this happens. If today you're waiting for resolve to complete so you can insert additionalTextEdits first, that suggests resolve must always have completed prior to the item being inserted (in which case the command would run immediately after the insert, same as it always does?).

[jrieken]

If today you're waiting for resolve to complete so you can insert additionalTextEdits first, that suggests resolve must always have completed prior to the item being inserted (in which case the command would run immediately after the insert, same as it always does?).

Let me clarify that: we do not and cannot wait for resolve to complete before making the main edit. When additional edits aren't resolved we attempt to apply them after resolve is done. Doing that is tricky, can surely be improved, but the editor with all its information is always better equipped for that job than a "context-free" command.

[DanTup]

Ah, gotcha - that's more like what I thought was happening.

What's the concern with supporting the command and having the caveat that commands will only run if the item was resolved prior to being selected? I understand that based on your numbers this might be 5% of completions and it depends on machine speed, but I feel like an extension author should decide on the trade-offs there (and they're already going to be taking any bug reports for things not working). If this flakiness is not acceptable to an extension, it can choose to compute up-front and avoid it.

In my opinion, it's absolutely not worth increasing the payload size and the up-front computation for this. resolve was specifically there to try and offload things that are not required to be shown in the main list and can be delayed such that they're only done for completion items that might actually be completed. We pushed as much as we can back into resolve (on the understanding it might never complete before selection) and moving it back inline feels like a step backwards.

Out of interest (a tangent, but related) - are there any plans to support streaming completions? LSP already seems to support this (partial results), but VS Code doesn't. I feel like that would solve this issue and some others.. the server could return all the in-scope results immediately (those that are imported, don't need extra edits, and are fast to find), and then provide a list of unimported symbols (which may take longer to compile, need extra edits etc.) as a second batch. Right now for large workspaces we have to make a decision whether to exit the completion request early if it takes too long (giving the user some results, but they might be incomplete), or keep listing items from the entire workspace until done (delaying showing anything to the user, when the item they want might have already been computed).

[genlu]

why don't you set the command during provide
why don't you use additional text edits for those edits

Both of these questions were answered in detail here:
https://github.com/microsoft/ms-lsp/issues/60#issuecomment-773495463

I'm copying the comment from @333fred here since that repo is now archived as private and many might not have access to it:

====================================================

Omnisharp is currently looking to add a command handler to make calculation of the expensive parts of a completion async and only executed for a single item: OmniSharp/omnisharp-roslyn#1986 (comment). To copy/paste my before and after images from that thread:

Sync Completion (the experience today, where textEdit has to be fully calculated up front):

Async Completion proof-of-concept (where textEdit is just a shortened bit of label, and the full edit is calculated later as part of the command callback):

The issue driving us towards a command-based (and client aware) solution is that the expensive part of the edit calculation intersects the current cursor location, and includes moving the cursor to a new location. For a single override/partial item, the time necessary to calculate this is negligible. However, C# is an OO language, and there are absolutely realistic cases where there are hundreds of possible overrides. To give on such example, Roslyn's compiler codebase makes heavy use of the visitor pattern for our AST. Every node will have a VisitX method on the base visitor type, and there are 203 concrete (ie, leaf) node types in our AST today. That's 203 VisitX methods, and when you type override and invoke completion in vscode today in a new visitor, that means 203+a few others (like object.Equals and DefaultVisit) edits have to be computed before the completion list can be shown.

I have tried to think about other ways to make this experience better by using a server-based command that sends a workspaceEdit to the client, but that cannot control the cursor location, which makes the experience annoying for the user. It also doesn't work well with other types of postfix completion rewrites, such as generating switch expressions (with snippet locations for moving the cursor between a pattern and result locations).

==============================

[jrieken]

@genlu Thanks for the response and the details. If I am not mistaken you actually want to modify insertText (or LSP speech textEdit.newText) during resolve and using command is just a workaround for that. See #183092 for the related discussion. Modifying the insertText during resolve is actually possible but we don't know yet if that's a good idea or not. Tho, I tend to think that it's better than using a command.

Both of these questions were answered in detail here: https://github.com/microsoft/ms-lsp/issues/60#issuecomment-773495463

It doesn't answer why the command isn't set during provide (instead of resolve). That would ensure the command is invoked in 100% of the accepts and (when done correctly) is very cheap to do.

---

@genlu general GH tip: you don't need to attach screen captures of GH comments. You can share the links of each comment by copying from the timestamp

[genlu]

If I am not mistaken you actually want to modify insertText (or LSP speech textEdit.newText) during resolve and using command is just a workaround for that.

It's not enough for us to merely change textEdit.newText in item resolution. Please see @333fred's explanation using completion of a method override item in the screenshot I pasted above.

I noticed this comment from the discussion. Allowing additionalTextEdit to overlap with textEdit would be really helpful. Although the fact that we can't adjust cursor location with the change (and insertTextFormat doesn't apply to it) means we'd still need to use a command to fully support the feature. FYI @akhera99 since this will be problematic for semantic snippets as well if we need to delay resolving them.

It doesn't answer why the command isn't set during provide (instead of resolve)

That'd be our last resort if nothing else panned out. Because our main challenge is computing the actual edits upfront for all those items is prohibitively slow, we basically have to implement an equivalent of completion resolve command ourselves, yet still facing the same (hypothetically) flaky issue. it's definitely doable, but having a platform supported mechanism to do so is still preferrable given the fact that many extension authors are facing the same problem: it's one thing to not having this support to begin with, it's another if you take it away after years of supporting, in the name of reducing flakiness, but then suggest an alternative with the same flakiness issue :)

general GH tip: you don't need to attach screen captures of GH comments. You can share the links of each comment by copying from the timestamp

I appreciate the tips. I would have done so if the comment wasn't made in a private repo. Since this is in the open repo, I think it's necessary for people outside of MSFT to be able to follow our discussion along.

[333fred]

It's not enough for us to merely change textEdit.newText in item resolution. Please see @333fred's explanation using completion of a method override item in the screenshot I pasted above.

I actually think that, if we could change the text edit during resolve, it should be doable (we may want to change more than just the text, for example changing whether the edit is a template or what the range of the edit is). In other words, we're using it as a workaround for lazily resolving the entire textEdit, not just the text.

[jrieken]

@333fred changing the range (whole textEdit) is off limits but nothing prevents you to use a "good" replace range to begin with. In above's sample your initial edit must not be "insert 'Equals'" but it can very well be "replace 'override' with 'override Equals' and the later, async edit can then be "replace 'override'" with "public override bool Equals(....)"

[jrieken]

Going forward

1. We wait for telemetry to representative.
2. We remove the loophole that @DanTup found BUT we again allow to set the command during resolve
3. We report setting the command during resolve as deprecated/bad API usage and will follow up the respective extensions

Sneak preview of the telemetry data: For the insiders cohort we see that 5% of suggestions are inserted without being resolved and that this happens for 25% of all sessions (users). So rarely but to very many users. For stable I expect this to be worst.

Note that in those cases the command will never be executed (contrary to additional edits). Please seriously reconsider to set a lightweight command during provide because that will make your extension's behaviour much less flaky.

Also, monitor #183092 which will track considering allowing the primary edit to be changed during resolve (API lingo insertText, LSP lingo textEdit.newText). That and/or (improved) additional edits are multiple ways better for making textual insertions.

@DanTup You seem to be doing this correctly in most cases 🏅 but seem to have a case in which accepting a suggestion needs to change another file. Sounds a bit esoteric but can you please file a separate, dedicated issue for that.

[jrieken]

PS: I don't want to discuss waiting for resolve so that we execute the command later. That's what we do for additional edits but doing this correctly for commands (which run out of context) is really hard. We never did that and I don't want to open the door for that.

[DanTup]

2. We remove the loophole that @DanTup found BUT we again allow to set the command during resolve

Do you mean you will do this now, or wait for telemetry? I hadn't started using the loophole, so I can avoid making any changes if this is being reverted soonish.

Thank you, I appreciate having this back (even if it may not be long-term). I can investigate what's involved in using a command, but I have concerns over payload size (and whether I can do it without more up-front computation). Could we:

1. provide some notice if commands will be blocked from resolve again (Dart SDKs tend to release approximately quarterly, so shipping a fix could take time - but I don't want to start this until we're certain it's the way)
2. discuss whether edits for another file could be supported in a different way (that can be attached during resolve) before changing anything (so if support is added, we can migrate straight to that)?

but seem to have a case in which accepting a suggestion needs to change another file. Sounds a bit esoteric but can you please file a separate, dedicated issue for that.

I'll open an issue, but just so it's captured here, the reason for this is that in Dart you can have libraries that are made up multiple ("part") files and all of the imports go on the root file. When you're accepting completion of an unimported symbol in a part file, the import needs to be added to the root file for that library. I don't know how common these kinds of files are (some users use them heavily, but I think most do not).

PS: I don't want to discuss waiting for resolve so that we execute the command later.

That's definitely fine by me - I'd have concerns around that too, and I always expected this to only run if resolve had already completed. The previous caveat (if you accept a completion item quickly AND it was not already imported AND you were in a part file, then the import might not be added automatically) was something I could live with and handle reports about (although I've never had a single report about it). Ofc, having something reliable if it doesn't compromise performance (due to computation of payload size) would be better.

Thanks!

Edit: Filed #185772

[jrieken]

Do you mean you will do this now, or wait for telemetry? I hadn't started using the loophole, so I can avoid making any changes if this is being reverted soonish.

It's back to what is was before. So, you can change/set the command during resolve again but you will be blamed for it.

[333fred]

but nothing prevents you to use a "good" replace range to begin with.

My main concern with this solution is that it means that we would need to aggressively expand the range of the edit to be as large as it possibly can be. Doable, but complicated and potentially fragile if we ever add a new completion that needs more in front (ie, potentially intersecting the edit).

[genlu]

Thank you @jrieken.

Sneak preview of the telemetry data: For the insiders cohort we see that 5% of suggestions are inserted without being resolved and that this happens for 25% of all sessions (users). So rarely but to very many users. For stable I expect this to be worst.

Given that an item will always be resolved for additionalTextEdit, I wonder if it'd make sense to get extension specific numbers on how often an item is inserted w/o being resolved that actually set the command during resolve. My hypothesis here is that, different kinds of items are provided based on code context, and users might examine the completion list and choose an item more deliberately when it comes to those items need command resolution (in the case of C#, choosing which method to override, and in @DanTup's case, one that would cause changes in other files,) compared to simply typing a type/variable name, maybe that extra 10-20ms is all it needs for the item to be resolves on successfully before insertion.

That and/or (improved) additional edits are multiple ways better for making textual insertions.

Does this mean the option of allowing additional edits to overlap with main edit is on the table? :)