Allow non-exhaustive bitenum fields to be either a Result<T> or Option<T>

[vortexofdoom]

Currently the bitfield macro requires that non-exhaustive enums be wrapped in an Option, but the generated methods return a Result, which I think is unintuitive.

I'd propose that we either require that they be wrapped as a Result, that we change the methods to return an Option (essentially ignoring invalid values when called) or provide support for either.

I can definitely see the value in knowing what the invalid value was, but sometimes an invalid bit configuration is closer in meaning to None than Err, so it'd be cool to include that.

[danlehmann]

It's ugly, but I think fixing this would be a breaking change. Unless you have a clever idea?

[vortexofdoom]

I can't think of a way to not be breaking either, but I think it'd be a worthwhile change eventually?

I think the ideal solution would be to define whether it would be Option/Result on the enum itself when you mark it as non-exhaustive, then you could just use the enum itself as a field, since you'd have already clarified how invalid configurations should be handled.

[danlehmann]

So I wonder if maybe this would be better fixed by newer syntax. The issue with either Option or Result is that it's not very symmetric. It applies to reads, but I think having to wrap every with_xxx argument in an Ok(x) would be somewhat annoying.

So maybe a better fix would be this:

• Fix bitenum so that it returns Result of the right size (e.g. u4 instead of u8)
• Instead of looking for Option, make this an attribute:

#[bits(4..=7, rw, exhaustive = false)]
foo: MyEnum,

If exhaustive is false, the result of new_with_raw_value is Result<MyEnum, u4>. If it's true (or missing), result is MyEnum

[pbl-pw]

I think #[bits(4..=7, rw, exhaustive = false)] still annoying

use arbitrary_int::UInt;

/// type witch can be non exhaustive repr by Raw
pub trait Uncheckable: Copy {
    type Raw: Copy;
    fn unchecked_raw_value(self) -> Self::Raw;
    fn try_from_raw_value(raw: Self::Raw) -> Result<Self, Self::Raw>;
}

/// Unchecked means must check on read, but not check on write, T into Unchecked always ok, T::Raw into Unchecked always ok
#[derive(Debug, Clone, Copy, Default)]
#[repr(transparent)]
pub struct Unchecked<T: Uncheckable> {
    raw: T::Raw,
}

impl<T: Uncheckable> Unchecked<T> {
    pub const fn raw_value(self) -> T::Raw {
        self.raw
    }

    pub const fn new_with_raw_value(raw: T::Raw) -> Self {
        Self { raw }
    }

    pub fn get(self) -> Result<T, T::Raw> {
        T::try_from_raw_value(self.raw)
    }

    pub unsafe fn get_unchecked(self) -> T {
        self.get().unwrap_unchecked()
    }

    pub fn get_or_default(self) -> T
    where
        T: Default,
    {
        self.get().unwrap_or_default()
    }
}

impl<T: Uncheckable> From<T> for Unchecked<T> {
    fn from(value: T) -> Self {
        Self { raw: value.unchecked_raw_value() }
    }
}

impl<T: Uncheckable> PartialEq<Unchecked<T>> for Unchecked<T>
where
    T::Raw: PartialEq,
{
    fn eq(&self, other: &Unchecked<T>) -> bool {
        self.raw == other.raw
    }
}
impl<T: Uncheckable> Eq for Unchecked<T> where T::Raw: Eq {}

impl<T: Uncheckable> PartialEq<T> for Unchecked<T>
where
    T::Raw: PartialEq,
{
    fn eq(&self, other: &T) -> bool {
        self.raw == other.unchecked_raw_value()
    }
}

/// impl uncheckable enum，enum must be defined by bitbybit::bitenum
#[doc(hidden)]
#[macro_export]
macro_rules! _p_unchecked_impl_uncheckable_enum {
    ($enum:ident, $repr:ty) => {
        impl $crate::unchecked::Uncheckable for $enum {
            type Raw = $repr;
            fn unchecked_raw_value(self) -> Self::Raw {
                self.raw_value()
            }
            fn try_from_raw_value(raw: Self::Raw) -> Result<Self, Self::Raw> {
                Self::new_with_raw_value(raw).map_err(|_| raw)
            }
        }
        impl $enum {
            pub const fn into_unchecked(self) -> $crate::unchecked::Unchecked<Self> {
                $crate::unchecked::Unchecked::new_with_raw_value(self.raw_value())
            }
        }
    };
}
#[doc(inline)]
pub use _p_unchecked_impl_uncheckable_enum as impl_uncheckable_enum;

macro_rules! impl_uncheckable_bit_int {
    ($src:ty) => {
        impl<const BITS: usize> Uncheckable for UInt<$src, BITS> {
            type Raw = $src;
            fn unchecked_raw_value(self) -> Self::Raw {
                self.value()
            }
            fn try_from_raw_value(raw: Self::Raw) -> Result<Self, Self::Raw> {
                if raw & !Self::MASK == 0 {
                    Ok(unsafe { Self::new_unchecked(raw) })
                } else {
                    Err(raw)
                }
            }
        }
        impl<const BITS: usize> From<$src> for Unchecked<UInt<$src, BITS>> {
            fn from(v: $src) -> Self {
                Unchecked::new_with_raw_value(v)
            }
        }
    };
}
impl_uncheckable_bit_int!(u8);
impl_uncheckable_bit_int!(u16);
impl_uncheckable_bit_int!(u32);
impl_uncheckable_bit_int!(u64);

I use this Unchecked custom type with bitfield and other values come from unreliable source. With some change to macro generated code, we could make this works:

#[bitenum(u2, exhaustive: false)]
#[rustfmt::skip]
pub enum SampleId {
	None  = 0,
	Begin = 1,
	End   = 2,
}


#[bitfield(u8)]
#[rustfmt::skip]
pub struct Flags {
	#[bits(0..=1, rw)] checked: SampleId,
	#[bits(2..=3, rw)] unchecked: Unchecked<SampleId>,
}

fn test(smp: Flags){
	smp.checked(); // ->SampleId;
	smp.with_checked(SampleId::Begin); // ->Flags;
	smp.unchecked(); // ->Result<SampleId, u2>;
	smp.with_unchecked(SampleId::End); // ->Flags,  T to Unchecked<T> always ok
}

and It's symmetric and simple in my opinion

[vortexofdoom]

Ok, so after thinking some more, I don't think this needs to be overly complicated. I think that rather than annotating the struct field (with the wrapper as now OR with an attribute), the return type (Result<Enum> vs Enum) could just be implicit based on whether the enum itself is marked exhaustive or not.

So you'd just have:

#[bitfield(u8)]
struct Foo {
    #[bits(0..=2, rw)]
    bar: Bar, // some enum
    ..
}

and if Bar is exhaustive, the bar() function will return Bar, if not, it will return Result<Bar>. I think that non-exhaustive enums returning a Result when taken from arbitrary bit patterns is probably easy enough to understand all on its own, and avoids the awkward Option wrapping or specifying exhaustive redundantly.

The way to do that would probably be to have an internal constructor in bitenum that returned the appropriate type, and then use that in the bitfield codegen. This could mean that we could change it to only require that attribute if the bitenum is non-exhaustive.

The original proposal allowing either type could also be implemented with an associated type for bitenum that tells what said constructor gives you, and you could specify this instead of a simple bool as part of specifying whether it's exhaustive (the attribute name could change, maybe invalid?):

#[bitenum(u2, invalid: Option)]
enum BitEnum {
    Foo,
    Bar,
    Baz,
}

// generated code
impl BitEnum {
    type Constructed = Option<Self>;

    fn from_raw(value: u2) -> Self::Constructed {
        // code here
    }
}

If the struct field doesn't need to be annotated in any way, it's not really terribly important if it's a Result or Option since you can always discard an Err value, but I figured I'd outline the approach anyway.