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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 12 commits into from
Mar 4, 2022
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
79c3416
Avoid Attribute.GetCustomAttributes() returning null for open generic…
madelson Feb 12, 2022
f0f1e6c
Fix GenericAttributeTests to reflect #64335
madelson Feb 12, 2022
fc0f401
Skip new attribute tests on Mono.
madelson Feb 12, 2022
224c985
Address feedback from https://github.com/dotnet/runtime/pull/65237
madelson Feb 16, 2022
a1717a9
Fixes for https://github.com/dotnet/runtime/pull/65237
madelson Feb 16, 2022
3631f5a
Additional follow-up from https://github.com/dotnet/runtime/pull/65237
madelson Feb 18, 2022
e62566d
Further test cleanup for https://github.com/dotnet/runtime/pull/65237
madelson Feb 19, 2022
e0aef33
Attempt to fix Mono test crashes by moving generic attribute testing …
madelson Feb 24, 2022
a3ecfe1
Fix GenericAttributeTests.cs compilation
madelson Feb 25, 2022
5179891
Comment out generic attribute assembly tests
madelson Feb 25, 2022
86160e1
Trim more tests that fail on Mono
madelson Feb 26, 2022
0bfa71c
retrigger checks
madelson Mar 1, 2022
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
View file
Open in desktop
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
40 changes: 25 additions & 15 deletions src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs
View file
Open in desktop
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 @@ -1480,15 +1473,32 @@ private static void GetPropertyOrFieldData(
blobStart = (IntPtr)pBlobStart;
}

private static object[] CreateAttributeArrayHelper(RuntimeType elementType, int elementCount)
private static object[] CreateAttributeArrayHelper(RuntimeType caType, int elementCount)
{
if (caType.IsValueType)
{
return CreateArray<object>();
}

if (caType.ContainsGenericParameters)
{
if (caType.IsSubclassOf(typeof(Attribute)))
{
return CreateArray<Attribute>();
}

return CreateArray<object>();
}

// If we have 0 elements, don't allocate a new array
if (elementCount == 0)
{
return elementType.GetEmptyArray();
return caType.GetEmptyArray();
}

return (object[])Array.CreateInstance(elementType, elementCount);
return (object[])Array.CreateInstance(caType, elementCount);

T[] CreateArray<T>() => elementCount == 0 ? Array.Empty<T>() : new T[elementCount];
Copy link
Member

@jkotas jkotas Feb 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

(Non-static) local methods with captured arguments are inefficient. The compiler will create display type to pass the captured locals around that comes with a lot of ceremony.

I think I would inline it. The local method is not worth it. Also, I think it may be worth it to check for Attribute and use the faster non-reflection based path for it. Something like:

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) (elementCount != 0) ? new Attribute[elementCount] : Array.Empty<Attribute>();
if (useObjectArray) (elementCount != 0) ? new object[elementCount] : Array.Empty<object>();
return (elementCount != 0) ? (object[])Array.CreateInstance(caType, elementCount) : caType.GetEmptyArray();

}
#endregion
}
Expand Down
16 changes: 3 additions & 13 deletions src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Attribute.CoreRT.cs
View file
Open in desktop
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
178 changes: 177 additions & 1 deletion src/libraries/System.Runtime/tests/System/Attributes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
using System.Runtime.InteropServices;
using Xunit;

[assembly: System.Tests.GenericAttribute<System.Type>]
[module: System.Tests.GenericAttribute<bool>]
[module:Debuggable(true,false)]
namespace System.Tests
{
Expand Down Expand Up @@ -231,7 +233,8 @@ public static void customAttributeCount()
// [TestAttributes.FooAttribute()]
// [TestAttributes.ComplicatedAttribute((Int32)1, Stuff = 2)]
// [System.Diagnostics.DebuggableAttribute((Boolean)True, (Boolean)False)]
Assert.Equal(4, customAttributes.Count);
// [System.Tests.GenericAttribute<bool>]
Assert.Equal(5, customAttributes.Count);
}

[Fact]
Expand Down Expand Up @@ -533,6 +536,123 @@ public static void NegTest8()
AssertExtensions.Throws<ArgumentException>(null, () => (ArgumentUsageAttribute)Attribute.GetCustomAttribute(paramInfos[0], attributeType, false));

}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/56887)", TestRuntimes.Mono)]
public static void GetCustomAttributesWorksWithOpenAndClosedGenericTypesForAssembly()
{
Assembly assembly = Assembly.GetExecutingAssembly();
GenericAttributesTestHelper<Type>(t => Attribute.GetCustomAttributes(assembly, t));
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/56887)", TestRuntimes.Mono)]
public static void GetCustomAttributesWorksWithOpenAndClosedGenericTypesForModule()
{
Module module = typeof(HasGenericAttribute).Module;
GenericAttributesTestHelper<bool>(t => Attribute.GetCustomAttributes(module, t));
}

[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());
}

[Fact]
public static void GetCustomAttributesBehaviorMemberInfoReturnsNull()
{
FieldInfo field = new CustomFieldInfo(null!);
if (PlatformDetection.IsMonoRuntime)
Copy link
Member

Choose a reason for hiding this comment

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

Should we fix Mono to return null in this corner case as well to avoid this condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jkotas I'll defer to your decision but I'm not sure it makes sense. Looking through the code it seems like null vs throw in both CoreCLR and Mono are going to be inconsistent depending on the member type and other factors (e. g. whether inherit is true or false). It would take quite a bit more test coverage and probably quite a few changes to get to a consistent state across all the different call flows.

Another option would just be to stop testing this case, since it is so niche so perhaps undefined behavior is fine.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Another option would just be to stop testing this case, since it is so niche so perhaps undefined behavior is fine.

I agree. We either care about this corner case and the behavior should be consistent across runtimes; or we do not care about this corner case and we should not be testing it at all.

{
Assert.Throws<NullReferenceException>(() => Attribute.GetCustomAttributes(field));
}
else
{
Attribute[] attributes = Attribute.GetCustomAttributes(field);
Assert.Null(attributes);
}
}

[Fact]
public static void GetCustomAttributesBehaviorMemberInfoReturnsStringArray()
{
FieldInfo field = new CustomFieldInfo(new[] { "a" });
Assert.Throws<InvalidCastException>(() => Attribute.GetCustomAttributes(field));
}

[Fact]
public static void GetCustomAttributesBehaviorMemberInfoReturnsAttributeInObjectArray()
{
FieldInfo field = new CustomFieldInfo(new object[] { new TestAttribute(0) });
Assert.Throws<InvalidCastException>(() => Attribute.GetCustomAttributes(field));
}

[Fact]
public static void GetCustomAttributesBehaviorMemberInfoReturnsStringAndAttributeInObjectArray()
{
FieldInfo field = new CustomFieldInfo(new object[] { "a", new TestAttribute(0) });
Assert.Throws<InvalidCastException>(() => Attribute.GetCustomAttributes(field));
}
}
[AttributeUsage(AttributeTargets.All)]
public class TestAttribute : Attribute
Expand Down Expand Up @@ -824,4 +944,60 @@ public class NameableAttribute : Attribute, INameable

[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;
}

public class CustomFieldInfo : FieldInfo
{
private readonly object[] _customAttributes;

public CustomFieldInfo(object[] customAttributes)
{
this._customAttributes = customAttributes;
}

public override FieldAttributes Attributes => default;
public override RuntimeFieldHandle FieldHandle => default;
public override Type FieldType => typeof(object);
public override Type DeclaringType => null;
public override string Name => "custom";
public override Type ReflectedType => null;

public override object[] GetCustomAttributes(bool inherit) => this._customAttributes;

public override object[] GetCustomAttributes(Type attributeType, bool inherit) => this._customAttributes;

public override object GetValue(object obj) => null;

public override bool IsDefined(Type attributeType, bool inherit) => false;

public override void SetValue(object obj, object value, BindingFlags invokeAttr, Binder binder, System.Globalization.CultureInfo culture)
{
}
}
}
4 changes: 2 additions & 2 deletions src/tests/reflection/GenericAttribute/GenericAttributeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,8 @@ static int Main(string[] args)
AssertAny(b10, a => (a as MultiAttribute<Type>)?.Value == typeof(Class));
AssertAny(b10, a => (a as MultiAttribute<Type>)?.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
Expand Down