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

Implement iter::Sum and iter::Product for Option #58975

Merged
merged 4 commits into from
May 29, 2019
Merged

Implement iter::Sum and iter::Product for Option #58975

merged 4 commits into from
May 29, 2019

Conversation

jtdowney
Copy link
Contributor

@jtdowney jtdowney commented Mar 6, 2019

This is similar to the existing implementation for Result. It will take each item into the accumulator unless a None is returned.

I based a lot of this on #38580. From that discussion it didn't seem like this addition would be too controversial or difficult. One thing I still don't understand is picking the values for the stable attribute. This is my first non-documentation PR for rust so I am open to any feedback on improvements.

This is similar to the existing implementation for `Result`. It will
take each item into the accumulator unless a `None` is returned.
@rust-highfive
Copy link
Collaborator

r? @kennytm

(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 6, 2019
@Centril Centril added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Mar 6, 2019
@Centril
Copy link
Contributor

Centril commented Mar 6, 2019

r? @SimonSapin

@rust-highfive rust-highfive assigned SimonSapin and unassigned kennytm Mar 6, 2019
@scottmcm
Copy link
Member

scottmcm commented Mar 7, 2019

Previous PR about Sum for Option: #50884

@jtdowney
Copy link
Contributor Author

jtdowney commented Mar 8, 2019

@scottmcm thanks, I missed this PR in my search. If folks still think it shouldn't be in core then that is fine with me. I was mostly confused about the lack of parity with Result which is somewhat unusual from my experience with rust.

@scottmcm
Copy link
Member

scottmcm commented Mar 8, 2019

@jtdowney Yours is consistent in behaviour with Result, right? I think the other wasn't, so it's possible the outcome will be different here.

@jtdowney
Copy link
Contributor Author

jtdowney commented Mar 8, 2019

Correct, this is PR is consistent with how iter::Product and iter::Sum work with Result today. I hadn't finished reading the other PR before I commented previously but they seem to be going at different things. My use case is solved today by converting the iterator of Option<T> to Result<T, ()> with something like iter.map(|v| v.ok_or(())).sum() but that seemed unnecessary which is why I sent the PR.

@Dylan-DPC-zz
Copy link

ping from triage anyone from @rust-lang/libs can review this?

@alexcrichton
Copy link
Member

This looks reasonable to me to merge! In that case I'll...

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Mar 19, 2019

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

Concerns:

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 proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Mar 19, 2019
@dtolnay
Copy link
Member

dtolnay commented Mar 19, 2019

@rfcbot concern not sold on these impls

The use cases that come to mind would all be more clearly written with a for-loop. Could you point out some places in real code where these impls would be beneficial?

I can see that part of the motivation is to be consistent with Result's Sum and Product impls. But I find that the behavior of summing an iterator of Result<T, E> into a Result<T, E> is more intuitive to me than the behavior of summing Option<T> into an Option<T>. Looking at the example code from the PR:

v.iter().map(|&x: &i32|
    if x < 0 { None }
    else { Some(x) }
).sum()

it would be easy to read this as summing only those items that are Some and ignoring Nones.

@jtdowney
Copy link
Contributor Author

jtdowney commented Mar 19, 2019

@dtolnay the specific case I hit when I discovered this was missing was taking a string, calling chars(), map(|c| c.to_digit(10)), and then summing that after some more filtering and mapping. That code you quoted in the PR was modified from the existing PR for Result. Happy to replace it with a better example.

@Centril Centril added relnotes Marks issues that should be documented in the release notes of the next release. and removed needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Mar 19, 2019
@Centril Centril added this to the 1.35 milestone Mar 19, 2019
src/libcore/iter/traits/accum.rs Outdated Show resolved Hide resolved
src/libcore/iter/traits/accum.rs Outdated Show resolved Hide resolved
@jtdowney
Copy link
Contributor Author

jtdowney commented Mar 20, 2019

If it helps, here is the code I was trying to write:

fn is_luhn_valid(value: &str) -> bool {
    value
        .chars()
        .rev()
        .map(|c| c.to_digit(10))
        .enumerate()
        .map(|(idx, n)| {
            if idx % 2 != 0 {
                n.map(|v| {
                    let v = v * 2;
                    if v > 9 {
                        v - 9
                    } else {
                        v
                    }
                })
            } else {
                n
            }
        })
        .sum::<Option<u32>>()
        .map(|total| total % 10 == 0)
        .unwrap_or(false)
}

fn main() {
    dbg!(is_luhn_valid("4111111111111111"));
    dbg!(is_luhn_valid("5454545454545454"));
}

This is an implementation of the luhn check algorithm.

@dtolnay
Copy link
Member

dtolnay commented Mar 20, 2019

I think a more inspired example in the documentation would be helpful -- something reasonably representative of the way that you might find this impl used in real code where it is the best solution to the problem being solved.

(Not that doc comments inside trait impls end up all that visible. Current rustdoc would bury it pretty thoroughly. But it would serve as long-term documentation of the use cases that we wanted to support by providing the impl.)


The use cases that come to mind would all be more clearly written with a for-loop.

I think unfortunately this applies to your Luhn implementation too. It is neat that you were able to find a way to express this all in a single method chain, and only one semicolon, but I think it comes out quite challenging to read. A reader looking to understand the implementation or code reviewer looking to confirm that it is correct would need to:

  • Infer based on code lower down that to_digit returns an Option.
  • Trace through the Option gymnastics to figure out that |(idx, n)| { ... } returns an Option.
  • Not get lost in 6 levels of indentation.
  • Probably look up what summing Options into an Option means.

Here is how I might write this instead. My implementation is the same number of lines (24) but some of them are now blank. I find this one far easier to follow -- curious what you think. Even the signature of to_digits is more obvious because we are using ? inside a function that returns Option. It is half as indented and the type-inference-in-your-head is far simpler.

Based on this use case I would still side against accepting the new impls.

fn is_luhn_valid(input: &str) -> bool {
    match luhn_sum(input) {
        Some(sum) => sum % 10 == 0,
        None => false,
    }
}

fn luhn_sum(input: &str) -> Option<u32> {
    let mut sum = 0;

    for (idx, ch) in input.chars().rev().enumerate() {
        let digit = ch.to_digit(10)?;

        if idx % 2 == 0 {
            sum += digit;
        } else if digit * 2 <= 9 {
            sum += digit * 2;
        } else {
            sum += digit * 2 - 9;
        }
    }

    Some(sum)
}

Another possibility which is similarly readable:

fn is_luhn_valid(input: &str) -> bool {
    let mut sum = 0;

    for (idx, ch) in input.chars().rev().enumerate() {
        let digit = match ch.to_digit(10) {
            Some(digit) => digit,
            None => return false,
        };

        if idx % 2 == 0 {
            sum += digit;
        } else if digit * 2 <= 9 {
            sum += digit * 2;
        } else {
            sum += digit * 2 - 9;
        }
    }

    sum % 10 == 0
}

@jtdowney
Copy link
Contributor Author

I am happy to provide a better doc use case but we will have to agree to disagree about the for loop. You seem to be discounting the number of cases we can't think up in this PR and excusing a clear lack of symmetry in the API with regards to Option vs Result.

@Centril
Copy link
Contributor

Centril commented Mar 21, 2019

I think unfortunately this applies to your Luhn implementation too. Nice job finding a way to express this all in a single method chain, and only one semicolon, but I think it comes out quite challenging to read. A reader looking to understand the implementation or code reviewer looking to confirm that it is correct would need to:

This seems like the usual matter of preference between an imperative vs. a functional style of writing the code. For example, I don't think doing sum += 3 times is great when you can extract it to a value first.

Trace through the Option gymnastics to figure out that |(idx, n)| { ... } returns an Option.

Even the signature of to_digits is more obvious because we are using ? inside a function that returns Option.

The functional style algorithm has n.map(|v| { ... }) which makes that clear.

Not get lost in 6 levels of indentation.

If this is a concern then extract the lambdas to the top level and you will have ~2 levels of indent.

What is nice about @jtdowney's version is that it encourages separation of concerns and extraction into smaller steps.

@dtolnay
Copy link
Member

dtolnay commented Mar 21, 2019

Thanks for the feedback -- I will give it some more thought under the understanding that @Centril finds the iterator chain easier to read.

For now, registering the comment about example code in the documentation, which I would still like to see:

@rfcbot concern more fleshed out example use case in documentation

@moshg
Copy link

moshg commented Apr 25, 2019

It might be appropriate to make the RFC of the API guideline about the equation of Option<T> and Result<T, ZST> or about the distinction between those and which we should use depending on the situation before stabilize this.
It might also be good the RFC about when we may implement traits.
e.g. whenever we have the natural semantics or when we can immediately understand the semantics of the functions using the trait with/without specifying the type parameters.

@jdahlstrom
Copy link

jdahlstrom commented May 2, 2019

I feel this should only be implemented if there were a single obvious semantics derivable from the types involved, or at least one clearly more intuitive than others. Which doesn't appear to be the case. I, as well, would intuitively expect None-filtering instead of shortcircuiting semantics here; the former is after all what eg. SQL and spreadsheets do.

What I would rather see is separate iterator adapters implemented for Iterator<Option<_>>: one unwrapping Somes until the first None, and another unwrapping all Somes, skipping Nones.

(Edit: the latter of course already exists and is spelled flatten! The most succinct if not the most intuitive way to spell the former is probably scan((), |_, opt| opt).)

@rfcbot
Copy link

rfcbot commented May 4, 2019

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.

The RFC will be merged soon.

@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 May 4, 2019
@Centril
Copy link
Contributor

Centril commented May 11, 2019

r? @sfackler Good deeds begets more =P

@rust-highfive rust-highfive assigned sfackler and unassigned SimonSapin May 11, 2019
@cristicbz
Copy link
Contributor

cristicbz commented May 14, 2019

@Centril @dtolnay I don't undertand why the "not sold on these impl"-s concern was resolved---in particular, the ambiguity around filtering Option-s which @dtolnay raised (my personal expectation is that Result short-circuits but Option filters.)

I accept that the filtering implementation may not be ideal either, but I don't get why we should commit to either if ambiguous. It's pretty easy to get the short-circuit behaviour with .ok_or(()) and the filtering behaviour with filter_map.

@Centril Centril modified the milestones: 1.36, 1.37 May 27, 2019
@Centril
Copy link
Contributor

Centril commented May 27, 2019

r? @dtolnay

@rust-highfive rust-highfive assigned dtolnay and unassigned sfackler May 27, 2019
@dtolnay
Copy link
Member

dtolnay commented May 28, 2019

@bors r+

@bors
Copy link
Contributor

bors commented May 28, 2019

📌 Commit 422a4c0 has been approved by dtolnay

@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 May 28, 2019
@dtolnay
Copy link
Member

dtolnay commented May 28, 2019

Bumped stable attrs to 1.37, since beta is currently 1.36.

@bors r+

@bors
Copy link
Contributor

bors commented May 28, 2019

📌 Commit 9f8d934 has been approved by dtolnay

@dtolnay
Copy link
Member

dtolnay commented May 28, 2019

@cristicbz the discussion persuaded me that these impls are valuable and that some people find it the most natural and readable way to express certain real-world logic. I don't know that I would ever use them, but it seems to be a matter of preference in terms of what people's background leads them to find more readable.

I don't think there is an ambiguity with filtering because filtering would not match the signature of these impl; it would produce T rather than Option<T>.

Centril added a commit to Centril/rust that referenced this pull request May 29, 2019
…r=dtolnay

Implement `iter::Sum` and `iter::Product` for `Option`

This is similar to the existing implementation for `Result`. It will take each item into the accumulator unless a `None` is returned.

I based a lot of this on rust-lang#38580. From that discussion it didn't seem like this addition would be too controversial or difficult. One thing I still don't understand is picking the values for the `stable` attribute. This is my first non-documentation PR for rust so I am open to any feedback on improvements.
Centril added a commit to Centril/rust that referenced this pull request May 29, 2019
…r=dtolnay

Implement `iter::Sum` and `iter::Product` for `Option`

This is similar to the existing implementation for `Result`. It will take each item into the accumulator unless a `None` is returned.

I based a lot of this on rust-lang#38580. From that discussion it didn't seem like this addition would be too controversial or difficult. One thing I still don't understand is picking the values for the `stable` attribute. This is my first non-documentation PR for rust so I am open to any feedback on improvements.
bors added a commit that referenced this pull request May 29, 2019
Rollup of 11 pull requests

Successful merges:

 - #58975 (Implement `iter::Sum` and `iter::Product` for `Option`)
 - #60542 (Add Step::sub_usize)
 - #60555 (Implement nth_back for RChunks(Exact)(Mut))
 - #60766 (Weak::into_raw)
 - #61048 (Feature/nth back chunks)
 - #61191 (librustc_errors: Move annotation collection to own impl)
 - #61235 (Stabilize bufreader_buffer feature)
 - #61249 (Rename Place::local to Place::local_or_deref_local)
 - #61291 (Avoid unneeded bug!() call)
 - #61294 (Rename `TraitOrImpl` to `Assoc` and `trait_or_impl` to `assoc`.)
 - #61297 (Remove LLVM instruction stats and other (obsolete) codegen stats.)

Failed merges:

r? @ghost
@bors bors merged commit 9f8d934 into rust-lang:master May 29, 2019
@timvermeulen
Copy link
Contributor

I realize I'm late to this, but is there any reason for having the OptionShunt type if we could forward it to the Sum and Product impls of Result as well?

iter.map(|x| x.ok_or(())).sum::<Result<_, _>>().ok()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.