@allisonchou @genlu for if this flag is correct to use in this case. @allisonchou you needed this for things like override completion. But did you not need it for import completion? If so, i'm curious why not.

I wasn't aware of import completion having a significant perf impact. If it does, I think it makes sense to mark it as a complex text edit. The primary downside is that it will block the UI thread.

The primary downside is that it will block the UI thread.

Can you expand on that @allisonchou that's a fairly big deal for me :-)

(please don't merge on this just yet. Thanks!)

I didn't think the flag was checked in non-lsp scenarios (and lsp doesn't do import completion today)?

Looking through our source, the only references to IsComplexTextEdit come from the CompletionHandler and the CompletionResolveHandler, and those turn off import completion: https://github.com/dotnet/roslyn/blob/main/src/Features/LanguageServer/Protocol/Handler/Completion/CompletionHandler.cs#L348-L359.

Ahh right, I forgot import completion is ignored. Then in that case I don't think this change affects the CompletionHandler at all, ignore my earlier comment.

Let me rephrase. First, if we enabled import completion for LSP (why is it off btw?), would we want this flag set on this or not? Thanks! :)

If we would want this flag, then i'm totally ok with this PR.

What's the intention of isComplexTextEdit? To mark items do more than inserting a string at current cursor location? And why do we need it?

Also note there are cases when import completion would insert fully qualified type name instead of add using (which is determined at commit time though).
http://sourceroslyn.io/#Microsoft.CodeAnalysis.Features/Completion/Providers/ImportCompletionProvider/AbstractImportCompletionProvider.cs,3bbcba1ee029cc87,references

Let me rephrase. First, if we enabled import completion for LSP (why is it off btw?), would we want this flag set on this or not?

If import completion has significant perf concerns (it sounds like from Fred it does), then I think we'd want the flag on. It would make the overall completion experience faster, but would make resolving individual items slower since resolving the item would require the UI thread.

(why is it off btw?)

I believe it's off due to parity issues, e.g. there's no way to display namespaces today on the right-hand side of the list. More details are in https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1076759.

What's the intention of isComplexTextEdit? To mark items do more than inserting a string at current cursor location? And why do we need it?

isComplexTextEdit is used in VS LSP to indicate if computing the text changes for a completion item may have significant perf concerns. If isComplexTextEdit is set to true, then we delay computing the text changes until the user selects the item in the list. This way we can pop up the completion list faster. The downside is resolving the item later requires the UI thread, hence we don't want to do this for most items if we can help it.

The downside is resolving the item later requires the UI thread,

Why does it require the UI thread?

I believe it's off due to parity issues, e.g. there's no way to display namespaces today on the right-hand side of the list. More details are in https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1076759.

That, and perf concerns with huge completion list size and serialization. However this may have been mitigated by Taylor's recent changes with optimized completion lists and less resolve data.

So I think once the work item to show the detail text is resolved we should try turning on import completion for LSP and evaluate the perf.

OK, if LSP needs to compute text change from ALL items up front when creating completion list, then we absolutely need to mark unimported items as isComplexTextEdit.

OK, if LSP needs to compute text change from ALL items up front when creating completion list, then we absolutely need to mark unimported items as isComplexTextEdit.

Some certainly do (such as vscode).

@CyrusNajmabadi any more concerns with this change?

Nope! LGTM. Just a request for LSP folks to work on integrating this functionality.