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.

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

Avoid Attribute.GetCustomAttributes() returning null for open generic type #65237

Merged
merged 12 commits into from
Mar 4, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 6 additions & 10 deletions src/coreclr/System.Private.CoreLib/src/System/Attribute.CoreCLR.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
};
}

Expand All @@ -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)
};
}

Expand Down Expand Up @@ -536,25 +534,23 @@ 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)
{
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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -902,7 +902,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<object>() : CreateAttributeArrayHelper(caType, 0);
return CreateAttributeArrayHelper(caType, 0);

if (type.IsGenericType && !type.IsGenericTypeDefinition)
type = (type.GetGenericTypeDefinition() as RuntimeType)!;
Expand All @@ -922,8 +922,6 @@ internal static object[] GetCustomAttributes(RuntimeType type, RuntimeType caTyp

RuntimeType.ListBuilder<object> 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]);
Expand All @@ -935,7 +933,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];
Expand Down Expand Up @@ -966,8 +964,6 @@ internal static object[] GetCustomAttributes(RuntimeMethodInfo method, RuntimeTy

RuntimeType.ListBuilder<object> 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]);
Expand All @@ -979,7 +975,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];
Expand Down Expand Up @@ -1126,16 +1122,13 @@ private static bool IsCustomAttributeDefined(
}

private static object[] GetCustomAttributes(
RuntimeModule decoratedModule, int decoratedMetadataToken, int pcaCount, RuntimeType? attributeFilterType)
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
RuntimeModule decoratedModule, int decoratedMetadataToken, int pcaCount, RuntimeType attributeFilterType)
{
RuntimeType.ListBuilder<object> 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];
Expand Down Expand Up @@ -1442,6 +1435,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<Attribute>() : new Attribute[elementCount];
}
if (useObjectArray)
{
return elementCount == 0 ? Array.Empty<object>() : new object[elementCount];
}
return elementCount == 0 ? caType.GetEmptyArray() : (object[])Array.CreateInstance(caType, elementCount);
}
#endregion

#region Private Static FCalls
Expand Down Expand Up @@ -1479,17 +1508,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
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<object>();

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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was required to prevent a failure in Microsoft.CSharp.RuntimeBinder.Tests.AccessTests.PublicMethod().

I feel a bit weird exposing the CreateAttributeArrayHelper method as internal, but this is really the exact logic we want here. It makes this branch consistent with other GetCustomAttribute methods which all allow the consumer to cast the array to the requested attribute type.

Moving this after the argument check presumably has some slight perf cost but ultimately feels more correct (and unless we change CreateAttributeArrayHelper to take Type instead of RuntimeType we need the check to happen first regardless).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Do we need a test to directly verify the behavior of ParamerInfo.GetCustomAttributes for this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added!


return CustomAttribute.GetCustomAttributes(this, attributeRuntimeType);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,19 +141,9 @@ private static Attribute[] Instantiate(IEnumerable<CustomAttributeData> 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;
}
Expand Down
8 changes: 8 additions & 0 deletions src/libraries/System.Reflection/tests/ParameterInfoTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
{
Expand Down
100 changes: 96 additions & 4 deletions src/libraries/System.Runtime/tests/System/Attributes.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
using System.Runtime.InteropServices;
using Xunit;

[module:Debuggable(true,false)]
[module: Debuggable(true, false)]
namespace System.Tests
{
public class AttributeIsDefinedTests
Expand Down Expand Up @@ -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<string>(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<TimeSpan>(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<Guid>(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<long>(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<ulong>(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<List<object>>(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<DateTime?>(t => Attribute.GetCustomAttributes(@event, t));
}

private static void GenericAttributesTestHelper<TGenericParameter>(Func<Type, Attribute[]> getCustomAttributes)
{
Attribute[] openGenericAttributes = getCustomAttributes(typeof(GenericAttribute<>));
Assert.Empty(openGenericAttributes);

Attribute[] closedGenericAttributes = getCustomAttributes(typeof(GenericAttribute<TGenericParameter>));
Assert.Equal(1, closedGenericAttributes.Length);
Assert.Equal(typeof(GenericAttribute<TGenericParameter>[]), closedGenericAttributes.GetType());
}
}

public static class GetCustomAttribute
Expand All @@ -226,7 +291,7 @@ public static class GetCustomAttribute
[Fact]
public static void customAttributeCount()
{
List<CustomAttributeData> customAttributes = typeof(GetCustomAttribute).Module.CustomAttributes.ToList();
List<CustomAttributeData> customAttributes = typeof(GetCustomAttribute).Module.CustomAttributes.ToList();
// [System.Security.UnverifiableCodeAttribute()]
// [TestAttributes.FooAttribute()]
// [TestAttributes.ComplicatedAttribute((Int32)1, Stuff = 2)]
Expand Down Expand Up @@ -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)
{
}
}
Expand Down Expand Up @@ -816,12 +881,39 @@ 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";
}

[Nameable]
public class ExampleWithAttribute { }

public class GenericAttribute<T> : Attribute
{
}

[GenericAttribute<string>]
public class HasGenericAttribute
{
[GenericAttribute<TimeSpan>]
internal bool Field;

[GenericAttribute<Guid>]
public HasGenericAttribute() { }

[GenericAttribute<long>]
public void Method([GenericAttribute<ulong>] int parameter)
{
this.Field = true;
this.Event += () => { };
}

[GenericAttribute<List<object>>]
public int Property { get; set; }

[GenericAttribute<DateTime?>]
public event Action Event;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
[assembly: MultiAttribute<bool>()]
[assembly: MultiAttribute<bool>(true)]

[module: SingleAttribute<long>()]

[AttributeUsage(AttributeTargets.Assembly | AttributeTargets.Class | AttributeTargets.Property, AllowMultiple = false)]
public class SingleAttribute<T> : Attribute
{
Expand Down
Loading