[Breaking change]: Fix type of the IComparer<T> on System.Linq.Queryable MinBy and MaxBy

[IDisposable]

Description

Change the generic type of the IComparer used in the MinBy and MaxBy in System.Linq.Queryable to the correct TKey instead of the current (and wrong) TSource.

Specifically, change wrong use of IComparer<TSource> for comparer

public static TSource? MinBy<TSource, TKey>(this IQueryable<TSource> source, Expression<Func<TSource, TKey>> keySelector, IComparer<TSource>? comparer)

public static TSource? MaxBy<TSource, TKey>(this IQueryable<TSource> source, Expression<Func<TSource, TKey>> keySelector, IComparer<TSource>? comparer)

to the correct IComparer<TKey> for comparer

public static TSource? MinBy<TSource, TKey>(this IQueryable<TSource> source, Expression<Func<TSource, TKey>> keySelector, IComparer<TKey>? comparer)

public static TSource? MaxBy<TSource, TKey>(this IQueryable<TSource> source, Expression<Func<TSource, TKey>> keySelector, IComparer<TKey>? comparer)

Version

.NET 10 Preview 4

Previous behavior

The versions of MaxBy and MinBy in System.Linq.Queryable that accepted a custom comparer were declared as taking an IComparer<TSource> when they SHOULD have been IComparer<TKey> to match what the keySelector was projecting.

Prior attempts to use a custom comparer would have only worked if TSource and TKey were the same type (e.g. both string or int). Other type combinations would result in an System.IndexOutOfRangeException as documented here in Dotnet Runtime Issue #113878 because the correct backing MaxBy or backing MinBy Method could not be located.

Note, this was never a compile-time error, it would only fail at runtime.

Unhandled exception. System.IndexOutOfRangeException: Index was outside the bounds of the array.
   at System.Linq.EnumerableRewriter.FindEnumerableMethodForQueryable(String name, ReadOnlyCollection`1 args, Type[] typeArgs)
   at System.Linq.EnumerableRewriter.VisitMethodCall(MethodCallExpression m)
   at System.Linq.EnumerableExecutor`1.Execute()
   at System.Linq.EnumerableQuery`1.System.Linq.IQueryProvider.Execute[TElement](Expression expression)
   at System.Linq.Queryable.MaxBy[TSource,TKey](IQueryable`1 source, Expression`1 keySelector, IComparer`1 comparer)
   at Program.<Main>$(String[] args) in C:\Users\eitsarpa\source\repos\ConsoleApp1\ConsoleApp1\Program.cs:line 1

Examples of this working correctly the TSource and TKey types are the both same type:

• MinBy unit test
• MaxBy unit test

New behavior

You can use a different type for TSource and TKey in the MaxBy and MinBy that accepted a custom comparer.

This means that any use will work correctly (as long as there is an IComparer<TKey>) and will no longer throw an System.IndexOutOfRangeException.

Type of breaking change

• Binary incompatible: Existing binaries might encounter a breaking change in behavior, such as failure to load or execute, and if so, require recompilation.
• Source incompatible: When recompiled using the new SDK or component or to target the new runtime, existing source code might require source changes to compile successfully.
• Behavioral change: Existing binaries might behave differently at run time.

Reason for change

The original implementation was incorrect see comment on Dotnet Runtime Issue #113878 as specified here and implemented here

Recommended action

None required, things will just quietly work now.

Feature area

LINQ

Affected APIs

System.Linq.Queryable.MinBy and System.Linq.Queryable.MaxBy Linq extension methods.

[teo-tsirpanis]

This is technically both a binary and a source breaking change since the method's signature will be altered, but the previous one was broken. 🤔

[IDisposable]

Added those checkmarks :)

[eiriktsarpalis]

We should hold off from taking any action until any changes are made to the product itself.

[IDisposable]

Reserved SYSLIB0061 for this change in dotnet/runtime#113996

[eiriktsarpalis]

Thanks. Closing this issue since I don't believe obsoleting an API counts as a breaking change.

[teo-tsirpanis]

I don't believe obsoleting an API counts as a breaking change

Some obsoletions have been documented as breaking changes (example 1, example 2, example 3).

[CamSoper]

@eiriktsarpalis We document obsoletions as breaking changes all the time. If you want to reopen, go right ahead! 🙂

[IDisposable]

Documentation completed at #45565