Sunday, August 17, 2014

Covariant return types in C# and how to not to carefully change a library

C# doesn't allow covariant return types in interface method implementations. Briefly

class A { }
class B : A { }

interface I
{
   A M();
}

class C : I
{
   B M() { ... }
}

will not compile. class C's implementation of M must return A. In Java and C++ (with the appropriate syntactic changes) the original code will have worked as written.

I'm not going to argue strongly one way or the other for the feature of covariant return types in C#, but it's absence illuminated a corner of the pitfalls of evolving a library that don't think I would have noticed otherwise.

To set the stage I will go through the abstract evolution of a library and a client.

First the library (or framework really) appeared:

interface I { ... }
abstract class Base {
   I GetI() { ... }
  ...
}

Base has various methods intended to be overridden by clients, and its all documented as you would expect.

Then a client appears

class Client : Base {
   ...
}

The client code works happily. The library evolves some, specificly, GetI changes to return a richer object. In the library:

interface I2 : I {
  ...
}

abstract class Base {
   I2 GetI() { ... }
}

The client code continues to work happily, the values returned from GetI still do all the I things they did before, they just happen to do more. After some time the client changes again:

interface IBase {
   I2 GetI();
   ...
}

class Client : Base, IBase {
    ...
}

Some important things to note. IBase is defined in the _client_ it does not exist in the library, it was not introduced by the library, the client code just extracted it out of some of the methods on Base. This is fine, client code is allowed to create interfaces after all. The other important thing to do note is that the client does implement IBase in the body of the Client class. Rather it simply lets the implementation of Base.GetI serve as the implementation of IBase.GetI. The client has other classes that do not inherit from Base and implement IBase, i.o.w. this was not done just to have some extra code lying around. The client is "borrowing" some of the abstractions of the framework. This was down to allow reuse of the client code in contexts where the framework of the library was not being used. I mention these reasons not to have a debate about the design of the client but merely to emphasize that their was a real reason for this.

The library now changes again, once again adding more functionality to the object returned from GetI().

interface I3 : I2 {
  ...
}

class Base {
   I3 GetI() { .... }
   ...
}

This time however, this change actually breaks the compilation of the client. Because the Client class is no longer implementing IBase. IBase was created in the client, the class Client was created in the client, but the client code effectively imposed a responsibility on the library code of implementing IBase, which it no longer does.

I find this to be fascinating situation. Each individual change on the part of the library was done in a way that was mindful of not breaking consumers. Likewise it is hard to say the client code did anything wrong. Is this a flaw in the language? If so is the flaw simply not supporting covariant return types? Or is it that base classes are allowed to implicitly interfaces? Or is it that the library is actually wrong, that changing the return type of a public method, even if it's a subtype of the previous version of the method? I'm not sure what the correct answer is, but I don't think the answer is as simple as "C# should support covariant return types", because it seems to me this fixes this problem by accident. As far as I can tell the writer of IBase wasn't thinking "C# has covariant return types so library can still evolve GetI and likewise the writer of Base was not thinking "someone may write an interface and use us to implement it by simply inheriting, but that's ok because C# has covariant return types." The writer of Base was certainly thinking in terms of the LSP when they changed the return type of GetI, granted.

At the end of the day this was fixed by the client simply explicitly implementing IBase.GetI, something like:

class Client : Base, IBase {
    I2 Ibase.GetI() { return GetI(); }
}

This is unsatisfying for the obvious reasons of the library went out of its way to ensure clients wouldn't have to change, and they ended up having to anyway.

It is clear that "obey LSP" is not a sufficient principle for determining how to perform backwards compatible library changes. So here are some guidelines about what can and cannot be done in a backwards compatible manner.

First, there is a minimum of one convention that can't be checked by the compiler unfortunately. That is, the library's namespace is the library's namespace. Many of the suggestions I am going to make rely on that. Fortunately this should not be too onerous on client code, and this is generally accepted as an unwritten rule anyway. I am making it explicit here because the entire impetus for this a seemingly backwards compatible change that actually relied on some fairly intricate unwritten rules.

I will use the term "member" below to refer to both methods and  variables.

1) Prefer sealed classes. A sealed class can't be inherited from, so you may add new members to it with impunity.
2) In all classes never remove public members. This I hope is obvious.
3) In non-sealed classes never remove protected members. A protected member is effectively public to any sub class.
4) Never change the signature of a public method, even if the changes would satisfy LSP. The preceding explains why sufficiently I hope.
5) The addition of new members in a non-sealed class must be accompanied by the addition of a new type in the library's namespace, to scope them properly. This is because names from the base class are visible in sub classes, and sub classes may have already used those names. Since the names of the members themselves can't be scoped to the namespace, we take advantage of a type. This can be done in two ways:

Introduce a new subclass:

class Base { ... }
class BaseWithX : Base { MemberType newMember; ... }

Existing client subclasses will only subclass existing subclasses of Base, and so can continue to use their own newMember if they happen to have  a member named that. If they want to take advantage of newMember they have to actively change their code, inheriting from BaseWithX, at which point they will be forced to deal with the ambiguity. If they don't touch their code, this change can't impact them.

Introduce a new type (probably an enum) that serves purely to separate client overloads from library overloads

enum GetI3E { Value };

class Base
{
     I2 GetI() { ... }
     I3 GetI(GetI3E e) { ... }
}
While clients may use a name you want to use for a method in a future release, they can't use a type that's in your library's namespace that doesn't exist yet. This allows the overloading mechanism to avoid you stepping on each other's toes. Note that you cannot reuse these types for different releases, once the client has access to the type they can use it in their own overloads, so new members in new releases must come with their new types.

If these guidelines are followed it is pretty difficult to break a client inadvertently, and with the exception of (1) it should be possible to follow them for C++ as well (or with an additional written rule that classes with no virtual methods shall not be inherited from). Next time though, GetI is just going to return a sealed class instead of an interface.





No comments: