Skip to content

Provide a common API across Option and the Ok and Err variants of Result. #113

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

Closed
wants to merge 7 commits into from

Conversation

brendanzab
Copy link
Member

@bstrie
Copy link
Contributor

bstrie commented Jun 10, 2014

I'm sad that this appears to make error handling uniformly more verbose.

The inconsistency will also make it more challenging to transition to an API with a single trait that abstracts over Option-style things once higher-kinded types are implemented post-1.0.

What would this actually allow us to do, in practical terms? Feel free to invent syntax as needed.


Old API | New API
--------------------|--------------------------------------------------
`.as_slice()` | <code>.as_ref().map_or(&[], &#124;x&#124; slice::ref_slice(x))</code>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to have HTML mangling?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I had to use those because the pipes were interacting with the markdown table syntax.

@thehydroimpulse
Copy link

@bstrie

What would this actually allow us to do, in practical terms? Feel free to invent syntax as needed.

Only a single trait will be required to implement the common methods. Separate implementations will be used for Option or Result specific methods.

// Methods in common with Result and Option.
trait Optional<M[T]> {
    pub fn unwrap_or(self, def: T) -> T { ... }
    pub fn unwrap_or_else(self, f: || -> T) -> T { ... }
}

impl<M[T]> Optional<M[T]> for Option<T> {
    // ...
}

// ...

The M[T] denotes a higher-kinded type (sometimes, you'll use M[_]). I'm not sure if that syntax will fly with everyone, though. I'm almost done an RFC for it, so we'll see.

So in short, we're allowed to abstract over more complex types.

@bstrie
Copy link
Contributor

bstrie commented Jun 10, 2014

I was mostly curious as to how this would make the usage of Option nicer, rather than its implementation. I really couldn't care less about the implementation, as long as it's as nice to use as we can make it.

@thehydroimpulse
Copy link

Gotcha.

What other designs have been considered? What is the impact of not doing this?

- `Result::for_err` could be renamed to `Result::err`. This would be more
succinct.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the rest is accepted, +1 on this.

@Valloric
Copy link

I really couldn't care less about the implementation, as long as it's as nice to use as we can make it.

This. The user doesn't care how nicely refactored the compiler internals are.

@brendanzab
Copy link
Member Author

I really couldn't care less about the implementation, as long as it's as nice to use as we can make it.

That is the point. To simplify things by providing a set of standard methods. You only have to remember one set of methods rather than two, inconsistent APIs.

@bstrie
Copy link
Contributor

bstrie commented Jun 10, 2014

I apologize if I seem grumpy. My complaints stem from the fact that I've never believed that our Option handling is as nice as it needs to be, given how fundamental to the language that it is. Other languages with Options all seem to have mechanisms to ease usage (Haskell, Swift, C#). I fear that people familiar with these languages will judge us unfavorably if idiomatic Option handling is overly clunky.

@schmee
Copy link

schmee commented Jun 10, 2014

Why not introduce some sugar for Option? I think Swift does this really nicely (at least it seems nice, I haven't written any Swift code), with T? as a shorthand for Option<T> and lots of sugar for unwrapping and chaining.

@sinistersnare
Copy link

As I understand it, is the reason that we do not give special sugar for Option type (or any of the Result types) is because the language designers do not want to 'bless' types? Maybe providing a couple general overloadable unary operators might be the answer to things like T?

@thehydroimpulse
Copy link

I really couldn't care less about the implementation, as long as it's as nice to use as we can make it.

I agree. In this situation, it would provide a much simpler implementation and a much nicer interface for users. That's with the introduction of monads and potentially re-introducing the do keyword (or do macro) similar to Haskell's. That's the benefit of HKTs in this scenario.

Here's an example using a fictitious do keyword:

do! {
    action1();
    action2();
    action3();
}

fn action1() -> Option<int> {
    return Some(5);
}

fn action2() -> Option<int> {
    return Some(55);
}

fn action3() -> Option<String> {
    return "foobar".to_string();
}

Moreover, one could bind the value of T from each operation:

do! {
    let a <- action1();
    let b <- action2();
    action3(a, b);
}

fn action3(a: int, b: int) -> Option<String> {
    return "".to_string();
}

The reason for having let a -> instead of let a = is because we aren't binding to the return value of action1() or action2() but the value within the Option.

This would ideally all be done through macros, so as to not give special treatment to the few types.

I find the HKT and monad abstraction to be much more appropriate than simple sugar like T?. This is also just a brief example on how HKTs would allow for a better user experience than just changing some method names.

(You would still need to handle a fail case, otherwise it might fail for you)

@bstrie
Copy link
Contributor

bstrie commented Jun 10, 2014

Is there a reason that we can't implement the do! macro today, in the same vein as the try! macro? Would the API unification presented in this RFC mean that we could use a single macro of this type for both Options and Results?

@thehydroimpulse
Copy link

do! is simply sugar on top of a monad's bind operator (>>= in Haskell). try! is significantly simpler (just a simple pattern match) whereas a monad requires us to be able to abstract over a type constructor.

I haven't put much thought about how this would all affect Result, however. The do! macro would simply return an Option<T>.

So one could do:

fn something_important() -> Option<String> {
    return do! {
        let a <- action1();
        let b <- action2();
        action3(a, b);
    };
}

Versus:

fn something_important() -> Option<String> {
    match (action1(), action2()) {
        (Some(a), Some(b)) => {
            match action3(a, b) {
                Some(s) => Some(s),
                None => return None
            }
        },
        _ => return None
    }
}

So it would result in composition similar to Result with try. However, try! is super simple.

@aturon
Copy link
Member

aturon commented Jun 10, 2014

I think it's worth separating out two aspects of this RFC:

  1. The Option and Result types could (and should?) implement some common interface. This makes it easier to transition code from one to the other, and makes it easier to remember the API. In today's API, there is a rather uneven selection of convenience methods available on the different types, especially when it comes to the Ok versus the Err variants of Result.
  2. The common interface should be "cleaned up" by reducing the number of convenience methods and instead reusing methods like as_ref.

It's hard to argue against the first point: it seems pretty clear that the overall API will be improved if we make the two types support a common interface. Is there any disagreement about that?

@bjz, perhaps it's worth outlining an additional alternative for your design that addresses point 1 above but not point 2, i.e. keeps more of the convenience methods currently available for Option and makes them available for Result as well?

Here are my thoughts on point 2:

  • I think dropping get_ref, get_mut, and take_unwrap are all fine, as the expanded form of each is only slightly longer and especially for get_ is more clear.
  • For the iter methods, I would rather leave in the existing convenience methods, because they are part of a widespread convention in the libraries for producing iterators for a type. (Whether this convention is the right one is a separate issue).
  • For the as_slice and as_mut_slice methods, the expanded form is pretty terrible, so I think these are conveniences worth having.

@brendanzab
Copy link
Member Author

@aturon Good points. Re. as_slice and as_mut_slice, are those used all that often? They seemed a tad extraneous to me - but I could be wrong.

@aturon
Copy link
Member

aturon commented Jun 10, 2014

@bjz I don't have any data here, but my feeling is: they are simple and easy to understand, especially given the as_ref and as_mut methods -- they're part of a family, and it seems reasonable to include the whole set.

@brendanzab
Copy link
Member Author

Ok, I restored the old iterator and slice conversion methods and mirrored them for Result. Hopefully this reduces the scope somewhat.

@pczarn
Copy link

pczarn commented Jun 11, 2014

I prefer the ForOk/ForErr symmetry suggested as an alternative. However, I would incorporate these adapters straight into the type Result<T, E, S = Ok> in order to improve expressiveness. The result can be in one of two states that determine the API's "bias".

There's a way to change the type without discarding the other value.

Type Implements method(s) With operations
Result<T, E, Ok> ok() (no operation)
Result<T, E, Ok> err() casts to a type where S = Err
Result<T, E, Ok> to_option() Converts to Option<T>, discarding the error.
Result<T, E, Err> ok() casts to a type where S = Ok
Result<T, E, Err> err() (no operation)
Result<T, E, Err> to_option() Converts to Option<E>, discarding the value.
Result<T, E, _> all in common with Option as described in the RFC

Additionally, old methods ok and err can be renamed to into_ok and into_err for convenience:

Old API Proposed API
.ok() .into_ok() or .ok().to_option()*
.err() into_err() or .err().to_option()*
.map_err(...) .err().map(...)
.or_else(...) .err().or(...)

* Since to_option is supposed to consume the result, shouldn't it be called into_option?

@brendanzab
Copy link
Member Author

So are you saying that you would have two phantom types, enum Ok {} and enum Err {}?

@pczarn
Copy link

pczarn commented Jun 12, 2014

Yes, either those enums or struct Ok; and struct Err;.

@brendanzab
Copy link
Member Author

They would have to be phantom types if they were going to reuse the same identifiers as the Result variants. Otherwise the constructors would conflict. I'm a little concerned about the 'complexity factor' of this proposal though.

@emberian
Copy link
Member

I'm in favor of this. Seems like a clean refactor to a consistent API. Consistency is king.

@brendanzab
Copy link
Member Author

@cmr Thoughts on the phantom type idea?

@emberian
Copy link
Member

The complexity does not pay for itself, I think.

@aturon
Copy link
Member

aturon commented Jun 12, 2014

@pczarn Can you elaborate on what you see as the benefits of using phantom types here? It looks like Result<T, E, Ok> is @bjz's Result<T, E> and Result<T, E, Err> is @bjz's ForErr<T, E>.

Is your proposal mainly about the aesthetics of the types (just one type with a parameter versus two types), or does it make client code nicer/easier in some way?

@aturon
Copy link
Member

aturon commented Jun 17, 2014

@bstrie How do you feel about the revised RFC, which brings the Result/Option APIs under a consistent umbrella while keeping almost all of the convenience methods intact?

@bstrie
Copy link
Contributor

bstrie commented Jun 17, 2014

I'm fine with this. Consistency is good. And if people decide that they need even more convenience methods in the future, we just need to be sure that they can be implemented sanely for both types.

@aturon
Copy link
Member

aturon commented Jun 17, 2014

I am in favor of this RFC, modulo one concern: I don't understand why the expect method is singled out and put into its own trait. My understanding was that eventually, with HKT, we could have an "OptionLike" trait encompassing the common interface. Why treat expect differently?

Also, minor note: the Result type already contains an unwrap method, but only when the error type is Show. The RFC lists it as an added method, and does not require Show. Do you plan to keep the current behavior, or change it?

`as_ref` and `as_mut` must create references for both `T` and `E` at once because `self` is taken by-reference
@brendanzab
Copy link
Member Author

The expect method requires std::Any, which is why it is currently in an extension trait in the std::option module. We could always make people do unwrap_or_else(|| fail!("...")), instead ofexpect("...")`, but that is a great deal more typing.

@brendanzab
Copy link
Member Author

Oh wait, Any is defined in core! 😅

@brendanzab
Copy link
Member Author

@aturon: re. unwrap, it seems like it would be hard to keep the current behavior of Result::unwrap, which prints out the error type on failure whilst keeping the type bounds consistent with Option :/

@brendanzab
Copy link
Member Author

Oh, it seems that the std::option::Expect trait was removed in rust-lang/rust#14610

This brings the RFC into line with rust-lang/rust#14610
@aturon
Copy link
Member

aturon commented Jun 19, 2014

@bjz I think it's fine to leave unwrap as-is: it will have the same signature across Result and Option, but will only be available on Result when the error type is Show.

I wonder whether for_err could just be err, since the old err method is being dropped? That would cut down some of the verbosity on error-related methods.

Also, to_option should probably be into_option.

@aturon
Copy link
Member

aturon commented Jun 19, 2014

@bjz Sorry, just noticed you suggested err in your alternatives section. Do you have an argument in favor of for_err instead?

@alexcrichton
Copy link
Member

We talked about this in person yesterday, but I wanted to write this down for posterity:

This proposal means that

foo.map_err(|e| /* ... */)

would become

foo.for_err().map(|e| /* ... */).to_result()

which I sadly find quite wordy :(

@alexcrichton
Copy link
Member

Or rather, you considered it a mild bug that ForErr::map returned Result instead of ForErr, and if it were to be fixed it would imply the above code.

@brendanzab
Copy link
Member Author

Thanks for writing these things down - I will update the PR accordingly today.

@aturon
Copy link
Member

aturon commented Jun 20, 2014

@alexcrichton FWIW, I think it's OK for the ForErr methods to yield Results, since (presumably?) most of the time you're doing a single error-biased operation. If you also make the biasing err rather than for_err, you get

foo.err().map(|e| /* ... */)

rather than today's

foo.map_err(|e| /* ... */)

which is not too bad.

@bjz I realized this morning that the semantics of methods like and and or on ForErr are not completely obvious. Can you spell them out?

@brendanzab
Copy link
Member Author

@aturon re. the semantics of and and or:

impl<T, E> Result<T, E> {
    pub fn and<U>(self, other: Result<U, E>) -> Result<U, E> {
        match self {
            Ok(_) => other,
            Err(e) => Err(e),
        }
    }

    pub fn or(self, other: Result<T, E>) -> Result<T, E> {
        match self {
            Ok(x) => Ok(x),
            Err(_) => other,
        }
    }
}

impl<T, E> ForErr<T, E> {
    pub fn and<F>(self, other: Result<T, F>) -> Result<T, F> {
        match self {
            ForErr(Err(_)) => other,
            ForErr(Ok(x)) => Ok(x),
        }
    }

    pub fn or(self, other: Result<T, E>) -> Result<T, E> {
        match self {
            ForErr(Err(e)) => Err(e),
            ForErr(Ok(_)) => other,
        }
    }
}

@aturon
Copy link
Member

aturon commented Jun 23, 2014

@bjz Thanks for the elaboration.

I have mixed feelings about these methods. The definitions you give here are somewhat symmetric to those in Result, except that they combine a ForErr with a Result and produce a Result. The outcome is that the names and and or here aren't terribly intuitive (to me at least).

Would it make sense to have them take ForErr as other instead? Are there other alternatives? Or can you give a good intuition for the definitions you gave?

Perhaps related: if you did eventually want to write a trait encompassing the Option/Result/ForErr APIs, the ForErr methods that currently take or return any Result values would instead need to take/return ForErr (which would be Self in the trait). But that comes at an ergonomic cost for the map method...

@brendanzab
Copy link
Member Author

Yeah, as we figured out the other day, for them to satisfy a truly common API the ForErr methods need to take and return ForErrs. This would result in more verbose error handling.

@aturon
Copy link
Member

aturon commented Jun 23, 2014

@bjz Right -- I wanted to get that in the public record :-)

But a broader point is, if we're not actually making ForErr match a "truly common" API, we should give careful thought to which forms of and/and_then/or/or_else are most intuitive and useful on ForErr. Can you give an argument for why the signatures/implementations you gave are the right ones?

@brendanzab
Copy link
Member Author

No reason - I think it was a brain-fart on my part :)

@bluss
Copy link
Member

bluss commented Jun 27, 2014

The proposal seems to add net complexity rather than remove it. Result gets new methods for iteration and slicing, and the result module a whole new type ForErr that users will be confronted with when reading the documentation.

I think -- my perspective is simpler API, simpler usage -- this proposal should be abandoned, take a step back, look at smaller things that can be adjusted in Result and Option -- for example removing .take_unwrap().

@nielsle
Copy link

nielsle commented Jul 11, 2014

Slightly orthogonal: It would be nice to have a simple method for converting an Option to a Result<T,E>.

pub fn ok_or<E>(self, f: || -> E) -> Result<T,E> { ... }

That would allow

let x: u32 = try!(from_str("25").ok_or(|| MyConversionError))

@brendanzab
Copy link
Member Author

Ok, @aturon and I are thinking that adding a ForErr adaptor struct is a big change, and I there are a number of issues with my proposed API for the type. That said, making the Result API consistent with Option is important, so @aturon will draw up a new RFC addressing that. map_err will remain in place for now. We can always tackle ForErr at a later date.

@aturon
Copy link
Member

aturon commented Aug 28, 2014

@bjz With the Option/Result stabilization that's about to land, I think we've covered the conservative version of the proposal here. Should probably close this RFC.

@pczarn
Copy link

pczarn commented Aug 28, 2014

I think the phantom type idea would easily work with HKT. That's the only benefit I see. (I'm surprised this RFC is still open)

@brendanzab
Copy link
Member Author

@aturon Ok, I'll close this now then!

@brendanzab brendanzab closed this Sep 1, 2014
withoutboats pushed a commit to withoutboats/rfcs that referenced this pull request Jan 15, 2017
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

Successfully merging this pull request may close these issues.