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

Add a foreach() method to std::iter::Iterator #1064

Closed
wants to merge 3 commits into from
Closed

Add a foreach() method to std::iter::Iterator #1064

wants to merge 3 commits into from

Conversation

withoutboats
Copy link
Contributor

This is a re-opening of this RFC which was not merged, per the discussion in this thread.

Rendered

```
fn foreach<F>(self, mut f: F) where F: FnMut(Self::Item) {
for item in self { f(item); }
}
Copy link
Member

Choose a reason for hiding this comment

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

it should take &mut self then it can be used on A) Iterators that can't be moved out from B) Iterator trait objects. Same functionality.

Copy link
Member

Choose a reason for hiding this comment

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

Last I checked, the idea was for consuming iterator to take by value for convenience, since the by-ref behavior can be recovered with the by_ref() adapter

Copy link
Member

Choose a reason for hiding this comment

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

for trait objects with manual cast to &mut Iterator rather than .by_ref() in that case.

What is the convenience difference? I think it should be callable everywhere the self version is.

@gsingh93
Copy link

+1, I've wanted this multiple times in the last few days.

@nagisa
Copy link
Member

nagisa commented Apr 16, 2015

So far none of the points arguing against the RFC have become false:

  • There’s a more general version of this function already available in the standard library as well as various chains that produce the desired behaviour;
  • THere’s the for-loop construct;
  • itertools package still provides a convenience wrapper for the for-loop.

@withoutboats
Copy link
Contributor Author

There’s a more general version of this function already available in the standard library

What function are you referring to? fold() is more general than every iterator method in std (because all possible iterator methods are degenerate cases of fold()).

The text of this RFC is intended to respond to the other points: while there are other ways to do this, they are unidiomatic, & this should not be an unidiomatic thing to do. Of course those points against remain true - so do all points in favor (none of these points are contingent on anything). This RFC has been opened because I think closing the prior RFC was a mistake.

@gsingh93
Copy link

  • Using other functions but discarding their results is ugly, IMO. Using a method with a clear name and that returns no result would be much better.
  • for already exists, but if we don't include this because of that we're essentially saying that Rust in general isn't in favor of sugar, which I believe is not the case (we just accepted the std::iter::once RFC, and that's also a convenience method).
  • The itertools has over 1,700 installs based on the stats on Cargo. Obviously there's no way to tell how much people use the equivalent of foreach in that library, but it seems it is relatively popular.

@huonw
Copy link
Member

huonw commented Apr 16, 2015

What function are you referring to? fold() is more general than every iterator method in std (because all possible iterator methods are degenerate cases of fold()).

The lack of laziness in Rust means this isn't true: fold is eager but map/filter/etc. are not. In any case, any and all are more "direct" generalisations of foreach, in that they have a closure called only on each element (like foreach) but also support break.

@bluss
Copy link
Member

bluss commented Apr 16, 2015

The itertools has over 1,700 installs based on the stats on Cargo.

Downloads. Every version must be downloaded anew. Given the spikes and their timings in the download chart, I'm almost suspecting that a significant part of them are from brson's cargo build runs. (Many other crates have the same pattern, spikes on exactly April 3 and April 10).

Edit: Some research shows one .intersperse() in yup-oauth2 has caused basically hundreds of downloads of its own.

@withoutboats
Copy link
Contributor Author

In any case, any and all are more "direct" generalisations of foreach, in that they have a closure called only on each element (like foreach) but also support break.

I hadn't considered doing this before, but it seems like a mistake: the value of chaining iterator methods to me is that they allow for a separation of concerns between each method call; it seems more appropriate to chain a take_while() with a foreach() than to have an all() with side effects.

@steveklabnik
Copy link
Member

I want this feature, but agree with @nagisa that nothing has really changed since we closed the last version of this RFC.

@bluss
Copy link
Member

bluss commented Apr 16, 2015

A change since last RFC: It was added to itertools, so it is easily available if you accept a dependency, but it is still being requested for libstd by users.

To me it seems like very basic functionality that is great to include in rust because it makes code easier to write and read. What I understand there is no rush here, we should schedule it not for 1.0 but for a later relase.

@withoutboats
Copy link
Contributor Author

All of the reasons in the prior RFC will always be true and are acknowledged in the text of this RFC. I understand not wanting to incentivize revisiting closed RFCs, I understand not wanting to add a whole bunch of TIMTOWTDIs to std, and I know that this RFC looks like yet-another-bikeshed, but I feel very strongly that the prior decision was mistaken in this case.

The lack of this feature makes me feel like I am doing something wrong when I am doing something totally fine. This feature is not only a convenience, it also enables Rust to maintain its very strong correlation between things that are convenient and things that are good practice.

@rkjnsn
Copy link
Contributor

rkjnsn commented Apr 16, 2015

I would like this. It feels quite strange to emphasize iterator chaining and then not provide such a method. If one is doing a complex sequence of actions with each value, using a for loop makes sense, but for situations where one is doing something simple like calling a method on each element, the syntax proposed here is much nicer.

@dgrunwald
Copy link
Contributor

Pretty much all iterator chaining methods so far are executed for their results, and usually don't have side effects apart from consuming the iterator and constructing their result.
foreach would be the only method executed for its side effects -- I think it's better to use a for loop when writing imperative code.

Note that C# doesn't have a ForEach extension method for the same reason.

@alexcrichton alexcrichton self-assigned this Apr 16, 2015
@lilyball
Copy link
Contributor

I do not agree that we should have a foreach() method, and I also disagree that a previously-closed RFC should be reopened unless something significant has changed (and nothing significant has).

@withoutboats
Copy link
Contributor Author

foreach would be the only method executed for its side effects -- I think it's better to use a for loop when writing imperative code.

First, inspect() exists, and is necessarily only executed for its side effects, so this is already untrue. However, foreach() is only "executed for its side effects" under a purist definition of "side effect" - some language features only provide an imperative API, and there is no way to consume the values into them except by imperative code in which the programmer's goal is technically a "side effect." It is exactly this sort of imperative code that is driving the blessed consumers internally.

If this RFC is closed without acceptance, I hope it will be clarified what the exclusion of foreach() is intended to mean. Eg:

  • Is it considered idiomatic to follow a significant chain of iterator methods with a for loop? Is this not perceived as less readable than chaining methods alone?
  • Is it considered unidiomatic to consume an iterator other than by means of one of the blessed consumers? If so, why?

@pythonesque
Copy link
Contributor

Certainly, there are many valid reasons not to include it, but I have wanted this on occasion, especially when I started using Rust. At this point I don't really want it and am comfortable just using a for loop in these situations, especially given the added power therein (particularly, being able to use things like try!() more easily); however, I have no idea if that's simply because I've gotten used to it.

@alexcrichton
Copy link
Member

Thanks for the RFC @withoutboats! As others have pointed out, however, this RFC was previously closed and there hasn't be a whole lot of new developments in the meantime. This sort of addition should be backwards-compatible to do in the future, so we're not locking ourselves into never providing this for now. The standard library is in general quite conservative in the functionality it exports today as we prepare for the 1.0 release. As a result I'm going to close this for now, but if we have new developments arise we can definitely revisit!

@withoutboats
Copy link
Contributor Author

What new development would cause you to reopen this rfc? Rust will always have for loops & folds and the itertools crate will always compile.

To be honest, it is very frustrating to have a feature that is of great practical value to me rejected for philosophical and procedural reasons. Not trying to be dramatic but this feels kafkaesque.

-----Original Message-----
From: Alex Crichton notifications@github.com
To: rust-lang/rfcs rfcs@noreply.github.com
Cc: Lee Aronson lee@libertad.ucsd.edu
Sent: Fri, 17 Apr 2015 5:10 PM
Subject: Re: [rfcs] Add a foreach() method to std::iter::Iterator (#1064)

Closed #1064.


Reply to this email directly or view it on GitHub:
#1064 (comment)

@gsingh93
Copy link

Can it at least be explained why a similar addition, the std::iter::once RFC, got accepted so easily, but this RFC is getting rejected? What exactly is the difference when both are providing sugar for things you can already do?

@lilyball
Copy link
Contributor

@withoutboats

To be honest, it is very frustrating to have a feature that is of great practical value to me rejected for philosophical and procedural reasons.

Given how easy it is to add the itertools crate to your project, is it really all that much of a problem? You obviously care about this functionality, but it seems not very many people do (certainly none of the code in the Rust project uses foreach(), otherwise it would exist, and speaking personally, none of the third-party Rust code I've seen uses it either). Heck, even inspect() is basically never used, but it exists primarily as a debugging aid (so you can e.g. add a log to every step of an iterator) so its lack of common use is not surprising.

What new development would cause you to reopen this rfc?

If the foreach() method from itertools becomes very widely-used in third-party code, that would be grounds to reevaluate whether it's something that's worth adding to the standard library. That's also true in general of itertools, if some particular API it provides ends up being significantly popular, and the API is determined to be stable enough to freeze, then it would warrant being considered for inclusion in the standard library.

@gsingh93

Can it at least be explained why a similar addition, the std::iter::once RFC, got accepted so easily

I wasn't involved with that RFC, but std::iter::once provides a commonly-useful function, one that was technically already provided by Option but was very non-intuitive and hard for people to discover. More generally, the idea of an iterator that yields a single element is very non-controversial, whereas the idea of having a method on Iterator whose sole job is to evaluate an iterator purely for the side-effects of evaluating it is, as you've seen, a somewhat controversial suggestion, as it's not currently considered idiomatic to evaluate iterators purely for their side-effects (the idiomatic approach being to use a for loop instead).

@bluss
Copy link
Member

bluss commented Apr 18, 2015

@kballard Without .foreach(), the recommendation to replace it with .map().count() will be common, which results in.. worse rust code.

@alexcrichton Nothing suggested this was targeting Rust 1.0.

@lilyball
Copy link
Contributor

lilyball commented Apr 18, 2015 via email

@withoutboats
Copy link
Contributor Author

I would be very surprised if the foreach() method in itertools becomes commonly used. Indeed, what will happen is that instead people will use a for loop or count(). In doing so, at best they will write less readable code and at worse they will be discouraged from using the excellent framework that std::iter provides for structuring your code.

Consider someone new to Rust, who has not internalized that iterators are lazy, writing something akin to this:

(0..100).filter(|x| x % 2)
        .map(|x| (x * 256).to_string())
        .filter_map(|string| string.find('0'))
        .map(|index| { tx.send(index).ok(); })

It does not work! So they seek out advice, comprehend that their problem is that the iterator will never be consumed, and now write this:

for index in (0..100).filter(|x| x % 2)
                     .map(|x| (x * 256).to_string())
                     .filter_map(|string| string.find('0')) {
    tx.send(index).ok();
}

This seems unambiguously less readable than the prior example. The code is now indented oddly, multiple language constructs are used which are superficially dissimilar, and the binding used in the final stage of transformation is declared at the beginning. I would be - and was - very unsatisfied with this outcome. We would be lucky if this is as far as it goes, though: being unhappy with this weird method-chain/for-loop combination, our new Rust user could refactor further:

for x in 0..100 {
    if x % 2 {
        let string = (x * 256).to_string();
        if let Some(index) = string.find('0') {
            tx.send(index);
        }
    }
}

There, that at least is familiar and consistent. But also, in my opinion, far worse! The value of std::iter has nothing to do with laziness or purity or anything mathematical, but the kind of code that it enables: it encourages the programmer to decompose their problem into distinct steps which can be modeled as data -> data transformations. This is indispensable, but the ``std::iter` framework is not composable with any interface that is not modeled as an object or collection. This is not the majority of use cases, but it is still a very real use case that is not supported currently. Of course, I am now just reiterating what I wrote in the RFC.


It is not accurate to present the prior decision regarding this feature as inexorably determined by a set of facts, such that it would only be appropriate to raise this RFC if those facts 'materially changed.' The facts enumerated cannot materially change, as I have repeated several times. What is new about this RFC is the perspective on the question that I have presented, which I do not feel has been addressed or even responded to in this conversation. I understand that every decision cannot be constantly relegislated, but there is no established procedure for re-opening a closed RFC, so closing this pending "new developments" - which I read as "until Rust no longer supports the for loop syntax" - is dismissive at best. It has thoroughly disheartened me from participating in the RFC process.

If the issue is just that the core team is too focused on May 15th to consider this issue, I do not begrudge you that at all! Please then postpone this RFC and I will bring it up again once the polishing sprint is over, but you are setting precedence that this feature is unnecessary solely on the grounds that I did not contribute to the conversation before aturon closed the prior RFC in March.

@alexcrichton
Copy link
Member

@withoutboats

What new development would cause you to reopen this rfc?

To add to what @kballard has already said, the opinion around this RFC hasn't really changed much in the past month (as there's not strong consensus that it's needed), but if consensus were reached then we could reconsider.

To be honest, it is very frustrating to have a feature that is of great practical value to me rejected for philosophical and procedural reasons

Unfortunately there are a lot of features that are desired in Rust and the standard library, but we aren't able to accommodate all of them. Adding new features needs to be done with care to ensure we don't evolve too hastily and have an appropriate amount of time to evaluate the current design we have today.


