From 85e2edf44e39553d27652203a67ba54e42643909 Mon Sep 17 00:00:00 2001 From: Alfons Hoogervorst Date: Tue, 13 May 2025 20:47:15 +0200 Subject: [PATCH 1/2] Add rule to detect inefficient use of partialResult --- .../Models/BuiltInRules.swift | 1 + .../NoMutatingPartialResultInReduceRule.swift | 99 +++++++++++++++++++ Tests/GeneratedTests/GeneratedTests.swift | 6 ++ .../default_rule_configurations.yml | 5 + 4 files changed, 111 insertions(+) create mode 100644 Source/SwiftLintBuiltInRules/Rules/Idiomatic/NoMutatingPartialResultInReduceRule.swift diff --git a/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift b/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift index 0138cc743e..8311aaa98b 100644 --- a/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift +++ b/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift @@ -131,6 +131,7 @@ public let builtInRules: [any Rule.Type] = [ NoFallthroughOnlyRule.self, NoGroupingExtensionRule.self, NoMagicNumbersRule.self, + NoMutatingPartialResultInReduceRule.self, NoSpaceInMethodCallRule.self, NonOptionalStringDataConversionRule.self, NonOverridableClassDeclarationRule.self, diff --git a/Source/SwiftLintBuiltInRules/Rules/Idiomatic/NoMutatingPartialResultInReduceRule.swift b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/NoMutatingPartialResultInReduceRule.swift new file mode 100644 index 0000000000..c826aa39c7 --- /dev/null +++ b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/NoMutatingPartialResultInReduceRule.swift @@ -0,0 +1,99 @@ +import SwiftLintCore +import SwiftSyntax + +@SwiftSyntaxRule(optIn: true) +struct NoMutatingPartialResultInReduceRule: Rule { + var configuration = SeverityConfiguration(.error) + + static let description = RuleDescription( + identifier: "no_mutating_partial_result_in_reduce", + name: "No Mutating Partial Result In Reduce", + description: "Do not mutate the partial result in a `reduce` operation", + kind: .idiomatic, + nonTriggeringExamples: [ + ], + triggeringExamples: [ + Example(""" + let array: [Type2] = input.reduce(Type1(), { partialResult, type in + ↓var partialResult = partialResult + partialResult = partialResult + [type] + return partialResult + } + """), + ] + ) +} + +private extension NoMutatingPartialResultInReduceRule { + final class Visitor: ViolationsSyntaxVisitor { + + override func visitPost(_ node: FunctionCallExprSyntax) { + // should be some reduce() + guard let memberAccessExprSyntax = node.calledExpression.as(MemberAccessExprSyntax.self), + memberAccessExprSyntax.declName.baseName.text == "reduce" else { + return + } + guard node.arguments.count == 2 else { + // we should have two arguments to reduce + return + } + guard node.arguments.kind == .labeledExprList, + node.arguments.first?.label?.text != "into" else { + // it's a `reduce(into:_)` + return + } + guard let closureExpr = node.arguments.last?.expression.as(ClosureExprSyntax.self), + let parameterList = closureExpr.signature?.parameterClause?.as(ClosureShorthandParameterListSyntax.self), + let partialResultVarName = parameterList.first?.name.text else { + // could not get the var name of the partial result + return + } + guard let variableDecls = closureExpr.statements.findVariablesReferencing(partialResultVarName) else { + // nothing found + return + } + let absolutePositions = variableDecls + .map(\.positionAfterSkippingLeadingTrivia) + self.violations.append(contentsOf: absolutePositions) + } + } +} + + +extension CodeBlockItemListSyntax { + + func findVariablesReferencing(_ name: String) -> [VariableDeclSyntax]? { + + func asVariableDecl(_ syntax: CodeBlockItemSyntax) -> VariableDeclSyntax? { + return syntax.item.as(VariableDeclSyntax.self) + } + + func isAssignmentToAVariable(_ variableDeclSyntax: VariableDeclSyntax) -> Bool { + return variableDeclSyntax.bindingSpecifier.tokenKind == .keyword(.var) + } + + func isReferencingOurVariable(_ variableDeclSyntax: VariableDeclSyntax) -> Bool { + guard let binding = variableDeclSyntax.bindings.first else { + // no variable assignment + return false + } + guard binding.pattern.is(IdentifierPatternSyntax.self) else { + // no identifier + return false + } + guard let initializer = binding.initializer else { + // no assignment + return false + } + guard let rhsValue = initializer.value.as(DeclReferenceExprSyntax.self) else { + return false + } + return rhsValue.baseName.text == name + } + let variableDecls = self.compactMap(asVariableDecl) + .filter(isAssignmentToAVariable) + .filter(isReferencingOurVariable) + return variableDecls.count > 0 ? variableDecls : nil + } + +} diff --git a/Tests/GeneratedTests/GeneratedTests.swift b/Tests/GeneratedTests/GeneratedTests.swift index 1b65b9e47b..569f31058b 100644 --- a/Tests/GeneratedTests/GeneratedTests.swift +++ b/Tests/GeneratedTests/GeneratedTests.swift @@ -781,6 +781,12 @@ final class NoMagicNumbersRuleGeneratedTests: SwiftLintTestCase { } } +final class NoMutatingPartialResultInReduceRuleGeneratedTests: SwiftLintTestCase { + func testWithDefaultConfiguration() { + verifyRule(NoMutatingPartialResultInReduceRule.description) + } +} + final class NoSpaceInMethodCallRuleGeneratedTests: SwiftLintTestCase { func testWithDefaultConfiguration() { verifyRule(NoSpaceInMethodCallRule.description) diff --git a/Tests/IntegrationTests/default_rule_configurations.yml b/Tests/IntegrationTests/default_rule_configurations.yml index 981d667680..c7940e99d5 100644 --- a/Tests/IntegrationTests/default_rule_configurations.yml +++ b/Tests/IntegrationTests/default_rule_configurations.yml @@ -728,6 +728,11 @@ no_magic_numbers: meta: opt-in: true correctable: false +no_mutating_partial_result_in_reduce: + severity: warning + meta: + opt-in: true + correctable: false no_space_in_method_call: severity: warning meta: From fcb7e5bec44ac1166bf8d55c98b1e44e9cdafe0f Mon Sep 17 00:00:00 2001 From: Alfons Hoogervorst Date: Wed, 14 May 2025 06:20:31 +0200 Subject: [PATCH 2/2] Refactor a bit --- .../NoMutatingPartialResultInReduceRule.swift | 146 ++++++++++++------ 1 file changed, 95 insertions(+), 51 deletions(-) diff --git a/Source/SwiftLintBuiltInRules/Rules/Idiomatic/NoMutatingPartialResultInReduceRule.swift b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/NoMutatingPartialResultInReduceRule.swift index c826aa39c7..28e15a5e5b 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Idiomatic/NoMutatingPartialResultInReduceRule.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/NoMutatingPartialResultInReduceRule.swift @@ -20,6 +20,13 @@ struct NoMutatingPartialResultInReduceRule: Rule { return partialResult } """), + Example(""" + let value = input.reduce(0, { accum, inc in + ↓var accum = accum + accum = accum + 1 + return accum + } + """) ] ) } @@ -27,73 +34,110 @@ struct NoMutatingPartialResultInReduceRule: Rule { private extension NoMutatingPartialResultInReduceRule { final class Visitor: ViolationsSyntaxVisitor { + /// Looking for: + /// * a function call `reduce`, + /// * with a trailing closure, + /// * the closure's first argument (the accumulator) + /// * get the variable declarations that reference + /// the accumulator, override func visitPost(_ node: FunctionCallExprSyntax) { - // should be some reduce() - guard let memberAccessExprSyntax = node.calledExpression.as(MemberAccessExprSyntax.self), - memberAccessExprSyntax.declName.baseName.text == "reduce" else { - return - } - guard node.arguments.count == 2 else { - // we should have two arguments to reduce - return - } - guard node.arguments.kind == .labeledExprList, - node.arguments.first?.label?.text != "into" else { - // it's a `reduce(into:_)` - return - } - guard let closureExpr = node.arguments.last?.expression.as(ClosureExprSyntax.self), - let parameterList = closureExpr.signature?.parameterClause?.as(ClosureShorthandParameterListSyntax.self), - let partialResultVarName = parameterList.first?.name.text else { + guard node.isReduceFunctionCall() else { return } + guard let partialResultVarName = node.reducePartialResultVariableName() else { // could not get the var name of the partial result return } - guard let variableDecls = closureExpr.statements.findVariablesReferencing(partialResultVarName) else { - // nothing found + guard let variables = node.trailingClosure()?.variablesReferencing(partialResultVarName) else { + // no variables referencing partial result variable return } - let absolutePositions = variableDecls + let absolutePositions = variables .map(\.positionAfterSkippingLeadingTrivia) self.violations.append(contentsOf: absolutePositions) } } } - -extension CodeBlockItemListSyntax { +private extension FunctionCallExprSyntax { - func findVariablesReferencing(_ name: String) -> [VariableDeclSyntax]? { - - func asVariableDecl(_ syntax: CodeBlockItemSyntax) -> VariableDeclSyntax? { - return syntax.item.as(VariableDeclSyntax.self) + func isReduceFunctionCall() -> Bool { + // should be some reduce() + guard let memberAccessExprSyntax = self.calledExpression.as(MemberAccessExprSyntax.self), + memberAccessExprSyntax.declName.baseName.text == "reduce" else { + return false } - - func isAssignmentToAVariable(_ variableDeclSyntax: VariableDeclSyntax) -> Bool { - return variableDeclSyntax.bindingSpecifier.tokenKind == .keyword(.var) + guard self.arguments.count == 2 else { + // we should have two arguments to reduce + return false } - - func isReferencingOurVariable(_ variableDeclSyntax: VariableDeclSyntax) -> Bool { - guard let binding = variableDeclSyntax.bindings.first else { - // no variable assignment - return false - } - guard binding.pattern.is(IdentifierPatternSyntax.self) else { - // no identifier - return false - } - guard let initializer = binding.initializer else { - // no assignment - return false - } - guard let rhsValue = initializer.value.as(DeclReferenceExprSyntax.self) else { - return false - } - return rhsValue.baseName.text == name + guard self.arguments.kind == .labeledExprList, + self.arguments.first?.label?.text != "into" else { + // it's a `reduce(into:_)` + return false } - let variableDecls = self.compactMap(asVariableDecl) - .filter(isAssignmentToAVariable) - .filter(isReferencingOurVariable) - return variableDecls.count > 0 ? variableDecls : nil + guard self.arguments.last?.expression.as(ClosureExprSyntax.self) != nil else { + // no closure + return false + } + return true + } + + func reducePartialResultVariableName() -> String? { + guard let closureExpr = self.arguments.last?.expression.as(ClosureExprSyntax.self), + let paramList = closureExpr.signature?.parameterClause?.as(ClosureShorthandParameterListSyntax.self), + let partialResultVarName = paramList.first?.name.text else { + // could not get the var name of the partial result + return nil + } + return partialResultVarName + } + + func trailingClosure() -> ClosureExprSyntax? { + return self.arguments.last?.expression.as(ClosureExprSyntax.self) + } +} + +private extension ClosureExprSyntax { + + func variablesReferencing(_ name: String) -> [VariableDeclSyntax]? { + return self.statements.variablesReferencing(name) } +} + +private extension CodeBlockItemListSyntax { + func variablesReferencing(_ name: String) -> [VariableDeclSyntax]? { + let variableDecls = self.compactMap(\.asVariableDecl) + .filter(\.isAssignmentToAVariable) + .filter(VariableReference(name: name).matches(_:)) + return !variableDecls.isEmpty ? variableDecls : nil + } +} + +private extension CodeBlockItemSyntax { + + var asVariableDecl: VariableDeclSyntax? { + return self.item.as(VariableDeclSyntax.self) + } +} + +private extension VariableDeclSyntax { + + var isAssignmentToAVariable: Bool { + return self.bindingSpecifier.tokenKind == .keyword(.var) + } +} + +private struct VariableReference { + let name: String + + func matches(_ variableDeclSyntax: VariableDeclSyntax) -> Bool { + // should be a variable assignment, with identifier and assignment + guard let binding = variableDeclSyntax.bindings.first, + binding.pattern.is(IdentifierPatternSyntax.self), + let initializer = binding.initializer, + let rhsValue = initializer.value.as(DeclReferenceExprSyntax.self) else { + return false + } + return rhsValue.baseName.text == name + } }