Runtime: Make IOrderedEnumerable<T> covariant

Created on 11 Jul 2015 · 28Comments · Source: dotnet/runtime

Today, I was trying to convert an IOrderedEnumerable<string> to an IOrderedEnumerable<object>, but the compiler responded with an error. Looking into the interface, there doesn't seem to be any reason why it can't be covariant, and in fact when I copied and pasted the interface and changed the class signature to IOrderedEnumerable<out T> everything compiled fine.

Can we change this in the BCL, as well? There doesn't seem to be a reason not to.

Sidenote: This may cause some breaking changes, but as the post points out this has already been done with IEnumerable<T> on a greater scale.

Proposed API

C# // Updates existing interface in being covariant. public interface IOrderedEnumerable<out TElement> : IEnumerable<TElement> { // Existing member IOrderedEnumerable<TElement> CreateOrderedEnumerable<TKey>(Func<TElement, TKey> keySelector, IComparer<TKey> comparer, bool descending); }

(edited by @terrajobst to include the API write-up from @JonHanna)

api-approved area-System.Linq up-for-grabs

Source

jamesqo

👍1

Most helpful comment

Seems like it's only a source breaking change. While unfortunate, we believe the value add for supporting covariance is higher.

terrajobst on 22 Nov 2016

👍2

All 28 comments

Just noticed that in the first link I posted, the table at the bottom lists IOrderedEnumerable<T> as covariant, when it's the only one there that's not (the out is missing). Was this a mistake by the writers, maybe?

jamesqo on 11 Jul 2015

I'm surprised to learn that it isn't covariant, as it does seem to naturally be a candidate for it. I suppose the main potential flaw is if there are current uses of CreateOrderedEnumerable that fail when called covariantly, especially as used by Enumerable.ThenBy

JonHanna on 11 Jul 2015

@hackcraft Except for the corner case in the link I posted, I don't see how existing implementations could break (but I definitely might be wrong). Can you post an example?

jamesqo on 12 Jul 2015

If you change the interface as suggested, then the following works fine:

[Fact]
public void CovariantOrdered()
{
    var ordered = Enumerable.Range(0, 100).Select(i => i.ToString()).OrderBy(i => i.Length);
    IOrderedEnumerable<IComparable> o = ordered;
    Assert.Equal(
        Enumerable.Range(0, 100).Select(i => i.ToString()).OrderBy(i => i.Length).ThenBy(i => i).ToList(),
        o.ThenBy(i => i).Cast<string>().ToList()
        );
}

The question is whether there's a trickier case, or another implementation of IOrderedEnumerable where this is a problem. I can't think of a way that there would be, but I'll try to come up with something this breaks. (You can write something that it allows to compile which throws, if your key-selector or comparer does casts, but you wouldn't have been able to get it to compile before).

I note that IOrderedQueryable<T> is covariant.

I'm beginning to think that IOrderedEnumerable<T> not being covariant may be a bug in the implementation rather than a lack in the API's design. As you note, some documentation does suggest it is indeed covariant. Perhaps the combination of such covariance with Func<T, TResult> being contravariant on T threw someone; it is actually necessary to make covariance on IOrderedEnumerable<T> work, but its more convoluted than most covariant/contravariant cases.

JonHanna on 22 Jul 2015

A further consideration. The following does not currently compile:

IOrderedEnumerable<IComparable> ordered = Enumerable
  .Range(0, 200)
  .Select(i => i.ToString())
  .OrderBy(i => i.Length);

Which means that while the following does compile:

IOrderedEnumerable<IComparable> ordered = Enumerable
  .Range(0, 200)
  .AsQueryable()
  .Select(i => i.ToString())
  .OrderBy(i => i.Length);
ordered = ordered.ThenBy(i => i);

Any attempt to use ordered tries to do what the first code that won't compile does, and of course can't find a matching method, so you get an InvalidOperationException.

This would seem to be a clear bug. Indeed, it's the potential bug that the test case Queryable_Tests.MatchSequencePattern() tries to catch; if you can do something with a queryable you should be able to do it with an enumerable, and exceptions to that are bugs.

JonHanna on 24 Jul 2015

Well, there's two commits that fix this, one relative to master should the absence be considered a bug, one relative to future should its future presence be considered an API addition.

JonHanna on 24 Jul 2015

Seems reasonable to me.

@ericstj We've done such as change in the past with IEnumerable<T>. Any other compat concerns?

terrajobst on 5 Jan 2016

The change to IEnumerable was made between 3.5 / 4.0 which weren't in-place so that's a little different. My impression is that the nature of this change is only source breaking but I'd need to try it out to be sure.

What's the runtime behavior of this if the implementation doesn't have the fix but the reference assembly does? In other words is the reference assembly change sufficient? If not then we need to consider this an API change since it will need to be brought back to desktop in a new version.

@JonHanna can you mock up an example compiled against the old surface area that would observe the difference, similar to the one in the SO post. Can you also try compiling against the new surface area but running on old.

ericstj on 5 Jan 2016

@ericstj It's source-breaking but not breaking on previously compiled code. Take a look at https://github.com/JonHanna/corefx/commit/9e93882f8857d17b4d5ecb8aaf85d72ead8abba8

This has the following simple test, based on the SO. It fails either way, but differently in each case:

``` C#
public class Base
{
public void DoSomething(IOrderedEnumerable strings)
{
Assert.Equal("", "Base");
}
}

public class Derived : Base
{
public void DoSomething(IOrderedEnumerable

Runtime: Make IOrderedEnumerable<T> covariant

Proposed API

Most helpful comment

All 28 comments

Proposed API

Related issues