rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
96.62k stars 12.48k forks source link

Tracking issue for TryFrom/TryInto traits #33417

Closed alexcrichton closed 5 years ago

alexcrichton commented 8 years ago

Tracking issue for https://github.com/rust-lang/rfcs/pull/1542


To do:

Zoxc commented 8 years ago

Is there a way to generically print an error with the original value if the conversion fails without requiring Clone so a related method which panics on failure could have nice error messages?

chris-morgan commented 8 years ago

A discussion of whether this should go in the prelude is pending for when this goes stable.

shepmaster commented 8 years ago

Apologies if this is covered somewhere else, but what would we like to see before marking this as stable? I'm pretty sure I've reimplemented this functionality a few times in various projects, so a common reusable trait would make me šŸ˜„

sfackler commented 8 years ago

We can bring it up for discussion for the next cycle.

alexcrichton commented 8 years ago

šŸ”” This issue is now entering a cycle-long final comment period for stabilization šŸ””

As a point of stabilization, the libs team would also like to add these traits to the prelude as part of their stabilization. This will require a crater run being 100% clean at minimum, but we're relatively confident that the current state of resolve makes it backwards compatible to add traits to the prelude.

ollie27 commented 8 years ago

I have a few questions about the purpose of these traits.

  1. Which types in std will these be implemented for?
  2. Which types should get implementations like impl TryFrom<T> for T?
  3. Which types should get implementations like impl TryFrom<U> for T if they already have impl From<U> for T?
alexcrichton commented 8 years ago

cc @sfackler, could you expand on the current set of impls and some of the rationale as well?

sfackler commented 8 years ago

I think in general the intuition should be exactly the same for that of From/Into, except when the conversion can fail. Just like we gradually add implementations of From and Into to standard library types, I expect we'll do the same for TryFrom and TryInto. The most important guideline here is that the conversion should be the "canonically obvious" one - if there's more than one reasonable way to convert one type to another, TryFrom or From may not be the right things to use.

In general, I don't think it should be expected that all types should impl TryFrom<T> for T or manually lift an impl From<U> for T. In particular, it's often not clear which error type to pick. However, those kinds of implementations are very much ones that can and should be defined to make a particular API work. For example, we have both of these kinds of implementations for various combinations of primitive integer types for the reasons outlined in the RFC. As another example, one ad-hoc trait that TryFrom/TryInto will replace is postgres::IntoConnectParams, which has a reflexive implementation to convert a ConnectParams into itself.

SimonSapin commented 8 years ago

In general, I don't think it should be expected that all types should impl TryFrom<T> for T or manually lift an impl From<U> for T.

When a TryFrom conversion happens to be infallible the error type should be enum Void {}, right? (Or a similar enum with some other name.) Which by the way sounds to me like a good reason to have a general purpose Void type in std.

sfackler commented 8 years ago

@SimonSapin that would break an IntoConnectParams style API as well as the integer conversion use case outlined in the RFC since the error types of the infallible and fallible conversions wouldn't match.

glaebhoerl commented 8 years ago

@SimonSapin https://github.com/rust-lang/rfcs/pull/1216

Ericson2314 commented 8 years ago

@sfackler Not if you use plain From for the error types(e.g. try!)! impl<T> From<!> for T can and should exist for that purpose.

ollie27 commented 8 years ago

Just like we gradually add implementations of From and Into to standard library types, I expect we'll do the same for TryFrom and TryInto.

That doesn't appear to have happened yet so I don't know if it's because nobody has got round to it or there are no applicable types in std. If there are applicable types in std it would be nice to see some implementations for them. For example are any of the following good implementations?

impl TryFrom<u32> for char impl TryFrom<char> for u8 impl TryFrom<Vec<u8>> for String impl TryFrom<&[u8]> for &str

In general, I don't think it should be expected that all types should impl TryFrom<T> for T or manually lift an impl From<U> for T.

The issue is that if you want to use TryInto<T> and T isn't in your crate then you have to just hope that impl TryFrom<T> for T has been implemented. That's an unfortunate limitation, one which doesn't exist for From/Into.

@SimonSapin that would break an IntoConnectParams style API as well as the integer conversion use case outlined in the RFC since the error types of the infallible and fallible conversions wouldn't match.

Does this mean the error type is somehow fixed based on the type you're converting to?

malbarbo commented 8 years ago

Why TryFrom::Err and TryInto::Err is not bounded by std::error::Error?

