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

Variant static interface dispatch crashes the runtime #78621

Closed
MichalStrehovsky opened this issue Nov 21, 2022 · 8 comments · Fixed by #88639
Closed

Variant static interface dispatch crashes the runtime #78621

MichalStrehovsky opened this issue Nov 21, 2022 · 8 comments · Fixed by #88639
Assignees
Milestone

Comments

@MichalStrehovsky
Copy link
Member

using System;

Console.WriteLine(GetTheFooString<FooBar, Base>());
Console.WriteLine(GetTheFooString<FooBar, Mid>());
Console.WriteLine(GetTheFooString<FooBar, Derived>());

Console.WriteLine(GetTheBarString<FooBar, Base>());
Console.WriteLine(GetTheBarString<FooBar, Mid>());
Console.WriteLine(GetTheBarString<FooBar, Derived>());

Console.WriteLine(GetTheFooString<FooBarBaz, Base>());
Console.WriteLine(GetTheFooString<FooBarBaz, Mid>());
Console.WriteLine(GetTheFooString<FooBarBaz, Derived>());

Console.WriteLine(GetTheBarString<FooBarBaz, Base>());
Console.WriteLine(GetTheBarString<FooBarBaz, Mid>());
Console.WriteLine(GetTheBarString<FooBarBaz, Derived>());


static string GetTheFooString<T, U>() where T : IFoo<U> => T.GetString();
static string GetTheBarString<T, U>() where T : IBar<U> => T.GetString();

interface IFoo<in T> { static virtual string GetString() => $"IFoo<{typeof(T).Name}>"; };
interface IBar<out T> { static virtual string GetString() => $"IBar<{typeof(T).Name}>"; };
class FooBar : IFoo<Base>, IBar<Derived> { }

interface IBaz : IFoo<Mid>, IBar<Mid>
{
    static string IFoo<Mid>.GetString() => "IBaz";
    static string IBar<Mid>.GetString() => "IBaz";
}
class FooBarBaz : FooBar, IBaz { }

class Base { }
class Mid : Base { }
class Derived : Mid { }

Expected

Doesn't crash

Actual

IFoo<Base>
IFoo<Base>
IFoo<Base>
IBar<Derived>
IBar<Derived>
IBar<Derived>
IFoo<Base>
Fatal error. Internal CLR error. (0x80131506)
   at Program.<<Main>$>g__GetTheFooString|0_0[[System.__Canon, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.__Canon, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]]()
   at Program.<Main>$(System.String[])
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 21, 2022
@am11
Copy link
Member

am11 commented Nov 21, 2022

Expected

Doesn't crash

Fwiw, mono does the right thing. e.g. with blazorwasm net8.0 app,

paste this code in Pages/Index.razor
@page "/"

@code
{
    class C
    {
        public static string GetTheFooString<T, U>() where T : IFoo<U> => T.GetString();
        public static string GetTheBarString<T, U>() where T : IBar<U> => T.GetString();
    }

    interface IFoo<in T> { static virtual string GetString() => $"IFoo<{typeof(T).Name}>"; };
    interface IBar<out T> { static virtual string GetString() => $"IBar<{typeof(T).Name}>"; };
    class FooBar : IFoo<Base>, IBar<Derived> { }

    interface IBaz : IFoo<Mid>, IBar<Mid>
    {
        static string IFoo<Mid>.GetString() => "IBaz";
        static string IBar<Mid>.GetString() => "IBaz";
    }
    class FooBarBaz : FooBar, IBaz { }

    class Base { }
    class Mid : Base { }
    class Derived : Mid { }    
}

@(C.GetTheFooString<FooBar, Base>())<br/>
@(C.GetTheFooString<FooBar, Mid>())<br/>
@(C.GetTheFooString<FooBar, Derived>())<br/>

@(C.GetTheBarString<FooBar, Base>())<br/>
@(C.GetTheBarString<FooBar, Mid>())<br/>
@(C.GetTheBarString<FooBar, Derived>())<br/>

@(C.GetTheFooString<FooBarBaz, Base>())<br/>
@(C.GetTheFooString<FooBarBaz, Mid>())<br/>
@(C.GetTheFooString<FooBarBaz, Derived>())<br/>

@(C.GetTheBarString<FooBarBaz, Base>())<br/>
@(C.GetTheBarString<FooBarBaz, Mid>())<br/>
@(C.GetTheBarString<FooBarBaz, Derived>())

and the browser will render:

IFoo<Base>
IFoo<Base>
IFoo<Base>
IBar<Derived>
IBar<Derived>
IBar<Derived>
IFoo<Base>
IBaz
IFoo<Base>
IBar<Derived>
IBaz
IBar<Derived>

@MichalStrehovsky
Copy link
Member Author

Cc @trylek @davidwrighton for static virtual bugs

@trylek trylek self-assigned this Nov 22, 2022
@trylek trylek removed the untriaged New issue has not been triaged by the area owner label Nov 22, 2022
@trylek trylek added this to the 8.0.0 milestone Nov 22, 2022
@davidwrighton davidwrighton self-assigned this Jun 23, 2023
@davidwrighton
Copy link
Member

@lambdageek could you point me at the logic in Mono that is choosing a specific implementation.

In particular, I see that FooBarBaz implements IBaz, IFoo<Mid>, IBar<Mid>, IFoo<Base> and IBar<Derived>. For the cases which crash (which is obviously a runtime bug), there are 2 implementations which could be chosen, such as IBaz.GetString or IFoo<Base>.GetString As far as I can tell from the definition of More Specific type from the default interfaces feature, IBaz and IFoo<Base> are unrelated types even though they will both satisfy IBar<Mid>.GetString and so are considered equally specific, and therefore dispatch should simply throw an AmbiguousImplementationException.

@AlekseyTs Do you agree that this should throw at runtime with an AmbiguousImplementationException?

@MichalStrehovsky
Copy link
Member Author

Would we expect a different behavior from variant dispatch on instance default interface methods? If I change the above program to use instance methods (and pass a this) - program is at the bottom for posterity, I get:

IFoo<Base>
IFoo<Base>
IFoo<Base>
IBar<Derived>
IBar<Derived>
IBar<Derived>
IFoo<Base>
IBaz
IBaz
IBaz
IBaz
IBar<Derived>

IIRC we had a discussion about ambiguities in the variant cases for instance methods here dotnet/coreclr#21355 (comment) and over emails that I cannot find. This is all just repressed memories at this point.

Click to expand the C# program
using System;

Console.WriteLine(GetTheFooString<FooBar, Base>(new FooBar()));
Console.WriteLine(GetTheFooString<FooBar, Mid>(new FooBar()));
Console.WriteLine(GetTheFooString<FooBar, Derived>(new FooBar()));

Console.WriteLine(GetTheBarString<FooBar, Base>(new FooBar()));
Console.WriteLine(GetTheBarString<FooBar, Mid>(new FooBar()));
Console.WriteLine(GetTheBarString<FooBar, Derived>(new FooBar()));

Console.WriteLine(GetTheFooString<FooBarBaz, Base>(new FooBarBaz()));
Console.WriteLine(GetTheFooString<FooBarBaz, Mid>(new FooBarBaz()));
Console.WriteLine(GetTheFooString<FooBarBaz, Derived>(new FooBarBaz()));

Console.WriteLine(GetTheBarString<FooBarBaz, Base>(new FooBarBaz()));
Console.WriteLine(GetTheBarString<FooBarBaz, Mid>(new FooBarBaz()));
Console.WriteLine(GetTheBarString<FooBarBaz, Derived>(new FooBarBaz()));


static string GetTheFooString<T, U>(T o) where T : IFoo<U> => o.GetString();
static string GetTheBarString<T, U>(T o) where T : IBar<U> => o.GetString();

interface IFoo<in T> { string GetString() => $"IFoo<{typeof(T).Name}>"; };
interface IBar<out T> { string GetString() => $"IBar<{typeof(T).Name}>"; };
class FooBar : IFoo<Base>, IBar<Derived> { }

interface IBaz : IFoo<Mid>, IBar<Mid>
{
    string IFoo<Mid>.GetString() => "IBaz";
    string IBar<Mid>.GetString() => "IBaz";
}
class FooBarBaz : FooBar, IBaz { }

class Base { }
class Mid : Base { }
class Derived : Mid { }

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jul 10, 2023

Do you agree that this should throw at runtime with an AmbiguousImplementationException?

I do not think I agree. Variance equivalence is not part of the most specific type rule. There is only one implementation of IFoo<Mid>.GetString in the picture from that perspective.

@lambdageek
Copy link
Member

@lambdageek could you point me at the logic in Mono that is choosing a specific implementation.

In general the code is class-setup-vtable.c: mono_class_setup_vtable_general is driving. Explicit overrides are considered by apply_override which builds up a conflict_map if there are multiple candidates - at this point we just collect the possible implementations - we don't attempt to prune anything. Just the existence of a conflict map is not indication of a conflict. We call handle_dim_conflicts to consider all the entries in the conflict map for each declaration and only note an actual conflict if there is not a single most specific class left. We store the list of conflicting declarations on the MonoClass. This all happens more or less independently of filling in the vtable slots.
At JIT time if there is a call to a marked ambiguous declaration we replace the call by a throw.
So even in the ambiguous case the vtable slot points to some implementation. We just count on the execution engine to use the ambiguous declarations list and to never to emit a call to that slot.

I haven't traced this specific example to understand what Mono is doing, but it seems like the conflict map would mark at least some of the declarations as ambiguous. On the other hand the vtable will have some arbitrary implementation in there (probably in declaration order - but I haven't checked). I suspect the JIT-time check "are we calling a method declaration that has been marked as ambiguous" isn't working right for some of these cases so we end up with a call to whatever was left in the vtable.

The other possibility is that we're not computing the ambiguous declarations correctly and so we don't consider that there are conflicts here.

For the instance testcase from #78621 (comment)
Mono gives:

IFoo<Base>
IFoo<Base>
IFoo<Base>
IBar<Derived>
IBar<Derived>
IBar<Derived>
IFoo<Base>
IBaz
IFoo<Base>
IBar<Derived>
IBaz
IBar<Derived>

note: The cases for GetTheFooString<FooBarBaz, Derived>(new FooBarBaz()) and GetTheBarString<FooBarBaz, Base>(new FooBarBaz()) are different from coreclr

@davidwrighton
Copy link
Member

Ok, so the universal agreement is that this should behave exactly like the instance case does, and that Mono disagrees with CoreCLR in what that exactly means. I'll flesh these test cases out, and make a final test case (that follows CoreCLR behavior, as it looks generally more correct to me than the Mono stuff(Especially as it is more likely customers have taken a dependency on CoreCLR behavior). Also, there is no mechanism for an ambiguous match to occur in this part of the code in CoreCLR, so I may put that in another test case.

@MichalStrehovsky
Copy link
Member Author

that follows CoreCLR behavior, as it looks generally more correct to me than the Mono stuff

Sounds good - Mono is known to have differences in this area even prior to default/static interface methods (mono/mono#6312).

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 11, 2023
davidwrighton added a commit that referenced this issue Jul 11, 2023
… implementations (#88639)

- Move the handling for variance so that it is in the same place as it is for instance methods
- Validate the behavior of such dispatch is equivalent to the dispatch behavior involving instance variant default dispatch
- Rework the fix for #80350 to avoid violating an invariant of the type system (which is that all MethodDef's of a TypeDef are loaded as MethodDescs when loading the open type).

Fixes #78621
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 11, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants