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

Tracking issue for TryFrom/TryInto traits #33417

Open
alexcrichton opened this Issue May 4, 2016 · 233 comments

Comments

Projects
None yet
@alexcrichton
Copy link
Member

alexcrichton commented May 4, 2016

Tracking issue for rust-lang/rfcs#1542


To do:

  • Is existing documentation satisfactory?
  • #56796 Change bounds on TryFrom blanket impl to use Into instead of From
  • (Re)stabilization PR

sfackler added a commit to sfackler/rust that referenced this issue May 5, 2016

sfackler added a commit to sfackler/rust that referenced this issue May 5, 2016

sfackler added a commit to sfackler/rust that referenced this issue May 7, 2016

Manishearth added a commit to Manishearth/rust that referenced this issue May 8, 2016

Manishearth added a commit to Manishearth/rust that referenced this issue May 8, 2016

Manishearth added a commit to Manishearth/rust that referenced this issue May 8, 2016

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented May 9, 2016

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

This comment has been minimized.

Copy link
Member

chris-morgan commented May 10, 2016

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

@shepmaster

This comment has been minimized.

Copy link
Member

shepmaster commented Jul 9, 2016

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 sfackler added the I-nominated label Jul 9, 2016

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Jul 9, 2016

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

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Jul 13, 2016

🔔 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

This comment has been minimized.

Copy link
Contributor

ollie27 commented Jul 14, 2016

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

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Jul 14, 2016

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

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Jul 15, 2016

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

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Jul 15, 2016

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

This comment has been minimized.

Copy link
Member

sfackler commented Jul 15, 2016

@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

This comment has been minimized.

Copy link
Contributor

glaebhoerl commented Jul 15, 2016

@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Jul 15, 2016

@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

This comment has been minimized.

Copy link
Contributor

ollie27 commented Jul 16, 2016

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

This comment has been minimized.

Copy link
Contributor

malbarbo commented Jul 27, 2016

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

@marco9999

This comment has been minimized.

Copy link

marco9999 commented Feb 3, 2019

Sorry if this is the wrong place, but looks like there is a mistake in the docs for TryFromIntError - it lists impl Debug for TryFromIntError, when actually there is no code for it. The compiler generates an error currently when trying to unwrap a result from a TryFrom usage.

Is there meant to be a fmt::Debug impl on this?

@lnicola

This comment has been minimized.

Copy link
Contributor

lnicola commented Feb 3, 2019

@marco9999

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
pub struct TryFromIntError(());

It's got a #[derive(Debug)] attribute.

@marco9999

This comment has been minimized.

Copy link

marco9999 commented Feb 3, 2019

Hmm yea there is... is something not working properly?

error[E0599]: no method named `unwrap` found for type `std::result::Result<usize, <T as std::convert::TryInto<usize>>::Error>` in the current scope
  --> src\types\b8_memory_mapper.rs:67:51
   |
67 |         let address: usize = T::try_into(address).unwrap();
   |                                                   ^^^^^^
   |
   = note: the method `unwrap` exists but the following trait bounds were not satisfied:
           `<T as std::convert::TryInto<usize>>::Error : std::fmt::Debug`
@lnicola

This comment has been minimized.

Copy link
Contributor

lnicola commented Feb 3, 2019

@marco9999 You're probably missing a generic constraint. TryFromIntError is only used by some types, but your T could be anything:

fn foo<T: TryInto<u8>>(x: T) -> u8
where
    <T as TryInto<u8>>::Error: Debug,
{
    x.try_into().unwrap()
}

Anyway, this is a bit out of topic, sorry everyone. IRC might be a better place to ask these questions.

@shepmaster

This comment has been minimized.

Copy link
Member

shepmaster commented Feb 4, 2019

I want to assert_eq!(some_value, std::num::TryFromIntError(()));

@icefoxen There's no useful value associated with TryFromIntError so such an assertion doesn't seem to have much value. If you have a Result<_, TryFromIntError> and it's an Err, there's no other value it could be.

assert!(some_value_result.is_err());

This seems reasonable to me.

pietroalbini added a commit to pietroalbini/rust that referenced this issue Feb 10, 2019

Rollup merge of rust-lang#57926 - icefoxen:test-doc-pr, r=frewsxcv
Tiny expansion to docs for `core::convert`.

This is not really significant, accept or reject as you wish.  I just want to make sure I understand how the PR process works and that I'm doing it right before doing a bigger one for rust-lang#33417.

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Feb 10, 2019

Rollup merge of rust-lang#57926 - icefoxen:test-doc-pr, r=frewsxcv
Tiny expansion to docs for `core::convert`.

This is not really significant, accept or reject as you wish.  I just want to make sure I understand how the PR process works and that I'm doing it right before doing a bigger one for rust-lang#33417.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Feb 10, 2019

Rollup merge of rust-lang#57926 - icefoxen:test-doc-pr, r=frewsxcv
Tiny expansion to docs for `core::convert`.

This is not really significant, accept or reject as you wish.  I just want to make sure I understand how the PR process works and that I'm doing it right before doing a bigger one for rust-lang#33417.
@glaebhoerl

