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 impl TryFrom<T> for E where E is a C-like #[repr(T)] enum #2783

Open
canndrew opened this issue Oct 15, 2019 · 23 comments
Open

Add impl TryFrom<T> for E where E is a C-like #[repr(T)] enum #2783

canndrew opened this issue Oct 15, 2019 · 23 comments
Labels
A-derive Deriving related proposals & ideas T-lang Relevant to the language team, which will review and decide on the RFC.

Comments

@canndrew
Copy link
Contributor

canndrew commented Oct 15, 2019

This is a problem that has come up a lot. The most recent discussion I could find about it is the internals thread here, but that thread has been locked and I figured this should probably have an "official" issue somewhere.

There's currently no (good) way to convert from an enum integer discriminant value to the enum itself. Enums should support deriving TryFrom for this purpose. eg.

#[repr(u32)]
#[derive(TryFrom)]
enum MyCoolEnum {
    Foo = 1,
    Bar = 2,
}

let x: u32 = ... ;
let my_cool_enum = MyCoolEnum::try_from(x).expect("invalid discriminant!");
@RustyYato

This comment has been minimized.

@Centril

This comment has been minimized.

@Centril Centril added A-derive Deriving related proposals & ideas T-lang Relevant to the language team, which will review and decide on the RFC. labels Oct 15, 2019
@RustyYato

This comment has been minimized.

@SimonSapin
Copy link
Contributor

This was easy enough:

macro_rules! enum_try_from_int {
    (
        #[repr($T: ident)]
        $( #[$meta: meta] )*
        $vis: vis enum $Name: ident {
            $(
                $Variant: ident = $value: expr
            ),*
            $( , )?
        }
    ) => {
        #[repr($T)]
        $( #[$meta] )*
        $vis enum $Name {
            $(
                $Variant = $value
            ),*
        }

        impl std::convert::TryFrom<$T> for $Name {
            type Error = ();

            fn try_from(value: $T) -> Result<$Name, ()> {
                match value {
                    $(
                        $value => Ok($Name::$Variant),
                    )*
                    _ => Err(())
                }
            }
        }
    }
}

enum_try_from_int! {
    #[repr(u32)]
    #[derive(PartialEq)]
    pub enum Foo {
        Bar = 4,
        Baz = 7,
    }
}

pub fn main() {
    use std::convert::TryFrom;
    assert!(Foo::try_from(7).unwrap() == Foo::Baz)
}

But yeah it would be nicer as a derive proc macro. It’s debatable whether this should be in the standard library rather than crates.io, but I personally wouldn’t be opposed.

@Centril
Copy link
Contributor

Centril commented Oct 15, 2019

I personally think (as a general principle) that whatever crate provides the trait should also provide the derive macro (well, they cannot be the same crate exactly, but the same project) if a reasonable implementation can be given. In the case of the standard library it would be the language that should provide said derives when structural impls can be given.

@canndrew
Copy link
Contributor Author

If I make a PR for this is it likely to get merged?

@Centril
Copy link
Contributor

Centril commented Oct 16, 2019

It's probably a large enough addition to the language to be RFC-worthy. I think we should also consider adding derives more holistically rather than a one-off.

@bluss
Copy link
Member

bluss commented Oct 19, 2019

I don't know the details of proc macros that well, will the derive always see the repr attribute, or is there an ordering issue?

@carbotaniuman
Copy link

@Centril Why does adding a simple Trait to an object require an entire RFC?

@Centril
Copy link
Contributor

Centril commented Oct 25, 2019

Well it's adding a general mechanism for deriving, not one specific object, and it is a language change that needs some design work so an RFC seems appropriate.

@SimonSapin
Copy link
Contributor

Doesn’t the #[derive(…)] mechanism already exist?

@Centril
Copy link
Contributor

Centril commented Oct 25, 2019

I think I phrased that poorly... Maybe "general" was too general here. I really meant specifically adding derive(TryFrom) to the language as opposed to adding one implementation to one type.

@canndrew Let's check the mood? cc @rust-lang/lang.

@bluss I believe that if #[repr(...)] is in front of the #[derive(...)] then it will be seen by it, but let's cc @petrochenkov.

@SimonSapin
Copy link
Contributor

#[proc_macro_derive] is already part of the language, so this would be adding one procedural macro to the standard library.

@carbotaniuman
Copy link

Although this does have some issues with letting users specify the error-result manually (but we could just restrict it to enums). And I think the intrinsic needed to do this are unstable? Shouldn't affect the stdlib tho.

@SimonSapin
Copy link
Contributor

I think it’s find for this conversion to always have the same TryFrom::Error associated type. The internals thread suggested std::num::TryFromIntError.

What intrinsic do you mean?

@carbotaniuman
Copy link

My bad, I mistook the intrinsic that got the discriminant of an non-C enum. Is there an intrinsic for an int to a C-enum already implemented like the one proposed in the internals thread?

@sfackler
Copy link
Member

Why would this need an intrinsic to convert from an int to an enum? The derive can just match.

@SimonSapin
Copy link
Contributor

Yes, there is std::mem::transmute.

And even without unsafe code, a match expression like in #2783 (comment) optimizes to a no-op when the integer values line up.

@nikomatsakis
Copy link
Contributor

My two cents:

I can't quite tell what is being proposed here -- however I have long wanted some form of macro like this to handle "C-like" enums. It seems like the behavior of derive(TryFrom) here is clear enough for the example, but I don't really know what should happen if this is applied to anything other than a C-like enum -- an error? Does it work for enums that don't have an explicit #[repr]?

It seems like we have plenty of precedent for shipping derives with the language -- most of the standard traits ship them -- so I'm not concerned about shipping one for TryFrom on that front, though this derive feels different from those in that it is much more "special purpose".

As far as procs, this ultimately strikes me as more of a "libs team" call than a lang team one, since we're not adding some fundamental new mechanism, as far as I can tell? Just employing the existing one. I think the libs team process there tends to not require RFCs per se?

@scottmcm
Copy link
Member

I've certainly seen people ask often how to do this in chat, so it does seem like a plausible place to do something.

On the other hand, just TryFrom seems weird here. If that exists, I'd also expect a derive for impl From<MyEnum> for u32 here, for example. But that one being derive(From) feels wrong.

So overall I wonder if it's better to just leave this kind of thing to https://docs.rs/enum_derive/0.1.7/enum_derive/ and friends...

@kornelski
Copy link
Contributor

This is a very common problem. People keep reinventing crates for this, and most crates settle on an interface that is identical or very close to what is proposed here.

@illicitonion
Copy link

This is a very common problem. People keep reinventing crates for this, and most crates settle on an interface that is identical or very close to what is proposed here.
...

FWIW, as one of the maintainers of num_enum, I'd be happy to donate it or any parts of it into std if that's useful (though I can imagine it may be simpler to re-implement from scratch :))

@Ddystopia
Copy link

Hello, what is the state of it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-derive Deriving related proposals & ideas T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

No branches or pull requests