dotnet / dotnet-api-docs

.NET API reference documentation (.NET 5+, .NET Core, .NET Framework)
https://docs.microsoft.com/dotnet/api/
Other
704 stars 1.55k forks source link

Enumerable.Min<TSource>() documentation should mention null filtering behavior #7923

Open binki opened 2 years ago

binki commented 2 years ago

In regards to null, the documentation states this:

If TSource is a reference type and the source sequence is empty or contains only values that are null, this method returns null.

It also states:

If type TSource implements IComparable, the Max(IEnumerable))) method uses that implementation to compare values. Otherwise, if type TSource implements IComparable, that implementation is used to compare values.

This leads me to believe that the minimum value will be calculated purely based on the output of IComparable<T>.Compare(T, T). That is, the output of .Min() would be the same as .OrderBy(x => x).First() and the output of .Max() would be the same as .OrderBy(x => x).Last(). Or that if Comparer<T>.Default.Compare(a, b) < 0, then the output of new[] { a, b, }.Min() would be a. However, I found the following behavior instead:

Console.WriteLine($"Comparer<string>.Default.Compare(null, \"a\") == {Comparer<string>.Default.Compare(null, "a")}");
Console.WriteLine($"new[]{{ null, \"a\", }}.OrderBy(x => x).First() == {new[] { null, "a", }.OrderBy(x => x).First()}");
Console.WriteLine($"new[]{{ null, \"a\", }}.Min() == {new[] { null, "a", }.Min()}");

with output:

Comparer<string>.Default.Compare(null, "a") == -1
new[]{ null, "a", }.OrderBy(x => x).First() ==
new[]{ null, "a", }.Min() == a

By experiencing this and examining the source, I know that .Min() and .Max() implicitly filter out any values equal to null as the first step (including nullable value types which box to null). However, this is not obvious from the documentation.

May you please alter the documentation to document this behavior?

Thanks!

dotnet-issue-labeler[bot] commented 2 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

binki commented 2 years ago

I guess this does not apply to .Max() unless you use the overload which allows passing in a custom IComparer<T> implementation or you have implemented the IComparable<T> interface. The IComparable<T> interface documentation states:

By definition, any object compares greater than null, and two null references compare equal to each other.

So in .Max() the filtering would normally have no effect on the result.

ghost commented 2 years ago

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

Issue Details
In regards to `null`, the documentation states this: > If `TSource` is a reference type and the source sequence is empty or contains only values that are `null`, this method returns `null`. It also states: > If type `TSource` implements [IComparable](https://docs.microsoft.com/en-us/dotnet/api/system.icomparable-1?view=net-6.0), the [Max(IEnumerable)](https://docs.microsoft.com/en-us/dotnet/api/system.linq.enumerable.max?view=net-6.0#system-linq-enumerable-max-1(system-collections-generic-ienumerable((-0)))) method uses that implementation to compare values. Otherwise, if type `TSource` implements [IComparable](https://docs.microsoft.com/en-us/dotnet/api/system.icomparable?view=net-6.0), that implementation is used to compare values. This leads me to believe that the minimum value will be calculated purely based on the output of `IComparable.Compare(T, T)`. That is, the output of `.Min()` would be the same as `.OrderBy(x => x).First()` and the output of `.Max()` would be the same as `.OrderBy(x => x).Last()`. Or that if `Comparer.Default.Compare(a, b) < 0`, then the output of `new[] { a, b, }.Min()` would be `a`. However, I found the following behavior instead: ```csharp Console.WriteLine($"Comparer.Default.Compare(null, \"a\") == {Comparer.Default.Compare(null, "a")}"); Console.WriteLine($"new[]{{ null, \"a\", }}.OrderBy(x => x).First() == {new[] { null, "a", }.OrderBy(x => x).First()}"); Console.WriteLine($"new[]{{ null, \"a\", }}.Min() == {new[] { null, "a", }.Min()}"); ``` with output: ``` Comparer.Default.Compare(null, "a") == -1 new[]{ null, "a", }.OrderBy(x => x).First() == new[]{ null, "a", }.Min() == a ``` By experiencing this and examining [the](https://github.com/dotnet/runtime/blob/main/src/libraries/System.Linq/src/System/Linq/Min.cs#L614) [source](https://github.com/dotnet/runtime/blob/e3ecc8372630f22011815d64099598e30bcb43a7/src/libraries/System.Linq/src/System/Linq/Min.cs#L614), I know that `.Min()` and `.Max()` implicitly filter out any values equal to `null` as the first step (including nullable value types which box to `null`). However, this is not obvious from the documentation. May you please alter the documentation to document this behavior? Thanks!
Author: binki
Assignees: -
Labels: `Pri3`, `area-System.Linq`, `:watch: Not Triaged`
Milestone: -
krwq commented 2 years ago

Thanks for reporting! @binki if you're interested in contributing a fix there is an Edit button (pen icon) on https://docs.microsoft.com/en-us/dotnet/api/system.linq.enumerable.min?view=net-6.0 which will point you to the right file to edit

krwq commented 2 years ago

Also if this was already fixed here: https://github.com/dotnet/dotnet-api-docs/pull/7927 please close the issue