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

Add transpose conversions for nested Option and Result #47193

Merged
merged 1 commit into from
Jan 21, 2018

Conversation

cramertj
Copy link
Member

@cramertj cramertj commented Jan 4, 2018

These impls are useful when working with combinator
methods that expect an option or a result, but you
have a Result<Option<T>, E> instead of an Option<Result<T, E>>
or vice versa.

@rust-highfive
Copy link
Collaborator

r? @TimNN

(rust_highfive has picked a reviewer for you, use r? to override)

@kennytm kennytm added relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 4, 2018
@cuviper
Copy link
Member

cuviper commented Jan 4, 2018

The general idea came up in the users forum too, including a few ways to generically convert Try types: https://users.rust-lang.org/t/convenience-method-for-flipping-option-result-to-result-option/13695

@shepmaster shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 6, 2018
@carols10cents
Copy link
Member

review ping for you @TimNN! Pinging you on IRC too!

@TimNN
Copy link
Contributor

TimNN commented Jan 9, 2018

The implementation looks good to me (although the last test is a bit hard to follow).

cc @rust-lang/libs regarding if we want these impls or not. (Does this need a crater run?)

@alexcrichton alexcrichton added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 9, 2018
@alexcrichton
Copy link
Member

The @rust-lang/libs team discussed this during triage today and our conclusion was that we may not wish for these methods in precisely this form. We were having difficulty (but @cramertj feel free to fill in the gaps) coming up with situations where you'd use this via something like .into(). We did think, however, that these could be useful as inherent methods on Option/Result types though, so maybe they'd be better there rather than via the From trait?

@dtolnay
Copy link
Member

dtolnay commented Jan 10, 2018

We were having difficulty coming up with situations where you would need these as Into conversions in a generic context.

// Nobody would write this.
fn f<T, E, R: Into<Result<Option<T>, E>>>(input: R) {

If we don't need them to be generic this way then we don't need them to be trait impls, so inherent methods could be a more discoverable solution and also behave better where type inference around an into-conversion would be ambiguous.

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

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

@cramertj
Copy link
Member Author

Yeah I think that makes sense! transpose or something seems good to me.

@shepmaster
Copy link
Member

People have certainly asked for the general functionality:

I know there's one for Result<Option<T>, E> somewhere, but not able to find it at the moment, so I'm in favor of some type of functionality.

@cramertj cramertj force-pushed the result-opts branch 2 times, most recently from f8aba68 to 2c0f47f Compare January 10, 2018 22:00
@cramertj cramertj changed the title Add From conversions for nested Option and Result Add transpose conversions for nested Option and Result Jan 10, 2018
@kennytm kennytm removed the relnotes Marks issues that should be documented in the release notes of the next release. label Jan 10, 2018
These impls are useful when working with combinator
methods that expect an option or a result, but you
have a Result<Option<T>, E> instead of an Option<Result<T, E>>
or vice versa.
@shepmaster shepmaster added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jan 13, 2018
@shepmaster
Copy link
Member

It sounds like the team is OK with the inherent methods, so I've updated the labels to reflect that. Please change it back if I read incorrectly.

@kennytm
Copy link
Member

kennytm commented Jan 17, 2018

Review ping for you @TimNN!

assert_eq!(x, y.transpose());
assert_eq!(x.transpose(), y);

let res: Result<Vec<i32>, BadNumErr> =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this part of the to be extremely hard to follow. Is there a specific purpose of this test? Is this a (simplified) real-world use case? Or is that supposed to test type inference?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a simplified version of a real use-case I had. I arrived at this solution when trying to handle possible errors in a filter_map closure.

@TimNN
Copy link
Contributor

TimNN commented Jan 19, 2018

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 19, 2018

📌 Commit c9ae249 has been approved by TimNN

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 20, 2018
Add transpose conversions for nested Option and Result

These impls are useful when working with combinator
methods that expect an option or a result, but you
have a `Result<Option<T>, E>` instead of an `Option<Result<T, E>>`
or vice versa.
bors added a commit that referenced this pull request Jan 21, 2018
Rollup of 10 pull requests

- Successful merges: #46938, #47193, #47508, #47510, #47532, #47535, #47559, #47568, #47573, #47578
- Failed merges:
@bors bors merged commit c9ae249 into rust-lang:master Jan 21, 2018
@golddranks
Copy link
Contributor

Just for the record, there was an earlier issue about a try_map and flip methods: rust-lang/rfcs#1815 It lead to this crate that provides similar functionality than this conversion: https://crates.io/crates/try_map It's great to see that this is going to be in std, since this is a commonly needed functionality, but there's always a threshold to find & use external crates for trivial things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.