Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Implement two pass algorithm for variant interface dispatch #21355

Merged
merged 2 commits into from
Dec 14, 2018
Merged

Implement two pass algorithm for variant interface dispatch #21355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
703c5a8
Implement two pass algorithm for variant interface dispatch
MichalStrehovsky Dec 4, 2018
d1b1a8b
Add test called out in CR
MichalStrehovsky Dec 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 34 additions & 5 deletions src/vm/methodtable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7008,12 +7008,28 @@ MethodTable::FindDispatchImpl(
//
// See if we can find a default method from one of the implemented interfaces
//

// Try exact match first
MethodDesc *pDefaultMethod = NULL;
if (FindDefaultInterfaceImplementation(
BOOL foundDefaultInterfaceImplementation = FindDefaultInterfaceImplementation(
pIfcMD, // the interface method being resolved
pIfcMT, // the interface being resolved
&pDefaultMethod,
throwOnConflict))
FALSE, // allowVariance
throwOnConflict);

// If there's no exact match, try a variant match
if (!foundDefaultInterfaceImplementation && pIfcMT->HasVariance())
{
foundDefaultInterfaceImplementation = FindDefaultInterfaceImplementation(
pIfcMD, // the interface method being resolved
pIfcMT, // the interface being resolved
&pDefaultMethod,
TRUE, // allowVariance
throwOnConflict);
}

if (foundDefaultInterfaceImplementation)
{
// Now, construct a DispatchSlot to return in *pImplSlot
DispatchSlot ds(pDefaultMethod->GetMethodEntryPoint());
Expand Down Expand Up @@ -7093,6 +7109,7 @@ BOOL MethodTable::FindDefaultInterfaceImplementation(
MethodDesc *pInterfaceMD,
MethodTable *pInterfaceMT,
MethodDesc **ppDefaultMethod,
BOOL allowVariance,
BOOL throwOnConflict
)
{
Expand Down Expand Up @@ -7151,7 +7168,7 @@ BOOL MethodTable::FindDefaultInterfaceImplementation(
{
if (pCurMT->HasSameTypeDefAs(pInterfaceMT))
{
if (!pInterfaceMD->IsAbstract())
if (allowVariance && !pInterfaceMD->IsAbstract())
{
// Generic variance match - we'll instantiate pCurMD with the right type arguments later
pCurMD = pInterfaceMD;
Expand Down Expand Up @@ -7209,7 +7226,8 @@ BOOL MethodTable::FindDefaultInterfaceImplementation(
// We do CanCastToInterface to also cover variance.
// We already know this is a method on the same type definition as the (generic)
// interface but we need to make sure the instantiations match.
if (pDeclMT->CanCastToInterface(pInterfaceMT))
if ((allowVariance && pDeclMT->CanCastToInterface(pInterfaceMT))
|| pDeclMT == pInterfaceMT)
{
// We have a match
pCurMD = pMD;
Expand Down Expand Up @@ -7265,7 +7283,11 @@ BOOL MethodTable::FindDefaultInterfaceImplementation(
break;
}

if (pCurMT->CanCastToInterface(pCandidateMT))
if (allowVariance && pCandidateMT->HasSameTypeDefAs(pCurMT))
{
// Variant match on the same type - this is a tie
}
else if (pCurMT->CanCastToInterface(pCandidateMT))
{
// pCurMT is a more specific choice than IFoo/IBar both overrides IBlah :
if (!seenMoreSpecific)
Expand Down Expand Up @@ -7312,6 +7334,8 @@ BOOL MethodTable::FindDefaultInterfaceImplementation(
}

// scan to see if there are any conflicts
// If we are doing second pass (allowing variance), we know don't actually look for
// a conflict anymore, but pick the first match.
MethodTable *pBestCandidateMT = NULL;
MethodDesc *pBestCandidateMD = NULL;
for (unsigned i = 0; i < candidatesCount; ++i)
Expand All @@ -7323,6 +7347,11 @@ BOOL MethodTable::FindDefaultInterfaceImplementation(
{
pBestCandidateMT = candidates[i].pMT;
pBestCandidateMD = candidates[i].pMD;

// If this is a second pass lookup, we know this is a variant match. As such
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm starting to understand some of the discussions from email in the last week or so. I don't see a problem with avoiding throwing in the diamond case where there are two most specific implementations, and I believe we should pick some implementation, but I do have a concern that we should be picking between the most specific implementations, not simply picking any possible implementation.

Imagine,

interface IFoo<in T> { string M() { return "IFoo:" + typeof(T).Name; } }
interface IRequiresImpInterface<in T> : IFoo<in T> { string IFoo<T>.M() { return "IRequiresImpInterface:" + typeof(T).Name; } }

class Base {}
class Derived : Base {}

class Test : IFoo<Base>, IRequiresImpInterface<Base>
{
    static void Main()
    {
        var t = new Test();
        ((IFoo<Base>)t).M(); // Clearly calls IRequiresImpInterface<Base>.M()
        ((IFoo<Derived>)t).M(); // I think becomes order dependent in this implementation between calling IRequiresImpInterface<Base> and IFoo<Base> That doesn't seem right to me. I believe it should unambiguously call IRequiresImpInterface<Base>.M()
    }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also if that test above gets codified, it should have the cases in IL where class Test explicitly implements IRequiresImpInterface only, the case where it implements IFoo before IRequiresImpInterface, and the one where it implements IFoo after IRequiresImpInterface.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlekseyTs please chime in if you disagree.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Only methods that implement interfaces that class explicitly claims to implement in metadata should be considered as candidates for variant interface dispatch. In the example above, method M in IFoo<Base> doesn't implement any interface that class Test implements. IFoo<T>.M in IRequiresImpInterface<Base> implements method IFoo<Base>.M for class Test. I assume implementations in base classes are handled similarly, i.e. only implementations from most derived bases are considered for variant interface dispatch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It already works that way. The loop that builds the list of candidates already kicks out those that are not most specific. In the second loop (where this comment is attached) we only go over the list of the most specific candidates to see which one is first (for the variant case) or which one is the only one that we came up with (or throw) for the non-variant case.

I've pushed out a commit that adds the test case from your comment to demonstrate we only consider the most specific ones. In that case, the variant dispatch is unambiguous because there's only one most specific implementation for it.

// we pick the first result as the winner and don't look for a conflict.
if (allowVariance)
break;
}
else if (pBestCandidateMT != candidates[i].pMT)
{
Expand Down
1 change: 1 addition & 0 deletions src/vm/methodtable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2462,6 +2462,7 @@ class MethodTable
MethodDesc *pInterfaceMD,
MethodTable *pObjectMT,
MethodDesc **ppDefaultMethod,
BOOL allowVariance,
BOOL throwOnConflict);
#endif // DACCESS_COMPILE

Expand Down
Loading