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

Documentation of Iterator flatten() improvement #82687

Closed
ralpha opened this issue Mar 1, 2021 · 5 comments · Fixed by #105034
Closed

Documentation of Iterator flatten() improvement #82687

ralpha opened this issue Mar 1, 2021 · 5 comments · Fixed by #105034
Assignees
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools A-iterators Area: Iterators C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@ralpha
Copy link

ralpha commented Mar 1, 2021

While linting my code I came across manual_flatten which was triggered in my code.

This suggests the change from:

for n in x {
    if let Some(n) = n {
        // something here
    }
}

Into:

for n in x.into_iter().flatten() {
    // something here
}

Both are valid code with same result.

But the rust docs on flatten() does not state that this is expected behavior. Yes, this is stated here.
But this was not strait forward (in my opinion) to find. It took me writing a whole bug report and looking a bunch of thing up to figure this out. (and I'm not even new to Rust anymore)

I would suggest adding some example code to make his explicit. Here is a suggestion:
(after the "Mapping and then flattening: <code>...</code>" section)

Flattening also works on other types like Option and Result:

let students_in_seats = vec![Some("Hasan"), Some("Sarah"), None, Some("Tim")];
let students_present: Vec<_> = students_in_seats.into_iter().flatten().collect();
assert_eq!(students_present, vec!["Hasan", "Sarah", "Tim"]);

(or something similar)

This makes both this behavior cleared, easier to find and exposes people to code like this so they are less surprised if clippy starts warning them.

It looks like this behavior might be more common in the future too.
rust-lang/rust-clippy#6061
rust-lang/rust-clippy#6676

@Nicholas-Baron
Copy link
Contributor

Nicholas-Baron commented Mar 2, 2021

@rustbot label T-doc T-libs.

@rustbot rustbot added A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Mar 2, 2021
@JohnTitor JohnTitor added A-iterators Area: Iterators C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Mar 2, 2021
@cuviper
Copy link
Member

cuviper commented Mar 11, 2021

I'm not sure I like clippy's suggestion, personally, but I think your suggested documentation is reasonable. I would also point out that mapping to Option/Result followed by flatten (or a similar flat_map) is better written as filter_map (with ok() for Result).

@ralpha
Copy link
Author

ralpha commented Mar 11, 2021

I was also confused when I saw the suggestion. But once I realizing what is was doing it sounded in line with things like unwrap. That does not mean that I like it, but at least people know it is intended that way and not a side-effect.

@nospam3089
Copy link

I encountered the same clippy warning when iterating over Results, and got perplexed. With the current state of documentation, I personally perceive the warning as suggestion to write obfuscated code hiding the fact that error checking is missing.

Adding the documentation change suggested by @ralpha would make it much easier to realize what flatten() does. Maybe it might be made even more clear how it works if phrasing the first line something like this:

Flattening works on all types implementing Iterator, like Option and Result:

Me too am no longer new to Rust, and thought I had a proficient understanding of flatten() after having used it to unroll nested arrays. Likely I'll not be the last person to be surprised it can also be used to discard Err values.

@HintringerFabian
Copy link
Contributor

@rustbot claim

compiler-errors added a commit to compiler-errors/rust that referenced this issue Jan 9, 2023
…flatten_doc, r=cuviper

Add example for iterator_flatten

Adds an Example to iterator_flatten
Fixes rust-lang#82687
@bors bors closed this as completed in 002eccc Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools A-iterators Area: Iterators C-enhancement Category: An issue proposing an enhancement or a PR with one. 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.

7 participants