nicklockwood / SwiftFormat

A command-line tool and Xcode Extension for formatting Swift code
MIT License
7.63k stars 623 forks source link

`preferForLoop` rule causes build error #1607

Open bookii opened 5 months ago

bookii commented 5 months ago

In 0.53.0, preferForLoop rule was added, but it causes build errors.

For instance, suppose there are properties of Implicitly Unwrapped Optional types like this:

@IBOutlet private var oneLabel: UILabel!
@IBOutlet private var twoLabel: UILabel!
@IBOutlet private var threeLabel: UILabel!

And for those arrays, if there is a forEach syntax like this:

// before conversion
[oneLabel, twoLabel, threeLabel].forEach { label in
    label.isHidden = true
}

In that case, the preferForLoop rule converts it into a for in syntax like the following:

// after conversion (this causes build error)
for label in [oneLabel, twoLabel, threeLabel] {
    label.isHidden = true
}

However, this causes a build error. The correct conversion should be as follows:

// conversion that does not cause build error
for label in [oneLabel, twoLabel, threeLabel] {
    label?.isHidden = true
}

Furthermore, ideally, it should be converted like this:

// ideal conversion
for label in [oneLabel, twoLabel, threeLabel] as [UILabel] {
    label.isHidden = true
}

Environment: SwiftFormat 0.53.0 Xcode 15.1

nicklockwood commented 5 months ago

I'm not sure I can do anything about this in the general case. For locally defined variables in principle this could be detected, but for an array defined in another file I wouldn't know that the values were optional.

superarts commented 5 months ago

It's not directly related with OP's issue, in my case swiftlint complains about the formatted code:

Unused Enumerated Violation: When the item is not used, `.indices` should be used instead of `.enumerated()` (unused_enumerated)

image

     func didSelectITem(at: Int) {
-        profilePrivacyRows.enumerated().forEach { index, _ in
+        for (index, _) in profilePrivacyRows.enumerated() {
             profilePrivacyRows[index].isSelected = at == index
         }
     }

Just FYI.

nicklockwood commented 5 months ago

@superarts SwiftLint is correct though - enumerated() isn't needed here (and wasn't needed for the foreach loop either). You should remove it.

It's just a limitation of SwiftLint that it can't detect this issue for forEach loops as well as regular for loops or it would have flagged it earlier.

(Ideally SwiftFormat could also do this enumerated()/indices substitution, but that would require a new rule.)

superarts commented 5 months ago

Thank you! Could you point a link about why for loop is preferred than forEach please? Is it because of the flexibility for loop provides?

nicklockwood commented 4 months ago

Thank you! Could you point a link about why for loop is preferred than forEach please? Is it because of the flexibility for loop provides?

@superarts Sorry I forgot to reply here. I struggled to find a single resources that explains all the reasons, but here are some arguments from well-known community members:

https://forums.swift.org/t/do-we-want-foreach/56929/2 https://forums.swift.org/t/remove-foreach/322