rust-num / num-rational

Generic Rational numbers for Rust
Apache License 2.0
142 stars 50 forks source link

clippy: `From` instead of `Into`. #120

Open waywardmonkeys opened 1 year ago

waywardmonkeys commented 1 year ago

I didn't include this in #119 as I wasn't sure if it would require a version bump and therefore be more controversial.

cuviper commented 1 year ago

It would be a problem if folks could write their own From<Ratio<Local>> for (Local, Local), but neither Ratio<Local> nor (Local, Local) are considered local to the crate defining Local. So num-rational users can't write this: playground

use num_rational::Ratio;

pub struct N(i32);

impl From<Ratio<N>> for (N, N) {
    fn from(val: Ratio<N>) -> (N, N) {
        (val.numer, val.denom)
    }
}
error[[E0117]](https://doc.rust-lang.org/stable/error_codes/E0117.html): only traits defined in the current crate can be implemented for arbitrary types
 --> src/lib.rs:5:1
  |
5 | impl From<Ratio<N>> for (N, N) {
  | ^^^^^--------------^^^^^------
  | |    |                  |
  | |    |                  this is not defined in the current crate because tuples are always foreign
  | |    `Ratio` is not defined in the current crate
  | impl doesn't use only types from inside the current crate
  |
  = note: define and implement a trait or new type instead

However, it does require Rust 1.41 for RFC 2451. CI on 1.31.0 says:

error[E0210]: type parameter `T` must be used as the type parameter for some local type (e.g. `MyStruct<T>`)
    --> src/lib.rs:1124:1
     |
1124 | impl<T> From<Ratio<T>> for (T, T) {
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type parameter `T` must be used as the type parameter for some local type
     |
     = note: only traits defined in the current crate can be implemented for a type parameter
cuviper commented 1 year ago

(We'll eventually bump MSRV -- I just haven't made any specific plans yet...)