Support EditorBrowsable in completion for unimported types #47771
Conversation
...pletion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.CacheEntry.cs
Outdated
Show resolved
Hide resolved
...pletion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.CacheEntry.cs
Outdated
Show resolved
Hide resolved
| @@ -40,6 +41,7 @@ internal AbstractTypeImportCompletionService(Workspace workspace) | |||
| bool forceCacheCreation, | |||
| CancellationToken cancellationToken) | |||
| { | |||
| var hideAdvancedmembers = currentProject.Solution.Workspace.Options.GetOption(CompletionOptions.HideAdvancedMembers, currentProject.Language); | |||
There was a problem hiding this comment.
| var hideAdvancedmembers = currentProject.Solution.Workspace.Options.GetOption(CompletionOptions.HideAdvancedMembers, currentProject.Language); | |
| var hideAdvancedmembers = currentProject.Solution.Options.GetOption(CompletionOptions.HideAdvancedMembers, currentProject.Language); |
(fix elsewhere if you used teh former).
the former is mutable and may change as the feature is running. the latter is an immutable snapshot.
There was a problem hiding this comment.
Good point. I guess this one needs to be fixed too
http://sourceroslyn.io/#Microsoft.CodeAnalysis.Features/Shared/Extensions/DocumentExtensions.cs,26
...ortable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.cs
Outdated
Show resolved
Hide resolved
...ortable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.cs
Outdated
Show resolved
Hide resolved
|
@dotnet/roslyn-ide Can I get another review on this? Thanks |
...pletion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.CacheEntry.cs
Outdated
Show resolved
Hide resolved
...ortable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Shared/Extensions/ISymbolExtensions.cs
Outdated
Show resolved
Hide resolved
...pletion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.CacheEntry.cs
Outdated
Show resolved
Hide resolved
...pletion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.CacheEntry.cs
Outdated
Show resolved
Hide resolved
...pletion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.CacheEntry.cs
Outdated
Show resolved
Hide resolved
...ortable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.cs
Outdated
Show resolved
Hide resolved
| { | ||
| if (!symbol.IsModuleType()) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| attributes = attributes.IsDefault ? symbol.GetAttributes() : attributes; | ||
| hideModuleNameAttribute ??= compilation.HideModuleNameAttribute(); | ||
|
|
||
| if (!hideModuleNameAttribute.HasValue) |
There was a problem hiding this comment.
afaiut, this will never be false. so we'll never go into this branch.
There was a problem hiding this comment.
IsEditorBrowsable can be called with default parameter value, in which case HasValue will be false.
There was a problem hiding this comment.
basically, my preference woudl be that instead of taking N extra parameters, you take an optional EBInfo struct (for caching those values). If that value is not provided, you instantiate a new one in this method.
There was a problem hiding this comment.
would it be hard to take an optional EBInfo?
There was a problem hiding this comment.
I felt it might be unnecessary given it's only called from two locations (that provides value for those optional parameters). But sure.
|
Ping @CyrusNajmabadi |
...pletion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.CacheEntry.cs
Outdated
Show resolved
Hide resolved
|
@vatsalyaagrawal @jinujoseph Do we want this bug fix for 16.8? There's no bug/feedback for it, but it came up because of an analyzer proposal (dotnet/roslyn-analyzers#3646) |
| }"; | ||
|
|
||
| var refDoc = @" | ||
| namespace Foo |
There was a problem hiding this comment.
References of "Foo" was removed from the code base a very long time ago in #21224
I'm not sure if you still don't want to include "Foo"s or not. But just noting.
There was a problem hiding this comment.
@CyrusNajmabadi Is this ban on Foo still a thing? There's a ton of them in our tests still.
There was a problem hiding this comment.
it's likely this will flag a review check at some point. so probably best to just not introduce new instances.
|
@genlu lets keep in master |
ghost
added this to the 
Extension method completion was fixed by #47590
This would help addressing the issue described in dotnet/roslyn-analyzers#3646