OrderByDescending fails in LINQ to Objects when a comparer returns int.MinValue - by Jon Skeet

Status : 

  Won't Fix<br /><br />
		Due to several factors the product team decided to focus its efforts on other items.<br /><br />
		A more detailed explanation for the resolution of this particular item may have been provided in the comments section.

Sign in
to vote
ID 634949 Comments
Status Closed Workarounds
Type Bug Repros 2
Opened 1/6/2011 1:30:42 PM
Access Restriction Public


IComparer<T> is allowed to return *any* integer so long as it falls into the right bucket for the comparison ("less than 0", "0", "more than 0").

As part of implementing my own version of LINQ to Objects, I wrote a unit test to make sure that I can handle a custom comparer returning int.MinValue and ordering in a descending manner: a naive implementation would reverse the order of the comparison simply by negating the result of the "ascending" comparison, but that fails for int.MinValue due to the nature of signed integers. (-int.MinValue == int.MinValue).

To my surprise, when I ran my unit test against the "real" LINQ to Objects, it failed!
Sign in to post a comment.
Posted by Jon Skeet on 2/7/2013 at 12:14 PM
Now that .NET 4.5 has shipped, any chance this could be reopened for .NET 5?
Posted by Richard Deeming on 12/14/2011 at 5:24 AM
If you're concerned about changing the order of the operands, why not change the code to:

return this.descending ? 0.CompareTo(num) : num;

That should work for any value returned from the IComparer<T> implementation:
0.CompareTo(int.MinValue) == 1;
0.CompareTo(int.MaxValue) == -1;
0.CompareTo(0) == 0;
Posted by Richard Deeming on 7/27/2011 at 8:18 AM
So in order to preserve compatibility for some theoretical comparer which depends on the order of the arguments passed to it (and is therefore taking a direct dependency on the internal details of the sorting algorithm used), you're not going to fix code which fails miserably with a perfectly valid comparer implementation?

Does that mean that you're never going to change the internal details of the sorting algorithm, just in case?

Backwards compatibility has its place, but so does common sense.
Posted by Microsoft on 4/4/2011 at 2:21 PM
Thanks for reporting this issue you've encountered with Visual Studio!

This is a great catch! Assuming nobody is relying on the current non-deterministic sort order for such comparers, we could explore fixing this in a future version of the .NET Framework.

Swapping arguments in the call to comparer.Compare as you point out would be the most straightforward and general way to support this. However, while well-behaved implementations of comparer.Compare should handle this fine, there may be some implementations out there with subtle bugs that are not expecting us to reverse the order in which we supply these arguments for a given list. Given our focus on runtime compatibility for the next release, we won't be able to fix this bug in the next version of Visual Studio, though we'll definitely keep this in mind for a future release!

Alex Turner
Program Manager
Visual Basic and C# Compiler
Posted by the-configurator on 1/7/2011 at 4:00 PM
I've tracked down the problem to the method CompareKeys(int, int) in System.Linq.Enumerable.EnumerableSorter<TElement, TKey> (in System.Core of course). It does the comparison and then:

    if (!this.descending)
        return num;
    return -num;

When it should either support int.MinValue by explicitly checking for that, or simply swap index1 and index2 in the first call in this method this.comparer.Compare(this.keys[index1], this.keys[index2]);
Posted by Microsoft on 1/6/2011 at 6:59 PM
Thanks for your feedback.
We are routing this issue to the appropriate group within the Visual Studio Product Team for triage and resolution. These specialized experts will follow-up with your issue.
Posted by Microsoft on 1/6/2011 at 1:58 PM
Thank you for your feedback, we are currently reviewing the issue you have submitted. If this issue is urgent, please contact support directly(http://support.microsoft.com)