Let Array#sort only use <=>, and let <=> return nil for partial comparability

[asterite]

Fixes #6608

This PR does a few things:

• Array#sort and related methods used to use < and <= instead of <=>, as documented. This is fixed to only use <=>.
• Merge Comparable and PartialComparable into a single type. <=> can now return nil, meaning that the two objects aren't comparable. This can be used when two objects of the same type can be compared, but not always (partial order). One such example are floats: they are usually comparable, except for NaN values.
• Let Array#sort and related methods handle the case when <=> return nil, and raise an exception in that case
• Let Number <=> Number return nil for the case of NaN values, but provide an optimized implementation for Int <=> Int that doesn't return nil

I ran the benchmarks provided in the PR that introduced intro sort, but only checking for sort and sort with block, and compared them to the current implementation. The performance is the same, except when <=> can return nil, in which the sorting is a bit less than 3 times slower. But that makes sense, because there's a nil check for every comparison now. I personally prefer to give a correct result than to silently fail at runtime. But this performance penalty only applies when <=> returns nil, so it's not a very common case.

The reason for removing PartialComparable and merging it with Comparable is that it's simpler to have just one module, and let it automatically indicate partial order if nil is returned. If nil isn't returned, compiling with --release should give the same performance, because the check for nil will be optimized out.

This PR also fixes the fact that even though we had PartialComparable, you couldn't sort such types (Array#sort didn't handle the possibility of nil from <=>).

Note: I replaced an Int32? restriction with U forall U to allow for sort's block to be of type Int32 or Int32?, for performance reasons. Ideally we should be able to do U < Int32? or some other condition over a free variable, but that's currently not supported. Eventually we can improve that code.

[Sija]

In exception messages title-case if preferred. Ditto below.

[asterite]

I wish we hand't change that. This is how it looks right now:

Unhandled exception: Index out of bounds (IndexError)
  from src/indexable.cr:596:8 in 'at'
  from src/indexable.cr:73:5 in '[]'
  from baz.cr:2:1 in '__crystal_main'
  from src/crystal/main.cr:97:5 in 'main_user_code'
  from src/crystal/main.cr:86:7 in 'main'
  from src/crystal/main.cr:106:3 in 'main'

It doesn't look good to have an uppercase letter after :. This is Ruby:

baz.cr:1:in `sort': comparison of Integer with 2 failed (ArgumentError)
	from baz.cr:1:in `<main>'

I'll capitalize it, but I'd like to discuss reverting this capitalization.

[asterite]

Fixed (rebased)

[straight-shoota]

I'd suggest to add the explanation about the performance regarding Nil to the API docs.

[asterite]

@straight-shoota What do you suggest? Something like "You can return nil if your need partial ordering. However, that will result in a slower performance when sorting such objects. In that case, you might considering not modeling your program correctly (as you should) just so that you can gain a small performance improvement that will probably never be the bottleneck of your program"?

[asterite]

@straight-shoota With the above I don't mean to be rude, I just don't understand how we can recommend someone not to model their program correctly because it will perform worse. But I'm open to suggestions.

[asterite]

@straight-shoota Or, well, I could add "Note that if there is a chance that<=> returns nil, Array#sort will perform a bit slower". But I'd rather not to, because then many would probably try to prematurely optimize their program at the cost of a worse modeling.

[straight-shoota]

No offense taken. But I think it should be worth mentioning that there is a difference in performance depending on whether Nil type is included or not. If written positively, like for completely comparable types the comparator value is not nilable and the nil check will be optimized out.
Obviously it is not a good advice to apply an incorrect comparison model just to be faster. But it is good to know that when you have a completely comparable type, that it wont waste performance on nil checks.

This was actually my most important concern when reading the PR (and your comments in #6608), how performance would play out for sorting completely comparable types vs. partially comparable types. And I think the implementation is really great.

[asterite]

@straight-shoota Sounds good. Where should I add such comment? In Comparable, in Comparable#<=> or in one of the Array#sort methods?

[straight-shoota]

I think I'd put it in Comparable following the description of nil return value.

[asterite]

@straight-shoota Added the performance note.

[straight-shoota]

@asterite Could you rebase this?

[asterite]

@straight-shoota Done! :-)

[straight-shoota]

Yes, inlined is much better indeed 👍

[straight-shoota]

@asterite Needs a rebase!

[asterite]

Rebased!

[asterite]

There was a recent PR that improved Comparable's docs. In this PR I had already done that so I kept my docs which also includes the fact that nil can be returned now from <=>. I should have mentioned that in the other PR to avoid wasting someone else's time.

[wooster0]

Shouldn't the src/partial_comparable.cr file now be deleted?

[asterite]

Yes, but it's a breaking change to do so, so we must be a bit careful.

[wooster0]

Unfortunately the Deprecated annotation doesn't support modules yet. I would say it should just be removed because it's not really used anywhere: https://github.com/search?l=Crystal&q=PartialComparable&type=Code.

[straight-shoota]

Why don't we add a deprecation notice in the API docs for 0.28.0 and remove it in 0.29.0?

[bcardiff]

We could flag the <=> method as deprecated during 0.28 if someone wants to tackle that.
The std lib path is not ignored by default, so that should warn about PartialComparable usages.

Maybe another check for deprecated methods is to detect overrides of abstract methods with @[Deprecated].