diff --git a/CHANGELOG.md b/CHANGELOG.md index b1b5a96b8b..cdba06db98 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,10 @@ * Add new `allowed_numbers` option to the `no_magic_numbers` rule. [Martin Redington](https://github.com/mildm8nnered) +* Add new `unnest_switches_using_tuple` opt-in rule that triggers when + nested switches are encountered that reference the same variable. These + can be replaced by a switch on a tuple. + [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..3b854e04a0 100644 --- a/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift +++ b/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift @@ -221,6 +221,7 @@ public let builtInRules: [any Rule.Type] = [ UnneededOverrideRule.self, UnneededParenthesesInClosureArgumentRule.self, UnneededSynthesizedInitializerRule.self, + UnnestSwitchesUsingTupleRule.self, UnownedVariableCaptureRule.self, UntypedErrorInCatchRule.self, UnusedClosureParameterRule.self, diff --git a/Source/SwiftLintBuiltInRules/Rules/Lint/UnnestSwitchesUsingTupleRule.swift b/Source/SwiftLintBuiltInRules/Rules/Lint/UnnestSwitchesUsingTupleRule.swift new file mode 100644 index 0000000000..5be7288fe0 --- /dev/null +++ b/Source/SwiftLintBuiltInRules/Rules/Lint/UnnestSwitchesUsingTupleRule.swift @@ -0,0 +1,287 @@ +import SwiftLintCore +import SwiftSyntax + +@SwiftSyntaxRule(optIn: true) +struct UnnestSwitchesUsingTupleRule: Rule { + var configuration = SeverityConfiguration(.warning) + + static let description = RuleDescription( + identifier: "unnest_switches_using_tuple", + name: "Unnest Switches Using Tuple", + description: "Prevent nesting switches by preferring a switch on a tuple", + kind: .lint, + nonTriggeringExamples: [ + Example(""" + switch a { + case 1: + switch b { + case 1: + break + case 2: + break + } + case 2: + let b = 5 + switch b { + case 1: + break + case 2: + break + } + } + """), + Example(""" + switch a { + case 1: + if (d) { + switch b { + case 1: + break + case 2: + break + } + } + case 2: + switch b { + case 1: + break + case 2: + break + } + } + """), + Example(""" + switch (a, b) { + case (1, 1): + break + case (1, 2): + break + case (2, 1): + break + case (2, 2): + break + } + """), + Example(""" + switch (a, b) { + case (1, 1): + break + case (1, 2): + break + case (2, _): + break + } + """), + ], + triggeringExamples: [ + Example(""" + ↓switch a { + case 1: + switch b { + case 1: + break + case 2: + break + } + case 2: + switch b { + case 1: + break + case 2: + break + } + } + """), + Example(""" + ↓switch a { + case 1: + switch b { + case 1: + break + case 2: + break + } + default: + switch b { + case 1: + break + case 2: + break + } + } + """), + ] + ) +} + +private extension UnnestSwitchesUsingTupleRule { + final class Visitor: ViolationsSyntaxVisitor { + override func visitPost(_ node: SwitchExprSyntax) { + guard let nestingSwitch = self.switchNestedSwitches[node] else { + // does this switch have a parent switch? + guard let parentSwitchExpr = node.firstParent(ofType: SwitchExprSyntax.self) else { + return + } + // file this node, which is a nested switch, under its parent + self.switchNestedSwitches[parentSwitchExpr, default: []].append(node) + return + } + // check the parent switch + defer { self.switchNestedSwitches = [:] } + guard !nestingSwitch.isEmpty else { + return + } + + // 1. We want only those nested switches that are in the + // scope of the case label + let switchesWithDirectParent = nestingSwitch + .filter { $0.isNestedSwitchCandidate() } + guard switchesWithDirectParent.count == nestingSwitch.count else { + return + } + + // 2. We only want the nested switches without a local reference + // to the switch's decl expression, so the following is not + // triggering a violation: + // + // let b = 1 // or a var + // switch (b) { + // } + // + let switchesWithoutLocalVarRefs = nestingSwitch + .filter { !$0.hasDeclarationInLabelScope(for: $0.referencedVariable) } + guard nestingSwitch.count == switchesWithoutLocalVarRefs.count else { + return + } + + // 3. The number of cases should be the same as the number + // of nested switches, so this will not trigger: + // + // switch (...) { + // case <1>: + // switch { + // } + // case <2>: + // // some other expressions + // } + guard nestingSwitch.count == node.cases.count else { + return + } + + // 4. Check that each of the nested switches references the + // _same_ variable. + let variables: Set = nestingSwitch + .compactMap { $0.referencedVariable } + .reduce(into: [], { $0.insert($1) }) + guard variables.count == 1 else { + return + } + + self.violations.append(node.positionAfterSkippingLeadingTrivia) + } + + private var switchNestedSwitches: [SwitchExprSyntax: [SwitchExprSyntax]] = [:] + } +} + +private extension SwitchExprSyntax { + func isNestedSwitchCandidate() -> Bool { + // we're looking for a nested switch that has a specific + // tree hierarchy: there should be a direct line to the + // parent switch + let parentTypesPath: [any SyntaxProtocol.Type] = [ + SwitchExprSyntax.self, + SwitchCaseListSyntax.self, + SwitchCaseSyntax.self, + CodeBlockItemListSyntax.self, + CodeBlockItemSyntax.self, + ExpressionStmtSyntax.self, + ].reversed() + return self.isBackwardsTraversable(using: parentTypesPath) + } + + func hasDeclarationInLabelScope(for variable: String?) -> Bool { + guard let variable else { + return false + } + guard let switchCaseSyntax = self.firstParent(ofType: SwitchCaseSyntax.self) else { + return false + } + guard let switchCodeBlockItem = self.firstParent(ofType: CodeBlockItemSyntax.self) else { + return false + } + let declReferencingVariable = switchCaseSyntax.statements + .prefix { $0 != switchCodeBlockItem } + .first { $0.containsReference(to: variable) } + return declReferencingVariable != nil + } + + var referencedVariable: String? { + self.subject.as(DeclReferenceExprSyntax.self)?.baseName.text + } +} + +private extension SwitchCaseSyntax { + func allStatements() -> [Syntax] { + self.statements + .map { Syntax($0.item) } as [Syntax] + } + + func variableDeclReferencing(_ variable: String?) -> VariableDeclSyntax? { + guard let variable else { + return nil + } + let allStatements = self.allStatements() + guard allStatements.isEmpty == false else { + return nil + } + // 1 & 2: get all the variable decl up to the nested switch statement + // 3: get the first one which references the variable + return allStatements + .prefix { $0.as(ExpressionStmtSyntax.self)?.expression.kind != .switchExpr } + .compactMap { $0.as(VariableDeclSyntax.self) } + .first { $0.references(variable) } + } +} + +private extension CodeBlockItemSyntax { + func containsReference(to variable: String) -> Bool { + guard self.item.kind == .variableDecl else { + return false + } + guard let variableDecl = self.item.as(VariableDeclSyntax.self) else { + return false + } + guard variableDecl.references(variable) else { + return false + } + return true + } +} + +private extension VariableDeclSyntax { + func references(_ variable: String) -> Bool { + self.bindings.first?.pattern.as(IdentifierPatternSyntax.self)?.identifier.text == variable + } +} + +private extension SyntaxProtocol { + func firstParent(ofType type: T.Type) -> T? { + var someParent = self.parent + while let current = someParent, current.as(type.self) == nil { + someParent = current.parent + } + return someParent?.as(type.self) + } + + func isBackwardsTraversable(using path: [any SyntaxProtocol.Type]?) -> Bool { + guard let path else { + return false + } + let mySelf: (any SyntaxProtocol)? = self + let parent = path.reduce(mySelf) { partialResult, parentType in + partialResult?.parent?.as(parentType) + } + return parent != nil + } +} diff --git a/Tests/GeneratedTests/GeneratedTests.swift b/Tests/GeneratedTests/GeneratedTests.swift index 1b65b9e47b..365bceb552 100644 --- a/Tests/GeneratedTests/GeneratedTests.swift +++ b/Tests/GeneratedTests/GeneratedTests.swift @@ -1321,6 +1321,12 @@ final class UnneededSynthesizedInitializerRuleGeneratedTests: SwiftLintTestCase } } +final class UnnestSwitchesUsingTupleRuleGeneratedTests: SwiftLintTestCase { + func testWithDefaultConfiguration() { + verifyRule(UnnestSwitchesUsingTupleRule.description) + } +} + final class UnownedVariableCaptureRuleGeneratedTests: SwiftLintTestCase { func testWithDefaultConfiguration() { verifyRule(UnownedVariableCaptureRule.description) diff --git a/Tests/IntegrationTests/default_rule_configurations.yml b/Tests/IntegrationTests/default_rule_configurations.yml index 981d667680..9a577bdf85 100644 --- a/Tests/IntegrationTests/default_rule_configurations.yml +++ b/Tests/IntegrationTests/default_rule_configurations.yml @@ -1262,6 +1262,11 @@ unneeded_synthesized_initializer: meta: opt-in: false correctable: true +unnest_switches_using_tuple: + severity: warning + meta: + opt-in: true + correctable: false unowned_variable_capture: severity: warning meta: