Fix declared type of custom comparer for MaxBy and MinBy (#113944)

* Fix declared type of custom comparer for MaxBy and MinBy

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.

* Update to keep the old MinBy and MaxBy around as Obsolete

Ensures the binary and source compatibility in case anyone was using the broken overloads.

* PR Feedback: Fixed missing markdown escaping for < and >

docs/project/list-of-diagnostics.md

@@ -115,6 +115,7 @@ The PR that reveals the implementation of the `<IncludeInternalObsoleteAttribute
 |  __`SYSLIB0058`__ | KeyExchangeAlgorithm, KeyExchangeStrength, CipherAlgorithm, CipherStrength, HashAlgorithm and HashStrength properties of SslStream are obsolete. Use NegotiatedCipherSuite instead. |
 |  __`SYSLIB0059`__ | SystemEvents.EventsThreadShutdown callbacks are not run before the process exits. Use AppDomain.ProcessExit instead. |
 |  __`SYSLIB0060`__ | The constructors on Rfc2898DeriveBytes are obsolete. Use the static Pbkdf2 method instead. |
+|  __`SYSLIB0061`__ | The Queryable MinBy and MaxBy taking an IComparer\<TSource\> are obsolete. Use the new ones that take an IComparer\<TKey\>. |
 
 ## Analyzer Warnings
 
@@ -185,7 +186,7 @@ The diagnostic id values reserved for .NET Libraries analyzer warnings are `SYSL
 |  __`SYSLIB1059`__ | Marshaller type does not support allocating constructor |
 |  __`SYSLIB1060`__ | Specified marshaller type is invalid |
 |  __`SYSLIB1061`__ | Marshaller type has incompatible method signatures |
-|  __`SYSLIB1062`__ | Project must be updated with '<AllowUnsafeBlocks>true</AllowUnsafeBlocks>' |
+|  __`SYSLIB1062`__ | Project must be updated with '\<AllowUnsafeBlocks\>true\</AllowUnsafeBlocks\>' |
 |  __`SYSLIB1063`__ | _`SYSLIB1063`-`SYSLIB1069` reserved for Microsoft.Interop.LibraryImportGenerator._ |
 |  __`SYSLIB1064`__ | _`SYSLIB1063`-`SYSLIB1069` reserved for Microsoft.Interop.LibraryImportGenerator._ |
 |  __`SYSLIB1065`__ | _`SYSLIB1063`-`SYSLIB1069` reserved for Microsoft.Interop.LibraryImportGenerator._ |

src/libraries/Common/src/System/Obsoletions.cs

@@ -192,6 +192,9 @@ namespace System
         internal const string Rfc2898DeriveBytesCtorMessage = "The constructors on Rfc2898DeriveBytes are obsolete. Use the static Pbkdf2 method instead.";
         internal const string Rfc2898DeriveBytesCtorDiagId = "SYSLIB0060";
 
+        internal const string QueryableMinByMaxByTSourceObsoleteMessage = "The Queryable MinBy and MaxBy taking an IComparer<TSource> are obsolete. Use the new ones that take an IComparer<TKey>.";
+        internal const string QueryableMinByMaxByTSourceObsoleteDiagId = "SYSLIB0061";
+
         // When adding a new diagnostic ID, add it to the table in docs\project\list-of-diagnostics.md as well.
         // Keep new const identifiers above this comment.
     }

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

@@ -128,12 +128,20 @@ namespace System.Linq
         public static long LongCount<TSource>(this System.Linq.IQueryable<TSource> source) { throw null; }
         public static long LongCount<TSource>(this System.Linq.IQueryable<TSource> source, System.Linq.Expressions.Expression<System.Func<TSource, bool>> predicate) { throw null; }
         public static TSource? MaxBy<TSource, TKey>(this System.Linq.IQueryable<TSource> source, System.Linq.Expressions.Expression<System.Func<TSource, TKey>> keySelector) { throw null; }
+        [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
+        [System.Runtime.CompilerServices.OverloadResolutionPriorityAttribute(-1)]
+        [System.ObsoleteAttribute("The Queryable MinBy and MaxBy taking an IComparer<TSource> are obsolete. Use the new ones that take an IComparer<TKey>.", DiagnosticId="SYSLIB0061", UrlFormat="https://aka.ms/dotnet-warnings/{0}")]
         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) { throw null; }
+        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) { throw null; }
         public static TSource? Max<TSource>(this System.Linq.IQueryable<TSource> source) { throw null; }
         public static TSource? Max<TSource>(this System.Linq.IQueryable<TSource> source, System.Collections.Generic.IComparer<TSource>? comparer) { throw null; }
         public static TResult? Max<TSource, TResult>(this System.Linq.IQueryable<TSource> source, System.Linq.Expressions.Expression<System.Func<TSource, TResult>> selector) { throw null; }
         public static TSource? MinBy<TSource, TKey>(this System.Linq.IQueryable<TSource> source, System.Linq.Expressions.Expression<System.Func<TSource, TKey>> keySelector) { throw null; }
+        [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
+        [System.Runtime.CompilerServices.OverloadResolutionPriorityAttribute(-1)]
+        [System.ObsoleteAttribute("The Queryable MinBy and MaxBy taking an IComparer<TSource> are obsolete. Use the new ones that take an IComparer<TKey>.", DiagnosticId="SYSLIB0061", UrlFormat="https://aka.ms/dotnet-warnings/{0}")]
         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) { throw null; }
+        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<TKey>? comparer) { throw null; }
         public static TSource? Min<TSource>(this System.Linq.IQueryable<TSource> source) { throw null; }
         public static TSource? Min<TSource>(this System.Linq.IQueryable<TSource> source, System.Collections.Generic.IComparer<TSource>? comparer) { throw null; }
         public static TResult? Min<TSource, TResult>(this System.Linq.IQueryable<TSource> source, System.Linq.Expressions.Expression<System.Func<TSource, TResult>> selector) { throw null; }

src/libraries/System.Linq.Queryable/src/System.Linq.Queryable.csproj

@@ -23,4 +23,9 @@
     <Reference Include="System.Runtime" />
   </ItemGroup>
 
+  <ItemGroup>
+    <Compile Include="$(CommonPath)System\Obsoletions.cs"
+             Link="Common\System\Obsoletions.cs" />
+  </ItemGroup>
+
 </Project>

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

@@ -3,8 +3,10 @@
 
 using System.Collections;
 using System.Collections.Generic;
+using System.ComponentModel;
 using System.Diagnostics.CodeAnalysis;
 using System.Linq.Expressions;
+using System.Runtime.CompilerServices;
 
 namespace System.Linq
 {
@@ -1780,11 +1782,14 @@ namespace System.Linq
         /// <typeparam name="TKey">The type of key to compare elements by.</typeparam>
         /// <param name="source">A sequence of values to determine the minimum value of.</param>
         /// <param name="keySelector">A function to extract the key for each element.</param>
-        /// <param name="comparer">The <see cref="IComparer{TKey}" /> to compare keys.</param>
+        /// <param name="comparer">The <see cref="IComparer{TSource}" /> to compare elements.</param>
         /// <returns>The value with the minimum key in the sequence.</returns>
         /// <exception cref="ArgumentNullException"><paramref name="source" /> is <see langword="null" />.</exception>
-        /// <exception cref="ArgumentException">No key extracted from <paramref name="source" /> implements the <see cref="IComparable" /> or <see cref="IComparable{TKey}" /> interface.</exception>
+        /// <exception cref="ArgumentException">No key extracted from <paramref name="source" /> implements the <see cref="IComparable" /> or <see cref="IComparable{TSource}" /> interface.</exception>
         [DynamicDependency("MinBy`2", typeof(Enumerable))]
+        [Obsolete(Obsoletions.QueryableMinByMaxByTSourceObsoleteMessage, DiagnosticId=Obsoletions.QueryableMinByMaxByTSourceObsoleteDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]
+        [EditorBrowsable(EditorBrowsableState.Never)]
+        [OverloadResolutionPriority(-1)]
         public static TSource? MinBy<TSource, TKey>(this IQueryable<TSource> source, Expression<Func<TSource, TKey>> keySelector, IComparer<TSource>? comparer)
         {
             ArgumentNullException.ThrowIfNull(source);
@@ -1799,6 +1804,30 @@ namespace System.Linq
                     Expression.Constant(comparer, typeof(IComparer<TSource>))));
         }
 
