clarify API Compatibility rules for virtual keyword

[smasher164]

The policy around adding a virtual keyword was decided back in dotnet/runtime#21750. While this may technically be a breaking change, this method of making a method virtual has been used in the past in a backwards-compatible way. C# users in particular have treated adding a virtual keyword as a legal API change.

---

Interface members are dynamically dispatched, regardless of whether the virtual keyword is present. However, the dispatch for a sealed interface method can be call, although the C# compiler generates callvirt.

Even if the dispatch is always the same, this could potentially be a breaking change if an interface member is changed from sealed to virtual or vice-versa. For example:

using System;

B b = new C();
b.F(); // prints "B" if sealed, "C" otherwise

interface B {
    public sealed void F() {
        Console.WriteLine("B");
    }
}

public class C: B {
    public void F() {
        Console.WriteLine("C");
    }
}

---

Perhaps API compatibility checkers should introduce the above checks when an additional flag is passed? Strict mode is already taken to mean bidirectional checking, so maybe a Full flag to indicate that all of the rules are verified?

/cc @ericstj @MichalStrehovsky @joperezr @ViktorHofer

[ericstj]

I would personally consider this "gray area". While I agree that someone could break a consumer by adding a virtual, they do not necessarily break the consumer. Code compiled against a version without the virtual will continue to run against the version with the virtual (nothing fails to bind, the same method will be called). Contrast that to other cases like removing virtual or removing a member - those will result in customer code failing to bind.

In your interface example perhaps the addition of sealed would be the breaking case that we should capture as a compatibility issue. If someone were to add sealed to a default interface method it would prevent derived types method's (potentially outside the assembly) from being called.

[MichalStrehovsky]

There are two non-obvious things at play here:

1. Semantics method declarations change when you're within an interface declaration. When you're within an interface, int Foo() => 123 and virtual int Foo() => 123 mean the same thing, because interface methods are implicitly virtual in C# (like in classes in Java). Adding sealed will just make the method not-virtual. So removing sealed in your example turned a regular instance method into a default interface method.
2. C# compiler will secretly make public methods virtual if they could provide implementation to an interface. Only virtual methods can provide interface method implementations.

Because of 2, this is only a breaking change if you recompile the assembly that contains C. When you recompile, the C# compiler will make C.F virtual, and it will suddenly implement the interface. But without C.F being virtual, things will continue working as usual.

The problem is really with introducing any default interface methods. Introducing a default interface method will make unrelated existing methods on existing types that have the same name and signature as the added method implement the default method. It's usually not desirable for that to happen. The runtime repo has been mostly staying away from default interface methods. They don't solve the problems one would think they solve.

[smasher164]

@ericstj

perhaps the addition of sealed would be the breaking case that we should capture as a compatibility issue

I agree that the addition of sealed is something that should be warned about. Not only could it prevent derived types' methods from being called, but from my discussion with @jaredpar, the dispatch mechanism for sealed interface methods could also be call.

@MichalStrehovsky

Introducing a default interface method will make unrelated existing methods on existing types that have the same name and signature as the added method implement the default method.

Currently if any interface method is introduced that does not have a default implementation, we emit a diagnostic. However, if there is a potential breakage even with a default implementation, is it even possible to detect that without looking at existing types that implement the interface?

Some method will be invoked regardless, whether or not that's surprising to users. Personally, I feel that this shouldn't be considered ABI-breaking, and could instead be documented where users learn about DIMs.

[MichalStrehovsky]

However, if there is a potential breakage even with a default implementation, is it even possible to detect that without looking at existing types that implement the interface?

There isn't. The breaking change rules usually follow the rule that "if a reasonable program would be broken by the change, the change is considered breaking". Of course, "reasonable program" is the grey area.

The problem you're describing (an unrelated method on a class will suddenly become an implementation of the interface method) is caused by the default method on its own, not by the removal of sealed. If it's something we wish to detect, we should flag any default methods added to existing interfaces (it would also flag this).

It's fine to add a new interface with default-implemented methods, but not a method to an existing interface.

[smasher164]

I put up a PR that emits a diagnostic when sealed is added to an interface member.