Restrict generic type of Ratio

[maxbla]

We effectively restrict the type of Ratios by only impling new() and new_raw() on T: Clone + Integer. This appears to work well, but putting the same restrictions on the T in the Ratio declaration itself would make this requirement more explicit. e.g.

/// Represents the ratio between two numbers.
#[derive(Copy, Clone, Debug)]
#[allow(missing_docs)]
pub struct Ratio<T: Clone + Integer> {
    /// Numerator.
    numer: T,
    /// Denominator.
    denom: T,
}

Testing out this change now, the compiler complains for every method in this library that uses Ratio<T> where T is not Clone and Integer (for example Display).

Unlike many of the issues I have been submitting recently, I believe this would not be a breaking change.

[cuviper]

Testing out this change now, the compiler complains for every method in this library that uses Ratio where T is not Clone and Integer (for example Display).

Unlike many of the issues I have been submitting recently, I believe this would not be a breaking change.

These statements seem to contradict each other. If previously-working code now fails to compile, this is a breaking change, and it can happen to third-party code as well.

[maxbla]

If previously-working code now fails to compile, this is a breaking change

I generally agree with this, but the only things that fail to compile are the Into and Display i.e. internal functions, which have have greater access to a private api.

1. The only way to construct a Ratio is using new() or new_raw() i.e. crates can't use Ratio{numer:5, denom:7} because the fields of Ratio are not pub.

2. new() and new_raw() are implemented only for T: Clone + Integer.

3. no crate could have constructed a Ratio<T> where T is not Clone + Integer

1 and 2 imply 3. Because of 3 this is not a breaking change. Does that sound right?

[cuviper]

Even if they can't construct it without Clone + Integer, they may still have other generic parts of their code that aren't fully constrained. It could even be for their own Display, like:

struct Foo<T> {
    ratio: Ratio<T>,
    other_stuff: ...
}

impl<T: Display + Eq + One> Display for Foo<T> {
    // ...
}

[cuviper]

I do agree that it would be nicer to be consistent with this, but it's still a breaking change.

[maxbla]

Hm... I hadn't thought of that. In a language with more dynamic types, I could imagine this not being a breaking change. It would be breaking, but it would also be rather easy to fix. In the breaking scenario you gave, the dev could just add Clone + Integer to T. All constructed Ratios are Clone + Integer so all uses of Display would just work without any refactoring required.

[cuviper]

Right, the additional bounds shouldn't be a problem in practice, but may require some code change to propagate the constraints throughout. So I consider this a feasible change when we do bump versions.

[cuviper]

Were you trying this before I had merged #48? Unfortunately, I had to drop all type constraints from those functions to let them be const, otherwise you get this error:

error[E0723]: trait bounds other than `Sized` on const fn parameters are unstable
  --> src/lib.rs:82:6
   |
82 | impl<T: Clone + Integer> Ratio<T> {
   |      ^
   |
   = note: for more information, see issue https://github.com/rust-lang/rust/issues/57563
   = help: add #![feature(const_fn)] to the crate attributes to enable

error: aborting due to previous error

[maxbla]

I was working on it around the same time. Really the issue is that I tested it out on my fork that didn't have new changes from master pulled in.

I guess we can save this until the issue the compiler links to is fixed. If we ever have a breaking changes tracking issue, throw this in there.