Invalid method is chosen by runtime as an interface implementation in presence of Default Interface Methods

[AlekseyTs]

Compile and run the following program (https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQxAgrgOyQExAagB8ABAJgAJiBGAdgFgAoAb0fLcqoDZKAWcgWQgBLTAAoAlK3YsG7OeQAqcWKMxwA7uQDCE8QG4p8xcpiqN20roOz2AX0PkH1bsT5KVASQCC5APaSbNhkjNmoAOmoATlFfMP44GAALXxwqKwc5cKiYuITknEtxfQd7BlLGEXgEADMIAGM4cm9GYPZiAGYOAAYBPJS08XIAXgA+cgAib1yk/vHrTM7qHviZgolhscmvafzSOcZyhjJyHxAmnyZyQ4hgWAR6mEoKACFyM4urxkZjrTfyZ5aTkWVGWfVS61GEy0O1m8zanQAbkIEDB0BAADbdXqrQobKEwgr7MpfI4ULwUM7NZhA8hIlFozFLbH5AZ48bkglUIkLWnI1EYrErXYQzYcoUpPbWa63GD3OqPY7PCknCiXQ4/ZVKwGBchqTQdQVg1mQ8ZaUic7nsPWURF8hmGnEiqHm8WEqWMIA==):

class Program
{
    static void Main()
    {
        Test(new C());
        Test(new C2());
    }
    
    static void Test(IA o)
    {
        System.Console.WriteLine(o.Method1());
        System.Console.WriteLine(o.Method2());
    }
}

interface IA
{
    public string Method1() => "IA.Method1";
    public string Method2() => "IA.Method2";
}

class A : IA { }

abstract class B : A { }

class C : B
{
    public string Method1() => "C.Method1";
    public virtual string Method2() => "C.Method2";
}

class A2 : IA
{
    public virtual string Method1() => "A2.Method1";
    public virtual string Method2() => "A2.Method2";
}

abstract class B2 : A2 { }

class C2 : B2
{
    new public string Method1() => "C2.Method1";
    new public virtual string Method2() => "C2.Method2";
}

Observed:

IA.Method1
C.Method2
A2.Method1
A2.Method2

Expected:

IA.Method1
IA.Method2
A2.Method1
A2.Method2

Note, the class C is at the bottom of inheritance hierarchy. It doesn't claim to extend the interface in IL and none of the methods in it claim to explicitly implement a method from the interface:

.class private auto ansi beforefieldinit C
    extends B
{
    // Methods
    .method public hidebysig 
        instance string Method1 () cil managed 
    {
        // Method begins at RVA 0x20a4
        // Code size 6 (0x6)
        .maxstack 8

        IL_0000: ldstr "C.Method1"
        IL_0005: ret
    } // end of method C::Method1

    .method public hidebysig newslot virtual 
        instance string Method2 () cil managed 
    {
        // Method begins at RVA 0x20ab
        // Code size 6 (0x6)
        .maxstack 8

        IL_0000: ldstr "C.Method2"
        IL_0005: ret
    } // end of method C::Method2

    .method public hidebysig specialname rtspecialname 
        instance void .ctor () cil managed 
    {
        // Method begins at RVA 0x20b2
        // Code size 8 (0x8)
        .maxstack 8

        IL_0000: ldarg.0
        IL_0001: call instance void B::.ctor()
        IL_0006: nop
        IL_0007: ret
    } // end of method C::.ctor

} // end of class C

It feels unexpected that C.Method2 is considered by runtime as a candidate to implement IA.Method2. Presence of DIM should make no difference here, I think.

[MichalStrehovsky]

Previous conversation: dotnet/csharplang#52 (comment)

[AaronRobinsonMSFT]

/cc @davidwrighton

[MichalStrehovsky]

Per dotnet/csharplang#52 (comment) this would likely be a breaking change.

A sample of when this would be problematic is:

interface IA
{
    void X();
}

abstract class A : IA
{
    // NOTE: no implementation for IA. C# would yell. IL or runtime don't care though.
}

class B : A
{
    public virtual void X()
    {
        Console.WriteLine("Hi");
    }
}

If we were to fix this, addition of a default implementation to IA.X would result in the default implementation being used instead of B.X when dispatching IA.X on B.

[mangod9]

Reading through the description this would required a breaking change, which would be risky to fix.

[davidwrighton]

This is by design. The spec specifies in section 12.2 of Partion II that "If there are any public virtual methods available on this class (directly or inherited) having the same name and signature as the interface method, ... then add them to the list for the corresponding methos on the interface" This applies to both explicitly and implicitly specified interfaces. (In the case where type C does not explicitly implement an interface, it is considered to be implicitly implemented via the implementation in type A.

[walczakb]

I am coming to this discussion from Stack Overflow.

Indeed, this behavior seems to agree with the specification pointed by @davidwrighton. However, to me, this design seems flawed. It leads to a crazy case of fragile base class problem, although the designers of C# seem to have put a lot of effort to eliminate all possible cases of the fragile base class problem. If I understand correctly, the point of distinguishing virtual and override in C# (in contrast to Java) is that when you write a class like this:

class C : A {
    public virtual void M() { /*...*/ }
}

you want to be sure your method M will not be accidentally called from the base class A even if it contains its own public virtual void M(). You do not expect it will be called when A is abstract and implements some interface that declares a method void M() (especially when the interface is private and you don't even know it exists). This happens even if your derived class is private:

class Library {
    private interface IA {
        string Method() => "Library.IA.Method";
    }
    public abstract class A : IA {
        public string OtherMethod() => ((IA)this).Method();
    }
}

class Program {
    private class C : Library.A {
        public virtual string Method() => "Program.C.Method";
    }
    static void Main() {
        C obj = new C();
        System.Console.WriteLine(obj.OtherMethod()); // prints Program.C.Method
    }
}

The method Library.A.OtherMethod calls Program.C.Method even though the latter seems to be inaccessible to the former. Another, very similar case:

class Library {
    private interface IA {
        string Method() => "Library.IA.Method";
    }
    public abstract class A : IA {
        public string OtherMethod() => ((IA)this).Method();
    }
}

class Program {
    private interface IC {
        string Method();
    }
    private class C : Library.A, IC {
        public string Method() => "Program.C.Method";
    }
    static void Main() {
        C obj = new C();
        System.Console.WriteLine(obj.OtherMethod()); // prints Program.C.Method
    }
}

Here again Program.C.Method is called. Although it is not declared as virtual, apparently the C# compiler makes it virtual, as it implicitly implements a method of IC.

Is this behavior really intended? I would consider it dangerous and very unexpected to C# programmers.

[davidwrighton]

Its not really a question of whether or not the behavior is sensical from a C# point of view. I agree, it clearly isn't. However, that isn't sufficient reason to break already existing code which was working before which is what would happen if the runtime changed this behavior. One of the goals of the design of default interface methods was that adding a default interface method implementation was not to be considered to be a breaking change. And while it wasn't possible in C# remove the default interface implementation for IA and have the application function (and print "Program.C.Method" it has been possible to do so in IL since v1.0 of .NET, and through various binary substitutions in circumstances where type A and type C are in difference assemblies, code where this overriding rule is used is running successfully for many, many customers. This leads me to be exceptionally cautious about attempting to simply fix the problem as any fix is likely to break far more customers than are assisted with the fix.

Arguably, what this shows is that we missed a case in our design of the adjusted metadata for default interface methods, and now we need some odd looking wart of new metadata or something that will allow the C# team to produce an dll that more exactly represents the C# semantics around interface dispatch, or we need some way for C# to specify that a method should not participate in satisfying an interface resolution for an interface specified only on a base type, or we will need to decide that the current state of affairs is ok. In general, I think what happened is that the C# team considered backwards compatibility at the C# language level, and the CLR team considered backwards compatibility at the IL level, but neither of us realized at the moment that the C# representation in IL of this form of override was never actually matching the C# semantics and that the new metadata form for default interface methods made this much more visible.

[AlekseyTs]

At the moment, I am leaning towards Won't Fixing this on both sides. For runtime, changing behavior is going to be a breaking change. For C#, once/if we get support for base calls, we can consider synthesizing explicit interface implementation for an abstract type implementing an interface via DIM.