Overflow on num::rational::Ratio

[cuviper]

From @rust-highfive on September 28, 2014 22:43

Issue by klutzy Wednesday Aug 27, 2014 at 07:50 GMT

For earlier discussion, see https://github.com/rust-lang/rust/issues/16782

This issue was labelled with: in the Rust repository

---

use std::i64::MIN;
use:num::rational::Ratio;

let a = Ratio::new(1, MIN);

Here a actually is -1 / MIN due to overflow in reduce():

        // keep denom positive!
        if self.denom < Zero::zero() {
            self.numer = -self.numer;
            self.denom = -self.denom;
        }

Copied from original issue: rust-num/num#12

[maxbla]

We have some options regarding how to fix this.

1. We could require that types used in a Ratio implement CheckedNeg. Ratio::new() would then return Option<Ratio>. I don't like this. It would mess up the API, and would be a breaking change.

2. We could check after negating the denominator if it is still negative (negating a negative that can't be negated is a no-op in release mode). If it is, we could panic. It is better to panic early than give garbage results. I think in this scenario, it might make sense to add checked_new(T,T) -> Option<Ratio<T>>

3. We could allow negative denominators. In this scenario, we would allow both numerator and denominator to be simultaneously negative. This would require additional logic all over the place, including in cmp(), hash() and reduce()

[vks]

2 sounds like the best option for now.

[vks]

At least in debug mode, this panics now. Can this be closed, or should it panic in release mode as well?

[inferiorhumanorgans]

It would be nice to implement 1, as an additional constructor (e.g. new_checked). I just ran into this while fuzzing a library and being able to get a Result back instead of catching a panic is a nice-to-have.