Introduce visitor that tracks declared variables while traversing the AST

CHANGELOG.md

@@ -25,6 +25,11 @@
   (e.g. `@testable`, `@_exported`, ...).  
   [hiltonc](https://github.com/hiltonc)
 
+* Do not trigger `redundant_self_in_closure` rule when another idenfier `x` in
+  scope shadows the field accessed by `self.x` to avoid semantical changes.  
+  [SimplyDanny](https://github.com/SimplyDanny)
+  [#5010](https://github.com/realm/SwiftLint/issues/5010)
+
 #### Bug Fixes
 
 * Do not trigger `prefer_self_in_static_references` rule on `typealias`

Source/SwiftLintBuiltInRules/Rules/Style/RedundantSelfInClosureRule.swift

@@ -1,5 +1,7 @@
 import SwiftSyntax
 
+// swiftlint:disable file_length
+
 struct RedundantSelfInClosureRule: SwiftSyntaxRule, CorrectableRule, ConfigurationProviderRule, OptInRule {
     var configuration = SeverityConfiguration<Self>(.warning)
 
@@ -40,10 +42,52 @@ struct RedundantSelfInClosureRule: SwiftSyntaxRule, CorrectableRule, Configurati
             """),
             Example("""
                 struct S {
-                    var x = 0
+                    var x = 0, error = 0, exception = 0
+                    var y: Int?, z: Int?, u: Int, v: Int?, w: Int?
+                    func f(_ work: @escaping (Int) -> Void) { work() }
+                    func g(x: Int) {
+                        f { u in
+                            self.x = x
+                            let x = 1
+                            self.x = 2
+                            if let y, let v {
+                                self.y = 3
+                                self.v = 1
+                            }
+                            guard let z else {
+                                let v = 4
+                                self.x = 5
+                                self.v = 6
+                                return
+                            }
+                            self.z = 7
+                            while let v { self.v = 8 }
+                            for w in [Int]() { self.w = 9 }
+                            self.u = u
+                            do {} catch { self.error = 10 }
+                            do {} catch let exception { self.exception = 11 }
+                        }
+                    }
+                }
+            """),
+            Example("""
+                enum E {
+                    case a(Int)
+                    case b(Int, Int)
+                }
+                struct S {
+                    var x: E = .a(3), y: Int, z: Int
                     func f(_ work: @escaping () -> Void) { work() }
                     func g(x: Int) {
-                        f { self.x = x }
+                        f {
+                            switch x {
+                            case let .a(y):
+                                self.y = 1
+                            case .b(let y, var z):
+                                self.y = 2
+                                self.z = 3
+                            }
+                        }
                     }
                 }
             """)
@@ -95,7 +139,44 @@ struct RedundantSelfInClosureRule: SwiftSyntaxRule, CorrectableRule, Configurati
                         f { [s = self] in s.x = 1 }
                     }
                 }
-            """)
+            """),
+            Example("""
+                struct S {
+                    var x = 0
+                    var y: Int?, z: Int?, v: Int?, w: Int?
+                    func f(_ work: @escaping () -> Void) { work() }
+                    func g(w: Int, _ v: Int) {
+                        f {
+                            self.w = 1
+                            ↓self.x = 2
+                            if let y { ↓self.x = 3 }
+                            else { ↓self.y = 3 }
+                            guard let z else {
+                                ↓self.z = 4
+                                ↓self.x = 5
+                                return
+                            }
+                            ↓self.y = 6
+                            while let y { ↓self.x = 7 }
+                            for y in [Int]() { ↓self.x = 8 }
+                            self.v = 9
+                            do {
+                                let x = 10
+                                self.x = 11
+                            }
+                            ↓self.x = 12
+                        }
+                    }
+                }
+            """),
+            Example("""
+                struct S {
+                    func f(_ work: @escaping () -> Void) { work() }
+                    func g() {
+                        f { let g = ↓self.g() }
+                    }
+                }
+            """, excludeFromDocumentation: true)
         ] + triggeringCompilerSpecificExamples,
         corrections: [
             Example("""
@@ -155,11 +236,11 @@ struct RedundantSelfInClosureRule: SwiftSyntaxRule, CorrectableRule, Configurati
 #endif
 
     func makeVisitor(file: SwiftLintFile) -> ViolationsSyntaxVisitor {
-        ScopeVisitor(viewMode: .sourceAccurate)
+        ContextVisitor()
     }
 
     func correct(file: SwiftLintFile) -> [Correction] {
-        let ranges = ScopeVisitor(viewMode: .sourceAccurate)
+        let ranges = ContextVisitor()
             .walk(file: file, handler: \.corrections)
             .compactMap { file.stringView.NSRange(start: $0.start, end: $0.end) }
             .filter { file.ruleEnabled(violatingRange: $0, for: self) != nil }
@@ -196,7 +277,7 @@ private enum SelfCaptureKind {
     case uncaptured
 }
 
-private class ScopeVisitor: ViolationsSyntaxVisitor {
+private class ContextVisitor: DeclaredIdentifiersTrackingVisitor {
     private var typeDeclarations = Stack<TypeDeclarationKind>()
     private var functionCalls = Stack<FunctionCallType>()
     private var selfCaptures = Stack<SelfCaptureKind>()
@@ -241,7 +322,8 @@ private class ScopeVisitor: ViolationsSyntaxVisitor {
         let localCorrections = ExplicitSelfVisitor(
             typeDeclarationKind: activeTypeDeclarationKind,
             functionCallType: activeFunctionCallType,
-            selfCaptureKind: activeSelfCaptureKind
+            selfCaptureKind: activeSelfCaptureKind,
+            scope: scope
         ).walk(tree: node.statements, handler: \.corrections)
         violations.append(contentsOf: localCorrections.map(\.start))
         corrections.append(contentsOf: localCorrections)
@@ -280,7 +362,7 @@ private class ScopeVisitor: ViolationsSyntaxVisitor {
     }
 }
 
-private class ExplicitSelfVisitor: ViolationsSyntaxVisitor {
+private class ExplicitSelfVisitor: DeclaredIdentifiersTrackingVisitor {
     private let typeDeclKind: TypeDeclarationKind
     private let functionCallType: FunctionCallType
     private let selfCaptureKind: SelfCaptureKind
@@ -289,27 +371,16 @@ private class ExplicitSelfVisitor: ViolationsSyntaxVisitor {
 
     init(typeDeclarationKind: TypeDeclarationKind,
          functionCallType: FunctionCallType,
-         selfCaptureKind: SelfCaptureKind) {
+         selfCaptureKind: SelfCaptureKind,
+         scope: Scope) {
         self.typeDeclKind = typeDeclarationKind
         self.functionCallType = functionCallType
         self.selfCaptureKind = selfCaptureKind
-        super.init(viewMode: .sourceAccurate)
-    }
-
-    override func visit(_ node: SequenceExprSyntax) -> SyntaxVisitorContinueKind {
-        let elements = node.elements
-        if elements.count == 3,
-           let assignee = elements.first?.as(MemberAccessExprSyntax.self), assignee.isBaseSelf,
-           elements.dropFirst(1).first?.is(AssignmentExprSyntax.self) == true,
-           elements.dropFirst(2).first?.as(IdentifierExprSyntax.self)?.identifier.text == assignee.name.text {
-            // We have something like `self.x = x` which is quite common and should thus be skipped.
-            return .skipChildren
-        }
-        return .visitChildren
+        super.init(scope: scope)
     }
 
     override func visitPost(_ node: MemberAccessExprSyntax) {
-        if node.isBaseSelf, isSelfRedundant {
+        if !hasSeenDeclaration(for: node.name.text), node.isBaseSelf, isSelfRedundant {
             corrections.append(
                 (start: node.positionAfterSkippingLeadingTrivia, end: node.dot.endPositionBeforeTrailingTrivia)
             )

Source/SwiftLintCore/Helpers/Stack.swift

@@ -29,6 +29,24 @@ public struct Stack<Element> {
     public func peek() -> Element? {
         elements.last
     }
+
+    /// Check whether the sequence contains an element that satisfies the given predicate.
+    ///
+    /// - parameter predicate: A closure that takes an element of the sequence
+    ///   and returns whether it represents a match.
+    /// - returns: `true` if the sequence contains an element that satisfies `predicate`.
+    public func contains(where predicate: (Element) -> Bool) -> Bool {
+        elements.contains(where: predicate)
+    }
+
+    /// Modify the last element.
+    ///
+    /// - parameter modifier: A function to applied to the last element to modify it in place.
+    public mutating func modifyLast(by modifier: (inout Element) -> Void) {
+        if elements.isNotEmpty {
+            modifier(&elements[count - 1])
+        }
+    }
 }
 
 extension Stack: CustomDebugStringConvertible where Element == CustomDebugStringConvertible {

Source/SwiftLintCore/Visitors/DeclaredIdentifiersTrackingVisitor.swift

@@ -0,0 +1,107 @@
+import SwiftSyntax
+
+/// A specialized `ViolationsSyntaxVisitor` that tracks declared identifiers per scope while traversing the AST.
+open class DeclaredIdentifiersTrackingVisitor: ViolationsSyntaxVisitor {
+    /// A type that remembers the declared identifers (in order) up to the current position in the code.
+    public typealias Scope = Stack<Set<String>>
+
+    /// The hierarchical stack of identifiers declared up to the current position in the code.
+    public private(set) var scope: Scope
+
+    /// Initializer.
+    ///
+    /// - parameter scope: A (potentially already pre-filled) scope to collect identifers into.
+    public init(scope: Scope = Scope()) {
+        self.scope = scope
+        super.init(viewMode: .sourceAccurate)
+    }
+
+    /// Indicate whether a given identifier is in scope.
+    ///
+    /// - parameter identifier: An identifier.
+    public func hasSeenDeclaration(for identifier: String) -> Bool {
+        scope.contains { $0.contains(identifier) }
+    }
+
+    // swiftlint:disable:next cyclomatic_complexity
+    override open func visit(_ node: CodeBlockItemListSyntax) -> SyntaxVisitorContinueKind {
+        guard let parent = node.parent, !parent.is(SourceFileSyntax.self), let grandParent = parent.parent else {
+            return .visitChildren
+        }
+        scope.openChildScope()
+        if let ifStmt = grandParent.as(IfExprSyntax.self), parent.keyPathInParent != \IfExprSyntax.elseBody {
+            collectIdentifiers(fromConditions: ifStmt.conditions)
+        } else if let whileStmt = grandParent.as(WhileStmtSyntax.self) {
+            collectIdentifiers(fromConditions: whileStmt.conditions)
+        } else if let pattern = grandParent.as(ForInStmtSyntax.self)?.pattern {
+            collectIdentifiers(fromPattern: pattern)
+        } else if let parameters = grandParent.as(FunctionDeclSyntax.self)?.signature.input.parameterList {
+            parameters.forEach { scope.addToCurrentScope(($0.secondName ?? $0.firstName).text) }
+        } else if let input = parent.as(ClosureExprSyntax.self)?.signature?.input {
+            switch input {
+            case let .input(parameters):
+                parameters.parameterList.forEach { scope.addToCurrentScope(($0.secondName ?? $0.firstName).text) }
+            case let .simpleInput(parameters):
+                parameters.forEach { scope.addToCurrentScope($0.name.text) }
+            }
+        } else if let switchCase = parent.as(SwitchCaseSyntax.self)?.label.as(SwitchCaseLabelSyntax.self) {
+            switchCase.caseItems
+                .compactMap { $0.pattern.as(ValueBindingPatternSyntax.self)?.valuePattern ?? $0.pattern }
+                .compactMap { $0.as(ExpressionPatternSyntax.self)?.expression.asFunctionCall }
+                .compactMap { $0.argumentList.as(TupleExprElementListSyntax.self) }
+                .flatMap { $0 }
+                .compactMap { $0.expression.as(UnresolvedPatternExprSyntax.self) }
+                .compactMap { $0.pattern.as(ValueBindingPatternSyntax.self)?.valuePattern ?? $0.pattern }
+                .compactMap { $0.as(IdentifierPatternSyntax.self) }
+                .forEach { scope.addToCurrentScope($0.identifier.text) }
+        } else if let catchClause = grandParent.as(CatchClauseSyntax.self) {
+            if let items = catchClause.catchItems {
+                items
+                    .compactMap { $0.pattern?.as(ValueBindingPatternSyntax.self)?.valuePattern }
+                    .forEach(collectIdentifiers(fromPattern:))
+            } else {
+                // A catch clause without explicit catch items has an implicit `error` variable in scope.
+                scope.addToCurrentScope("error")
+            }
+        }
+        return .visitChildren
+    }
+
+    override open func visitPost(_ node: CodeBlockItemListSyntax) {
+        scope.pop()
+    }
+
+    override open func visitPost(_ node: VariableDeclSyntax) {
+        if node.parent?.is(MemberDeclListItemSyntax.self) != true {
+            for binding in node.bindings {
+                collectIdentifiers(fromPattern: binding.pattern)
+            }
+        }
+    }
+
+    override open func visitPost(_ node: GuardStmtSyntax) {
+        collectIdentifiers(fromConditions: node.conditions)
+    }
+
+    private func collectIdentifiers(fromConditions conditions: ConditionElementListSyntax) {
+        conditions
+            .compactMap { $0.condition.as(OptionalBindingConditionSyntax.self)?.pattern }
+            .forEach { collectIdentifiers(fromPattern: $0) }
+    }
+
+    private func collectIdentifiers(fromPattern pattern: PatternSyntax) {
+        if let name = pattern.as(IdentifierPatternSyntax.self)?.identifier.text {
+            scope.addToCurrentScope(name)
+        }
+    }
+}
+
+private extension DeclaredIdentifiersTrackingVisitor.Scope {
+    mutating func addToCurrentScope(_ identifier: String) {
+        modifyLast { $0.insert(identifier) }
+    }
+
+    mutating func openChildScope() {
+        push([])
+    }
+}