System.Linq.Enumerable.OrderByDescending() should warn about .netfx incompatibility with IComparer<T>.Compare() returning int.MinValue

[binki]

Please document that System.Linq.Enumerable.OrderByDescending() on .netfx does not support IComparer<T>.Compare() returning int.MinValue: https://stackoverflow.com/q/35537357 .

This is important for people targeting not only .netfx but various versions of .netstandard.

[binki]

The documentation for IComparer<T>.Compare() should also document the incompatibility.

[KalleOlaviNiemitalo]

dotnet/runtime#14793 reported the bug that was fixed in .NET Core.

[binki]

Right, but it still exists in .netfx and the behavior will probably be preserved there because bug fixes are breaking changes.

[carlossanlop]

Tagging area owners @eiriktsarpalis @adamsitnik for triaging.

[eiriktsarpalis]

This is a rare corner case that only impacts full framework. I'm not sure it meets the bar for addition to documentation, but happy to be corrected on this.

[binki]

@eiriktsarpalis

This is a rare corner case that only impacts full framework. I'm not sure it meets the bar for addition to documentation, but happy to be corrected on this.

But this is a very weird and confusing behavior. One would assume that it is just as safe to return -int.MinValue as it is to return -1 and may even use it intentionally. However, if the documentation for this interface stated that returning such a value is unsupported, at least it gives the programmer a chance to avoid a very confusing and hard to debug issue—because one expects .OrderByDescending() to always work correctly.

Even if it only affects full framework, any code written targeting <.netstandard-2.1 needs to be written to be compatible with .netfx. Otherwise, why target .netstandard?

[eiriktsarpalis]

One would assume that it is just as safe to return -int.MinValue as it is to return -1 and may even use it intentionally.

Do we have evidence of this being used at all? It's the first time I'm seeing this. -1, 0 and 1 is the established convention.

any code written targeting <.netstandard-2.1 needs to be written to be compatible with .netfx.

This is always true. Netstandard simply defines a set of APIs but does not guarantee that runtime implementations are observationally equivalent, primarily due to recent bugfixes and intentionally introduced breaking changes. If maintaining a netstandard2.0 library that explicitly supports .NET framework you should always target netfx in your unit tests.

[binki]

@eiriktsarpalis

One would assume that it is just as safe to return -int.MinValue as it is to return -1 and may even use it intentionally.

Do we have evidence of this being used at all? It's the first time I'm seeing this. -1, 0 and 1 is the established convention.

I did so in a closed codebase where the returned value was the difference between two numbers clamped to a range of int.MinValue through int.MaxValue. That is, until we noticed it was behaving incorrectly (only when used with .OrderByDescending()—worked fine with .OrderBy()) and I tracked down https://github.com/microsoft/referencesource/blob/a48449cb48a9a693903668a71449ac719b76867c/System.Core/System/Linq/Enumerable.cs#L2660 .

any code written targeting <.netstandard-2.1 needs to be written to be compatible with .netfx.

This is always true. Netstandard simply defines a set of APIs but does not guarantee that runtime implementations are observationally equivalent, primarily due to recent bugfixes and intentionally introduced breaking changes. If maintaining a netstandard2.0 library that explicitly supports .NET framework you should always target netfx in your unit tests.

But the standard interface documentation is worthless if it doesn’t document the behaviors that one can and can’t expect of the APIs in it. If the behavior is undefined if int.MinValue is returned (because different implementations behave differently for that) but all implementations are guaranteed to function for values >int.MinValue, that should be stated. Then you can waste less time finding out why int.MinValue causes a failure because it’s at least documented.

It seems that you are saying that .netstandard defines a set of method signatures without any documentation. You are saying that the only thing the standard guarantees is that I will not get linking errors (MethodMissingException) when I run my program against a framework which implements the .netstandard I built against. That is not true. When I look at https://docs.microsoft.com/en-us/dotnet/api/system.linq.enumerable.orderbydescending?view=netstandard-2.0 , I see the following which describes the behavior that I should be able to expect out of the API when using it:

Returns

IOrderedEnumerable<TSource>

An IOrderedEnumerable<TElement> whose elements are sorted in descending order according to a key.

However, when my key includes int.MinValue for certain elements, the resulting enumerable is not sorted in descending order according to that key.

Documenting undefined behavior is very useful to the developer and should be done…