rust-num / num-traits

Numeric traits for generic mathematics in Rust
Apache License 2.0
732 stars 135 forks source link

Add safe cast operation traits #88

Closed npmccallum closed 5 years ago

npmccallum commented 6 years ago

This commit adds safe cast operation traits which each have only one of the properties from a naked cast. This allows the clear definition of specific properties in a generic context.

All methods defined in this commit are zero-cost abstractions.

cuviper commented 6 years ago

Regarding the CI failure, you need #[cfg(has_i128)] on 128-bit functionality.

npmccallum commented 6 years ago

Regarding the CI failure, you need #[cfg(has_i128)] on 128-bit functionality.

Is this still needed now that i128 is stable?

npmccallum commented 6 years ago

And overall, grouping these in a safe module has a weird implication for the rest of the cast module.

Considering that I'm trying to move implicit behavior to explicit behavior, I think safe is pretty reasonable. Honestly, I'd prefer that these interfaces (properly reviewed) replace the rest of the cast module.

The traits like FromPrimitive and ToPrimitive are explicitly safe conversions too, just fallible.

At runtime. One of the whole benefits of Rust is catching errors at compile time. And casts are a pretty regular source of bugs.

cuviper commented 6 years ago

Yes. Think of a nuclear silo. Pretend there is a flag that controls some aspect of launching the rocket in bit 11. Do you want people passing a small integer into the function and getting unexpected behavior?

I'd rather the interface clearly define: 16 bits are the minimum. The compiler will fail otherwise.

This is a scenario where I would eschew numeric conversions altogether, and instead insist on a strong newtype wrapper, probably even based on bitflags.

Regarding the CI failure, you need #[cfg(has_i128)] on 128-bit functionality.

Is this still needed now that i128 is stable?

Yes, I maintain strong backwards compatibility here. If I were to require a new rustc in num-traits, that would force every dependent crate to increase their requirement too. I choose to remain conservative about this.

At runtime. One of the whole benefits of Rust is catching errors at compile time. And casts are a pretty regular source of bugs.

As much as possible is at compile time, but some things are necessarily at runtime, like integer overflow and bounds checking. For numeric conversions, we have:

Your traits add grow/trim/sign dimensions to the static case, but I'm not yet convinced that those are really any safer than the simple lossless/lossy distinction.

Considering that I'm trying to move implicit behavior to explicit behavior, I think safe is pretty reasonable. Honestly, I'd prefer that these interfaces (properly reviewed) replace the rest of the cast module.

I think what I'm coming around to, is that I don't really agree that these are better than the existing cast module. Even if I did, given that the others can't go away without a breaking change (which I'm avoiding as long as I can), we'd end up with a redundancy here that will make it more confusing for the users.

However, there is always room for a new addition on crates.io! I was trying to recall some other existing crates in this space, and I believe it was conv that I've seen before. I also found cast. These are both fallible (runtime checked) though, unlike your static guarantees.

cuviper commented 6 years ago

I thought you might also be interested in this thread: https://internals.rust-lang.org/t/idea-foo-bar-type-conversion-operator/8542

cuviper commented 5 years ago

I'm going to close this, since we disagree on design and the discussion stalled. You can re-open or start a new issue if you'd like to revisit this.