From b42c5128b188863e8f6afdbc945be654e48a6d67 Mon Sep 17 00:00:00 2001 From: Alfons Hoogervorst Date: Sat, 17 May 2025 14:09:58 +0200 Subject: [PATCH 1/3] Add new rule `unnest_switches_using_tuple` --- CHANGELOG.md | 4 + .../Models/BuiltInRules.swift | 1 + .../Lint/UnnestSwitchesUsingTupleRule.swift | 204 ++++++++++++++++++ Tests/GeneratedTests/GeneratedTests.swift | 6 + .../default_rule_configurations.yml | 5 + 5 files changed, 220 insertions(+) create mode 100644 Source/SwiftLintBuiltInRules/Rules/Lint/UnnestSwitchesUsingTupleRule.swift 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..86e6b61dcf --- /dev/null +++ b/Source/SwiftLintBuiltInRules/Rules/Lint/UnnestSwitchesUsingTupleRule.swift @@ -0,0 +1,204 @@ +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, 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 + } + """), + Example(""" + switch a { + case 1: + let b = something + switch b { + case 1: + break + case 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 + } + case 2: + break + } + """), + Example(""" + switch a { + case 1: + ↓switch b { + case 1: + switch c { + case 1: + break + } + case 2: + 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 parent = node.firstParent(ofKind: .switchExpr), + let parentSwitchExpr = parent.as(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 + } + // we only want the nested switched 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 variables: Set = nestingSwitch + .filter { !$0.hasDeclarationInLabelScope(for: $0.referencedVariable) } + .compactMap { $0.referencedVariable } + .reduce(into: [], { p, e in p.insert(e) }) + + // we want all nested switches to reference the same variable + // to be able to unnest a switch using tuples + guard variables.count == 1 else { + // different variables referenced + return + } + self.violations.append(node.positionAfterSkippingLeadingTrivia) + } + + private var switchNestedSwitches: [SwitchExprSyntax: [SwitchExprSyntax]] = [:] + } +} + +private extension SwitchExprSyntax { + + func hasDeclarationInLabelScope(for variable: String?) -> Bool { + guard let variable else { + return false + } + guard let switchCaseSyntax = self.firstParent(ofKind: .switchCase)?.as(SwitchCaseSyntax.self) else { + return false + } + guard let switchCodeBlockItem = self.firstParent(ofKind: .codeBlockItem)?.as(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 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(ofKind type: SyntaxKind) -> Syntax? { + var someParent: Syntax? = self.parent + while let current = someParent, current.kind != type { + someParent = current.parent + } + return someParent + } +} 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: From 3ab03a2dba9b364cbd386e3f8728a5576fdb4c6a Mon Sep 17 00:00:00 2001 From: Alfons Hoogervorst Date: Sat, 17 May 2025 15:02:35 +0200 Subject: [PATCH 2/3] Use short-hand notation --- .../Rules/Lint/UnnestSwitchesUsingTupleRule.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/SwiftLintBuiltInRules/Rules/Lint/UnnestSwitchesUsingTupleRule.swift b/Source/SwiftLintBuiltInRules/Rules/Lint/UnnestSwitchesUsingTupleRule.swift index 86e6b61dcf..7da33f4482 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Lint/UnnestSwitchesUsingTupleRule.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Lint/UnnestSwitchesUsingTupleRule.swift @@ -131,7 +131,7 @@ private extension UnnestSwitchesUsingTupleRule { let variables: Set = nestingSwitch .filter { !$0.hasDeclarationInLabelScope(for: $0.referencedVariable) } .compactMap { $0.referencedVariable } - .reduce(into: [], { p, e in p.insert(e) }) + .reduce(into: [], { $0.insert($1) }) // we want all nested switches to reference the same variable // to be able to unnest a switch using tuples From f7dc97639e8e3e78d7b4627a6a2ae95409745d7b Mon Sep 17 00:00:00 2001 From: Alfons Hoogervorst Date: Sun, 18 May 2025 22:14:40 +0200 Subject: [PATCH 3/3] Fix rule tightening up conditions --- .../Lint/UnnestSwitchesUsingTupleRule.swift | 205 ++++++++++++------ 1 file changed, 144 insertions(+), 61 deletions(-) diff --git a/Source/SwiftLintBuiltInRules/Rules/Lint/UnnestSwitchesUsingTupleRule.swift b/Source/SwiftLintBuiltInRules/Rules/Lint/UnnestSwitchesUsingTupleRule.swift index 7da33f4482..5be7288fe0 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Lint/UnnestSwitchesUsingTupleRule.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Lint/UnnestSwitchesUsingTupleRule.swift @@ -11,9 +11,48 @@ struct UnnestSwitchesUsingTupleRule: Rule { 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): + case (1, 1): break case (1, 2): break @@ -26,38 +65,24 @@ struct UnnestSwitchesUsingTupleRule: Rule { Example(""" switch (a, b) { case (1, 1): - break + break case (1, 2): break case (2, _): break } """), - Example(""" - switch a { - case 1: - let b = something - switch b { - case 1: - break - case 2: - break - } - case 2: - break - } - """), ], triggeringExamples: [ Example(""" ↓switch a { - case 1: + case 1: switch b { case 1: break case 2: break - } + } case 2: switch b { case 1: @@ -65,35 +90,24 @@ struct UnnestSwitchesUsingTupleRule: Rule { case 2: break } - } + } """), Example(""" ↓switch a { - case 1: + case 1: switch b { case 1: - break + break case 2: - break + break } - case 2: - break - } - """), - Example(""" - switch a { - case 1: - ↓switch b { + default: + switch b { case 1: - switch c { - case 1: - break - } + break case 2: - break + break } - case 2: - break } """), ] @@ -101,14 +115,11 @@ struct UnnestSwitchesUsingTupleRule: Rule { } 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 parent = node.firstParent(ofKind: .switchExpr), - let parentSwitchExpr = parent.as(SwitchExprSyntax.self) else { + guard let parentSwitchExpr = node.firstParent(ofType: SwitchExprSyntax.self) else { return } // file this node, which is a nested switch, under its parent @@ -120,42 +131,83 @@ private extension UnnestSwitchesUsingTupleRule { guard !nestingSwitch.isEmpty else { return } - // we only want the nested switched without a local reference - // to the switch's decl expression, so the following is - // not triggering a violation: + + // 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 variables: Set = nestingSwitch + 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) }) - - // we want all nested switches to reference the same variable - // to be able to unnest a switch using tuples guard variables.count == 1 else { - // different variables referenced 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(ofKind: .switchCase)?.as(SwitchCaseSyntax.self) else { + guard let switchCaseSyntax = self.firstParent(ofType: SwitchCaseSyntax.self) else { return false } - guard let switchCodeBlockItem = self.firstParent(ofKind: .codeBlockItem)?.as(CodeBlockItemSyntax.self) else { + guard let switchCodeBlockItem = self.firstParent(ofType: CodeBlockItemSyntax.self) else { return false } let declReferencingVariable = switchCaseSyntax.statements @@ -163,14 +215,36 @@ private extension SwitchExprSyntax { .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 @@ -186,19 +260,28 @@ private extension CodeBlockItemSyntax { } private extension VariableDeclSyntax { - func references(_ variable: String) -> Bool { self.bindings.first?.pattern.as(IdentifierPatternSyntax.self)?.identifier.text == variable } } private extension SyntaxProtocol { - - func firstParent(ofKind type: SyntaxKind) -> Syntax? { - var someParent: Syntax? = self.parent - while let current = someParent, current.kind != type { + 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 + 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 } }