From f99ba2e2b8fbf03be5058ad23e8cdce9c4a09da6 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Fri, 11 Mar 2022 13:12:20 -0800 Subject: [PATCH] Revert "Avoid Attribute.GetCustomAttributes() returning null for open generic type (#65237)" (#66508) This reverts commit 20294957616061616e672b43f1d962460b7f4234. --- .../src/System/Attribute.CoreCLR.cs | 16 +-- .../Reflection/RuntimeCustomAttributeData.cs | 64 ++++------- .../System/Reflection/RuntimeParameterInfo.cs | 6 +- .../src/System/Reflection/Attribute.CoreRT.cs | 16 ++- .../tests/ParameterInfoTests.cs | 8 -- .../System.Runtime/tests/System/Attributes.cs | 100 +----------------- .../GenericAttributeMetadata.cs | 2 - .../GenericAttribute/GenericAttributeTests.cs | 14 +-- 8 files changed, 56 insertions(+), 170 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Attribute.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Attribute.CoreCLR.cs index c84e918d84303..f33849d2a949e 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Attribute.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Attribute.CoreCLR.cs @@ -434,8 +434,10 @@ private static AttributeUsageAttribute InternalGetAttributeUsage(Type type) SR.Format(SR.Format_AttributeUsage, type)); } - private static Attribute[] CreateAttributeArrayHelper(Type elementType, int elementCount) => - elementType.ContainsGenericParameters ? new Attribute[elementCount] : (Attribute[])Array.CreateInstance(elementType, elementCount); + private static Attribute[] CreateAttributeArrayHelper(Type elementType, int elementCount) + { + return (Attribute[])Array.CreateInstance(elementType, elementCount); + } #endregion #endregion @@ -457,7 +459,7 @@ public static Attribute[] GetCustomAttributes(MemberInfo element!!, Type attribu { MemberTypes.Property => InternalGetCustomAttributes((PropertyInfo)element, attributeType, inherit), MemberTypes.Event => InternalGetCustomAttributes((EventInfo)element, attributeType, inherit), - _ => (Attribute[])element.GetCustomAttributes(attributeType, inherit) + _ => (element.GetCustomAttributes(attributeType, inherit) as Attribute[])!, }; } @@ -472,7 +474,7 @@ public static Attribute[] GetCustomAttributes(MemberInfo element!!, bool inherit { MemberTypes.Property => InternalGetCustomAttributes((PropertyInfo)element, typeof(Attribute), inherit), MemberTypes.Event => InternalGetCustomAttributes((EventInfo)element, typeof(Attribute), inherit), - _ => (Attribute[])element.GetCustomAttributes(typeof(Attribute), inherit) + _ => (element.GetCustomAttributes(typeof(Attribute), inherit) as Attribute[])!, }; } @@ -534,11 +536,12 @@ public static Attribute[] GetCustomAttributes(ParameterInfo element!!, Type attr if (element.Member == null) throw new ArgumentException(SR.Argument_InvalidParameterInfo, nameof(element)); + MemberInfo member = element.Member; if (member.MemberType == MemberTypes.Method && inherit) return InternalParamGetCustomAttributes(element, attributeType, inherit); - return (Attribute[])element.GetCustomAttributes(attributeType, inherit); + return (element.GetCustomAttributes(attributeType, inherit) as Attribute[])!; } public static Attribute[] GetCustomAttributes(ParameterInfo element!!, bool inherit) @@ -546,11 +549,12 @@ public static Attribute[] GetCustomAttributes(ParameterInfo element!!, bool inhe if (element.Member == null) throw new ArgumentException(SR.Argument_InvalidParameterInfo, nameof(element)); + MemberInfo member = element.Member; if (member.MemberType == MemberTypes.Method && inherit) return InternalParamGetCustomAttributes(element, null, inherit); - return (Attribute[])element.GetCustomAttributes(typeof(Attribute), inherit); + return (element.GetCustomAttributes(typeof(Attribute), inherit) as Attribute[])!; } public static bool IsDefined(ParameterInfo element, Type attributeType) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs index d28edee240381..bb642f913da71 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs @@ -899,7 +899,7 @@ internal static object[] GetCustomAttributes(RuntimeType type, RuntimeType caTyp Debug.Assert(caType is not null); if (type.GetElementType() is not null) - return CreateAttributeArrayHelper(caType, 0); + return (caType.IsValueType) ? Array.Empty() : CreateAttributeArrayHelper(caType, 0); if (type.IsGenericType && !type.IsGenericTypeDefinition) type = (type.GetGenericTypeDefinition() as RuntimeType)!; @@ -919,6 +919,8 @@ internal static object[] GetCustomAttributes(RuntimeType type, RuntimeType caTyp RuntimeType.ListBuilder result = default; bool mustBeInheritable = false; + bool useObjectArray = (caType.IsValueType || caType.ContainsGenericParameters); + RuntimeType arrayType = useObjectArray ? (RuntimeType)typeof(object) : caType; for (int i = 0; i < pcas.Count; i++) result.Add(pcas[i]); @@ -930,7 +932,7 @@ internal static object[] GetCustomAttributes(RuntimeType type, RuntimeType caTyp type = (type.BaseType as RuntimeType)!; } - object[] typedResult = CreateAttributeArrayHelper(caType, result.Count); + object[] typedResult = CreateAttributeArrayHelper(arrayType, result.Count); for (int i = 0; i < result.Count; i++) { typedResult[i] = result[i]; @@ -961,6 +963,8 @@ internal static object[] GetCustomAttributes(RuntimeMethodInfo method, RuntimeTy RuntimeType.ListBuilder result = default; bool mustBeInheritable = false; + bool useObjectArray = (caType.IsValueType || caType.ContainsGenericParameters); + RuntimeType arrayType = useObjectArray ? (RuntimeType)typeof(object) : caType; for (int i = 0; i < pcas.Count; i++) result.Add(pcas[i]); @@ -972,7 +976,7 @@ internal static object[] GetCustomAttributes(RuntimeMethodInfo method, RuntimeTy method = method.GetParentDefinition()!; } - object[] typedResult = CreateAttributeArrayHelper(caType, result.Count); + object[] typedResult = CreateAttributeArrayHelper(arrayType, result.Count); for (int i = 0; i < result.Count; i++) { typedResult[i] = result[i]; @@ -1119,13 +1123,16 @@ private static bool IsCustomAttributeDefined( } private static object[] GetCustomAttributes( - RuntimeModule decoratedModule, int decoratedMetadataToken, int pcaCount, RuntimeType attributeFilterType) + RuntimeModule decoratedModule, int decoratedMetadataToken, int pcaCount, RuntimeType? attributeFilterType) { RuntimeType.ListBuilder attributes = default; AddCustomAttributes(ref attributes, decoratedModule, decoratedMetadataToken, attributeFilterType, false, default); - object[] result = CreateAttributeArrayHelper(attributeFilterType, attributes.Count + pcaCount); + bool useObjectArray = attributeFilterType is null || attributeFilterType.IsValueType || attributeFilterType.ContainsGenericParameters; + RuntimeType arrayType = useObjectArray ? (RuntimeType)typeof(object) : attributeFilterType!; + + object[] result = CreateAttributeArrayHelper(arrayType, attributes.Count + pcaCount); for (int i = 0; i < attributes.Count; i++) { result[i] = attributes[i]; @@ -1432,42 +1439,6 @@ internal static AttributeUsageAttribute GetAttributeUsage(RuntimeType decoratedA return attributeUsageAttribute ?? AttributeUsageAttribute.Default; } - - internal static object[] CreateAttributeArrayHelper(RuntimeType caType, int elementCount) - { - bool useAttributeArray = false; - bool useObjectArray = false; - - if (caType == typeof(Attribute)) - { - useAttributeArray = true; - } - else if (caType.IsValueType) - { - useObjectArray = true; - } - else if (caType.ContainsGenericParameters) - { - if (caType.IsSubclassOf(typeof(Attribute))) - { - useAttributeArray = true; - } - else - { - useObjectArray = true; - } - } - - if (useAttributeArray) - { - return elementCount == 0 ? Array.Empty() : new Attribute[elementCount]; - } - if (useObjectArray) - { - return elementCount == 0 ? Array.Empty() : new object[elementCount]; - } - return elementCount == 0 ? caType.GetEmptyArray() : (object[])Array.CreateInstance(caType, elementCount); - } #endregion #region Private Static FCalls @@ -1505,6 +1476,17 @@ private static void GetPropertyOrFieldData( module, &pBlobStart, (byte*)blobEnd, out name, out isProperty, out type, out value); blobStart = (IntPtr)pBlobStart; } + + private static object[] CreateAttributeArrayHelper(RuntimeType elementType, int elementCount) + { + // If we have 0 elements, don't allocate a new array + if (elementCount == 0) + { + return elementType.GetEmptyArray(); + } + + return (object[])Array.CreateInstance(elementType, elementCount); + } #endregion } diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeParameterInfo.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeParameterInfo.cs index f889133b531f9..33777c745dbfc 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeParameterInfo.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeParameterInfo.cs @@ -509,12 +509,12 @@ public override object[] GetCustomAttributes(bool inherit) public override object[] GetCustomAttributes(Type attributeType!!, bool inherit) { + if (MdToken.IsNullToken(m_tkParamDef)) + return Array.Empty(); + if (attributeType.UnderlyingSystemType is not RuntimeType attributeRuntimeType) throw new ArgumentException(SR.Arg_MustBeType, nameof(attributeType)); - if (MdToken.IsNullToken(m_tkParamDef)) - return CustomAttribute.CreateAttributeArrayHelper(attributeRuntimeType, 0); - return CustomAttribute.GetCustomAttributes(this, attributeRuntimeType); } diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Attribute.CoreRT.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Attribute.CoreRT.cs index f7604612b5ad5..d7f9bb693ea03 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Attribute.CoreRT.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Attribute.CoreRT.cs @@ -141,9 +141,19 @@ private static Attribute[] Instantiate(IEnumerable cads, Ty attributes.Add(instantiatedAttribute); } int count = attributes.Count; - Attribute[] result = actualElementType.ContainsGenericParameters - ? new Attribute[count] - : (Attribute[])Array.CreateInstance(actualElementType, count); + Attribute[] result; + try + { + result = (Attribute[])Array.CreateInstance(actualElementType, count); + } + catch (NotSupportedException) when (actualElementType.ContainsGenericParameters) + { + // This is here for desktop compatibility (using try-catch as control flow to avoid slowing down the mainline case.) + // GetCustomAttributes() normally returns an array of the exact attribute type requested except when + // the requested type is an open type. Its ICustomAttributeProvider counterpart would return an Object[] array but that's + // not possible with this api's return type so it returns null instead. + return null; + } attributes.CopyTo(result, 0); return result; } diff --git a/src/libraries/System.Reflection/tests/ParameterInfoTests.cs b/src/libraries/System.Reflection/tests/ParameterInfoTests.cs index 26d2d65867f69..f6285546a8713 100644 --- a/src/libraries/System.Reflection/tests/ParameterInfoTests.cs +++ b/src/libraries/System.Reflection/tests/ParameterInfoTests.cs @@ -245,14 +245,6 @@ public void CustomAttributesInheritanceTest(int paramIndex, bool exists, int exp Assert.Equal(expectedMyAttributeValue, exists ? myAttribute.Value : expectedMyAttributeValue); } - [Fact] - public static void GetCustomAttributesOnParameterWithNullMetadataTokenReturnsArrayOfCorrectType() - { - var parameterWithNullMetadataToken = typeof(int[]).GetProperty(nameof(Array.Length)).GetMethod.ReturnParameter; - Assert.Equal(typeof(Attribute[]), Attribute.GetCustomAttributes(parameterWithNullMetadataToken).GetType()); - Assert.Equal(typeof(MyAttribute[]), Attribute.GetCustomAttributes(parameterWithNullMetadataToken, typeof(MyAttribute)).GetType()); - } - [Fact] public void VerifyGetCustomAttributesData() { diff --git a/src/libraries/System.Runtime/tests/System/Attributes.cs b/src/libraries/System.Runtime/tests/System/Attributes.cs index 3dda3f56b3566..e219d30a2bef4 100644 --- a/src/libraries/System.Runtime/tests/System/Attributes.cs +++ b/src/libraries/System.Runtime/tests/System/Attributes.cs @@ -20,7 +20,7 @@ using System.Runtime.InteropServices; using Xunit; -[module: Debuggable(true, false)] +[module:Debuggable(true,false)] namespace System.Tests { public class AttributeIsDefinedTests @@ -218,71 +218,6 @@ public static void GetCustomAttributes_Interface() { Assert.True(typeof(ExampleWithAttribute).GetCustomAttributes(typeof(INameable), inherit: false)[0] is NameableAttribute); } - - [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/56887)", TestRuntimes.Mono)] - public static void GetCustomAttributesWorksWithOpenAndClosedGenericTypesForType() - { - GenericAttributesTestHelper(t => Attribute.GetCustomAttributes(typeof(HasGenericAttribute), t)); - } - - [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/56887)", TestRuntimes.Mono)] - public static void GetCustomAttributesWorksWithOpenAndClosedGenericTypesForField() - { - FieldInfo field = typeof(HasGenericAttribute).GetField(nameof(HasGenericAttribute.Field), BindingFlags.NonPublic | BindingFlags.Instance); - GenericAttributesTestHelper(t => Attribute.GetCustomAttributes(field, t)); - } - - [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/56887)", TestRuntimes.Mono)] - public static void GetCustomAttributesWorksWithOpenAndClosedGenericTypesForConstructor() - { - ConstructorInfo method = typeof(HasGenericAttribute).GetConstructor(Type.EmptyTypes); - GenericAttributesTestHelper(t => Attribute.GetCustomAttributes(method, t)); - } - - [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/56887)", TestRuntimes.Mono)] - public static void GetCustomAttributesWorksWithOpenAndClosedGenericTypesForMethod() - { - MethodInfo method = typeof(HasGenericAttribute).GetMethod(nameof(HasGenericAttribute.Method)); - GenericAttributesTestHelper(t => Attribute.GetCustomAttributes(method, t)); - } - - [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/56887)", TestRuntimes.Mono)] - public static void GetCustomAttributesWorksWithOpenAndClosedGenericTypesForParameter() - { - ParameterInfo parameter = typeof(HasGenericAttribute).GetMethod(nameof(HasGenericAttribute.Method)).GetParameters()[0]; - GenericAttributesTestHelper(t => Attribute.GetCustomAttributes(parameter, t)); - } - - [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/56887)", TestRuntimes.Mono)] - public static void GetCustomAttributesWorksWithOpenAndClosedGenericTypesForProperty() - { - PropertyInfo property = typeof(HasGenericAttribute).GetProperty(nameof(HasGenericAttribute.Property)); - GenericAttributesTestHelper>(t => Attribute.GetCustomAttributes(property, t)); - } - - [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/56887)", TestRuntimes.Mono)] - public static void GetCustomAttributesWorksWithOpenAndClosedGenericTypesForEvent() - { - EventInfo @event = typeof(HasGenericAttribute).GetEvent(nameof(HasGenericAttribute.Event)); - GenericAttributesTestHelper(t => Attribute.GetCustomAttributes(@event, t)); - } - - private static void GenericAttributesTestHelper(Func getCustomAttributes) - { - Attribute[] openGenericAttributes = getCustomAttributes(typeof(GenericAttribute<>)); - Assert.Empty(openGenericAttributes); - - Attribute[] closedGenericAttributes = getCustomAttributes(typeof(GenericAttribute)); - Assert.Equal(1, closedGenericAttributes.Length); - Assert.Equal(typeof(GenericAttribute[]), closedGenericAttributes.GetType()); - } } public static class GetCustomAttribute @@ -291,7 +226,7 @@ public static class GetCustomAttribute [Fact] public static void customAttributeCount() { - List customAttributes = typeof(GetCustomAttribute).Module.CustomAttributes.ToList(); + List customAttributes = typeof(GetCustomAttribute).Module.CustomAttributes.ToList(); // [System.Security.UnverifiableCodeAttribute()] // [TestAttributes.FooAttribute()] // [TestAttributes.ComplicatedAttribute((Int32)1, Stuff = 2)] @@ -725,7 +660,7 @@ public override object TypeId } public class BaseClass { - public virtual void TestMethod([ArgumentUsage("for test")] string[] strArray, params string[] strList) + public virtual void TestMethod([ArgumentUsage("for test")]string[] strArray, params string[] strList) { } } @@ -881,7 +816,7 @@ public interface INameable string Name { get; } } - [AttributeUsage(AttributeTargets.All, AllowMultiple = true)] + [AttributeUsage (AttributeTargets.All, AllowMultiple = true)] public class NameableAttribute : Attribute, INameable { string INameable.Name => "Nameable"; @@ -889,31 +824,4 @@ public class NameableAttribute : Attribute, INameable [Nameable] public class ExampleWithAttribute { } - - public class GenericAttribute : Attribute - { - } - - [GenericAttribute] - public class HasGenericAttribute - { - [GenericAttribute] - internal bool Field; - - [GenericAttribute] - public HasGenericAttribute() { } - - [GenericAttribute] - public void Method([GenericAttribute] int parameter) - { - this.Field = true; - this.Event += () => { }; - } - - [GenericAttribute>] - public int Property { get; set; } - - [GenericAttribute] - public event Action Event; - } } diff --git a/src/tests/reflection/GenericAttribute/GenericAttributeMetadata.cs b/src/tests/reflection/GenericAttribute/GenericAttributeMetadata.cs index 3bea1e621945c..d0ef6353f4a8f 100644 --- a/src/tests/reflection/GenericAttribute/GenericAttributeMetadata.cs +++ b/src/tests/reflection/GenericAttribute/GenericAttributeMetadata.cs @@ -17,8 +17,6 @@ [assembly: MultiAttribute()] [assembly: MultiAttribute(true)] -[module: SingleAttribute()] - [AttributeUsage(AttributeTargets.Assembly | AttributeTargets.Class | AttributeTargets.Property, AllowMultiple = false)] public class SingleAttribute : Attribute { diff --git a/src/tests/reflection/GenericAttribute/GenericAttributeTests.cs b/src/tests/reflection/GenericAttribute/GenericAttributeTests.cs index b1dcd6bbbe397..e7661d2d5aea8 100644 --- a/src/tests/reflection/GenericAttribute/GenericAttributeTests.cs +++ b/src/tests/reflection/GenericAttribute/GenericAttributeTests.cs @@ -18,16 +18,8 @@ static int Main(string[] args) Assert(((ICustomAttributeProvider)assembly).IsDefined(typeof(SingleAttribute), true)); Assert(CustomAttributeExtensions.IsDefined(assembly, typeof(SingleAttribute))); Assert(((ICustomAttributeProvider)assembly).IsDefined(typeof(SingleAttribute), true)); - Assert(!CustomAttributeExtensions.GetCustomAttributes(assembly, typeof(SingleAttribute<>)).GetEnumerator().MoveNext()); - Assert(!CustomAttributeExtensions.GetCustomAttributes(assembly, typeof(SingleAttribute<>)).GetEnumerator().MoveNext()); -*/ - // Module module = programTypeInfo.Module; - // AssertAny(CustomAttributeExtensions.GetCustomAttributes(module), a => a is SingleAttribute); - // Assert(CustomAttributeExtensions.GetCustomAttributes(module, typeof(SingleAttribute)).GetEnumerator().MoveNext()); - // Assert(CustomAttributeExtensions.GetCustomAttributes(module, typeof(SingleAttribute)).GetEnumerator().MoveNext()); - // Assert(!CustomAttributeExtensions.GetCustomAttributes(module, typeof(SingleAttribute<>)).GetEnumerator().MoveNext()); - // Assert(!CustomAttributeExtensions.GetCustomAttributes(module, typeof(SingleAttribute<>)).GetEnumerator().MoveNext()); +*/ TypeInfo programTypeInfo = typeof(Class).GetTypeInfo(); Assert(CustomAttributeExtensions.GetCustomAttribute>(programTypeInfo) != null); @@ -160,8 +152,8 @@ static int Main(string[] args) AssertAny(b10, a => (a as MultiAttribute)?.Value == typeof(Class)); AssertAny(b10, a => (a as MultiAttribute)?.Value == typeof(Class.Derive)); - Assert(!CustomAttributeExtensions.GetCustomAttributes(programTypeInfo, typeof(MultiAttribute<>), false).GetEnumerator().MoveNext()); - Assert(!CustomAttributeExtensions.GetCustomAttributes(programTypeInfo, typeof(MultiAttribute<>), true).GetEnumerator().MoveNext()); + Assert(CustomAttributeExtensions.GetCustomAttributes(programTypeInfo, typeof(MultiAttribute<>), false) == null); + Assert(CustomAttributeExtensions.GetCustomAttributes(programTypeInfo, typeof(MultiAttribute<>), true) == null); Assert(!((ICustomAttributeProvider)programTypeInfo).GetCustomAttributes(typeof(MultiAttribute<>), true).GetEnumerator().MoveNext()); // Test coverage for CustomAttributeData api surface