Skip to content

Commit

Permalink
Filter out RequiredMemberAttribute from metadata symbols (#63497)
Browse files Browse the repository at this point in the history 
Fixes #61511.
  • Loading branch information
@333fred
333fred authored Aug 25, 2022
1 parent e379ce9 commit 5b4dc71
Show file tree
Hide file tree
Showing 9 changed files with 168 additions and 37 deletions.
23 changes: 9 additions & 14 deletions src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEFieldSymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -581,20 +581,15 @@ public override ImmutableArray<CSharpAttributeData> GetAttributes()
{
var containingPEModuleSymbol = (PEModuleSymbol)this.ContainingModule;

if (FilterOutDecimalConstantAttribute())
{
// filter out DecimalConstantAttribute
var attributes = containingPEModuleSymbol.GetCustomAttributesForToken(
_handle,
out _,
AttributeDescription.DecimalConstantAttribute);

ImmutableInterlocked.InterlockedInitialize(ref _lazyCustomAttributes, attributes);
}
else
{
containingPEModuleSymbol.LoadCustomAttributes(_handle, ref _lazyCustomAttributes);
}
var attributes = containingPEModuleSymbol.GetCustomAttributesForToken(
_handle,
out _,
FilterOutDecimalConstantAttribute() ? AttributeDescription.DecimalConstantAttribute : default,
out CustomAttributeHandle required,
AttributeDescription.RequiredMemberAttribute);

ImmutableInterlocked.InterlockedInitialize(ref _lazyCustomAttributes, attributes);
_packedFlags.SetHasRequiredMemberAttribute(!required.IsNil);
}
return _lazyCustomAttributes;
}
Expand Down
4 changes: 3 additions & 1 deletion src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -982,7 +982,9 @@ public override ImmutableArray<CSharpAttributeData> GetAttributes()
filteredOutAttribute4: out _,
filterOut4: (checkForRequiredMembers && ObsoleteAttributeData is null) ? AttributeDescription.ObsoleteAttribute : default,
filteredOutAttribute5: out _,
filterOut5: default);
filterOut5: default,
filteredOutAttribute6: out _,
filterOut6: default);

isExtensionMethod = !extensionAttribute.IsNil;
isReadOnly = !isReadOnlyAttribute.IsNil;
Expand Down
29 changes: 22 additions & 7 deletions src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEModuleSymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -294,11 +294,20 @@ internal ImmutableArray<CSharpAttributeData> GetCustomAttributesForToken(EntityH
out CustomAttributeHandle filteredOutAttribute1,
AttributeDescription filterOut1)
{
return GetCustomAttributesForToken(token, out filteredOutAttribute1, filterOut1, out _, default, out _, default, out _, default, out _, default);
return GetCustomAttributesForToken(token, out filteredOutAttribute1, filterOut1, out _, default, out _, default, out _, default, out _, default, out _, default);
}

internal ImmutableArray<CSharpAttributeData> GetCustomAttributesForToken(EntityHandle token,
out CustomAttributeHandle filteredOutAttribute1,
AttributeDescription filterOut1,
out CustomAttributeHandle filteredOutAttribute2,
AttributeDescription filterOut2)
{
return GetCustomAttributesForToken(token, out filteredOutAttribute1, filterOut1, out filteredOutAttribute2, filterOut2, out _, default, out _, default, out _, default, out _, default);
}

/// <summary>
/// Returns attributes with up-to four filters applied. For each filter, the last application of the
/// Returns attributes with up-to 6 filters applied. For each filter, the last application of the
/// attribute will be tracked and returned.
/// </summary>
internal ImmutableArray<CSharpAttributeData> GetCustomAttributesForToken(EntityHandle token,
Expand All @@ -311,13 +320,16 @@ internal ImmutableArray<CSharpAttributeData> GetCustomAttributesForToken(EntityH
out CustomAttributeHandle filteredOutAttribute4,
AttributeDescription filterOut4,
out CustomAttributeHandle filteredOutAttribute5,
AttributeDescription filterOut5)
AttributeDescription filterOut5,
out CustomAttributeHandle filteredOutAttribute6,
AttributeDescription filterOut6)
{
filteredOutAttribute1 = default;
filteredOutAttribute2 = default;
filteredOutAttribute3 = default;
filteredOutAttribute4 = default;
filteredOutAttribute5 = default;
filteredOutAttribute6 = default;
ArrayBuilder<CSharpAttributeData> customAttributesBuilder = null;

try
Expand Down Expand Up @@ -357,6 +369,12 @@ internal ImmutableArray<CSharpAttributeData> GetCustomAttributesForToken(EntityH
continue;
}

if (matchesFilter(customAttributeHandle, filterOut6))
{
filteredOutAttribute6 = customAttributeHandle;
continue;
}

if (customAttributesBuilder == null)
{
customAttributesBuilder = ArrayBuilder<CSharpAttributeData>.GetInstance();
Expand Down Expand Up @@ -436,10 +454,7 @@ internal ImmutableArray<CSharpAttributeData> GetCustomAttributesFilterCompilerAt
filteredOutAttribute1: out CustomAttributeHandle extensionAttribute,
filterOut1: AttributeDescription.CaseSensitiveExtensionAttribute,
filteredOutAttribute2: out CustomAttributeHandle isReadOnlyAttribute,
filterOut2: AttributeDescription.IsReadOnlyAttribute,
filteredOutAttribute3: out _, filterOut3: default,
filteredOutAttribute4: out _, filterOut4: default,
filteredOutAttribute5: out _, filterOut5: default);
filterOut2: AttributeDescription.IsReadOnlyAttribute);

foundExtension = !extensionAttribute.IsNil;
foundReadOnly = !isReadOnlyAttribute.IsNil;
Expand Down
12 changes: 11 additions & 1 deletion src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -680,9 +680,19 @@ public override ImmutableArray<CSharpAttributeData> GetAttributes()
IsRefLikeType ? AttributeDescription.IsByRefLikeAttribute : default,
out _,
// Filter out [CompilerFeatureRequired]
(IsRefLikeType && DeriveCompilerFeatureRequiredDiagnostic() is null) ? AttributeDescription.CompilerFeatureRequiredAttribute : default);
(IsRefLikeType && DeriveCompilerFeatureRequiredDiagnostic() is null) ? AttributeDescription.CompilerFeatureRequiredAttribute : default,
out CustomAttributeHandle requiredHandle,
// Filter out [RequiredMember]
AttributeDescription.RequiredMemberAttribute);

ImmutableInterlocked.InterlockedInitialize(ref uncommon.lazyCustomAttributes, loadedCustomAttributes);

if (!uncommon.lazyHasRequiredMembers.HasValue())
{
uncommon.lazyHasRequiredMembers = (!requiredHandle.IsNil).ToThreeState();
}

Debug.Assert(uncommon.lazyHasRequiredMembers.Value() != requiredHandle.IsNil);
}

return uncommon.lazyCustomAttributes;
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1040,6 +1040,8 @@ public override ImmutableArray<CSharpAttributeData> GetAttributes()
out _,
AttributeDescription.ScopedRefAttribute,
out _,
default,
out _,
default);

if (!paramArrayAttribute.IsNil || !constantAttribute.IsNil)
Expand Down
5 changes: 4 additions & 1 deletion src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -642,9 +642,12 @@ public override ImmutableArray<CSharpAttributeData> GetAttributes()
ImmutableArray<CSharpAttributeData> attributes = containingPEModuleSymbol.GetCustomAttributesForToken(
_handle,
out _,
this.RefKind == RefKind.RefReadOnly ? AttributeDescription.IsReadOnlyAttribute : default);
this.RefKind == RefKind.RefReadOnly ? AttributeDescription.IsReadOnlyAttribute : default,
out CustomAttributeHandle required,
AttributeDescription.RequiredMemberAttribute);

ImmutableInterlocked.InterlockedInitialize(ref _lazyCustomAttributes, attributes);
_flags.SetHasRequiredMemberAttribute(!required.IsNil);
}
return _lazyCustomAttributes;
}
Expand Down
70 changes: 62 additions & 8 deletions src/Compilers/CSharp/Test/Symbol/Symbols/RequiredMembersTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,7 @@ private static Action<ModuleSymbol> ValidateRequiredMembersInModule(string[] mem
AssertEx.NotNull(member, $"Member {memberPath} was not found");
Assert.True(member is PropertySymbol or FieldSymbol, $"Unexpected member symbol type {member.Kind}");
Assert.True(member.IsRequired());
if (module is SourceModuleSymbol)
{
Assert.All(member.GetAttributes(), attr => AssertEx.NotEqual("System.Runtime.CompilerServices.RequiredMemberAttribute", attr.AttributeClass.ToTestDisplayString()));
}
else
{
AssertEx.Any(member.GetAttributes(), attr => attr.AttributeClass.ToTestDisplayString() == "System.Runtime.CompilerServices.RequiredMemberAttribute");
}
Assert.All(member.GetAttributes(), attr => AssertEx.NotEqual("System.Runtime.CompilerServices.RequiredMemberAttribute", attr.AttributeClass.ToTestDisplayString()));
requiredTypes.Add((NamedTypeSymbol)member.ContainingType);
}
Expand Down Expand Up @@ -4953,6 +4946,67 @@ class C
// public required int Test { get; set; }
Diagnostic(ErrorCode.ERR_DuplicateNameInClass, "Test").WithArguments("C", "Test").WithLocation(4, 25)
);
}

[Theory, CombinatorialData]
public void FirstAccessOfIsRequiredDoesNotMatter(bool accessAttributesFirst)
{
// Accessing attributes will populate IsRequired if it's not already populated, so we want to test both codepaths explicitly.
var comp = CreateCompilationWithRequiredMembers("""
public class C
{
public required int Field1;
public required int Property1 { get; set; }
}

public class D
{
public int Field2;
public int Property2 { get; set; }
}
""");

CompileAndVerify(comp, symbolValidator: module =>
{
var c = module.ContainingAssembly.GetTypeByMetadataName("C");
AssertEx.NotNull(c);
FieldSymbol field1 = c.GetMember<FieldSymbol>("Field1");
PropertySymbol property1 = c.GetMember<PropertySymbol>("Property1");
var d = module.ContainingAssembly.GetTypeByMetadataName("D");
AssertEx.NotNull(d);
FieldSymbol field2 = d.GetMember<FieldSymbol>("Field2");
PropertySymbol property2 = d.GetMember<PropertySymbol>("Property2");
if (accessAttributesFirst)
{
assertAttributesEmpty();
assertIsRequired();
}
else
{
assertIsRequired();
assertAttributesEmpty();
}
void assertIsRequired()
{
Assert.True(c.HasDeclaredRequiredMembers);
Assert.True(field1.IsRequired);
Assert.True(property1.IsRequired);
Assert.False(d.HasDeclaredRequiredMembers);
Assert.False(field2.IsRequired);
Assert.False(property2.IsRequired);
}
void assertAttributesEmpty()
{
Assert.Empty(c.GetAttributes());
Assert.Empty(field1.GetAttributes());
Assert.Empty(property1.GetAttributes());
Assert.Empty(d.GetAttributes());
Assert.Empty(field2.GetAttributes());
Assert.Empty(property2.GetAttributes());
}
});
}
}
45 changes: 45 additions & 0 deletions src/Compilers/Test/Utilities/CSharp/RequiredMemberAttributesVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// See the LICENSE file in the project root for more information.

using System.Collections.Immutable;
using System.Reflection.Metadata;
using System.Text;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.CSharp.Symbols.Metadata.PE;
Expand All @@ -25,6 +26,50 @@ private RequiredMemberAttributesVisitor(StringBuilder builder) : base(builder)

protected override SymbolDisplayFormat DisplayFormat => SymbolDisplayFormat.TestFormat;

protected override void ReportSymbol(Symbol symbol)
{
EntityHandle handle;
PEModule module;

switch (symbol)
{
case PENamedTypeSymbol namedType:
handle = namedType.Handle;
module = ((PEModuleSymbol)namedType.ContainingModule).Module;
break;

case PEFieldSymbol field:
handle = field.Handle;
module = ((PEModuleSymbol)field.ContainingModule).Module;
break;

case PEPropertySymbol property:
handle = property.Handle;
module = ((PEModuleSymbol)property.ContainingModule).Module;
break;

default:
base.ReportSymbol(symbol);
return;
}

var attribute = module.GetAttributeHandle(handle, AttributeDescription.RequiredMemberAttribute);

if (attribute.IsNil)
{
return;
}

ReportContainingSymbols(symbol);
_builder.Append(GetIndentString(symbol));
_builder.Append("[RequiredMember] ");
_builder.AppendLine(symbol.ToDisplayString(DisplayFormat));
_reported.Add(symbol);

// If attributes aren't filtered out, this will print extra data and cause an error in test assertion.
base.ReportSymbol(symbol);
}

protected override CSharpAttributeData? GetTargetAttribute(ImmutableArray<CSharpAttributeData> attributes)
=> GetAttribute(attributes, "System.Runtime.CompilerServices", "RequiredMemberAttribute");

Expand Down
15 changes: 10 additions & 5 deletions src/EditorFeatures/Test/MetadataAsSource/MetadataAsSourceTests.CSharp.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -758,7 +758,11 @@ public class C
public async Task TestRequiredProperty(bool signaturesOnly)
{
var metadataSource = """
public class C { public required int Property { get; set; } }
public class C
{
public required int Property { get; set; }
public required int Field;
}
namespace System.Runtime.CompilerServices
{
public sealed class RequiredMemberAttribute : Attribute { }
Expand All @@ -783,14 +787,12 @@ public CompilerFeatureRequiredAttribute(string featureName)
// {CodeAnalysisResources.InMemoryAssembly}
#endregion
using System.Runtime.CompilerServices;
[RequiredMember]
public class [|C|]
{{
public required int Field;
public C();
[RequiredMember]
public required int Property {{ get; set; }}
}}",
false => $@"#region {FeaturesResources.Assembly} ReferencedAssembly, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null
Expand All @@ -804,6 +806,9 @@ public class [|C|]
[RequiredMember]
public class [|C|]
{{
[RequiredMember]
public int Field;
[RequiredMember]
public int Property {{ get; set; }}
Expand Down

0 comments on commit 5b4dc71

Please sign in to comment.