From 5b4dc7134d51a1010616d1f4cb15590a8a911218 Mon Sep 17 00:00:00 2001 From: Fred Silberberg Date: Thu, 25 Aug 2022 15:21:56 -0700 Subject: [PATCH] Filter out `RequiredMemberAttribute` from metadata symbols (#63497) Fixes https://github.com/dotnet/roslyn/issues/61511. --- .../Symbols/Metadata/PE/PEFieldSymbol.cs | 23 +++--- .../Symbols/Metadata/PE/PEMethodSymbol.cs | 4 +- .../Symbols/Metadata/PE/PEModuleSymbol.cs | 29 ++++++-- .../Symbols/Metadata/PE/PENamedTypeSymbol.cs | 12 +++- .../Symbols/Metadata/PE/PEParameterSymbol.cs | 2 + .../Symbols/Metadata/PE/PEPropertySymbol.cs | 5 +- .../Symbol/Symbols/RequiredMembersTests.cs | 70 ++++++++++++++++--- .../CSharp/RequiredMemberAttributesVisitor.cs | 45 ++++++++++++ .../MetadataAsSourceTests.CSharp.cs | 15 ++-- 9 files changed, 168 insertions(+), 37 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEFieldSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEFieldSymbol.cs index b3e9d89b6c80e..6c5843faa621a 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEFieldSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEFieldSymbol.cs @@ -581,20 +581,15 @@ public override ImmutableArray 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; } diff --git a/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs index 4da3ee989e5c5..57acf3592cd9c 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs @@ -982,7 +982,9 @@ public override ImmutableArray 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; diff --git a/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEModuleSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEModuleSymbol.cs index ff1f0d5a8cc13..2c1292bca6f4a 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEModuleSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEModuleSymbol.cs @@ -294,11 +294,20 @@ internal ImmutableArray 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 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); } /// - /// 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. /// internal ImmutableArray GetCustomAttributesForToken(EntityHandle token, @@ -311,13 +320,16 @@ internal ImmutableArray 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 customAttributesBuilder = null; try @@ -357,6 +369,12 @@ internal ImmutableArray GetCustomAttributesForToken(EntityH continue; } + if (matchesFilter(customAttributeHandle, filterOut6)) + { + filteredOutAttribute6 = customAttributeHandle; + continue; + } + if (customAttributesBuilder == null) { customAttributesBuilder = ArrayBuilder.GetInstance(); @@ -436,10 +454,7 @@ internal ImmutableArray 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; diff --git a/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs index b7aa01e6898ab..5e52a4f88f75c 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs @@ -680,9 +680,19 @@ public override ImmutableArray 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; diff --git a/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs index b936413e260d1..104979207ba6c 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs @@ -1040,6 +1040,8 @@ public override ImmutableArray GetAttributes() out _, AttributeDescription.ScopedRefAttribute, out _, + default, + out _, default); if (!paramArrayAttribute.IsNil || !constantAttribute.IsNil) diff --git a/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs index 14224e42c3994..57f6f9695c9b7 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs @@ -642,9 +642,12 @@ public override ImmutableArray GetAttributes() ImmutableArray 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; } diff --git a/src/Compilers/CSharp/Test/Symbol/Symbols/RequiredMembersTests.cs b/src/Compilers/CSharp/Test/Symbol/Symbols/RequiredMembersTests.cs index df787d86012e3..e9fafc59148b8 100644 --- a/src/Compilers/CSharp/Test/Symbol/Symbols/RequiredMembersTests.cs +++ b/src/Compilers/CSharp/Test/Symbol/Symbols/RequiredMembersTests.cs @@ -56,14 +56,7 @@ private static Action 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); } @@ -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("Field1"); + PropertySymbol property1 = c.GetMember("Property1"); + var d = module.ContainingAssembly.GetTypeByMetadataName("D"); + AssertEx.NotNull(d); + FieldSymbol field2 = d.GetMember("Field2"); + PropertySymbol property2 = d.GetMember("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()); + } + }); } } diff --git a/src/Compilers/Test/Utilities/CSharp/RequiredMemberAttributesVisitor.cs b/src/Compilers/Test/Utilities/CSharp/RequiredMemberAttributesVisitor.cs index 00c93aa972ba1..cf5d1f0023457 100644 --- a/src/Compilers/Test/Utilities/CSharp/RequiredMemberAttributesVisitor.cs +++ b/src/Compilers/Test/Utilities/CSharp/RequiredMemberAttributesVisitor.cs @@ -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; @@ -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 attributes) => GetAttribute(attributes, "System.Runtime.CompilerServices", "RequiredMemberAttribute"); diff --git a/src/EditorFeatures/Test/MetadataAsSource/MetadataAsSourceTests.CSharp.cs b/src/EditorFeatures/Test/MetadataAsSource/MetadataAsSourceTests.CSharp.cs index 1ee1da581f388..a340489bb147b 100644 --- a/src/EditorFeatures/Test/MetadataAsSource/MetadataAsSourceTests.CSharp.cs +++ b/src/EditorFeatures/Test/MetadataAsSource/MetadataAsSourceTests.CSharp.cs @@ -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 { } @@ -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 @@ -804,6 +806,9 @@ public class [|C|] [RequiredMember] public class [|C|] {{ + [RequiredMember] + public int Field; + [RequiredMember] public int Property {{ get; set; }}