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

TryFrom<Repr> for enums using FromRepr? #328

Open
jaskij opened this issue Feb 1, 2024 · 2 comments
Open

TryFrom<Repr> for enums using FromRepr? #328

jaskij opened this issue Feb 1, 2024 · 2 comments

Comments

@jaskij
Copy link

jaskij commented Feb 1, 2024

I have quite a few places where .try_into()? is cleaner and more idiomatic than .from_repr().ok_or(/* insert error here */)?.

As such, I would love if there was a macro that implemented that TryInto. I put an example TryFrom at thend, it's a pretty generic thing, as most of the stuff is already done by FromRepr.

There's two open questions:

  1. What error type should it return? It should definitely be more specific than anyhow::Error, and I would love for it to contain the failed value.
  2. Should this be allowed on enums which actually have content? Especially content that does not implement Default?
#[derive(FromRepr)]
#[repr(u16)]
enum Foo {
    Bar = 1,
    Baz = 16,
}


impl TryFrom<u16> for Foo {
    type Error = anyhow::Error;

    fn try_from(value: u16) -> std::result::Result<Self, Self::Error> {
        if let Some(foo) = Self::from_repr(value) {
            Ok(foo)
        } else {
            Err(anyhow!("unknown mode value: {}", value))
        }
    }
}
@Peternator7
Copy link
Owner

I appreciate the sentiment; however, I am hesitant to make a change like this. I get a lot of issues asking for the same implementation of an existing macro, but with a different trait, or a different error type. They're all valid, but it makes configuration confusing, and it's difficult to pick one error type in particular that will make everyone happy.

Here's a playground link that shows why it's hard. The error type being anything other than zero-sized changes the size of the returned Result from 2 bytes to 3 (which gets padded up to 4). That's not much, and it might get inlined, but it is an observable difference that someone might care about so it's easier to generate the optimal code and then allow users to define a wrapper impl if they want to change the error handling.

If you're interested on working on this, I would accept a PR if the following were configurable.

  • The error type
  • The function/callback that turns a repr into an Error

Something like #[strum(try_from_repr(MyError, my_error_func))]

@jaskij
Copy link
Author

jaskij commented Feb 13, 2024

I see what you mean about sizes. I personally don't see caring about the size of a Result<> as opposed to the enum itself, but someone might.

I would like to work on this, seems like the type of thing to get started with proc macros. Unfortunately, I do not have the capability to commit to working on it.

In the interest of further discussion (in case I or someone else does end up writing a PR in the future), some questions:

  • What about the actual implementation? Should it be separate, or depend on FromRepr? I don't remember if strum supports nostd, but if it does, those codebases are quite sensitive to code size.
  • If the implementations are to be separated, would you want common parts factored out of strum_macros::from_repr::from_repr_inner()? At a glance, at least getting the repr could be used in both macros.

Thinking on it further, while it does not align with the current API of strum, and would be a bit hacky, I'm willing to bet this could actually be done with a declarative macro.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants