Use CompletionService in IntellisenseService #840

filipw · 2017-04-30T12:18:34Z

At the moment, the CompletionService is only used for keywords, everything else is coming from Recommender.

This PR introduces relying on CompletionService for all completions.
The symbols are still processed the same way as before, so none of the existing features are broken, but extra features available via CompletionService also show up in intellisense now (as we process all of its providers).
This will close a lot of open bugs via a relatively risk-free change.

I will push some tests into this soon but here are some of the examples of new features that are introduced via this PR:

Attribute completion without `Attribute`

Cref completion

Object initializer members completion

Named parameters completion

Override support

Note: because of this Regex in omnisharp-vscode, where brackets are removed, it appears a little broken. This change would need to be done in omnisharp-vscode.

DustinCampbell

Nice! Exciting change!

I never opened up the floodgates with CompletionService here because it's not really good enough for completions with multiple text edits (like override completion), or completions that need the commit character in order to complete properly. Personally, I'd like us to figure out override completion a little better before we merge this (to get closer to user expectations), but this should go along ways toward relieving some pain.

DustinCampbell · 2017-04-30T13:42:10Z

src/OmniSharp.Roslyn.CSharp/Services/Intellisense/IntellisenseService.cs

+                            // as the display text.
+                            var response = new AutoCompleteResponse()
+                            {
+                                CompletionText = item.DisplayText,


This probably isn't good enough for override completion. Completions on override will just be wrong until we can properly support CompletionService because the completion is multi-line, moves the caret, and may add usings above.

Completion on partial will also be broken.

FYI that this is handled by calling when before an item is committed. http://source.roslyn.io/#Microsoft.CodeAnalysis.Features/Completion/CompletionService.cs,bfe3e3a9f66443fb.

To support completion properly we need a new api similar to what LSP provides with additionalTextEdits?

Right, but we'll want to be more of a "resolve" API, so we don't have to go computing this for every completion item.

actually overrides and partials won't even reach that specific line because they'd branch out here since these providers encode symbols. So the formatting/presentation of i.e. symbol information will be consistent with the rest of the experience, but yeah ultimately the DisplayText will be used for completion.

From what I experimented, even the simple DisplayText at the moment isn't that bad. It actually, in case of overrides, has the following format https://github.com/dotnet/roslyn/blob/614299ff83da9959fa07131c6d0ffbc58873b6ae/src/Features/Core/Portable/Completion/Providers/AbstractOverrideCompletionProvider.ItemGetter.cs#L21. So you get to see the full signature, and that's what can be inserted into the code even using the current object/endpoint model we have (of course things like missing usings would have to be resolved manually separately by the user)

It's definitely sub par experience compared to VS but at the same time maybe it's better than not having anything?
But of course, we could also just disable the complex providers like override/partial from participating in the old intellisense endpoint and only add them once we have a new one

DustinCampbell · 2017-04-30T13:53:45Z

src/OmniSharp.Roslyn.CSharp/Services/Intellisense/CompletionItemExtensions.cs

+
+        public static async Task<IEnumerable<ISymbol>> GetCompletionSymbols(this CompletionItem completionItem, IEnumerable<ISymbol> recommendedSymbols, Document document)
+        {
+            // for SymbolCompletionProvider, use the logic of extracting information from recommended symbols


Why? Isn't call GetSymbols(...) good enough below for all cases?

I kind of hate that we pre-compute recommendedSymbols (which is essentially like calling the CompletionService twice) just to handle this special case.

unfortunately since this change dotnet/roslyn#15688, the SymbolCompletionProvider doesn't store encoded symbols in the properties of the completion item anymore, so just calling GetSymbols() results in an empty array. Instead, the only thing I could think of was that they are fetched using Recommender. I am sure you'd know more background behind this change and if there is a better way to do it?

Ah yes, I'd forgotten about that. Computing all symbol IDs was simply too expensive.

DustinCampbell · 2017-04-30T14:03:56Z

src/OmniSharp.Roslyn.CSharp/Services/Intellisense/CompletionItemExtensions.cs

+        public static async Task<IEnumerable<ISymbol>> GetCompletionSymbols(this CompletionItem completionItem, IEnumerable<ISymbol> recommendedSymbols, Document document)
+        {
+            // for SymbolCompletionProvider, use the logic of extracting information from recommended symbols
+            if (completionItem.Properties.ContainsKey("Provider") && completionItem.Properties["Provider"] == "Microsoft.CodeAnalysis.CSharp.Completion.Providers.SymbolCompletionProvider")


It might be good to throw an exception if the type Microsoft.CodeAnalysis.CSharp.Completion.Providers.SymbolCompletionProvider doesn't exist. That way, we aren't searching around for the reason why completion starts failing if this internal type's name changes.

DustinCampbell · 2017-04-30T14:04:59Z

src/OmniSharp.Roslyn.CSharp/Services/Intellisense/CompletionItemExtensions.cs

+        static CompletionItemExtensions()
+        {
+            var symbolCompletionItemType = typeof(CompletionItem).GetTypeInfo().Assembly.GetType("Microsoft.CodeAnalysis.Completion.Providers.SymbolCompletionItem");
+            _getSymbolsAsync = symbolCompletionItemType.GetMethod("GetSymbolsAsync", BindingFlags.Public | BindingFlags.Static);


We've got some extension methods for this sort of thing (used by MetadataHelper) that throw exceptions nicely when they fail. I'd recommend using those to ensure we can easily spot failures if internal API names change.

yes good point 👍

filipw · 2017-04-30T15:03:39Z

thanks, I agree that in order to properly benefit from all the power of CompletionService we'll need a brand new endpoint.
What I tried to do here was to improve the current behavior as much as possible, with relatively small change to the current way the server operates, to solve a few annoyances 😃

david-driscoll

Since there are complications with partials and overrides, perhaps we can hide those behind a flag that must be turned on in omnisharp.json?

It's a little more work to remove them from the output, however that lets us keep a nice default for user expectations, but also offer something a little more bleeding edge.

david-driscoll · 2017-04-30T15:09:33Z

src/OmniSharp.Roslyn.CSharp/Services/Intellisense/IntellisenseService.cs

+                            // as the display text.
+                            var response = new AutoCompleteResponse()
+                            {
+                                CompletionText = item.DisplayText,


To support completion properly we need a new api similar to what LSP provides with additionalTextEdits?

DustinCampbell · 2017-05-01T23:06:57Z

The failures on *Nix look real.

use async suffix on GetCompletionSymbolsAsync move semantic model

filipw · 2017-05-02T12:32:37Z

The failures on *Nix look real.

Yes it was actually caused by this https://github.com/dotnet/corefx/issues/15825#issuecomment-277722247 "bug" ("behavior"?) of CoreFX - default US culture-based sort behavior is different on Windows and *nix. It should be fine now.

DustinCampbell · 2017-05-02T14:35:41Z

src/OmniSharp.Roslyn.CSharp/Services/Intellisense/CompletionItemExtensions.cs

+            if (completionItem.Properties.ContainsKey("Provider") && completionItem.Properties["Provider"] == "Microsoft.CodeAnalysis.CSharp.Completion.Providers.SymbolCompletionProvider")
+            {
+                var symbols = recommendedSymbols.Where(x => x.Name == completionItem.Properties["SymbolName"] && (int)x.Kind == int.Parse(completionItem.Properties["SymbolKind"])).Distinct();
+                return symbols ?? Enumerable.Empty<ISymbol>();


I don't think Where(...) or Distinct(...) will ever return null.

you are right 😊

DustinCampbell · 2017-05-03T16:29:44Z

Note: because of this Regex in omnisharp-vscode, where brackets are removed, it appears a little broken. This change would need to be done in omnisharp-vscode.

FWIW, I've never noticed that regex. It existed before I was on the project, so it's presence is a mystery.

DustinCampbell

Let's do this. We might want to revisit the experience around completion items that insert methods (e.g. override, partial), but this is definitely an improvement!

david-driscoll

DustinCampbell · 2017-05-03T17:30:32Z

All right. Merging... 😄

filipw · 2017-05-03T17:39:22Z

awesome! 🐣☀️

lukepuplett · 2021-06-15T18:54:17Z

Hello - sorry to bother but a cluster of issues all point to this. I don't get intellisense for C# object initializers in VSCode and it seems this has been a feature for years, right? I'm fully up to date, I think; I don't know how to check the version of OmniSharp.

My question is: where do I go for help on this?

darkguy2008 · 2021-07-19T02:18:07Z

I have to +1 @lukepuplett as it seems I'm experiencing the same thing. Trying to get intellisense for class properties in its initializer is sending me straight to the Process namespace (halfway through the full .NET namespace list) instead of sending me to the top of the list (where the properties are).

I opened a bug report in VSCode here but maybe it's related to this instead? microsoft/vscode#128963

filipw requested review from DustinCampbell and david-driscoll April 30, 2017 12:19

DustinCampbell reviewed Apr 30, 2017

View reviewed changes

david-driscoll reviewed Apr 30, 2017

View reviewed changes

filipw added 8 commits May 2, 2017 08:37

first stab at improving IntellisenseService

dbdc713

remved IntellisenseV2Service

8f233f0

clean up of commented code, comments and logic

73c78c4

extracted CompletionItemExtensions

e82f2e8

cache GetSymbolsAsync

5a34a46

use OmniSharp reflection extensions

d8e066a

use async suffix on GetCompletionSymbolsAsync move semantic model

added attribute completion and object initializer completion tests

ab55def

added more tests to IntellisenseFacts

b179067

filipw force-pushed the feature/completion-service branch from a6b9794 to 70c6c11 Compare May 2, 2017 06:37

filipw closed this May 2, 2017

filipw reopened this May 2, 2017

filipw force-pushed the feature/completion-service branch from a1507e6 to 5c8d067 Compare May 2, 2017 12:08

DustinCampbell mentioned this pull request May 2, 2017

Argument name suggestions in methods/ctors dotnet/vscode-csharp#1254

Closed

filipw force-pushed the feature/completion-service branch 2 times, most recently from 96b3033 to 5643a01 Compare May 2, 2017 13:23

better completion sorting, and consistent across Windows and Nix

5643a01

filipw mentioned this pull request May 2, 2017

give intellisense for accessible object properties on object initialization dotnet/vscode-csharp#260

Closed

DustinCampbell reviewed May 2, 2017

View reviewed changes

no need to check for null after Where

62fe53a

This was referenced May 2, 2017

CSX - Xml docs of types and members are displaying the "<see ...>" tag unparsed #698

Open

Fix attribute name #614

Closed

DustinCampbell approved these changes May 3, 2017

View reviewed changes

david-driscoll approved these changes May 3, 2017

View reviewed changes

DustinCampbell merged commit 9a811e7 into OmniSharp:dev May 3, 2017

filipw deleted the feature/completion-service branch May 3, 2017 21:14

filipw mentioned this pull request May 4, 2017

[WIP] csx build script spike Particular/NServiceBus.RabbitMQ#232

Closed

david-driscoll mentioned this pull request May 13, 2017

Better code completions #7

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use CompletionService in IntellisenseService #840

Use CompletionService in IntellisenseService #840

filipw commented Apr 30, 2017

DustinCampbell left a comment

DustinCampbell Apr 30, 2017

DustinCampbell Apr 30, 2017

DustinCampbell Apr 30, 2017

david-driscoll Apr 30, 2017

DustinCampbell Apr 30, 2017

filipw Apr 30, 2017

DustinCampbell Apr 30, 2017

DustinCampbell Apr 30, 2017

filipw Apr 30, 2017

DustinCampbell Apr 30, 2017

DustinCampbell Apr 30, 2017

DustinCampbell Apr 30, 2017

filipw Apr 30, 2017

filipw commented Apr 30, 2017

david-driscoll left a comment •

edited

Loading

david-driscoll Apr 30, 2017

DustinCampbell commented May 1, 2017

filipw commented May 2, 2017

DustinCampbell May 2, 2017

filipw May 2, 2017

DustinCampbell commented May 3, 2017

DustinCampbell left a comment

david-driscoll left a comment

DustinCampbell commented May 3, 2017

filipw commented May 3, 2017

lukepuplett commented Jun 15, 2021

darkguy2008 commented Jul 19, 2021

Use CompletionService in IntellisenseService #840

Use CompletionService in IntellisenseService #840

Conversation

filipw commented Apr 30, 2017

Attribute completion without Attribute

Cref completion

Object initializer members completion

Named parameters completion

Override support

DustinCampbell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipw commented Apr 30, 2017

david-driscoll left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell commented May 1, 2017

filipw commented May 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell commented May 3, 2017

DustinCampbell left a comment

Choose a reason for hiding this comment

david-driscoll left a comment

Choose a reason for hiding this comment

DustinCampbell commented May 3, 2017

filipw commented May 3, 2017

lukepuplett commented Jun 15, 2021

darkguy2008 commented Jul 19, 2021

Attribute completion without `Attribute`

david-driscoll left a comment •

edited

Loading