Don't use symbolid in symbol completion #15688

rchande · 2016-12-05T22:30:27Z

When creating SymbolCompletionItems, creators now have to specify if they want to encode a SymbolId for later use in documentation display. Providers that create a lot of items (Symbol completion, Cref compeltion) don't store a SymbolId. Instead, when trying to compute documentation for a symbol, they rereun their lookup logic and attempt to find a symbol with the same name and kind as the currently selected completion and display the documentation for that symbol.

rchande · 2016-12-05T22:30:44Z

Tag @dotnet/roslyn-ide @CyrusNajmabadi for review

CyrusNajmabadi · 2016-12-05T22:33:03Z

src/Features/CSharp/Portable/Completion/CompletionProviders/CrefCompletionProvider.cs

+            // To get a Speculative SemanticModel (which is much faster), we need to 
+            // walk up to the node the DocumentationTrivia is attached to.
+            var parentNode = token.Parent.FirstAncestorOrSelf<DocumentationCommentTriviaSyntax>()?.ParentTrivia.Token.Parent;
+            _testSpeculativeNodeCallbackOpt?.Invoke(parentNode);


This entire method seems hugely complex. Can we not store more information in the item to make it so that recovery at this point is super simple to do?

CyrusNajmabadi · 2016-12-05T22:33:19Z

...s/Core/Portable/Completion/Providers/AbstractRecommendationServiceBasedCompletionProvider.cs

@@ -10,6 +10,7 @@
 using Microsoft.CodeAnalysis.Shared.Extensions.ContextQuery;
 using Microsoft.CodeAnalysis.Shared.Utilities;
 using Roslyn.Utilities;
+using Microsoft.CodeAnalysis.Text;


CyrusNajmabadi · 2016-12-05T22:34:04Z

...s/Core/Portable/Completion/Providers/AbstractRecommendationServiceBasedCompletionProvider.cs

+
+            var kind = SymbolCompletionItem.GetKind(item);
+
+            var bestSymbols = symbols.Where(s => (kind != null && s.Kind == kind) && s.Name == name).ToImmutableArray();


inner parens not needed. Also, you can do .WhereAsArray.

CyrusNajmabadi · 2016-12-05T22:34:15Z

...s/Core/Portable/Completion/Providers/AbstractRecommendationServiceBasedCompletionProvider.cs

