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.

Sign up for GitHub

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
Merged

Support EditorBrowsable in completion for unimported types #47771

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2559390
Add tests
genlu Sep 16, 2020
37f0eb9
Only show editor browsable types from unimported namespace
genlu Sep 16, 2020
71ae6c7
Use ImmutableArray and Optional
genlu Sep 17, 2020
b368fe3
Address review comment
genlu Sep 17, 2020
0ddaa26
Merge remote-tracking branch 'dotnet/master' into EditorBrowsableUnim…
genlu Sep 18, 2020
6945e72
Address review comments
genlu Sep 18, 2020
32ec65e
Address review comments
genlu Sep 23, 2020
c2eb3b5
Combine loops
genlu Sep 24, 2020
01ac927
Address review comments
genlu Sep 24, 2020
064b403
Add comments
genlu Sep 24, 2020
4c00f6c
Undo code move
genlu Sep 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
138 changes: 136 additions & 2 deletions ...orFeatures/CSharpTest/Completion/CompletionProviders/TypeImportCompletionProviderTests.cs
View file
Open in desktop
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
79 changes: 50 additions & 29 deletions ...tion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.CacheEntry.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,13 @@
using System;
using System.Collections.Immutable;
using System.Composition;
using System.Linq;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Roslyn.Utilities;

using static Microsoft.CodeAnalysis.Shared.Utilities.EditorBrowsableHelpers;

namespace Microsoft.CodeAnalysis.Completion.Providers.ImportCompletion
{
internal abstract partial class AbstractTypeImportCompletionService
Expand Down Expand Up @@ -41,46 +42,52 @@ 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.
// Otherwise, we can simply return cached items.
var isSameLanguage = Language == language;
if (isSameLanguage && !isAttributeContext)
{
return ItemInfos.Where(info => info.IsPublic || isInternalsVisible).SelectAsArray(info => info.Item);
}
using var _ = ArrayBuilder<CompletionItem>.GetInstance(out var builder);

var builder = ArrayBuilder<CompletionItem>.GetInstance();
foreach (var info in ItemInfos)
{
if (info.IsPublic || isInternalsVisible)
if (!info.IsPublic && !isInternalsVisible)
{
var item = info.Item;
if (isAttributeContext)
{
if (!info.IsAttribute)
{
continue;
}
continue;
}

item = GetAppropriateAttributeItem(info.Item, isCaseSensitive);
}
// Option to show advanced members is false so we need to exclude them.
if (hideAdvancedMembers && info.IsEditorBrowsableStateAdvanced)
{
continue;
}

if (!isSameLanguage && info.IsGeneric)
var item = info.Item;

if (isAttributeContext)
{
// Don't show non attribute item in attribute context
if (!info.IsAttribute)
{
// We don't want to cache this item.
item = ImportCompletionItem.CreateItemWithGenericDisplaySuffix(item, genericTypeSuffix);
continue;
}

builder.Add(item);
// We are in attribute context, will not show or complete with "Attribute" suffix.
item = GetAppropriateAttributeItem(info.Item, isCaseSensitive);
}

// C# and VB the display text is different for generics, i.e. <T> and (Of T). For simpllicity, we only cache for one language.
// But when we trigger in a project with different language than when the cache entry was created for, we will need to
// change the generic suffix accordingly.
if (!isSameLanguage && info.IsGeneric)
{
// We don't want to cache this item.
item = ImportCompletionItem.CreateItemWithGenericDisplaySuffix(item, genericTypeSuffix);
}

builder.Add(item);
}

return builder.ToImmutableAndFree();
return builder.ToImmutable();

static CompletionItem GetAppropriateAttributeItem(CompletionItem attributeItem, bool isCaseSensitive)
{
Expand All @@ -99,14 +106,16 @@ 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)
public Builder(Checksum checksum, string language, string genericTypeSuffix, EditorBrowsableInfo editorBrowsableInfo)
{
_checksum = checksum;
_language = language;
_genericTypeSuffix = genericTypeSuffix;
_editorBrowsableInfo = editorBrowsableInfo;

_itemsBuilder = ArrayBuilder<TypeImportCompletionItemInfo>.GetInstance();
}
Expand All @@ -121,6 +130,18 @@ public CacheEntry ToReferenceCacheEntry()

public void AddItem(INamedTypeSymbol symbol, string containingNamespace, bool isPublic)
{
// We want to cache items with EditoBrowsableState == Advanced regardless of current "hide adv members" option value
var (isBrowsable, isEditorBrowsableStateAdvanced) = symbol.IsEditorBrowsableWithState(
hideAdvancedMembers: false,
_editorBrowsableInfo.Compilation,
_editorBrowsableInfo);

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 +161,7 @@ public void AddItem(INamedTypeSymbol symbol, string containingNamespace, bool is
CompletionItemFlags.CachedAndExpanded,
extensionMethodData: null);

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

public void Dispose()
Expand Down
Loading