Roslyn: Completion should provide extension methods from unimported namespaces

[genlu]

[clairernovotny]

What is the priority on this, from a R# compete perspective, I can't fully move away from it without this.

[genlu]

I will start working on this very soon, once the existing usability issues for type import completion are fixed.

[clairernovotny]

Bump, how's this coming along?

[genlu]

There's some delay, I have been mostly working on fixing/improving existing type import completion. But I will start this (for real this time :))

[sandyarmstrong]

@genlu is this likely for 16.4? Can we adjust the milestone?

VSmac users are missing this now that we have moved to Roslyn's implementation of import completion.

[CyrusNajmabadi]

@sandyarmstrong how was this previously implemented for VSMac?

[sandyarmstrong]

@CyrusNajmabadi I'm not that familiar with the old editor implementation, but I believe you'll find it here: https://github.com/mono/monodevelop/blob/release-8.3/main/src/addins/CSharpBinding/MonoDevelop.CSharp.Completion/CSharpCompletionTextEditorExtension.cs#L283

[CyrusNajmabadi]

Oh interesting. So monodevelop jsut wrote their own completion rather than using Roslyns (or porting this back to roslyn). Hrmm... that's tricky. I think we'd have ot try this out and validate that perf was ok.

For example, with extension-methods we have to make sure that what's on the LHS of the dot is compatible with the this arg of the extension method. That involves expensive binding and i'm not sure how fast that will be when testing it against every extension method in the solution.

[clairernovotny]

Extension methods are super important to finish this feature off.... R# still "wins" without this feature.

[CyrusNajmabadi]

R# still "wins" without this feature.

No question. My concern is primarily around perf.

[genlu]

@sandyarmstrong this is currently targeting for 16.4. Didn't make in 16.3 because we have been spending a lot of the focus on polishing unimported type completion after it was shipped as an experiment feature, which I think is in a good shape now.

[Dreamescaper]

Just wanted to share some thoughts about perf.
In IntelliSense Extender I used the most straightforward implementation possible, without any additional caching, smth like that:

var extMethodSymbols = GetAllTypes(semanticModel.Compilation.GlobalNamespace)
    .Where(t => t.MightContainExtensionMethods)
    .SelectMany(t => t.GetMembers())
    .OfType<IMethodSymbol>()
    .Select(m => m.ReduceExtensionMethod(syntaxContext.AccessedSymbolType))
    .Where(m => m != null)
    .ToArray();

With that on my laptop it takes ~40ms for medium size project (10k total types available), and ~45ms for Roslyn's CSharp.EditorFeatures.

(Haven't investigated any caching or other perf improvements, since unimported types were much slower for me at that point)

[CyrusNajmabadi]

and ~45ms for Roslyn's CSharp.EditorFeatures.

Good to know. That's definitely not good as that's basically teh entire time slice allocated for completion. i.e. end-to-end completion tries to be in a 50ms window. The reason for that is that completion has a blocking-behavior on the user. i.e. if you type something which is considered a completion character, we will block all further typing until completion computes, selects an item, and inserts it.

If the time goes beyond 50ms that blocking starts becoming perceptible to the user as delays in typing. As such, we try to keep completion far below that. That's really important as all sorts of random other things in VS can add on top of that 50ms (for example, an errant GC). If this is 45ms that would be far too close to comfort to me. We'd basically likely go over the limit in many circumstances (including less powerful machines) and we'd likely not be able to add additional completion features since we'd have no more buffer to fit them in.

I hope that helps clarify the perf concern here! :)

[Dreamescaper]

Sure, perf concerns are definitely valid.
Not suggesting to make it this way, just sharing ;)

[CyrusNajmabadi]

Not suggesting to make it this way, just sharing ;)

Totally! And your info was super appreciated. I was just making it clear that this is a definite concern, and just because vs4mac (or omnisharp) had something doesn't mean that the same may necessarily be possible (or cheap) for roslyn to provide.

[genlu]

With that on my laptop it takes ~40ms for medium size project (10k total types available), and ~45ms for Roslyn's CSharp.EditorFeatures.

@Dreamescaper Thanks! This is mostly consistent with the prototype I implemented using the simple logic you mentioned (adding checks for accessibility introduces a little additional overhead though), which in my opinion isn't a terrible baseline to start with. Especially considering that the completion for unimported type often cause more overhead by simply dumping thousands of completion items into the list (This doesn't include time spent in the provider itself, mind you, just from completion system handling all those items after they are calculated, obviously we still have some optimization to do here :))

That said, I'm trying to add some text based filtering, before asking compiler to start binding members. Hopefully this would avoid most of the unnecessary computation.

The reason for that is that completion has a blocking-behavior on the user. i.e. if you type something which is considered a completion character, we will block all further typing until completion computes, selects an item, and inserts it.

FYI, VS has imposed a timeout for this scenario. So by default, completion won't block commit for more than 250ms.

[CyrusNajmabadi]

FYI, VS has imposed a timeout for this scenario. So by default, completion won't block commit for more than 250ms.

Right. It won't block. but it does that at the expense of cancelling completion i thought. So if this ends up taking too long, what that means is that completion just becomes broken for me. I can type, but the feature no longer works.

Right?

[genlu]

@CyrusNajmabadi I think that's correct. @AmadeusW would probably be able to provide a detailed answer here.

[CyrusNajmabadi]

@genlu Then it is even more *critical that extension-completion be extremely low overhead. Otehrwise, it risks completely breaking muscle memory or breaking hte user experience entirely. For example, imagine on my laptop that computing these consistently takes 300ms. Then i literally will not get top-level completion ever since it will always go over the cap and constantly be killed off.

This would actually explain what i see occasionally in VS where i'm sure i typed something properly, but i don't get the results in VS that i expect. :grr: :-/

[genlu]

Alright, merged. The feature is controlled by the same option as unimported types, which is off by default for now.