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

Carrier trait for ? operator #1718

Closed
nrc opened this issue Aug 18, 2016 · 96 comments
Closed

Carrier trait for ? operator #1718

nrc opened this issue Aug 18, 2016 · 96 comments
Labels
T-lang Relevant to the language team, which will review and decide on the RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC.

Comments

@nrc
Copy link
Member

nrc commented Aug 18, 2016

We accepted an RFC to add the ? operator, that RFC included a Carrier trait to allow applying ? to types other than Result, e.g., Option. However, we accepted a version of the RFC without that in order to get some actual movement and some experience with the ? operator.

The bare ? operator is now on the road to stabilisation. We would like to consider adding a Carrier trait and that will require a new RFC. There is in fact a 'dummy' Carrier trait in libcore and used in the implementation of ?. However, its purpose is to ensure ? is backwards compatible around type inference. It is effectively only implemented for Result and is not intended to be a long-term solution. There are other options for the trait using HKT, variant types, and just existing features. It's unclear what is preferred right now.

One important question is whether we should allow conversion between types (e.g., Result to Option) or whether we should only allow the ? operator to work on one type 'at a time'.

Links:

? operator RFC PR
? operator tracking issue
Niko's thoughts

@Ericson2314
Copy link
Contributor

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Aug 18, 2016

I am still a fan of @Stebalien's variant on my original proposal:

enum Carrier<C, R> {
    Continue(C),
    Return(R),
}
trait IntoCarrier<Return> {
    type Continue;
    fn into_carrier(self) -> Carrier<Self::Continue, Return>;
}

Coupled with the "design guidelines" that one should only use the IntoCarrier trait to convert within one set of types. So for example, these would be good impls:

impl<T,U,E,F> IntoCarrier<Result<U,F>> for Result<T,E> where E: Into<F> {
    type Continue = T;
} // good
impl<T> IntoCarrier<Option<T>> for Option<T> {
    type Continue = T;
} // good

But this would be considered an anti-pattern:

impl<T> IntoCarrier<T, Result<T,()>> for Option<T> { } // bad

@mitsuhiko
Copy link
Contributor

I really like this carrier proposal and I would vote in favor of supporting a carrier for option.

One thing that I keep running into as a weirder example currently is the common case of a result being wrapped in an option. This is something that shows up in the context of iterators frequently.

A good example is the walkdir crate. From what I can tell Rust could already express a generic implementation for that case however I wanted to raise this as something to consider.

The case I can see third party libraries implement is a conversion from future to result. What I like about the IntoCarrier is that - if I understand it correctly - it could be implemented both ways by a third party crate: future to result and result to future.

@mitsuhiko
Copy link
Contributor

I implemented a few variants of carrier here to see how it works: https://gist.github.com/mitsuhiko/51177b5bf1ebe81cfdd36946a249bba3

I really like that this would permit error handling within iterators. It's much better than what I did in the past where I had to make a manual iter_try! macro. What do others feel about this?

@nikomatsakis
Copy link
Contributor

@mitsuhiko ah, that's a nice idea to prototype it like that. I also did some experiments in play around type inference and collect -- it seemed like all the types were inferring just as I wanted them, but I feel like I have to do some experimenting to make sure I was hitting the problem cases.

The idea of 'throwing' a result error into Option<Result<...>> is an interesting one! I have definitely been in this situation before and it is annoying; it'd be very nice for ? to just handle it for you. It seems to be kind of skirting right along the line of interconvering between types -- you're introducing a Some, but you're preserving the notion that a Err(x) translates to an Err(x). It's pretty hard to imagine wanting to map Err to None.

I think we will certainly see some other cases where you want to interconvert between "related" types or patterns that have a known semantic meaning.

I also expect that we will get a certain amount of pressure to interconvert Option<T> and Result<T,()> but I am still in favor of resisting that pressure in the general case. :)

@mitsuhiko
Copy link
Contributor

mitsuhiko commented Aug 19, 2016

I don't want to start a bikeshed here but I want to throw up some ideas for making the carrier a bit less confusing to people. The carrier primarily exists internally but I assume the docs will sooner or later explain ? and error handling and will have to mention it.

Right now it's Carrier, Carrier::Continue and Carrier::Return conceptionally.

What about it being called a CompletionRecord and the two enums are CompletionRecord::Value (Could also be CompletionRecord::NormalCompletion) and CompletionRecord::Abrupt (Could also be CompletionRecord::EarlyReturn)? These are inspired by the internal values in the JavaScript spec and I think they make things easier to explain and do not overload already existing words. In particular Carrier is super abstract and hard to explain and Carrier::Continue and Carrier::Return for me imply in a way that they would have something to do with the keywords of the same name. Gets especially confusing because Continue holds an unwrapped value and Return a result like object.

I also expect that we will get a certain amount of pressure to interconvert Option<T> and Result<T,()> but I am still in favor of resisting that pressure in the general case. :)

Huge plus + 1 on not adding this. I was originally on the side of supporting this but the more I play around with this and error_chain the more I'm convinced that this will be a source of confusion. It's already easy to foo().or_ok(Err(...))? it.

@seanmonstar
Copy link
Contributor

I also find the Carrier name undesirable.

On Fri, Aug 19, 2016, 8:26 AM Armin Ronacher notifications@github.com
wrote:

I don't want to start a bikeshed here but I want to throw up some ideas
for making the carrier a bit less confusing to people. The carrier
primarily exists internally but I assume the docs will sooner or later
explain ? and error handling and will have to mention it.

Right now it's Carrier, Carrier::Continue and Carrier::Return
conceptionally.

What about it being called a CompletionRecord and the two enums are
CompletionRecord::Value and CompletionRecord::Abrupt? These are inspired
by the internal values in the JavaScript spec and I think they make things
easier to explain and do not overload already existing words. In particular
Carrier is super abstract and hard to explain and Carrier::Continue and
Carrier::Return for me imply in a way that they would have something to
do with the keywords of the same name. Gets especially confusing because
Continue holds an unwrapped value and Return a result like object.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#1718 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADJF_DJw5VfknCMDUsQW0mRuy1xAYZVks5qhcsxgaJpZM4JnHoi
.

@nikomatsakis
Copy link
Contributor

I dislike the name Carrier, I've just been trying to focus on the core capabilities. I don't find CompletionRecord very intuitive but I like Normal and Abrupt -- I think.

@Stebalien
Copy link
Contributor

Stebalien commented Aug 19, 2016

Joining everyone in the bikeshed, I agree that Carrier is confusing (I actually have no idea what it means). What about just naming it after the operator: QuestionMark. Not a good name but it's unlikely to confuse anyone.

Also I'm not a fan of Abrupt/Normal because they impose meaning on the operations. There's no reason that an early return is necessarily abrupt or continuing on is necessarily normal. This is why I initially objected to using a Result.

Carrier::Return(X) literally means "please return X immediately" so, IMO, the name is correct. However, EarlyReturn is arguably more accurate.

Carrier::Continue(X) is a bit unfortunate as it has nothing to do with loops (my intention was "please continue with value X". Maybe Proceed? Value (not a verb...)? I don't really know.

@ticki
Copy link
Contributor

ticki commented Aug 19, 2016

@nikomatsakis @Stebalien I don't think a Carrier enum is necessary. Result has essentially the same meaning (one is successful, another is an error/early).

@Stebalien
Copy link
Contributor

@ticki But it's not about success/failure. For example, an iterator returning None isn't an error; it just means that it's done.

@mitsuhiko
Copy link
Contributor

@nikomatsakis the main argument for CompletionRecord would be that it has some equivalent elsewhere we can use and help to establish as a term. Personally I would like the name Outcome for the type.

@ticki overloading result is not great, because it overloads the type a lot with API that users should not be exposed to. I already fear that someone going to the result docs is overwhelmed by how much API there is. (Also it would get super confusing if error handling in iterators is considered like I had in my gist)

@joshtriplett
Copy link
Member

@mitsuhiko "Outcome" seems too much like "Result".

More generally, though, I think we should avoid bikeshedding the name until we agree on the semantics. Once we define the semantics, we'll have a better idea of the right name for them.

@joshtriplett
Copy link
Member

@nikomatsakis @Stebalien Is the intent of the separate IntoCarrier trait (rather than using Into/From) to limit the type conversions to those explicitly intended for use with ?, rather than all conversions supported with Into?

@Ericson2314
Copy link
Contributor

Ericson2314 commented Aug 20, 2016

Repeating myself from the internals thread: I now think full Monad support is less challenging than it looks, and am wary about stabilizing any trait for this before HKTs lands.

@eddyb
Copy link
Member

eddyb commented Aug 20, 2016

@Ericson2314 As long as it doesn't involve closures, I agree that something more like Monad is worth investigating. HKT may not be necessary, at least not to prototype.

@ExpHP
Copy link

ExpHP commented Aug 20, 2016

@Ericson2314: I am intrigued and surprised, as up until this point I was not aware that there was any ongoing development (in the language or elsewhere) that would bring the language closer towards HKTs. Hopefully without derailing the thread too much, is there a place where one can read more about this?

@Ericson2314
Copy link
Contributor

@eddyb It's still my closure plan :). Adapting https://internals.rust-lang.org/t/autoclosures-via-swift/3616/52:

This

let x = do {
   loop { 
       let y = monadic_action()?;
        if !is_ok(y) {
            break Monad::return(None);
        }
       normal_action();
    }
};

get's desugared into

let x = {
    let edge0 = late |()| monadic_action().bind(edge1);
    let edge1 = late |temp0| {
        let y = temp0;
        if !is_ok(y) { 
            drop(y); 
            Monad::return(None) 
        } else {
            normal_action();
            drop(y);
            Monad::return(()).bind(edge0)
        }
    }; 
    edge0(())
};

The late modifier indicates that the closure is constructed as late as possible, i.e. before it is eliminated by being called or passed somewhere else. This avoids the cyclic borrowing or moving that would ordinarily render this code invalid. It doesn't avoid the fact that different closures bind different values. For a first attempt, https://github.com/thepowersgang/stack_dst-rs could do the trick.


In any event others have ideas in this arena, and we already want to make nice "synchronous" sugar for futures-rs. I believe the future monad has a sufficiently complex representation that anything that works for it ought to generalize for monads in general.

@comex
Copy link

comex commented Aug 21, 2016

@Ericson2314 FWIW, LLVM is getting native support for coroutines, and monadic do can be implemented as a wrapper around that, which may or may not be more efficient - it constructs one big function for the whole coroutine (do block) with an entry point that jumps to the appropriate place based on the current state, rather than splitting it across multiple closures. The biggest issue - and one not covered by the future monad, incidentally, or by your desugaring per se - is copying the function state in order to call the callback passed to bind more than once, if the original code uses variables of non-Copy type. This is doable but requires some thought, e.g. it would require special casing the Clone trait.

@eddyb
Copy link
Member

eddyb commented Aug 21, 2016

@comex IMO LLVM's coroutines are the completely wrong level for Rust, they fundamentally rely on LLVM's ability to optimize out heap allocations. We can do much better on the MIR and with ADTs.

@ahmedcharles
Copy link

Niko's examples of how to use IntoCarrier didn't seem to work, so I figured I'd try implementing it:

enum Carrier<C, R> {
    Continue(C),
    Return(R),
}

trait IntoCarrier<Return> {
    type Continue;
    fn into_carrier(self) -> Carrier<Self::Continue, Return>;
}

// impl for Result
impl<T, E, F> IntoCarrier<Result<T, F>> for Result<T, E> where E: Into<F> {
    type Continue = T;
    fn into_carrier(self) -> Carrier<Self::Continue, Result<T, F>> {
        match self {
            Ok(v) => Carrier::Continue(v),
            Err(e) => Carrier::Return(Err(e.into())),
        }
    }
}

// impl for Option
impl<T> IntoCarrier<Option<T>> for Option<T> {
    type Continue = T;
    fn into_carrier(self) -> Carrier<Self::Continue, Option<T>> {
        match self {
            Some(s) => Carrier::Continue(s),
            None => Carrier::Return(None),
        }
    }
}

// impl for bool (just because)
impl IntoCarrier<bool> for bool {
    type Continue = bool;
    fn into_carrier(self) -> Carrier<Self::Continue, bool> {
        if self { Carrier::Continue(self) } else { Carrier::Return(self) }
    }
}

// anti-pattern
impl<T> IntoCarrier<Result<T, ()>> for Option<T> {
    type Continue = T;
    fn into_carrier(self) -> Carrier<Self::Continue, Result<T, ()>> {
        match self {
            Some(s) => Carrier::Continue(s),
            None => Carrier::Return(Err(())),
        }
    }
}

macro_rules! t {
    ($expr:expr) => (match IntoCarrier::into_carrier($expr) {
        Carrier::Continue(v) => v,
        Carrier::Return(e) => return e,
    })
}

fn to_result(ok: bool, v: i32, e: u32) -> Result<i32, u32> {
    if ok { Ok(v) } else { Err(e) }
}

fn test_result(ok: bool) -> Result<i32, u32> {
    let i: i32 = t!(to_result(ok, -1, 1));
    Ok(i)
}

fn to_option(some: bool, v: i32) -> Option<i32> {
    if some { Some(v) } else { None }
}

fn test_option(some: bool) -> Option<i32> {
    let i: i32 = t!(to_option(some, -1));
    Some(i)
}

fn to_bool(b: bool) -> bool { b }

fn test_bool(b: bool) -> bool {
    let i: bool = t!(to_bool(b));
    i
}

fn to_antipattern(some: bool, v: i32) -> Option<i32> {
    if some { Some(v) } else { None }
}

fn test_antipattern(ok: bool) -> Result<i32, ()> {
    let i: i32 = t!(to_antipattern(ok, -1));
    Ok(i)
}

fn main() {
    assert_eq!(test_result(true), Ok(-1i32));
    assert_eq!(test_result(false), Err(1u32));
    assert_eq!(test_option(true), Some(-1i32));
    assert_eq!(test_option(false), None);
    assert_eq!(test_bool(true), true);
    assert_eq!(test_bool(false), false);
    assert_eq!(test_antipattern(true), Ok(-1i32));
    assert_eq!(test_antipattern(false), Err(()));
}

@comex
Copy link

comex commented Aug 21, 2016

@eddyb hmm, I didn't pay much attention to the allocation stuff when I was skimming the coroutine docs. I guess that's a poor match as is... and I can see why changing it would be hard. Even so, fundamentally, I'm skeptical that the best design is one that takes away much of the LLVM optimizer's knowledge of the control flow graph, if there is an alternative that does not. MIR optimizations are nice but they aren't everything.

@eddyb
Copy link
Member

eddyb commented Aug 21, 2016

@comex Being skeptical is good 😄. I honestly don't know yet just how far we can take it, experiments are most definitely needed before we can settle on one mechanism - maybe neither is as great on their own as would be a hybrid, who knows? I only have opinions and speculations so far, no hard data yet.

bors added a commit to rust-lang/rust that referenced this issue Aug 21, 2016
Carrier trait (third attempt)

This adds a `Carrier` trait to operate with `?`. The only public implementation is for `Result`, so effectively the trait does not exist, however, it ensures future compatibility for the `?` operator. This is not intended to be used, nor is it intended to be a long-term solution.

Although this exact PR has not been through Crater, I do not expect it to be a breaking change based on putting numerous similar PRs though Crater in the past.

cc:
* [? tracking issue](#31436)
* [previous PR](#35056)
* [RFC issue](rust-lang/rfcs#1718) for discussion of long-term Carrier trait solutions.

r? @nikomatsakis
@mitsuhiko
Copy link
Contributor

mitsuhiko commented Aug 23, 2016

I started a repository here to explore with the carriers more: https://github.com/mitsuhiko/rust-carrier

Primarily a few things I noticed so far:

  • it's really hard to explain in documentation with the current names I have chosen but still slightly easier than Carrier.
  • Option<U> -> Option<U> is useless, but Option<U> to Option<V> is useful.
  • The support for iterators is really useful. I tried it in a project of mine and it's great!
  • Custom carriers are going to be useful for quite a few cases. In particular there are often complex objects such as HTTP responses which hold a lot of information and upon closer introspection one could consider such an object to be in the state of failure. For instance an HTTP response object that has 404 as status code might be interesting to convert in some situations into an HttpError. Implementing carriers for this is a better place than a general Into.
  • I played around with implementing carriers from *const T into Option<U> where the null pointer is converted into None. This actually is a lot more useful than I thought it would be based on the limited amount of testing I did on it so far.

WRT the Monad discussion: I cannot express enough how I would like any HKT or Monad discussion not to take place here. The concept is already hard enough and it should be used by as many users as possible. The code examples shown here demonstrate quite well the problems this will cause for actually explaining the error handling to mere mortals.

@est31
Copy link
Member

est31 commented Aug 23, 2016

I like the term EarlyReturn the best. Return can be mixed with the normal return feature of functions, and Abrupt can be mixed with abort and maybe even panics.

@norru
Copy link

norru commented Jan 4, 2017

Who is eligible to vote for the trait's name? If it matters, I like Try::try() for all the reasons stated by others in previous comments.

@nikomatsakis
Copy link
Contributor

I'll try to draw up this RFC ASAP. I'm OK with either Try::try or QuestionMark::ask (or QuestionMark::try, really). Ultimately this decision can get hashed out on the RFC thread or settled by the lang/libs teams.

@nikomatsakis
Copy link
Contributor

Actually, if anyone is interested in working on the RFC jointly, please contact me! This seems like a good opportunity to get into the RFC process.

@Wyverald
Copy link

Wyverald commented Jan 8, 2017

Shouldn't the T and E in Carrier/QuestionMark/Try should be associated types instead of type parameters? To me it hardly makes any sense for some type to implement both Carrier<Foo> and Carrier<Bar>.

@burdges
Copy link

burdges commented Jan 8, 2017

Assuming this remains the proposal, Carrier<C,R> is a struct type parameters are the only type variable available, while IntoCarrier<Return> is the trait for Option, Result, etc. IntoCarrier<Return> has an associated type for Continue, but Return must be a type parameter because many different error types convert into one error type using the E: Into<F> in

impl<T,U,E,F> IntoCarrier<Result<U,F>> for Result<T,E> where E: Into<F> { .. }

I'd imagine all Carrier<C,R> values get consumed immediately, so you should never see them in practice.

@bluss
Copy link
Member

bluss commented Jan 8, 2017

This #1718 (comment) is the most recent proposal.

@burdges
Copy link

burdges commented Jan 8, 2017

I see, so the trait normally being used is QuestionMark<T,Result<T, E>> with ask returning Result<T, Result<T,E>>. It still lets ask do return Ok(T), which sounds good for state machines, complex error scenarios, etc.

Why does it "not work so well with the existing inference"? Are issues with these generic impls?

impl<T,U,E,F> QuestionMark<T,Result<U,F>> for Result<T,E> where E: Into<F> {
    ask(self) -> Result<T, Option<U>> {
        match self {
            Ok(t) => Ok(t),
            Err(e) => Err(Err(e.into())),
        }
    }
}

impl<T,U> QuestionMark<T,Option<U>> for Option<T> {
    ask(self) -> Result<T, Option<U>> {
        match self {
            Some(x) => Ok(x),
            None => Err(()),
        }
    }
}

Or simply that error forgetting impls cannot be permitted because they cannot be controlled?

impl<T> QuestionMark<T,()> for Option<T> {
    ask(self) -> Result<T,()> {
        match self {
            Some(x) => Ok(x),
            None => Err(()),
        }
    }
}

/// Looks safer than `Into<()>`.
trait IgnoreError {
    ignore_error(self) { }
}

impl<T,E,F> QuestionMark<T,()> for Result<T,E> where E: IgnoreError {
    ask(self) -> Result<T, ()> {
        match self {
            Ok(t) => Ok(t),
            Err(e) => { ignore_error(e); Err(()) },
        }
    }
}

@mglagla
Copy link
Contributor

mglagla commented Jan 12, 2017

Copy-pasting my response from the internals since I didn't see this thread:

I'm against QuestionMark for the operator name.
It's inconsistent with the names for other operators, e.g. Not,Add, Div instead of ExclamationMark, Plus, Minus.
They all have been given names for their intended uses instead of the literal name of the symbol.
Also if, in the future of Rust, we want to add another operator that also uses ? somehow in a different context, then the name "question mark" would carry meaning for two different operators.

Note that this is already the case: * is both used for Mul and Deref, - is both used for Sub and Neg.

Seeing as ? replaces the try! macro, Try seems to me a sensible choice. Or at the very least, some name that makes its intended use obvious.

@shepmaster
Copy link
Member

shepmaster commented Jan 15, 2017

The most recent suggestion appears to work with my main use case, so I am excited to see the RFC!

#[derive(Debug)]
struct Status<L, T, F> {
    location: L,
    kind: StatusKind<T, F>,
}

#[derive(Debug)]
enum StatusKind<T, F> {
    Success(T),
    Failure(F),
}

Using the return type of Result is unfortunate from my point of view. While not quite as strange as the usage by []::binary_search, it does feel a bit like we are using it because it's a generic container of either of two types because the standard library doesn't have something like the either crate. However, since the trait in question will be so highly tied to error handling, it's not a far jump. It's mostly the fact that ask returning Err is not a failure, which is usually the case for Result-bearing methods.

@seanmonstar
Copy link
Contributor

I brought this up in the futures crate, but the design also seems relevant for here: considering the pattern from futures of having Result<Async<T>, E>, and you only want the value of Ok(Async::Ready(val)), otherwise you wish to return the result immediately.

The futures trait could possibly change such that Async::NotReady is an Err case, in which case the most recent proposal would just work out of the box. It seems to me that that question is tricky to answer.

An alternative idea is for this trait to not return Result<T, E>. There are two distinct ideas here: 1) is this a value I can use right now, or should I just return the whole thing, and 2) the result of this action was an error, boom.

In the futures case, there is desire to return when the value Ok(Async::NotReady), and it wouldn't make sense to wrap that in an Err (Err(Ok(Async::NotReady))?). Instead, we really do want to describe that "this is a thing you can use now, and that is a thing to just return immediately". I don't yet care too much about the naming of that type, but if we changed the trait and de-sugaring to something like the following, then the futures case could work also:

trait Try<C, R> {
    fn try(self) -> Tried<C, R>; 
}

An implementation for the futures crate:

impl<C, R> Try<C, Poll<C, R>> for Poll<C, R> {
    fn try(self) -> Tried<C, Poll<C, R>> {
        match self {
            Ok(Async::Ready(val)) => Tried::Continue(val),
            other => Tried::Return(other),
        }
    }
}

De-sugaring of let val = self.inner.poll()?:

let val = match Try::try(self.inner.poll()) {
    Tried::Continue(val) => val,
    Tried::Return(val) => return val, // or catch { ... }
}

@nikomatsakis
Copy link
Contributor

Using the return type of Result is unfortunate from my point of view. While not quite as strange as the usage by []::binary_search, it does feel a bit like we are using it because it's a generic container of either of two types because the standard library doesn't have something like the either crate

I actually find Result to be a reasonable match. The method tests whether to continue: Ok signals "yes, continue, the result is ok". Err signals "no, do not continue, return this value". This is sort of a "meta-result", but the semantics feel like a reasonable match.

@nikomatsakis
Copy link
Contributor

@seanmonstar It seems to me that the futures crate would indeed be better off defining its own type, rather than using an actual Result, if we wanted to use ? this way.

@alexcrichton
Copy link
Member

Note that the reason futures return Result<Async<T>, E> is to work with try! by default. If enum Poll worked with ? by default then I think we could definitely change how this is done in the 0.2 release.

@nikomatsakis
Copy link
Contributor

Here is a draft of the ? RFC: https://gist.github.com/nikomatsakis/90b00b5017ffe3fa5c33628cd01b7507

@withoutboats
Copy link
Contributor

Poll could also just be a typedef for Async<Result<T, E>> and that could implement QuestionMark/Try.

@alexcrichton
Copy link
Member

@nikomatsakis oh some interesting thoughts:

we do not want to allow x? to be used in a function that return a Result<T, E> or a Poll<T, E> (from the futures example).

I forgot a case actually but we actually did want precisely this. When implementing Future where your error type is io::Error and you're doing I/O locally, it's quite convenient to use try! to get rid of the error on the I/O operation quickly.

Basically I forgot that Result<Async<T>, E> was chosen over enum Poll<T, E> not only b/c you could try! just the error away easily but also if you executed a fallible operation locally inside of an implementation of a future you could try it away easily as well.

All that's just to say that this may not be the best example, and does kinda throw doubt in my mind as to whether we'd switch to enum Poll<T, E> for futures if it didn't work with Result<T, E> (something we'd have to discuss ourselves, separately, of course).

@withoutboats
Copy link
Contributor

@alexcrichton To be clear, the impl you want is this? QuestionMark<Poll<T, E>> for Result<T, E>

That is indeed a violation of the orphan rules (right now at least).

@alexcrichton
Copy link
Member

@withoutboats morally, yeah that's what we'd want. One of the current motivations for type Poll<T, E> = Result<Async<T>, E> was that you could try! a Result<T, E> into a Poll<T, E>.

We may not want to put too much weight on the design of futures today, though. We haven't really exhaustively explored our options here so this may just be too much in the realm of "futures specific problems".

@mitsuhiko
Copy link
Contributor

I think the RFC needs an explanation of why this uses Result over a custom enum. It's unclear to me what value this system gets out of reusing Result.

@withoutboats
Copy link
Contributor

@alexcrichton Yea I don't think this trait should change because of that issue with futures! But I do think its an interesting example of the orphan rules being more restrictive than we'd like, & (unlike many examples) its a way I think we could consider changing.

That example is disallowed to allow std to impl QuestionMark<U> for Result<T, E>. However, if & when specialization is stabilized, we could relax the orphan rule on the basis that that more generic impl can be added as long as it is default.

@nikomatsakis
Copy link
Contributor

@alexcrichton @withoutboats

Hmm, so the current setup makes the Self type be the value to which ? is applied. This seems natural and it means that if you define a new type (e.g., Poll) you can define the return types to which it can be mapped. But of course you want the opposite, it sounds like. =)

I agree with @withoutboats that this is a shortcoming of the orphan rules. In particular, if the types were not generic, you actually could do impl Try<Poll> for Result. I also agree that specialization may let us side-step this problem, though not the version we have currently implemented. It may also be worth trying to improve the rules themselves, though I'm not sure how we could do it without some form of annotation.

@nikomatsakis
Copy link
Contributor

@mitsuhiko

I think the RFC needs an explanation of why this uses Result over a custom enum. It's unclear to me what value this system gets out of reusing Result.

I'll add some text. To be honest, I don't care too much one way or the other about this specific point.

@nikomatsakis
Copy link
Contributor

Updated draft RFC slightly (same url).

I was just re-reading it though, and I realize that in the RFC text itself I give an example of converting a Result<X, HttpError> into a HttpResponse, which of course won't work well with the orphan rules -- that is, it's the same as @alexcrichton's example of converting a io::Result<T> into a Poll<T, io::Error>. Annoying.

Are there any use-cases for going the other way that we know of? (i.e., converting from a Poll<T, io::Error> into a io::Result<T>)? Maybe we should switch the order of the trait parameters (while still pursuing improvements to specialization and/or the orphan rules). I can at least add it as an unresolved question. (But the resulting signatures do seem very unnatural.)

@alexcrichton
Copy link
Member

@nikomatsakis looks good to me!

@nikomatsakis
Copy link
Contributor

Posted as #1859.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-lang Relevant to the language team, which will review and decide on the RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

No branches or pull requests