search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.98k stars 4.66k forks source link

IEnumerable.OrderBy should detect TKey parameters that are not IComparable. #704

Open TonyValenti opened 4 years ago

TonyValenti commented 4 years ago

Hi All! It would be really great if the signature for Linq's Enumerable.OrderBy was updated such that TKey had to implement IComparable. The same is true regarding ThenBy. This would make it possible to catch certain errors at compile time rather than run time.

Clockwork-Muse commented 4 years ago

Obligatory first note: Breaking change, and thus unlikely.

The reason this likely wasn't done is because of OrderBy<TSource,TKey> (this System.Collections.Generic.IEnumerable source, Func<TSource,TKey> keySelector, System.Collections.Generic.IComparer comparer) - this allows the ordering to be called on things that don't implement IComparable, because there are any number of things that don't have any "natural" sort order (person, country, bank account, etc). The other methods then just call this one with a supplied default.

And while it would be nice to catch this at compile time, this is an area that absolutely should have test coverage in consumer code, meaning it should be caught at build time, not caught by users in deployed code.

TonyValenti commented 4 years ago

@Clockwork-Muse - There are a lot of overloads of each. I think the overload you mentioned would be safe to ignore the constraint.

eiriktsarpalis commented 4 years ago

Triage: as already mentioned, this is a breaking change. However it might make sense to add a Roslyn analyzer for detecting this.