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 Option::zip and Option::zip_with (feature option_zip) #70086

Open
1 of 4 tasks
LukasKalbertodt opened this issue Mar 17, 2020 · 12 comments
Open
1 of 4 tasks
Labels
A-result-option B-unstable C-tracking-issue Libs-Small Libs-Tracked T-libs-api

Comments

@LukasKalbertodt
Copy link
Member

LukasKalbertodt commented Mar 17, 2020

This is a tracking issue for #![feature(option_zip)] which was introduced in #69997.

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also uses as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Unresolved Questions

  • Should we also include zip_with in the public API? Seems like a niche API and is easily replaced with a.zip(b).map(f).
@LukasKalbertodt LukasKalbertodt added T-libs-api B-unstable C-tracking-issue labels Mar 17, 2020
@cwfitzgerald
Copy link

cwfitzgerald commented May 25, 2020

For what it's worth: my personal two cents are that both should be included, as even if zip_with's use may be a bit more rare, I think the pattern of (Option, Option) -> Option<Something> is common enough, and the terseness win is great enough, that it should be kept.

Edit: One note is that zip_with and zip().map() take different functions as argument, the former takes FnOnce(T, U) -> R the other takes FnOnce((T, U)) -> R, the former of which is generally more directly useful to calling off to other methods.

Edit 2: Is there a better place for this discussion, or is here okay?

@WaffleLapkin
Copy link
Contributor

WaffleLapkin commented May 25, 2020

Edit: One note is that zip_with and zip().map() take different functions as argument, the former takes FnOnce(T, U) -> R the other takes FnOnce((T, U)) -> R, the former of which is generally more directly useful to calling off to other methods.

Yeah, I've just found the same thing. E.g.:

let a: Option<i32> = ...;
let b: Option<i32> = ...;
let res = a.zip_with(b, Add::add);
// or
let res = a.zip(b).map(|(a, b)| a + b); // destruction needed 

I think in this example zip_with is nicer/clearer than zip+map

@tesuji
Copy link
Contributor

tesuji commented Jun 3, 2020

I opened a PR to stabilize Option::zip only: #72938

@qm3ster
Copy link
Contributor

qm3ster commented Jun 10, 2020

I was originally looking for this, but I feel like the upcoming try_blocks greatly diminished the uses.

let a = Some(7);
let b = Some(true);
let out = a.iter().zip(b).map(|(a, b)| s * o as u8).next(); //stable
let out = a.zip(b).map(|(a, b)| a * b as u8); // #![feature(option_zip)]
let out = a.zip_with(b, |a, b| a * b as u8); // #![feature(option_zip)]
let out: Option<_> = try { a? * b? as u8 }; // #![feature(try_blocks)]

wow, such expressivity!
@WaffleLapkin's case:

let res = a.zip_with(b, Add::add);
let res = try { a? + b? };

Option::zip is still king for the trivial zipping action in isolation:

wants_tuple(a.iter().zip(b).next()); //stable
wants_tuple(a.zip(b)); // #![feature(option_zip)]
wants_tuple(try { (a? * b?) }); // #![feature(try_blocks)]

@WaffleLapkin
Copy link
Contributor

WaffleLapkin commented Jun 11, 2020

One interesting note is that try { (a?, b?) } is lazy, where is a.zip(b) is not. However, I personally think that the later is easier to read (I think that early returns are weird in cases like that or Ok(res?)).

Maybe we should also implement smt like this:

fn zip_lazy(self, f: impl FnOnce() -> Option<U>) -> Option<T, U> { ... }

? 🤔

RalfJung added a commit to RalfJung/rust that referenced this issue Jun 15, 2020
Stabilize Option::zip

This PR stabilizes the following API:

```rust
impl<T> Option<T> {
    pub fn zip<U>(self, other: Option<U>) -> Option<(T, U)>;
}
```

This API has real world usage as seen in <https://grep.app/search?q=-%3E%20Option%3C%5C%28T%2C%5Cs%3FU%5C%29%3E&regexp=true&filter[lang][0]=Rust>.

The `zip_with` method is left unstably as this API is kinda niche
and it hasn't received much usage in Rust repositories on GitHub.

cc rust-lang#70086
RalfJung added a commit to RalfJung/rust that referenced this issue Jun 15, 2020
Stabilize Option::zip

This PR stabilizes the following API:

```rust
impl<T> Option<T> {
    pub fn zip<U>(self, other: Option<U>) -> Option<(T, U)>;
}
```

This API has real world usage as seen in <https://grep.app/search?q=-%3E%20Option%3C%5C%28T%2C%5Cs%3FU%5C%29%3E&regexp=true&filter[lang][0]=Rust>.

The `zip_with` method is left unstably as this API is kinda niche
and it hasn't received much usage in Rust repositories on GitHub.

cc rust-lang#70086
flip1995 pushed a commit to flip1995/rust that referenced this issue Jun 23, 2020
Stabilize Option::zip

This PR stabilizes the following API:

```rust
impl<T> Option<T> {
    pub fn zip<U>(self, other: Option<U>) -> Option<(T, U)>;
}
```

This API has real world usage as seen in <https://grep.app/search?q=-%3E%20Option%3C%5C%28T%2C%5Cs%3FU%5C%29%3E&regexp=true&filter[lang][0]=Rust>.

The `zip_with` method is left unstably as this API is kinda niche
and it hasn't received much usage in Rust repositories on GitHub.

cc rust-lang#70086
@KodrAus KodrAus added A-result-option Libs-Small Libs-Tracked labels Jul 29, 2020
@lebensterben
Copy link

lebensterben commented Sep 1, 2020

There's one use case, suppose foo and bar are Option<Vec<T>>.
With this new feature, we can have
foo.zip_with(bar, Vec::extend) for example.
If we use zip() and map(), that'd be
foo.zip(bar).map(|first, second| first.extend(second))
That's just unnecessarily long.

@qm3ster
Copy link
Contributor

qm3ster commented Sep 5, 2020

@lebensterben do you mean foo: Option<&mut Vec<T>>? Or foo.as_mut() (in either)?
Because otherwise it would also be immediately dropped.

foo.as_mut().map(|first| Some(first.append(&mut bar?)));
// or
let _: Option<_> = try{ foo.as_mut()?.append(&mut bar?) };

@sherubthakur
Copy link

sherubthakur commented Dec 9, 2020

I really like the interface haskell's lift functions have

liftA :: Applicative f => (a -> b) -> f a -> f b
liftA3 :: Applicative f => (a -> b -> c -> d) -> f a -> f b -> f c -> f d
liftA2 :: (a -> b -> c) -> f a -> f b -> f c

Maybe we can have something similar in Rust

Option::lift(f, optional1)
Option::lift2(f, optional1, optiona2)
// etc

Probably the same interface for all applicatives (Iterators, Result, etc).

@sollyucko
Copy link
Contributor

sollyucko commented Dec 9, 2020

Would lift be equivalent to map and lift2 equivalent to zip_with?

@sherubthakur
Copy link

sherubthakur commented Dec 9, 2020

@sollyucko yeah! If we consider doing it the number up to which we should have these functions is debatable.

dtolnay added a commit to dtolnay/clang-ast that referenced this issue Apr 28, 2021
Required by the use of Option::zip.

    error[E0658]: use of unstable library feature 'option_zip'
       --> src/loc.rs:551:14
        |
    551 |             .zip(self.expansion_loc.as_ref())
        |              ^^^
        |
        = note: see issue #70086 <rust-lang/rust#70086> for more information

    error[E0658]: use of unstable library feature 'option_zip'
       --> src/loc.rs:529:36
        |
    529 |             spelling_included_from.zip(expansion_included_from).map_or(
        |                                    ^^^
        |
        = note: see issue #70086 <rust-lang/rust#70086> for more information
@sdroege
Copy link
Contributor

sdroege commented Jul 27, 2021

Should we also include zip_with in the public API? Seems like a niche API and is easily replaced with a.zip(b).map(f).

We have quite many calls to zip() followed by map() now in the GStreamer Rust bindings for working with Option<u64>, e.g. to add them together. x.zip(y).map(|(x, y)| x + y) is more annoying to type than x.zip_with(y, |x, y| x + y) but I can see the point of not including it because if you include it, why not also have a shorter form (zip_and_then?) of the equally often used x.zip(y).and_then(|(x, y)| x.checked_add(y)).

@qm3ster
Copy link
Contributor

qm3ster commented Jul 27, 2021

@sdroege

let a = try { x? + y? };
let b = try { x?.checked_add(y?)? };

feels_good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-result-option B-unstable C-tracking-issue Libs-Small Libs-Tracked T-libs-api
Projects
None yet
Development

No branches or pull requests

10 participants