Ensure MaxBy/MinBy return first element if all keys are null. (#61364)

dotnet · Nov 9, 2021 · 61d603f · 61d603f
1 parent 057e34d
commit 61d603f
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 36 deletions.
diff --git a/src/libraries/System.Linq/src/System/Linq/Max.cs b/src/libraries/System.Linq/src/System/Linq/Max.cs
@@ -583,15 +583,22 @@ public static decimal Max(this IEnumerable<decimal> source)
 
             if (default(TKey) is null)
             {
-                while (key == null)
+                if (key == null)
                 {
-                    if (!e.MoveNext())
+                    TSource firstValue = value;
+
+                    do
                     {
-                        return value;
-                    }
+                        if (!e.MoveNext())
+                        {
+                            // All keys are null, surface the first element.
+                            return firstValue;
+                        }
 
-                    value = e.Current;
-                    key = keySelector(value);
+                        value = e.Current;
+                        key = keySelector(value);
+                    }
+                    while (key == null);
                 }
 
                 while (e.MoveNext())

diff --git a/src/libraries/System.Linq/src/System/Linq/Min.cs b/src/libraries/System.Linq/src/System/Linq/Min.cs
@@ -541,15 +541,22 @@ public static decimal Min(this IEnumerable<decimal> source)
 
             if (default(TKey) is null)
             {
-                while (key == null)
+                if (key == null)
                 {
-                    if (!e.MoveNext())
+                    TSource firstValue = value;
+
+                    do
                     {
-                        return value;
-                    }
+                        if (!e.MoveNext())
+                        {
+                            // All keys are null, surface the first element.
+                            return firstValue;
+                        }
 
-                    value = e.Current;
-                    key = keySelector(value);
+                        value = e.Current;
+                        key = keySelector(value);
+                    }
+                    while (key == null);
                 }
 
                 while (e.MoveNext())

diff --git a/src/libraries/System.Linq/tests/MaxTests.cs b/src/libraries/System.Linq/tests/MaxTests.cs
@@ -890,27 +890,27 @@ public static void MaxBy_Generic_EmptyReferenceSource_ReturnsNull()
         }
 
         [Fact]
-        public static void MaxBy_Generic_StructSourceAllKeysAreNull_ReturnsLastElement()
+        public static void MaxBy_Generic_StructSourceAllKeysAreNull_ReturnsFirstElement()
         {
-            Assert.Equal(4, Enumerable.Range(0, 5).MaxBy(x => default(string)));
-            Assert.Equal(4, Enumerable.Range(0, 5).MaxBy(x => default(string), comparer: null));
-            Assert.Equal(4, Enumerable.Range(0, 5).MaxBy(x => default(string), Comparer<string>.Create((_, _) => throw new InvalidOperationException("comparer should not be called."))));
+            Assert.Equal(0, Enumerable.Range(0, 5).MaxBy(x => default(string)));
+            Assert.Equal(0, Enumerable.Range(0, 5).MaxBy(x => default(string), comparer: null));
+            Assert.Equal(0, Enumerable.Range(0, 5).MaxBy(x => default(string), Comparer<string>.Create((_, _) => throw new InvalidOperationException("comparer should not be called."))));
         }
 
         [Fact]
-        public static void MaxBy_Generic_NullableSourceAllKeysAreNull_ReturnsLastElement()
+        public static void MaxBy_Generic_NullableSourceAllKeysAreNull_ReturnsFirstElement()
         {
-            Assert.Equal(4, Enumerable.Range(0, 5).Cast<int?>().MaxBy(x => default(int?)));
-            Assert.Equal(4, Enumerable.Range(0, 5).Cast<int?>().MaxBy(x => default(int?), comparer: null));
-            Assert.Equal(4, Enumerable.Range(0, 5).Cast<int?>().MaxBy(x => default(int?), Comparer<int?>.Create((_, _) => throw new InvalidOperationException("comparer should not be called."))));
+            Assert.Equal(0, Enumerable.Range(0, 5).Cast<int?>().MaxBy(x => default(int?)));
+            Assert.Equal(0, Enumerable.Range(0, 5).Cast<int?>().MaxBy(x => default(int?), comparer: null));
+            Assert.Equal(0, Enumerable.Range(0, 5).Cast<int?>().MaxBy(x => default(int?), Comparer<int?>.Create((_, _) => throw new InvalidOperationException("comparer should not be called."))));
         }
 
         [Fact]
-        public static void MaxBy_Generic_ReferenceSourceAllKeysAreNull_ReturnsLastElement()
+        public static void MaxBy_Generic_ReferenceSourceAllKeysAreNull_ReturnsFirstElement()
         {
-            Assert.Equal("4", Enumerable.Range(0, 5).Select(x => x.ToString()).MaxBy(x => default(string)));
-            Assert.Equal("4", Enumerable.Range(0, 5).Select(x => x.ToString()).MaxBy(x => default(string), comparer: null));
-            Assert.Equal("4", Enumerable.Range(0, 5).Select(x => x.ToString()).MaxBy(x => default(string), Comparer<string>.Create((_, _) => throw new InvalidOperationException("comparer should not be called."))));
+            Assert.Equal("0", Enumerable.Range(0, 5).Select(x => x.ToString()).MaxBy(x => default(string)));
+            Assert.Equal("0", Enumerable.Range(0, 5).Select(x => x.ToString()).MaxBy(x => default(string), comparer: null));
+            Assert.Equal("0", Enumerable.Range(0, 5).Select(x => x.ToString()).MaxBy(x => default(string), Comparer<string>.Create((_, _) => throw new InvalidOperationException("comparer should not be called."))));
         }
 
         [Theory]

diff --git a/src/libraries/System.Linq/tests/MinTests.cs b/src/libraries/System.Linq/tests/MinTests.cs
@@ -868,27 +868,27 @@ public static void MinBy_Generic_EmptyReferenceSource_ReturnsNull()
         }
 
         [Fact]
-        public static void MinBy_Generic_StructSourceAllKeysAreNull_ReturnsLastElement()
+        public static void MinBy_Generic_StructSourceAllKeysAreNull_ReturnsFirstElement()
         {
-            Assert.Equal(4, Enumerable.Range(0, 5).MinBy(x => default(string)));
-            Assert.Equal(4, Enumerable.Range(0, 5).MinBy(x => default(string), comparer: null));
-            Assert.Equal(4, Enumerable.Range(0, 5).MinBy(x => default(string), Comparer<string>.Create((_, _) => throw new InvalidOperationException("comparer should not be called."))));
+            Assert.Equal(0, Enumerable.Range(0, 5).MinBy(x => default(string)));
+            Assert.Equal(0, Enumerable.Range(0, 5).MinBy(x => default(string), comparer: null));
+            Assert.Equal(0, Enumerable.Range(0, 5).MinBy(x => default(string), Comparer<string>.Create((_, _) => throw new InvalidOperationException("comparer should not be called."))));
         }
 
         [Fact]
-        public static void MinBy_Generic_NullableSourceAllKeysAreNull_ReturnsLastElement()
+        public static void MinBy_Generic_NullableSourceAllKeysAreNull_ReturnsFirstElement()
         {
-            Assert.Equal(4, Enumerable.Range(0, 5).Cast<int?>().MinBy(x => default(int?)));
-            Assert.Equal(4, Enumerable.Range(0, 5).Cast<int?>().MinBy(x => default(int?), comparer: null));
-            Assert.Equal(4, Enumerable.Range(0, 5).Cast<int?>().MinBy(x => default(int?), Comparer<int?>.Create((_, _) => throw new InvalidOperationException("comparer should not be called."))));
+            Assert.Equal(0, Enumerable.Range(0, 5).Cast<int?>().MinBy(x => default(int?)));
+            Assert.Equal(0, Enumerable.Range(0, 5).Cast<int?>().MinBy(x => default(int?), comparer: null));
+            Assert.Equal(0, Enumerable.Range(0, 5).Cast<int?>().MinBy(x => default(int?), Comparer<int?>.Create((_, _) => throw new InvalidOperationException("comparer should not be called."))));
         }
 
         [Fact]
-        public static void MinBy_Generic_ReferenceSourceAllKeysAreNull_ReturnsLastElement()
+        public static void MinBy_Generic_ReferenceSourceAllKeysAreNull_ReturnsFirstElement()
         {
-            Assert.Equal("4", Enumerable.Range(0, 5).Select(x => x.ToString()).MinBy(x => default(string)));
-            Assert.Equal("4", Enumerable.Range(0, 5).Select(x => x.ToString()).MinBy(x => default(string), comparer: null));
-            Assert.Equal("4", Enumerable.Range(0, 5).Select(x => x.ToString()).MinBy(x => default(string), Comparer<string>.Create((_, _) => throw new InvalidOperationException("comparer should not be called."))));
+            Assert.Equal("0", Enumerable.Range(0, 5).Select(x => x.ToString()).MinBy(x => default(string)));
+            Assert.Equal("0", Enumerable.Range(0, 5).Select(x => x.ToString()).MinBy(x => default(string), comparer: null));
+            Assert.Equal("0", Enumerable.Range(0, 5).Select(x => x.ToString()).MinBy(x => default(string), Comparer<string>.Create((_, _) => throw new InvalidOperationException("comparer should not be called."))));
         }
 
         [Theory]