From 20294957616061616e672b43f1d962460b7f4234 Mon Sep 17 00:00:00 2001 From: madelson <1269046+madelson@users.noreply.github.com> Date: Thu, 3 Mar 2022 23:19:52 -0500 Subject: [PATCH] Avoid Attribute.GetCustomAttributes() returning null for open generic type (#65237) * Avoid Attribute.GetCustomAttributes() returning null for open generic type. Fix #64335 --- .../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, 170 insertions(+), 56 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 f33849d2a949e..c84e918d84303 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Attribute.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Attribute.CoreCLR.cs @@ -434,10 +434,8 @@ private static AttributeUsageAttribute InternalGetAttributeUsage(Type type) SR.Format(SR.Format_AttributeUsage, type)); } - private static Attribute[] CreateAttributeArrayHelper(Type elementType, int elementCount) - { - return (Attribute[])Array.CreateInstance(elementType, elementCount); - } + private static Attribute[] CreateAttributeArrayHelper(Type elementType, int elementCount) => + elementType.ContainsGenericParameters ? new Attribute[elementCount] : (Attribute[])Array.CreateInstance(elementType, elementCount); #endregion #endregion @@ -459,7 +457,7 @@ public static Attribute[] GetCustomAttributes(MemberInfo element!!, Type attribu { MemberTypes.Property => InternalGetCustomAttributes((PropertyInfo)element, attributeType, inherit), MemberTypes.Event => InternalGetCustomAttributes((EventInfo)element, attributeType, inherit), - _ => (element.GetCustomAttributes(attributeType, inherit) as Attribute[])!, + _ => (Attribute[])element.GetCustomAttributes(attributeType, inherit) }; } @@ -474,7 +472,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), - _ => (element.GetCustomAttributes(typeof(Attribute), inherit) as Attribute[])!, + _ => (Attribute[])element.GetCustomAttributes(typeof(Attribute), inherit) }; } @@ -536,12 +534,11 @@ 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 (element.GetCustomAttributes(attributeType, inherit) as Attribute[])!; + return (Attribute[])element.GetCustomAttributes(attributeType, inherit); } public static Attribute[] GetCustomAttributes(ParameterInfo element!!, bool inherit) @@ -549,12 +546,11 @@ 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 (element.GetCustomAttributes(typeof(Attribute), inherit) as Attribute[])!; + return (Attribute[])element.GetCustomAttributes(typeof(Attribute), inherit); } 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 bb642f913da71..d28edee240381 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 (caType.IsValueType) ? Array.Empty() : CreateAttributeArrayHelper(caType, 0); + return CreateAttributeArrayHelper(caType, 0); if (type.IsGenericType && !type.IsGenericTypeDefinition) type = (type.GetGenericTypeDefinition() as RuntimeType)!; @@ -919,8 +919,6 @@ 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]); @@ -932,7 +930,7 @@ internal static object[] GetCustomAttributes(RuntimeType type, RuntimeType caTyp type = (type.BaseType as RuntimeType)!; } - object[] typedResult = CreateAttributeArrayHelper(arrayType, result.Count); + object[] typedResult = CreateAttributeArrayHelper(caType, result.Count); for (int i = 0; i < result.Count; i++) { typedResult[i] = result[i]; @@ -963,8 +961,6 @@ 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]); @@ -976,7 +972,7 @@ internal static object[] GetCustomAttributes(RuntimeMethodInfo method, RuntimeTy method = method.GetParentDefinition()!; } - object[] typedResult = CreateAttributeArrayHelper(arrayType, result.Count); + object[] typedResult = CreateAttributeArrayHelper(caType, result.Count); for (int i = 0; i < result.Count; i++) { typedResult[i] = result[i]; @@ -1123,16 +1119,13 @@ 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); - bool useObjectArray = attributeFilterType is null || attributeFilterType.IsValueType || attributeFilterType.ContainsGenericParameters; - RuntimeType arrayType = useObjectArray ? (RuntimeType)typeof(object) : attributeFilterType!; - - object[] result = CreateAttributeArrayHelper(arrayType, attributes.Count + pcaCount); + object[] result = CreateAttributeArrayHelper(attributeFilterType, attributes.Count + pcaCount); for (int i = 0; i < attributes.Count; i++) { result[i] = attributes[i]; @@ -1439,6 +1432,42 @@ 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 @@ -1476,17 +1505,6 @@ 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 33777c745dbfc..f889133b531f9 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 d7f9bb693ea03..f7604612b5ad5 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,19 +141,9 @@ private static Attribute[] Instantiate(IEnumerable cads, Ty attributes.Add(instantiatedAttribute); } int count = attributes.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; - } + Attribute[] result = actualElementType.ContainsGenericParameters + ? new Attribute[count] + : (Attribute[])Array.CreateInstance(actualElementType, count); 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 f6285546a8713..26d2d65867f69 100644 --- a/src/libraries/System.Reflection/tests/ParameterInfoTests.cs +++ b/src/libraries/System.Reflection/tests/ParameterInfoTests.cs @@ -245,6 +245,14 @@ 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 e219d30a2bef4..3dda3f56b3566 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,6 +218,71 @@ 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 @@ -226,7 +291,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)] @@ -660,7 +725,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) { } } @@ -816,7 +881,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"; @@ -824,4 +889,31 @@ 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 d0ef6353f4a8f..3bea1e621945c 100644 --- a/src/tests/reflection/GenericAttribute/GenericAttributeMetadata.cs +++ b/src/tests/reflection/GenericAttribute/GenericAttributeMetadata.cs @@ -17,6 +17,8 @@ [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 e7661d2d5aea8..b1dcd6bbbe397 100644 --- a/src/tests/reflection/GenericAttribute/GenericAttributeTests.cs +++ b/src/tests/reflection/GenericAttribute/GenericAttributeTests.cs @@ -18,9 +18,17 @@ 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); Assert(((ICustomAttributeProvider)programTypeInfo).GetCustomAttributes(typeof(SingleAttribute), true) != null); @@ -152,8 +160,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) == null); - Assert(CustomAttributeExtensions.GetCustomAttributes(programTypeInfo, typeof(MultiAttribute<>), true) == null); + Assert(!CustomAttributeExtensions.GetCustomAttributes(programTypeInfo, typeof(MultiAttribute<>), false).GetEnumerator().MoveNext()); + Assert(!CustomAttributeExtensions.GetCustomAttributes(programTypeInfo, typeof(MultiAttribute<>), true).GetEnumerator().MoveNext()); Assert(!((ICustomAttributeProvider)programTypeInfo).GetCustomAttributes(typeof(MultiAttribute<>), true).GetEnumerator().MoveNext()); // Test coverage for CustomAttributeData api surface