Add a contract for Ordered and additional traits

[bkamins]

Based on the discussions with @nalimilan this PR proposes:

• to add a contract to Ordered trait so that external packages can also use it
• add Ordered trait to relevant exported types that define isless methods in a safe way

From this PR I have excluded the following types defined in standard Julia installation:

• AbstractArray
• Tuple
• NamedTuple
• Pair
• Ptr
• VersionNumber
• Base.CoreLogging.LogLevel
• Base.Enums.Enum
• Base.SHA1
• Base.Unicode.GraphemeIterator
• Base.UUID
• Pkg.Resolve.FieldValue
• Pkg.Resolve.VersionWeight
• Sockets.IPAddr

as I thought that either very rare or do not meet the contract I proposed (like AbstractArray)

[bkamins]

If we agree to the proposed changes I will add tests. Also a decision is if something should be exported from Base (e.g. Ordered or OrderStyle).

[bkamins]

CC @nalimilan

[nalimilan]

Let's hear what @timholy says. Maybe we could make it more explicit whether the trait is optional (e.g. enables optimizations) or whether it should be implemented by types that implement isless.

If the idea is right, I would even go as far as defining the trait for all types that match it, even if they are "rare". For AbstractArray{T}, you could simply return OrderStyle(T) (unless I'm missing a corner case).

[timholy]

Seems like a good idea. It's scary to have people ask you what you actually mean, though.

[bkamins]

CI fails as some types are undefined at this stage. I will fix this and add tests once we get back more feedback on the proposal.

[bkamins]

@timholy - CI fails but I think it is a bug in isambiguous definition in Base.

Here is a MWE (tested on 1.6.0-DEV.584 (2020-08-05)):

julia> f(::Type{<:Union{Missing, Int}}) = "int"
f (generic function with 1 method)

julia> f(::Type{<:Union{Missing, Float64}}) = "float"
f (generic function with 2 methods)

julia> f(::Type{Missing}) = "missing"
f (generic function with 3 methods)

julia> f(::Type{Union{}}) = "bottom"
f (generic function with 4 methods)

julia> f(Int)
"int"

julia> f(Float64)
"float"

julia> f(Missing)
"missing"

julia> f(Union{})
"bottom"

julia> f(Union{Missing, Int})
"int"

julia> f(Union{Missing, Float64})
"float"

julia> mt = collect(methods(f));

julia> mt[3]
f(::Type{var"#s1"} where var"#s1"<:Union{Missing, Float64}) in Main at REPL[2]:1

julia> mt[4]
f(::Type{var"#s1"} where var"#s1"<:Union{Missing, Int64}) in Main at REPL[1]:1

julia> Base.isambiguous(mt[3], mt[4], ambiguous_bottom=true)
true

julia> Base.isambiguous(mt[3], mt[4], ambiguous_bottom=false)

this is fixed by adding:

julia> f(::Type{<:Missing}) = "missing"
f (generic function with 5 methods)

definition (note <:), but I think it should not be required (I added this change to the PR). Am I missing something here or indeed this is a bug in isambiguous?

[bkamins]

Having thought about it more maybe it is OK to require defining Type{<:Missing} in signature given that if only Type{Missing} is specified the compiler would have to check if also Type{Union{}} method is required (not sure what would be better - but for leaf types this check should not be possible to add).

[bkamins]

Also related is #37255

[martinholters]

Re. the ambiguity, due to

julia> Union{Type{Union{}}, Type{Missing}} == Type{<:Missing}
false

it is not sufficient to define methods for Type{Union{}} and Type{Missing} to cover the intersection which is Type{<:Missing}.

[bkamins]

Agreed - I was confused by the fact that you actually do not have a situation when Type{<:Missing} method could be invoked from an instance of the type, but it does not matter here - we are working with type system only.

[vtjnash]

@bkamins you're right, but we haven't bother to teach the ambiguity checker about that sort of method tiling possibility since it hasn't come up before

[bkamins]

Sure - I think what is now is OK after thinking about it and I just added <: in the PR and all works.
The thing that I think is mildly problematic is #37255 (still in practice probably it does not matter) but this is unrelated (I simply discovered it when trying to better understand how internals worked).

[bkamins]

Is there any recommendation where tests of this should be put?

[bkamins]

@nalimilan - are we OK with this proposal? If yes, then I will add tests somewhere and finalize this PR (if you have a recommendation where to put the tests please let me know).

[kmsquire]

Just to ask, does Ordered mean “sorted”? It wasn’t obvious from a quick perusal of the discussion above (and I’m not at a computer, so I can’t easily review the changes).

I only ask because OrderedCollections.jl (and by extension, DataStructures.jl) distinguish between “ordered” and “sorted”, and it would be nice to be consistent.

[kmsquire]

Ah, never mind--glancing at the PR, I understand now that this refers to something different.

[nalimilan]

@kmsquire "Ordered" here more or less means "sortable". The latter would probably be more in line with the OrderedCollections terminology I guess. Feel free to file an issue to discuss renaming this (I don't think it's used a lot currently as it's not even documented). The name Unordered isn't great either as it's used even for types which are ordered by haven't declared it. So we could imagine using OrderingUnknown as the default instead. EDIT: but it's orthogonal to this PR.

[nalimilan]

@nalimilan - are we OK with this proposal? If yes, then I will add tests somewhere and finalize this PR (if you have a recommendation where to put the tests please let me know).

Looks OK, I've made a proposal regarding the wording. No idea where to put tests.

[bkamins]

Thank you. I have added the tests to sorting.jl

[rafaqz]

This would be very useful to have for packages like DimensionalData.jll. Currently we need to use try/catch on issorted, which is awful.

Are there any blockers to merging this?

[bkamins]

@rafaqz - someone probably needs to take the "ownership" of this functionality. I did not push for it as it was not resolved quickly, so I had to use try-catch in DataFrames.jl two years ago.

In summary - I can finalize this PR if we want it, but I would suggest that it is clear who later tracks its consequences in the ecosystem.

[rafaqz]

I guess it's probably you and me who need this the most, but I also have very limited time to make sure it's implemented widely enough to be a useful replacement for try - catch. I do have concerns about all the ecosystem packages exporting sortable Number types like Unitful.jl etc. I don't know how many PRs it would be.

[bkamins]

I don't know how many PRs it would be.

That was my thinking. I.e. in DataFrames.jl I needed to fall back to try-catch and making sure that every major package properly implemented the updated interface was beyond my capabilities (though this PR could still be useful for performance, as if some type has this trait you do not need to use try-catch).