Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use CompletionService in IntellisenseService #840

Merged
merged 10 commits into from
May 3, 2017
Merged

Use CompletionService in IntellisenseService #840

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
dbdc713
first stab at improving IntellisenseService
filipw Apr 30, 2017
8f233f0
remved IntellisenseV2Service
filipw Apr 30, 2017
73c78c4
clean up of commented code, comments and logic
filipw Apr 30, 2017
e82f2e8
extracted CompletionItemExtensions
filipw Apr 30, 2017
5a34a46
cache GetSymbolsAsync
filipw Apr 30, 2017
d8e066a
use OmniSharp reflection extensions
filipw Apr 30, 2017
ab55def
added attribute completion and object initializer completion tests
filipw Apr 30, 2017
b179067
added more tests to IntellisenseFacts
filipw Apr 30, 2017
5643a01
better completion sorting, and consistent across Windows and Nix
filipw May 2, 2017
62fe53a
no need to check for null after Where
filipw May 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions src/OmniSharp.Roslyn.CSharp/Services/Intellisense/CompletionItemExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
using System.Reflection;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Completion;
using OmniSharp.Utilities;

namespace OmniSharp.Roslyn.CSharp.Services.Intellisense
{
internal static class CompletionItemExtensions
{
private static MethodInfo _getSymbolsAsync;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes good point 👍

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (completionItem.Properties.ContainsKey("Provider") && completionItem.Properties["Provider"] == "Microsoft.CodeAnalysis.CSharp.Completion.Providers.SymbolCompletionProvider")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
var symbols = recommendedSymbols.Where(x => x.Name == completionItem.Properties["SymbolName"] && (int)x.Kind == int.Parse(completionItem.Properties["SymbolKind"])).Distinct();
return symbols ?? Enumerable.Empty<ISymbol>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right 😊

}

// if the completion provider encoded symbols into Properties, we can return them
if (completionItem.Properties.ContainsKey("Symbols"))
{
// the API to decode symbols is not public at the moment
// http://source.roslyn.io/#Microsoft.CodeAnalysis.Features/Completion/Providers/SymbolCompletionItem.cs,93
var decodedSymbolsTask = _getSymbolsAsync.InvokeStatic<Task<ImmutableArray<ISymbol>>>(new object[] { completionItem, document, default(CancellationToken) });
if (decodedSymbolsTask != null)
{
return await decodedSymbolsTask;
}
}

return Enumerable.Empty<ISymbol>();
}
}
}
92 changes: 50 additions & 42 deletions src/OmniSharp.Roslyn.CSharp/Services/Intellisense/IntellisenseService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System;
using System.Collections.Generic;
using System.Composition;
using System.Linq;
Expand Down Expand Up @@ -37,54 +38,60 @@ public async Task<IEnumerable<AutoCompleteResponse>> Handle(AutoCompleteRequest
{
var sourceText = await document.GetTextAsync();
var position = sourceText.Lines.GetPosition(new LinePosition(request.Line, request.Column));

var service = CompletionService.GetService(document);
var completionList = await service.GetCompletionsAsync(document, position);

// Add keywords from the completion list. We'll use the recommender service to get symbols
// to create snippets from.

if (completionList != null)
{
// get recommened symbols to match them up later with SymbolCompletionProvider
var semanticModel = await document.GetSemanticModelAsync();
var recommendedSymbols = await Recommender.GetRecommendedSymbolsAtPositionAsync(semanticModel, position, _workspace);

foreach (var item in completionList.Items)
{
if (item.Tags.Contains(CompletionTags.Keyword))
var completionText = item.DisplayText;
if (completionText.IsValidCompletionFor(wordToComplete))
{
// Note: For keywords, we'll just assume that the completion text is the same
// as the display text.
var keyword = item.DisplayText;
if (keyword.IsValidCompletionFor(wordToComplete))
var symbols = await item.GetCompletionSymbolsAsync(recommendedSymbols, document);
if (symbols.Any())
{
var response = new AutoCompleteResponse()
foreach (var symbol in symbols)
{
CompletionText = item.DisplayText,
DisplayText = item.DisplayText,
Snippet = item.DisplayText,
Kind = request.WantKind ? "Keyword" : null
};

completions.Add(response);
if (symbol != null)
{
if (request.WantSnippet)
{
foreach (var completion in MakeSnippetedResponses(request, symbol, item.DisplayText))
{
completions.Add(completion);
}
}
else
{
completions.Add(MakeAutoCompleteResponse(request, symbol, item.DisplayText));
}
}
}

// if we had any symbols from the completion, we can continue, otherwise it means
// the completion didn't have an associated symbol so we'll add it manually
continue;
}
}
}
}

var model = await document.GetSemanticModelAsync();
var symbols = await Recommender.GetRecommendedSymbolsAtPositionAsync(model, position, _workspace);
// for other completions, i.e. keywords, create a simple AutoCompleteResponse
// we'll just assume that the completion text is the same
// as the display text.
var response = new AutoCompleteResponse()
{
CompletionText = item.DisplayText,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completion on partial will also be broken.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

DisplayText = item.DisplayText,
Snippet = item.DisplayText,
Kind = request.WantKind ? item.Tags.First() : null
};

foreach (var symbol in symbols.Where(s => s.Name.IsValidCompletionFor(wordToComplete)))
{
if (request.WantSnippet)
{
foreach (var completion in MakeSnippetedResponses(request, symbol))
{
completions.Add(completion);
completions.Add(response);
}
}
else
{
completions.Add(MakeAutoCompleteResponse(request, symbol));
}
}
}

