Rewrite for_where, adding allow_for_as_filter config

Fixes #4040

CHANGELOG.md

@@ -167,6 +167,11 @@
 
 * Print violations in realtime if `--progress` and `--output` are both set.  
   [JP Simard](https://github.com/jpsim)
+* Rewrite `for_where` rule with SwiftSyntax, adding a new configuration
+  `allow_for_as_filter` to allow using `for in` with a single `if` inside
+  when there's a `return` statement inside the `if`'s body.  
+  [Marcelo Fabri](https://github.com/marcelofabri)
+  [#4040](https://github.com/realm/SwiftLint/issues/4040)
 
 #### Bug Fixes
 

Source/SwiftLintFramework/Rules/Idiomatic/ForWhereRule.swift

@@ -1,7 +1,7 @@
-import SourceKittenFramework
+import SwiftSyntax
 
-public struct ForWhereRule: ASTRule, ConfigurationProviderRule {
-    public var configuration = SeverityConfiguration(.warning)
+public struct ForWhereRule: SwiftSyntaxRule, ConfigurationProviderRule {
+    public var configuration = ForWhereRuleConfiguration()
 
     public init() {}
 
@@ -66,14 +66,32 @@ public struct ForWhereRule: ASTRule, ConfigurationProviderRule {
               if user.id == 1 && user.age > 18 { }
             }
             """),
+            Example("""
+            for user in users {
+              if user.id == 1, user.age > 18 { }
+            }
+            """),
             // if case
             Example("""
             for (index, value) in array.enumerated() {
               if case .valueB(_) = value {
                 return index
               }
             }
-            """)
+            """),
+            Example("""
+            for user in users {
+              if user.id == 1 { return true }
+            }
+            """, configuration: ["allow_for_as_filter": true]),
+            Example("""
+            for user in users {
+              if user.id == 1 {
+                let derivedValue = calculateValue(from: user)
+                return derivedValue != 0
+              }
+            }
+            """, configuration: ["allow_for_as_filter": true])
         ],
         triggeringExamples: [
             Example("""
@@ -88,84 +106,94 @@ public struct ForWhereRule: ASTRule, ConfigurationProviderRule {
                     subview.removeFromSuperview()
                 }
             }
-            """)
+            """),
+            Example("""
+            for subview in subviews {
+                ↓if !(subview is UIStackView) {
+                    subview.removeConstraints(subview.constraints)
+                    subview.removeFromSuperview()
+                }
+            }
+            """, configuration: ["allow_for_as_filter": true])
         ]
     )
 
-    private static let commentKinds = SyntaxKind.commentAndStringKinds
-
-    public func validate(file: SwiftLintFile, kind: StatementKind,
-                         dictionary: SourceKittenDictionary) -> [StyleViolation] {
-        guard kind == .forEach,
-            let subDictionary = forBody(dictionary: dictionary),
-            subDictionary.substructure.count == 1,
-            let bodyDictionary = subDictionary.substructure.first,
-            bodyDictionary.statementKind == .if,
-            isOnlyOneIf(dictionary: bodyDictionary),
-            isOnlyIfInsideFor(forDictionary: subDictionary, ifDictionary: bodyDictionary, file: file),
-            !isComplexCondition(dictionary: bodyDictionary, file: file),
-            let offset = bodyDictionary .offset else {
-                return []
-        }
-
-        return [
-            StyleViolation(ruleDescription: Self.description,
-                           severity: configuration.severity,
-                           location: Location(file: file, byteOffset: offset))
-        ]
+    public func makeVisitor(file: SwiftLintFile) -> ViolationsSyntaxVisitor? {
+        ForWhereVisitor(allowForAsFilter: configuration.allowForAsFilter)
     }
 
-    private func forBody(dictionary: SourceKittenDictionary) -> SourceKittenDictionary? {
-        return dictionary.substructure.first(where: { subDict -> Bool in
-            subDict.statementKind == .brace
-        })
+    public func makeViolation(file: SwiftLintFile, position: AbsolutePosition) -> StyleViolation {
+        StyleViolation(
+            ruleDescription: Self.description,
+            severity: configuration.severityConfiguration.severity,
+            location: Location(file: file, position: position)
+        )
     }
+}
+
+private class ForWhereVisitor: SyntaxVisitor, ViolationsSyntaxVisitor {
+    private(set) var violationPositions: [AbsolutePosition] = []
+    private let allowForAsFilter: Bool
 
-    private func isOnlyOneIf(dictionary: SourceKittenDictionary) -> Bool {
-        let substructure = dictionary.substructure
-        let onlyOneBlock = substructure.filter { $0.statementKind == .brace }.count == 1
-        let noOtherIf = substructure.allSatisfy { $0.statementKind != .if }
-        return onlyOneBlock && noOtherIf
+    init(allowForAsFilter: Bool) {
+        self.allowForAsFilter = allowForAsFilter
     }
 
-    private func isOnlyIfInsideFor(forDictionary: SourceKittenDictionary,
-                                   ifDictionary: SourceKittenDictionary,
-                                   file: SwiftLintFile) -> Bool {
-        guard let offset = forDictionary.offset,
-            let length = forDictionary.length,
-            let ifOffset = ifDictionary.offset,
-            let ifLength = ifDictionary.length else {
-                return false
+    override func visitPost(_ node: ForInStmtSyntax) {
+        guard node.whereClause == nil,
+              case let statements = node.body.statements,
+              statements.count == 1,
+              let ifStatement = statements.first?.item.as(IfStmtSyntax.self),
+              ifStatement.elseBody == nil,
+              !ifStatement.containsOptionalBinding,
+              !ifStatement.containsPatternCondition,
+              ifStatement.conditions.count == 1,
+              let condition = ifStatement.conditions.first,
+              !condition.containsMultipleConditions else {
+            return
+        }
+
+        if allowForAsFilter, ifStatement.containsReturnStatement {
+            return
         }
 
-        let beforeIfRange = ByteRange(location: offset, length: ifOffset - offset)
-        let ifFinalPosition = ifOffset + ifLength
-        let afterIfRange = ByteRange(location: ifFinalPosition, length: offset + length - ifFinalPosition)
-        let allKinds = file.syntaxMap.kinds(inByteRange: beforeIfRange) +
-            file.syntaxMap.kinds(inByteRange: afterIfRange)
+        violationPositions.append(ifStatement.positionAfterSkippingLeadingTrivia)
+    }
+}
+
+private extension IfStmtSyntax {
+    var containsOptionalBinding: Bool {
+        conditions.contains { element in
+            element.condition.is(OptionalBindingConditionSyntax.self)
+        }
+    }
 
-        let doesntContainComments = !allKinds.contains { kind in
-            !Self.commentKinds.contains(kind)
+    var containsPatternCondition: Bool {
+        conditions.contains { element in
+            element.condition.is(MatchingPatternConditionSyntax.self)
         }
+    }
 
-        return doesntContainComments
+    var containsReturnStatement: Bool {
+        body.statements.contains { element in
+            element.item.is(ReturnStmtSyntax.self)
+        }
     }
+}
 
-    private func isComplexCondition(dictionary: SourceKittenDictionary, file: SwiftLintFile) -> Bool {
-        let kind = "source.lang.swift.structure.elem.condition_expr"
-        return dictionary.elements.contains { element in
-            guard element.kind == kind,
-                let range = element.byteRange.flatMap(file.stringView.byteRangeToNSRange)
-            else {
-                return false
-            }
+private extension ConditionElementSyntax {
+    var containsMultipleConditions: Bool {
+        guard let condition = condition.as(SequenceExprSyntax.self) else {
+            return false
+        }
 
-            let containsKeyword = file.match(pattern: "\\blet|var|case\\b", with: [.keyword], range: range).isNotEmpty
-            if containsKeyword {
-                return true
+        return condition.elements.contains { expr in
+            guard let binaryExpr = expr.as(BinaryOperatorExprSyntax.self) else {
+                return false
             }
 
-            return file.match(pattern: "\\|\\||&&", with: [], range: range).isNotEmpty
+            let operators: Set = ["&&", "||"]
+            return operators.contains(binaryExpr.operatorToken.text)
         }
     }
 }

Source/SwiftLintFramework/Rules/RuleConfigurations/ForWhereRuleConfiguration.swift

@@ -0,0 +1,20 @@
+public struct ForWhereRuleConfiguration: RuleConfiguration, Equatable {
+    private(set) var severityConfiguration = SeverityConfiguration(.warning)
+    private(set) var allowForAsFilter = false
+
+    public var consoleDescription: String {
+        return severityConfiguration.consoleDescription + ", allow_for_as_filter: \(allowForAsFilter)"
+    }
+
+    public mutating func apply(configuration: Any) throws {
+        guard let configuration = configuration as? [String: Any] else {
+            throw ConfigurationError.unknownConfiguration
+        }
+
+        allowForAsFilter = configuration["allow_for_as_filter"] as? Bool ?? false
+
+        if let severityString = configuration["severity"] as? String {
+            try severityConfiguration.apply(configuration: severityString)
+        }
+    }
+}