sinui0 commented 1 year ago

Hi, thanks for the nice crate!

I've forked this project to try out using Result<Variant, Error<EnumType>> as a return type instead. I've found it to be quite ergonomic for my uses (state machine) as it works well with ?.

Having the error variant be generic over the enum makes it quite natural to implement From<Error<EnumType>> for propagation. This eliminates the need to map an Option at every call site.

Would you consider supporting this API in this crate? Supporting a try variant seems natural and idiomatic. This seems to be more-or-less what you suggested in #84, but also returns Result in the as_* and as_*_mut accessors. Maybe these accessors can be renamed to try_as_* and try_as_*_mut for EnumTryAsInner

Happy to open a PR, but I think there are some design decisions that need to be made that ought to be discussed first.

Example API

use enum_try_as_inner::{EnumTryAsInner, Error};

#[derive(Debug)]
struct Green {
    passed: usize,
}

#[derive(Debug)]
struct Red {
    waiting: usize,
}

#[derive(Debug, EnumTryAsInner)]
enum State {
    Green(Green),
    Red(Red),
}

#[derive(Debug, thiserror::Error)]
enum LightError {
    #[error(transparent)]
    StateError(#[from] Error<State>),
    #[error("not enough cars have passed: {0}")]
    NotEnoughPassed(usize),
}

struct Light {
    state: State,
}

impl Light {
    fn increment_passed(&mut self) -> Result<(), LightError> {
        self.state.as_green_mut()?.passed += 1;

        Ok(())
    }

    fn turn_red(&mut self) -> Result<(), LightError> {
        let &Green { passed } = self.state.as_green()?;

        if passed < 8 {
            return Err(LightError::NotEnoughPassed(passed));
        }

        self.state = State::Red(Red { waiting: 0 });

        Ok(())
    }
}

Error Type

I've not yet settled on a design for the error type, there are a handful of approaches with different trade-offs.

Single error type

A simple approach could look like this, where Error<T> is exported by this crate and T is the enum.

struct Error<T>(Option<T>);

impl<T> Error<T> {
  /// Returns the value, if present.
  pub fn into_value(self) -> Option<T> {
    self.0
  }
}

For the try_into_* method, the value would be preserved for cases where the User wants to do something with the actual value. try_as_* methods would have None.

Expected variant

In many cases I expect it to be helpful to propagate the expected variant, like so:

struct Error<T> {
  /// eg. `MyEnum::Variant0`
  expected: &'static str,
  value: Option<T>,
}

where the proc macro defines a static string representation of each enum variant which can be passed into the error for the different accessors. This is simple enough with stringify in the accessor method.

Runtime info

The short-coming of the above approach is that the try_as errors provide no information about the actual value. Users may wish to implement additional handling based on such. A potential improvement would be to add an actual field:

struct Error<T> {
  expected: &'static str,
  actual: &'static str,
  value: Option<T>
}

However, this would require implementing something like From<T> for &'static str which may not be desirable. strum does provide this. Could even go as far as to replace the static strings with generated discriminants. But again, this may start getting bloaty.

Error-per-enum

I think in #84 you were contemplating generating an error type for each enum, where it is generic over the variant? This would avoid having to provide an error type with this crate.

#[derive(EnumTryAsInner)]
enum MyEnum {
  One(bool),
  Two(bool),
}

// generated by macro
struct MyEnumError<T>(Option<T>);

// error type is ambiguous
impl From<MyEnumError<bool>> for MyOtherError {
  ..
}

Though, as illustrated above, the error type signature can become ambigious if the enum has variants containing the same type. This could be alleviated with the static string repr approach shown earlier, which would let the user match on the actual variant in the conversion.

Thanks for any consideration! If this seems too out of scope for you I can polish my fork and publish under a different name.

sinui0 commented 1 year ago

I went ahead and published enum-try-as-inner, but would be happy to move this functionality into this crate in the future if so desired. Thanks!

bluejekyll commented 1 year ago

I’ll take a look at your comments when I have a moment soon. Sorry for missing them earlier.

bluejekyll / enum-as-inner