bluejekyll / enum-as-inner

Macros for deriving as functions to access Enums as their inner components
Other
92 stars 11 forks source link

Result types from into_*? #84

Open max-sixty opened 2 years ago

max-sixty commented 2 years ago

Thanks for the great crate! I've just replaced a bunch of methods with enum-as-inner: https://github.com/max-sixty/prql/pull/146. I did have one question:

The Result from .into_* is different from most Result types — it seems to be Result<OutType, InType>>. Is this intentional? It's not the only place that doesn't conform to Result<OutType, Err>, so possibly this being surprising is because of my lack of rust understanding?

Currently I'm doing .map_err a lot, rather than just ?.

Thank you!

bluejekyll commented 2 years ago

So you should be able to deal with this based on the ? semantics (I'm pretty sure).

If you implement this for your Error type (using your example terms):

impl From<InType> for YourErrorType {
    fn from(in: InType) -> Self {
        ...
        Self
    }
}

And then ? should perform the auto coercion.

max-sixty commented 2 years ago

Yes, that's right!

I find it a bit surprising that we'd implement a From<ManyVariants> for Error — would we include a message about being unable to convert the type into its inner? Presumably if this were a common pattern, we'd use this for more than just this library. And we don't have that much context in that function — e.g. the type it was intending to convert to.

(as above though, it's very likely this is a deficiency in my rust knowledge, and feel free to point me to a link, or even close, if you're confident)

bluejekyll commented 2 years ago

I think the From impl is the simplest thing in this situation. To your point though, we could make an Error type that we return from these fallible functions and then you'd perform the coercion on that. Personally, I like the fact that this returns the source value in the Result, as it allows you to continue to own the data. This can be useful in state machines and other scenarios.

That error type would be: struct Error<T>(T), and then our into functions could return a Result<OutType, Error<InType>>, that would remove any concerns on incorrect type coercion, as you'd then make your From impl be impl From<Error<InType>> for YourErrorType ...

I can see the an advantage, though I think this will be low priority for me at least. I'd happily take a PR for it though.

agirorn commented 1 year ago

What bothers me the most about using the Result<OutType, InType>> is that the InType does not include the backtrace. And if I want the backtrace in it then I have to code gymnastics to get it. I mostly use this crate for testing Vec's and picking out a specific index and casting it into a specific value and when it is the wrong enum variant then that is a failure in the test.

#[derive(Clone)]
struct One {
  is: String;
}
struct Two {
  is: String;
}
#[derive(EnumAsInner, Clone)]
enum E { 
  One(One),
  Two(Two),
}

let list: Vec<E> = get_list_from_somewhere();
let one = list[0].clone().into_one()?; // It would be really nice to get a good standard error here with backtrace
assert_eq!(one.id, "some_id);

Could we not achieve both objectives of returning an error when failing to cast into a variant and continuing to own the data by returning an error?

use std::backtrace::Backtrace;

#[derive(Debug, Clone)]
struct EnumAsInnerError<T> {
   ... the needed error properties like message and backtrace
  message: String, // This could include a message like "Unable to convert Two into One"
  original: T
  backtrace: Backtrace
}

I'm fairly new to rust so be gentle if my EnumAsInnerError example is not 100% :)

agirorn commented 1 year ago

I did some experiments to have the into_variant functions return a generic error and these are my findings so far.

  1. Rust does not print the backtrace when you use the unwrap() shorthand ? so if the desire is to get a backtrace from errors you have to use the longer .into_x().unwrap()

  2. The crate enum-as-inner can't export the error struct or enum. When I try to do it I get this error

error: `proc-macro` crate types currently cannot export any items other than functions tagged with `#[proc_macro]`, `#[proc_macro_derive]`, or `#[proc_macro_attribute]`

Assuming you want to explore the idea of returning a more generic error from the crate.

The second issue of being unable to return the error from the crate could be overcome by having another crate that returns the Error struct&enum or using some crate from crates.io that already has this capability.

use enum_as_inner::EnumAsInner;
use enum_as_inner_types::EnumAsInnerError;

#[derive(Clone)]
struct One {
  is: String;
}
struct Two {
  is: String;
}
#[derive(EnumAsInner, Clone)]
enum E { 
  One(One),
  Two(Two),
}

let list: Vec<E> = get_list_from_somewhere();
let one = list[0].clone().into_one();
if let Err(err) = one {
    // Getting at the original value after consuming it when `.into_one()` was called
    assert_eq!(err.original, E::Two { id "some id" });
} else {
    panic!("this should not happen");
}

let list: Vec<E> = get_list_from_somewhere();
let one = list[0].clone().into_one().unwrap(); // Failing with a backtrace
assert_eq!(one.id, "some_id);

Another thing that I think would be really valuable to get from the error message is what you intended to coerce the value into and as a result what you got.

Let's say for this example from above

let e = E::One(One { id: "one".to_string });
let two = e.into_two().unwrap(); // this fails 

You could get this error

thread 'main' panicked at src/main.rs:9:13:
called `Result::unwrap()` on an `Err` value: Unable to coerce E::One into E::Two. Original value E::One(One { id: "one" })
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

This would give the user of the crate a lot of relevant information initially when their code fails.

sinui0 commented 1 year ago

@agirorn You might find https://github.com/bluejekyll/enum-as-inner/issues/101#issuecomment-1780423005 this relevant