Start moving omnisharp to directly using Roslyn's completion service, with a separate resolve step #1877
Conversation
… with a separate resolve step
Today, Omnisharp uses Roslyn's completion service to gather completions, but doesn't then use the other completion helpers to fill in the rest of the detail of the completion. Additionally, Omnisharp does a bunch of work when creating and populating completions, and this can cause slow and inconsistent behavior when invoking completions in the middle of a pre-typed word. Some current issues:
* vscode fuzzy-matching changes based on whether you invoke completion after typing a few characters.
* Completions with large numbers of elements can take a bit of time to load, as Omnisharp does filtering and documentation building for all of them.
* Completion is not well setup to play nicely with LSP, which uses a much different interface.
* Completion is not capable of providing unimported types, particularly because providing the edits would be extremely expensive if not done only on request.
To solve these issues, this adds a new completion service that mirrors the structure of LSP completion and completion/resolve:
* On request, only the completion item and initial edit are provided.
* A new response service is added, which looks up documentation when a particular item is selected in the completion window.
* This response service will additionally be able to provide extra edits, such as inserting the rest of an override statement or adding imports for unimported types, but these are not yet implemented.
src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs
Outdated
Show resolved
Hide resolved
|
thank you so much! this makes me extremely happy 👏
yes that is a good idea
that was my fear too. is there some diffing service in roslyn that we can just use, or do you need to roll out something by hand?
IMHO this is also a bug, and overloads the server unnecessarily. But shouldn't this be rather fixed client side? e.g. at the moment VS Code would send |
…eto completion. LSP does not support a concept of asynchronous edits that touch the current cursor location: anything that does needs to advertise those edits up front. That makes it impossible to do override or partial completion lazily: completing the method body _requires_ touching the insertion text, as depending on editorconfig settings braces might need to be inserted at the end of the line. Further, moving the cursor requires using a snippet, which cannot be accomplished in an additionalTextEdit. So, for those 4 providers that need to compute additional work at the current cursor location, we do so. We take any work that comes before the cursor location (such as turning override into public override returntype) and turn that into an additionalTextEdit, and then snippitize the rest of the completion and use it as insertionText.
src/OmniSharp.Abstractions/Models/v1/Completion/CompletionItem.cs
Outdated
Show resolved
Hide resolved
|
Here's what override completion looks like:
I still need to adjust the documentation stuff. I can drastically simplify the implementation and clean up code in both the new quickinfo provider and the completion service around it with a couple of small changes, but for this iteration I was focused on override completion :D |
|
this is ⭐️⭐️⭐️! |
We do send the trigger character, and whether the completion was triggered explicitly or by typing. It's just that Roslyn thinks "This was triggered by typing |
This allows it to be fully shared by the QuickInfoProvider and the new CompletionService, with much less special casing and encapsulation violations. Additionally, the responses from the CompletionService now no longer specify the same lower-cased names as the the LSP completion endpoint. This brings the service into consistency with the rest of omnisharp, and it won't be very hard for the LSP provider to wrap it.
|
@filipw I believe this is now in a good state. |
thank you! I saw you made a corresponding VS Code PR too
|
yes it does 👍 |
|
Is it available in the current stable version? How do I complete using "override "? |
|
Yes, it's available. You just invoke completion and it should work. |
|
I am trying to use on vscode using C# extension and when I type "override+space" it shows nothing, is there any configuration for this? |
|
Did you trigger completion, such as by hitting ctrl+space after typing override? |
|
I just tried that with and without space after the override, did not work. latest stable omnisharp and vscode. |
|
Please open an issue and include your omnisharp log. |
Today, Omnisharp uses Roslyn's completion service to gather completions, but doesn't then use the other completion helpers to fill in the rest of the detail of the completion. Additionally, Omnisharp does a bunch of work when creating and populating completions, and this can cause slow and inconsistent behavior when invoking completions in the middle of a pre-typed word. Some current issues:
To solve these issues, this adds a new completion service that mirrors the structure of LSP completion and completion/resolve:
An example of what this looks like in vscode:
Open questions/things still todo:
additionalTextEditsto do the same thing here, though the restrictions around the current insertion point are concerning and may be a technical limitation to work out.detailsection of aCompletionItem) to show the unimported namespace, like VS. However, is there something else we want to put in there?overrideand possibly a few other scenarios. This is because the Roslyn APIs forShouldTriggerCompletionreturns true for space characters. Is this something that we actually want to have happen? It seems to me like this might be a bug, at which point we should likely ask Roslyn to fix the bug, not try to work around it ourselves.@filipw @david-driscoll @JoeRobich for a first look. I'm putting it in draft for now while I try and figure out the output diffing bit, but I wanted to at least get eyes on what I've implemented so far.
This supersedes #884.
Fixes #1809.
Fixes #1725.
I think this fixes #78?