Fix virtual static dispatch for variant interfaces when using default implementations#88639
Fix virtual static dispatch for variant interfaces when using default implementations#88639davidwrighton merged 5 commits intodotnet:mainfrom
Conversation
…ike variant interface default method dispatch
…plementationException
| @@ -8911,7 +8912,7 @@ MethodTable::ResolveVirtualStaticMethod( | |||
| // Search for match on a per-level in the type hierarchy | |||
| for (MethodTable* pMT = this; pMT != nullptr; pMT = pMT->GetParentMethodTable()) | |||
There was a problem hiding this comment.
Shouldn't this parent type descent also be in a 2-pass loop over allowVariance = FALSE, TRUE when allowVariantMatches is set i.o.w. shouldn't it also be in the below loop used for default interface resolution?
There was a problem hiding this comment.
Hmm, after getting my head around the tests I take it back, the variant lookup should take place before descending into parent.
| // Variant or equivalent matching interface found | ||
| // Attempt to resolve on variance matched interface | ||
| pMD = pMT->TryResolveVirtualStaticMethodOnThisType(pItfInMap, pInterfaceMD, verifyImplemented, level); | ||
| pMD = pMT->TryResolveVirtualStaticMethodOnThisType(pItfInMap, pInterfaceMD, verifyImplemented, /*allowVariance*/ FALSE, level); |
There was a problem hiding this comment.
Shouldn't we be setting allowVariance to TRUE (or perhaps to allowVariantMatches) here? Otherwise I don't see where we are supposed to be performing the variant resolution in this method; can you please explain to me what am I missing?
There was a problem hiding this comment.
Alternatively, if it's the case that in practice all variant resolution is only limited to the default interface implementation case (which is probably correct), why do we need this additional pass over the interface map in this place?
trylek
left a comment
trylek
left a comment
There was a problem hiding this comment.
LGTM, thanks David! I had a bit of trouble getting my head around the exact code flow in SVM resolution but I now think it is actually correct.
|
@davidwrighton I didn't see the new test run with Mono - I think there might be some issue with the pipeline triggers when only a test changed. Could you touch some file in |
ghost
locked as resolved and limited conversation to collaborators
3 participants
Fixes #78621