Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add blanket TryFrom impl when From is implemented. #44174

Merged
merged 13 commits into from
Sep 30, 2017
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 28 additions & 12 deletions src/libcore/convert.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,23 @@

#![stable(feature = "rust1", since = "1.0.0")]

use str::FromStr;
use fmt;

/// A type used as the error type for implementations of fallible conversion
/// traits in cases where conversions cannot actually fail.
///
/// Because `Infallible` has no variants, a value of this type can never exist.
/// 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 {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()?) }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


#[unstable(feature = "try_from", issue = "33417")]
impl fmt::Debug for Infallible {
fn fmt(&self, _: &mut fmt::Formatter) -> fmt::Result {
match *self {}
}
}

/// A cheap reference-to-reference conversion. Used to convert a value to a
/// reference value within generic code.
Expand Down Expand Up @@ -417,6 +433,17 @@ impl<T, U> TryInto<U> for T where U: TryFrom<T>
}
}

// 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> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

type Error = Infallible;

fn try_from(value: U) -> Result<Self, Self::Error> {
Ok(T::from(value))
}
}

////////////////////////////////////////////////////////////////////////////////
// CONCRETE IMPLS
////////////////////////////////////////////////////////////////////////////////
Expand All @@ -442,14 +469,3 @@ impl AsRef<str> for str {
self
}
}

// FromStr implies TryFrom<&str>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`

#[unstable(feature = "try_from", issue = "33417")]
impl<'a, T> TryFrom<&'a str> for T where T: FromStr
{
type Error = <T as FromStr>::Err;

fn try_from(s: &'a str) -> Result<T, Self::Error> {
FromStr::from_str(s)
}
}
2 changes: 2 additions & 0 deletions src/libcore/iter/range.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ macro_rules! step_impl_unsigned {
}

#[inline]
#[allow(unreachable_patterns)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just remove the Err branch?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

