From 27f217508c1e097f74274027b783103364b6a8c2 Mon Sep 17 00:00:00 2001 From: Leonardo Rodrigues Date: Sun, 20 Oct 2024 18:19:35 +1100 Subject: [PATCH 1/2] Add prefer_if_and_switch_expressions rule --- .swiftlint.yml | 1 + .../Models/BuiltInRules.swift | 1 + .../PreferIfAndSwitchExpressionsRule.swift | 108 +++++++++++++++ ...erIfAndSwitchExpressionsRuleExamples.swift | 126 ++++++++++++++++++ Tests/GeneratedTests/GeneratedTests.swift | 6 + .../default_rule_configurations.yml | 2 + 6 files changed, 244 insertions(+) create mode 100644 Source/SwiftLintBuiltInRules/Rules/Idiomatic/PreferIfAndSwitchExpressionsRule.swift create mode 100644 Source/SwiftLintBuiltInRules/Rules/Idiomatic/PreferIfAndSwitchExpressionsRuleExamples.swift diff --git a/.swiftlint.yml b/.swiftlint.yml index 8dc4c564ed..5fd5856240 100644 --- a/.swiftlint.yml +++ b/.swiftlint.yml @@ -37,6 +37,7 @@ disabled_rules: - no_grouping_extension - no_magic_numbers - one_declaration_per_file + - prefer_if_and_switch_expressions - prefer_nimble - prefixed_toplevel_constant - required_deinit diff --git a/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift b/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift index eeb3abce7c..499a0a485b 100644 --- a/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift +++ b/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift @@ -149,6 +149,7 @@ public let builtInRules: [any Rule.Type] = [ OverrideInExtensionRule.self, PatternMatchingKeywordsRule.self, PeriodSpacingRule.self, + PreferIfAndSwitchExpressionsRule.self, PreferKeyPathRule.self, PreferNimbleRule.self, PreferSelfInStaticReferencesRule.self, diff --git a/Source/SwiftLintBuiltInRules/Rules/Idiomatic/PreferIfAndSwitchExpressionsRule.swift b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/PreferIfAndSwitchExpressionsRule.swift new file mode 100644 index 0000000000..06959ab4ea --- /dev/null +++ b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/PreferIfAndSwitchExpressionsRule.swift @@ -0,0 +1,108 @@ +import SwiftOperators +import SwiftSyntax + +@SwiftSyntaxRule +struct PreferIfAndSwitchExpressionsRule: OptInRule { + var configuration = SeverityConfiguration(.warning) + + static let description = RuleDescription( + identifier: "prefer_if_and_switch_expressions", + name: "Prefer if and switch expressions", + description: "Use if and switch expressions instead of statements that perform similar actions in its branches", + kind: .idiomatic, + nonTriggeringExamples: PreferIfAndSwitchExpressionsRuleExamples.nonTriggeringExamples, + triggeringExamples: PreferIfAndSwitchExpressionsRuleExamples.triggeringExamples + ) +} + +private extension PreferIfAndSwitchExpressionsRule { + final class Visitor: ViolationsSyntaxVisitor { + override func visitPost(_ node: IfExprSyntax) { + guard node.parent?.is(IfExprSyntax.self) != true else { return } + + if let actions = node.actionInBranches(), actionToReplace(actionsInBranches: actions) != nil { + violations.append(node.positionAfterSkippingLeadingTrivia) + } + } + + override func visitPost(_ node: SwitchExprSyntax) { + let cases = node.cases.compactMap { $0.as(SwitchCaseSyntax.self) } + let actions = cases.map { $0.statements.extractSingleAction() } + if actionToReplace(actionsInBranches: actions) != nil { + violations.append(node.positionAfterSkippingLeadingTrivia) + } + } + + /// Compares the actions and return the equivalent action that would replace them. + func actionToReplace(actionsInBranches actions: [Action?]) -> Action? { + let validActions = actions.filter { $0 != .throw } + + let nonNilActions = validActions.compactMap { $0 } + guard nonNilActions.count == validActions.count, nonNilActions.count >= 2 else { + return nil + } + + return if nonNilActions.dropFirst().allSatisfy({ $0 == nonNilActions.first }) { + nonNilActions.first + } else { + nil + } + } + } +} + +/// Represents an action executed in one branch of an if or switch expression +/// that may cause a violation of the rule. +private enum Action: Equatable { + case assignment(identifier: String) + case `return` + case `throw` +} + +private extension IfExprSyntax { + /// Retrieves the list of `Action`. + /// Returns `nil` if there is no unconditional else block. + func actionInBranches() -> [Action?]? { + guard let elseBody else { return nil } + + var branchesActions = switch elseBody { + case let .ifExpr(ifExprSyntax): + ifExprSyntax.actionInBranches() + case let .codeBlock(finalElse): + [finalElse.statements.extractSingleAction()] + } + + if branchesActions != nil { + let thenBlockAction = body.statements.extractSingleAction() + branchesActions?.append(thenBlockAction) + } + + return branchesActions + } +} + +private extension CodeBlockItemListSyntax { + /// Extracts the only action in the block. + func extractSingleAction() -> Action? { + guard let statement = onlyElement else { return nil } + + if statement.item.is(ReturnStmtSyntax.self) { + return .return + } + + if statement.item.is(ThrowStmtSyntax.self) { + return .throw + } + + if + let sequenceExpr = statement.item.as(SequenceExprSyntax.self), + let folded = try? OperatorTable.standardOperators.foldSingle(sequenceExpr), + let operatorExpr = folded.as(InfixOperatorExprSyntax.self), + operatorExpr.operator.is(AssignmentExprSyntax.self), + let declReference = operatorExpr.leftOperand.as(DeclReferenceExprSyntax.self) { + return .assignment(identifier: declReference.baseName.text) + } + + return nil + } +} diff --git a/Source/SwiftLintBuiltInRules/Rules/Idiomatic/PreferIfAndSwitchExpressionsRuleExamples.swift b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/PreferIfAndSwitchExpressionsRuleExamples.swift new file mode 100644 index 0000000000..f6fbde5e24 --- /dev/null +++ b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/PreferIfAndSwitchExpressionsRuleExamples.swift @@ -0,0 +1,126 @@ +internal struct PreferIfAndSwitchExpressionsRuleExamples { + static let nonTriggeringExamples = [ + Example(""" + if cond { + // Nothing + } else { + return 2 + } + return 1 + """), + Example(""" + if cond { + print("Hey") + return 1 + } else { + return 2 + } + """), + Example(""" + return if cond { + 1 + } else { + 2 + } + """), + Example(""" + return if cond { + 1 + } else { + 2 + } + """), + Example(""" + if cond { + return 1 + } else { + throw TestError.test + } + """), + Example(""" + let x = switch value { + case 1: + "One" + case 2: + "Two" + default: + fatalError() + } + """), + Example(""" + if value { + x = "One" + } else { + y = "Two" + } + """), + Example(""" + switch value { + case 1: + x = "One" + case 2: + y = "Two" + default: + z = "Three" + } + """), + ] + + static let triggeringExamples = [ + Example(""" + func f(cond: Bool) -> Int { + ↓if cond { + return 1 + } else if self.otherCond { + return 2 + } else { + return 3 + } + } + """), + Example(""" + func f(cond: Bool) { + let r: Int + ↓if cond { + r = 1 + } else { + // Some comment + r = 2 + } + } + """), + Example(""" + func test(x: Int) throws -> String { + ↓switch x { + case 1: + return "One" + case 2: + return "Two" + default: + return "More" + } + } + """), + Example(""" + func test(x: Int) throws -> String { + ↓switch x { + case 1: + return "One" + case 2: + return "Two" + default: + throw TestError.test + } + } + """), + Example(""" + init(rawValue: String) throws { + ↓switch rawValue { + case "lemon": self = .lemon + case "lime": self = .lime + default: throw TestError.test + } + } + """), + ] +} diff --git a/Tests/GeneratedTests/GeneratedTests.swift b/Tests/GeneratedTests/GeneratedTests.swift index d00101a90b..0d14706ff1 100644 --- a/Tests/GeneratedTests/GeneratedTests.swift +++ b/Tests/GeneratedTests/GeneratedTests.swift @@ -883,6 +883,12 @@ final class PeriodSpacingRuleGeneratedTests: SwiftLintTestCase { } } +final class PreferIfAndSwitchExpressionsRuleGeneratedTests: SwiftLintTestCase { + func testWithDefaultConfiguration() { + verifyRule(PreferIfAndSwitchExpressionsRule.description) + } +} + final class PreferKeyPathRuleGeneratedTests: SwiftLintTestCase { func testWithDefaultConfiguration() { verifyRule(PreferKeyPathRule.description) diff --git a/Tests/IntegrationTests/default_rule_configurations.yml b/Tests/IntegrationTests/default_rule_configurations.yml index 57148068b8..9766893045 100644 --- a/Tests/IntegrationTests/default_rule_configurations.yml +++ b/Tests/IntegrationTests/default_rule_configurations.yml @@ -404,6 +404,8 @@ pattern_matching_keywords: severity: warning period_spacing: severity: warning +prefer_if_and_switch_expressions: + severity: warning prefer_key_path: severity: warning restrict_to_standard_functions: true From 9a00c26f203f80f5a41752843bb663ffb4170631 Mon Sep 17 00:00:00 2001 From: Leonardo Rodrigues Date: Mon, 21 Oct 2024 17:44:11 +1100 Subject: [PATCH 2/2] Temp mark rule as enabled by default --- .../Rules/Idiomatic/PreferIfAndSwitchExpressionsRule.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/SwiftLintBuiltInRules/Rules/Idiomatic/PreferIfAndSwitchExpressionsRule.swift b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/PreferIfAndSwitchExpressionsRule.swift index 06959ab4ea..d29f26a1ca 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Idiomatic/PreferIfAndSwitchExpressionsRule.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/PreferIfAndSwitchExpressionsRule.swift @@ -2,7 +2,7 @@ import SwiftOperators import SwiftSyntax @SwiftSyntaxRule -struct PreferIfAndSwitchExpressionsRule: OptInRule { +struct PreferIfAndSwitchExpressionsRule: Rule { var configuration = SeverityConfiguration(.warning) static let description = RuleDescription(