[release/9.0-staging] Fix IDynamicInterfaceCastable with shared generic code #109918
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.
ghost added the 
Member
eiriktsarpalis left a comment
eiriktsarpalis left a comment There was a problem hiding this comment.
Thanks. Shouldn't this also include the regression test from #109917?
Member
We can have a tell mode change after? I don't know what are the rules around backports, I thought it's pull request granularity. I did validate locally that this cherry pick fixes that case. |
Member
| It is perfectly fine to cherry pick multiple main PRs into a backport PR. |
Member
Sounds good, added it. |
jeffschwMSFT approved these changes Nov 19, 2024
Member
jeffschwMSFT left a comment
jeffschwMSFT left a comment There was a problem hiding this comment.
lgtm. please get a code review. we will take for consideration in 9.0.x
jeffschwMSFT added the Servicing-consider 
jkotas approved these changes Nov 20, 2024
jeffschwMSFT added Servicing-approved 
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Backport of #108235 to release/9.0-staging
/cc @MichalStrehovsky
Customer Impact
Regression
Second one is a regression.
Testing
Added targeted regression test for both.
The first one was a known missing feature that we didn't think is reachable with CsWinRT. It is.
Testing missed the second on because it's an interaction between multiple systems.
Risk
This has been checked into .NET 10 for 2 months already. The risk should be low. We also extended testing for this in main branch.
IMPORTANT: If this backport is for a servicing release, please verify that:
The PR target branch is
release/X.0-staging, notrelease/X.0.If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.