Fix obtaining type handles of IDynamicInterfaceCastableImplementation #109875
Conversation
Fixes dotnet#109496. We try to optimize away type handles (MethodTables) of types that are only needed due to metadata. Trying to grab type handle of such type throws. This fixes it by unconditionally generating type handles of IDynamicInterfaceCastableImplementation.
| // (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. |
There was a problem hiding this comment.
Our of curiosity, is this something we could fix? What would those annotations look like?
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
Would a proposal for
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.Constructed)]be viable? Eg. we useGetUninitializedObjectin 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 addedConstructedand updatedPublicConstructorsandPrivateConstructorsto both also implyConstructed, that would seem like it would not be a breaking change? And then we could update annotations to just requireConstructed, 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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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 (
GetUninitializedObjectis 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).
|
/backport to release/9.0-staging |
|
Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/11888115234 |
…dotnet#109875) Fixes dotnet#109496. We try to optimize away type handles (MethodTables) of types that are only needed due to metadata. Trying to grab type handle of such type throws. This fixes it by unconditionally generating type handles of IDynamicInterfaceCastableImplementation.
Fixes #109496.
We try to optimize away type handles (MethodTables) of types that are only needed due to metadata. Trying to grab type handle of such type throws. This fixes it by unconditionally generating type handles of IDynamicInterfaceCastableImplementation.
Cc @dotnet/ilc-contrib