Generalize some definitions for Irrational to AbstractIrrational

[devmotion]

In IrrationalConstants 0.2 we recently switched to custom irrational types <: AbstractIrrational (created by a custom @irrational macro) to fix type piracy (see e.g. #46531). During that transition we noticed that we had to copy existing code in base that currently does not support AbstractIrrational (and hence not our custom irrational types), e.g., https://github.com/JuliaMath/IrrationalConstants.jl/blob/c9a73a62a9e2f90abe3b1a3f92edeb487fcc9f8d/src/macro.jl#L7-L18 and JuliaMath/IrrationalConstants.jl#24.

This PR widens some of these definitions which should allow us to remove the downstream code at some point. I did not know if it would be safe to generalize hash as well (I'm not familiar with why it is defined the way it is), so I did not include it in the initial commit.

[garrison]

The changes to ==, <, and <= might be fine if AbstractIrrational were only ever used to implement constants which are fully specified by their type. However, this is not true throughout the ecosystem: see e.g. RationalRoots.jl or MultiplesOfPi.jl. Is it really too much of a burden to define these methods in packages when this is the correct implementation? When it is not, a MethodError is a much better user outcome than a wrong result.

[devmotion]

I thought it would be a reasonable default but I can see that it might not be correct in all cases and hence maybe one would want to remove it from this PR. I think there are a few things to consider here:

• Already currently, Base defines default fallbacks for ==(::AbstractIrrational, ::AbstractIrrational) etc. which are also not always correct.
• Similar to the default for <(::AbstractIrrational, ::AbstractIrrational) one could add checks such as Float64(x) == Float64(y) || throw(MethodError(==, (x, y))) for the fallback for ==(x::T, y::T) where {T<:AbstractIrrational} etc. to minimize the number of cases in which the default implementation is not correct. Instead of checking the floating point representation an alternative could be to check if the arguments are egal or, less generally, singletons.
• Maybe more generally, Irrational and @irrational should be replaced with IrrationalConstant and the accompanying macro, in which case == etc. could be defined only for IrrationalConstant or, even safer, for the singleton type generated by the macro.

I think the other generalizations should be less controversial and I could also split the PR, if desired.

[garrison]

• Already currently, Base defines default fallbacks for ==(::AbstractIrrational, ::AbstractIrrational) etc. which are also not always correct.

Thanks, this is a good point. I brought this up back in #26550, and at the time it seemed the consensus was that if someone was creating a type that derived from AbstractIrrational that might not always be irrational, then it is fair to require that type to override ==. However, this PR (as currently stands) would expand this such that any AbstractIrrational that is not a constant would have to define == too.

[devmotion]

The current fallbacks can be wrong even in cases where no non-irrational numbers and only constants are involved, so one doesn't have to consider such special cases which possibly might require some custom treatment and additional overloads anyway. The ==(::AbstractIrrational, ::AbstractIrrational) = false fallback is wrong as soon as there exist two implementations of the same irrational number with different (possibly singleton) types. If these definitions live e.g. in different packages, there is no way to guarantee that these incorrect definitions are noticed at all and it is e.g. also not clear in which package they should be fixed. So I think the current fallbacks already lead to problems even for constant definitions.

[devmotion]

Bump 🙂 I still think lifting some of the type restrictions would be helpful for downstream packages such as IrrationalConstants.