Skip to content
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 @@ -132,6 +132,18 @@ public static void GetMetadataDependencies(ref DependencyList dependencies, Node
default:
Debug.Assert(type.IsDefType);

// We generally postpone creating MethodTables until absolutely needed.
// IDynamicInterfaceCastableImplementation is special in the sense that just obtaining a System.Type
// (by e.g. browsing custom attribute metadata) gives the user enough to pass this to runtime APIs
// that need a MethodTable. We don't have a legitimate type handle without the MethodTable. Other
// kinds of APIs that expect a MethodTable have enough dataflow annotation to trigger warnings.
// There's no dataflow annotations on the IDynamicInterfaceCastable.GetInterfaceImplementation API.
Copy link
Contributor

Choose a reason for hiding this comment

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

Our of curiosity, is this something we could fix? What would those annotations look like?

Copy link
Member Author

Choose a reason for hiding this comment

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

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.NotActuallyAMemberButConsiderThisConstructed)].

We have one more place where this would be useful and that's GetUninitializedObject, but there we could approximate this by simply saying "keep all constructors". That one doesn't work here because interfaces don't have constructors.

I don't know if we want a whole new thing just to annotate this API. Also it would be a breaking change to add now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would a proposal for [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.Constructed)] be viable? Eg. we use GetUninitializedObject in a few places in the Store, and having to manually mark all public + private constructors everywhere is quite annoying, plus it likely keeps a bit more than it should. If we added Constructed and updated PublicConstructors and PrivateConstructors to both also imply Constructed, that would seem like it would not be a breaking change? And then we could update annotations to just require Constructed, where possible. Thoughts? I'd be happy to open a proposal if you think it seems reasonable 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

Would a proposal for [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.Constructed)] be viable? Eg. we use GetUninitializedObject in a few places in the Store, and having to manually mark all public + private constructors everywhere is quite annoying, plus it likely keeps a bit more than it should. If we added Constructed and updated PublicConstructors and PrivateConstructors to both also imply Constructed, that would seem like it would not be a breaking change? And then we could update annotations to just require Constructed, where possible. Thoughts? I'd be happy to open a proposal if you think it seems reasonable 🙂

Doing breaking changes in DAM annotations is still uncharted territory. This API is currently not annotated at all. I don't know if we could add annotation on it and whether all important users would be able to annotate.

Re-annotating GetUninitializedObject sounds more promising but I don't know if we really get much savings from it. How does it look like size-wise if you just violate the annotation and pass System.Type without annotations here? Do the constructors do meaningful amounts of work that would produce real savings? This API is also a bit niche, typically only used in serializers that need to use source gen anyway.

Copy link
Member

Choose a reason for hiding this comment

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

violate the annotation and pass System.Type without annotations here?

Is it safe to violate the annotation if there is a follow up constructor call? I assume that the follow up constructor call should keep the type constructable.

(GetUninitializedObject without a follow up constructor call is a questionable pattern that we should not be encouraging.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it safe to violate the annotation if there is a follow up constructor call? I assume that the follow up constructor call should keep the type constructable.

(GetUninitializedObject without a follow up constructor call is a questionable pattern that we should not be encouraging.)

It's not straightforward currently:

  • Reflection-calling the constructor would be fine (GetUninitializedObject is annotated as needing all constructors, but in fact we'd just need a single constructor being reflection-callable for this to work). This assumes that the code reflection-calling the constructor is not producing trimming warnings.
  • Calling the constructor with UnsafeAccessor would probably work for IL trimming today (marking constructor marks the type as constructed). If would probably not work for native AOT (it would just be visible as method call, we don't currently do a "type is constructed" implication from that).

if (type.IsInterface && ((MetadataType)type).IsDynamicInterfaceCastableImplementation())
{
dependencies ??= new DependencyList();
dependencies.Add(nodeFactory.ReflectedType(type), "Reflected IDynamicInterfaceCastableImplementation");
}

TypeDesc typeDefinition = type.GetTypeDefinition();
if (typeDefinition != type)
{
Expand Down
55 changes: 55 additions & 0 deletions src/tests/nativeaot/SmokeTests/UnitTests/Interfaces.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Text;
using System.Collections.Generic;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Diagnostics.CodeAnalysis;
Expand Down Expand Up @@ -60,6 +61,7 @@ public static int Run()
TestDefaultDynamicStaticNonGeneric.Run();
TestDefaultDynamicStaticGeneric.Run();
TestDynamicStaticGenericVirtualMethods.Run();
TestRuntime109496Regression.Run();

return Pass;
}
Expand Down Expand Up @@ -1890,4 +1892,57 @@ public static void Run()
Console.WriteLine(s_entry.Enter1<SimpleCallStruct<object>>("One"));
}
}

class TestRuntime109496Regression
{
class CastableThing : IDynamicInterfaceCastable
{
RuntimeTypeHandle IDynamicInterfaceCastable.GetInterfaceImplementation(RuntimeTypeHandle interfaceType)
=> Type.GetTypeFromHandle(interfaceType).GetCustomAttribute<TypeAttribute>().TheType.TypeHandle;
bool IDynamicInterfaceCastable.IsInterfaceImplemented(RuntimeTypeHandle interfaceType, bool throwIfNotImplemented)
=> Type.GetTypeFromHandle(interfaceType).IsDefined(typeof(TypeAttribute));
}

[Type(typeof(IMyInterfaceImpl))]
interface IMyInterface
{
int Method();
}

[DynamicInterfaceCastableImplementation]
interface IMyInterfaceImpl : IMyInterface
{
int IMyInterface.Method() => 42;
}

[Type(typeof(IMyGenericInterfaceImpl<int>))]
interface IMyGenericInterface
{
int Method();
}

[DynamicInterfaceCastableImplementation]
interface IMyGenericInterfaceImpl<T> : IMyGenericInterface
{
int IMyGenericInterface.Method() => typeof(T).Name.Length;
}

class TypeAttribute : Attribute
{
public Type TheType { get; }

public TypeAttribute(Type t) => TheType = t;
}

public static void Run()
{
object o = new CastableThing();

if (((IMyInterface)o).Method() != 42)
throw new Exception();

if (((IMyGenericInterface)o).Method() != 5)
throw new Exception();
}
}
}