diff --git a/CHANGELOG.md b/CHANGELOG.md index d61ae13a07..9c70d89a42 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,10 @@ * Add new `allowed_numbers` option to the `no_magic_numbers` rule. [Martin Redington](https://github.com/mildm8nnered) +* Add new `reduce_into_instead_of_loop` opt-in rule that triggers when a + `for ... in ` is used to update a sequence where a `reduce(into:)` is + preferred. + [Alfons Hoogervorst](https://github.com/snofla/SwiftLint) ### Bug Fixes diff --git a/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift b/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift index 0138cc743e..3301b2c216 100644 --- a/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift +++ b/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift @@ -169,6 +169,7 @@ public let builtInRules: [any Rule.Type] = [ QuickDiscouragedPendingTestRule.self, RawValueForCamelCasedCodableEnumRule.self, ReduceBooleanRule.self, + ReduceIntoInsteadOfLoopRule.self, ReduceIntoRule.self, RedundantDiscardableLetRule.self, RedundantNilCoalescingRule.self, diff --git a/Source/SwiftLintBuiltInRules/Rules/Idiomatic/ReduceIntoInsteadOfLoopRule.swift b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/ReduceIntoInsteadOfLoopRule.swift new file mode 100644 index 0000000000..762d9ee3a8 --- /dev/null +++ b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/ReduceIntoInsteadOfLoopRule.swift @@ -0,0 +1,364 @@ +import SwiftSyntax + +typealias ReferencedVariable = ReduceIntoInsteadOfLoopRule.ReferencedVariable +typealias CollectionType = ReduceIntoInsteadOfLoopRule.CollectionType + +@SwiftSyntaxRule(optIn: true) +struct ReduceIntoInsteadOfLoopRule: Rule { + var configuration = SeverityConfiguration(.warning) + + static let description = RuleDescription( + identifier: "reduce_into_instead_of_loop", + name: "Reduce Into Instead Of Loop", + description: "Prefer using `reduce(into:)` instead of mutating a sequence in a `for _ in ...` loop", + kind: .idiomatic, + nonTriggeringExamples: ReduceIntoInsteadOfLoopRuleExamples.nonTriggeringExamples, + triggeringExamples: ReduceIntoInsteadOfLoopRuleExamples.triggeringExamples + ) +} + +internal extension ReduceIntoInsteadOfLoopRule { + final class Visitor: ViolationsSyntaxVisitor { + override func visitPost(_ node: CodeBlockItemListSyntax) { + // Collect all varDecls in the same scope as forInStmts + guard let all = node.allVariableDeclsForInStatmts() else { + return + } + // Select those varDecls that have initialisers + let selected = all.reduce(into: [ForStmtSyntax: [VariableDeclSyntax]]()) { partialResult, element in + // we're interested fully type declared and implicitly declared by initializer + let interestingVariableDecls = element.value.filter { variableDecl in + variableDecl.isTypeAnnotatedAndInitializer + || variableDecl.isCollectionTypeInitializer + } + guard !interestingVariableDecls.isEmpty else { + return + } + partialResult[element.key] = interestingVariableDecls + } + guard !selected.isEmpty else { + return + } + // Collect all varDecls that are referenced (and in our case possibly + // mutate) in the forInStmt + let referencedVars = selected.reduce(into: Set()) { partialResult, keyValue in + let (forInStmt, variableDecls) = keyValue + if let allReferencedVars = forInStmt.referencedVariables(for: variableDecls) { + partialResult.formUnion(allReferencedVars) + } + } + guard !referencedVars.isEmpty else { + return + } + // If there are referenced varDecls, then report violations + self.violations.append(contentsOf: referencedVars.map { $0.position }) + } + } + + static let collectionTypes: [CollectionType] = [ + CollectionType(name: "Set", genericArguments: 1), + CollectionType(name: "Array", genericArguments: 1), + CollectionType(name: "Dictionary", genericArguments: 2), + ] + + static let collectionNames: [String: CollectionType] = + ReduceIntoInsteadOfLoopRule.collectionTypes.reduce(into: [:]) { partialResult, type in + partialResult[type.name] = type + } +} + +private extension CodeBlockItemListSyntax { + /// Returns a dictionary with all VariableDecls preceding a ForInStmt, at the same scope level + func allVariableDeclsForInStatmts() -> [ForStmtSyntax: [VariableDeclSyntax]]? { + typealias IndexRange = Range + typealias IndexRangeForStmts = (range: IndexRange, forStmt: ForStmtSyntax) + // Collect all ForInStmts and track their index ranges + let indexRangeForStatements: [IndexRangeForStmts] = self.reduce(into: []) { partialResult, codeBlockItem in + guard let codeBlockItemIndex = self.index(of: codeBlockItem) else { + return + } + guard codeBlockItem.item.kind == .forStmt, + let forStmt = codeBlockItem.item.as(ForStmtSyntax.self), + forStmt.inKeyword.tokenKind == .keyword(.in) else { + return + } + guard let lastEncountered = partialResult.last else { + // First item encountered + partialResult.append((range: self.startIndex.. = []`, `: Array<> = []`, or `: Dictionary<> = [:]` + var isTypeAnnotatedAndInitializer: Bool { + guard self.isVar, + self.identifier != nil, + let identifierPatternSyntax = self.firstPatternOf(IdentifierPatternSyntax.self) else { + return false + } + // Is type-annotated, and initialized? + guard let patternBindingSyntax = identifierPatternSyntax.parent?.as(PatternBindingSyntax.self), + let typeAnnotation = patternBindingSyntax.typeAnnotation, + let type = typeAnnotation.collectionDeclarationType, + let initializerClause = patternBindingSyntax.initializer else { + return false + } + return initializerClause.isTypeInitializer(for: type) + } + + /// Is initialized with empty collection: `= Set(), = Array(), = Dictionary[:]` + /// but a couple of more, see `InitializerClauseExprSyntax.isCollectionInitializer` + var isCollectionTypeInitializer: Bool { + guard self.isVar && self.identifier != nil else { + return false + } + guard let initializerClausePatternBinding = self.bindings.first(where: { patternBindingSyntax in + patternBindingSyntax.initializer != nil + }) else { + return false + } + guard let initializerClause = initializerClausePatternBinding.initializer else { + return false + } + guard initializerClause.isTypeInitializer() else { + return false + } + return true + } +} + +private extension TypeAnnotationSyntax { + /// Returns one of the collection types we define + var collectionDeclarationType: CollectionType? { + if let genericTypeName = self.genericCollectionDeclarationType { + return genericTypeName + } + if let array = self.arrayDeclarationType { + return array + } + if let dictionary = self.dictionaryDeclarationType { + return dictionary + } + return nil + } + + /// var x: Set<>, var x: Array<>, var x: Dictionary<> + var genericCollectionDeclarationType: CollectionType? { + guard let simpleTypeIdentifier = self.type.as(IdentifierTypeSyntax.self), + let genericArgumentClause = simpleTypeIdentifier.genericArgumentClause, + genericArgumentClause.leftAngle.tokenKind == .leftAngle, + genericArgumentClause.rightAngle.tokenKind == .rightAngle, + case .identifier(let name) = simpleTypeIdentifier.name.tokenKind, + let collectionType = CollectionType.names[name], + genericArgumentClause.arguments.count == collectionType.genericArguments else { + return nil + } + return collectionType + } + + /// var x: [Y] + var arrayDeclarationType: CollectionType? { + guard let arrayType = self.type.as(ArrayTypeSyntax.self), + case .leftSquare = arrayType.leftSquare.tokenKind, + case .rightSquare = arrayType.rightSquare.tokenKind, + arrayType.element.kind == .identifierType else { + return nil + } + return .array + } + + /// var x: [K: V] + var dictionaryDeclarationType: CollectionType? { + guard let dictionaryType = self.type.as(DictionaryTypeSyntax.self), + case .leftSquare = dictionaryType.leftSquare.tokenKind, + case .rightSquare = dictionaryType.rightSquare.tokenKind, + case .colon = dictionaryType.colon.tokenKind else { + return nil + } + return .dictionary + } +} + +private extension InitializerClauseSyntax { + /// --- + /// If `nil` we don't know the type and investigate the following, which is a`FunctionCallExpr`: + /// var x = Set(...) + /// var y = Array(...) + /// var z = Dictionary(...) + /// Otherwise checks for `FunctionCallExpr`,`MemberAccessExpr`,`DictionaryExpr` and `ArrayExpr`: + /// 1. `= Set(...)` | `Set(...)` | `.init(...)` | `[]` + /// 2. `= Array(...)` | `Array(...)` | `.init(...)` | `[]` + /// 2b. `= [Type]()` + /// 3. `= Dictionary()` | `Dictionary()` | `.init(..)` | `[:]` + func isTypeInitializer(for collectionType: CollectionType? = nil) -> Bool { + func isSupportedType(with name: String) -> Bool { + if let collectionType { + return collectionType.name == name + } + return CollectionType.names[name] != nil + } + guard self.equal.tokenKind == .equal else { return false } + if let functionCallExpr = self.value.as(FunctionCallExprSyntax.self) { + // either construction using explicit specialisation, or general construction + if let specializeExpr = functionCallExpr.calledExpression.as(GenericSpecializationExprSyntax.self), + let identifierExpr = specializeExpr.expression.as(DeclReferenceExprSyntax.self), + case .identifier(let typename) = identifierExpr.baseName.tokenKind { + return isSupportedType(with: typename) + } + if let identifierExpr = functionCallExpr.calledExpression.as(DeclReferenceExprSyntax.self), + case .identifier(let typename) = identifierExpr.baseName.tokenKind { + return isSupportedType(with: typename) + } + if let memberAccessExpr = functionCallExpr.calledExpression.as(MemberAccessExprSyntax.self), + memberAccessExpr.declName.baseName.tokenKind == .keyword(.`init`) { + // found a collection initialisation expression of `.init()` + // e.g. var array: [Int] = .init() + return true + } + if let arrayExpr = functionCallExpr.calledExpression.as(ArrayExprSyntax.self), + arrayExpr.elements.count == 1, + arrayExpr.elements.first?.expression.is(DeclReferenceExprSyntax.self) == true { + // found an array initialisation expression of `[type]`() + // e.g. var array = [Int]() + return true + } + } else if collectionType == .dictionary, + self.value.as(DictionaryExprSyntax.self) != nil { + return true + } else if collectionType == .array || collectionType == .set, + self.value.as(ArrayExprSyntax.self) != nil { + return true + } + return false + } +} + +private extension ForStmtSyntax { + /// Checks whether any of the collection variables in scope are referenced + /// in the forStmt. + /// Note: When a varDecl is referenced, that's already a good trigger for + /// the warning: detecting which functions are mutating is not possible, + /// other than checking for a lhs-assignment. + func referencedVariables(for variables: [VariableDeclSyntax]?) -> Set? { + guard let variables, !variables.isEmpty else { + return nil + } + let codeBlockItemList = self.body.statements + let references: Set = codeBlockItemList.reduce(into: [], { partialResult, codeBlock in + // no need to cover one liner: someMutation(); someOtherMutation() + variables.forEach { variableDecl in + if let referenced = codeBlock.referencedVariable(for: variableDecl) { + partialResult.insert(referenced) + } + } + }) + return references.isEmpty ? nil : references + } +} + +private extension CodeBlockItemSyntax { + func referencedVariable(for variableDecl: VariableDeclSyntax?) -> ReferencedVariable? { + guard let identifier = variableDecl?.identifier else { + return nil + } + return self.referencedVariable(for: identifier) + } + + func referencedVariable(for varName: String) -> ReferencedVariable? { + if let functionCallExpr = self.item.as(FunctionCallExprSyntax.self) { + return functionCallExpr.referencedVariable(for: varName) + } + if let sequenceExpr = self.item.as(SequenceExprSyntax.self) { + return sequenceExpr.referencedVariable(for: varName) + } + return nil + } +} + +private extension FunctionCallExprSyntax { + /// varName.method(x, y, n) + func referencedVariable(for varName: String) -> ReferencedVariable? { + guard self.leftParen?.tokenKind == .leftParen, + self.rightParen?.tokenKind == .rightParen, + let memberAccessExpr = self.calledExpression.as(MemberAccessExprSyntax.self), + memberAccessExpr.period.tokenKind == .period, + let identifierExpr = memberAccessExpr.base?.as(DeclReferenceExprSyntax.self), + identifierExpr.baseName.tokenKind == .identifier(varName), + case .identifier(let name) = memberAccessExpr.declName.baseName.tokenKind else { + return nil + } + return ReferencedVariable( + name: varName, + position: memberAccessExpr.positionAfterSkippingLeadingTrivia, + kind: .method(name: name, arguments: self.arguments.count) + ) + } +} + +private extension SequenceExprSyntax { + /// Detect assignment expression: + /// varName[xxx] = ... // index + /// varName = ... + func referencedVariable(for varName: String) -> ReferencedVariable? { + let exprList = self.elements + guard exprList.count >= 2 else { + return nil + } + let firstExpr = exprList[exprList.startIndex] + let secondExpr = exprList[exprList.index(after: exprList.startIndex)] + guard let assignmentExpr = secondExpr.as(AssignmentExprSyntax.self), + assignmentExpr.equal.tokenKind == .equal else { + // no assignment expression + return nil + } + if let subscrExpr = firstExpr.as(SubscriptCallExprSyntax.self), + subscrExpr.leftSquare.tokenKind == .leftSquare, + subscrExpr.rightSquare.tokenKind == .rightSquare, + let identifierExpr = subscrExpr.calledExpression.as(DeclReferenceExprSyntax.self), + identifierExpr.baseName.tokenKind == .identifier(varName) { + return ReferencedVariable( + name: varName, + position: exprList.positionAfterSkippingLeadingTrivia, + kind: .assignment(subscript: true) + ) + } + if let declExpr = firstExpr.as(DeclReferenceExprSyntax.self), + declExpr.baseName.tokenKind == .identifier(varName) { + return ReferencedVariable( + name: varName, + position: exprList.positionAfterSkippingLeadingTrivia, + kind: .assignment(subscript: false) + ) + } + return nil + } +} diff --git a/Source/SwiftLintBuiltInRules/Rules/Idiomatic/ReduceIntoInsteadOfLoopRuleExamples.swift b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/ReduceIntoInsteadOfLoopRuleExamples.swift new file mode 100644 index 0000000000..d5641bdbb8 --- /dev/null +++ b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/ReduceIntoInsteadOfLoopRuleExamples.swift @@ -0,0 +1,119 @@ +struct ReduceIntoInsteadOfLoopRuleExamples { + static let nonTriggeringExamples: [Example] = [ + Example(""" + let result: [SomeType] = someSequence.reduce(into: [], { result, eachN in + result.insert(eachN) + }) + """), + Example(""" + let result: Set = someSequence.reduce(into: []], { result, eachN in + result.insert(eachN) + }) + """), + Example(""" + let result: [SomeType1: SomeType2] = someSequence.reduce(into: [:], { result, eachN in + result[SomeType1Value] = SomeType2Value + }) + """), + ] + + static let triggeringExamples: [Example] = + triggeringArrayExamples + + triggeringSetExamples + + triggeringDictionaryExamples +} + +extension ReduceIntoInsteadOfLoopRuleExamples { + private static let triggeringDictionaryExamples: [Example] = [ + Example(""" + var result: Dictionary = [:] + for eachN in someSequence { + ↓result[SomeType1Value] = SomeType2Value + eachN + } + """), + Example(""" + var result: Dictionary = [:] + for eachN in someSequence { + ↓result.someMethod(eachN) + } + """), + Example(""" + var result: Dictionary = .init() + for eachN in someSequence { + ↓result.someMethod(eachN) + } + """), + Example(""" + var result = Dictionary() + for eachN in someSequence { + ↓result.someMethod(eachN) + } + """), + ] + + private static let triggeringSetExamples: [Example] = [ + Example(""" + var result: Set = [] + for eachN in someSequence { + ↓result = result + [eachN] + } + """), + Example(""" + var result: Set = [] + for eachN in someSequence { + ↓result.someMethod(eachN) + } + """), + Example(""" + var result: Set = .init() + for eachN in someSequence { + ↓result.someMethod(eachN) + } + """), + Example(""" + var result = Set() + for eachN in someSequence { + ↓result.someMethod(eachN) + } + """), + ] + + private static let triggeringArrayExamples: [Example] = [ + Example(""" + var result: [SomeType] = [] + for eachN in someSequence { + ↓result[5] = eachN + } + """), + Example(""" + var result: [SomeType] = [] + for eachN in someSequence { + ↓result = result + [eachN] + } + """), + Example(""" + var result: [SomeType] = [] + for eachN in someSequence { + ↓result.someMethod(eachN) + } + """), + Example(""" + var result: [SomeType] = .init() + for eachN in someSequence { + ↓result.someMethod(eachN) + } + """), + Example(""" + var result = Array() + for eachN in someSequence { + ↓result.someMethod(eachN) + } + """), + Example(""" + var result = [SomeType]() + for eachN in someSequence { + ↓result.someMethod(eachN) + } + """), + ] +} diff --git a/Source/SwiftLintBuiltInRules/Rules/Idiomatic/ReduceIntoInsteadOfLoopRuleHelpers.swift b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/ReduceIntoInsteadOfLoopRuleHelpers.swift new file mode 100644 index 0000000000..fbcfbccd2f --- /dev/null +++ b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/ReduceIntoInsteadOfLoopRuleHelpers.swift @@ -0,0 +1,27 @@ +import SwiftSyntax + +struct ReduceIntoInsteadOfLoopRuleHelpers { } + +extension VariableDeclSyntax { + /// Binding is a var: "`var` someVar = <...>" + var isVar: Bool { + self.bindingSpecifier.tokenKind == .keyword(.var) + } + + var identifier: String? { + guard let identifierPattern = self.firstPatternOf(IdentifierPatternSyntax.self), + case .identifier(let name) = identifierPattern.identifier.tokenKind else { + return nil + } + return name + } + + /// Returns the first binding with a `pattern` of type + /// `type`. + func firstPatternOf(_ type: T.Type) -> T? { + let result = self.bindings.first { patternBinding in + patternBinding.pattern.as(type) != nil + } + return result?.pattern.as(type) + } +} diff --git a/Source/SwiftLintBuiltInRules/Rules/Idiomatic/ReduceIntoInsteadOfLoopRuleModels.swift b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/ReduceIntoInsteadOfLoopRuleModels.swift new file mode 100644 index 0000000000..1a3523b8c3 --- /dev/null +++ b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/ReduceIntoInsteadOfLoopRuleModels.swift @@ -0,0 +1,55 @@ +import SwiftSyntax + +struct ReduceIntoInsteadOfLoopRuleModels {} + +internal extension ReduceIntoInsteadOfLoopRule { + struct ReferencedVariable: Hashable { + let name: String + let position: AbsolutePosition + let kind: Kind + + func hash(into hasher: inout Hasher) { + hasher.combine(self.name) + hasher.combine(self.position.utf8Offset) + hasher.combine(self.kind) + } + } + + enum Kind: Hashable { + case method(name: String, arguments: Int) + case assignment(subscript: Bool) + + func hash(into hasher: inout Hasher) { + switch self { + case let .method(name, arguments): + hasher.combine("method") + hasher.combine(name) + hasher.combine(arguments) + case let .assignment(`subscript`): + hasher.combine("assignment") + hasher.combine(`subscript`) + } + } + } + + struct CollectionType: Hashable { + let name: String + let genericArguments: Int + + static let types: [Self] = [ + .set, + .array, + .dictionary, + ] + + static let array = Self(name: "Array", genericArguments: 1) + static let set = Self(name: "Set", genericArguments: 1) + static let dictionary = Self(name: "Dictionary", genericArguments: 2) + + static let names: [String: Self] = { + Self.types.reduce(into: [String: Self]()) { partialResult, collectionType in + partialResult[collectionType.name] = collectionType + } + }() + } +} diff --git a/Tests/GeneratedTests/GeneratedTests.swift b/Tests/GeneratedTests/GeneratedTests.swift index 1b65b9e47b..5c33727384 100644 --- a/Tests/GeneratedTests/GeneratedTests.swift +++ b/Tests/GeneratedTests/GeneratedTests.swift @@ -1009,6 +1009,12 @@ final class ReduceBooleanRuleGeneratedTests: SwiftLintTestCase { } } +final class ReduceIntoInsteadOfLoopRuleGeneratedTests: SwiftLintTestCase { + func testWithDefaultConfiguration() { + verifyRule(ReduceIntoInsteadOfLoopRule.description) + } +} + final class ReduceIntoRuleGeneratedTests: SwiftLintTestCase { func testWithDefaultConfiguration() { verifyRule(ReduceIntoRule.description) diff --git a/Tests/IntegrationTests/default_rule_configurations.yml b/Tests/IntegrationTests/default_rule_configurations.yml index 981d667680..16d6198547 100644 --- a/Tests/IntegrationTests/default_rule_configurations.yml +++ b/Tests/IntegrationTests/default_rule_configurations.yml @@ -964,6 +964,11 @@ reduce_into: meta: opt-in: true correctable: false +reduce_into_instead_of_loop: + severity: warning + meta: + opt-in: true + correctable: false redundant_discardable_let: severity: warning ignore_swiftui_view_bodies: false