Queryable.{Max,Min}By overloads accept an IComparer<TSource> instead of IComparer<TKey>

[jcracknell]

runtime/src/libraries/System.Linq.Queryable/src/System/Linq/Queryable.cs

Line 1878 in 98b365d

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

It's entirely possible nobody has ever used these overloads as I am unclear what an IQueryable implementation would do with the comparer, however the analogous Enumerable methods appear to correctly accept an IComparer<TKey>:

runtime/src/libraries/System.Linq/src/System/Linq/Max.cs

Line 421 in 98b365d

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

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-linq
See info in area-owners.md if you want to be subscribed.

[KalleOlaviNiemitalo]

In the approved API #27687 (comment), the methods in System.Linq.Queryable were:

public static TSource MinBy<TSource, TResult>(this IQueryable<TSource> source, Expression<Func<TSource, TResult>> selector, IComparer<TResult>? comparer);

public static TSource MaxBy<TSource, TResult>(this IQueryable<TSource> source, Expression<Func<TSource, TResult>> selector, IComparer<TResult>? comparer);

but in #50335, they are:

public static TSource? MinBy<TSource, TKey>(this System.Linq.IQueryable<TSource> source, System.Linq.Expressions.Expression<System.Func<TSource, TKey>> keySelector, System.Collections.Generic.IComparer<TSource>? comparer);

public static TSource? MaxBy<TSource, TKey>(this System.Linq.IQueryable<TSource> source, System.Linq.Expressions.Expression<System.Func<TSource, TKey>> keySelector, System.Collections.Generic.IComparer<TSource>? comparer);

If the correct methods are added, can the incorrect methods be deleted (breaking binary compatibility), or do they have to be deprecated (perhaps making some calls ambiguous)?

[eiriktsarpalis]

Oh my. Out of curiosity, what happens if you try to execute that method using a TKey that doesn't match TSource?

[eiriktsarpalis]

This is what happens when you call a method with a TKey not matching TSource:

Enumerable.Range(1, 10)
    .AsQueryable()
    .MaxBy(key => key.ToString(), Comparer<int>.Default);

Giving

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

It seems that we might just afford to make a breaking change, but if not we could just mark the incorrect overload as obsolete/EB.Never/OverloadResolutionPriority(int.MinValue).

cc @stephentoub @jkotas

[KalleOlaviNiemitalo]

SharpLab

using System;
using System.Collections.Generic;
using System.Linq;
public class C {
    public static void Main() {
        string[] numbers = { "one", "two", "three", "four" };

        string? longest = Enumerable.MaxBy<string, int>(numbers, element => element.Length, Comparer<int>.Default);
        Console.WriteLine($"Enumerable: {longest}");

        longest = Queryable.MaxBy<string, int>(numbers.AsQueryable(), element => element.Length, Comparer<string>.Default);
        Console.WriteLine($"Queryable: {longest}");
    }
}

Results

Enumerable: three

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.Queryable.MaxBy[TSource,TKey](IQueryable`1 source, Expression`1 keySelector, IComparer`1 comparer)
   at C.Main()
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

[IDisposable]

I've got this completed as a breaking change since this could NEVER have actually worked when the TKey and TSource types differred.

See PR #113944

This requires an APICompat update with /p:ApiCompatGenerateSuppressionFile=true