Expand All @@ -93,10 +100,11 @@ public async Task<IEnumerable<AutoCompleteResponse>> Handle(AutoCompleteRequest
.ThenByDescending(c => c.CompletionText.IsValidCompletionStartsWithIgnoreCase(wordToComplete))
.ThenByDescending(c => c.CompletionText.IsCamelCaseMatch(wordToComplete))
.ThenByDescending(c => c.CompletionText.IsSubsequenceMatch(wordToComplete))
.ThenBy(c => c.CompletionText);
.ThenBy(c => c.DisplayText, StringComparer.OrdinalIgnoreCase)
.ThenBy(c => c.CompletionText, StringComparer.OrdinalIgnoreCase);
}

private IEnumerable<AutoCompleteResponse> MakeSnippetedResponses(AutoCompleteRequest request, ISymbol symbol)
private IEnumerable<AutoCompleteResponse> MakeSnippetedResponses(AutoCompleteRequest request, ISymbol symbol, string displayName)
{
var completions = new List<AutoCompleteResponse>();

Expand All @@ -105,41 +113,41 @@ private IEnumerable<AutoCompleteResponse> MakeSnippetedResponses(AutoCompleteReq
{
if (methodSymbol.Parameters.Any(p => p.IsOptional))
{
completions.Add(MakeAutoCompleteResponse(request, symbol, false));
completions.Add(MakeAutoCompleteResponse(request, symbol, displayName, false));
}

completions.Add(MakeAutoCompleteResponse(request, symbol));
completions.Add(MakeAutoCompleteResponse(request, symbol, displayName));

return completions;
}

var typeSymbol = symbol as INamedTypeSymbol;
if (typeSymbol != null)
{
completions.Add(MakeAutoCompleteResponse(request, symbol));
completions.Add(MakeAutoCompleteResponse(request, symbol, displayName));

if (typeSymbol.TypeKind != TypeKind.Enum)
{
foreach (var ctor in typeSymbol.InstanceConstructors)
{
completions.Add(MakeAutoCompleteResponse(request, ctor));
completions.Add(MakeAutoCompleteResponse(request, ctor, displayName));
}
}

return completions;
}

return new[] { MakeAutoCompleteResponse(request, symbol) };
return new[] { MakeAutoCompleteResponse(request, symbol, displayName) };
}

private AutoCompleteResponse MakeAutoCompleteResponse(AutoCompleteRequest request, ISymbol symbol, bool includeOptionalParams = true)
private AutoCompleteResponse MakeAutoCompleteResponse(AutoCompleteRequest request, ISymbol symbol, string displayName, bool includeOptionalParams = true)
{
var displayNameGenerator = new SnippetGenerator();
displayNameGenerator.IncludeMarkers = false;
displayNameGenerator.IncludeOptionalParameters = includeOptionalParams;

var response = new AutoCompleteResponse();
response.CompletionText = symbol.Name;
response.CompletionText = displayName;

// TODO: Do something more intelligent here
response.DisplayText = displayNameGenerator.Generate(symbol);
Expand Down
104 changes: 104 additions & 0 deletions tests/OmniSharp.Roslyn.CSharp.Tests/IntellisenseFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,110 @@ var x$$
ContainsCompletions(completions.Select(c => c.CompletionText), Array.Empty<string>());
}

[Theory]
[InlineData("dummy.cs")]
[InlineData("dummy.csx")]
public async Task Returns_attribute_without_attribute_suffix(string filename)
{
const string source =
@"using System;

public class BarAttribute : Attribute {}

[B$$
public class Foo {}";

var completions = await FindCompletionsAsync(filename, source);
ContainsCompletions(completions.Select(c => c.CompletionText).Take(1), "Bar");
}

[Theory]
[InlineData("dummy.cs")]
[InlineData("dummy.csx")]
public async Task Returns_members_in_object_initializer_context(string filename)
{
const string source =
@"public class MyClass1 {
public string Foo {get; set;}
}

public class MyClass2 {

public MyClass2()
{
var c = new MyClass1 {
F$$
}
}
";

var completions = await FindCompletionsAsync(filename, source);
ContainsCompletions(completions.Select(c => c.CompletionText), "Foo");
}

[Theory]
[InlineData("dummy.cs")]
[InlineData("dummy.csx")]
public async Task Returns_parameter_name_inside_a_method(string filename)
{
const string source =
@"public class MyClass1 {
public void SayHi(string text) {}
}

public class MyClass2 {

public MyClass2()
{
var c = new MyClass1();
c.SayHi(te$$
}
}
";

var completions = await FindCompletionsAsync(filename, source);
ContainsCompletions(completions.Select(c => c.CompletionText).Take(1), "text:");
}

[Theory]
[InlineData("dummy.cs")]
[InlineData("dummy.csx")]
public async Task Returns_override_signatures(string filename)
{
const string source =
@"class Foo
{
public virtual void Test(string text) {}
public virtual void Test(string text, string moreText) {}
}

class FooChild : Foo
{
override $$
}
";

var completions = await FindCompletionsAsync(filename, source);
ContainsCompletions(completions.Select(c => c.CompletionText), "Equals(object obj)", "GetHashCode()", "Test(string text)", "Test(string text, string moreText)", "ToString()");
}

[Theory]
[InlineData("dummy.cs")]
[InlineData("dummy.csx")]
public async Task Returns_cref_completion(string filename)
{
const string source =
@" /// <summary>
/// A comment. <see cref=""My$$"" /> for more details
/// </summary>
public class MyClass1 {
}
";

var completions = await FindCompletionsAsync(filename, source);
ContainsCompletions(completions.Select(c => c.CompletionText).Take(1), "MyClass1");
}

private void ContainsCompletions(IEnumerable<string> completions, params string[] expected)
{
if (!completions.SequenceEqual(expected))
Expand Down