shepmaster commented 8 years ago

not bounded by std::error::Error?

That would prevent Err being (), which is a viable error type for infallible conversions.

ghost commented 8 years ago

If () is the error type, the function could return Err(()). This doesn't make the function infallible. It just fails to provide a useful error when it does fail. When writing code that uses conversions which may or may not be fallible, I think the best way is to bound by Into and write a specialization that uses TryInto.

chris-morgan commented 8 years ago

Once RFC 1216 is implemented, ! will be the appropriate error type for infallible conversions. I think that would mean that an Error bound on TryFrom::Err/TryInto::Err would be OK.

Iā€™d certainly like it if implementing From<T> yielded a TryFrom<T, Err = !> implementation (and ditto for Into, of course).

canndrew commented 8 years ago

I think that would mean that an Error bound on TryFrom::Err/TryInto::Err would be OK.

Yes, we'll definitely have impl Error for ! { ... } for precisely these sorts of cases.

sfackler commented 8 years ago

I'm a bit worried about the type differences in the integer conversions use case, but it does seem like we should probably punt on stabilizing this until !-as-a-type lands to have a chance to experiment.

malbarbo commented 8 years ago

In my POV all associated types that represents errors should be bounded by Error. Sadly, we already left one type without it: FromStr::Err (The only other public types are TryInto and TryFrom, checked with ack 'type Err;' and ack 'type Error;'). Let not make this again.

alexcrichton commented 8 years ago

Discussed recently, the libs team decided to punt on stabilizing these traits until the ! type is worked out.

Razican commented 8 years ago

I guess that now this is no longer blocked, right?

alexcrichton commented 7 years ago

Removing nomination as @sfackler is going to investigate ! types and these traits.

theduke commented 7 years ago

Will this have a change to get stabilized in the foreseeable future?

briansmith commented 7 years ago

Why TryFrom::Err and TryInto::Err is not bounded by std::error::Error?

std::err::Error doesn't exist in #![no_std] builds. Also, in many cases there is no benefit in implementing std::err::Error for an error type even when it is available. Thus, I would prefer to not have any such bound.

briansmith commented 7 years ago

I have experimented with implementing this myself in my own library. I'd like to reiterate the concern expressed by @SimonSapin in https://github.com/rust-lang/rfcs/pull/1542#issuecomment-206804137: try!(x.try_into()); is confusing because the word ā€œtryā€ is used two different ways in the same statement.

I understand that many people thing that such things should be written x.try_into()?;, however I am one of a substantial number of people (based on all the debates) that strongly prefer to not use the ? syntax because of...all the reasons mentioned in all the debates.

Personally I think we should still try to find some pattern that doesn't require the try_ prefix on the names.

sfackler commented 7 years ago

I don't feel particularly strongly about the naming, but I can't personally think of anything that's better.

canndrew commented 7 years ago

It's already become semi-standard to use try_ for flavours of functions that return a Result.

however I am one of a substantial number of people (based on all the debates) that strongly prefer to not use the ? syntax because of...all the reasons mentioned in all the debates.

That debate's over now though isn't it? I mean, I sympathize with that side of the debate: I don't know why people said they find try!() so annoying, ? is less visible, encourages lazy error-handling and it seems like a waste of syntax for something we already had a perfectly good macro for (unless it gets extended to become something much more general in the future).

But that's in the past now. ? is stable and it's not going away. So we all might as well all switch to it so that we're all using the same thing and we can stop worrying about name conflicts with try!.

SimonSapin commented 7 years ago

I'd like to reiterate the concern expressed by @SimonSapin in rust-lang/rfcs#1542 (comment)

Since Iā€™m cited by name, let me say that I donā€™t really have that concern anymore. At the time I made this comment the ? operator was a proposal whose future was uncertain, but now itā€™s here to stay.

Also, I think stabilizing sooner rather than latter is more important than another round of names bikeshedding. Itā€™s been months since the RFC was accepted and this has been implemented #[unstable].

briansmith commented 7 years ago

It's already become semi-standard to use try_ for flavours of functions that return a Result.

This is the thing I find most strange about this feature. Most functions I write returns a Result but I've never named any of those functions with a try_ prefix except when trying to experiment with this trait.

Also, I haven't found any practical advantage to writing this:

impl TryInto<X> for Y {
    type Err = MyErrorType;

   fn try_into(self) -> Result<X, Self::Err> { ... }
}

Instead, I can always just write this, much less syntactic overhead:

    fn into_x(self) -> Result<X, MyErrorType> { ... }

I've never had to write generic code that was parameterized by TryInto or TryFrom despite having lots of conversions, so the latter form is sufficient for all my uses in types I define. I think having TryInto<...> or TryFrom<...> parameters seems like questionable form.

Also, I found that naming the associated error type Err instead of Error was error-prone as I always typed Error. I noticed that this mistake was made even during the drafting of the RFC itself: https://github.com/rust-lang/rfcs/pull/1542#r60139383. Also, code that uses Result already uses the name Err extensively since it is a Result constructor.

There was an alternate proposal that focused on integer types specifically and used terminology like ā€œwidenā€ and ā€œnarrow,ā€ e.g. x = try!(x.narrow()); that I'd also implemented. I found that proposal was sufficient for my uses of the functionality proposed here in my actual usage as I only ended up doing such conversions on built-in integer types. It is also more ergonomic and clearer (IMO) for the use cases it is sufficient for.

canndrew commented 7 years ago

Also, I haven't found any practical advantage to writing this...

I sort-of agree. This trait is for where a thing can be used to create another thing but sometimes that process can fail - but that sounds like just about every function. I mean, should we have these impls?:

impl TryInto<TcpStream> for SocketAddr {
    type Err = io::Error;
    fn try_into(self) -> Result<TcpStream, io::Error> {
        TcpStream::connect(self)
    }
}

impl<T> TryInto<MutexGuard<T>> for Mutex<T> {
    type Err = TryLockError<MutexGuard<T>>;
    fn try_into(self) -> Result<Mutex<T>, Self::Err> {
        self.try_lock()
    }
}

impl TryInto<process::Output> for process::Child {
    type Err = io::Error;
    fn try_into(self) -> Result<process::Output, io::Error> {
        self.wait_with_output()
    }
}

impl TryInto<String> for Vec<u8> {
    type Err = FromUtf8Error;
    fn try_into(self) -> Result<String, FromUtf8Error> {
        String::from_utf8(self)
    }
}

This trait seems almost too-generic. In all the above examples it would be much better to explicitly say what you're actually doing rather than call try_into.

I think having TryInto<...> or TryFrom<...> parameters seems like questionable form.

Also agree. Why wouldn't you just do the conversion and handle the error before passing the value to the function?

ghost commented 7 years ago

std::err::Error doesn't exist in #![no_std] builds. Also, in many cases there is no benefit in implementing std::err::Error for an error type even when it is available. Thus, I would prefer to not have any such bound.

The one benefit to being bounded by std::error::Error is that it can be the return value of another error's cause(). I really don't know why there isn't a core::error::Error, but I haven't looked into that.

Also, I found that naming the associated error type Err instead of Error was error-prone as I always typed Error. I noticed that this mistake was made even during the drafting of the RFC itself: rust-lang/rfcs#1542 (comment). Also, code that uses Result already uses the name Err extensively since it is a Result constructor.

FromStr, which is stable, also uses Err for its associated type. Whether it's the best name or not, I think it's important to keep it consistent throughout the standard library.

Whether or not TryFrom and TryInto are too general, I would really like to see fallible conversions, at least between integer types, in the standard library. I have a crate for this, but I think the use cases go far enough to make it standard. Back when Rust was alpha or beta, I remember using FromPrimitive and ToPrimitive for this purpose, but those traits had even bigger problems.

sfackler commented 7 years ago

Error can't be moved into core due to coherence issues.

Also due to coherence issues Box<Error> doesn't implement Error, another reason we don't bound the Err type.

There really isn't any need to bound it at the trait definition, anyway - you can always add that bound yourself later:

where T: TryInto<Foo>, T::Err: Error
m4b commented 7 years ago

I don't usually comment on these threads, but I've been waiting for this for a while and as expressed by several above, I'm not sure what the hold up is; I always want a from trait that can fail. I have code all over that is called try_from... This trait perfectly encapsulates that idea. Please let me use it on stable rust.

I also wrote a whole bunch of other things, but I've since deleted it as unfortunately, trait coherence prevents this trait from being as useful as it could be for me. E.g., I've essentially reimplemented a specialized version of this exact same trait for the primitive types for a highly generic parser of those types. Don't worry, I will rant about this some other time.

That being said, I believe str::parse will benefit greatly from this, as it also specializes a FromStr trait as the bound - which is exactly (TryFrom<str>) hand specialized.