+        /// <summary>Returns the minimum value in a generic <see cref="IQueryable{T}"/> according to a specified key selector function.</summary>
+        /// <typeparam name="TSource">The type of the elements of <paramref name="source" />.</typeparam>
+        /// <typeparam name="TKey">The type of key to compare elements by.</typeparam>
+        /// <param name="source">A sequence of values to determine the minimum value of.</param>
+        /// <param name="keySelector">A function to extract the key for each element.</param>
+        /// <param name="comparer">The <see cref="IComparer{TKey}" /> to compare keys.</param>
+        /// <returns>The value with the minimum key in the sequence.</returns>
+        /// <exception cref="ArgumentNullException"><paramref name="source" /> is <see langword="null" />.</exception>
+        /// <exception cref="ArgumentException">No key extracted from <paramref name="source" /> implements the <see cref="IComparable" /> or <see cref="IComparable{TKey}" /> interface.</exception>
+        [DynamicDependency("MinBy`2", typeof(Enumerable))]
+        public static TSource? MinBy<TSource, TKey>(this IQueryable<TSource> source, Expression<Func<TSource, TKey>> keySelector, IComparer<TKey>? comparer)
+        {
+            ArgumentNullException.ThrowIfNull(source);
+            ArgumentNullException.ThrowIfNull(keySelector);
+
+            return source.Provider.Execute<TSource>(
+                Expression.Call(
+                    null,
+                    new Func<IQueryable<TSource>, Expression<Func<TSource, TKey>>, IComparer<TKey>, TSource?>(MinBy).Method,
+                    source.Expression,
+                    Expression.Quote(keySelector),
+                    Expression.Constant(comparer, typeof(IComparer<TKey>))));
+        }
+
         [DynamicDependency("Max`1", typeof(Enumerable))]
         public static TSource? Max<TSource>(this IQueryable<TSource> source)
         {
@@ -1870,11 +1899,14 @@ namespace System.Linq
         /// <typeparam name="TKey">The type of key to compare elements by.</typeparam>
         /// <param name="source">A sequence of values to determine the maximum value of.</param>
         /// <param name="keySelector">A function to extract the key for each element.</param>
-        /// <param name="comparer">The <see cref="IComparer{TKey}" /> to compare keys.</param>
+        /// <param name="comparer">The <see cref="IComparer{TSource}" /> to compare elements.</param>
         /// <returns>The value with the maximum key in the sequence.</returns>
         /// <exception cref="ArgumentNullException"><paramref name="source" /> is <see langword="null" />.</exception>
-        /// <exception cref="ArgumentException">No key extracted from <paramref name="source" /> implements the <see cref="IComparable" /> or <see cref="IComparable{TKey}" /> interface.</exception>
+        /// <exception cref="ArgumentException">No key extracted from <paramref name="source" /> implements the <see cref="IComparable" /> or <see cref="IComparable{TSource}" /> interface.</exception>
         [DynamicDependency("MaxBy`2", typeof(Enumerable))]
+        [Obsolete(Obsoletions.QueryableMinByMaxByTSourceObsoleteMessage, DiagnosticId=Obsoletions.QueryableMinByMaxByTSourceObsoleteDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]
+        [EditorBrowsable(EditorBrowsableState.Never)]
+        [OverloadResolutionPriority(-1)]
         public static TSource? MaxBy<TSource, TKey>(this IQueryable<TSource> source, Expression<Func<TSource, TKey>> keySelector, IComparer<TSource>? comparer)
         {
             ArgumentNullException.ThrowIfNull(source);
@@ -1889,6 +1921,30 @@ namespace System.Linq
                     Expression.Constant(comparer, typeof(IComparer<TSource>))));
         }
 
+        /// <summary>Returns the maximum value in a generic <see cref="IQueryable{T}"/> according to a specified key selector function.</summary>
+        /// <typeparam name="TSource">The type of the elements of <paramref name="source" />.</typeparam>
+        /// <typeparam name="TKey">The type of key to compare elements by.</typeparam>
+        /// <param name="source">A sequence of values to determine the maximum value of.</param>
+        /// <param name="keySelector">A function to extract the key for each element.</param>
+        /// <param name="comparer">The <see cref="IComparer{TKey}" /> to compare keys.</param>
+        /// <returns>The value with the maximum key in the sequence.</returns>
+        /// <exception cref="ArgumentNullException"><paramref name="source" /> is <see langword="null" />.</exception>
+        /// <exception cref="ArgumentException">No key extracted from <paramref name="source" /> implements the <see cref="IComparable" /> or <see cref="IComparable{TKey}" /> interface.</exception>
+        [DynamicDependency("MaxBy`2", typeof(Enumerable))]
+        public static TSource? MaxBy<TSource, TKey>(this IQueryable<TSource> source, Expression<Func<TSource, TKey>> keySelector, IComparer<TKey>? comparer)
+        {
+            ArgumentNullException.ThrowIfNull(source);
+            ArgumentNullException.ThrowIfNull(keySelector);
+
+            return source.Provider.Execute<TSource>(
+                Expression.Call(
+                    null,
+                    new Func<IQueryable<TSource>, Expression<Func<TSource, TKey>>, IComparer<TKey>, TSource?>(MaxBy).Method,
+                    source.Expression,
+                    Expression.Quote(keySelector),
+                    Expression.Constant(comparer, typeof(IComparer<TKey>))));
+        }
+
         [DynamicDependency("Sum", typeof(Enumerable))]
         public static int Sum(this IQueryable<int> source)
         {

src/libraries/System.Linq.Queryable/tests/MaxTests.cs

@@ -638,5 +638,26 @@ namespace System.Linq.Tests
             IQueryable<int> source = Enumerable.Range(1, 20).AsQueryable();
             Assert.Equal(20, source.MaxBy(x => -x, Comparer<int>.Create((x, y) => -x.CompareTo(y))));
         }
+
+        private sealed class Folk
+        {
+            public string Name { get; set; }
+            public int Age { get; set; }
+        }
+
+        [Fact]
+        public void MaxBy_CustomComparer_DistinctTypes()
+        {
+            var data = new Folk[] {
+                new Folk { Name="Doug", Age=42 },
+                new Folk { Name="John", Age=18 },
+                new Folk { Name="Bob", Age=21 }
+            };
+
+            IQueryable<Folk> source = data.AsQueryable();
+            var result = source.MaxBy(e => e.Age, Comparer<int>.Default);
+            Assert.NotNull(result);
+            Assert.Equal("Doug", result.Name);
+        }
     }
 }

src/libraries/System.Linq.Queryable/tests/MinTests.cs

@@ -606,5 +606,26 @@ namespace System.Linq.Tests
             IQueryable<int> source = Enumerable.Range(1, 20).AsQueryable();
             Assert.Equal(1, source.MinBy(x => -x, Comparer<int>.Create((x, y) => -x.CompareTo(y))));
         }
+
+        private sealed class Folk
+        {
+            public string Name { get; set; }
+            public int Age { get; set; }
+        }
+
+        [Fact]
+        public void MinBy_CustomComparer_DistinctTypes()
+        {
+            var data = new Folk[] {
+                new Folk { Name="Doug", Age=42 },
+                new Folk { Name="John", Age=18 },
+                new Folk { Name="Bob", Age=21 }
+            };
+
+            IQueryable<Folk> source = data.AsQueryable();
+            var result = source.MinBy(e => e.Age, Comparer<int>.Default);
+            Assert.NotNull(result);
+            Assert.Equal("John", result.Name);
+        }
     }
 }

src/libraries/apicompat/ApiCompatBaseline.NetCoreAppLatestStable.xml

@@ -331,4 +331,4 @@
     <Left>net9.0/System.Runtime.dll</Left>
     <Right>net10.0/System.Runtime.dll</Right>
   </Suppression>
-</Suppressions>
+</Suppressions>