+        {
+            var position = SymbolCompletionItem.GetContextPosition(item);
+            var name = SymbolCompletionItem.GetSymbolName(item);
+            var semanticModel = await document.GetSemanticModelForSpanAsync(new TextSpan(position, 0), cancellationToken).ConfigureAwait(false);


wrap long lines.

CyrusNajmabadi · 2016-12-05T22:35:20Z

src/Features/Core/Portable/Completion/Providers/SymbolCompletionItem.cs

+        public static CompletionItem CreateWithNameAndKind(
+            string displayText,
+            ISymbol symbol,
+            int contextPosition = -1,


what does a -1 position mean. Why would we never not have a context position?

CyrusNajmabadi · 2016-12-05T22:35:37Z

src/Features/Core/Portable/Completion/Providers/SymbolCompletionItem2.cs

+        internal static SymbolKind? GetKind(CompletionItem item)
+        {
+            string kind;
+            if (item.Properties.TryGetValue("SymbolKind", out kind))


CyrusNajmabadi · 2016-12-05T22:35:41Z

src/Features/Core/Portable/Completion/Providers/SymbolCompletionItem2.cs

+        internal static string GetSymbolName(CompletionItem item)
+        {
+            string name;
+            if (item.Properties.TryGetValue("SymbolName", out name))


CyrusNajmabadi · 2016-12-05T22:36:13Z

src/Features/Core/Portable/Completion/Providers/SymbolCompletionItem2.cs

+
+namespace Microsoft.CodeAnalysis.Completion.Providers
+{
+    internal static partial class SymbolCompletionItem


why is this in a new part?

CyrusNajmabadi · 2016-12-05T22:37:58Z

Have we verified the perf benefit here? (it totally seems reasonable, i just want to make sure).
I really think we can simplify this by just encoding the data we had when the completion items were created, thus obviating the need for so much complexity at recovery time.

basically, i have no trust that something isn't busted at recovery time. That code should be exceptionally simple so that there's practically no way to get it wrong.

Pilchie · 2016-12-05T22:39:01Z

Also tagging @mattwar

rchande · 2016-12-05T22:59:40Z

@CyrusNajmabadi

Re: (2) the problem is that everything we store needs to be serializable to string. Since we're not storing the SymbolId any more, we actually have to get a new SemanticModel and look up symbols again. See what happens in AbstractRecommendationBasedCOmpletionProvider.cs--we create a SemanticModel at the same position as when completion was started and then the same Recommender API to get symbols again.

For the cref providers, I think what's needed is a better refactoring of the symbol lookup code so that ProvideCompletionsAsync and GetDescriptionAsync actually call the same code. Does that seem reasonable?

rchande · 2016-12-05T23:00:16Z

@CyrusNajmabadi Re: (1) I haven't taken a trace yet but this change removes the actual call the customers trace spent all of its time in. I'll verify this this week.

mattwar · 2016-12-05T23:01:29Z

👍

rchande · 2016-12-05T23:39:10Z

@CyrusNajmabadi cleaned things up if you want to take another look

CyrusNajmabadi · 2016-12-06T01:10:08Z

For the cref providers, I think what's needed is a better refactoring of the symbol lookup code so that ProvideCompletionsAsync and GetDescriptionAsync actually call the same code. Does that seem reasonable?

Yes. What i would expect to see would be nothing but extraction of a few values in the completion item, which we would then use to call into the same code that created the items in the first place. GetDescriptionAsync should be extremely simple.

CyrusNajmabadi · 2016-12-06T01:14:01Z

src/Features/CSharp/Portable/Completion/CompletionProviders/CrefCompletionProvider.cs

@@ -60,10 +67,29 @@ internal override bool IsInsertionTrigger(SourceText text, int characterPosition
            var options = context.Options;
            var cancellationToken = context.CancellationToken;

+            var (token, semanticModel, symbols) = await GetSymbols(document, position, cancellationToken).ConfigureAwait(false);
+            var filteredSymbols = symbols.FilterToVisibleAndBrowsableSymbols(options.GetOption(CompletionOptions.HideAdvancedMembers, semanticModel.Language), semanticModel.Compilation);


why is FilterToVisibleAndBrowsableSymbols not in GetSymbols?

Moved into and propagated editorvisible option which is apparently not consumed by the VB side.

CyrusNajmabadi · 2016-12-06T01:15:06Z

src/Features/CSharp/Portable/Completion/CompletionProviders/CrefCompletionProvider.cs

@@ -312,8 +322,16 @@ private static TextSpan GetCompletionItemSpan(SourceText text, int position)
                rules: GetRules(insertionText));
        }

-        protected override Task<CompletionDescription> GetDescriptionWorkerAsync(Document document, CompletionItem item, CancellationToken cancellationToken)
-            => SymbolCompletionItem.GetDescriptionAsync(item, document, cancellationToken);
+        protected override async Task<CompletionDescription> GetDescriptionWorkerAsync(Document document, CompletionItem item, CancellationToken cancellationToken)


much better :)

CyrusNajmabadi · 2016-12-06T01:16:12Z

src/Features/Core/Portable/Completion/Providers/SymbolCompletionItem.cs

+                          filterText, matchPriority, supportedPlatforms, properties, tags, rules);
+        }
+
+        public static CompletionItem CreateWithSymbolId(


i don't see why we need both CreateWithSymbolId.

There are lots of callers of both overloads: some providers wind up with a list of symbols collapsed into one item and some don't.

that seems excessive. for the providers with just one symbol (really, that happens?) why not just call the above method putting the symbol into an immutable array.

same comment applies.

CyrusNajmabadi · 2016-12-06T01:16:25Z

src/Features/Core/Portable/Completion/Providers/SymbolCompletionItem.cs

+                            filterText, matchPriority, supportedPlatforms, properties, tags, rules);
+        }
+
+        public static CompletionItem CreateWithNameAndKind(


why do we need both CreateWithNameAndKinds?

There were only 2 callers of the single-symbol overload so I switched them to just calling the collection overload.

CyrusNajmabadi · 2016-12-06T01:16:44Z

src/Features/Core/Portable/Completion/Providers/SymbolCompletionItem.cs

+            var workspace = document.Project.Solution.Workspace;
+
+            var position = SymbolCompletionItem.GetDescriptionPosition(item);
+            if (position == -1)


is this possible?

I don't think so.

CyrusNajmabadi · 2016-12-06T01:17:17Z

src/Features/VisualBasic/Portable/Completion/CompletionProviders/CrefCompletionProvider.vb

+            Dim tree = Await document.GetSyntaxTreeAsync(cancellationToken).ConfigureAwait(False)
+            Dim token = tree.GetTargetToken(position, cancellationToken)
+
+


remove extra line.

CyrusNajmabadi · 2016-12-06T01:18:03Z

src/Features/VisualBasic/Portable/Completion/CompletionProviders/CrefCompletionProvider.vb

+            Dim symbolName = SymbolCompletionItem.GetSymbolName(item)
+            Dim symbolKind = SymbolCompletionItem.GetKind(item).GetValueOrDefault()
+            Dim bestSymbols = symbols.Where(Function(s) s.Kind = symbolKind AndAlso s.Name = symbolName).ToImmutableArray()
+            Return Await SymbolCompletionItem.GetDescriptionAsync(item, bestSymbols, document, cancellationToken).ConfigureAwait(False)


these 5 lines of code look like they were repeated elsewhere. Unify plz :)

CyrusNajmabadi · 2016-12-06T01:18:50Z

Looks much nicer. Still needs some work though. Thanks!

CyrusNajmabadi · 2016-12-06T21:05:07Z

src/Features/CSharp/Portable/Completion/CompletionProviders/CrefCompletionProvider.cs

+            var hideAdvancedMembers = options.GetOption(CompletionOptions.HideAdvancedMembers, semanticModel.Language);
+            var serializedOptions = ImmutableDictionary<string, string>.Empty.Add(HideAdvancedMembers, hideAdvancedMembers.ToString());
+
+            var items = CreateCompletionItems(document.Project.Solution.Workspace, semanticModel, symbols, token, span, position, serializedOptions);


wrap long lines.

CyrusNajmabadi · 2016-12-06T21:05:34Z

src/Features/CSharp/Portable/Completion/CompletionProviders/CrefCompletionProvider.cs

-
-            context.IsExclusive = true;
+            var symbols = GetSymbols(token, semanticModel, cancellationToken)
+                .FilterToVisibleAndBrowsableSymbols(options.GetOption(CompletionOptions.HideAdvancedMembers, semanticModel.Language), semanticModel.Compilation);


wrap long lines.

CyrusNajmabadi · 2016-12-06T21:06:03Z

src/Features/CSharp/Portable/Completion/CompletionProviders/CrefCompletionProvider.cs

                rules: GetRules(insertionText));
        }

-        protected override Task<CompletionDescription> GetDescriptionWorkerAsync(Document document, CompletionItem item, CancellationToken cancellationToken)
-            => SymbolCompletionItem.GetDescriptionAsync(item, document, cancellationToken);
+


too many newlines.

CyrusNajmabadi · 2016-12-06T21:07:13Z

...s/Core/Portable/Completion/Providers/AbstractRecommendationServiceBasedCompletionProvider.cs

@@ -1,15 +1,16 @@
-using System.Collections.Generic;


CyrusNajmabadi · 2016-12-06T21:07:30Z

...s/Core/Portable/Completion/Providers/AbstractRecommendationServiceBasedCompletionProvider.cs

@@ -100,5 +101,21 @@ private static int ComputeSymbolMatchPriority(ISymbol symbol)

            return SymbolMatchPriority.PreferType;
        }
+
+        protected override async Task<CompletionDescription> GetDescriptionWorkerAsync(Document document, CompletionItem item, CancellationToken cancellationToken)


wrap long lines.

CyrusNajmabadi · 2016-12-06T21:08:03Z

...s/Core/Portable/Completion/Providers/AbstractRecommendationServiceBasedCompletionProvider.cs

+            var name = SymbolCompletionItem.GetSymbolName(item);
+            var semanticModel = await document.GetSemanticModelForSpanAsync(new TextSpan(position, 0), cancellationToken)
+                .ConfigureAwait(false);
+            var symbols = await Recommender.GetImmutableRecommendedSymbolsAtPositionAsync(


instead of taking an semantic model, workspace and options, could this not just take a document?

I believe it takes a SemanticModel so that callers can produce SpeculativeSemanticModels as neccessary.

CyrusNajmabadi · 2016-12-06T21:08:18Z

src/Features/Core/Portable/Completion/Providers/SymbolCompletionItem.cs

-                    contextDocument = document.Project.Solution.GetDocument(contextId);
-                }
-            }
+            Document contextDocument = FindAppropriateDocumentForDescriptionContext(document, supportedPlatforms);


CyrusNajmabadi · 2016-12-06T21:10:21Z

src/Features/VisualBasic/Portable/Completion/CompletionProviders/CrefCompletionProvider.vb

+            Dim token = tree.GetTargetToken(position, cancellationToken)
+
+            If IsCrefTypeParameterContext(token) Then
+                Return (Nothing, Nothing, Nothing)


can just return 'nothing'.

CyrusNajmabadi · 2016-12-06T21:10:38Z

src/Features/VisualBasic/Portable/Completion/CompletionProviders/CrefCompletionProvider.vb

                displayText:=displayString,
                insertionText:=Nothing,
-                symbol:=symbol,
+                symbols:=ImmutableArray.Create(symbol),