So correct me if I'm wrong, but I believe stablilizing and using this for str::parse will:

  1. remove the boilerplate reimplementation, and hence remove a less familiar, and specialized trait
  2. add TryFrom<str> in the signature, which properly is in the convert module and is more readily obvious what it does
  3. will allow upstream clients to implement TryFrom<str> for their data types and get str.parse::<YourSweetDataType>() for free, along with other try_from they feel like implementing, which makes for better logical code organization.

Lastly, abstractions aside, code re-use, bla bla, I think one of the understated benefits of traits like this is the semantic purchase they provide for beginners and veterans alike. Essentially they provide (or are beginning to provide, the more we stabilize) a uniform landscape with familiar, canonicalized behavior. Default, From, Clone are really great examples of this. They provide a memorable function landscape which users can reach to when performing certain operations, and whose behavior and semantics they already have a good grasp on (learn once, apply everywhere). E.g.:

  1. I want a default version; oh let me reach for SomeType::default()
  2. I want to convert this, I wonder if SomeType::from(other) is implemented (you can just type it and see if it compiles, without reaching for documentation)
  3. I want to clone this, clone()
  4. I want to try to get this from this, and since errors are an integral part of rust, it can fail in the signature, so let me try_from - oh wait :P

All of these methods become commonplace and become apart of the Rust users toolkit, which imho reduces logical burden by allowing us to reach for familiar concepts (and thats just another name for a Trait!) whose documentation and semantic behavior we've already internalized.

Of course they don't always match up, in which case we specialize our datastructures, but traits like this reduce the API burden of users and coders alike by giving them access to concepts they've already studied, instead of reading documentation/finding your from_some_thing, etc. Without even having to read your documentation, by using std traits, users can navigate your api and datastructures in a logical, robust and efficient manner.

In some sense, it's formalizing a gentleperson's agreement amongst ourselves, and makes it easier and more familiar for us to perform certain familiar operations.

And that's all I have to say about that ;)

sfackler commented 7 years ago

This was previously blocked on an investigation into the possibility of using ! as the error type for infallible integer conversions. As the feature is currently implemented, this fails in even the most simple cases: https://is.gd/Ws3K7V.

Are we still thinking about changing the method names, or should we put this feature into FCP?

Ixrec commented 7 years ago

@sfackler That playground link works for me if I change the return type on line 29 from Result<u32, ()> to Result<u32, !>: https://is.gd/A9pWbU It does fail to recognize that let Ok(x) = val; is an irrefutable pattern when val has Err type !, but that doesn't seem like a blocking issue.

sfackler commented 7 years ago

@Ixrec a primary motivation for these traits was conversions to and from C integer typedefs. If I have a function

fn foo(x: i64) -> Result<c_long, TryFromIntError> {
    x.try_into()
}

This will compile on i686 targets but not on x86_64 targets.

Similarly, say I want to replace the IntoConnectParams type: https://docs.rs/postgres/0.13.4/postgres/params/trait.IntoConnectParams.html. How can I do that if there's a blanket impl<T> TryFrom<T> for T { type Error = ! }? I need the reflexive implementation for ConnectParams, but with a different concrete error type than !.

canndrew commented 7 years ago

It does fail to recognize that let Ok(x) = val; is an irrefutable pattern when val has Err type !

Note that there's a PR open for that.

If I have a function ...

This should work though

fn foo(x: i64) -> Result<c_long, TryFromIntError> {
    let val = x.try_into()?;
    Ok(val)
}
jimmycuadra commented 7 years ago

At the risk of being an annoying +1 comment, I just want to mention that after macros 1.1 arrives in Rust 1.15, try_from will be the last feature keeping Ruma on nightly Rust. Stable try_from is eagerly anticipated!

jimmycuadra commented 7 years ago

On a more substantial note...

This is the thing I find most strange about this feature. Most functions I write returns a Result but I've never named any of those functions with a try_ prefix except when trying to experiment with this trait.

That's a good observation, but I think the reason for the try_ prefix is not that it's necessary to identify the return type as a Result, but to distinguish it from the non-fallible equivalent.

Also, I found that naming the associated error type Err instead of Error was error-prone as I always typed Error. I noticed that this mistake was made even during the drafting of the RFC itself: rust-lang/rfcs#1542 (comment). Also, code that uses Result already uses the name Err extensively since it is a Result constructor.