This comment has been minimized.

Copy link
Contributor

glaebhoerl commented Feb 13, 2019

Cross-referencing: #58302

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Feb 13, 2019

Thanks @glaebhoerl.

Due to a blocking bug being fixed (#49593) I was hoping that the never type could be stabilized Soon® #57012 and unblock this. However a new issue (#57012 (comment)) has come up, and we also don’t have consensus on another one (#57012 (comment)).

So in a libs meeting last week I brought up again the idea, I believe first proposed by @scottmcm in #33417 (comment), to stabilize TryFrom and TryInto without the never type, and instead have an empty enum that could later be made an alias to !.

Last time we discussed this (#33417 (comment)), we couldn’t remember why we hadn’t done this the previous time.

Last week @dtolnay reminded us of the issue: before ! becomes a full type, it can already be used in place of the return type of a function to indicates that it never returns. This includes function pointer types. So, assuming #58302 lands in this cycle, code like this will be valid in Rust 1.34.0:

use std::convert::Infallible;
trait MyTrait {}
impl MyTrait for fn() -> ! {}
impl MyTrait for fn() -> Infallible {}

Because fn() -> ! and fn() -> Infallible are two different (pointer) types, the two impls do not overlap. But if we replace the empty enum with a pub type Infallible = !; type alias (when ! becomes a full type), then the two impls will start to overlap and code like the above will break.

So changing the enum to an alias would be a breaking change. On principle we wouldn’t allow it in the standard library, but in this case we felt that:

  • One has to go out of their way to build code that is broken by this change, so it seems unlikely to happen in practice
  • We’ll use Crater to get additional signal when the time come
  • If we end up judging the breakage to be significant enough, having both the never type and an empty enum with the same role is an inconsistency we can live with.

Based on this discussion, I submitted #58302 which is now in Final Comment Period.

@icefoxen

This comment has been minimized.

Copy link
Contributor

icefoxen commented Feb 14, 2019

#58015 should be ready for review/merge now.

@xfix

This comment has been minimized.

Copy link
Contributor

xfix commented Feb 25, 2019

@kennytm Isn't it possible to refer to ! in stable already? For instance, consider the following:

trait MyTrait {
    type Output;
}

impl<T> MyTrait for fn() -> T {
    type Output = T;
}

type Void = <fn() -> ! as MyTrait>::Output;

After doing this, Void refers to ! type.

@KrishnaSannasi

This comment has been minimized.

Copy link
Contributor

KrishnaSannasi commented Feb 25, 2019

That looks like a bug, which means stability guarantees don't extend to it. Using the never type (!) as a type in any capacity is still unstable, at least until #57012 is merged.

bors added a commit that referenced this issue Feb 25, 2019

Auto merge of #58302 - SimonSapin:tryfrom, r=alexcrichton
Stabilize TryFrom and TryInto with a convert::Infallible empty enum

This is the plan proposed in #33417 (comment)

bors added a commit that referenced this issue Feb 25, 2019

Auto merge of #58302 - SimonSapin:tryfrom, r=alexcrichton
Stabilize TryFrom and TryInto with a convert::Infallible empty enum

This is the plan proposed in #33417 (comment)

Centril added a commit to Centril/rust that referenced this issue Feb 28, 2019

Rollup merge of rust-lang#58015 - icefoxen:tryfrom-docs, r=SimonSapin
Expand docs for `TryFrom` and `TryInto`.

The examples are still lacking for now, both for module docs and for methods/impl's.  Will be adding those in further pushes.

Should hopefully resolve the doc concern in rust-lang#33417 when finished?

Centril added a commit to Centril/rust that referenced this issue Feb 28, 2019

Rollup merge of rust-lang#58015 - icefoxen:tryfrom-docs, r=SimonSapin
Expand docs for `TryFrom` and `TryInto`.

The examples are still lacking for now, both for module docs and for methods/impl's.  Will be adding those in further pushes.

Should hopefully resolve the doc concern in rust-lang#33417 when finished?
@bartsmykla

This comment has been minimized.

Copy link

bartsmykla commented Mar 12, 2019

How can I help with documentation? :-)

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Mar 12, 2019

Oh I thought #58015 had landed, but it hasn’t yet… Let’s discuss it there.

bors added a commit that referenced this issue Mar 12, 2019

Auto merge of #58015 - icefoxen:tryfrom-docs, r=SimonSapin
Expand docs for `TryFrom` and `TryInto`.

The examples are still lacking for now, both for module docs and for methods/impl's.  Will be adding those in further pushes.

Should hopefully resolve the doc concern in #33417 when finished?
@o01eg

This comment has been minimized.

Copy link
Contributor

o01eg commented Mar 30, 2019

Could TryFrom trait have a method to check if argument could be converted without consuming it?

fn check(value: &T) -> bool
@Kerollmops

This comment has been minimized.

Copy link
Contributor

Kerollmops commented Mar 30, 2019

One way to work with non-consuming impossible conversion could be to return the consumed non-convertible value along with the associated error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.