Do we want `forEach`?

Hi everyone,

As mentioned in some posts (here for instance), the forEach syntax is not available for AsyncSequence, which lowers the consistency and parity with Sequence.

@Philippe_Hausler is this a discussion still in progress, or has it been decided to not go down that road (you seemed to be in favour of implementing it)? Perhaps implementing it in this repo instead of the standard library would lower the apprehensions.

public extension AsyncSequence {
  func forEach(_ block: (Element) async throws -> Void) async rethrows {
    for try await element in self {
      try await block(element)
    }
  }
}

Thanks.

Content warning: heavy editorializing

Speaking personally: forEach is a blight on Sequence and I wish it didn’t exist. People use it arbitrarily instead of for…in for no apparent good reason. Frequently the answer they give for why is “it’s swifty” or “it’s more functional”. No. Just use a for loop.

forEach certainly has its uses. I believe the original intent was to stick on the end of a long composed chain of operations i.e. myArray.lazy.map(foo).filter(bar).forEach. This is reasonable… except often these long chains are a bad idea. Less experienced developers see them in code, try to replicate them, and then get tied up in knots with type mismatches. It would be far better for them to break the code up and see the individual types, but they can’t because of the chain. Maybe tooling could fix this but… does it need fixing? “Doctor doctor, it hurts when I compose too many things”. Well, don’t compose so much.

The main use I reach for in forEach is debugging… it would be nice sometimes to be able to stick .forEach(print) on the end of an expression. Unfortunately print has default arguments which don’t work with point free style so you have to write { print($0) } which kind of spoils the fun.

I think there’s another reason why forEach on async sequence specifically isn’t a great idea and that’s that it obscures the awaiting done by it. Unlike with synchronous sequences you may find execution paused for a long time awaiting the next element, and a proper for loop will make what is going on clearer in the debugger.

Anyway in conclusion sorry for the rant and please don’t add it, deprecate the one on Sequence instead (this will not happen but I can dream).

37 Likes

+1 for consistency

2 Likes

The biggest semantic difference between for...in and forEach is that no matter what's inside the loop once you hit the line right after the loop you know that the forEach has iterated through all the elements, while with for...in there is no such guarantee (due to break).

9 Likes

I think forEach brings some diversity that certain group appreciates. It's a matter of personal preference and I do imagine some people would "hate" it but other would love it too. I reach out to forEach in certain situations when I think it helps the readability.

Option 1 is:

for item in Timer(...)
  .map(...)
  .filter(...)
  .compactMap(...)
  .myFunc(...) {
    ... my loop code ...
}

and Option 2 is:

Timer(...)
  .map(...)
  .filter(...)
  .compactMap(...)
  .myFunc(...)
  .forEach {
    .. code ...
  }

I think in some situations the latter better expresses the fact that I'm starting with a timer that produces some values, that I process, etc. etc. and finally in the forEach I don't even need to define a variable because it's clear I'm processing each of the resulting items.

The for syntax in this case forces me to have a variable that I don't want to name and inserts item right at the beginning where it doesn't play any role.

I like having and using both, depending on the code I'm working on.

9 Likes

Both options are not very readable IMO and are also harder to debug, inspect the types etc. Option 3 is to do the chain, assign it to a variable with a meaningful name, and then iterate over it. “I don’t even need a variable” seems like an anti-pattern to me that forEach encourages and this is a lot of what is bad about it.

If your for loops are so large that knowing this at a glance (i.e. is there a break in there?) is a problem, something else is wrong. But as a counter to this: return inside large bodies is an accident waiting to happen, because if someone mistakes the forEach for a proper loop, it will not do what they think it does.

Consistency is a means, not an end (I.e. consistency amongst a family helps with understanding what all the family of APIs does). So it shouldn’t really used as a justification to add an API. Either forEach is a good API or a bad one (I think it’s bad) and that should determine whether it should be added.

9 Likes

Personally I am swayed in both directions; the contradiction does however reveal some interesting points. Materially we have two distinct means here; consistency (with Sequence) to shove a function at the tail of a application, and a desire for a certain aesthetic of personal readability.

The objections about the type solver being overburdened with the potential failures in my view exposes two distinct issues - one; that we have a soft spot performance wise not just for forEach but for ANY trailing closure (including other chains like map/filter/reduce/flatMap etc), and two; the solver has a contrary direction to how folks write things (from my limited understanding) that it progresses from left to right in the statement whereas folks reason about things from right to left. This reminds me of the debate about build systems; "recursive make considered harmful" - in that a bottom up system is at odds with a top down reasoning.

The bigger question is; what should the signature of this method actually be?
func forEach(_ apply: (Element) async throws -> Void) async rethows
or...
func forEach(_ apply: (Element) async -> Void) async rethrows
or...
func forEach(priority: TaskPriority? = nil, _ apply: @Sendable @escaping (Element) async -> Void) -> Task<xxx>

*the last one really being sink... which has its own can-of-worms that I am definitely not interested in tackling right now.

Truth be told: the "discussion" was partially me deciding that other things to tackle were higher priority after talking with folks about it. Shy of the handiness of being able to pass a function in (not a closure) I wasn't really able to determine what if any advantage it really serves even in the synchronous form.

Given a choice between taking the time to discuss, review, and implement things like the functionality of share or switchToLatest and forEach I would gladly let it fall by the wayside again - 'cause why have two ways of doing the same thing?

Perhaps my personal opinion is: if an overwhelming majority of folks really want it I am not going to stand in the way, but I am also not going to expend a lot of effort to prioritize it.

4 Likes

If such code is difficult to debug or to inspect its types, the tooling is deficient.

I very much do not agree that supplementary tooling should drive the way in which we express programs, and in fact my feeling is the complete opposite. If forEach is a more appropriate fit for a developer's particular circumstance, they should use it as much as they like. There should be a constant pressure on tooling to improve and meet the needs and preferences of contemporary developers.

Ultimately Swift is supposed to support all programming styles. If developers want to use forEach, I say we give it to them, gladly, and that we do not suggest that they might be "holding it wrong".

9 Likes

The use case I have in mind is this:

  • I use forEach if i know the code should iterate through all elements no matter what's in the loop now or in the future. That's an additional restriction, in a way forEach compares to for...in like for/while compares to if+goto.
  • later on another developer or myself a few month later forgets about that initial design goal and tries to do the early break - at which point the compiler helps and raises an error (if there was for...in it wouldn't be the case).
  • your comment about return is valid, and sometimes less experienced or, say, tired developer will think it is doing something else compared to what it is doing in the forEach context (effectively continue in for...in terms). review / unit testing / testing would help here to catch that bug.

imho forEach is as useful as map or filter. (yes, in principle we could live without those either and implement them manually every time...)

4 Likes

If we’re having a religious argument, I’m strongly on the side of .forEach. One person’s “this is ugly code” is another person’s “this is much more readable code.” I like the constraints of .forEach, and knowing that it’s going to process everything and there’s not extra BS happening (compared to a general-use for() loop).

If I glance at a .forEach there’s a bunch of stuff I just instantly know about the code, which I think is super-valuable.

5 Likes

Except this isn’t really true. return in a forEach is a for-loop’s continue, and functions that throw exit early without processing all elements, like a break.

9 Likes

Fair. But I still think as a capper for a chain of transforms, .forEach can’t be beat. I personally would rather have the UNIX pipe metaphor over a BASIC ‘assign a variable and use it later’ metaphor.

8 Likes

Serious question: how often would we expect the use of .forEach to be actually just wrapped in a task?

e.g.:

let doingStuff = Task {
  try await someLongDefinition.map { ... }.filter { ... }.forEach(doStuff)
}
1 Like

I use it a lot. Please don’t get rid of it.

Ex: aBunchaViewsToHide.forEach { $0.isHiddden = true }

The discussion is not about whether to get rid of it, it's about whether to add an AsyncAlgorithms version of it.

That said, I don't understand what you're gaining from that. Isn't that exactly the same as a for loop, except with a less clear variable name ($0)?

It's more than a less clearly named for in ... loop. It's useful and flowy at the end of a long chain of higher-order functions such as filter, map, ..., as Wil described. I believe it's actually more clear than assigning the result to a throwaway variable if all you're doing is iterating over the result.

If anything, I'd be in favor of giving .forEach more power by having it return its input sequence as a discardable result, so .forEach could be used in the middle of such a transformation chain as well.

I meant the specific case in the post I was replying to… but I see Discourse hid the reply relationship because they were adjacent. Unhelpful of it in this case, ah well.

2 Likes

I'm not sure what you're asking here. Why is wrapping in a Task relevant here? I would expect this forEach to be equally applicable whether wrapped in a Task or not, but I would think most use would eventually (in the medium term) be in naturally async methods.

Personally, I think we should have forEach. Not just for parity with Sequence, but because it lends itself well to one of the fundamental ways of using Swift, chaining. You can make the same arguments used against forEach against pretty much the entire rest of Swift's collection APIs. Doesn't make them bad, just means users need to know you use them differently than raw loops. So far that doesn't seem to have been an issue. That a tool isn't useful in 100% of cases isn't a reason not to have the tool.

In fact, I'd go farther. I'd say forEach's signature should change to forEach(...) -> Self, so that later operations can be chained. This is a nice increase in utility for a very small change.

4 Likes

The reasoning was more so to understand how folks intend or see its use. It was a pattern that I have been seeing emerge when using some of these things in app contexts and I was wondering if some of the use cases folks were seeing were similar to that of .sink (but perhaps a better name).

How would that even work? AsyncSequence is not double-pass capable. So unless it is an escaping "diagnostic window for side effects" (which I have much stronger objections that Ben's objection to .forEach on... mainly because it makes thread safety and self consistency virtually impossible because it breaks the monad's encapsulation), im not sure I follow on how that could be achieved.

forEach was useful with UIKit due to the abundance of classes, but it will continue on the trip towards complete worthlessness unless two things are fixed:

  1. Cannot reference 'mutating' method as function value.

I.e. it's only good when you already have an existing named closure to use with it.

[1, 2, 3, 4].forEach(&myStruct.add) ?

2 Likes