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

#[non_exhaustive] enum support #94

Open
ollie-etl opened this issue Mar 30, 2023 · 9 comments
Open

#[non_exhaustive] enum support #94

ollie-etl opened this issue Mar 30, 2023 · 9 comments

Comments

@ollie-etl
Copy link

I recently hit a bug in packet parsing where I had a non-exhaustive enum, representing an Organization Identifier, which is represented using a non-exhaustive enum. This makes sense: its intractable to keep a full list of all possible Organizations, but the ones I care about I want to have enumerated.

However, this panics.

#[derive(BitfieldSpecifier)]
#[non_exhaustive]
#[bits = 16]
pub enum Ouid {
    SomeOrg = 0x0001,
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn supports_non_exhaustive() {
        Ouid::from_bytes(0).unwrap();
    }
}

I think the correct behavior for non_exhaustive tagged Specifier is to not check the fields

@hecatia-elegua
Copy link

Does the example code have an error? Of course it panics if you unwrap and don't have a variant for 0. I tried this with 1 and it works without panic.

@ollie-etl
Copy link
Author

@hecatia-elegua

Of course it panics if you unwrap and don't have a variant for 0

The example is with an non-exhaustive enum. I had hoped that, as it has both a defined representation in bits, and is marked non_exhaustive, i might be able to convert where i don't know what other states might exist in future. That is after all what non_exhaustive forces downstream crates to do with this enum

@hecatia-elegua
Copy link

Well you can do that, right? It does return a Result, you just can't unwrap it. What should unwrapping return here, otherwise?

@ollie-etl
Copy link
Author

My view is that the conversion from u16 to Ouid in the above example should be infallible, as we're modelling an Ouid which is explicitly marked as non-exhaustive.

As it is, this would be completely fine if the BitSpecifier were only ever consumed in a downstream crate - the non-exhaustive attribute already ensures that code contains a default case when pattern matching the enum. Making it safe when the enum is consumed within the same crate as the bitfield specifier is defined is harder, because the non-exhaustive attribute doesn't enforce the default case, and I'm unsure if there is a mechanism to do so.

@hecatia-elegua
Copy link

hecatia-elegua commented Apr 11, 2023

match ouid {
    Ouid::SomeOrg => /* handle SomeOrg */
    _ => /* handle non-defined Ouid */
}

This works, since ouid is already type Ouid.

from_bytes would need to match all unnamed variant-ranges to something, so I still don't know how that would work without panic and without being fallible. Well, we could define some kind of "catch-all" variant, but then we don't really need non_exhaustive. We can't parse u16 as "Nothing", or I'm completely misunderstanding something here.

@hecatia-elegua
Copy link

One idea I had was to use some kind of conditional compilation to define a variant only on the defining crate, which is hidden for downstream crates. Then we could parse into such a variant and downstream crates could not match on it, only match on _.
But afaik that's not possible since the downstream crate can't specify it uses the "no-special-variant" version while the crate it depends on uses the "special-variant" version.

@ollie-etl
Copy link
Author

Looking at this more, I don't think there is any way, short of compiling to a seperate crate that gives the framework required to do this safely. I'm not that up on macro magic, but I doubt is sufficiently powerful to generate a new dependency crate at compile time?

@ollie-etl
Copy link
Author

from_bytes would need to match all unnamed variant-ranges to something

It could transmute. In this case, where the memory representation is known, and all variants are explicitly given, then I believe this would be safe.

The problem remains that we cannot enforce non-exhaustive matching within the defining crate

@hecatia-elegua
Copy link

Actually, it won't transmute.
If we call Ouid::from_bytes(0) and implement it somewhat like this:

match value {
  1 => Ouid::SomeOrg
  _ => core::mem::transmute(value)
}

it will just return Ouid::SomeOrg.

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