4

While browsing the .Net Framework source code for the Enumerable class it has occurred to me that its internal EnumerableSorter class CompareKeys method used for sorting has a following line:

return descending ? -c : c;

Where c is the result of the IComparer.Compare Method (T, T) method call, that doesn't actually force us to use only -1, 1 or 0 to denote ordering.

Taking into account that -Int32.MinValue == Int32.MinValue due to integer overflow, it can lead to incorrect sorting, as can be proved by the following snippet:

public class Value : IComparable<Value>
{
    private readonly Int32 _value;
    public Value(Int32 value)
    {
        this._value = value;
    }

    public Int32 CompareTo(Value other)
    {
        if (other == null)
            throw new ArgumentNullException(nameof(other));
        var cmp = this._value.CompareTo(other._value);
        if (cmp > 0)
            return Int32.MaxValue;
        if (cmp < 0)
            return Int32.MinValue;
        return 0;
    }

    public override String ToString()
    {
        return this._value.ToString();
    }
}

private static void Print<T>(String header, IEnumerable<T> values)
{
    Console.WriteLine(header);
    foreach (var item in values)
    {
        Console.WriteLine(item);
    }
    Console.WriteLine();
}

public static void Main()
{
    try
    {
        var notSorted = new[] { 1, 3, 2 }
            .Select(i => 
                new Value(i))
            .ToArray();

        Print("Not sorted", notSorted);
        Print("Sorted by", notSorted.OrderBy(item => item));
        Print("Sorted by descending", notSorted.OrderByDescending(item => item));
    }
    catch (Exception exc)
    {
        Console.WriteLine(exc);
    }
    Console.WriteLine("Press any key...");
    Console.ReadKey(true);         
}

For OrderByDescending it produces:

Sorted by descending
3
1
2

It was expected, but it is also an obviously incorrect result.

So it seems to be a defect in the .Net, though a very unlikely to occur if CompareTo implemented in a reasonable way. Am I right?

UPDATE:

As pointed by SLaks the issue has been known for a long time but hasn't been fixed despite all the new releases - https://connect.microsoft.com/VisualStudio/feedback/details/634949/orderbydescending-fails-in-linq-to-objects-when-a-comparer-returns-int-minvalue

As pointed by usr .Net Core has this issue fixed - https://github.com/dotnet/corefx/blob/35e03c78d89d02f2d3b4a1f8b277a35c88f45750/src/System.Linq/src/System/Linq/OrderedEnumerable.cs#L628

Community
  • 1
  • 1
Eugene Podskal
  • 10,270
  • 5
  • 31
  • 53
  • 1
    https://connect.microsoft.com/VisualStudio/feedback/details/634949/orderbydescending-fails-in-linq-to-objects-when-a-comparer-returns-int-minvalue – SLaks Feb 21 '16 at 14:33
  • @SLaks Thanks. My google-fu failed me this time. – Eugene Podskal Feb 21 '16 at 14:34
  • 1
    Looks fixed: https://github.com/dotnet/corefx/blob/35e03c78d89d02f2d3b4a1f8b277a35c88f45750/src/System.Linq/src/System/Linq/OrderedEnumerable.cs#L628 – usr Feb 21 '16 at 14:48
  • @usr Hmm, seems to be, but it is .Net Core. - .Net Framework 4.6.1 still has this issue. – Eugene Podskal Feb 21 '16 at 14:51
  • My understanding is that the code is mostly shared and that the .NET Framework will trail Core. – usr Feb 21 '16 at 14:53
  • One of the best things about going open source. These sorts of things get fixed. – kjbartel Feb 21 '16 at 15:02
  • @kjbartel Often it's true, but let us not fall into a fallacy that [with many eyes all bugs are shallow](http://osswatch.jiscinvolve.org/wp/2014/04/10/if-all-bugs-are-shallow-why-was-heatbleed-only-just-fixed/). – Eugene Podskal Feb 21 '16 at 15:05
  • 1
    @EugenePodskal Actually I was more referring to the countless bugs on connect which have been closed as "won't fix". I think every bug I've personally reported had been closed that way. – kjbartel Feb 21 '16 at 15:13
  • 1
    True, I never report at Connect anymore. It's a grave for bugs. Whoever is working on these issues their goal must be to close as many as possible. Maybe a pay bonus tied to issues "resolved" per day. The github repos have very responsive people (the devs). – usr Feb 21 '16 at 15:19
  • Is there a _question_ here? If so, I can't see it. Yes, you've identified a (known) bug in .NET. Yes, there are obvious work-arounds (e.g. provide your own comparer). What _specific_ "practical, answerable" programming problem are you having trouble with here? Why is this a good, [on-topic](https://stackoverflow.com/help/on-topic) question for Stack Overflow? – Peter Duniho Feb 21 '16 at 17:44
  • 2
    @usr: it's true that Connect bugs are often closed as "Won't Fix". But the problem is that due to developer resource constraints (especially when dealing with legacy APIs), it's just not important enough to spend time fixing bugs. I have reported many bugs to Connect and while some have been closed "Won't Fix", the most egregious problems were eventually fixed (e.g. an 8GB limitation on the gzip support in .NET). In any case, it really is the _only_ place one can report a bug and be sure that Microsoft will at least _consider_ fixing it. – Peter Duniho Feb 21 '16 at 17:47
  • @PeterDuniho At least this SO question is still around documenting the issue and showing up in Google search results even though https://connect.microsoft.com/ has long since died ;-). – binki Jun 04 '20 at 17:51

1 Answers1

1

Seems that there isn't much of an answer to be made, so:

As pointed by SLaks the issue has been known for a long time but has never been fixed in the .NET Framework despite all the new releases (.Net 4.6.1 as of now) - https://connect.microsoft.com/VisualStudio/feedback/details/634949/orderbydescending-fails-in-linq-to-objects-when-a-comparer-returns-int-minvalue.

The only way to avoid this problem is to not return the Int32.MinValue from CompareTo implementations.


But as pointed by usr .Net Core has this issue fixed - https://github.com/dotnet/corefx/blob/35e03c78d89d02f2d3b4a1f8b277a35c88f45750/src/System.Linq/src/System/Linq/OrderedEnumerable.cs#L628

binki
  • 7,754
  • 5
  • 64
  • 110
Eugene Podskal
  • 10,270
  • 5
  • 31
  • 53