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

Implement two pass algorithm for variant interface dispatch #21355

Merged
merged 2 commits into from Dec 14, 2018

Conversation

MichalStrehovsky
Copy link
Member

@MichalStrehovsky MichalStrehovsky commented Dec 4, 2018

Fixes #20452.

@MichalStrehovsky MichalStrehovsky added this to the 3.0 milestone Dec 4, 2018
@MichalStrehovsky
Copy link
Member Author

@MichalStrehovsky MichalStrehovsky commented Dec 4, 2018

Cc @sergiy-k

Copy link
Member

@davidwrighton davidwrighton left a comment

I think we covered the diamond case in discussion, but not the non-diamond case. Please add handling for non-diamond scenarios.

@@ -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

@davidwrighton davidwrighton Dec 5, 2018

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

@davidwrighton davidwrighton Dec 5, 2018

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

@davidwrighton davidwrighton Dec 5, 2018

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.

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

@MichalStrehovsky MichalStrehovsky Dec 5, 2018

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.

@MichalStrehovsky
Copy link
Member Author

@MichalStrehovsky MichalStrehovsky commented Dec 12, 2018

@dotnet-bot test this please

@MichalStrehovsky
Copy link
Member Author

@MichalStrehovsky MichalStrehovsky commented Dec 12, 2018

@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test

@MichalStrehovsky
Copy link
Member Author

@MichalStrehovsky MichalStrehovsky commented Dec 12, 2018

@dotnet-bot test this please

@MichalStrehovsky
Copy link
Member Author

@MichalStrehovsky MichalStrehovsky commented Dec 12, 2018

@dotnet-bot test Ubuntu arm Cross Checked Innerloop Build and Test
@dotnet-bot test Ubuntu arm Cross Checked no_tiered_compilation_innerloop Build and Test
@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test
@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0)

1 similar comment
@MichalStrehovsky
Copy link
Member Author

@MichalStrehovsky MichalStrehovsky commented Dec 13, 2018

@dotnet-bot test Ubuntu arm Cross Checked Innerloop Build and Test
@dotnet-bot test Ubuntu arm Cross Checked no_tiered_compilation_innerloop Build and Test
@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test
@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0)

@MichalStrehovsky
Copy link
Member Author

@MichalStrehovsky MichalStrehovsky commented Dec 13, 2018

This keeps failing with weird Jenkinsonisms (java.lang.UnsupportedOperationException: Refusing to marshal org.kohsuke.github.GHUser for security reasons; see https://jenkins.io/redirect/class-filter/). I'll just close and reopen...

@MichalStrehovsky
Copy link
Member Author

@MichalStrehovsky MichalStrehovsky commented Dec 13, 2018

Seems like Jenkins is not going to give up to Azure DevOps without making our lives miserable for a bit more.

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test
@dotnet-bot test Ubuntu arm Cross Checked Innerloop Build and Test
@dotnet-bot test Ubuntu arm Cross Checked no_tiered_compilation_innerloop Build and Test
@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test
@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0)
@dotnet-bot test Windows_NT x64 Checked CoreFX Tests
@dotnet-bot test Windows_NT x64 Checked Innerloop Build and Test
@dotnet-bot test Windows_NT x64 Release CoreFX Tests

@MichalStrehovsky
Copy link
Member Author

@MichalStrehovsky MichalStrehovsky commented Dec 13, 2018

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test
@dotnet-bot test Ubuntu arm Cross Checked Innerloop Build and Test
@dotnet-bot test Ubuntu arm Cross Checked no_tiered_compilation_innerloop Build and Test
@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test
@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0)

1 similar comment
@MichalStrehovsky
Copy link
Member Author

@MichalStrehovsky MichalStrehovsky commented Dec 14, 2018

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test
@dotnet-bot test Ubuntu arm Cross Checked Innerloop Build and Test
@dotnet-bot test Ubuntu arm Cross Checked no_tiered_compilation_innerloop Build and Test
@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test
@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0)

@MichalStrehovsky MichalStrehovsky merged commit 1649da1 into dotnet:master Dec 14, 2018
31 checks passed
@MichalStrehovsky MichalStrehovsky deleted the twopassvariance branch Dec 14, 2018
picenka21 pushed a commit to picenka21/runtime that referenced this issue Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants