Robbepop / modular-bitfield

Macro to generate bitfields for structs that allow for modular use of enums.
Apache License 2.0
155 stars 40 forks source link

`#[non_exhaustive]` enum support #94

Open ollie-etl opened 1 year ago

ollie-etl commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago
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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.