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

Tracking Issue for Result<Option> and Option<Result> Conversion #47338

Closed
cramertj opened this Issue Jan 10, 2018 · 24 comments

Comments

@cramertj
Copy link
Member

cramertj commented Jan 10, 2018

Initial implementation in #47193.

@epage

This comment has been minimized.

Copy link

epage commented Jan 25, 2018

A nice-to-have but not required would be someway for us to make this general so it can be used with other types, for example with Either<Result>.

I saw Into mentioned in #47193 and the challenge is ergonomics.

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Jan 25, 2018

Does it make sense to implement transpose for Result<Result<T, E1>, E2>?

@qnighy

This comment has been minimized.

Copy link
Contributor

qnighy commented Jan 25, 2018

Then perhaps Option<Option<T>>::transpose can also exist (maybe for consistency or for real use)? It's not an identity.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Feb 4, 2018

Nice addition 👍 The generalization of this, which we should experiment with when we get GATs, should just be sequenceA for any Traversable functor.

@clarfon

This comment has been minimized.

Copy link
Contributor

clarfon commented Mar 22, 2018

Conversions for Option<Option<T>> and Result<Result<T, E1>, E2> are just catch { val? } andcatch { Ok(val??) }, so, I'm not sure if they need dedicated functions.

@qnighy

This comment has been minimized.

Copy link
Contributor

qnighy commented Apr 18, 2018

Conversions for Option<Option<T>> and Result<Result<T, E1>, E2> are just catch { val? } and catch { Ok(val??) }

I don't think so. The code below doesn't compile.

#![feature(catch_expr)]

fn transpose<T, E1, E2>(x: Result<Result<T, E1>, E2>) -> Result<Result<T, E2>, E1> {
    do catch {
        Ok(x??)
    }
}
@clarfon

This comment has been minimized.

Copy link
Contributor

clarfon commented Apr 18, 2018

@qnighy that's because you're missing From<E1> for E2

@qnighy

This comment has been minimized.

Copy link
Contributor

qnighy commented May 21, 2018

@clarcharr perhaps are we talking about different functions? My understanding is that Result<Result<T, E1>, E2>::transpose is an involution. catch { Ok(val??) } isn't, as it transforms both Err(e) and Ok(Err(e)) into Err(e).

@goose121

This comment has been minimized.

Copy link

goose121 commented Oct 19, 2018

@clarcharr perhaps are we talking about different functions? My understanding is that Result<Result<T, E1>, E2>::transpose is an involution. catch { Ok(val??) } isn't, as it transforms both Err(e) and Ok(Err(e)) into Err(e).

Also, is it just me, or does catch { Ok(val??) } seem like a rather obscure expression? I feel like a Rust programmer coming across this who hasn't seen it before would be pretty confused.

@mexus

This comment has been minimized.

Copy link

mexus commented Dec 24, 2018

Hi everyone,

is there anything which prevents this useful feature from being stabilized? I've come here from a link from the docs about the transpose_result feature, but I can't really find any ongoing argument here. Could it be that there's another issue with discussions towards stabilization of the feature?

Thanks :)

@cramertj

This comment has been minimized.

Copy link
Member Author

cramertj commented Dec 26, 2018

Ping @rust-lang/libs this has been sitting around for a year now and I don't expect we'll see much more experimentation with it if folks have to rely on a nightly feature. What would y'all think about stabilizing?

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Dec 26, 2018

Sounds ok to me.

@rfcbot fcp merge

This issue tracks these methods:

impl<T, E> Option<Result<T, E>> {
    pub fn transpose(self) -> Result<Option<T>, E> {…}
}
impl<T, E> Result<Option<T>, E> {
   pub fn transpose(self) -> Option<Result<T, E>> {…}
}
@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Dec 26, 2018

Team member @SimonSapin has proposed to merge 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.

@traviscross

This comment has been minimized.

Copy link

traviscross commented Dec 30, 2018

This will be an improvement under any name, but here's an argument for stabilizing this as flip:

  • Unlike transpose and invert, flip doesn't have common meanings in mathematics and matrix algebra (including with SIMD) which would lead to confusion in such code.
  • flip is a more common English word than transpose. It feels less like jargon.
  • flip is 5 characters shorter without compromising clarity, and most inherent methods on Option, Result, and Iterator have short names.

The first point feels particularly compelling. This method will be used in all kinds of code. People reviewing code dealing with matrices will strongly expect transpose to have a particular meaning that this use would violate.

@clarfon

This comment has been minimized.

Copy link
Contributor

clarfon commented Dec 30, 2018

The term flip to me feels like you're converting Ok to Err, rather than swapping the order of a Result and Option. Same with invert.

@clarfon

This comment has been minimized.

Copy link
Contributor

clarfon commented Dec 30, 2018

Perhaps going with lift_option and lift_result might be better? To me, going from Result<Option<T>, E> to Option<Result<T, E>> is lifting the Option out, and the reverse is lifting the Result out. It feels way less like jargon while not being too verbose.

@traviscross

This comment has been minimized.

Copy link

traviscross commented Dec 30, 2018

Agreed those would be better than transpose. However, lift is usually associated with monadic operations, and you have to squint a bit to see it here. (Incidentally, Haskell calls this particular operation sequence.)

The semantic argument for flip is that we want this operation to "flip the type inside-out." However, I see where you're coming from in terms of reading this closer to "flipping a bit."

Perhaps we should consider the merits of evert. While it's not a common word, it is an old word and a short word. It has precisely the correct meaning, "to turn inside out." It's not a word we'd ever be tempted to use for another purpose. We wouldn't have to include the name of the resulting type in the method name. And it doesn't come with the kind of associations with other behavior that words like lift, transpose, and invert have. This wouldn't be the first time that technology rescued an old word from obscurity and gave it new life.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 2, 2019

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

@troiganto

This comment has been minimized.

Copy link

troiganto commented Jan 10, 2019

As someone who's already well aware of these two functions, I would not mind the name evert one bit.

However, it has one disadvantage that should be considered: people will likely have a hard time searching for functions named evert in the standard library. If they proceed to implement them themselves and only later find out about them, I'm very sure they will blame it on the functions' obscure name.

Maybe it might be better to pick a slightly ambiguous name like transpose or flip just for the sake of discoverability. Because these functions are only defined on two very narrowly defined types (Option<Result<T, E>> and Result<Option<T>, E>), I think a mix-up with functions defined e.g. on matrices seems unlikely to me.

@epage

This comment has been minimized.

Copy link

epage commented Jan 10, 2019

RE evert

My main concern is discoverability. Once you get past the "how will people think to see if this exists" (maybe a help in a compiler error?), there is the "how will it stand out".

Personally, I gloss over weird names in libraries (e.g. iota in C++) and I think evert would fall in that category. No idea if this is because I assume they are for esoteric advanced stuff, an inherent blindspot of ignorance, or something else. I assume other people, especially new users, would do the same.

@8573

This comment has been minimized.

Copy link

8573 commented Jan 10, 2019

For discoverability, could the documentation for the option and result modules, and maybe even the Book, have sections on or examples of the use of evert?

@traviscross

This comment has been minimized.

Copy link

traviscross commented Jan 11, 2019

Regarding discoverability, it seems unlikely that anyone will first find these methods by name regardless of what we call them. There's just not a common name for this operation.

People will find these 1) by searching the web for "convert Rust Result Option to Option Result" or "chain Rust Result methods with Option" or other variations, 2) by scanning the Option or Result documentation for a method of the correct signature, or 3) by being introduced to the concept through the book, through blogs, or through seeing discussions such as this one.

Regarding matrices, as soon as you have a function that returns Option<&Matrix> and you use the Result type for error handling, there's every likelihood of having code that will intermix evert/flip/transpose with the matrix transpose, invert, etc. operations.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 12, 2019

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

@Centril Centril self-assigned this Jan 13, 2019

bors added a commit that referenced this issue Jan 13, 2019

Auto merge of #57567 - Centril:stabilize-transpose, r=alexreg
Stabilize `transpose_result` in 1.33

fixes #47338.

FCP completed: #47338 (comment)

r? @alexreg

@bors bors closed this in #57567 Jan 13, 2019

@Stargateur

This comment has been minimized.

Copy link

Stargateur commented Feb 1, 2019

Someone proposed to have something close to Hoogle for search this kind of feature.

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.