@gsingh93

Can it at least be explained why a similar addition, the std::iter::once RFC, got accepted so easily, but this RFC is getting rejected

To add to what @kballard said, I believe std::iter::once also had much stronger consensus than this RFC (for many of the reasons he listed)


@bluss

@alexcrichton Nothing suggested this was targeting Rust 1.0.

I agree! I suppose I should have been more general in describing the current design of the standard library as conservative and less emphasize the 1.0 aspect. We do plan to grow the standard library but we intend to do so purposefully and carefully to make sure it doesn't grow too fast.


@withoutboats

I'm sorry for your frustration here, but I'd also like to emphasize that one of the greatest features of Rust is how extensible it is. Not having a feature in the standard library does not mean that it is impossible to write it in Rust, as itertools has shown. Additionally, having Cargo as a package manager means that it's super easy to add itertools as a dependency for all of your own projects. I do understand that it's not the same as being in the standard library, but the situation for being able to use a foreach function isn't necessarily so dire.

I also forgot our normal procedure of making a postponed issue for RFCs that just aren't being accepted at this time, so I've opened an issue on this topic: rust-lang/rust#24566

@steveklabnik
Copy link
Member

I opened an RFC issue rather than a rust-lang/rust issue: #1070

@lilyball
Copy link
Contributor

lilyball commented Apr 19, 2015 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.