Skip to content
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

Improve into_outcome w/ try! #597

Open
pjenvey opened this issue Apr 4, 2018 · 2 comments
Open

Improve into_outcome w/ try! #597

pjenvey opened this issue Apr 4, 2018 · 2 comments
Labels
suggestion A suggestion to change functionality

Comments

@pjenvey
Copy link

pjenvey commented Apr 4, 2018

It's common to convert between different Error types using From conversions (the popular error_chain and failure crates both promote this). Rust's try! automatically calls .into() to facilitate these conversions.

into_outcome is not uncommonly used w/ try! but when done so becomes a barrier from try! making the conversion. In this situation callers are forced to call .into() themselves manually, e.g.:

https://github.com/mozilla-services/megaphone/blob/98cf97b/src/http.rs#L46

into_outcome could and really should call .into() on Errors for us. A diff for a couple of the impls:

diff --git a/lib/src/data/from_data.rs b/lib/src/data/from_data.rs
index 22b9836..80d6e32 100644
--- a/lib/src/data/from_data.rs
+++ b/lib/src/data/from_data.rs
@@ -9,7 +9,9 @@ use data::Data;
 /// Type alias for the `Outcome` of a `FromData` conversion.
 pub type Outcome<S, E> = outcome::Outcome<S, (Status, E), Data>;
 
-impl<'a, S, E> IntoOutcome<S, (Status, E), Data> for Result<S, E> {
+impl<'a, S, E, EF> IntoOutcome<S, (Status, E), Data> for Result<S, EF>
+    where EF: Into<E>
+{
     type Failure = Status;
     type Forward = Data;
 
@@ -17,7 +19,7 @@ impl<'a, S, E> IntoOutcome<S, (Status, E), Data> for Result<S, E> {
     fn into_outcome(self, status: Status) -> Outcome<S, E> {
         match self {
             Ok(val) => Success(val),
-            Err(err) => Failure((status, err))
+            Err(err) => Failure((status, err.into()))
         }
     }
 
diff --git a/lib/src/request/from_request.rs b/lib/src/request/from_request.rs
index 4919216..336f978 100644
--- a/lib/src/request/from_request.rs
+++ b/lib/src/request/from_request.rs
@@ -12,7 +12,9 @@ use http::uri::URI;
 /// Type alias for the `Outcome` of a `FromRequest` conversion.
 pub type Outcome<S, E> = outcome::Outcome<S, (Status, E), ()>;
 
-impl<S, E> IntoOutcome<S, (Status, E), ()> for Result<S, E> {
+impl<S, E, EF> IntoOutcome<S, (Status, E), ()> for Result<S, EF>
+    where EF: Into<E>
+{
     type Failure = Status;
     type Forward = ();
 
@@ -20,7 +22,7 @@ impl<S, E> IntoOutcome<S, (Status, E), ()> for Result<S, E> {
     fn into_outcome(self, status: Status) -> Outcome<S, E> {
         match self {
             Ok(val) => Success(val),
-            Err(err) => Failure((status, err))
+            Err(err) => Failure((status, err.into()))
         }
     }
@SergioBenitez SergioBenitez added the suggestion A suggestion to change functionality label Apr 16, 2018
@SergioBenitez
Copy link
Member

SergioBenitez commented Apr 16, 2018

I agree we should do this, but I don't think this is the right place to fix it. Instead, since ? already calls From::from(e) for the error value e, we'd expect ? to do thing it always does (call into() on the error value), even in your code example. Unfortunate it doesn't because of how the Try trait is written. In short, for an Outcome<Success, Error, Forward>, instead of getting:

Try::from_error(From::from(v : Error))

We get:

Try::from_error(From::from(v : Result<Forward, Error>))

And thus .into() is called on the Result, not on the Error as you'd hope. The reason this happens is because we're forced to encode two failure states as one type (see rust-lang/rust#42327 (comment) for some more details on the issues and proposed solutions). Here, we've chosen Result.

One way to fix this, barring upstream changes to the Try trait (which seem inevitable), is to make the Err type of the Try implementation some custom type, say BadOutcome<F, E>. Then, if we add:

impl<F, T: From<F>, E> From<T> for BadOutcome<T, E>;
impl<E, T: From<E>, F> From<T> for BadOutcome<F, T>;

We'd presumably get the behavior we want. Except trait coherence prevents this and any other approach similar to this from working due to existing blanket impls in core. So it seems we'd need to wait for a more powerful Try trait.

@SergioBenitez
Copy link
Member

The recent try_v2 merge means that ? works as expected on 0.4. Unfortunately the API is still nightly only, but we'll be sure to get it in as soon as it's not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
suggestion A suggestion to change functionality
Projects
None yet
Development

No branches or pull requests

2 participants