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::unzip() #87800

Closed
3 tasks done
Kixiron opened this issue Aug 5, 2021 · 27 comments · Fixed by #98204
Closed
3 tasks done

Tracking Issue for Option::unzip() #87800

Kixiron opened this issue Aug 5, 2021 · 27 comments · Fixed by #98204
Labels
A-result-option Area: Result and Option combinators C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Kixiron
Copy link
Member

Kixiron commented Aug 5, 2021

Feature gate: #![feature(unzip_option)]

This is a tracking issue for the Option::unzip() method, which turns an Option<(T, U)> into an (Option<T>, Option<U>).
This is the inverse of Option::zip() and is a nice little convenience function

Public API

// core::option

impl<T, U> Option<(T, U)> {
    pub fn unzip(self) -> (Option<T>, Option<U>);
}

Steps / History

Unresolved Questions

  • None yet.
@Kixiron Kixiron added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Aug 5, 2021
@m-ou-se m-ou-se added the A-result-option Area: Result and Option combinators label Aug 9, 2021
@camsteffen
Copy link
Contributor

What is a use case for this?

I don't think this can be justified merely by the existence of Option::zip. It seems much less useful. I think Option::zip is useful because it functions as a logical AND.

@Kixiron
Copy link
Member Author

Kixiron commented Aug 11, 2021

The use case is for turning an option of tuples into a tuple of options, I've ran into needing it a few times

@camsteffen
Copy link
Contributor

Simply stating what the function does is not a use case. What is the code, what does it do, and importantly what does the alternative look like?

@Kixiron
Copy link
Member Author

Kixiron commented Aug 11, 2021

My code looked generally like this

let (x, y) = if let Some((x, y)) = foo() {
    (Some(x), Some(y))
} else {
    (None, None)
};

// Use x and y separately

With unzip that'd be this

let (x, y) = foo().unzip();

The only alternatives to unzip are relatively verbose match or if let statements

@camsteffen
Copy link
Contributor

Can you give more context? A use case would show how the function might be used in a real life scenario. It should also demonstrate that the usage cannot be easily refactored to something else. There is no way to know how // Use x and y separately could be refactored without seeing actual code.

@Kixiron
Copy link
Member Author

Kixiron commented Aug 11, 2021

x and y are each used separately, they're passed as individual arguments or something. Otherwise you'd have to either rebind each of them with thing.map(|(x, )| x) or inline that map at each use site

@Kixiron
Copy link
Member Author

Kixiron commented Aug 23, 2021

Some code I was trying to write today:

// Without `.unzip()`
impl<'a, T> Iterator for EdgeIter<'a, T> {
    type Item = &'a Edge<T>;

    fn next(&mut self) -> Option<Self::Item> {
        let edges = self.edges.and_then(|edges| {
            if let [edge, rest @ ..] = edges {
                Some((rest, edge))
            } else {
                None
            }
        });
        self.edges = edges.map(|edges| edges.0);

        edges.map(|edges| edges.1).and_then(|next_id| {
            let next = self.edge_data.get(next_id);
            debug_assert!(next.is_some(), "edge data missing edge {}", next_id);

            next
        })
    }
}

// With `.unzip()`
impl<'a, T> Iterator for EdgeIter<'a, T> {
    type Item = &'a Edge<T>;

    fn next(&mut self) -> Option<Self::Item> {
        let (edges, next) = self
            .edges
            .and_then(|edges| {
                if let [edge, rest @ ..] = edges {
                    Some((rest, edge))
                } else {
                    None
                }
            })
            .unzip();
        self.edges = edges;

        next.and_then(|next_id| {
            let next = self.edge_data.get(next_id);
            debug_assert!(next.is_some(), "edge data missing edge {}", next_id);

            next
        })
    }
}

@camsteffen
Copy link
Contributor

How about this?

fn next(&mut self) -> Option<Self::Item> {
    if let Some(edges) = self.edges {
        if let [next_id, rest @ ..] = edges {
            self.edges = Some(rest);
            let next = self.edge_data.get(next_id);
            debug_assert!(next.is_some(), "edge data missing edge {}", next_id);
            return next;
        }
    }
    None
}

@Kixiron
Copy link
Member Author

Kixiron commented Aug 23, 2021

Doesn't really work, there's a significantly larger amount of work going on that I removed

@camsteffen
Copy link
Contributor

camsteffen commented Aug 23, 2021

Okay, then you still haven't demonstrated the usefulness of Option::unzip.

@repnop
Copy link
Contributor

repnop commented Aug 23, 2021

I don't think its particularly productive to argue over some arbitrary metric of "usefulness" that's entirely subjective since this it may come up more often in some domains compared to others -- if you have concerns with the method w.r.t its interactions with other parts of the language or the implementation, it's much more productive to discuss those. Plus, having the inverse operation for one that already exists seems perfectly reasonable, at least to me.

Okay, then you still haven't demonstrated the usefulness of Option::unzip.

This comes off quite rude, Kixiron already posted snippets of code with which he found it handy to have, and is not required to copy and paste entire files of code to "prove" its usefulness to you.

@camsteffen
Copy link
Contributor

I'm sorry for coming off rude.

@mehcode
Copy link
Contributor

mehcode commented Aug 28, 2021

To add a concrete data point, I often run into needing this when dealing with databases. I'll have a nice rusty optional tuple of data that I need to transpose into a tuple of options to then pass into a query.

I'd love to see this for N variants. The itertools crate has a izip! for an N-variant Option::zip that is quite useful. But having a simple two-variant in the std is a useful building block.

@jimblandy
Copy link
Contributor

jimblandy commented Sep 23, 2021

I was surprised to find myself wanting this today:

pub fn map_interpolation(
    interp: (&str, Span),
    sampling: Option<(crate::Sampling, Span)>,
) -> Result<crate::Interpolation, Error<'_>> {
    // Option::unzip is not stable.
    let (sampling, sampling_span) = match sampling {
        Some((sampling, span)) => (Some(sampling), Some(span)),
        None => (None, None),
    };
    match interp.0 {
        "linear" => Ok(crate::Interpolation::Linear(
            sampling.unwrap_or(crate::Sampling::Center),
        )),
        "flat" => {
            if let Some(span) = sampling_span {
                return Err(Error::SamplingNotPermitted {
                    interpolation: interp.1,
                    sampling: span,
                });
            }
            Ok(crate::Interpolation::Flat)
        }
        "perspective" => Ok(crate::Interpolation::Perspective(
            sampling.unwrap_or(crate::Sampling::Center),
        )),
        _ => Err(Error::UnknownInterpolation(interp.1)),
    }
}

The two components of sampling are needed on different paths, and the unzip seems nicer than scattering little .map(|_, span| span) adapters everywhere.

@jplatte

This comment has been minimized.

@lilyball
Copy link
Contributor

I just ran into this today. I have code like

let foo = self.bar.map(|bar| {
    let (value, guard) = process(bar);
    Some(value, guard)
});

and what I really want to say is

let (foo, _guard) = self.bar.map(|bar| {
    let (value, guard) = process(bar);
    Some(value, guard)
}).unzip();

Having to split this apart myself is annoying. Doing this as a map makes perfect sense, instead I'm going to have to use an if let Some(bar) = self.bar { … } else { (None, None) }.

@wbenny
Copy link

wbenny commented Mar 21, 2022

Same,

I have roughly following code:

pub struct Metadata {
    pub file_type: Option<String>,
    pub file_mime: Option<String>,
    ...
}

fn get_type_and_mime(path: &str) -> Option<(String, String)>;

fn from_path(path: &str) -> Metadata {
    let (file_type, file_mime) = get_type_and_mime(path).map_or( // < this could be replaced by unzip() but you playin
                (None, None),
                |(t0, t1)| (Some(t0), Some(t1))
            );
    // ...
    Metadata {
        file_type,
        file_mime,
        ...
    }
}

@coolreader18
Copy link
Contributor

coolreader18 commented Jun 2, 2022

Would've found this useful in conjunction with split_once today:

let (host, port) = addr_str.split_once(':').unzip();
let host = host.unwrap_or(&addr_str);
let port = port.map(|p| p.parse::<u16>()).transpose()?;

@Stargateur
Copy link
Contributor

Stargateur commented Jun 3, 2022

I'm unsure I will ever need this cause I also never used zip, but feel very reasonable to me since there have been no remark on the implementation since +6 months I think you can do a PR for a stabilisation @Kixiron

@Kixiron
Copy link
Member Author

Kixiron commented Jun 3, 2022

@rust-lang/libs-api can we start the FCP for this?

@cyqsimon
Copy link
Contributor

I would like to add that this is very often useful when dealing with functions that return a value pair, which is fairly common.

E.g. mpsc::channel, UnixStream::pair

@karx1
Copy link

karx1 commented Jul 11, 2022

I needed this today, I have:

        let orig = manager
            .get_focused()
            .await
            .map(|w| (w, manager.get_framed_window(w)));

However, I need to use the focused and the framed window separately, which is where unzip would have come in handy.

@Amanieu
Copy link
Member

Amanieu commented Jul 11, 2022

Proposing stabilization of Option::unzip():

// core::option

impl<T, U> Option<(T, U)> {
    pub fn unzip(self) -> (Option<T>, Option<U>);
}

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Jul 11, 2022

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jul 11, 2022
@rfcbot rfcbot added the disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. label Jul 11, 2022
@danjl1100
Copy link

I stumbled upon a use case similar to the posts above, and I think this would be useful. 👍

To clarify (and possibly strengthen the argument for stabilizing)...
Is the general consensus that is only for 2-tuples Option<(T, U)> (and not that "larger N-tuples" Option<(T, U, V, ...)>) because the current Option::zip() method only operates on 2-tuples?

This seems like a natural rationale for why this PR may be accepted, while future a N-tuple unzip may not.

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Sep 13, 2022
@rfcbot
Copy link

rfcbot commented Sep 13, 2022

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

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Sep 23, 2022
@rfcbot
Copy link

rfcbot commented Sep 23, 2022

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

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Sep 23, 2022
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Sep 29, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Oct 25, 2022
Stabilize `Option::unzip()`

Stabilizes `Option::unzip()`, closes rust-lang#87800

`@rustbot` modify labels: +T-libs-api
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Oct 25, 2022
Stabilize `Option::unzip()`

Stabilizes `Option::unzip()`, closes rust-lang#87800

``@rustbot`` modify labels: +T-libs-api
@bors bors closed this as completed in d2d44f6 Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-result-option Area: Result and Option combinators C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.