Crystal: Inconsistent `Enumerable#minmax` behavior when an element is not comparable

[kwwagner1693]

Summary

The current (0.31.1) implementation of Enumerable#minmax does not define any behavior when the Enumerable contains an element which is not comparable with other elements. Consequently, some rather inconsistent results can occur depending on the ordering of elements in the Enumerable. Since the ordering of an Enumerable should not have any effect on the minimum or maximum element, this inconsistency appears erroneous, and I believe it should be fixed. While I have not verified, this inconsistency is likely an issue with all variants of #min and #max as well.

Example

I encountered this issue specifically in the context of trying to extract a minimum or maximum value from an Enumerable(Float64) which contained Float64::NAN values. It would seem that whenever the first element of an Enumerable is NaN, the result of #minmax will always be {NaN, NaN} regardless of any other non-NaN elements. In any other case, the result of #minmax will be as expected, with all NaN elements ignored.

nan_first = [Float64::NAN, 1.0, 2.0, Float64::NAN]
puts nan_first.minmax # => {NaN, NaN}

number_first = [1.0, Float64::NAN, 2.0, Float64::NAN]
puts number_first.minmax # => {1.0, 2.0}

Possible Resolutions

In looking into this issue, I noticed that Enumerable#minmax only ever uses < and > between elements and never seems to have been updated to use the partial comparability of <=>. I think it would be best to follow the precedent of https://github.com/crystal-lang/crystal/pull/6611 and rework Enumerable#min, Enumerable#max, and Enumerable#minmax to use <=> and raise when an element is not comparable.

[asterite]

Good catch! I think in this case a runtime exception should happen, similar to what happens when you sort an array with NaN.

[TedTran2019]

Hey-- I'm a bit new to coding and new to open source as well. I have knowledge of C and Ruby, but I've never used the Crystal language or navigated large code bases before. Would it be okay for me to try at my hand at this, or would it be better to leave it to someone who's experienced and can solve the issue within a reasonable amount of time? (If it is a valid issue). I can probably spend about 5 - 10~ hours a week going at it and I'd really like to learn and contribute. Thanks for the feedback! (Do I just wait to be assigned or whatever, or do I just go for it then make a pull request?)

[bararchy]

@TedTran2019 go for it, create a PR. :tada:

[TedTran2019]

@bararchy Thanks, I'll do my best

[straight-shoota]

This issue got the community:newcomer label because its considered a relatively easy task that can be managed without much in-depth knowledge. So it fits perfectly for you! Looking forward for your contributions ❤️

[Blacksmoke16]

This can be closed, was handled in https://github.com/crystal-lang/crystal/pull/8490