Add blanket TryFrom impl when From is implemented. #44174

jimmycuadra · 2017-08-30T05:22:31Z

Adds impl<T, U> TryFrom<T> for U where U: From<T>.

Removes impl<'a, T> TryFrom<&'a str> for T where T: FromStr (originally added in #40281) due to overlapping impls caused by the new blanket impl. This removal is to be discussed further on the tracking issue for TryFrom.

Refs #33417.

/cc @sfackler, @scottmcm (thank you for the help!), and @aturon

Adds `impl<T, U> TryFrom<T> for U where U: From<T>`. Removes `impl<'a, T> TryFrom<&'a str> for T where T: FromStr` due to overlapping impls caused by the new blanket impl. This removal is to be discussed further on the tracking issue for TryFrom. Refs #33417.

rust-highfive · 2017-08-30T05:22:44Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

scottmcm

Thanks for taking this on, @jimmycuadra!

scottmcm · 2017-08-30T06:45:53Z

src/libcore/num/mod.rs

-        impl TryFrom<$source> for $target {
-            type Error = TryFromIntError;
-
+        impl From<$source> for $target {


I think this needs to stay TryFrom, at least for now, since this would be insta-stable and I don't think libs wants it until the portability lint RFC is available (if ever).

scottmcm · 2017-08-30T06:48:44Z

src/libcore/convert.rs

+// Infallible conversions are semantically equivalent to fallible conversions
+// with an uninhabited error type.
+#[unstable(feature = "try_from", issue = "33417")]
+impl<T, U> TryFrom<U> for T where T: From<U> {


I have no idea if it's better or worse, but could this be on U:Into<T>, to pick up more impls?

Sounds fine to me. I'd be happy to change it to that if everyone is in agreement to do it.

I'd rather not simply because it'd be confusing for from to not work but try_from to work in those cases.

scottmcm · 2017-08-30T06:50:59Z

src/libcore/convert.rs

@@ -442,14 +470,3 @@ impl AsRef<str> for str {
        self
    }
 }
-
-// FromStr implies TryFrom<&str>


Looks like this broke something in Travis:

[00:53:44] error[E0277]: the trait bound `char: core::convert::From<&str>` is not satisfied [00:53:44] --> /checkout/src/libcore/../libcore/tests/char.rs:35:16 [00:53:44] | [00:53:44] 35 | assert_eq!(char::try_from("a").unwrap(), 'a'); [00:53:44] | ^^^^^^^^^^^^^^ the trait `core::convert::From<&str>` is not implemented for `char` [00:53:44] | [00:53:44] = help: the following implementations were found: [00:53:44] <char as core::convert::From<u8>> [00:53:44] = note: required because of the requirements on the impl of `core::convert::TryFrom<&str>` for `char`

scottmcm · 2017-08-30T06:52:28Z

src/libcore/convert.rs

+/// an error type, and signals to both the compiler and the user that the error
+/// case is impossible.
+#[unstable(feature = "try_from", issue = "33417")]
+pub enum Infallible {}


Ideally this could become ! at some point. Is that the kind of thing that goes in comments, or only in tracking issues?

I thought about it, but assuming TryFrom stabilizes first and this ends up in the stable docs, I wouldn't want to explain it in terms of an unstable feature that could change or theoretically go away completely. When the never type lands, it'll simply the explanation here, though.

I'm not too fond of this; maybe we could rename it NeverError or something similar to add Error to the name?

Also, I'd rather this implement Error too.

scottmcm · 2017-08-30T06:53:09Z

src/libcore/num/mod.rs

-try_from_unbounded!(i16, i16, i32, i64, i128);
-try_from_unbounded!(i32, i32, i64, i128);
-try_from_unbounded!(i64, i64, i128);
-try_from_unbounded!(i128, i128);


Nice to see all these go away 🎉

See #44174 (comment)

See https://travis-ci.org/rust-lang/rust/jobs/269861252

alexcrichton · 2017-08-30T14:42:21Z

r? @sfackler

Seems fine to me, but I think @sfackler is a little more "in the loop" here

clarfonthey · 2017-08-31T17:49:42Z

src/libcore/convert.rs

-use str::FromStr;
+use fmt;
+
+/// An uninhabited type used as the error type for implementations of fallible


I think that adding "uninhabited" here might confuse new users. The latter paragraph explains better so I think we can just omit it?

clarfonthey · 2017-08-31T17:50:22Z

src/libcore/iter/range.rs

@@ -89,6 +89,7 @@ macro_rules! step_impl_unsigned {
            }

            #[inline]
+            #[allow(unreachable_patterns)]


Why not just remove the Err branch?

Because it's only unreachable for certain $ts (like u128), not all of them (like u8).

clarfonthey · 2017-08-31T17:52:42Z

src/libcore/tests/char.rs

@@ -32,7 +32,6 @@ fn test_convert() {
 #[test]
 fn test_from_str() {
    assert_eq!(char::from_str("a").unwrap(), 'a');
-    assert_eq!(char::try_from("a").unwrap(), 'a');


Why was this test removed? Shouldn't it succeed?

This was because of the FromStr --> TryFrom impl being removed. The test fails without it.

clarfonthey · 2017-08-31T17:59:25Z

I would very much like to oppose reverting the FromStr -> TryFrom switch because the latter allows parse errors to depend on the lifetime of the string, allowing for slices from the string to be contained in the error.

Although the overlap is quite a problem. Perhaps we could use specialisation to get around it? If we default the FromStr impl it should work, I think. I don't mind preferring From<&str> to FromStr.

Of course, this part of specialisation isn't implemented afaik...

Maybe in the meantime we could use clippy to suggest implementing TryFrom when FromStr is implemented, then after specialisation make the switch.

scottmcm · 2017-09-01T00:55:39Z

src/libcore/convert.rs

+/// an error type, and signals to both the compiler and the user that the error
+/// case is impossible.
+#[unstable(feature = "try_from", issue = "33417")]
+pub enum Infallible {}


This comment reminded me of something: Can you please add an impl From<Infallible> for TryFromIntError? That way code like this will compile even if the try_into is actually infallible:

fn foo(x: c_int) -> Result<i32, TryFromIntError> { Ok(x.try_into()?) }

scottmcm · 2017-09-01T00:57:56Z

src/libcore/num/mod.rs

@@ -2508,11 +2508,11 @@ macro_rules! try_from_unbounded {
    ($source:ty, $($target:ty),*) => {$(
        #[unstable(feature = "try_from", issue = "33417")]
        impl TryFrom<$source> for $target {
-            type Error = TryFromIntError;
+            type Error = Infallible;


Hmm, this might need to remain TryFromIntError to avoid portability hazards...

Isn't this macro only being called when the target's pointer width makes the conversion infallible?

Is this macro used anymore?

Yes, it's used in the target-specific ptr_try_from_impls module.

I don't think we want these impls changing based on your target.

Most calls to this macro were removed in this PR. The ones that are still there are the conversions from specifically sized integer types to usize/isize. For example, u32 is only an infallible conversion to usize if the target's pointer width is 32 or higher.

jimmycuadra · 2017-09-01T07:13:39Z

@clarcharr

I completely agree with you that removing the FromStr --> TryFrom impl is undesirable. I did it because the new blanket impl conflicts with that one, as you suspected: https://gist.github.com/jimmycuadra/f59871d00741a8445f17419485d0ba29

I tried using specialization to get around it by making the new blanket impl the default, but it didn't change the compiler error. After discussing with @scottmcm on IRC, we thought we'd try temporarily removing it and then discussing what to do about it more on the tracking issue. Personally, I'm also fine in having some/all of that discussion here, especially if we can find a way for the two impls to coexist before merging this PR.

clarfonthey · 2017-09-02T16:00:16Z

Now that I've thought about it, I think that the best long-term solution would be to deprecate (or at least soft deprecate) str::parse in favour of TryFrom::try_from. This impl is much more important and honestly, the hack of FromStr -> TryFrom wasn't going to last long-term anyway.

jimmycuadra · 2017-09-03T10:51:46Z

Works for me. I can submit another PR deprecating parse and/or FromStr as soon as we stabilize try_from.

carols10cents · 2017-09-11T14:43:14Z

@jimmycuadra What's the status of this PR? I'm having a hard time telling.

jimmycuadra · 2017-09-11T20:44:03Z

@carols10cents Just waiting for merge, as far as I can see, unless anyone on the libs team wants to go a different way for dealing with FromStr.

clarfonthey · 2017-09-11T21:11:24Z

src/libcore/convert.rs

+/// It is used only to satisfy trait signatures that expect an error type, and
+/// signals to both the compiler and the user that the error case is impossible.
+#[unstable(feature = "try_from", issue = "33417")]
+pub enum Infallible {}


I'm still too not sold on the name Infalliable; perhaps NeverError or something similar would be better?

We also should have more impls for this, like Eq, PartialEq, Ord, PartialOrd, Hash, and Error.

Agreed on the extra derived impls. I will add those. NeverError probably goes against the style guidelines around "stutter" in names. I'd be open to just Never, though. After all, that's the way you say ! (as a type).

After thinking about it, maybe CannotError? I think it should probably end in Error by convention.

jimmycuadra · 2017-09-24T07:59:45Z

@sfackler Made a few more changes. I think everything is good now.

aidanhs · 2017-09-28T01:05:20Z

Pinging @sfackler for review (and also on IRC)!

sfackler · 2017-09-28T01:48:24Z

src/libcore/num/mod.rs

+#[unstable(feature = "try_from", issue = "33417")]
+impl From<Infallible> for TryFromIntError {
+    fn from(_: Infallible) -> TryFromIntError {
+        TryFromIntError(())


This might also be a bit more clearly expressed as match value {}.

sfackler · 2017-09-28T01:48:44Z

There's one other little nit, but it doesn't need to block this any longer :)

@bors r+

bors · 2017-09-28T01:48:45Z

📌 Commit ba74a86 has been approved by sfackler

bors · 2017-09-28T03:22:46Z

⌛ Testing commit ba74a86 with merge b63d442ac1112da695b5862fe6e9cec2a88a27fd...

bors · 2017-09-28T03:28:18Z

💔 Test failed - status-appveyor

kennytm · 2017-09-28T06:17:32Z

Legit. Looks like there's conflict about impl TryFrom<u8> for usize (also u16 → usize, i8 → isize and i16 → isize) on i686-pc-windows-msvc.

error[E0520]: `Error` specializes an item from a parent `impl`, but that item is not marked `default`
    --> src\libcore\num\mod.rs:2522:13
     |
2522 |               type Error = Infallible;
     |               ^^^^^^^^^^^^^^^^^^^^^^^^ cannot specialize default item `Error`
...
2671 |       rev!(try_from_unbounded, usize, u8, u16, u32);
     |       ---------------------------------------------- in this macro invocation
     | 
    ::: src\libcore\convert.rs
     |
440  | / impl<T, U> TryFrom<U> for T where T: From<U> {
441  | |     type Error = Infallible;
442  | |
443  | |     fn try_from(value: U) -> Result<Self, Self::Error> {
444  | |         Ok(T::from(value))
445  | |     }
446  | | }
     | |_- parent `impl` is here
     |
     = note: to specialize, `Error` in the parent `impl` must be marked `default`

ollie27 · 2017-09-29T16:38:03Z

src/libcore/num/mod.rs

    rev!(try_from_upper_bounded, usize, u64, u128);
    rev!(try_from_lower_bounded, usize, i8, i16, i32);
    rev!(try_from_both_bounded, usize, i64, i128);

-    rev!(try_from_unbounded, isize, u8, u16);


This impl needs to remain, it's the i8 to isize impl below that needs to be removed. The same two impls need to be removed for the 16-bit case as well. Basically ba74a86 needs to be reverted.

jimmycuadra · 2017-09-29T21:24:39Z

I addressed @ollie27's comment and fixed the nit from @sfackler's previous comment.

sfackler · 2017-09-29T21:34:47Z

@bors r+

bors · 2017-09-29T21:34:47Z

📌 Commit 27d95d3 has been approved by sfackler

bors · 2017-09-29T22:35:30Z

⌛ Testing commit 27d95d3 with merge b7041bf...

@sfackler

Add blanket TryFrom impl when From is implemented. Adds `impl<T, U> TryFrom<T> for U where U: From<T>`. Removes `impl<'a, T> TryFrom<&'a str> for T where T: FromStr` (originally added in #40281) due to overlapping impls caused by the new blanket impl. This removal is to be discussed further on the tracking issue for TryFrom. Refs #33417. /cc @sfackler, @scottmcm (thank you for the help!), and @aturon

bors · 2017-09-30T01:31:30Z

☀️ Test successful - status-appveyor, status-travis
Approved by: sfackler
Pushing b7041bf to master...

arthurprs · 2017-10-02T19:44:59Z

Losing the FromStr impl is sad 😭 but I have no suggestions to fix.

clarfonthey · 2017-10-02T23:55:21Z

IMHO try_into should become the more idiomatic way to do parse anyway.

arthurprs · 2017-10-03T07:52:48Z

They're the exact same, I'd be happy to see FromStr deprecated eventually.

rust-highfive assigned alexcrichton Aug 30, 2017

scottmcm reviewed Aug 30, 2017

View reviewed changes

jimmycuadra added 2 commits August 30, 2017 04:42

Implement TryFrom explicitly for infallible numeric conversions.

b0edfce

See #44174 (comment)

Remove test case that assumes FromStr provides TryFrom<&'a str>.

414ee9a

See https://travis-ci.org/rust-lang/rust/jobs/269861252

rust-highfive assigned sfackler and unassigned alexcrichton Aug 30, 2017

kennytm mentioned this pull request Aug 31, 2017

Add cast function to primitive integers #42456

Closed

aidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 31, 2017

clarfonthey reviewed Aug 31, 2017

View reviewed changes

scottmcm reviewed Sep 1, 2017

View reviewed changes

jimmycuadra added 2 commits September 1, 2017 00:26

Reword docs for Infallible to make them easier to understand.

36c0ff8

impl From<Infallible> for TryFromIntError.

93a56cd

arielb1 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 Sep 5, 2017

sfackler mentioned this pull request Sep 7, 2017

Have From<&str> automatically provide FromStr rust-lang/rfcs#2143

Open

clarfonthey reviewed Sep 11, 2017

View reviewed changes

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

Add back mistakenly removed numeric conversions.

ba74a86

sfackler reviewed Sep 28, 2017

View reviewed changes

Remove conflicting TryFrom impls on 32-bit targets.

1a29e82

ollie27 reviewed Sep 29, 2017

View reviewed changes

jimmycuadra added 2 commits September 29, 2017 14:04

Simplify implementation of From<Infallible> for TryFromIntError.

966cf33

Fix more TryFrom impls for integers.

27d95d3

bors merged commit 27d95d3 into rust-lang:master Sep 30, 2017

bors mentioned this pull request Sep 30, 2017

Implement TryFrom<&[T]> for &[T; N] #44764

Merged

jimmycuadra deleted the try-from-infallible branch September 30, 2017 03:26

CraZySacX mentioned this pull request Oct 1, 2017

impl From<std::convert::Infallible> for Error rust-lang-deprecated/error-chain#229

Open

jimmycuadra mentioned this pull request Oct 4, 2017

Tracking issue for TryFrom/TryInto traits #33417

Closed

3 tasks

arielb1 mentioned this pull request Nov 2, 2017

Broken code after 2017-09-30 nightly #45142

Closed

Kern-- mentioned this pull request Jan 9, 2018

Make protocol work on stable rust and lastest nightly compiler. dylanmckay/protocol#6

Merged

Add blanket TryFrom impl when From is implemented. #44174

Add blanket TryFrom impl when From is implemented. #44174

Conversation

jimmycuadra commented Aug 30, 2017

rust-highfive commented Aug 30, 2017

scottmcm left a 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

alexcrichton commented Aug 30, 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

clarfonthey commented Aug 31, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmycuadra commented Sep 1, 2017

clarfonthey commented Sep 2, 2017

jimmycuadra commented Sep 3, 2017

carols10cents commented Sep 11, 2017

jimmycuadra commented Sep 11, 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

jimmycuadra commented Sep 24, 2017

aidanhs commented Sep 28, 2017

Choose a reason for hiding this comment

sfackler commented Sep 28, 2017

bors commented Sep 28, 2017

bors commented Sep 28, 2017

bors commented Sep 28, 2017

kennytm commented Sep 28, 2017 • edited Loading

Choose a reason for hiding this comment

jimmycuadra commented Sep 29, 2017

sfackler commented Sep 29, 2017

bors commented Sep 29, 2017

bors commented Sep 29, 2017

bors commented Sep 30, 2017

arthurprs commented Oct 2, 2017

clarfonthey commented Oct 2, 2017

arthurprs commented Oct 3, 2017

clarfonthey commented Aug 31, 2017 •

edited

Loading

kennytm commented Sep 28, 2017 •

edited

Loading