How much does this degrade performance?

I imagine the performance difference is negligible, but it seems strange to go out of the way to handle unconstructable values. reinterpret is known to produce potentially invalid values and so is reading from uninitialized arrays. Operations on malformed inputs are not guaranteed to be correct across most of the places where they occur.

0//0 is invalid. Is it zero? Not exactly. But is it not zero? Hard to say. The issue is that in some cases you check for zero because you want a zero in that spot and in other cases you check for zero because you do not. There is no consistently useful stance one can take on the value of an invalid. So while I understand it was useful to treat invalids as nonzero in your motivating example, I don't think this is a universally useful solution. It will be exactly opposite to some peoples' needs.

Even with this (particular) fix, reinterpret(Rational{Int}, (0, -1)) will be iszero. This rational is invalid and unconstructable just like 0//0, but something went very wrong so it doesn't seem responsible to call it iszero either. Should reinterpret(Rational{Int}, (3,3)) be isone? These functions should at least be consistent in their treatment of invalids.

It would seem that this PR starts the slippery slope toward checking isvalid at every call site with Rational (but it actually doesn't even properly check for validity, as written). And further for every type that possesses invalid bitstrings. That seems ridiculous (all those checks would add up in cost), so I'm reluctant to start it here.

I would prefer that this PR simply delete this method (and the matching isone) entirely and just use the fallback iszero(x::Any) = x == zero(x). The only drawback of this change is that it would allocate for BigInt, so I would also accept iszero(x::Rational) = iszero(numerator(x)) & isone(denominator(x)) (equivalent to the fallback but does not construct a new value, and it also mirrors the current implementation of isone). Either version would ensure consistent behavior on invalids while also satisfying your motivating issue.

I don't love testing for specific valid-like behavior on invalid values. That would seem to imply this as API that becomes at least a minor change to adjust later. EDIT: I do think that all invalids should not satisfy iszero, so the test is useful but should check more than just 0//0.

Viewpoint to consider: 0 // 0 is special and a lot like NaN. Maybe Julia should even have isnan(Base.unsafe_rational(0, 0))?

Viewpoint to consider: 0 // 0 is special and a lot like NaN. Maybe Julia should even have isnan(Base.unsafe_rational(0, 0))?

Yes! It follows a lot of its properties

I think discussion of 0//0 as NaN belongs on a separate issue. It has negligible impact on the issue here.

But is it not zero? Hard to say. The issue is that in some cases you check for zero because you want a zero in that spot and in other cases you check for zero because you do not. There is no consistently useful stance one can take on the value of an invalid

I'm intrigued by this: is there a motivating example where one does want to treat 0//0 as 0? This seems to counter the properties that one may expect from such a number.

I'm intrigued by this: is there a motivating example where one does want to treat 0//0 as 0? This seems to counter the properties that one may expect from such a number.

What I attempted and failed to allude to is the difference between the following two lines, either of which a sane person could write (if sqrt didn't do this already)

x >= 0 ? sqrt(x) : error() # look for what I want
x < 0 ? error() : sqrt(x) # reject what I don't want

These two appear somewhat equivalent at first glance, but they are not equivalent. The only difference here (for most Real, at least) is the handling of NaN, where the first will error but the second will pass the NaN to the calculation (Julia's sqrt uses the second pattern because we choose to error on negatives but not on NaN). Someone implementing the same calculation where you encountered your problem, but from the opposite paradigm, might find it more convenient that we do group 0//0 as iszero. Although I'm not actually advocating we do that, either.

For example, a responsible coder might assume that !iszero(x) implies !iszero(numerator(x)). This is true for all valid Rational and, prior to this PR, also all invalids. But I'm not proposing we keep that as it isn't the "sanest" invariant to preserve.

When the invariants of the type are violated, it becomes very situational what the "correct" fallback behavior should be. I find the "look for what I want" pattern to be slightly more robust to this, which is why I prefer it. But there are no answers that are "best" or "most convenient" in all situations.

In any case, I will still advocate for one of my counter-proposals that fulfill the invariant that iszero(x) imply x == zero(x) (i.e., iszero is always false for invalids). Anything else is technically wrong (by @doc iszero), although experiencing incorrect behavior on invalids isn't objectionable (hence my puzzlement over this PR in the first place).

Alright, thanks, I'm starting to get convinced that this might not be the right approach

I still think iszero(x::Rational) = iszero(numerator(x)) & isone(denominator(x)) would be a reasonable improvement over the status quo and would match the isone implementation. My concerns was mostly over being too opinionated on invalids. But such a version would reject all invalids as not iszero.

Should the test be updated as well? Although, I can't think of cases other than invalids where the denominator check would be necessary.

Maybe just check for a few "prototypical" ones?

iszero(zero(Rational{Int})) == true
iszero(reinterpret(Rational{Int}, (0, typemin(Int)))) == false
iszero(reinterpret(Rational{Int}, (0, -1))) == false
iszero(reinterpret(Rational{Int}, (0, 0))) == false
iszero(reinterpret(Rational{Int}, (0, 2))) == false
iszero(reinterpret(Rational{Int}, (0, typemax(Int)))) == false