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

Improve eagerly evaluating an iterator for its side-effects #44546

Closed
mattico opened this issue Sep 13, 2017 · 11 comments
Closed

Improve eagerly evaluating an iterator for its side-effects #44546

mattico opened this issue Sep 13, 2017 · 11 comments
Labels
C-feature-accepted Category: A feature request that has been accepted pending implementation. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@mattico
Copy link
Contributor

mattico commented Sep 13, 2017

0b_0101_001_1010 had a good idea on Reddit. I'll paraphrase it here.

The only way to eagerly consume an iterator, e.g., for its side-effects, on stable Rust is to call .collect or .count (.for_each is nightly only). Most beginners don't associate .count with this, so they use .collect, which is horrible.

There are a few things we could do to help make it more obvious what the correct thing to do is:

  • make .collect's result #[must_use]. If you are throwing .collect's result away, you are doing it wrong, and should not have used .collect in the first place. A loop or .for_each would do.
  • make .count's result #[must_use]: while .count is "efficent" (when compared to .collect) it doesn't convey what the code is actually doing
  • add an .eval() == .for_each(|_|): because .for_each(|_|) is also ugly
  • address this in docs: .collect means copy the results of the iterator into a collection, not "eagerly evaluate", or similar
@Mark-Simulacrum
Copy link
Member

Making collect must_use seems reasonable. I don't think it'll be a common accident in practice, but might help beginners out -- for _ in iter {} seems reasonable enough for the edge case where you have a side-effect causing iterator. IMO, it's an antipattern for an iterator to cause side effects anyway...

@cuviper
Copy link
Member

cuviper commented Sep 13, 2017

  • add an .eval() == .for_each(|_|): because .for_each(|_|) is also ugly

I suspect that most of the time you might reach for this, there will probably be a .map(f) right before that can become .for_each(f) instead.

IMO, it's an antipattern for an iterator to cause side effects anyway...

It's not an antipattern for an iter_mut(), where the primary side effect is to update items.

@ghost
Copy link

ghost commented Sep 13, 2017

Just a comment on naming... 🚲 🚲 🚲
I think .drain() would be more idiomatic than .eval().

@bluss
Copy link
Member

bluss commented Sep 13, 2017

.exhaust() is my favourite name (by @tbu-). On balance I think this method would be a win, although I am also hoping that .for_each will simply fill this role.

See also previous discussion in rust-itertools/itertools/pull/131

@tbu-
Copy link
Contributor

tbu- commented Sep 13, 2017

I think we should have a dedicated name for this, even for_each(|_| ()) is a bit annoying to type, to just evaluate the iterator.

@Mark-Simulacrum Mark-Simulacrum added C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Sep 17, 2017
@dtolnay
Copy link
Member

dtolnay commented Nov 14, 2017

As of #45379 there is collect::<()>().

let mut sum = 0;
(0..10).map(|x| sum += x).collect::<()>();
println!("{}", sum);

It is slightly more handy than exhaust() because you get all the usual folding you get with FromIterator, such as running through a sequence of Results until the first failure if any.

use std::io::*;
let res: Result<()> = (0..10)
    .map(|x| writeln!(stdout(), "{}", x))
    .collect();
assert!(res.is_ok());

Does that address the request in this issue or do we feel that there is more room for improvement?

@tbu-
Copy link
Contributor

tbu- commented Nov 14, 2017

I'd still say that it's annoying to type, compare collect::<()>() to exhaust().

@bluss
Copy link
Member

bluss commented Nov 15, 2017

And collect to unit is hard to discover, too, isn't it? (It is kind of automatically available if one calls collect(); maybe that would be the only intuitive entry.)

@dtolnay dtolnay added C-feature-accepted Category: A feature request that has been accepted pending implementation. and removed C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Nov 19, 2017
@dtolnay
Copy link
Member

dtolnay commented Nov 19, 2017

I would be prepared to consider a PR for Iterator::exhaust.

@kennytm
Copy link
Member

kennytm commented Nov 19, 2017

So basically revive #45168 and change the name to exhaust 🙂

@varkor
Copy link
Member

varkor commented Apr 16, 2018

Closed by #48945, where it was decided that iter.for_each(drop) was a better alternative to a new iterator method.

@varkor varkor closed this as completed Apr 16, 2018
winksaville added a commit to winksaville/rustlings that referenced this issue Dec 16, 2020
iterators4.rs was particularly instructive.

I started out by searching for the initial C recursion and for loop
versions and tried to use a range iteration but couldn't figure the
syntax. So I created the first 7 answers and then used hint.

I then seached for: how to use iterator for side effects only
 https://www.google.com/search?q=how+to+use+iterator+for+side+effects+only+in+rust

And saw this:
 rust-lang/rust#44546 (comment)
And:
 rust-lang/rust#44546 (comment)

And with that I implemented Answer 8

But still wasn't happy as I felt there must be a better way where I didn't
need to throw away things. So I looking at the iterator documentation:
 https://doc.rust-lang.org/std/iter/trait.Iterator.html

I saw "fold" and low and behold that was exactly what I needed! Then I
saw "fold_first" and it was "simpler" as the first value in the range
is the initial value for the accumulator so fold_first only has one
parameters not two. But fold_first returns an Option which means I need
to return the unwrap(). Anyway both Answers 9 and 10 are simpler then
any other solution!

I do wonder which is fastest.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-accepted Category: A feature request that has been accepted pending implementation. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants