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

Support IDynamicInterfaceCastable with shared generic code #72909

Closed
MichalStrehovsky opened this issue Aug 23, 2021 · 7 comments · Fixed by #108235
Closed

Support IDynamicInterfaceCastable with shared generic code #72909

MichalStrehovsky opened this issue Aug 23, 2021 · 7 comments · Fixed by #108235
Labels
area-NativeAOT-coreclr in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Aug 23, 2021

Below snippet will fail with a NullRef while trying to do generic lookup in IInterfaceCastableImpl. This is because we didn't provide generic context.

The implementation strategy will be somewhere along the lines of:

  • When generating the sealed vtable, add a delta (like we do for fat function pointers) to canonical entries on IDynamicCastable implementations so that we know more is needed at runtime.
  • Do something to the file format so that we can express "the index of the interface through which implementation is provided" to cover [DynIntfCastableImpl] interface IFooImpl : ISomething<object>, ISomething<string> { } when both ISomething provide default implementations.
  • Whenever a IDynamicCastable implementation is looked up at runtime and the found method has the delta bit set, create a thunkpool thunk that puts the "interface through which implementation is provided" in the TLS slot.
  • Make a cache for the thunkpool thunks.
  • Update the shared generic helper to take the value from the TLS slot.

This is a bunch of work.

It's unclear to me if anyone uses IDynIntfCast with generics. It doesn't work great with generics. E.g. interface variance is broken.

using System;
using System.Runtime.InteropServices;

var s = (IInterface<string>)new CastableClass<IInterface<string>, IInterfaceCastableImpl<string>>();
s.GetCookie();

class CastableClass<TInterface, TImpl> : IDynamicInterfaceCastable
{
    RuntimeTypeHandle IDynamicInterfaceCastable.GetInterfaceImplementation(RuntimeTypeHandle interfaceType)
        => typeof(TImpl).TypeHandle;
    bool IDynamicInterfaceCastable.IsInterfaceImplemented(RuntimeTypeHandle interfaceType, bool throwIfNotImplemented)
        => true;
}

interface IInterface<out T>
{
    string GetCookie();
}

[DynamicInterfaceCastableImplementation]
interface IInterfaceCastableImpl<T> : IInterface<T>
{
    string IInterface<T>.GetCookie() => typeof(T).ToString();
}
MichalStrehovsky referenced this issue in MichalStrehovsky/runtimelab Aug 23, 2021
This is enough to get all our test to pass.

The rest is tracked in https://github.com/dotnet/runtimelab/issues/1442.

It's a bunch of work and at this point I don't know how much we need it.
@jkoritzinsky
Copy link
Member

C#/WinRT uses IDIC with generics to an extent (and they're looking at figuring out a way to handle generic variance themselves for the few cases they have), so at least the basics need to be covered. (#1443 might cover all the cases required).

@MichalStrehovsky
Copy link
Member Author

I think we're going to hit it in the Marshaler<T>.FromAbi calls used to get return values in CsWinRT.

Btw, does generic variance actually work with IDIC? If I add these two lines at the end of the top-level main above:

IInterface<object> s2 = s;
s2.GetCookie();

I get "'Type 'IInterfaceCastableImpl`1[System.String]' returned by IDynamicInterfaceCastable does not implement the requested interface 'IInterface`1[System.Object]'.'". My intuition would say that this should be valid.

@jkoritzinsky
Copy link
Member

If you get that exception on the GetCookie call, then you're seeing the same things C#/WinRT is seeing. Variance is not intrinsically supported in the system today, but covariance can be emulated manually by having the implementation of GetInterfaceImplementation look at the known "supported" types and using reflection to determine if the requested type (IInterface<object> in this case) can be supported by one of the known-supported implementation interfaces (the implementation for IInterface<string> in your example).

This is definitely not fast (though precalculating it can help), but it's a reasonable approximation of what the old built-in WinRT support used to do (and the path I have advised C#/WinRT to go down when they contacted me about issues with covariance).

MichalStrehovsky referenced this issue in dotnet/runtimelab Aug 24, 2021
This is enough to get all our test to pass.

The rest is tracked in https://github.com/dotnet/runtimelab/issues/1442.

It's a bunch of work and at this point I don't know how much we need it.
@MichalStrehovsky
Copy link
Member Author

I think if we didn't block the variant case here:

if (!implType.ImplementInterface(interfaceType))
throw new InvalidOperationException(SR.Format(SR.IDynamicInterfaceCastable_DoesNotImplementRequested, implType, interfaceType));
, it might "just work".

If I understand what you wrote correctly, does it mean that CsWinRT will actually go and MakeGenericType a IInterfaceCastableImpl<object> to answer the cast? Would the statically referenced IInterfaceCastableImpl<string> be able to do the job if we didn't explicitly block it?

@jkoritzinsky
Copy link
Member

I think if we didn't block the variant case here: dotnet/runtime@918e6a9/src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/DynamicInterfaceCastableHelpers.cs#L37-L38, it might "just work".

It would be worth trying to extend that check to also allow the variant case. I wouldn't want to remove that check completely.

If I understand what you wrote correctly, does it mean that CsWinRT will actually go and MakeGenericType a IInterfaceCastableImpl<object> to answer the cast? Would the statically referenced IInterfaceCastableImpl<string> be able to do the job if we didn't explicitly block it?

I believe that was the direction we were going down, yes.

@AaronRobinsonMSFT
Copy link
Member

I think if we didn't block the variant case here:

if (!implType.ImplementInterface(interfaceType))
throw new InvalidOperationException(SR.Format(SR.IDynamicInterfaceCastable_DoesNotImplementRequested, implType, interfaceType));
, it might "just work".

Agreed. This check is a bit too aggressive in my opinion. There are other issues here but that really should be

if (!implType.IsAssignableTo(interfaceType))
    throw new InvalidOperationException(SR.Format(SR.IDynamicInterfaceCastable_DoesNotImplementRequested, implType, interfaceType));

I will update this tomorrow and add some tests. Do we have any of these variance issues tracked in dotnet/runtime? If not, I'd like to see an issue on this.

@MichalStrehovsky
Copy link
Member Author

Filed #58619

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 27, 2022
@MichalStrehovsky MichalStrehovsky transferred this issue from dotnet/runtimelab Jul 27, 2022
@MichalStrehovsky MichalStrehovsky added this to the 8.0.0 milestone Jul 29, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jul 29, 2022
@MichalStrehovsky MichalStrehovsky modified the milestones: 8.0.0, 9.0.0 Jul 11, 2023
@MichalStrehovsky MichalStrehovsky modified the milestones: 9.0.0, Future Feb 12, 2024
MichalStrehovsky added a commit to MichalStrehovsky/runtime that referenced this issue Sep 25, 2024
Fixes dotnet#72909.

Internal team ran into this. Turns out CsWinRT also needs this, but they're were working around instead pushing on a fix.

The big problem with this one is that we have an interface call to a default interface method that requires generic context. This means we need some kind of instantiating thunk (since callsite didn't provide generic context because it didn't know it). The normal default interface case uses an instantiating thunk that simply indexes into the interface list of `this`. We know the index of the interface (we don't know the concrete type because `T`s could be involved), but we can easily compute it at runtime from `this`.

The problem with `IDynamicInterfaceCastable` is that `this` is useless (the class doesn't know anything about the interface). So we need to get the generic context from somewhere else. In this PR, I'm using the thunkpool as "somewhere else". When we finish interface lookup and find out `IDynamicInterfaceCastable` provided a shared method, we create a thunkpool thunk that stashes away the context. We then call the "default interface method instantiating thunk" and instead of indexing into interface list of `this`, we index into interface list of whatever was stashed away. So there are two thunks before we reach the point of executing the method body.
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Sep 25, 2024
sirntar pushed a commit to sirntar/runtime that referenced this issue Sep 30, 2024
Fixes dotnet#72909.

Internal team ran into this. Turns out CsWinRT also needs this, but they're were working around instead pushing on a fix.

The big problem with this one is that we have an interface call to a default interface method that requires generic context. This means we need some kind of instantiating thunk (since callsite didn't provide generic context because it didn't know it). The normal default interface case uses an instantiating thunk that simply indexes into the interface list of `this`. We know the index of the interface (we don't know the concrete type because `T`s could be involved), but we can easily compute it at runtime from `this`.

The problem with `IDynamicInterfaceCastable` is that `this` is useless (the class doesn't know anything about the interface). So we need to get the generic context from somewhere else. In this PR, I'm using the thunkpool as "somewhere else". When we finish interface lookup and find out `IDynamicInterfaceCastable` provided a shared method, we create a thunkpool thunk that stashes away the context. We then call the "default interface method instantiating thunk" and instead of indexing into interface list of `this`, we index into interface list of whatever was stashed away. So there are two thunks before we reach the point of executing the method body.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NativeAOT-coreclr in-pr There is an active PR which will close this issue when it is merged
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants