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 TryFrom and TryInto traits #1542

Merged
merged 1 commit into from
May 4, 2016
Merged

Conversation

sfackler
Copy link
Member

@sfackler sfackler self-assigned this Mar 12, 2016
@seanmonstar
Copy link
Contributor

Additional existing third party trait: hyper::IntoUrl.

@comex
Copy link

comex commented Mar 13, 2016

👎 on implementing it for infallible conversions.

@ticki
Copy link
Contributor

ticki commented Mar 13, 2016

I wonder how this interacts with enum casting? Will it be possible to cast an integral representation to an enum variant?

@sfackler
Copy link
Member Author

@comex one of the major motivations for implementing fallible conversion traits for integers is for FFI use cases. Could you clarify how you see ergonomic casts between, for example, u64 and c_ulong working if there is no impl TryFrom<u64> for u64?


```rust
let value: isize = ...;
let value: u32 = try!(value.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.

This example is pretty untypical.
These overflows, like arithmetic overflows, are very rarely handled, so most of the time people will have to deal with

let value: u32 = value.try_into().unwrap();

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not all that uncommon in my experience:
https://github.com/sfackler/rust-postgres/blob/master/src/types/mod.rs#L700
https://github.com/sfackler/rust-postgres-large-object/blob/master/src/lib.rs#L220

But in any case, what you do with the Err once you get it doesn't really have much to do with this RFC - it's focused on how you get that Err.

@nrc nrc added the A-libs label Mar 13, 2016
@alexcrichton alexcrichton added T-libs-api Relevant to the library API team, which will review and decide on the RFC. and removed A-libs labels Mar 14, 2016
@jimmycuadra
Copy link

From an API discoverability perspective, I think these traits would be useful. I was looking for something that did this a while back and was surprised to find that there was no fallible equivalent of From and Into. Instead, some kind folks on IRC directed me to the totally non-obvious parse and FromStr. Once you know about them, it works okay, but the proposed API is much more intuitive to me.

@Zoxc
Copy link

Zoxc commented Mar 28, 2016

I would like the signature of the methods to be changed to return (Result<To, (From, Self::Err)>) where From is the original from value which can be printed on error without requiring Clone.

For example in this function:

fn coerce<B, A: TryInto<B> + Debug>(from: A) -> B {
    match from.try_into() {
        Ok(v) => v,
        Err((f, err)) => {
            panic!("Cannot coerce value {} of type {} into type {} because {}", f, type_name::<A>(), type_name::<B>(), err);
        }
    }
}

Alternatively there could be a method on the error giving a way to extract it.

@glaebhoerl
Copy link
Contributor

I think that's a good idea. A lot of try_foo-style methods that already exist do that because many types don't even implement Clone. (Or it's just expensive.)

@SimonSapin
Copy link
Contributor

To repeat my comment in rust-lang/rust#31405, the naming convention of try_FOO for things that return Result is consistent with try_clone.

The try! macro however takes a Result and does not generally return one. So we may end up with try!(x.try_clone()) and try!(y.try_into()) (which sounds repetitive and has two different meaning for the same word) being used a lot… But then if the ? operator sticks around maybe try! will be deprecated or go out of fashion, leaving us with x.try_clone()? and y.try_into()? which looks nicer to me.

@alexcrichton alexcrichton added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Apr 15, 2016
@alexcrichton
Copy link
Member

🔔 This RFC is now entering its week-long final comment period 🔔

@alexcrichton
Copy link
Member

Overall I like the idea of having these traits in the standard library as they provide a nice vector for us to implement all sorts of conversions in a standardized fashion. I also think that the naming here, (as @SimonSapin mentioned) seems good (I'm personally less worried about the try stuttering).

On question I'd have is whether we'd want to add these traits to the prelude? One of the reasons Into and From are so easy to use is because of their prominent placement and lack of a need to import them. I'd be of the opinion that these likely want to go into the prelude at some point, but we may not have all of the resolve changes in place to do that without unduly breaking crates. To me though one of the major motivations for this RFC is integer conversions, which I believe aren't gonna work out unless they're in the prelude, so I'd be fine adding it sooner rather than later if we can.

Finally, as to some other points brought up on this RFC:

  • I like the addition impls of the traits between every combination of primitive types. That's easy to understand, always works, and is predictable.
  • I'm ok not having a "lifting" impl from From to TryFrom. That'd also require the addition of a Void type which while it has come up from time to time hasn't ever been clearly well motivated for libstd.
  • I don't think we can do anything about other ad-hoc traits unless we don't break code. That may require specialization or some other kinds of trickery that may take awhile to create. That being said while they both exist we should strive to make them interoperable and have the same set of impls for both (at least). Methods like parse won't go away, they may just change trait bounds if it's compatible.
  • I don't think we should change the Result type returned to carry the payload of the original object. Not all implementors may always have access to the original object once a conversion has failed, and the error type can always contain the original object anyway. Additionally, that'd close off the door for an Error implementation for the error side of Result, so it may not always work with try!/?


```rust
impl<T, U> TryInto<U> for T where U: TryFrom<T> {
type Error = U::Err;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Err, not Error.

@petrochenkov
Copy link
Contributor

Into usually goes hand by hand with AsRef and AsMut, so I'm surprised nobody mentioned TryAsRef and TryAsMut yet.
Any fundamental reasons why they would be less useful than their infallible counterparts or TryInto?

@sfackler
Copy link
Member Author

It seems to me that there would be very few contexts in which you'd actually be able to implement TryAsRef or TryAsMut since they return references. I can't really think of contexts in which you possibly have a reference to some other type.

@nodakai
Copy link

nodakai commented Apr 19, 2016

On which types are we going to implement the proposed traits?

  1. Are they solely for built-in integer types?
  2. Float types?
  3. Boolean?
  4. Pointers?
  5. I'd like to hear more discussions on ad-hoc conversions like &str <-> Ipv4Addr
  6. Enums? (Core language change?)

Also, the proposed interface does not obviously eliminate such silly implementations like

    fn try_from(t: usize) -> Result<u8, TryFromIntError> { Ok(42) }

Is it possible to add kind of "axioms" that all implementers should satisfy? For example, for S: Copy+Eq and T:Copy+Eq, I'd expect this assertion to hold for all implementers:

    let x0: S = ...;
    let x1: S = ...;
    let y0: T = x0.try_into()?;
    let y1: T = x1.try_into()?;
    assert_eq!(x0 == x1, y0 == y1);

@aturon
Copy link
Member

aturon commented Apr 19, 2016

I'm in favor of this RFC. To me, the biggest use-case is ergonomic overloaded construction via try_from, the same way that we replaced a pile of ad hoc constructors with from with the initial conversion traits.

@alexcrichton has already made most of the other points I wanted to, so I'll leave it at 👍!

@sfackler
Copy link
Member Author

@nodakai

On which types are we going to implement the proposed traits?

Are they solely for built-in integer types?
Float types?
Boolean?
Pointers?
I'd like to hear more discussions on ad-hoc conversions like &str <-> Ipv4Addr
Enums? (Core language change?)

The traits are for things that can be converted to another type in a potentially fallible way, and as is the case for From and Into, the set of implementations will expand over time.

Implementations of FromStr are the kinds of things that would work as as TryFrom/TryInto implementations, which includes the &str -> Ipv4Addr conversion.

Also, the proposed interface does not obviously eliminate such silly implementations like

fn try_from(t: usize) -> Result<u8, TryFromIntError> { Ok(42) }

In what way is this different from any other interface in the standard library? We can't guard against pathologically malicious implementors.

Is it possible to add kind of "axioms" that all implementers should satisfy? For example, for S: Copy+Eq and T:Copy+Eq, I'd expect this assertion to hold for all implementers:

let x0: S = ...;
let x1: S = ...;
let y0: T = x0.try_into()?;
let y1: T = x1.try_into()?;
assert_eq!(x0 == x1, y0 == y1);

I do not see us making any strong guarantees in this sense - we don't for Into and From, which simply state that they are "conversion that consumes self, which may or may not be expensive". In particular, any guarantees like the one proposed will disallow a &str -> Ipv4Addr implementation since it involves DNS lookup that may change over time.

@sfackler
Copy link
Member Author

sfackler commented Apr 19, 2016

@alexcrichton

The prelude question is pretty complicated. We certainly can't add these traits to the prelude with the current resolve implementation. The conv crate provides TryFrom and TryInto traits identical to those proposed here, and we wouldn't want to break all crates using those.

I think these will be useful/used in practice for integral conversions because they are already a pain to write. Something like this:

// ...
let x = if x < 0 || x > u32::max_value() as i64 {
    return Err(Whatever);
} else {
    x as u32
};

will be simplified to

use std::convert::TryInto;
// ...
let x = try!(x.try_into());

@ollie27
Copy link
Member

ollie27 commented Apr 19, 2016

I'm also curious if there are any concrete use cases other than int to int conversions which in my opinion are much better handled by something like #1218?

I like the addition impls of the traits between every combination of primitive types.

I assume this was meant to say primitive integers as all primitive types doesn't make any sense.

One of the main advantages of From is that we have impl<T> From<T> for T. If TryFrom doesn't have that which types should manually implement TryFrom for themselves?

I kind of think fallible conversions can be left a bit ad hoc, for example Vec<u8> to String has from_utf8 which presumably is a good use case for this trait but that still leaves from_utf8_lossy and from_utf8_unchecked as ad hoc methods.

@sfackler
Copy link
Member Author

@ollie27 The RFC mentions some in the motivation section. Here's a list of traits that all do this off the top of my head:

  • std::str::FromStr
  • std::net::ToSocketAddrs
  • postgres::IntoConnectParams
  • hyper::IntoUrl

@SimonSapin
Copy link
Contributor

I'm ok not having a "lifting" impl from From to TryFrom. That'd also require the addition of a Void type which while it has come up from time to time hasn't ever been clearly well motivated for libstd.

FWIW std already has at least one Void-like type: std::string::ParseError is an empty enum used for <String as FromStr>::Err.

@SimonSapin
Copy link
Contributor

In particular, any guarantees like the one proposed will disallow a &str -> Ipv4Addr implementation since it involves DNS lookup that may change over time.

(&str -> Ipv4Addr doesn’t have to do DNS, it can simply support IPv4 syntax like Ipv4Addr::from_str currently does. I’d even argue that any kind of external lookup is not appropriate for TryFrom/TryInto.)

@petrochenkov
Copy link
Contributor

@sfackler

We certainly can't add these traits to the prelude with the current resolve implementation.

Why exactly?

The conv crate provides TryFrom and TryInto traits identical to those proposed here, and we wouldn't want to break all crates using those.

Prelude names are shadowable, if code uses TryFrom from the conv crate, it will continue use it and prelude::TryFrom will be shadowed.

@sfackler
Copy link
Member Author

Hmm, I may have imagined that there would be more troubles adding this to the prelude than there are.

@nodakai
Copy link

nodakai commented Apr 24, 2016

@sfackler

The traits are for things that can be converted to another type in a potentially fallible way, and as is the case for From and Into, the set of implementations will expand over time.

I thought it would be nice to discuss how far it should be expanded in the future at this stage, and in order to do so, we need some guideline on its applicability.

In what way is this different from any other interface in the standard library? We can't guard against pathologically malicious implementors.

I do not see us making any strong guarantees in this sense - we don't for Into and From, which simply state that they are "conversion that consumes self, which may or may not be expensive".

I might argue that's just an unfortunate lack and shouldn't be taken as the norm.

The prominent applications of the proposed crates are built-in integers, and their success/failure will be defined by whether "the value is preserved" or not, right? I was wondering if all the implementers could follow an extended definition of "value-preserving."

In particular, any guarantees like the one proposed will disallow a &str -> Ipv4Addr implementation since it involves DNS lookup that may change over time.

As @SimonSapin pointed out, it can still make sense without DNS, but anyways, I'd argue this is exactly why I'd like to bring up the point at this stage, not at the implementation phase.

@alexcrichton
Copy link
Member

The libs team discussed this RFC at triage today and the decision was to merge. We had some small discussion about the prelude-ness of these traits, but the conclusion was that this likely wants to wait until they're stable in any case.

Thanks again for the RFC @sfackler!

Tracking issue: rust-lang/rust#33417

@alexcrichton alexcrichton merged commit adc2547 into rust-lang:master May 4, 2016
@Kixunil
Copy link

Kixunil commented Sep 21, 2016

@petrochenkov Regarding TryAsRef I know of at least one case in which it would be useful: Json (or some kind of dynamic type). The user of library might want to read some value in Json and return Err if the value doesn't exist or has different type.

P.S. I hope I didn't do any harm by replying late and little bit off-topic. (Please let me know if I should avoid doing it in the future.)

@Kroc
Copy link

Kroc commented Sep 17, 2018

Just learning Rust, and was surprised this wasn't in stable! I'm writing an assembler and I'm using a data-driven methodology where I'm avoiding any functions; all computing is done by type conversion; e.g. you want to parse a string? Convert it into a TokenStream, the logic is internal. Where this falls apart is fallibility; If I'm trying to convert a string token to a scalar value (such as parse), there isn't an easy way to error handle this other than panics.

TryFrom solves my issue perfectly!

@steveklabnik
Copy link
Member

steveklabnik commented Sep 17, 2018 via email

@Centril Centril added A-conversions Conversion related proposals & ideas A-traits-libstd Standard library trait related proposals & ideas labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-conversions Conversion related proposals & ideas A-traits-libstd Standard library trait related proposals & ideas final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.