fn add_usize(&self, n: usize) -> Option<Self> {
match <$t>::try_from(n) {
Ok(n_as_t) => self.checked_add(n_as_t),
Expand Down Expand Up @@ -120,6 +121,7 @@ macro_rules! step_impl_signed {
}

#[inline]
#[allow(unreachable_patterns)]
fn add_usize(&self, n: usize) -> Option<Self> {
match <$unsigned>::try_from(n) {
Ok(n_as_unsigned) => {
Expand Down
49 changes: 20 additions & 29 deletions src/libcore/num/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

#![stable(feature = "rust1", since = "1.0.0")]

use convert::TryFrom;
use convert::{Infallible, TryFrom};
use fmt;
use intrinsics;
use str::FromStr;
Expand Down Expand Up @@ -2503,16 +2503,23 @@ impl fmt::Display for TryFromIntError {
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

// no possible bounds violation
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this macro used anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


#[inline]
fn try_from(u: $source) -> Result<$target, TryFromIntError> {
Ok(u as $target)
fn try_from(value: $source) -> Result<Self, Self::Error> {
Ok(value as $target)
}
}
)*}
Expand Down Expand Up @@ -2584,31 +2591,17 @@ macro_rules! rev {
}

/// intra-sign conversions
try_from_unbounded!(u8, u8, u16, u32, u64, u128);
try_from_unbounded!(u16, u16, u32, u64, u128);
try_from_unbounded!(u32, u32, u64, u128);
try_from_unbounded!(u64, u64, u128);
try_from_unbounded!(u128, u128);
try_from_upper_bounded!(u16, u8);
try_from_upper_bounded!(u32, u16, u8);
try_from_upper_bounded!(u64, u32, u16, u8);
try_from_upper_bounded!(u128, u64, u32, u16, u8);

try_from_unbounded!(i8, i8, i16, i32, i64, i128);
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see all these go away 🎉

try_from_both_bounded!(i16, i8);
try_from_both_bounded!(i32, i16, i8);
try_from_both_bounded!(i64, i32, i16, i8);
try_from_both_bounded!(i128, i64, i32, i16, i8);

// unsigned-to-signed
try_from_unbounded!(u8, i16, i32, i64, i128);
try_from_unbounded!(u16, i32, i64, i128);
try_from_unbounded!(u32, i64, i128);
try_from_unbounded!(u64, i128);
try_from_upper_bounded!(u8, i8);
try_from_upper_bounded!(u16, i8, i16);
try_from_upper_bounded!(u32, i8, i16, i32);
Expand All @@ -2627,15 +2620,13 @@ try_from_both_bounded!(i64, u32, u16, u8);
try_from_both_bounded!(i128, u64, u32, u16, u8);

// usize/isize
try_from_unbounded!(usize, usize);
try_from_upper_bounded!(usize, isize);
try_from_lower_bounded!(isize, usize);
try_from_unbounded!(isize, isize);

#[cfg(target_pointer_width = "16")]
mod ptr_try_from_impls {
use super::TryFromIntError;
use convert::TryFrom;
use convert::{Infallible, TryFrom};

try_from_upper_bounded!(usize, u8);
try_from_unbounded!(usize, u16, u32, u64, u128);
Expand All @@ -2647,21 +2638,21 @@ mod ptr_try_from_impls {
try_from_both_bounded!(isize, i8);
try_from_unbounded!(isize, i16, i32, i64, i128);

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

rev!(try_from_unbounded, isize, u8);
rev!(try_from_upper_bounded, isize, u16, u32, u64, u128);
rev!(try_from_unbounded, isize, i8, i16);
rev!(try_from_unbounded, isize, i16);
rev!(try_from_both_bounded, isize, i32, i64, i128);
}

#[cfg(target_pointer_width = "32")]
mod ptr_try_from_impls {
use super::TryFromIntError;
use convert::TryFrom;
use convert::{Infallible, TryFrom};

try_from_upper_bounded!(usize, u8, u16);
try_from_unbounded!(usize, u32, u64, u128);
Expand All @@ -2673,21 +2664,21 @@ mod ptr_try_from_impls {
try_from_both_bounded!(isize, i8, i16);
try_from_unbounded!(isize, i32, i64, i128);

rev!(try_from_unbounded, usize, u8, u16, u32);
rev!(try_from_unbounded, usize, u16, u32);
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

rev!(try_from_upper_bounded, isize, u32, u64, u128);
rev!(try_from_unbounded, isize, i8, i16, i32);
rev!(try_from_unbounded, isize, i16, i32);
rev!(try_from_both_bounded, isize, i64, i128);
}

#[cfg(target_pointer_width = "64")]
mod ptr_try_from_impls {
use super::TryFromIntError;
use convert::TryFrom;
use convert::{Infallible, TryFrom};

try_from_upper_bounded!(usize, u8, u16, u32);
try_from_unbounded!(usize, u64, u128);
Expand All @@ -2699,14 +2690,14 @@ mod ptr_try_from_impls {
try_from_both_bounded!(isize, i8, i16, i32);
try_from_unbounded!(isize, i64, i128);

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

rev!(try_from_unbounded, isize, u8, u16, u32);
rev!(try_from_upper_bounded, isize, u64, u128);
rev!(try_from_unbounded, isize, i8, i16, i32, i64);
rev!(try_from_unbounded, isize, i16, i32, i64);
rev!(try_from_both_bounded, isize, i128);
}

Expand Down
7 changes: 2 additions & 5 deletions src/libcore/str/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ use self::pattern::Pattern;
use self::pattern::{Searcher, ReverseSearcher, DoubleEndedSearcher};

use char;
use convert::TryFrom;
use fmt;
use iter::{Map, Cloned, FusedIterator};
use slice::{self, SliceIndex};
Expand Down Expand Up @@ -2142,7 +2141,7 @@ pub trait StrExt {
#[stable(feature = "core", since = "1.6.0")]
fn is_empty(&self) -> bool;
#[stable(feature = "core", since = "1.6.0")]
fn parse<'a, T: TryFrom<&'a str>>(&'a self) -> Result<T, T::Error>;
fn parse<T: FromStr>(&self) -> Result<T, T::Err>;
}

// truncate `&str` to length at most equal to `max`
Expand Down Expand Up @@ -2462,9 +2461,7 @@ impl StrExt for str {
fn is_empty(&self) -> bool { self.len() == 0 }

#[inline]
fn parse<'a, T>(&'a self) -> Result<T, T::Error> where T: TryFrom<&'a str> {
T::try_from(self)
}
fn parse<T: FromStr>(&self) -> Result<T, T::Err> { FromStr::from_str(self) }
}

#[stable(feature = "rust1", since = "1.0.0")]
Expand Down
1 change: 0 additions & 1 deletion src/libcore/tests/char.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assert_eq!(char::from_str("\0").unwrap(), '\0');
assert_eq!(char::from_str("\u{D7FF}").unwrap(), '\u{d7FF}');
assert!(char::from_str("").is_err());
Expand Down
10 changes: 9 additions & 1 deletion src/libcore/tests/num/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use core::convert::TryFrom;
use core::convert::{TryFrom, TryInto};
use core::cmp::PartialEq;
use core::fmt::Debug;
use core::marker::Copy;
use core::num::TryFromIntError;
use core::ops::{Add, Sub, Mul, Div, Rem};
use core::option::Option;
use core::option::Option::{Some, None};
Expand Down Expand Up @@ -134,6 +135,13 @@ fn test_empty() {
assert_eq!("".parse::<u8>().ok(), None);
}

#[test]
fn test_infallible_try_from_int_error() {
let func = |x: i8| -> Result<i32, TryFromIntError> { Ok(x.try_into()?) };

assert!(func(0).is_ok());
}

macro_rules! test_impl_from {
($fn_name: ident, $Small: ty, $Large: ty) => {
#[test]
Expand Down