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.

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

Support EditorBrowsable in completion for unimported types #47771

Merged
merged 11 commits into from
Sep 25, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
using Microsoft.CodeAnalysis.Experiments;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.Test.Utilities;
using Microsoft.VisualStudio.Composition;
using Roslyn.Test.Utilities;
using Xunit;

Expand All @@ -35,12 +34,15 @@ internal override Type GetCompletionProviderType()

private bool DisallowAddingImports { get; set; }

private bool HideAdvancedMembers { get; set; }

protected override OptionSet WithChangedOptions(OptionSet options)
{
return options
.WithChangedOption(CompletionOptions.ShowItemsFromUnimportedNamespaces, LanguageNames.CSharp, ShowImportCompletionItemsOptionValue)
.WithChangedOption(CompletionServiceOptions.IsExpandedCompletion, IsExpandedCompletion)
.WithChangedOption(CompletionServiceOptions.DisallowAddingImports, DisallowAddingImports);
.WithChangedOption(CompletionServiceOptions.DisallowAddingImports, DisallowAddingImports)
.WithChangedOption(CompletionOptions.HideAdvancedMembers, LanguageNames.CSharp, HideAdvancedMembers);
}

protected override TestComposition GetComposition()
Expand Down Expand Up @@ -1413,6 +1415,138 @@ private static void AssertRelativeOrder(List<string> expectedTypesInRelativeOrde
}
}

[InlineData(true)]
[InlineData(false)]
[Theory, Trait(Traits.Feature, Traits.Features.Completion)]
public async Task TestBrowsableAwaysFromReferences(bool isProjectReference)
{
var srcDoc = @"
class Program
{
void M()
{
$$
}
}";

var refDoc = @"
namespace Foo
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@CyrusNajmabadi Is this ban on Foo still a thing? There's a ton of them in our tests still.

Copy link
Member

Choose a reason for hiding this comment

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

it's likely this will flag a review check at some point. so probably best to just not introduce new instances.

{
[System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Always)]
public class Goo
{
}
}";

var markup = isProjectReference switch
{
true => CreateMarkupForProjectWithProjectReference(srcDoc, refDoc, LanguageNames.CSharp, LanguageNames.CSharp),
false => CreateMarkupForProjectWithMetadataReference(srcDoc, refDoc, LanguageNames.CSharp, LanguageNames.CSharp)
};

await VerifyTypeImportItemExistsAsync(
markup,
"Goo",
glyph: (int)Glyph.ClassPublic,
inlineDescription: "Foo");
}

[InlineData(true)]
[InlineData(false)]
[Theory, Trait(Traits.Feature, Traits.Features.Completion)]
public async Task TestBrowsableNeverFromReferences(bool isProjectReference)
{
var srcDoc = @"
class Program
{
void M()
{
$$
}
}";

var refDoc = @"
namespace Foo
{
[System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
public class Goo
{
}
}";

var (markup, shouldContainItem) = isProjectReference switch
{
true => (CreateMarkupForProjectWithProjectReference(srcDoc, refDoc, LanguageNames.CSharp, LanguageNames.CSharp), true),
false => (CreateMarkupForProjectWithMetadataReference(srcDoc, refDoc, LanguageNames.CSharp, LanguageNames.CSharp), false),
};

if (shouldContainItem)
{
await VerifyTypeImportItemExistsAsync(
markup,
"Goo",
glyph: (int)Glyph.ClassPublic,
inlineDescription: "Foo");
}
else
{
await VerifyTypeImportItemIsAbsentAsync(
markup,
"Goo",
inlineDescription: "Foo");
}
}

[InlineData(true, true)]
[InlineData(true, false)]
[InlineData(false, true)]
[InlineData(false, false)]
[Theory, Trait(Traits.Feature, Traits.Features.Completion)]
public async Task TestBrowsableAdvancedFromReferences(bool isProjectReference, bool hideAdvancedMembers)
{
HideAdvancedMembers = hideAdvancedMembers;

var srcDoc = @"
class Program
{
void M()
{
$$
}
}";

var refDoc = @"
namespace Foo
{
[System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Advanced)]
public class Goo
{
}
}";

var (markup, shouldContainItem) = isProjectReference switch
{
true => (CreateMarkupForProjectWithProjectReference(srcDoc, refDoc, LanguageNames.CSharp, LanguageNames.CSharp), true),
false => (CreateMarkupForProjectWithMetadataReference(srcDoc, refDoc, LanguageNames.CSharp, LanguageNames.CSharp), !hideAdvancedMembers),
};

if (shouldContainItem)
{
await VerifyTypeImportItemExistsAsync(
markup,
"Goo",
glyph: (int)Glyph.ClassPublic,
inlineDescription: "Foo");
}
else
{
await VerifyTypeImportItemIsAbsentAsync(
markup,
"Goo",
inlineDescription: "Foo");
}
}

private Task VerifyTypeImportItemExistsAsync(string markup, string expectedItem, int glyph, string inlineDescription, string displayTextSuffix = null, string expectedDescriptionOrNull = null, CompletionItemFlags? flags = null)
=> VerifyItemExistsAsync(markup, expectedItem, displayTextSuffix: displayTextSuffix, glyph: glyph, inlineDescription: inlineDescription, expectedDescriptionOrNull: expectedDescriptionOrNull, flags: flags);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,18 @@ private readonly struct CacheEntry

private ImmutableArray<TypeImportCompletionItemInfo> ItemInfos { get; }

public bool ContainsAdvancedMembers { get; }
genlu marked this conversation as resolved.
Show resolved Hide resolved

private CacheEntry(
Checksum checksum,
string language,
bool containsAdvancedmembers,
ImmutableArray<TypeImportCompletionItemInfo> items)
{
Checksum = checksum;
Language = language;

ContainsAdvancedMembers = containsAdvancedmembers;
ItemInfos = items;
}

Expand All @@ -41,15 +45,19 @@ public ImmutableArray<CompletionItem> GetItemsForContext(
string genericTypeSuffix,
bool isInternalsVisible,
bool isAttributeContext,
bool isCaseSensitive)
bool isCaseSensitive,
bool hideAdvancedMembers)
{
// We will need to adjust some items if the request is made in:
// 1. attribute context, then we will not show or complete with "Attribute" suffix.
// 2. a project with different language than when the cache entry was created,
// then we will change the generic suffix accordingly.
// 3. asked to hide advanced members and there is advanced member in the cache
// Otherwise, we can simply return cached items.
var isSameLanguage = Language == language;
if (isSameLanguage && !isAttributeContext)
if (isSameLanguage &&
!isAttributeContext &&
!(hideAdvancedMembers && ContainsAdvancedMembers))
genlu marked this conversation as resolved.
Show resolved Hide resolved
{
return ItemInfos.Where(info => info.IsPublic || isInternalsVisible).SelectAsArray(info => info.Item);
}
genlu marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -59,6 +67,11 @@ public ImmutableArray<CompletionItem> GetItemsForContext(
{
if (info.IsPublic || isInternalsVisible)
{
if (hideAdvancedMembers && info.IsEditorBrowsableStateAdvanced)
{
continue;
}

var item = info.Item;
if (isAttributeContext)
{
Expand Down Expand Up @@ -99,14 +112,18 @@ public class Builder : IDisposable
private readonly string _language;
private readonly string _genericTypeSuffix;
private readonly Checksum _checksum;
private readonly EditorBrowsableInfo _editorBrowsableInfo;

private readonly ArrayBuilder<TypeImportCompletionItemInfo> _itemsBuilder;

public Builder(Checksum checksum, string language, string genericTypeSuffix)
private bool _containsAdvancedMembers;

public Builder(Checksum checksum, string language, string genericTypeSuffix, EditorBrowsableInfo editorBrowsableInfo)
{
_checksum = checksum;
_language = language;
_genericTypeSuffix = genericTypeSuffix;
_editorBrowsableInfo = editorBrowsableInfo;

_itemsBuilder = ArrayBuilder<TypeImportCompletionItemInfo>.GetInstance();
}
Expand All @@ -116,11 +133,28 @@ public CacheEntry ToReferenceCacheEntry()
return new CacheEntry(
_checksum,
_language,
_containsAdvancedMembers,
_itemsBuilder.ToImmutable());
}

public void AddItem(INamedTypeSymbol symbol, string containingNamespace, bool isPublic)
{
// We want to cache items with EditoBrowsableState == Advanced regarless of current "hide adv members" option value
genlu marked this conversation as resolved.
Show resolved Hide resolved
var (isBrowsable, isEditorBrowsableStateAdvanced) = symbol.IsEditorBrowsableWithState(
hideAdvancedMembers: false,
_editorBrowsableInfo.Compilation,
_editorBrowsableInfo.EditorBrowsableAttributeConstructor,
_editorBrowsableInfo.TypeLibTypeAttributeConstructors,
_editorBrowsableInfo.TypeLibFuncAttributeConstructors,
_editorBrowsableInfo.TypeLibVarAttributeConstructors,
_editorBrowsableInfo.HideModuleNameAttribute);

if (!isBrowsable)
{
// Hide this item from completion
return;
}

var isGeneric = symbol.Arity > 0;

// Need to determine if a type is an attribute up front since we want to filter out
Expand All @@ -140,7 +174,8 @@ public void AddItem(INamedTypeSymbol symbol, string containingNamespace, bool is
CompletionItemFlags.CachedAndExpanded,
extensionMethodData: null);

_itemsBuilder.Add(new TypeImportCompletionItemInfo(item, isPublic, isGeneric, isAttribute));
_containsAdvancedMembers = _containsAdvancedMembers || isEditorBrowsableStateAdvanced;
_itemsBuilder.Add(new TypeImportCompletionItemInfo(item, isPublic, isGeneric, isAttribute, isEditorBrowsableStateAdvanced));
}

public void Dispose()
Expand Down
Loading