Fix declared type of custom comparer for MaxBy and MinBy

[IDisposable]

Use the TKey output of the keySelector (not the input TSource) for the the custom IComparer since that's what the keySelector will emit.

Fixes #113878

This is a semi-BREAKING CHANGE but is justifiable in that the only code that could have worked before was when TKey and TSource were the same type (e.g. both int as in the test cases)... and that code will continue to compile correctly.

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

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[KalleOlaviNiemitalo]

the only code that could have worked before was when TKey and TSource were the same type (e.g. both int as in the test cases)...

Although different types in MinBy/MaxBy did not work with EnumerableQuery<T>, some other IQueryProvider may have supported them.

and that code will continue to compile correctly

So if TSource is the same as TKey, then this pull request does not break source compatibility. It still breaks binary compatibility, though.

[IDisposable]

I created the breaking-change issue on docs dotnet/docs#45535

[IDisposable]

So if TSource is the same as TKey, then this pull request does not break source compatibility. It still breaks binary compatibility, though.

Seeking clarification, it certainly doesn't change runtime-behavior in this case (e.g. it used to work, and still does). It does change the declaration and thus the method signature, but no compile-time difference would be visible, correct?

[IDisposable]

Although different types in MinBy/MaxBy did not work with EnumerableQuery<T>, some other IQueryProvider may have supported them.

Seeking clarification again, these are static extension methods declared against static class Queryable and the code body does a Expression.Call using the "specified" method using (e.g. for MinBy)

new Func<IQueryable<TSource>, Expression<Func<TSource, TKey>>, TSource?>(MinBy).Method

This is compile-time bound to the implementation in /src/libraries/System.Linq/src/System/Linq/Min.cs

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

So how does another IQueryProvider get inserted into this?

I know I'm missing something...

[KalleOlaviNiemitalo]

This is compile-time bound to the implementation in /src/libraries/System.Linq/src/System/Linq/Min.cs

Queryable.MaxBy constructs a LINQ expression that represents calling the same Queryable.MaxBy method, and calls IQueryProvider.Execute giving the expression as an argument. If the IQueryable was created by calling Queryable.AsQueryable<T>(IEnumerable<T>), then it is an instance of EnumerableQuery<T> and implements IQueryProvider.Execute by rewriting the expression at run time to call Enumerable.MaxBy rather than Queryable.MaxBy. That rewrite then fails if the types don’t match, like the stack trace at #113878 (comment) shows.

However, if the IQueryable<T> is not an instance of EnumerableQuery<T> and was instead created by something like Entity Framework, then its IQueryProvider can implement Execute in a different way: for example, recognise that the expression represents a Queryable.MaxBy call with a supported comparer, translate the expression to SQL, and send it to a database server to be executed there. In such a translation, the type mismatch might not matter.

That scenario seems unlikely though, because the developer of the IQueryProvider would have noticed the type mismatch when implementing support for Queryable.MaxBy, especially when writing tests for whether the IQueryProvider correctly detects that the caller provided a comparer instance for which no translation is known.

[huoyaoyuan]

The area owners and FXDC haven't made a conclusion yet, please hold off for making changes. Maybe they decide to hide instead of removing the problematic one.

[KalleOlaviNiemitalo]

Seeking clarification, it certainly doesn't change runtime-behavior in this case (e.g. it used to work, and still does). It does change the declaration and thus the method signature, but no compile-time difference would be visible, correct?

The .NET Runtime currently has

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 you compile a method that calls Queryable.MaxBy

using System.Linq;
IQueryable<int> query = new[] { 1 }.AsQueryable();
_ = Queryable.MaxBy(query, item => -item, null);

then the call is like this in IL:

call !!0 [System.Linq.Queryable]System.Linq.Queryable::MaxBy<int32, int32>(class [System.Linq.Expressions]System.Linq.IQueryable`1<!!0>, class [System.Linq.Expressions]System.Linq.Expressions.Expression`1<class [System.Runtime]System.Func`2<!!0, !!1>>, class [System.Runtime]System.Collections.Generic.IComparer`1<!!0>)

Here, !!0 and !!1 mean the TSource and TKey type parameters of the Queryable.MaxBy method. So IComparer`1<!!0> means IComparer<TSource>.

If you change the method to

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<TKey>? comparer);

and try to run the previously compiled application binary on this version of .NET Runtime, then the call won’t find the method. If the source code of the application is recompiled against the new version of .NET Runtime, then the call will use IComparer`1<!!1> and it will work with that version but not with older versions.

Such a change breaking binary compatibility might still be acceptable but I want it to be clear what it breaks.

The alternative is add a new overload and hide and obsolete the old one. That would preserve binary compatibility with previously compiled assemblies, but might cause ambiguity in languages that don't support OverloadResolutionPriorityAttribute; perhaps in PowerShell.

[IDisposable]

Just let me know if I have to go back and mark the old ones obsolete! I was just proceeding based on this but would be happy to insert back the old ones :)

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).

[IDisposable]

@KalleOlaviNiemitalo thanks tons for the explainer... I'm a bit rusty on IL resolution games.

[eiriktsarpalis]

Let's hold off from merging until a final decision has been made in API review.

[eiriktsarpalis]

Removing APIs introduces a lot of complications which would rather avoid. Even though I'm still waiting on approval the direction we're going for is adding the correct APIs while obsolete the incorrect ones:

public static class Queryable
{
+   [Obsolete("SYSLIBxxxx"), EditorBrowsable(EditorBrowsableState.Never), OverloadResolutionPriority(-1)]
    public static TSource? MinBy<TSource, TKey>(this IQueryable<TSource> source, Expression<Func<TSource, TKey>> keySelector, IComparer<TSource>? comparer);
+   [Obsolete("SYSLIBxxxx"), EditorBrowsable(EditorBrowsableState.Never), OverloadResolutionPriority(-1)]
    public static TSource? MaxBy<TSource, TKey>(this IQueryable<TSource> source, Expression<Func<TSource, TKey>> keySelector, IComparer<TSource>? 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);
}

We have an obsoletion process in place which involves picking the next available SYSLIB id from the list of obsoletions in the linked document.

[eiriktsarpalis]

The above has now been approved as proposed. Please update the PR as convenient.

[IDisposable]

Obsoletions added (not sure if this will require some APICompat, but reverted my changes in that XML file). I followed the directions in the obsoletion process, so the /src/libraries/System.Linq.Queryable/ref/System.Linq.Queryable.cs file has explicit namespaces and inlined string literals, while the actual code in /src/libraries/System.Linq.Queryable/System.Linq.Queryable.cs has things pulled from the /src/libraries/Common/Obsoletions.cs file so the .csproj file has a linked inclusion.

I think this is ready for review again @eiriktsarpalis @huoyaoyuan @KalleOlaviNiemitalo @teo-tsirpanis @jeffhandley

[eiriktsarpalis]

Excellent. Thank you for the contribution.

[hez2010]

Should this be backported? Otherwise those APIs are not usable in earlier versions of .NET.

[eiriktsarpalis]

I don't think this meets the bar for servicing.

[IDisposable]

Breaking change documentation in docs PR dotnet/docs#45565