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

Limit impact of typeof() in a custom attribute #620

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
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,37 @@ internal static string ToDisplayStringIfAvailable(this Type type, List<int> gene
}
else
{
return null;
// First, see if Type.Name is available. If Type.Name is available, then we can be reasonably confident that it is safe to call Type.FullName.
// We'll still wrap the call in a try-catch as a failsafe.
string s = type.InternalNameIfAvailable;
if (s == null)
return null;

try
{
s = type.FullName;
}
catch (MissingMetadataException)
{
}

// Insert commas so that CreateConstructedGenericTypeStringIfAvailable can fill the blanks.
// This is not strictly correct for types nested under generic types, but at this point we're doing
// best effort within reason.
if (type.IsGenericTypeDefinition)
{
s += "[";
int genericArgCount = type.GetGenericArguments().Length;
while (genericArgCount-- > 0)
{
genericParameterOffsets.Add(s.Length);
if (genericArgCount > 0)
s = s + ",";
}
s += "]";
}

return s;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -252,12 +252,8 @@ private static bool AddDependenciesFromCustomAttributeArgument(DependencyList de
if (factory.MetadataManager.IsReflectionBlocked(typeofType))
return false;

// We go for the entire EEType because the reflection stack at runtime might need an EEType anyway
// (e.g. if this is a `typeof(SomeType[,,])` just having the metadata for SomeType is not enough).
// Theoretically, we only need the EEType if this is a constructed type, but let's go for the full
// EEType for the sake of consistency.
dependencies.Add(factory.MaximallyConstructableType(typeofType), "Custom attribute blob");

// Grab the metadata nodes that will be necessary to represent the typeof in the metadata blob
TypeMetadataNode.GetMetadataDependencies(ref dependencies, factory, typeofType, "Custom attribute blob");
return true;
}
}
Expand Down
51 changes: 46 additions & 5 deletions src/tests/nativeaot/SmokeTests/Reflection/Reflection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -638,6 +638,8 @@ struct FirstNeverUsedType { }

struct SecondNeverUsedType { }

struct ThirdNeverUsedType { }

class Gen<T> { }

class TypeAttribute : Attribute
Expand Down Expand Up @@ -667,21 +669,60 @@ class Holder2 { }
[EnumArray(EnumArray = new MyEnum[] { 0 })]
class Holder3 { }

[Type(SomeType = typeof(ThirdNeverUsedType))]
class Holder4 { }

public static void Run()
{
Console.WriteLine(nameof(TestAttributeExpressions));

TypeAttribute attr1 = typeof(Holder1).GetCustomAttribute<TypeAttribute>();
if (attr1.SomeType.ToString() != "ReflectionTest+TestAttributeExpressions+FirstNeverUsedType*[,]")
throw new Exception();
// We don't create EETypes for types referenced from typeof which makes
// getting their constructed version to fail at runtime because
// we place restrictions on on the ability of getting System.Type for
// constructed types.
//
// The workaround is to put some dataflow annotations on the Type-typed
// parameter/property/fields.
//
// This is testing that we get an actionable exception message.
//
// The reason why we don't create EETypes for these is size -
// and one wouldn't be able to do much with just the type anyway.
// We would need at least some methods and at that point we have reflectable
// methods and therefore an EEType.

TypeAttribute attr2 = typeof(Holder2).GetCustomAttribute<TypeAttribute>();
if (attr2.SomeType.ToString() != "ReflectionTest+TestAttributeExpressions+Gen`1[ReflectionTest+TestAttributeExpressions+SecondNeverUsedType]")
try
{
typeof(Holder1).GetCustomAttribute<TypeAttribute>();
throw new Exception();
}
catch (Exception ex)
{
if (!ex.ToString().Contains("ReflectionTest+TestAttributeExpressions+FirstNeverUsedType*[,]"))
throw;
}

try
{
typeof(Holder2).GetCustomAttribute<TypeAttribute>();
}
catch (Exception ex)
{
if (!ex.ToString().Contains("ReflectionTest+TestAttributeExpressions+Gen`1[ReflectionTest+TestAttributeExpressions+SecondNeverUsedType]"))
throw;
}

// Make sure we created EEType for the enum array.

EnumArrayAttribute attr3 = typeof(Holder3).GetCustomAttribute<EnumArrayAttribute>();
if (attr3.EnumArray[0] != 0)
throw new Exception();

// Unconstructed types don't have the problem with missing EETypes described above

TypeAttribute attr4 = typeof(Holder4).GetCustomAttribute<TypeAttribute>();
if (attr4.SomeType.ToString() != "ReflectionTest+TestAttributeExpressions+ThirdNeverUsedType")
throw new Exception();
}
}

Expand Down