i.e. like what you did here.

CyrusNajmabadi · 2016-12-06T21:10:54Z

Looking better! :)

rchande · 2016-12-07T03:15:57Z

@CyrusNajmabadi Any remaining thoughts? I had to add a bit of complexity in order to properly support linked files but it uses most of the existing infrastructure.

rchande · 2016-12-07T03:16:17Z

@jasonmalinowski @DustinCampbell Do we have a branch we're targeting for QFE changes?

DustinCampbell · 2016-12-07T14:13:22Z

I guess it would be microupdate? I'm not 100% sure though. @jasonmalinowski will know best.

jasonmalinowski · 2016-12-07T18:54:55Z

@rchande: you should target this against microupdate.

CyrusNajmabadi · 2016-12-13T21:34:39Z

src/Features/Core/Portable/Completion/Providers/AbstractCrefCompletionProvider.cs

+            }
+
+            var options = document.Project.Solution.Workspace.Options
+                .WithChangedOption(new OptionKey(CompletionOptions.HideAdvancedMembers, document.Project.Language), hideAdvancedMembers);


why are we changing options? why are we not just using the option as defined by the user?

Doing this actually overrides the user's choice here.

CyrusNajmabadi · 2016-12-13T21:35:02Z

...s/Core/Portable/Completion/Providers/AbstractRecommendationServiceBasedCompletionProvider.cs

+            var kind = SymbolCompletionItem.GetKind(item);
+            var relatedDocumentIds = document.Project.Solution.GetRelatedDocumentIds(document.Id).Concat(document.Id);
+            var options = document.Project.Solution.Workspace.Options;
+            var totalSymbols = await base.GetPerContextSymbols(document, position, options, relatedDocumentIds, preselect: false, cancellationToken: cancellationToken).ConfigureAwait(false);


Use editorbrowsable options when computing cref descriptions Share cref description computation code between C#/VB Etc

dnfclas added the cla-already-signed label Dec 5, 2016

rchande changed the title ~~Don't use symbolid in symbl completion~~ Don't use symbolid in symbol completion Dec 5, 2016

CyrusNajmabadi reviewed Dec 5, 2016

View reviewed changes

CyrusNajmabadi reviewed Dec 6, 2016

View reviewed changes

rchande changed the base branch from master to microupdate December 7, 2016 20:33

rchande changed the base branch from microupdate to master December 7, 2016 20:48

rchande mentioned this pull request Dec 8, 2016

Don't use SymbolId when creating completion items (MU edition) #15772

Merged

CyrusNajmabadi reviewed Dec 13, 2016

View reviewed changes

Pilchie added the Area-IDE label Dec 15, 2016

Ravi Chande added 8 commits January 4, 2017 10:21

Don't use symbolid in symbl completion

0598b18

CR feedback

06cd97a

Various CR feedback:

47400de

Use editorbrowsable options when computing cref descriptions Share cref description computation code between C#/VB Etc

Satisfy RepoUtil

95eb04f

Fix tests

da54972

Line wrapping/copyright headers

0054e90

Pass through correct SemanticModel

50bd970

Make description computation work in linked files

df29fa8

rchande force-pushed the fastersymbolcompletion branch from 23992e4 to df29fa8 Compare January 4, 2017 18:25

Use correct version of ValueTuple

c1fe0af

rchande merged commit 16a48cd into dotnet:master Jan 4, 2017

filipw mentioned this pull request Apr 30, 2017

Use CompletionService in IntellisenseService OmniSharp/omnisharp-roslyn#840

Merged


		var kind = SymbolCompletionItem.GetKind(item);

		var bestSymbols = symbols.Where(s => (kind != null && s.Kind == kind) && s.Name == name).ToImmutableArray();

		Dim tree = Await document.GetSyntaxTreeAsync(cancellationToken).ConfigureAwait(False)
		Dim token = tree.GetTargetToken(position, cancellationToken)

Don't use symbolid in symbol completion #15688

Don't use symbolid in symbol completion #15688

Conversation

rchande commented Dec 5, 2016

rchande commented Dec 5, 2016

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

CyrusNajmabadi commented Dec 5, 2016

Pilchie commented Dec 5, 2016

rchande commented Dec 5, 2016

rchande commented Dec 5, 2016

mattwar commented Dec 5, 2016

rchande commented Dec 5, 2016

CyrusNajmabadi commented Dec 6, 2016

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

CyrusNajmabadi commented Dec 6, 2016

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

CyrusNajmabadi commented Dec 6, 2016

rchande commented Dec 7, 2016

rchande commented Dec 7, 2016

DustinCampbell commented Dec 7, 2016

jasonmalinowski commented Dec 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment