Search code examples
c#linqsorting.net-4.6.1

OrderByDescending operates incorrectly due to integer overflow


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


Solution

  • 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