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

Proposal: support covariant return types #357

Closed
gafter opened this issue Feb 10, 2015 · 72 comments
Closed

Proposal: support covariant return types #357

gafter opened this issue Feb 10, 2015 · 72 comments
Assignees
Labels
Area-Language Design Feature Request Language-C# Language-VB Resolution-External The behavior lies outside the functionality covered by this repository

Comments

@gafter
Copy link
Member

gafter commented Feb 10, 2015

Support covariant return types. Specifically, allow an overriding method to have a more derived reference type than the method it overrides. This would apply to methods and properties, and be supported in classes and interfaces. This is one possible alternative to a this type proposed in #311.

This would be useful in the factory pattern. For example, in the Roslyn code base we would have

class Compilation ...
{
    virtual Compilation WithOptions(Options options)...
}
class CSharpCompilation : Compilation
{
    override CSharpCompilation WithOptions(Options options)...
}

The implementation of this would be for the compiler to emit the overriding method as a "new" virtual method that hides the base class method, along with a bridge method that implements the base class method with a call to the derived class method.

@HaloFour
Copy link

How would that work if a consumer called the virtual method on the base class given that the derived class can't both override and shadow the method?

public class Foo
{
    public virtual object Baz()
    {
        return "fizz";
    }
}

public class Bar : Foo
{
    public override string Baz()
    {
        return string.Concat(base.Baz(), "buzz");
    }
}

What would Bar look like in this case? What would happen if you called Foo.Baz?

@MgSam
Copy link

MgSam commented Feb 10, 2015

Yes- please please do this feature. As I'm sure you're all aware, this feature is far and away one of the most commonly asked about issues on StackOverflow and elsewhere. People expect this to "just work" and when it doesn't it throws a big wrench into the whole design of a factory pattern.

Would be interested to see the details of the compiler implementation fleshed out a little further.

@gafter
Copy link
Member Author

gafter commented Feb 11, 2015

@HaloFour although you cannot hide and override in source, the compiler can arrange to do that in IL.

@HaloFour
Copy link

@gafter Okay, great. That makes perfect sense.

@sharwell
Copy link
Member

This could work. In this example, the override returns the same type as the base method. The C# compiler generates a special attribute indicating the covariant return type. The C# compiler would use this attribute for semantic analysis (such as a method which overrides Bar.Baz), and then silently insert the necessary cast at call sites to methods with this attribute.

Callers (in other assemblies) using earlier versions of C# have no problem consuming these APIs; they just have to insert the casts themselves.

Source (C# "vNextNext"):

public class Foo
{
    public virtual object Baz()
    {
        return "fizz";
    }
}

public class Bar : Foo
{
    public override string Baz()
    {
        return string.Concat(base.Baz(), "buzz");
    }
}

public class Program
{
    public static void Main(string[] args)
    {
        Bar bar = new Bar();
        string result = bar.Baz();
        Console.WriteLine(result);
    }
}

Result (C# "vNow"):

public class Foo
{
    public virtual object Baz()
    {
        return "fizz";
    }
}

public class Bar : Foo
{
    [return: System.Runtime.CompilerServices.ReturnType(typeof(string))]
    public override object Baz()
    {
        return string.Concat(base.Baz(), "buzz");
    }
}

public class Program
{
    public static void Main(string[] args)
    {
        Bar bar = new Bar();
        string result = (string)bar.Baz();
        Console.WriteLine(result);
    }
}

@HaloFour
Copy link

@sharwell Why the need for an attribute? That would require an addition to the CLR for something that I imagine won't be implemented for C# 6.0 "vNow"/"vNext" anyway.

Why not just solve it "more correctly" in C# 7.0 "vNextNext" by having the compiler emit both the overriding method and the shadowing method with the same name and parameters? Then you don't have a type-erasure problem and any existing compiler would already support binding to the correct overload. That would allow the language support for this feature to be supported on literally any framework version and even by older versions of the compilers.

Bar would be emitted as:

public class Bar : Foo
{
    public override object Baz()
    {
        return this.Baz();
    }

    public new string Baz() // not legal C#, perfectly legal in IL
    {
        return string.Concat(base.Baz(), "buzz");
    }
}

The compiler would then bind calls to Bar.Baz to the shadowed method and calls to Foo.Baz to the virtual method, which is the current behavior of the compiler and eliminates the need for any casting.

@sharwell
Copy link
Member

@HaloFour A custom attribute would allow the compilers for multiple languages that compile to IL to provide and consume this feature to any other language capable of expressing this concept. A prime example is the ParamArrayAttribute. The proposal you describe has additional problems:

  1. The shadow method would introduce a new frame in the stack trace that would be avoided in my proposal. The method would also need to pass through the parameters, which doubles the expense of this part of the call.
  2. What would the VB compiler do if the user tries to override this method (since there are now two methods that differ only by return type)? What would older versions of the C# compiler do?
  3. The method with a covariant return type is marked as override, but the MethodInfo for the compiled method indicates it is a new slot. The MethodInfo.GetBaseDefinition would not provide the expected result.

@HaloFour
Copy link

  1. It would be tiny and very easily inlined. This also only applies to
    virtual calls through the base type.
  2. The VB and C# compilers already support this, they had to for normal
    method shadowing to work. I did test and confirm in C# 5.0 using an
    assembly written in IL.
  3. Which is already the case with shadows. Using an attribute would cause
    the same problem as the return type would not be what was expected.

Furthermore, using shadowing would allow a covariant return to be a value
type when the base return type is object and direct callers would avoid
that box. With type erasure that is unavoidable.

Lastly, this compiler candy would work regardless of which framework
version is being targeted given there is no dependency on new classes.
Projects could target 2.0 (or even 1.0) or CoreCLR or Mono.
On Feb 11, 2015 12:26 PM, "Sam Harwell" notifications@github.com wrote:

@HaloFour https://github.com/HaloFour A custom attribute would allow
the compilers for multiple languages that compile to IL to provide and
consume this feature to any other language capable of expressing this
concept. A prime example is the ParamArrayAttribute
https://msdn.microsoft.com/en-us/library/system.paramarrayattribute.aspx.
The proposal you describe has additional problems:

  1. The shadow method would introduce a new frame in the stack trace
    that would be avoided in my proposal. The method would also need to pass
    through the parameters, which doubles the expense of this part of the call.
  2. What would the VB compiler do if the user tries to override this
    method (since there are now two methods that differ only by return type)?
    What would older versions of the C# compiler do?
  3. The method with a covariant return type is marked as override, but
    the MethodInfo for the compiled method indicates it is a new slot. The
    MethodInfo.GetBaseDefinition
    https://msdn.microsoft.com/en-us/library/system.reflection.methodinfo.getbasedefinition.aspx
    would not provide the expected result.


Reply to this email directly or view it on GitHub
#357 (comment).

@sharwell
Copy link
Member

@HaloFour So we are clear, your points are presented well even if I do not draw the same conclusion at this time. I appreciate that you are challenging me really think about the position I've taken and whether or not it would work in the long run.

Several features of C# and other .NET languages are supported by custom attributes, and this list grows over time. Another prime example is ExtensionAttribute, which is used to define extension methods. Users working with older versions of .NET can still use the extension method functionality by providing their own definition of ExtensionAttribute in the absence of a framework-provided attribute.

After working through the following example, I believe the concerns 1 and 2 would be handled equally well with your proposal. Consider three classes:

  1. Class A is the base class, and defines a virtual method Foo that returns object.
  2. Class B extends A, and defines an override of Foo that returns string. In your example, this would result in a new virtual method Foo that returns string, as well as a compiler-generated "anonymous" shadow method that matches the signature of A.Foo, and simply delegates the call to B.Foo.
    • The call overhead as well as the stack frame issue can be resolved by using the tail. prefix for the call instruction, at least in all cases where boxing is not required.
    • The compiler-generated shadow method can be marked sealed to ensure types derived from B honor the covariant return type contract even when consumed from a language that does not "understand" covariant return types.
  3. Class C extends B. If C wants to override method Foo from a base type, it must override the virtual method B.Foo which returns string.

Do you have an example where the .NET framework uses shadowing when overriding a method defined in a base type?

@HaloFour
Copy link

@sharwell Thanks, although I'm really just arguing the point for @gafter since this is the implementation as he described it.

I have no examples of where .NET does this currently. I'm not aware of any language other than IL that permits this to be expressed.

I do think that it's proper if class C wants to override Foo that the return value would have to be string (or another covariant return type in other cases).

Playing with it a little more I am running into issues with ambiguity. C# can handle a single level of inheritance just fine. B can override A and provide an overriding version of Foo that returns object as well as a shadowing version returning IEnumerable<int> and C# overload resolution handles that just fine when calling B.Foo. However, if you add in C which does the same and defines a new Foo returning IList<int> then the C# compiler will fail when attempting to call C.Foo.

VB.NET is even worse in that it cannot resolve B.Foo.

If those issues cannot be resolved in a manner that would be backward compatible that would effectively limit the functionality to projects using the post-Roslyn compilers where presumably the overload resolution would be modified to handle these situations.

I'm going to play with this a little more. I'd like to hear from @gafter regarding this problem.

Update:
It seems that the Common Language Runtime spec does permit an override to have a different name and potentially different visibility. I'm going to see if I can take my sample assembly, mess with it to explicitly override using private/renamed methods and see if the compilers can consume them.

Update 2:
Through explicit use of the .override clause in IL I was able to resolve all of the overload resolution issues. I was able to override using a method with a different visibility and name.

@HaloFour
Copy link

@sharwell To ping the thread, using explicit overrides to hide the base member through changing name/visibility fixes the overload resolution problems I was experiencing with C# and VB.NET. You effectively end up with the following:

public class Foo {
    public virtual object M() {
        return "fizz";
    }
}

public class Bar : Foo {
    // sorta equivalent to explicit implementation except for overriding, which is legal in IL
    private sealed override object Foo.M() {
        return this.M();
    }

    public new virtual string M() {
        return string.Concat(base.M(), "buzz");
    }
}

Compilers looking at Bar only see one M method that returns string.

@sharwell
Copy link
Member

Any issue with using tail.call instead of just call in Bar."Foo.M"?

Certain other downsides remain:

  1. Calling ((Foo)obj).M() will result in one additional call for each method in the inheritance chain that introduces a covariant return type.
  2. There is no way using reflection to identify that string Bar.M() overrides object Foo.M().

@sharwell
Copy link
Member

Summary so far

Shadow method

Advantages:

  • Seamless compatibility with languages that do not support covariant return types, including enforcement of the rule that a derived type cannot widen the set of types returned by a base class' definition of a method
  • Compiler-only feature (no dependency on changes in the BCL)
  • Ability to call methods which return a covariant value type without boxing an intermediate result

Disadvantages:

  • Additional call overhead when calling the base method
  • Might be difficult to use reflection to determine if a method overrides a method from a base type

Attribute applied to return type

Advantages:

  • Compatible with languages that do not support covariant return types
  • Call overhead limited to a dynamic downcast (and potentially unboxing a value type), regardless of the number of covariant restrictions exist in the inheritance hierarchy
  • Less reliance on the JIT to inline calls for efficiency

Disadvantages:

  • Enforcement that a derived type not widen the set of types returned by a base definition of a method requires the compiler understand a new attribute
  • Requires a new attribute to be defined in the BCL
  • Covariant methods which return a value type produce an intermediate value that needs to be unboxed

@HaloFour
Copy link

@sharwell Looks like tail. would work just fine.

1.. If the compiler emitted a third private non-virtual method which contained the actual method logic that would probably be a little more efficient, especially if the compiler emitted a tail. call instead of callvirt to avoid the null check:

public class Bar : Foo {
    // sorta equivalent to explicit implementation except for overriding, which is legal in IL
    private sealed override object Foo.M() {
        return M_helper();
    }

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    private string M_helper() {
        return string.Concat(base.M(), "buzz");
    }

    public new virtual string M() {
        return M_helper();
    }
}

Update: Realized that this doesn't make sense. If the overriding method called some non-virtual private method then calling the virtual method on the base class wouldn't result in a call to an overriding method of the virtual shadow method.

2.. This is true, and I don't know that there is anything that can be done to directly address that.

I will be honest that part of the reason that I don't like the attribute direction is that I've been doing a lot of Java lately and the concept of type-erasure just makes my skin crawl. 😄

@HaloFour
Copy link

@sharwell Well you're definitely right about the base method overhead. Put together a test going three levels of overrides deep using the explicit overrides and shadowing strategy as well as standard overrides.

I have three classes, cleverly named C1, C2 and C3, where C3 inherits from C2 and so on. C1 defines four methods, M1 through M4, each of which has the return type of object. Method M1 and method M3 return string and method M2 and method M4 return int. In class C2 the method M1 has a return type of IConvertible and the method M2 has a return type of IEquatable<int>. In class C3 the method M1 has a return type of string and the method M2 has a return type of int. Each of the overrides simply returns a string or an int and makes no attempt to call the base method or transform the value.

In the test I create an instance of C3 and assign that to variables of each C1, C2 and C3. I then call each of the four methods in a hard loop.

Method Return Type Time
C1::M1() object 05.5152478
C1::M2() object 03.4010111
C1::M3() object 00.3548070
C1::M4() object 00.8707667
C2::M1() IConvertible 03.0672045
C2::M2() IEquatable<int> 00.8000051
C2::M3() object 01.0113014
C2::M4() object 01.9512956
C3::M1() string 00.3536757
C3::M2() int 00.3025481
C3::M3() object 00.4555915
C3::M4() object 00.8899974

Here is the IL for these methods: Covariant Returns IL

And here is the C# for the test project: Covariant Returns Test Project

I don't doubt that this test could be done a little better and perhaps there are better strategies for supporting "true" covariant returns from the compiler. Despite the overhead I still prefer a method which results in a method signature that includes the covariant type.

@gafter
Copy link
Member Author

gafter commented Mar 12, 2015

When implementing multiple levels of overriding of covariant-returning functions, it is no necessary for the compiler to generate a long chain of method invocations. The compiler can implement this with at most a single intermediate method at runtime. That is the way the Java compiler does it.

@jnm2
Copy link
Contributor

jnm2 commented Mar 18, 2015

If you're planning on shadowing, one extremely important gotcha to add to the list:
Shadowing methods can't inherit method attributes from the base method. I suppose the workaround would be for the compiler to copy all the attributes from the base method to the shadowing method. If this isn't dealt with, if you change the return value on your C# override method you will unknowingly erase all the method's attributes, including any parameter or return value attributes.

IMO shadowing is dirtier from a conceptual standpoint and a reflection standpoint.

@gafter
Copy link
Member Author

gafter commented Mar 19, 2015

@jnm2 Yes, you should expect the compiler to produce code that has the appropriate semantics.

@dgrunwald
Copy link

What about using the function to construct a delegate? new Func<string>(bar.M)

This would work fine with the shadowing solution, but how would the reflection solution handle this case?

@kbirger
Copy link

kbirger commented Apr 29, 2015

Wow. Fantastic posts from @sharwell and @HaloFour.

If you guys have any of your working samples around still, I'd love to see and try to understand a little bit better.

In general, I see more value in the attribute approach as it would scale better with multiple levels if the inlining doesn't work out as guaranteed, but one thing that I wonder about is the casting issue. Would it work with a direct cast, or would it have to be a full conversion cast?

@andersborum
Copy link

Definitely second this feature - it's a feature we've been asking for years. Although one should favor composition over inheritance, this is a feature that one expects to be available in a modern version of the language.

And no, the current workaround with shadowing the inherited member is not something the team should be proud of pushing to the developers.

@gafter
Copy link
Member Author

gafter commented Jan 19, 2016

@Thaina I don't really understand your proposal. Either the class is generic or it isn't. What kind of CLR code is generated under the covers has nothing to do with how the language is specified.

@iskiselev Yes, covariant returns should work for all kinds of implicit reference conversions (6.1.7). But adding support for contravariant method arguments would be a breaking change.

@Thaina
Copy link

Thaina commented Jan 20, 2016

@gafter The purpose of main proposal, aka covariant return type, is to be able to mark the type of virtual function to be derived class when it was overridden

Most of the time, it is used to let virtual function return derived class

So I just see that we could do it by using generic. Normally I would make a class like this

class Compilation<T> where T : Compilation<T>
{
    public virtual T WithOptions(...) { }
}

class CSharpCompilation : Compilation<CSharpCompilation>
{
    public override CSharpCompilation WithOptions(...) { }
}

What I want to propose is, we should have this keyword can be used as auto generic. So the above could be just do like this

class Compilation<this>
{
    public virtual this WithOptions(...) { }
    public static this CreateCompilation(...) { }
}

class CSharpCompilation : Compilation
{
    public override CSharpCompilation WithOptions(...) { }
}

var compilation = CSharpCompilation.CreateCompilation(...); //// this will return CSharpCompilation

Or maybe it could just allow using this as typename and the class will be generic

class Compilation
{
    public this WithOptions(...) { }
    public static this CreateCompilation(...) { }
}

class CSharpCompilation : Compilation
{
    public override CSharpCompilation WithOptions(...) { }
}

@HaloFour
Copy link

@Thaina This proposal is independent of that specific use case, although it does manage to satisfy the need specifically in overridden virtual methods. Note that modifying the arity/genericity of a class does have a real impact. Compliation and Compilation<> are two completely different types according to the CLR, complete with different names.

@iskiselev
Copy link

Will this work with read-only properties also?

@Thaina
Copy link

Thaina commented Jan 20, 2016

@HaloFour I'm not sure what's more use case we should apply to. To let covariant return type can return anything other than derived class it then make a class couple dependencies

The most agreeable use cases is the derived class like the example, other than that we should use generic to mutate type

And about the same name problem, that's why I was propose

class Compilation
{
    public this WithOptions(...) { }
    public static this CreateCompilation(...) { }
}

class CSharpCompilation : Compilation
{
    public override CSharpCompilation WithOptions(...) { }
}

@HaloFour
Copy link

@Thaina A lack of imagination is no reason to hobble a feature, particularly by trying to tie it to generics which has absolutely nothing to do with this. I use this feature frequently in Java, where it is supported, and it is useful in allowing me to avoid extraneous casts, even in cases where I'm not trying to create a fluent API. RxJersey uses it to narrow the results from reactive invoker implementations to the appropriate result type, critical for use in its fluent API. And the feature doesn't require generics in Java either.

@Thaina
Copy link

Thaina commented Jan 20, 2016

@HaloFour I was using my imagination more than you to suspect that it really useful BUT it then make a class couple dependency

It make dependencies between class like friend in C++

Suppose we support return covariant on anything

class L { }
class LL : L { }

class A
{
     virtual L GetL() { }
}
class B : A
{
     override LL GetL() { }
}

Did you see B then making chain dependency on LL?
So I want to constraint it only for derived class

Which then it would more benefit to have base class specify this keyword to return derived type. It solve the example @gafter present

And if there are any other things that would be able to change type in the future. I think that then it should be generic from the start

Something like this

class L { }
class LL : L { }

class A<T> where T : L
{
     virtual T GetL() { }
}
class A : A<L>
{
     override L GetL() { }
}
class B : A<LL>
{
     override LL GetL() { }
}

@HaloFour
Copy link

@Thaina

Suppose we support return covariant on anything. Did you see B then making chain dependency on LL?

Yes, I do see B taking a dependency on LL, because B has a dependency on LL. Being able to do that is the entire purpose of this proposal. I can then assign the result of B.GetL() directly to a variable of LL without a cast, or chain a call to a member of LL without a cast:

new B().GetL().CallLLMethod()

vs:

((LL)new B().GetL()).CallLLMethod()

Even more fun when you chain it a couple of levels.

@Thaina
Copy link

Thaina commented Jan 20, 2016

@HaloFour That is not entire purpose. Even the main example of proposal is about return the derived class, not arbitrary covariant

And now you are the one who don't use imagination. Do you see my example?

class L { }
class LL : L { }

class A<T> where T : L
{
     virtual T GetL() { }
}
class A : A<L>
{
     override L GetL() { }
}
class B : A<LL>
{
     override LL GetL() { }
}

Then when we use B it will be like this
new B().GetL().CallLLMethod() without any cast
The need of covariant can solve by generic in most case. So I think what we really need just a shortcut to some obvious generic

@HaloFour
Copy link

@Thaina No, that's one use case. I'd suggest reading the first paragraph of this proposal again. It clearly states:

Support covariant return types. Specifically, allow an overriding method to have a more derived reference type than the method it overrides. This would apply to methods and properties, and be supported in classes and interfaces.

Generics are completely unnecessary here and would create a SIGNIFICANTLY sloppier solution that CANNOT be extended to existing interfaces or classes. I'm sorry that you have problems understanding this.

@Thaina
Copy link

Thaina commented Jan 20, 2016

@HaloFour I know that is one use case so I was state most of the time

I understand the situation and I could say You are underestimate generic. Surely it not necessary to but it could solve problem in another way if you use your imagination

@sharwell
Copy link
Member

@Thaina In your example, B is no longer derived from A, and you cannot assign an instance of B to a variable of type A. Furthermore, you can also not assign B to a variable of type A<L> (the base type of A).

There are some other problems with your example as well. The primary one involves the coupling argument. Suppose you have the following:

class L { }
class LL : L { }

class A
{
     virtual L GetL() { return new L(); }
}

class B : A
{
     override L GetL() { return new LL(); }
}

As you can see, B.GetL is already coupled to LL. Changing its return type from L to LL has no impact on coupling.

I also notice that it appears that you are confusing this issue with #155. However, covariant return types are a completely different proposal from the special case described there.

@Thaina
Copy link

Thaina commented Jan 20, 2016

@sharwell For your first paragraph, that's why I was thinking about something like #155 so it would not need generic

For your last argument. I'm not confuse. I'm just thinking we could do things another way around for problem of the main proposal. I don't think a couple design like that should be used
I am very agree with self covariant because it not couple. But then maybe #155 is better than override?

And for covariant with things other than self. Should we use generic constraint instead of override? That's what I'm thinking about

@ldematte
Copy link

@iskiselev @gafter I have a unit test for that case, it will have to work as expected.

Speaking of unit tests, I need some advice. Right now I have placed all my tests in a new file, but I am not sure this is the right way to go.

I am thinking to add tests for the positive, usable, "should compile and work" cases here:
src/Compilers/Test/Resources/Core/SymbolsTests/Methods/CSMethods.cs

And the corresponding tests on (compiled) attributes etc. in
src/Compilers/CSharp/Test/Symbol/Symbols/Metadata/PE/LoadingMethods.cs

Then, place the little source snippet tests (both for positive, should compile cases and for "should give error" cases) under src/Compilers/CSharp/Test/Symbol/Symbols/

But, should I keep them in a separate file (CovariantOverrideTests.cs). or put them in an existing file, or split them among several existing files, e.g.
TypeTests.cs
OverriddenOrHiddenMembersTests.cs
InterfaceImplementationTests.cs

?

@gafter
Copy link
Member Author

gafter commented Jan 20, 2016

@ldematte Placing all of the new tests in one or more new files is probably a good way to go. Negative tests (error messages) and positive tests (program behavior) are fine to be mixed if you want, or separated, whichever feels best to you. Some people prefer they be separated, so if you want to avoid review comments suggesting that, I guess separating them is preferred. It is a good idea to keep the metadata tests separate.

@alrz
Copy link
Member

alrz commented Mar 3, 2016

Would it be possible to return a constrained generic type instead of a concrete type?

class Foo {
  public virtual object P1 { get; }
  public virtual Base P2 { get; }
}
class Foo<T,U> : Foo where T : class where U : Base {
  public override T P1 { get; }
  public override U P2 { get; }
}

@chtenb
Copy link

chtenb commented Jan 26, 2017

Is there any information available on whether this feature will make it in a future version of C#? If yes, which version?

@gafter
Copy link
Member Author

gafter commented Jan 26, 2017

@Chiel92 no, there is no information available about when this might make it into C#

@gafter
Copy link
Member Author

gafter commented Mar 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Language Design Feature Request Language-C# Language-VB Resolution-External The behavior lies outside the functionality covered by this repository
Projects
None yet
Development

No branches or pull requests