Completion for unimported extension methods #39126

genlu · 2019-10-08T00:30:11Z

TODO:

Finalize the experience when indices are not completely available (e.g. show a banner in completion list saying still loading stuff?)
~~More tests for VB~~
~~Perf analysis.~~
~~Needs to Call into remote process for filtering, since that's where index is built.~~
~~Implement item description~~
~~Determine the bail condition (e.g. if any of the filter items isn't calculated? And/Or a timebox? )~~ now it bails if any of the required filter is not already created. Using expander would force build them.
~~Telemetry~~

There's an existing bug in SymbolTreeInfo that would cause us to miss extension methods declared in PE references which was compiled from VB source code. #39558

@CyrusNajmabadi Could you please give this a quick look and let me know if the implementation of the filter makes sense? Thanks!

…olTreeIndex

…xtCompletion

CyrusNajmabadi · 2019-10-08T17:58:01Z

happy to look. but perf analysis is def necessary.

...e/Completion/CompletionProviders/ImportCompletion/ExtensionMethodImportCompletionProvider.cs

...arp/Portable/Completion/CompletionProviders/ImportCompletion/TypeImportCompletionProvider.cs

...letion/Providers/ImportCompletionProvider/AbstractExtensionMethodImportCompletionProvider.cs

CyrusNajmabadi · 2019-10-08T18:08:34Z

...letion/Providers/ImportCompletionProvider/AbstractExtensionMethodImportCompletionProvider.cs

+
+                completionContext.AddItems(items);
+
+                telemetryCounter.TotalExtensionMethodsProvided = items.Count;


todo: review this.

...letion/Providers/ImportCompletionProvider/AbstractExtensionMethodImportCompletionProvider.cs

...e/Portable/Completion/Providers/ImportCompletionProvider/AbstractImportCompletionProvider.cs

CyrusNajmabadi · 2019-10-08T18:10:45Z

...e/Portable/Completion/Providers/ImportCompletionProvider/AbstractImportCompletionProvider.cs

+                return;
+            }
+
+            completionContext.ExpandItemsAvailable = true;


what's this?

why do you need to do this if you already exported the member that said you were IsExpandItemProvider ? I don't get the purpose of having both flags.

There are two possible cases if expand provider returns nothing, first is there's actually nothing, another is we might be able to return something but didn't (e.g. timed out). We want to grey out the expander in the first case but show it as selectable in the latter.

can you doc this? i honestly find it very confusing still. i would prefer ths all be unified somehow .

There's already doc for ExpandItemsAvailable property

src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfo.Node.cs

CyrusNajmabadi · 2019-10-08T18:16:16Z

src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfo.Node.cs

+
+            public ParameterTypeInfo GetArrayType(ParameterTypeInfo elementType, ArrayShape shape) => ComplexInfo;
+
+            public ParameterTypeInfo GetSZArrayType(ParameterTypeInfo elementType) => ComplexInfo;


doc most of the code here.

src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfo.Node.cs

CyrusNajmabadi · 2019-10-08T18:20:52Z

src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfo.cs

+        // categories for filtering purpose. Whether a method is simple is determined based on if we can determine
+        // it's target type easily with a pure text matching. For complex methods, we will need to rely on symbol
+        // to decide if it's feasible.
+        // All primitive types, types in regular use form are considered simple.


"types in regular use form"?

please add exmaples/explanations.

src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfo.cs

CyrusNajmabadi · 2019-10-08T18:24:11Z

src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfo_Metadata.cs

@@ -407,10 +422,18 @@ private void GenerateMetadataNodes()
                        // we just pull in methods that have attributes on them.


i wonder if we can do better. at least see if hte attribute is called "Extension".

Sure can do, it's just unclear how much benefit it would provide

it would probably greatly decrease the number of methods we pull in.

this still seems worth looking into IMO.

this doesn't seem to cause any issues during my test in roslyn.sln. Will file a bug and consider this in a follow-up PR

CyrusNajmabadi · 2019-10-31T23:10:26Z

...etion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.CacheEntry.cs

+                    var info = await document.GetSyntaxTreeIndexAsync(loadOnly, cancellationToken).ConfigureAwait(false);
+                    if (info == null)
+                    {
+                        return null;


please doc that this is an intentional decision, and you didn't accidentally forget to 'continue' here.

...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs

CyrusNajmabadi · 2019-10-31T23:12:06Z

...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs

+                {
+                    if (s_indexingTask.IsCompleted)
+                    {
+                        s_indexingTask = Task.Run(() => GetPossibleExtensionMethodMatchesAsync(project, allTypeNames, forceIndexCreation: false, isPrecalculation: true, CancellationToken.None));


why are we passing forceIndexCreation: false here?

CyrusNajmabadi · 2019-10-31T23:13:23Z

...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs

+                    // Don't provide anything if we don't have all the required SyntaxTreeIndex created.
+                    if (cacheEntry == null)
+                    {
+                        return null;


CyrusNajmabadi · 2019-10-31T23:14:45Z

...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs

+                    }
+                }
+
+                // We are just trying to populate the cache in background, no need to return any results.


ew. this is not pretty. a better way would be to extract out the cache computation logic and then call into that from both locations rather than having the precalculation logic cause it to happen (but hten skip some work) by calling into the actual query codepath.

CyrusNajmabadi · 2019-10-31T23:16:20Z

...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs

+                cancellationToken.ThrowIfCancellationRequested();
+
+                var indexOfLastDot = fullyQualifiedContainerName.LastIndexOf('.');
+                var qualifiedNamespaceName = indexOfLastDot > 0 ? fullyQualifiedContainerName.Substring(0, indexOfLastDot) : string.Empty;


that's a pity. so we force a string-alloc on every item found? is this ok because the expetation is that the filter will hav ea small number of results?

yes, it's on every containing type we found. And yes, with filter, this number should be fairly small so it's unlikely to cause perf issues.

CyrusNajmabadi · 2019-10-31T23:18:11Z

...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs

+                    IMethodSymbol? reducedMethodSymbol = null;
+
+                    if (methodSymbol.IsExtensionMethod &&
+                        (methodNames.Count == 0 || methodNames.Contains(methodSymbol.Name)) &&


i don't understand teh methodNames.Count == 0 check. if we had zero method names, how would we get into the outer loop in the first place. I also don't understand the inner check. We only asked for methods with this speicfic name (from methodNames), so how would methodNames not contain the method name??

Good catch. It was a leftover from previous logic. Removed!

CyrusNajmabadi · 2019-10-31T23:20:37Z

...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs

+
+            var semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false);
+            var items = GetExtensionMethodItems(assembly.GlobalNamespace, receiverTypeSymbol,
+                semanticModel!, position, namespaceInScope, matchedMethods, counter, cancellationToken);


i don't see how this could work. here's the issue:

you looked at all dependent projects to add their pe references and get the indices for them.

you're now walking your particular assembly.GlobalNamespace symbol actually looking for the extension methods.

This would not find any extension methods for pe references that this compilation doesn't itself point at.

So basically, i just don't get how this coudl work.

As discussed offline, transitive PE reference isn't support in C#/VB, so this is simply a mistake and I will remove related code.

...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs

CyrusNajmabadi · 2019-10-31T23:21:53Z

...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs

+
+        /// <summary>
+        /// The purpose of this is to help us keeping track of conflicting types with data required to create corresponding
+        /// items in a tree, which is easy to use while navigating symbol tree recursively.


would be good to give an example of what counts as a 'conflict'.

CyrusNajmabadi · 2019-10-31T23:22:50Z

...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs

+        /// The purpose of this is to help us keeping track of conflicting types with data required to create corresponding
+        /// items in a tree, which is easy to use while navigating symbol tree recursively.
+        /// </summary>
+        private class ConflictNameNode : IDisposable


i'm not really sure how conflicts arise. your indices already are associated with source or pe-references. so you can triviall walk the corresponding IAssembly for each to find your matches, and you should never run into a conflict in these specific iassembly symbols.

CyrusNajmabadi · 2019-10-31T23:25:14Z

...Features/Core/Portable/Completion/Providers/ImportCompletionProvider/ImportCompletionItem.cs

+            }
+
+            // Otherwise, this is a type item, so we don't have SymbolKey data. But we should still have all
+            // the data to construct its full metadata name


dumb question. why not just have the TypeItem do the same thing the extension method item does? then you can operate on both consistently.

Because after the cached items are created, type items don't operate on symbols at all (except for getting description), holding on to those SymbolKey data in cache seems wasteful. Whereas, we need symbols to provide extension method items every time.

CyrusNajmabadi · 2019-10-31T23:25:43Z

...e/Portable/Completion/Providers/ImportCompletionProvider/SerializableImportCompletionItem.cs

+
+namespace Microsoft.CodeAnalysis.Completion.Providers
+{
+    internal sealed class SerializableImportCompletionItem


can these be structs to save on allocations?

Sure. Fixed

CyrusNajmabadi · 2019-10-31T23:27:56Z

src/Workspaces/CSharp/Portable/FindSymbols/CSharpDeclaredSymbolInfoFactoryService.cs

+                        return TryGetSimpleTypeName(aliasQualifiedNameNode.Name, typeParameterNames, out simpleTypeName);
+
+                    case QualifiedNameSyntax qualifiedNameNode:
+                        // For an identifier to the right of a '.', it can't be a type parameter,


... that can't happen. :)

src/Workspaces/CSharp/Portable/LanguageServices/CSharpSyntaxFactsService.cs

src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfo_Metadata.cs

CyrusNajmabadi · 2019-10-31T23:37:55Z

src/Workspaces/Core/Portable/FindSymbols/SyntaxTree/SyntaxTreeIndex_Create.cs

            }
        }

+        private static readonly Func<KeyValuePair<string, ArrayBuilder<int>>, string> s_getKey = kvp => kvp.Key;
+        private static readonly Func<KeyValuePair<string, ArrayBuilder<int>>, ImmutableArray<int>> s_getValuesAsImmutableArray = kvp => kvp.Value.ToImmutableAndFree();


this second one is a bit terrifying.

Moved free part to finally

src/Workspaces/Core/Portable/FindSymbols/SyntaxTree/SyntaxTreeIndex_Forwarders.cs

CyrusNajmabadi · 2019-10-31T23:39:19Z

src/Workspaces/Core/Portable/LanguageServices/SyntaxFactsService/ISyntaxFactsService.cs

+        /// In VB, we have a member access expression with a null expression, this may be one of the
+        /// following forms:
+        ///     1) new With { .a = 1, .b = .a      .a refers to the anonymous type
+        ///     2) With obj : .m                   .m refers to the obj type


do you test thsi for extension method import?

I have made the change so we don't support this scenario, otherwise we'd need to check whether it's on LHS or RHS, would be fairly simple, but given how rare this is, I'd prefer to avoid the extra complexity here.

let me rephrase :) can we have a test to make sure we don't crash here :)

Yes, we have a test for this

src/Workspaces/Core/Portable/Shared/Extensions/DocumentExtensions.cs

CyrusNajmabadi · 2019-10-31T23:40:59Z

overall looking good. however last pass did raise at least 2-3 serious concerns on my part. either things are broken based on my reading, or i'm not understanding something here. def need clarity in those areas. thanks!

genlu · 2019-11-06T19:39:43Z

I got verbal approval from @CyrusNajmabadi via gitter discussion. I will try the change suggested here in a separate PR and see if it can benefit perf.

genlu added 5 commits September 27, 2019 16:32

Baseline implementation of import completion for extension methods

8b3de6a

Implement filter for extension methods based on SyntaxTreeIndex

d4a44d3

Implement filter for extension methods in PE references based on Symb…

3996699

…olTreeIndex

Add SyntaxTrrInfo based filter for VB

59e1705

Add tests for import completion of extension methods

d557184

genlu added the Area-IDE label Oct 8, 2019

genlu requested a review from a team as a code owner October 8, 2019 00:30

Merge remote-tracking branch 'dotnet/release/dev16.5-preview1' into e…

771650a

…xtCompletion

genlu requested a review from CyrusNajmabadi October 8, 2019 17:26

Add NRT annotations and some minor changes

811a9aa