Add TryFrom implementation for bool, f32 and f64 #43220

GuillaumeGomez · 2017-07-13T18:06:23Z

hanna-kruppe · 2017-07-13T18:11:57Z

src/libcore/num/mod.rs

+            type Error = ();
+
+            fn try_from(u: $source) -> Result<bool, ()> {
+                Ok(u != 0)


Is there precedent in Rust for this being the canonical way (or even "a" right way) of turning integers into bools? For example, there's no as cast from integers to bools, no From impl, no num::NumCast, etc.

Not as far as I know. I just copied the C behavior.

hanna-kruppe · 2017-07-13T18:22:29Z

src/libcore/num/mod.rs

-same_sign_try_from_int_impl!(i128, i128, i8, i16, i32, i64, i128, isize);
-same_sign_try_from_int_impl!(u128, usize, u8, u16, u32, u64, u128, usize);
-same_sign_try_from_int_impl!(i128, isize, i8, i16, i32, i64, i128, isize);
+same_sign_try_from_int_impl!(u128, u8, u8, u16, u32, u64, u128, usize, f32, f64);


If I read the macro right, this will cast fN::{MIN,MAX} to {u,i}128? That will overflow for all combinations except f32 -> i128.

No, the macro is getting the max for the source value (the one you implement the trait on). Then you cast the target to the source and make the comparison. So it cannot overflow.

However it might have issues when casting a floating type to an integer one as described in @kennytm's comment.

kennytm · 2017-07-13T18:21:12Z

src/libcore/num/mod.rs

@@ -2504,6 +2504,30 @@ impl fmt::Display for TryFromIntError {
    }
 }

+#[unstable(feature = "try_from", issue = "33417")]
+impl TryFrom<bool> for bool {
+    type Error = ();


The Error type shouldn't be (). Probably TryFromBoolError.

It should really be ! or some other uninhabited type.

(since the conversion is infallible)

The real solution to this is an eventual impl<A, B> TryFrom<A> for B where B: From<A> with type Error = !;, but that's blocked for several reasons.

Actually this one should be covered by the (not yet existing) blanket impl impl<T, U> TryFrom<T> for U where U: From<T>.

So what you should I do in here? :)

@GuillaumeGomez Create something like std::string::ParseError.

I'd advise to leave these unimplemented entirely until the blanket impl comes-- otherwise the blanket impl will be a breaking change (changing the error type from your custom uninhabited type to !).

@cramertj Breaking change for an unstable feature is fine 😃. (Although I recommend the entire PR be held off 😛.) The blanket impl still won't use ! directly to avoid being blocked by never_type.

Ah, I see-- yeah, you could just backwards-compatibly change your custom type into a type alias to !. The reason I prefer ! is that it has coercions that custom enums don't have, although I suppose those might be added someday.

kennytm · 2017-07-13T18:28:28Z

src/libcore/num/mod.rs

+same_sign_try_from_int_impl!(i128, i32, i8, i16, i32, i64, i128, isize, f32, f64);
+same_sign_try_from_int_impl!(u128, u64, u8, u16, u32, u64, u128, usize, f32, f64);
+same_sign_try_from_int_impl!(i128, i64, i8, i16, i32, i64, i128, isize, f32, f64);
+same_sign_try_from_int_impl!(u128, u128, u8, u16, u32, u64, u128, usize, f32, f64);


I believe these will trigger #10184 ("float as int" can be UB — consider u8::try_from(1.0e+100_f64) which will involve a (1.0e+100_f64 as u128) > 255 check in the middle).

They should use the float types themselves as $storage.

I'll handle this cast differently then.

kennytm · 2017-07-13T18:35:21Z

src/libcore/num/mod.rs

+
+                let min = $target::MIN as $storage;
+                let max = $target::MAX as $storage;
+                if u as $storage < min || u as $storage > max {


These basically rules out conversion of float::INFINITY and float::NAN, i.e. Ok(f64::from(f32::INFINITY)) and f64::try_from(f32::INFINITY) will produce different result.

Excellent point! I'll add a special case for Infinity and NaN.

kennytm · 2017-07-13T18:51:16Z

cc #33417.

I think this PR should not be merged until the NumCast RFC mentioned in #42456 is prepared, and the interaction with TryFrom is cleared out (thus the 👎).

GuillaumeGomez · 2017-07-13T20:49:24Z

It's fine as is. I fix the current issues and then it's open until...

oyvindln · 2017-07-13T21:20:02Z

src/libcore/num/mod.rs

+impl TryFrom<bool> for bool {
+    type Error = TryFromBoolError;
+
+    fn try_from(u: bool) -> Result<bool, TryFromBoolError> {


May want to add [inline] here. At least for the integer types, the calls to try_from() didn't inline properly without having [inline], so it might be similar for bool/float.

shepmaster · 2017-07-14T19:56:02Z

Hmm. As this touches the standard library, I'm going to guess this is a libs thing and randomly pick... r? @BurntSushi

BurntSushi · 2017-07-15T16:40:41Z

The TryFrom conversions for floats seem reasonable to me? Could someone elaborate on what the issues are there and why we're waiting on a numcast RFC for this? @kennytm?

The conversions for bools seem more controversial to me. Do we have any precedent for those conversions anywhere else? What is the use case for having them?

kennytm · 2017-07-15T17:10:48Z

@BurntSushi We want to have a blanket impl

impl<T: From<U>> TryFrom<U> for T {
    type Error = !;  // <-- or an empty enum before `!` is stable
    fn try_from(u: U) -> Result<Self, Self::Error> {
        Ok(Self::from(u))
    }
}

Since u64: From<u32> but not the other way around, <u64 as TryFrom<u32>>::Error and <u32 as TryFrom<u64>>::Error will have different types, which is unnatural and can be a footgun when the integer size is platform-dependent e.g. what is <c_ulong as TryFrom<u64>>::Error?

It is the same for floats, f64: From<f32> but f32: !From<f64>. Size of floating-point number can also be platform-dependent, e.g. on macOS/iOS the CGFloat type is pointer-sized, so the FFI issue applies here as well.

The plan as of June 6th was to remove u32: TryFrom<u64> and replace with a different trait Cast, although the first try (#42456) was shot down due to controversy around the name and the provided API. The same principle should apply to floats too i.e. fallible conversion between floats should be done via a separate API, say FloatCast, not TryFrom.

BurntSushi · 2017-07-15T17:13:07Z

@kennytm I see. I don't think I'm tracking progress on that, so I'm going to hand this off to @sfackler who I think has been working on the TryFrom stuff?

alexcrichton · 2017-07-20T18:44:18Z

ping r? @sfackler

carols10cents · 2017-07-24T15:33:11Z

friendly ping for review @sfackler @rust-lang/libs!

sfackler · 2017-07-24T15:49:07Z

The TryFrom<bool> for bool should be taken care of by the addition of a blanket impl for T: From<U>.

I'm not sure how I feel about the float impls. Arguably the implementation should fail if the number can't be represented exactly as the float type?

bors · 2017-07-25T03:27:40Z

☔ The latest upstream changes (presumably #43248) made this pull request unmergeable. Please resolve the merge conflicts.

alexcrichton · 2017-07-27T23:25:09Z

@GuillaumeGomez do you hae thoughts on @sfackler's recent comment?

oyvindln · 2017-07-27T23:53:19Z

The try_from impls should probably be marked inline like the existing ones.

GuillaumeGomez · 2017-07-28T08:23:26Z

I'll update my PR this week-end.

ollie27

I don't think there is much reason to continue this PR. The int to bool conversion is completely arbitrary and if you need != 0 then that's what you should use not .try_into().unwrap(). Even if the float conversions had the correct bounds, had int to float and had tests there is still the question of whether TryFrom should allow rounding. We have impl From<i32> for f64 but not impl From<i32> for f32 because even though the bounds of i32 fit well inside the bounds of f32, you can lose precision.

ollie27 · 2017-07-28T15:06:33Z

src/libcore/num/mod.rs

+same_sign_try_from_int_impl!(f64, i16, f32, f64);
+same_sign_try_from_int_impl!(f64, u16, f32, f64);
+same_sign_try_from_int_impl!(f64, i8, f32, f64);
+same_sign_try_from_int_impl!(f64, u8, f32, f64);


For what it's worth the bounds for these conversions are wrong. NaN needs to be taken into account. Also float to int conversions round towards 0 so for example -0.9 as u8 == 0 and 255.9 as u8 == 255 so the bounds need to allow that.

ollie27 · 2017-07-28T15:11:26Z

src/libcore/num/mod.rs

+                } else {
+                    let min = $target::MIN as $storage;
+                    let max = $target::MAX as $storage;
+                    if u as $storage < min || u as $storage > max {


These bounds are also wrong for f64 to f32 which is the only conversion that actually needs a manual TryFrom conversion. The source value is rounded when casting to f32 so some values larger than f32::MAX as f64 will round down to f32::MAX. How fptrunc performs this rounding is apparently undefined so I'm not completely sure what the bounds will need to be though.

GuillaumeGomez · 2017-07-30T13:02:24Z

Just like @ollie27, I think this PR has no more interest. I'm closing it.

hanna-kruppe reviewed Jul 13, 2017

View reviewed changes

kennytm reviewed Jul 13, 2017

View reviewed changes

GuillaumeGomez force-pushed the try-from branch from e57e048 to 49b627f Compare July 13, 2017 20:45

Add TryFrom implementation for bool, f32 and f64

ecc48c5

GuillaumeGomez force-pushed the try-from branch from 49b627f to ecc48c5 Compare July 13, 2017 20:49

oyvindln reviewed Jul 13, 2017

View reviewed changes

rust-highfive assigned BurntSushi Jul 14, 2017

shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 14, 2017

llogiq mentioned this pull request Jul 15, 2017

improve the TryFrom implementations #43248

Merged

BurntSushi assigned sfackler and unassigned BurntSushi Jul 15, 2017

alexcrichton added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 27, 2017

ollie27 suggested changes Jul 28, 2017

View reviewed changes

GuillaumeGomez closed this Jul 30, 2017

GuillaumeGomez deleted the try-from branch November 24, 2017 20:55

Add TryFrom implementation for bool, f32 and f64 #43220

Add TryFrom implementation for bool, f32 and f64 #43220

Conversation

GuillaumeGomez commented Jul 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennytm Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennytm commented Jul 13, 2017

GuillaumeGomez commented Jul 13, 2017

Choose a reason for hiding this comment

shepmaster commented Jul 14, 2017

BurntSushi commented Jul 15, 2017

kennytm commented Jul 15, 2017

BurntSushi commented Jul 15, 2017

alexcrichton commented Jul 20, 2017

carols10cents commented Jul 24, 2017

sfackler commented Jul 24, 2017

bors commented Jul 25, 2017

alexcrichton commented Jul 27, 2017

oyvindln commented Jul 27, 2017

GuillaumeGomez commented Jul 28, 2017

ollie27 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jul 30, 2017 • edited Loading

kennytm Jul 13, 2017 •

edited

Loading

GuillaumeGomez commented Jul 30, 2017 •

edited

Loading