Add rule to prefer for loops over functional forEach { ... } method

[calda]

Summary

This PR proposes a new rule to prefer using for loops over the functional forEach { ... } method, unless using forEach as the last element in a functional chain.

Autocorrect is implemented in the new forLoop SwiftFormat rule, which was added in nicklockwood/SwiftFormat#1490.

// WRONG
planets.forEach { planet in
  planet.terraform()
}

// WRONG
planets.forEach {
  $0.terraform()
}

// RIGHT
for planet in planets {
  planet.terraform()
}

// ALSO FINE, since forEach is useful when paired with other functional methods in a chain.
planets
  .filter { !$0.isGasGiant }
  .map { PlanetTerraformer(planet: $0) }
  .forEach { $0.terraform() }

Reasoning

For loops are more idiomatic than the forEach method, and are typically familiar to all developers who have experience with C-family languages.

For loops are also more expressive than the forEach method. For loops support the return, continue, and break control flow keywords, while forEach only supports return (which has the same behavior as continue in a for loop).

Please react with 👍/👎 if you agree or disagree with this proposal.

[calda]

Adding a link to a comment about forEach that I just found from Ben Cohen (core team member who leads work on the standard library): https://forums.swift.org/t/do-we-want-foreach/56929/2. It's maybe a bit harsh, but it's interesting to see his perspective on this (and that he wishes they could deprecate forEach!).

[bachand]

Adding a link to a comment about forEach that I just found from Ben Cohen (core team member who leads work on the standard library): https://forums.swift.org/t/do-we-want-foreach/56929/2. It's maybe a bit harsh, but it's interesting to see his perspective on this (and that he wishes they could deprecate forEach!).

Thanks for sharing this. It seems like our style guide is aligned with Ben's perspective, which is validating IMHO.

[bachand]

NIce catch.

[bachand]

Asking to learn (and I apologize for the lazy Q, as I know I could try this out...): at what point does the SwiftFormat rule implementation allow forEach(…)? Is it once the expression is multiple lines? I'd love if you want to link me to the code that makes this decision, if that's straightforward to do.

[calda]

By default the SwiftFormat rule avoids applying the rule in two cases:

1. If the subject of the forEach call includes line breaks (handled here with an explicit check). For example, it would be weird to convert:

planets
  .filter({ !$0.isGasGiant })
  .map({ PlanetTerraformer($0) })
  .forEach({ $0.terraform() })

to a for loop formatted something funny-looking like

for planet in planets
  .filter({ !$0.isGasGiant })
  .map({ PlanetTerraformer($0) })
{
  print(planet)
}

2. If the subject of the forEach call uses trailing closure syntax, like planets.filter { !$0.isGasGiant }.forEach { print($0) } (handled here by virtue of } not being handled in one of the switch cases -- should realistically be called out more explicitly).

We exclude this case because converting this to a for loop causes a new warning to be emitted:

// warning: trailing closure in this context is confusable with the body of the statement; pass as a parenthesized argument to silence this warning
for planet in planets.filter { !$0.isGasGiant } {
  print(planet)
}

A typical use case of chained functional operations hits both of these rules.

[calda]

If the subject of the forEach call uses trailing closure syntax, like planets.filter { !$0.isGasGiant }.forEach { print($0) } (handled here by virtue of } not being handled in one of the switch cases -- should realistically be called out more explicitly).

I improved how this is handled in the code here: nicklockwood/SwiftFormat@4d92ba2

[bachand]

Thank you for explaining @calda and for that follow up!

[bachand]

👍

[swiftal64]

I would prefer to give the developer the option of choosing the best tool for the job.

For example, occasionally I group things in logical chunks, and one-liners can sometimes help with organization:

let stars = StarFactory.generate(1)
let planets = PlanetFactor.generate(8)
let moons = MoonFactory.generate(290)

view.addSubview(stars)
view.addSubview(planets)
view.addSubview(moons)

stars.forEach { $0.orbit() }
planets.forEach { $0.orbit() }
moons.forEach { $0.orbit() }

[calda]

@swiftal64, perhaps for loops can still be used in this way:

for star in stars { star.orbit() }
for planet in planets { planet.orbit() }
for moon in moons { moon.orbit() }

This sort of usage would still be consistent with the style guide.