Limit impact of typeof() in a custom attribute

[MichalStrehovsky]

When a type is referenced by a typeof in a custom attribute, don't immediately create a constructed EEType for it.

This is a heuristic - we don't know what the type will be used for. The field/property/constructor parameter that is initialized with the typeof should ideally have a dataflow annotation that describes the intent - the dataflow infrastructure will then make sure we have all that's needed.

I hit this while I was working on #619 - the stubbed out method had a [AsyncStateMachine(typeof(<GetNonFileStreamAsync>d__2))] attribute on it. The code I'm making conditional was making a constructed type for the state machine. Then #618 created a perfect storm that brought a constructed HttpClient object back into the picture and rendered #619 completely ineffective by itself.

[jkotas]

the dataflow infrastructure will then make sure we have all that's needed.

How is the dataflow infrastructure going to do this?

[MichalStrehovsky]

class FooAttribute : Attribute
{
    [DynamicallAccessedMembers(Constructors)]
    public Type Foo;
}

[Foo(Foo = typeof(OtherClass))]
class SomeClass
{
}

class OtherClass
{
}

Dataflow analysis considers this an assignment of OtherClass into Foo. It will make sure we keep all constructors on OtherClass. This also keeps the owning type. Also makes it possible to Activator.CreateInstance whatever we get out of Foo without warnings.

[jkotas]

Is there a good place in the reflection stack that shows the dependency on EEType and that can be linked from here?

It looks odd to me that the reflection stack does not need EETypes for regular types, but it does need them for arrays and generics.

[MichalStrehovsky]

IIRC is this one for arrays:

runtimelab/src/coreclr/nativeaot/System.Private.Reflection.Core/src/System/Reflection/Runtime/General/TypeUnifier.cs

Lines 278 to 289 in 2c62504

	private static void ValidateElementType(RuntimeTypeInfo elementType, RuntimeTypeHandle typeHandle, bool multiDim, int rank)
	{
	Debug.Assert(multiDim || rank == 1);
	if (elementType.IsByRef)
	throw new TypeLoadException(SR.Format(SR.ArgumentException_InvalidArrayElementType, elementType));
	// We only permit creating parameterized types if the pay-for-play policy specifically allows them *or* if the result
	// type would be an open type.
	if (typeHandle.IsNull() && !elementType.ContainsGenericParameters && !(elementType is RuntimeCLSIDTypeInfo))
	throw ReflectionCoreExecution.ExecutionDomain.CreateMissingArrayTypeException(elementType, multiDim, rank);
	}

And this one for generic types:

runtimelab/src/coreclr/nativeaot/System.Private.Reflection.Core/src/System/Reflection/Runtime/General/TypeUnifier.cs

Lines 426 to 443 in 2c62504

	protected sealed override RuntimeConstructedGenericTypeInfo Factory(UnificationKey key)
	{
	bool atLeastOneOpenType = false;
	foreach (RuntimeTypeInfo genericTypeArgument in key.GenericTypeArguments)
	{
	if (genericTypeArgument.IsByRef || genericTypeArgument.IsGenericTypeDefinition)
	throw new ArgumentException(SR.Format(SR.ArgumentException_InvalidTypeArgument, genericTypeArgument));
	if (genericTypeArgument.ContainsGenericParameters)
	atLeastOneOpenType = true;
	}
	// We only permit creating parameterized types if the pay-for-play policy specifically allows them *or* if the result
	// type would be an open type.
	if (key.TypeHandle.IsNull() && !atLeastOneOpenType)
	throw ReflectionCoreExecution.ExecutionDomain.CreateMissingConstructedGenericTypeException(key.GenericTypeDefinition, key.GenericTypeArguments.CloneTypeArray());
	return new RuntimeConstructedGenericTypeInfo(key);
	}

(The // We only permit part)

If we just go and delete those lines things will probably still work.

The intent of that code was to blame MakeGenericType/MakeArrayType for reflection failures at runtime instead of having these APIs succeed and getting an exception once we actually want to do something with the type (call static method, activate, etc.).

I think this behavior makes sense for user code because this API is the problem.

It's annoying for framework code because e.g. without the special casing in the compiler we will get an exception when trying to GetCustomAttributeData for the attribute.

[MichalStrehovsky]

On a second thought, maybe the compiler doesn't need to do this special casing here. This situation is the same as what we would get when e.g. querying fields:

using System;

ClassWithField.Klass = new ClassWithField() { MyGen = null };

typeof(Gen<>).ToString();

foreach (var f in typeof(ClassWithField).GetFields(System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.Public))
{
    Console.WriteLine(f.FieldType.FullName);
}

class ClassWithField
{
    public static ClassWithField Klass;

    public Gen<int> MyGen;
}

class Gen<T> { }

This will fail with:

Unhandled Exception: System.Reflection.MissingMetadataException: 'Gen<System.Int32>' is missing metadata. For more information, please visit http://go.microsoft.com/fwlink/?LinkID=392859
   at System.Reflection.Runtime.TypeInfos.RuntimeConstructedGenericTypeInfo.ConstructedGenericTypeTable.Factory(RuntimeConstructedGenericTypeInfo.UnificationKey) + 0x24b
   at System.Collections.Concurrent.ConcurrentUnifierWKeyed`2.GetOrAdd(K) + 0x23f
   at System.Reflection.Runtime.TypeInfos.RuntimeConstructedGenericTypeInfo.GetRuntimeConstructedGenericTypeInfo(RuntimeTypeInfo, RuntimeTypeInfo[], RuntimeTypeHandle) + 0x6f
   at System.Reflection.Runtime.TypeInfos.RuntimeConstructedGenericTypeInfo.GetRuntimeConstructedGenericTypeInfo(RuntimeTypeInfo, RuntimeTypeInfo[]) + 0x56
   at System.Reflection.Runtime.General.TypeUnifier.GetConstructedGenericType(RuntimeTypeInfo, RuntimeTypeInfo[]) + 0x2a
   at System.Reflection.Runtime.General.TypeResolver.TryResolveTypeSignature(TypeSpecificationHandle, MetadataReader, TypeContext, Exception&) + 0x945
   at System.Reflection.Runtime.General.TypeResolver.TryResolve(Handle, MetadataReader, TypeContext, Exception&) + 0x160
   at System.Reflection.Runtime.General.TypeResolver.Resolve(Handle, MetadataReader, TypeContext) + 0x61
   at System.Reflection.Runtime.FieldInfos.NativeFormat.NativeFormatRuntimeFieldInfo.get_FieldRuntimeType() + 0x93
   at System.Reflection.Runtime.FieldInfos.RuntimeFieldInfo.get_FieldType() + 0x70
   at <Program>$.<Main>$(String[]) + 0x130

The compiler doesn't try to go out of its way to make sure we can access the field type either. Maybe that's fine?

[jkotas]

I do not have opinion, except that consistency and less special cases is good.

[MichalStrehovsky]

Okay, let's see if we can get away without it. We can always restore that behavior.

[jkotas]

Thank you!

[MichalStrehovsky]

Of course I wrote a test for this exact scenario.

Looks like the exception message leaves things to be desired, so I need to fix that first to make this change acceptable.