Support IDynamicInterfaceCastable with shared generic code #72909
Comments
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.
|
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). |
|
I think we're going to hit it in the 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. |
|
If you get that exception on the 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). |
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.
|
I think if we didn't block the variant case here: If I understand what you wrote correctly, does it mean that CsWinRT will actually go and |
It would be worth trying to extend that check to also allow the variant case. I wouldn't want to remove that check completely.
I believe that was the direction we were going down, yes. |
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. |
|
Filed #58619 |
ghost
added
the
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
bot
added
the
in-pr
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.
Fixes #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.
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.
…ic code (#109918) * Fix IDynamicInterfaceCastable with shared generic code Fixes #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. * Finishing touches * Regression test --------- Co-authored-by: Michal Strehovský <MichalStrehovsky@users.noreply.github.com>
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:
[DynIntfCastableImpl] interface IFooImpl : ISomething<object>, ISomething<string> { }when both ISomething provide default implementations.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.
The text was updated successfully, but these errors were encountered: