diff --git a/.swiftlint.yml b/.swiftlint.yml index aeb9d9cfd..f686ac8c8 100644 --- a/.swiftlint.yml +++ b/.swiftlint.yml @@ -40,6 +40,7 @@ disabled_rules: - prefer_nimble - prefer_self_in_static_references - prefixed_toplevel_constant + - redundant_self_in_closure - required_deinit - self_binding - sorted_enum_cases diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f22a5ff3..c42d5d518 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,17 @@ * Add `sorted_enum_cases` rule which warns when enum cases are not sorted. [kimdv](https://github.com/kimdv) +* Add new `redundant_self_in_closure` rule that triggers in closures on + explicitly used `self` when it's actually not needed due to: + * Strongly captured `self` (`{ [self] in ... }`) + * Closure used in a struct declaration (`self` can always be omitted) + * Anonymous closures that are directly called (`{ ... }()`) as they are + definitly not escaping + * Weakly captured `self` with explicit unwrapping + + [SimplyDanny](https://github.com/SimplyDanny) + [#59](https://github.com/realm/SwiftLint/issues/59) + * Extend `xct_specific_matcher` rule to check for boolean asserts on (un)equal comparisons. The rule can be configured with the matchers that should trigger rule violations. By default, all matchers trigger, but that can be limited to diff --git a/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift b/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift index ffdaa68ae..71b88767b 100644 --- a/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift +++ b/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift @@ -165,6 +165,7 @@ public let builtInRules: [Rule.Type] = [ RedundantNilCoalescingRule.self, RedundantObjcAttributeRule.self, RedundantOptionalInitializationRule.self, + RedundantSelfInClosureRule.self, RedundantSetAccessControlRule.self, RedundantStringEnumValueRule.self, RedundantTypeAnnotationRule.self, diff --git a/Source/SwiftLintBuiltInRules/Rules/Style/RedundantSelfInClosureRule.swift b/Source/SwiftLintBuiltInRules/Rules/Style/RedundantSelfInClosureRule.swift new file mode 100644 index 000000000..c5068132c --- /dev/null +++ b/Source/SwiftLintBuiltInRules/Rules/Style/RedundantSelfInClosureRule.swift @@ -0,0 +1,315 @@ +import SwiftSyntax + +struct RedundantSelfInClosureRule: SwiftSyntaxRule, CorrectableRule, ConfigurationProviderRule, OptInRule { + var configuration = SeverityConfiguration(.warning) + + static var description = RuleDescription( + identifier: "redundant_self_in_closure", + name: "Redundant Self in Closure", + description: "Explicit use of 'self' is not required", + kind: .style, + nonTriggeringExamples: [ + Example(""" + struct S { + var x = 0 + func f(_ work: @escaping () -> Void) { work() } + func g() { + f { + x = 1 + f { x = 1 } + g() + } + } + } + """), + Example(""" + class C { + var x = 0 + func f(_ work: @escaping () -> Void) { work() } + func g() { + f { [weak self] in + self?.x = 1 + self?.g() + guard let self = self ?? C() else { return } + self?.x = 1 + } + C().f { self.x = 1 } + f { [weak self] in if let self { x = 1 } } + } + } + """) + ], + triggeringExamples: [ + Example(""" + struct S { + var x = 0 + func f(_ work: @escaping () -> Void) { work() } + func g() { + f { + ↓self.x = 1 + if ↓self.x == 1 { ↓self.g() } + } + } + } + """), + Example(""" + class C { + var x = 0 + func g() { + { + ↓self.x = 1 + ↓self.g() + }() + } + } + """), + Example(""" + class C { + var x = 0 + func f(_ work: @escaping () -> Void) { work() } + func g() { + f { [self] in + ↓self.x = 1 + ↓self.g() + f { self.x = 1 } + } + } + } + """), + Example(""" + class C { + var x = 0 + func f(_ work: @escaping () -> Void) { work() } + func g() { + f { [unowned self] in ↓self.x = 1 } + f { [self = self] in ↓self.x = 1 } + f { [s = self] in s.x = 1 } + } + } + """) + ] + triggeringCompilerSpecificExamples, + corrections: [ + Example(""" + struct S { + var x = 0 + func f(_ work: @escaping () -> Void) { work() } + func g() { + f { + ↓self.x = 1 + if ↓self.x == 1 { ↓self.g() } + } + } + } + """): Example(""" + struct S { + var x = 0 + func f(_ work: @escaping () -> Void) { work() } + func g() { + f { + x = 1 + if x == 1 { g() } + } + } + } + """) + ] + ) + +#if compiler(>=5.8) + private static let triggeringCompilerSpecificExamples = [ + Example(""" + class C { + var x = 0 + func f(_ work: @escaping () -> Void) { work() } + func g() { + f { [weak self] in + self?.x = 1 + guard let self else { return } + ↓self.x = 1 + } + f { [weak self] in + self?.x = 1 + if let self = self else { ↓self.x = 1 } + self?.x = 1 + } + f { [weak self] in + self?.x = 1 + while let self else { ↓self.x = 1 } + self?.x = 1 + } + } + } + """) + ] +#else + private static let triggeringCompilerSpecificExamples = [Example]() +#endif + + func makeVisitor(file: SwiftLintFile) -> ViolationsSyntaxVisitor { + ScopeVisitor(viewMode: .sourceAccurate) + } + + func correct(file: SwiftLintFile) -> [Correction] { + let ranges = ScopeVisitor(viewMode: .sourceAccurate) + .walk(file: file, handler: \.corrections) + .compactMap { file.stringView.NSRange(start: $0.start, end: $0.end) } + .filter { file.ruleEnabled(violatingRange: $0, for: self) != nil } + .reversed() + + var corrections = [Correction]() + var contents = file.contents + for range in ranges { + let contentsNSString = contents.bridge() + contents = contentsNSString.replacingCharacters(in: range, with: "") + let location = Location(file: file, characterOffset: range.location) + corrections.append(Correction(ruleDescription: Self.description, location: location)) + } + + file.write(contents) + + return corrections + } +} + +private enum TypeDeclarationKind { + case likeStruct + case likeClass +} + +private enum FunctionCallType { + case anonymousClosure + case function +} + +private enum SelfCaptureKind { + case strong + case weak + case uncaptured +} + +private class ScopeVisitor: ViolationsSyntaxVisitor { + private var typeDeclarations = Stack() + private var functionCalls = Stack() + private var selfCaptures = Stack() + + private(set) var corrections = [(start: AbsolutePosition, end: AbsolutePosition)]() + + override var skippableDeclarations: [DeclSyntaxProtocol.Type] { .extensionsAndProtocols } + + override func visit(_ node: ActorDeclSyntax) -> SyntaxVisitorContinueKind { + typeDeclarations.push(.likeClass) + return .visitChildren + } + + override func visitPost(_ node: ActorDeclSyntax) { + typeDeclarations.pop() + } + + override func visit(_ node: ClassDeclSyntax) -> SyntaxVisitorContinueKind { + typeDeclarations.push(.likeClass) + return .visitChildren + } + + override func visitPost(_ node: ClassDeclSyntax) { + typeDeclarations.pop() + } + + override func visit(_ node: ClosureExprSyntax) -> SyntaxVisitorContinueKind { + if let selfItem = node.signature?.capture?.items?.first(where: \.capturesSelf) { + selfCaptures.push(selfItem.capturesWeakly ? .weak : .strong) + } else { + selfCaptures.push(.uncaptured) + } + return .visitChildren + } + + override func visitPost(_ node: ClosureExprSyntax) { + guard let activeTypeDeclarationKind = typeDeclarations.peek(), + let activeFunctionCallType = functionCalls.peek(), + let activeSelfCaptureKind = selfCaptures.peek() else { + return + } + let localCorrections = ExplicitSelfVisitor( + typeDeclarationKind: activeTypeDeclarationKind, + functionCallType: activeFunctionCallType, + selfCaptureKind: activeSelfCaptureKind + ).walk(tree: node.statements, handler: \.corrections) + violations.append(contentsOf: localCorrections.map(\.start)) + corrections.append(contentsOf: localCorrections) + selfCaptures.pop() + } + + override func visit(_ node: EnumDeclSyntax) -> SyntaxVisitorContinueKind { + typeDeclarations.push(.likeStruct) + return .visitChildren + } + + override func visitPost(_ node: EnumDeclSyntax) { + typeDeclarations.pop() + } + + override func visit(_ node: FunctionCallExprSyntax) -> SyntaxVisitorContinueKind { + if node.calledExpression.is(ClosureExprSyntax.self) { + functionCalls.push(.anonymousClosure) + } else { + functionCalls.push(.function) + } + return .visitChildren + } + + override func visitPost(_ node: FunctionCallExprSyntax) { + functionCalls.pop() + } + + override func visit(_ node: StructDeclSyntax) -> SyntaxVisitorContinueKind { + typeDeclarations.push(.likeStruct) + return .visitChildren + } + + override func visitPost(_ node: StructDeclSyntax) { + typeDeclarations.pop() + } +} + +private class ExplicitSelfVisitor: ViolationsSyntaxVisitor { + private let typeDeclKind: TypeDeclarationKind + private let functionCallType: FunctionCallType + private let selfCaptureKind: SelfCaptureKind + + private(set) var corrections = [(start: AbsolutePosition, end: AbsolutePosition)]() + + init(typeDeclarationKind: TypeDeclarationKind, + functionCallType: FunctionCallType, + selfCaptureKind: SelfCaptureKind) { + self.typeDeclKind = typeDeclarationKind + self.functionCallType = functionCallType + self.selfCaptureKind = selfCaptureKind + super.init(viewMode: .sourceAccurate) + } + + override func visitPost(_ node: MemberAccessExprSyntax) { + if node.base?.as(IdentifierExprSyntax.self)?.isSelf == true, isSelfRedundant { + corrections.append( + (start: node.positionAfterSkippingLeadingTrivia, end: node.dot.endPositionBeforeTrailingTrivia) + ) + } + } + + override func visit(_ node: ClosureExprSyntax) -> SyntaxVisitorContinueKind { + // Will be handled separately by the parent visitor. + .skipChildren + } + + var isSelfRedundant: Bool { + if typeDeclKind == .likeStruct || functionCallType == .anonymousClosure { + return true + } + if selfCaptureKind == .strong && SwiftVersion.current >= .fiveDotThree { + return true + } + if selfCaptureKind == .weak && SwiftVersion.current >= .fiveDotEight { + return true + } + return false + } +} diff --git a/Source/SwiftLintCore/Extensions/SwiftSyntax+SwiftLint.swift b/Source/SwiftLintCore/Extensions/SwiftSyntax+SwiftLint.swift index fe591fc46..7fa40385d 100644 --- a/Source/SwiftLintCore/Extensions/SwiftSyntax+SwiftLint.swift +++ b/Source/SwiftLintCore/Extensions/SwiftSyntax+SwiftLint.swift @@ -328,7 +328,6 @@ public extension IntegerLiteralExprSyntax { guard case let .integerLiteral(number) = digits.tokenKind else { return false } - return number.isZero } } @@ -338,11 +337,32 @@ public extension FloatLiteralExprSyntax { guard case let .floatingLiteral(number) = floatingDigits.tokenKind else { return false } - return number.isZero } } +public extension IdentifierExprSyntax { + var isSelf: Bool { + identifier.text == "self" + } +} + +public extension MemberAccessExprSyntax { + var isSelfAccess: Bool { + base?.as(IdentifierExprSyntax.self)?.isSelf == true + } +} + +public extension ClosureCaptureItemSyntax { + var capturesSelf: Bool { + expression.as(IdentifierExprSyntax.self)?.isSelf == true + } + + var capturesWeakly: Bool { + specifier?.specifier.text == "weak" + } +} + private extension String { var isZero: Bool { if self == "0" { // fast path diff --git a/Source/SwiftLintCore/Models/SwiftVersion.swift b/Source/SwiftLintCore/Models/SwiftVersion.swift index 2f7273c56..08c187569 100644 --- a/Source/SwiftLintCore/Models/SwiftVersion.swift +++ b/Source/SwiftLintCore/Models/SwiftVersion.swift @@ -33,6 +33,8 @@ public extension SwiftVersion { static let fiveDotSix = SwiftVersion(rawValue: "5.6.0") /// Swift 5.7.x - https://swift.org/download/#swift-57 static let fiveDotSeven = SwiftVersion(rawValue: "5.7.0") + /// Swift 5.8.x - https://swift.org/download/#swift-58 + static let fiveDotEight = SwiftVersion(rawValue: "5.8.0") /// The current detected Swift compiler version, based on the currently accessible SourceKit version. /// diff --git a/Tests/GeneratedTests/GeneratedTests.swift b/Tests/GeneratedTests/GeneratedTests.swift index 1a15a3ff1..3b5ea994c 100644 --- a/Tests/GeneratedTests/GeneratedTests.swift +++ b/Tests/GeneratedTests/GeneratedTests.swift @@ -980,6 +980,12 @@ class RedundantOptionalInitializationRuleGeneratedTests: SwiftLintTestCase { } } +class RedundantSelfInClosureRuleGeneratedTests: SwiftLintTestCase { + func testWithDefaultConfiguration() { + verifyRule(RedundantSelfInClosureRule.description) + } +} + class RedundantSetAccessControlRuleGeneratedTests: SwiftLintTestCase { func testWithDefaultConfiguration() { verifyRule(RedundantSetAccessControlRule.description)