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 Option::{zip,zip_with} methods under "option_zip" gate #69997

Merged
merged 3 commits into from
Mar 21, 2020

Conversation

WaffleLapkin
Copy link
Member

@WaffleLapkin WaffleLapkin commented Mar 14, 2020

Edit: Tracking issue


This PR introduces 2 methods - Option::zip and Option::zip_with with
respective signatures:

  • zip: (Option<T>, Option<U>) -> Option<(T, U)>
  • zip_with: (Option<T>, Option<U>, (T, U) -> R) -> Option<R>
    Both are under the feature gate "option_zip".

I'm not sure about the name "zip", maybe we can find a better name for this.
(I would prefer union for example, but this is a keyword :( )


Recently in a russian rust begginers telegram chat a newbie asked (translated):

Are there any methods for these conversions:

  1. (Option<A>, Option<B>) -> Option<(A, B)>
  2. Vec<Option<T>> -> Option<Vec<T>>

?

While second (2.) is clearly vec.into_iter().collect::<Option<Vec<_>>(), the
first one isn't that clear.

I couldn't find anything similar in the core and I've come to this solution:

let tuple: (Option<A>, Option<B>) = ...;
let res: Option<(A, B)> = tuple.0.and_then(|a| tuple.1.map(|b| (a, b)));

However this solution isn't "nice" (same for just match/if let), so I thought
that this functionality should be in core.

@rust-highfive
Copy link
Collaborator

r? @LukasKalbertodt

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 14, 2020
@Centril
Copy link
Contributor

Centril commented Mar 14, 2020

I'm not sure about the name "zip", maybe we can find a better name for this.

zip is clearly the right name as it is analogous to Iterator::zip. (We don't have Iterator::zip_with though.)

@WaffleLapkin
Copy link
Member Author

Other currently possible solutions:

let tuple: (Option<A>, Option<B>) = ...;
let res: Option<(A, B)> = match tuple {
    (Some(a), Some(b)) => Some((a, b)),
    _ => None,
};
#![feature(try_blocks)] // require nightly
let tuple: (Option<A>, Option<B>) = ...;
let res: Option<(A, B)> = try { (tuple.0?, tuple.1?) }

(playground)

@WaffleLapkin
Copy link
Member Author

We don't have Iterator::zip_with though

Hmmm, a.zip_with(b, |x, y| ...) is the same as a.zip(b).map(|(x, y)| ...)

@Centril
Copy link
Contributor

Centril commented Mar 14, 2020

I'm in favor of adding Option::zip at least by analogy to Iterator::zip (see also Option::flatten which was also accepted by analogy to Iterator::flatten). Also, I believe Option::zip to be a useful and fairly common operation to justify its addition on its own merits (as opposed to a precedent based justification).

I'm less sure about zip_with though, as it does not have similar precedent.

@bors
Copy link
Contributor

bors commented Mar 17, 2020

☔ The latest upstream changes (presumably #70062) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

Thanks a bunch for the PR!

I agree with @Centril and am also in favor of adding Option::zip. I think I'd prefer not to add Option::zip_with though. I think it's pretty niche. But I'm fine with merging it as unstable to find out if users find good uses for it.

I added a few line comments, but nothing serious. And you need to rebase.

src/libcore/option.rs Outdated Show resolved Hide resolved
src/libcore/option.rs Outdated Show resolved Hide resolved
src/libcore/option.rs Outdated Show resolved Hide resolved
src/libcore/option.rs Outdated Show resolved Hide resolved
src/libcore/option.rs Outdated Show resolved Hide resolved
src/libcore/option.rs Outdated Show resolved Hide resolved
This commit introduces 2 methods - `Option::zip` and `Option::zip_with` with
respective signatures:
- zip: `(Option<T>, Option<U>) -> Option<(T, U)>`
- zip_with: `(Option<T>, Option<U>, (T, U) -> R) -> Option<R>`
Both are under the feature gate "option_zip".

I'm not sure about the name "zip", maybe we can find a better name for this.
(I would prefer `union` for example, but this is a keyword :( )

--------------------------------------------------------------------------------

Recently in a russian rust begginers telegram chat a newbie asked (translated):
> Are there any methods for these conversions:
>
> 1. `(Option<A>, Option<B>) -> Option<(A, B)>`
> 2. `Vec<Option<T>> -> Option<Vec<T>>`
>
> ?

While second (2.) is clearly `vec.into_iter().collect::<Option<Vec<_>>()`, the
first one isn't that clear.

I couldn't find anything similar in the `core` and I've come to this solution:
```rust
let tuple: (Option<A>, Option<B>) = ...;
let res: Option<(A, B)> = tuple.0.and_then(|a| tuple.1.map(|b| (a, b)));
```

However this solution isn't "nice" (same for just `match`/`if let`), so I thought
that this functionality should be in `core`.
@WaffleLapkin
Copy link
Member Author

If Option::zip_with addition is so questionable, shouldn't we use a different gate for it ("option_zip_with")?

- remove `#[inline]` attributes (see rust-lang#69997 (comment))
- fill tracking issue in `#[unstable]` attributes
- slightly improve the docs
@LukasKalbertodt
Copy link
Member

If Option::zip_with addition is so questionable, shouldn't we use a different gate for it ("option_zip_with")?

I don't have a strong preference, but I'd say you can keep it as is for now. I suspect that when we are discussing stabilizing zip, we will either also stabilize zip_with or remove it completely. I doubt we would be keeping it as unstable.

Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

Just a tiny thing still.

src/libcore/option.rs Outdated Show resolved Hide resolved
src/libcore/option.rs Outdated Show resolved Hide resolved
@LukasKalbertodt
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Mar 20, 2020

📌 Commit 121bffc has been approved by LukasKalbertodt

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 20, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 21, 2020
…bertodt

add `Option::{zip,zip_with}` methods under "option_zip" gate

This PR introduces 2 methods - `Option::zip` and `Option::zip_with` with
respective signatures:
- zip: `(Option<T>, Option<U>) -> Option<(T, U)>`
- zip_with: `(Option<T>, Option<U>, (T, U) -> R) -> Option<R>`
Both are under the feature gate "option_zip".

I'm not sure about the name "zip", maybe we can find a better name for this.
(I would prefer `union` for example, but this is a keyword :( )

--------------------------------------------------------------------------------

Recently in a russian rust begginers telegram chat a newbie asked (translated):
> Are there any methods for these conversions:
>
> 1. `(Option<A>, Option<B>) -> Option<(A, B)>`
> 2. `Vec<Option<T>> -> Option<Vec<T>>`
>
> ?

While second (2.) is clearly `vec.into_iter().collect::<Option<Vec<_>>()`, the
first one isn't that clear.

I couldn't find anything similar in the `core` and I've come to this solution:
```rust
let tuple: (Option<A>, Option<B>) = ...;
let res: Option<(A, B)> = tuple.0.and_then(|a| tuple.1.map(|b| (a, b)));
```

However this solution isn't "nice" (same for just `match`/`if let`), so I thought
that this functionality should be in `core`.
Centril added a commit to Centril/rust that referenced this pull request Mar 21, 2020
…bertodt

add `Option::{zip,zip_with}` methods under "option_zip" gate

This PR introduces 2 methods - `Option::zip` and `Option::zip_with` with
respective signatures:
- zip: `(Option<T>, Option<U>) -> Option<(T, U)>`
- zip_with: `(Option<T>, Option<U>, (T, U) -> R) -> Option<R>`
Both are under the feature gate "option_zip".

I'm not sure about the name "zip", maybe we can find a better name for this.
(I would prefer `union` for example, but this is a keyword :( )

--------------------------------------------------------------------------------

Recently in a russian rust begginers telegram chat a newbie asked (translated):
> Are there any methods for these conversions:
>
> 1. `(Option<A>, Option<B>) -> Option<(A, B)>`
> 2. `Vec<Option<T>> -> Option<Vec<T>>`
>
> ?

While second (2.) is clearly `vec.into_iter().collect::<Option<Vec<_>>()`, the
first one isn't that clear.

I couldn't find anything similar in the `core` and I've come to this solution:
```rust
let tuple: (Option<A>, Option<B>) = ...;
let res: Option<(A, B)> = tuple.0.and_then(|a| tuple.1.map(|b| (a, b)));
```

However this solution isn't "nice" (same for just `match`/`if let`), so I thought
that this functionality should be in `core`.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 21, 2020
Rollup of 16 pull requests

Successful merges:

 - rust-lang#65097 (Make std::sync::Arc compatible with ThreadSanitizer)
 - rust-lang#69033 (Use generator resume arguments in the async/await lowering)
 - rust-lang#69997 (add `Option::{zip,zip_with}` methods under "option_zip" gate)
 - rust-lang#70038 (Remove the call that makes miri fail)
 - rust-lang#70058 (can_begin_literal_maybe_minus: `true` on `"-"? lit` NTs.)
 - rust-lang#70111 (BTreeMap: remove shared root)
 - rust-lang#70139 (add delay_span_bug to TransmuteSizeDiff, just to be sure)
 - rust-lang#70165 (Remove the erase regions MIR transform)
 - rust-lang#70166 (Derive PartialEq, Eq and Hash for RangeInclusive)
 - rust-lang#70176 (Add tests for rust-lang#58319 and rust-lang#65131)
 - rust-lang#70177 (Fix oudated comment for NamedRegionMap)
 - rust-lang#70184 (expand_include: set `.directory` to dir of included file.)
 - rust-lang#70187 (more clippy fixes)
 - rust-lang#70188 (Clean up E0439 explanation)
 - rust-lang#70189 (Abi::is_signed: assert that we are a Scalar)
 - rust-lang#70194 (#[must_use] on split_off())

Failed merges:

r? @ghost
@bors bors merged commit 801a25a into rust-lang:master Mar 21, 2020
@WaffleLapkin WaffleLapkin deleted the option_zip branch March 21, 2020 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants