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 1 commit
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
13 changes: 7 additions & 6 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 @@ -438,6 +438,9 @@ private static Attribute[] CreateAttributeArrayHelper(Type elementType, int elem
{
return (Attribute[])Array.CreateInstance(elementType, elementCount);
}

private static Attribute[] ToAttributeArray(object[] getCustomAttributesResult) =>
jkotas marked this conversation as resolved.
Show resolved Hide resolved
getCustomAttributesResult as Attribute[] ?? Array.Empty<Attribute>();
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
#endregion

#endregion
Expand All @@ -459,7 +462,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[])!,
_ => ToAttributeArray(element.GetCustomAttributes(attributeType, inherit))
};
}

Expand All @@ -474,7 +477,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[])!,
_ => ToAttributeArray(element.GetCustomAttributes(typeof(Attribute), inherit))
};
}

Expand Down Expand Up @@ -536,25 +539,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 ToAttributeArray(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 ToAttributeArray(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
65 changes: 65 additions & 0 deletions 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 @@ -533,6 +533,33 @@ public static void NegTest8()
AssertExtensions.Throws<ArgumentException>(null, () => (ArgumentUsageAttribute)Attribute.GetCustomAttribute(paramInfos[0], attributeType, false));

}

// reproduces https://github.com/dotnet/runtime/issues/64335
[Fact]
public static void GetCustomAttributesDoesNotReturnNullForOpenGenericType()
{
Attribute[] openGenericAttributes = Attribute.GetCustomAttributes(typeof(HasGenericAttribute), typeof(GenericAttribute<>));
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
Assert.Empty(openGenericAttributes);
Assert.Equal(typeof(Attribute[]), openGenericAttributes.GetType());

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

[Fact]
public static void GetCustomAttributesReturnsAttributeArrayForMisbehavingCustomMemberInfo()
{
FieldInfo fieldWithNullAttributes = new CustomFieldInfo(null!);
Attribute[] fieldWithNullAttributesAttributes = Attribute.GetCustomAttributes(fieldWithNullAttributes);
Assert.NotNull(fieldWithNullAttributesAttributes);
Assert.Empty(fieldWithNullAttributesAttributes);

FieldInfo fieldWithStringAttributes = new CustomFieldInfo(new[] { "a", "b" });
Attribute[] fieldWithStringAttributesAttributes = Attribute.GetCustomAttributes(fieldWithStringAttributes);
Assert.NotNull(fieldWithStringAttributesAttributes);
Assert.Empty(fieldWithStringAttributesAttributes);
}
}
[AttributeUsage(AttributeTargets.All)]
public class TestAttribute : Attribute
Expand Down Expand Up @@ -824,4 +851,42 @@ public class NameableAttribute : Attribute, INameable

[Nameable]
public class ExampleWithAttribute { }

public class GenericAttribute<T> : Attribute
{
}

[GenericAttribute<string>]
public class HasGenericAttribute
{
}

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)
{
}
}
}