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

Added Option::reduce #87036

Closed
wants to merge 2 commits into from
Closed

Added Option::reduce #87036

wants to merge 2 commits into from

Conversation

zesterer
Copy link
Contributor

@zesterer zesterer commented Jul 10, 2021

This PR adds a function that I and others have needed on a regular occurrence (more frequently than at least half of those on Option already) that does not have a concise inline alternative, nor can it seemingly be composed from existing functions on Option.

I've found this function to be particularly useful in the context of compiler development when one has to resolve two potentially non-existent values together, although it crops up in many other contexts. For example, two error values can be resolved like so:

err0.reduce(err1, |err0, err1| err0.merge_with(err1))

This expands to:

match (err0, err1) {
    (Some(err0), Some(err1)) => Some(err0.merge_with(err1)),
    (Some(err0), None) => Some(err0),
    (None, Some(err1)) => Some(err1),
    (None, None) => None,
}

(I've expanded this example for the sake of clarity)

Additionally, although not implemented in this PR, there is another, similar function that crops up almost as frequently:

impl<T> Option<T> {
    pub fn reduce<F>(self, other: T, f: F)
        where F: FnOnce(T, T) -> T
    {
        match self {
            Some(a) => f(a, other),
            None => other,
        }
    }
}

However, this is trivially emulated with a.into_iter().reduce(b, f).

@rust-highfive
Copy link
Collaborator

r? @yaahc

(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 Jul 10, 2021
@the8472
Copy link
Member

the8472 commented Jul 10, 2021

Bikeshed, this looks a lot like Iterator::reduce, so it could be named that way.

@zesterer
Copy link
Contributor Author

Bikeshed, this looks a lot like Iterator::reduce, so it could be named that way.

I quite like that idea, very much open to changing it to that if others agree.

library/core/src/option.rs Outdated Show resolved Hide resolved
library/core/src/option.rs Outdated Show resolved Hide resolved
Comment on lines 1079 to 1081
/// assert_eq!(Some(2).or_zip_with(Some(3), |a, b| a + b), Some(5));
/// assert_eq!(Some(2).or_zip_with(None, |a, b| a + b), Some(2));
/// assert_eq!(None.or_zip_with(Some(3), |a, b| a + b), Some(3));
Copy link
Member

Choose a reason for hiding this comment

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

These do not look like real use cases to me. At a first glance, you are trying to sum these up if any of them has a value, and get None when both of them do not. What does this achieve? You can either resolve this by responding to me or changing the example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've altered the example to hopefully make the use-case clearly. Regardless, it is only an example. It's difficult to demonstrate a real use-case in such a limited, controlled environment.

library/core/src/option.rs Outdated Show resolved Hide resolved
@zesterer zesterer changed the title Added Option::or_zip_with Added Option::reduce Jul 12, 2021
@zesterer
Copy link
Contributor Author

On the recommendation by @the8472 I've renamed this method to reduce. Additionally, I've cleaned up and improved the documentation & example.

@rust-log-analyzer

This comment has been minimized.

@yaahc
Copy link
Member

yaahc commented Jul 12, 2021

Looks like this is a duplicate of #87036 which we rejected last month.

From the other thread:

Thanks for your PR.

I'm not convinced we should add this, though. I can't think of a lot of use cases that aren't already covered by Iterator::reduce, and I don't think it'd be clear to a reader what this function does, which I think is especially important for methods on commonly used types like Option.

Pinging @rust-lang/libs in case anyone feels like this would be an important addition. Otherwise I'll close this PR.

I still feel the same way as I did when the last issue came up, but since this has come up twice in as many months perhaps we should re-evaluate.

@zesterer
Copy link
Contributor Author

zesterer commented Jul 12, 2021

Looks like this is a duplicate of #87036 which we rejected last month.

From the other thread:

Thanks for your PR.
I'm not convinced we should add this, though. I can't think of a lot of use cases that aren't already covered by Iterator::reduce, and I don't think it'd be clear to a reader what this function does, which I think is especially important for methods on commonly used types like Option.
Pinging @rust-lang/libs in case anyone feels like this would be an important addition. Otherwise I'll close this PR.

I still feel the same way as I did when the last issue came up, but since this has come up twice in as many months perhaps we should re-evaluate.

Thanks for the response.

The function I'm proposing here does not do the same thing as Iterator::reduce (I discuss the similar but distinct function that does do a similar thing in the issue body). Instead, it performs an operation that I believe is not possible to compose out of existing Option/Iterator methods.

I'm not sure which past issue you're referring to (I believe you might have accidentally linked to this one).

@yaahc
Copy link
Member

yaahc commented Jul 12, 2021

Oops, I linked this PR instead of the other one: #84695

The function I'm proposing here does not do the same thing as Iterator::reduce (I discuss the similar but distinct function that does do a similar thing in the issue body). Instead, it performs an operation that I believe is not possible to compose out of existing Option/Iterator methods.

I'm not following how this can't be done with Iterator. This example seems to reproduce what you're describing: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=92c6ea1718d4992b2a09a5f4327013a4

@zesterer
Copy link
Contributor Author

Oops, I linked this PR instead of the other one: #84695

The function I'm proposing here does not do the same thing as Iterator::reduce (I discuss the similar but distinct function that does do a similar thing in the issue body). Instead, it performs an operation that I believe is not possible to compose out of existing Option/Iterator methods.

I'm not following how this can't be done with Iterator. This example seems to reproduce what you're describing: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=92c6ea1718d4992b2a09a5f4327013a4

Ah, I forget that Iterator::chain takes an impl IntoIterator! That's more concise than past alternatives I've used.

I still believe that it's a common enough occurrence to warrant a dedicated function, but I won't resurrect a debate that's been so recently discussed (when I opened the issue, I didn't search for "reduce" due to the aforementioned name change).

Thanks for the clarification!

@zesterer zesterer closed this Jul 12, 2021
@WaffleLapkin
Copy link
Member

I've made a micro crate that provides the function, in case anyone needs it too: opt_reduce.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants