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

Fallible try_map on Option #1815

Open
golddranks opened this issue Dec 11, 2016 · 10 comments
Open

Fallible try_map on Option #1815

golddranks opened this issue Dec 11, 2016 · 10 comments
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.

Comments

@golddranks
Copy link

golddranks commented Dec 11, 2016

(This is a re-post of rust-lang/rust#38282, just in the correct place.) Using Option::map together with the ? operator is a pain. If you are mapping over an optional type, you can't use ? inside the closure to signal error. This means that it's often impractical to map functions that return Results over optional types. Here's a way to alleviate that:

item.explanation = item.explanation
    .and_then(|s| sanitize_links(&s).ok() ); // FIXME silently ignores errors

...but as noted in the comment, in the cases where the error matters, this is bad, and needs to be refactored into a match statement.

It would help the ergonomics, if Option<T> had a method – let's call it fallible_map EDIT: a better name was suggested by @killercup: try_map – like this:

try_map(self, FnOnce(T) → Result<U, E>) → Result<Option<U>, E>

What it would do, it would map the function over T, but wrap an Ok result into Option and return that Option wrapped into a Result. This would allow mapping fallible functions over Options:

item.explanation = item.explanation
    .try_map(|s| sanitize_links(&s))?;

...which, combined with ?, allows neat, fluid APIs while handling errors properly.

Does adding this kind of an API to Option need an RFC?

A simple implementation was demonstrated by @killercup here. As he mentioned, it could live in a 3rd party crate, but as a simple helper method that is in many ways similar to the already-existing Option::map, Option::and_then and others, helping with massaging the types in the right shape, I could easily imagine it being part of the standard API.

Note that alternative to this would be a method over Option<Result<T, E>> that would "flip" the types to Result<Option<T>, E>.

@golddranks golddranks changed the title Fallible map on Option Fallible try_map on Option Dec 11, 2016
@Stebalien
Copy link
Contributor

Note that alternative to this would be a method over Option<Result<T, E>> that would "flip" the types to Result<Option<T>, E>.

I've done this before and find it more generally useful (e.g., when using match statements). However, try_map would be useful as well so...

@golddranks
Copy link
Author

golddranks commented Dec 13, 2016

I was informed recently that this function is called traverse in Haskell. However, I think that the name try_map is easier to understand instinctively.

@Stebalien If there would be a method that flipped the types, what would you call it? I think that both would be useful.

@golddranks
Copy link
Author

golddranks commented Dec 14, 2016

I just published a try_map crate that implements try_map and flip methods. This is slightly different from @killercup's implementation, since that used references whereas this one uses values. (In accordance with the original map method. Values are more flexible anyway, since if you want to map over references, you can always call as_ref on the Option.)

https://github.com/golddranks/try_map

@killercup
Copy link
Member

killercup commented Dec 14, 2016 via email

@Stebalien
Copy link
Contributor

I was informed recently that this function is called traverse in Haskell.

traverse doesn't really mean anything to me but I can't think of a better name (I called it invert but that's no better).

@Ericson2314
Copy link
Contributor

Haskell's sequence :: Monad m => t (m a) -> m (t a) is probably closer to invert.

@golddranks
Copy link
Author

golddranks commented Dec 14, 2016

Just tried to implement try_map for Vec, but it seems that we need higher kinded types or associated type constructors to be able to express the trait generically. (Or then I just can't figure out how to express the types without being generic over type constructors.)

Curiously, the I didn't have any problem tweaking the trait that provides flip slightly to support generic containers – so I added support for the conversion Vec<Result<T>> → Result<Vec<T>>. Of course, there's multiple ways one could do such a conversion in, and each of them should deserve their own methods. I implemented simply a short-circuiting one that returns error when it encounters one inner error. However, such a thing might be out of scope of this discussion.

However, this brings up the following question: if some new helper methods would be added on Option, are their signatures compatible with the APIs we would introduce, when we possibly have some form of associated type constructors or HKTs in the future, or do we have to deprecate old ones when introducing new ones?

@golddranks
Copy link
Author

Btw. released a version 0.2 of try_map with the generalized FlipResultExt trait.

@golddranks
Copy link
Author

golddranks commented Dec 14, 2016

One more thing: obviously a flip for Vec is more of a curiosity, since it wouldn't perform too well. (Having to collect first time for map and then second time for flip.) Implementation for Iterator would be better, but that doesn't fit the signature of flip. I think that the ongoing discussion about short-circuiting iterators has more relevance to that.

@killercup
Copy link
Member

@golddranks I think you can change the trait to have an associated type Target which is returned by try_map. For Option, it'd be Result<Option<U>, E>, for Vec, Result<Vec<U>, E>, so you have (untested!):

impl<T, U, E> FallibleMapExt<T, U, E> for Vec<T> {
    type Target = Result<Vec<U>, E>;

    fn try_map<F>(self, f: F) -> Self::Target where
        F: FnOnce(T) -> Result<U, E>
    {
        let res = Vec::with_capacity(self.len());

        for item in self {
            res.push(f(item)?);
        }
        
        Ok(res)
    }
}

People should probably prefer vec.iter().map(…).collect::<Result<Vec<_>, Box<Error>>>() to get all the benefits of iterators, though.

@nrc nrc added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Dec 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

5 participants