-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Conversation
I'm sad that this appears to make error handling uniformly more verbose.
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(&[], |x| slice::ref_slice(x))</code> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Only a single trait will be required to implement the common methods. Separate implementations will be used for // 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 So in short, we're allowed to abstract over more complex types. |
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. |
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. |
There was a problem hiding this comment.
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.
This. The user doesn't care how nicely refactored the compiler internals are. |
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. |
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. |
Why not introduce some sugar for |
As I understand it, is the reason that we do not give special sugar for |
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 Here's an example using a fictitious 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 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 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 (You would still need to handle a fail case, otherwise it might fail for you) |
Is there a reason that we can't implement the |
I haven't put much thought about how this would all affect 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 |
I think it's worth separating out two aspects of this RFC:
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 Here are my thoughts on point 2:
|
@aturon Good points. Re. |
@bjz I don't have any data here, but my feeling is: they are simple and easy to understand, especially given the |
Ok, I restored the old iterator and slice conversion methods and mirrored them for |
I prefer the There's a way to change the type without discarding the other value.
Additionally, old methods
* Since |
So are you saying that you would have two phantom types, |
Yes, either those enums or |
They would have to be phantom types if they were going to reuse the same identifiers as the |
I'm in favor of this. Seems like a clean refactor to a consistent API. Consistency is king. |
@cmr Thoughts on the phantom type idea? |
The complexity does not pay for itself, I think. |
@pczarn Can you elaborate on what you see as the benefits of using phantom types here? It looks like 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? |
@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? |
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. |
I am in favor of this RFC, modulo one concern: I don't understand why the Also, minor note: the |
`as_ref` and `as_mut` must create references for both `T` and `E` at once because `self` is taken by-reference
The |
Oh wait, |
@aturon: re. |
Oh, it seems that the |
This brings the RFC into line with rust-lang/rust#14610
@bjz I think it's fine to leave I wonder whether Also, |
@bjz Sorry, just noticed you suggested |
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 :( |
Or rather, you considered it a mild bug that |
Thanks for writing these things down - I will update the PR accordingly today. |
@alexcrichton FWIW, I think it's OK for the 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 |
@aturon re. the semantics of 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,
}
}
} |
@bjz Thanks for the elaboration. I have mixed feelings about these methods. The definitions you give here are somewhat symmetric to those in Would it make sense to have them take Perhaps related: if you did eventually want to write a trait encompassing the |
Yeah, as we figured out the other day, for them to satisfy a truly common API the |
@bjz Right -- I wanted to get that in the public record :-) But a broader point is, if we're not actually making |
No reason - I think it was a brain-fart on my part :) |
The proposal seems to add net complexity rather than remove it. 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 |
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)) |
Ok, @aturon and I are thinking that adding a |
@bjz With the |
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) |
@aturon Ok, I'll close this now then! |
Rendered