Add namespace-name suggestions to C# and VB #11968
Conversation
| @@ -212,6 +212,11 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Recommendations | |||
| Else | |||
| symbols = context.SemanticModel _ | |||
There was a problem hiding this comment.
I don't fully understand this couldBeMergedNamespace thing or it's consequences, but I couldn't hit the positive code path. Am I missing something I'm missing here?
There was a problem hiding this comment.
The couldBeMergedNamespace thing is an error recovery scenario where GetSymbolInfo returns a bunch of CandidateSymbols that are all namespaces. I tried chasing through source control to find the original related bug but didn't find it, but I don't think you need to worry about it.
|
Tagging @CyrusNajmabadi, @rchande, @DustinCampbell and @Pilchie as potential reviewers. |
| @@ -603,6 +603,18 @@ public static bool IsAttributeNameContext(this SyntaxTree syntaxTree, int positi | |||
| return syntaxTree.IsTypeContext(position, cancellationToken, semanticModelOpt); | |||
| } | |||
|
|
|||
| public static bool IsNamespaceDeclarationNameContext(this SyntaxTree syntaxTree, int position, CancellationToken cancellationToken) | |||
| { | |||
| var token = syntaxTree.FindTokenOnLeftOfPosition(position, cancellationToken); | |||
There was a problem hiding this comment.
i think you need a ".GetPreviousTokenIfTouchingWord(position)". That way if you're actually on the 'namespace' keyword itself, that we don't think we're in the namespcae name portion.
There was a problem hiding this comment.
Huh, you're right about it returning the wrong value when on namespace, but it doesn't seem to be causing any problems. Subsequent checks in the recommendation service must be mitigating it. I'll need to investigate this a bit more.
There was a problem hiding this comment.
Oh, good catch! It doesn't show up in the simple case, but if we have nested namespaces...
namespace Microsoft
{
name$$space CodeAnalysis
{
}
}
...then it starts suggesting top level namespaces! I think this deserves a few additional tests.
There was a problem hiding this comment.
Fixed. I've also added a check to bail early on scripts, since apparently they don't support namespace declarations.
|
Overall looking good! I had a few small questions/concerns. Thanks. |
|
@CyrusNajmabadi Thanks, I'll fix those up and add comments as appropriate. Do you have any suggestions regarding my question about the |
|
@CyrusNajmabadi I've updated the PR based your feedback. I've fixed everything except the |
|
test vsi please |
|
👍 Nice job! |
|
@rchande and @dotnet/roslyn-ide can you take a look? |
| } | ||
|
|
||
| return SpecializedCollections.EmptyEnumerable<CompletionItem>(); | ||
| } | ||
|
|
||
| private async Task<IEnumerable<CompletionItem>> GetSnippetCompletionItemsAsync(Workspace workspace, SemanticModel semanticModel, int position, TextSpan itemSpan, bool isPreProcessorContext, CancellationToken cancellationToken) | ||
| private async Task<IEnumerable<CompletionItem>> GetSnippetCompletionItemsAsync(Workspace workspace, SemanticModel semanticModel, TextSpan itemSpan, bool isPreProcessorContext, CancellationToken cancellationToken) |
There was a problem hiding this comment.
Yes, unrelated. I noticed the param was unused as I was passing through. Should I revert it?
There was a problem hiding this comment.
Don't need to revert this.
|
👍 |
|
I don't have access to the vsi_p2 test failure results, so please let me know if there's something I need to investigate. |
|
Nope. It's a known issue. |
|
Thanks Lorcan! |
Part of #7213, this adds namespace-name suggestions to C# and VB.