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

Result::pass(), turning Result<T,E> into Result<U,F>, if From is set up #1996

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
@skade
Copy link
Contributor

skade commented May 9, 2017

IRLO discussion here.

Rendered

I would be willing to implement this or mentor an implementation.

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented May 9, 2017

FWIW, here's a stab at how the example might look like using ? and catch to do the conversions:

fn main() {
    let listener = TcpListener::bind("127.0.0.1:7878").unwrap();

    for stream in listener.incoming() {
        let res: Result<(), ServerError> = 
            do catch {
                handle(&mut stream?)
            };
        if let Err(e) = res {
            println!("Error occured: {:?}", e);
        }
    }
}

fn handle(stream: &mut TcpStream) -> Result<(), ServerError> {
    Ok(write!(stream, "hello!")?)
}

Or somewhat weird-looking but both shorter and stable:

let res = (|| handle(&mut stream?))();

Compilable: https://play.rust-lang.org/?gist=6877f6d66d7e1255797b8386c61ca96b&version=nightly&backtrace=1

(I don't have an opinion on the API right now, but since it's proposed to do a similar error conversion as ? I was curious how they'd compare. I think the proposed pass API would, like the catch above, be unable to infer the correct type for res in this example.)

@BurntSushi BurntSushi added the T-libs label May 9, 2017

@skade

This comment has been minimized.

Copy link
Contributor Author

skade commented May 9, 2017

    Ok(write!(stream, "hello!")?)

This is definitely convoluted and hard to explain. There, I would prefer:

write!(stream, "hello!").map_err(ServerError::from)

The pass API can be implemented as an external trait and does correctly infer.

pass (or similar) would allow chaining of Results in an easier fashion by providing the missing bridge between two result types.

https://is.gd/ddt2vr

I will extend the motivating example by adding more result cases in handle (which the original this RFC is derived from does) and provide a rewritten example in the design chapter.

Code added for completeness sake:

#![feature(catch_expr)]

use std::net::{TcpListener, TcpStream};
use std::io::Write;

#[derive(Debug)]
enum ServerError {
    IoError(std::io::Error),
    // some more cases
}

impl From<std::io::Error> for ServerError {
    fn from(e: std::io::Error) -> ServerError {
        ServerError::IoError(e)
    }
}


trait ResultExt<T, E> {
    fn pass<X, Y>(self) -> Result<X, Y> where T: Into<X>, E: Into<Y>;
}

impl<T,E> ResultExt<T, E> for Result<T, E> {
    fn pass<X, Y>(self) -> Result<X, Y> where T: Into<X>, E: Into<Y> {
        self.map(T::into).map_err(E::into)
    }
}


fn main() {
    let listener = TcpListener::bind("127.0.0.1:7878").unwrap();

    for stream in listener.incoming() {
        let res = stream.pass()
                        .and_then(|mut stream| handle(&mut stream) );

        if let Err(e) = res {
            println!("Error occured: {:?}", e);
        }
    }
}

fn handle(stream: &mut TcpStream) -> Result<(), ServerError> {
    write!(stream, "hello!").pass()
}
@sunjay

This comment has been minimized.

Copy link
Member

sunjay commented May 9, 2017

Interesting RFC! I think this would be a great conversion to have. It sounds like it would make it really easy to write code like this:

fn bar() -> Result<X, Y> {
    foo().pass()
}

Here's my question: If into could already accomplish what pass does, why do we need to add pass?

The RFC mentions that "This is more general, but possibly harder to discover, as the appropriate method to call would end up into()."

Except that's the whole point of having a general conversation trait. If someone knows that they can call into() to convert between different things, they only ever need to remember that one name.

We should be encouraging people to understand and use into instead of adding in more methods that do the same thing as that trait.

I think it would be better to avoid adding another method for people to remember and use into instead. If you're worried about discoverablity, we can add something to the module level docs. I don't think it's worth it to add another method here just for this.

I really like your idea of implementing that conversation. I just don't agree that adding another method called pass is a good way to implement it when we've already got an established trait for conversions.

@BurntSushi

This comment has been minimized.

Copy link
Member

BurntSushi commented May 9, 2017

@sunjay I suspect most folks would prefer it working with Into, but if you read the pre-RFC thread, it seems it might not be possible.

@sunjay

This comment has been minimized.

Copy link
Member

sunjay commented May 9, 2017

Thanks @BurntSushi!

@skade It may be worth adding something more to the RFC about why Into isn't possible.

@skade

This comment has been minimized.

Copy link
Contributor Author

skade commented May 9, 2017

The RFC mentions that "This is more general, but possibly harder to discover, as the appropriate method to call would end up into()."

My initial argument was that, you will have an already big API surface of result around handling Err and Ok (and_then, and, or, or_else...), but .into() would go into that huge blob of things that are convertible through From. On the other hand, people that want this API know From already.

The argument about collisions from the RFC thread seems more striking to me, though. I'll add it later.

@aturon aturon self-assigned this May 9, 2017

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented May 10, 2017

I want to make a sort of philosophical comment about using Into here:

Someday it will work (because we want intersection impl specialization as soon as its feasible to implement it), and the impl should exist so that you can use it generically.

However, I think there are many cases where creating an alias for a trait method that's an inherent method with a type specific name is just a good idea. An example of this that comes immediately to mind is the Extend trait. Extend covers the behaviors that in many other languages would be called String::concat, Vec::append. and HashMap::merge.

Its great that you can abstract over these fundamentally similar operations (cough cough monoid), but for a newer user who just wants to merge two hash maps together, Extend, with its highly generic API, is not at all discoverable. (They're looking for a function that takes another HashMap, its hard to figure out at first that extend takes an IntoIterator and HashMap implements IntoIterator so of course this does the exact same thing.)

I'm not sure pass is a perfect application of this principle, but I think we shouldn't shy away from providing both the highly generic impl and the type-specific alias for it.

@skade

This comment has been minimized.

Copy link
Contributor Author

skade commented May 10, 2017

I updated the RFC with the feedback from Twitter, here and IRLO.

Also, I found that From definitely does not work without specialization:

impl<T, E, U: From<T>, F: From<E>> From<Result<T, E>> for Result<U,F> {
    fn from(self) -> Result<U,E> {
        self.map(U::from).map_err(F::from)
    }
}

The reason is that - currently - From<T> for <T> already covers From<Result<_,_>> for Result<_,_> and would collide.

I also added "coalesce" as a name suggestion to the bikeshed.

@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented May 10, 2017

@withoutboats

OTOH, if you know that extend exists, you will probably use it rather than try to remember the type-specific method (similarly with flat_map vs. filter_map etc.). Especially with an "non-obvious" name such as pass.

@skade

This comment has been minimized.

Copy link
Contributor Author

skade commented May 10, 2017

@arielb1 The name is a stand-in, feel free to bikeshed.

I don't agree that people will use the more general name at any time, though, see the whole behaviour around (String::from, into and friends). They mostly follow a convention.

But, as written above, From isn't possible, currently, anyways.

@skade

This comment has been minimized.

Copy link
Contributor Author

skade commented May 11, 2017

I'm really struggling with the name. pass() feels a bit pythonic, but that doesn't extend to situations where this is not used on return values. I put forward(), and coalesce(), but I'm not happy with that either. Maybe one of the synonyms?

As a non-native speaker, I'm a bit at loss there :).

@burdges

This comment has been minimized.

Copy link

burdges commented May 11, 2017

I suppose into_either would be quite explicit, unless we get an Either type eventually. ;)

@phaazon

This comment has been minimized.

Copy link
Contributor

phaazon commented May 12, 2017

I think we’d also benefit from a map_both method, or something like that, which will be faster than map then map_err.

@alilleybrinker

This comment has been minimized.

Copy link

alilleybrinker commented May 12, 2017

So I don't think there's an existing name for anything like this in Haskell. I agree that pass is an odd name. Type wise, the function is doing something like Result<A, B> -> Result<C, D>, so it's a conversion. There's already the std::convert module. Maybe a name like convert or some synonym of it would be best (alter, adjust).

@phaazon

This comment has been minimized.

Copy link
Contributor

phaazon commented May 12, 2017

Yes, Haskell has it, and it’s called bimap – applied to Either, it becomes mapBoth.

See this

@skade

This comment has been minimized.

Copy link
Contributor Author

skade commented May 13, 2017

@phaazon I'm not sure if I agree with the need for a bimap method here.

res.map_err(function_one).map(function_two)

is fine, IMHO, unless the goal is chaining results together like the proposed method does. I just don't think passing functions in Rust is as ergonomic as in Haskell and that shows here.

@AndrewBrinker I always avoid very generic names like convert, but adjust sounds very close to home. I'll add both to the 🚲 🏡

@jan-hudec

This comment has been minimized.

Copy link

jan-hudec commented May 14, 2017

@withoutboats

However, I think there are many cases where creating an alias for a trait method that's an inherent method with a type specific name is just a good idea.

I disagree. When there is just one name, everybody will learn that one name, so everybody will know what it means when they read each others code. When there are type-specific aliases, especially aliases that are different for different types, most won't remember all of them and will have to look them up all the time. And remember, code is written once, but read many times.

An example of this that comes immediately to mind is the Extend trait. Extend covers the behaviors that in many other languages would be called String::concat, Vec::append. and HashMap::merge.

That would be one language I don't want to have anything to do with, because that would be just mess. Fortunately the one I usually use has append for all sequential containers and insert for all associative containers (it does have insert for sequences too, but that additionally takes a position). And even then its string is criticised for containing too many specific methods that could be generic functions (what would be traits in Rust).

Its great that you can abstract over these fundamentally similar operations (cough cough monoid), but for a newer user who just wants to merge two hash maps together, Extend, with its highly generic API, is not at all discoverable.

Yet, it is important that they discover Extend itself and not an alias, because once they find Extend, they can use it everywhere, while with a type-specific alias they'll need to dig through the documentation again tomorrow when they actually need to convert in the process.

This should be solved by improving the documentation, not adding methods.

@ciphergoth

This comment has been minimized.

Copy link
Contributor

ciphergoth commented Jun 28, 2017

Nitpick - don't forget to fill in the start date with 2017-05-09

@weargoggles

This comment has been minimized.

Copy link

weargoggles commented Aug 18, 2017

I'm having trouble tracking down the rationale for From's reflexive implementation. Can anyone point me at the conversion RFC? If that reflexive implementation could be removed, specialisation would not be necessary.

@weargoggles

This comment has been minimized.

Copy link

weargoggles commented Aug 18, 2017

Apologies. I've found the conversion traits RFC. It doesn't specify that there should be reflexive implementation of the trait, does anyone know if it's required elsewhere?
https://github.com/rust-lang/rfcs/blob/master/text/0529-conversion-traits.md

@skade

This comment has been minimized.

Copy link
Contributor Author

skade commented Aug 18, 2017

@weargoggles It's hinted at under the name of blanket implemenations : https://github.com/nox/rust-rfcs/blob/master/text/0529-conversion-traits.md#blanket-impls

Non-reflexive From reduces its usefulness a lot, it is often used in APIs where "T, or anything that can be converted to T" is wanted.

fn open<P: Into<Path>>(&P) -> Result<File, Error> {

}

It would be awkward if that function couldn't be called with a Path.

Also, in the context of this RFC, it means that you shouldn't call pass when Result<T, X> should be transformed to Result<T, E>.

@weargoggles

This comment has been minimized.

Copy link

weargoggles commented Aug 18, 2017

@skade Oh! I understand. Thanks for clearing that up.

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Aug 21, 2017

@weargoggles From is also used inside ?, where the T->T impl is what lets you ? an io::Result in a method that returns an io::Result (or any situation not requiring error conversion).

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Aug 22, 2017

The libs team discussed this RFC in our meeting today (sorry for the very long delay!).

The general feeling was that:

  • Everyone was in favor of the basic idea here, but felt far more comfortable with the conversion on the error component (which is already done with ?) than for the success component. Do you think there's a strong motivation on the success side?

  • Of course the above affects the naming question, but the libs team is also fine going to FCP and resolving the name at that point.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jan 26, 2018

I'd love to see this method added, I think it is useful.

But imo.. the names pass, adjust, and convert are all fairly uninformative and I think that has to be fixed at least prior to stabilization.

We could say: .bimap_into() and Haskellers will probably understand that as the composition of bimap and into, but it will not be generally understood.

Another idea is .map_both_into(), which is fairly descriptive, but also long and might thus lessen the benefit of the method.

Tho... After writing and reaching this point I've started to think that a more general name is a good thing because it is a very general operation. With that rationale, I think the best name that isn't .into() would be .mapped() which suggests that it has been mapped (i.e: you can't provide functions to it..) and that some form of conversion in the form of mapping is going on. We normally don't use past participles and prefer more imperative style "do this!"-style naming, but I think it might be suitable in this case.

@burdges

This comment has been minimized.

Copy link

burdges commented Jan 26, 2018

I feel the word map only confuses matters here, so .mapped() may suffer the same issues as pass, etc. I think .into_both() or .into_on_both() or .into_for_both() work because they talk about into, and they provide a convention for the Ok or Err variants if desired.

Right now, this reads .map(|x| x.into()).map_err(|y| y.into()) or maybe .map(Into::into).map_err(Into::into), just for comparison.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jan 27, 2018

@burdges I'd strike .into_both() because it suggests that you are getting back a product (both) instead of a sum, but if you reverse that as .both_into(), then I don't think that follows. The name .into_on_both() is good tho.

I'm not sure I get why mapping is wrong here tho if in the past tense - something has been (bi-)mapped, with Into::into. How about .mapped_into()?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Apr 19, 2018

This has been here for quite some time now and I'm not personally sold that the convenience here is worth it, so I'd like to propose that we close this RFC:

@rfcbot fcp close

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Apr 19, 2018

Team member @alexcrichton has proposed to close this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented May 2, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@golddranks

This comment has been minimized.

Copy link

golddranks commented May 8, 2018

I felt the need for this just today, while wrapping two things with similar interfaces into an enum. The things have similar but not the same return values: Result<State<ZipFile>, ZipError> and Result<State<GZipFile>, GZipError>, and the common interface is Result<State<File>, Error>. (I want to have runtime dispatch for a closed set of things while having the data inlined, which is the reason I'm not using traits.) When implementing a common interface for that enum, it would be very ergonomic if it sufficed to call just match the enum and call .into() for the results of the variants.

I also agree with @withoutboats that a separate method for doing the conversion wouldn't be too bad! I'm sad to see this proposal moving towards closing, especially that @withoutboats and @aturon who seemed to be positive towards this proposal haven't checked their boxes. At least I hope that we will revisit this when specialization allows From to be implemented for Result.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented May 12, 2018

The final comment period, with a disposition to close, as per the review above, is now complete.

By the power vested in me by Rust, I hereby close this RFC.

@rfcbot rfcbot added closed and removed final-comment-period labels May 12, 2018

@rfcbot rfcbot closed this May 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.