I agree on this one. Most other error types I have come across in libraries are named "Error" and I like that so far "Err" has only ever meant Result::Err. It seems like stabilizing it as "Err" will result (no pun intended) in people constantly getting the name wrong.

sfackler commented 7 years ago

@canndrew Of course it's possible to make work, but the entire point of that motivation for this feature is to make it easy to correctly handle these kinds of platform differences - we want to avoid the entire space of "compiles on x86 but not ARM".

I think I chose Err for consistency with FromStr but I would be very happy to switch to Error before stabilizing!

shepmaster commented 7 years ago

the last feature keeping Ruma on nightly Rust

Likewise, the alternate playground backend only needs nightly for access to TryFrom; the nightly serde stuff was too unstable for my liking, so I moved to the build script setup. With 1.15, I'll be moving back to #[derive]. I'm eagerly awaiting this feature becoming stable!

canndrew commented 7 years ago

@sfackler Sorry I wasn't following. In the case of integer conversions it sounds like what we really need is to not typedef c_ulong to either u32 or u64 depending on platform but to somehow make it a newtype. That way a TryFrom<c_ulong> impl can't interfere with a TryFrom<u{32,64}> impl. After all, we're always going to have "compiles one platform but not the other" problems if we define types differently on different platforms. It's a shame to have to sacrifice the otherwise-entirely-logical TryFrom<T> for U where U: From<T> impl just so that we can supporting what seems like questionable practice.

I'm not seriously suggesting we block this RFC until we get an integer newtype RFC written+merged+stabilized. But we should keep it in mind for the future.

Similarly, say I want to replace the IntoConnectParams type:

What's the problem here though? Why wouldn't you use one error type for TryFrom<ConnectParams> and another for TryFrom<&'a str>?

sfackler commented 7 years ago

I would not advocate that we break literally all FFI code in the world. Having tried and failed to pick up similar integer newtype wrappers like Wrapping, there are massive ergonomic costs.

Is there a impl<T> From<!> for T in the standard library? I don't see it in the docs but that might just be a rustdoc bug. If it's not there, then there's no way to convert the TryFrom<ConnectParams> impl's ! Error into the one I actually need.

canndrew commented 7 years ago

Having tried and failed to pick up similar integer newtype wrappers like Wrapping, there are massive ergonomic costs.

I was thinking something more like being able to define your own integer types a. la. C++:

trait IntLiteral: Integer {
    const SUFFIX: &'static str;
    const fn from_bytes(is_negative: bool, bytes: &[u8]) -> Option<Self>; // or whatever
}

impl IntLiteral for c_ulong {
    const SUFFIX: &'static str = "c_ulong";
    ...
}

extern fn foo(x: c_ulong);

foo(123c_ulong); // use a c_ulong literal
foo(123); // infer the type of the integer

Would that solve most the ergonomic issues? I don't actually like C++ user defined literals - or features in general that give people the power to change the language in confusing ways - but I guess it could end up being the lesser of two evils.

Is there a impl<T> From<!> for T in the standard library?

Not currently because it conflicts with the From<T> for T impl. My understanding is that impl specialization should eventually be able to handle this though.

sfackler commented 7 years ago

That sounds like something we should circle back to in a couple of years.

What's the timeline on stabilization of specialization and !?

canndrew commented 7 years ago

For specialization I don't know. For ! itself it's mostly a matter of whenever I can get my patches merged.

BlacklightShining commented 7 years ago

@canndrew I definitely agree that it shouldn't be implemented for everything. The docs say attempt to construct Self via a conversion, but what counts as a conversion? How aboutā€¦changing the same thing from one representation to another, or adding or removing a wrapper? This covers your Vec<u8> -> String and Mutex<T> -> MutexGuard<T>, as well as things like u32 -> char and &str -> i64; while excluding SocketAddr -> TcpStream and process::Child -> process::Output.

BlacklightShining commented 7 years ago

I feel like impl From<T> for U should probably imply TryFrom<T, Err=!> for U. Without that, functions that take TryFrom<T>s can't also take From<T>s. (Using try_from()|try_into() for a concrete, infallible conversion would probably just be an anti-pattern. You would be able to do it, butā€¦it would be silly, so just don't.)

aldanor commented 7 years ago

I feel like impl From for U should probably imply TryFrom<T, Err=!> for U.

Agreed.

You would be able to do it, butā€¦it would be silly, so just don't.

Sounds like a potential clippy lint.