From 407f3c47becb2977dfbee9be0b58b4ab963290bd Mon Sep 17 00:00:00 2001 From: Rodion Ivashkov Date: Tue, 10 Mar 2026 01:48:44 +0300 Subject: [PATCH 1/3] Improve pattern_matching_keywords SwiftSyntax rule --- .../PatternMatchingKeywordsRule.swift | 247 ++++++++++++++---- .../PatternMatchingKeywordsRuleTests.swift | 131 ++++++++++ 2 files changed, 331 insertions(+), 47 deletions(-) create mode 100644 Tests/BuiltInRulesTests/PatternMatchingKeywordsRuleTests.swift diff --git a/Source/SwiftLintBuiltInRules/Rules/Idiomatic/PatternMatchingKeywordsRule.swift b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/PatternMatchingKeywordsRule.swift index 0e1510b976..6ce5ef7b0f 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Idiomatic/PatternMatchingKeywordsRule.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/PatternMatchingKeywordsRule.swift @@ -7,9 +7,12 @@ struct PatternMatchingKeywordsRule: Rule { static let description = RuleDescription( identifier: "pattern_matching_keywords", name: "Pattern Matching Keywords", - description: "Combine multiple pattern matching bindings by moving keywords out of tuples", + description: """ + Combine multiple pattern matching bindings by moving keywords out of tuples + and enum associated values + """, kind: .idiomatic, - nonTriggeringExamples: [ + nonTriggeringExamples: switchWrapped(examples: [ Example("default"), Example("case 1"), Example("case bar"), @@ -22,88 +25,238 @@ struct PatternMatchingKeywordsRule: Rule { Example("case .foo(var x)"), Example("case var .foo(x, y)"), Example("case (y, let x, z)"), - ].map(wrapInSwitch), - triggeringExamples: [ - Example("case (↓let x, ↓let y)"), - Example("case (↓let x, ↓let y, .foo)"), - Example("case (↓let x, ↓let y, _)"), - Example("case (↓let x, ↓let y, f())"), - Example("case (↓let x, ↓let y, s.f())"), - Example("case (↓let x, ↓let y, s.t)"), + Example("case (foo, let x)"), + Example("case (foo, let x, let y)"), + Example("case .foo(bar, let x)"), + Example("case (let x, y)"), + Example("case .foo(let x, y)"), + Example("case (.foo(let x), y)"), + Example("case let .foo(x, y), let .bar(x, y)"), + Example("case var .foo(x, y), var .bar(x, y)"), + Example("case let .foo(x, y), let .bar(x, y), let .baz(x, y)"), + Example("case .foo(bar: let x, baz: var y)"), + Example("case (.yamlParsing(var x), (.yamlParsing(var y), z))"), + Example("case (.foo(let x), (y, let z))"), + ]) + [ + Example("if case let (x, y) = foo {}"), + Example("guard case let (x, y) = foo else { return }"), + Example("while case let (x, y) = foo {}"), + Example("for case let (x, y) in foos {}"), + Example("if case (foo, let x) = value {}"), + Example("guard case .foo(bar, let x) = value else { return }"), + Example("do {} catch let Pattern.error(x, y) {}"), + Example("do {} catch (foo, let x) {}"), + ], + triggeringExamples: switchWrapped(examples: [ + Example("case (↓let x, ↓let y)"), + Example("case (↓let x, ↓let y, .foo)"), + Example("case (↓let x, ↓let y, _)"), + Example("case (↓let x, ↓let y, 1)"), + Example("case (↓let x, ↓let y, f())"), + Example("case (↓let x, ↓let y, s.f())"), + Example("case (↓let x, ↓let y, s.t)"), Example("case .foo(↓let x, ↓let y)"), - Example("case (.yamlParsing(↓let x), .yamlParsing(↓let y))"), - Example("case (↓var x, ↓var y)"), + Example("case .foo(bar: ↓let x, baz: ↓let y)"), Example("case .foo(↓var x, ↓var y)"), - Example("case (.yamlParsing(↓var x), .yamlParsing(↓var y))"), - ].map(wrapInSwitch) + Example("case .foo(bar: ↓var x, baz: ↓var y)"), + Example("case (.yamlParsing(↓let x), .yamlParsing(↓let y))"), + Example("case (.yamlParsing(↓var x), (.yamlParsing(↓var y), _))"), + Example("case ((↓let x, ↓let y), z)"), + Example("case .foo((↓let x, ↓let y), z)"), + Example("case (.foo(↓let x, ↓let y), z)"), + Example("case .foo(.bar(↓let x), .bar(↓let y))"), + Example("case .foo(.bar(↓let x), .bar(↓let y), .baz)"), + Example("case .foo(↓let x, ↓let y), .bar(↓let x, ↓let y)"), + Example("case .foo(↓var x, ↓var y), .bar(↓var x, ↓var y)"), + ]) + [ + Example("if case (↓let x, ↓let y) = foo {}"), + Example("guard case (↓let x, ↓let y) = foo else { return }"), + Example("while case (↓let x, ↓let y) = foo {}"), + Example("for case (↓let x, ↓let y) in foos {}"), + Example("if case .foo(bar: ↓let x, baz: ↓let y) = value {}"), + Example("do {} catch Pattern.error(↓let x, ↓let y) {}"), + Example("do {} catch (↓let x, ↓let y) {}"), + Example("do {} catch Foo.outer(.inner(↓let x), .inner(↓let y)) {}"), + ] ) - private static func wrapInSwitch(_ example: Example) -> Example { - example.with(code: """ - switch foo { - \(example.code): break - } - """) + private static func switchWrapped(examples: [Example]) -> [Example] { + examples.map { example in + example.with( + code: """ + switch foo { + \(example.code): break + } + """ + ) + } } } private extension PatternMatchingKeywordsRule { final class Visitor: ViolationsSyntaxVisitor { override func visitPost(_ node: SwitchCaseItemSyntax) { - let localViolations = TupleVisitor(configuration: configuration, file: file) - .walk(tree: node.pattern, handler: \.violations) - violations.append(contentsOf: localViolations) + appendViolations(from: node.pattern) + } + + override func visitPost(_ node: MatchingPatternConditionSyntax) { + appendViolations(from: node.pattern) + } + + override func visitPost(_ node: ForStmtSyntax) { + guard node.caseKeyword != nil else { + return + } + + appendViolations(from: node.pattern) + } + + override func visitPost(_ node: CatchItemSyntax) { + guard let pattern = node.pattern else { + return + } + + appendViolations(from: pattern) + } + + private func appendViolations(from pattern: PatternSyntax) { + violations.append(contentsOf: PatternViolationCollector.collect(in: pattern)) } } } -private final class TupleVisitor: ViolationsSyntaxVisitor { - override func visitPost(_ node: LabeledExprListSyntax) { - let list = node.flatteningEnumPatterns().map(\.expression.categorized) - if list.contains(where: \.isReference) { +private enum PatternViolationCollector { + static func collect(in pattern: PatternSyntax) -> [AbsolutePosition] { + var violations: [AbsolutePosition] = [] + collect(from: pattern, into: &violations) + return violations + } + + private static func collect(from pattern: PatternSyntax, into violations: inout [AbsolutePosition]) { + if let binding = pattern.as(ValueBindingPatternSyntax.self) { + collect(from: binding.pattern, into: &violations) return } - let specifiers = list.compactMap { if case let .binding(specifier) = $0 { specifier } else { nil } } - if specifiers.count > 1, specifiers.allSatisfy({ $0.tokenKind == specifiers.first?.tokenKind }) { - violations.append(contentsOf: specifiers.map(\.positionAfterSkippingLeadingTrivia)) + + guard let expression = pattern.as(ExpressionPatternSyntax.self)?.expression else { + return } + + collect(from: expression, into: &violations) } -} -private extension LabeledExprListSyntax { - func flatteningEnumPatterns() -> [LabeledExprSyntax] { - flatMap { elem in - guard let pattern = elem.expression.as(FunctionCallExprSyntax.self), - pattern.calledExpression.is(MemberAccessExprSyntax.self) else { - return [elem] + private static func collect(from expression: ExprSyntax, into violations: inout [AbsolutePosition]) { + if let childExpressions = expression.immediatePatternGroupChildren { + appendViolationsIfBindingsCanBeLifted(from: childExpressions, into: &violations) + + for childExpression in childExpressions { + collect(from: childExpression, into: &violations) } - return Array(pattern.arguments) + return + } + + if let pattern = expression.as(PatternExprSyntax.self)?.pattern { + collect(from: pattern, into: &violations) + } + } + + private static func appendViolationsIfBindingsCanBeLifted( + from expressions: [ExprSyntax], + into violations: inout [AbsolutePosition] + ) { + let categories = expressions.map(GroupCategoryResolver.category(for:)) + + if categories.contains(where: \.isReference) { + return + } + + let specifiers = categories.compactMap(\.bindingSpecifier) + guard specifiers.count > 1, let first = specifiers.first else { + return + } + + if specifiers.allSatisfy({ $0.tokenKind == first.tokenKind }) { + violations.append(contentsOf: specifiers.map(\.positionAfterSkippingLeadingTrivia)) } } } -private enum ArgumentType { +private enum GroupCategory { case binding(specifier: TokenSyntax) case reference - case constant + case neutral var isReference: Bool { switch self { - case .reference: true - default: false + case .reference: + return true + default: + return false + } + } + + var bindingSpecifier: TokenSyntax? { + switch self { + case let .binding(specifier): + return specifier + default: + return nil } } } -private extension ExprSyntax { - var categorized: ArgumentType { - if let binding = `as`(PatternExprSyntax.self)?.pattern.as(ValueBindingPatternSyntax.self) { +private enum GroupCategoryResolver { + static func category(for expression: ExprSyntax) -> GroupCategory { + if let binding = expression.as(PatternExprSyntax.self)?.pattern.as(ValueBindingPatternSyntax.self) { return .binding(specifier: binding.bindingSpecifier) } - if `is`(DeclReferenceExprSyntax.self) { + + // `case (foo, let x)` must not become `case let (foo, x)`, + // because `foo` would stop matching an existing value and + // would instead become a newly introduced binding. + if expression.is(DeclReferenceExprSyntax.self) { return .reference } - return .constant + + if let childExpressions = expression.immediatePatternGroupChildren { + return liftedCategory(for: childExpressions) + } + + return .neutral + } + + private static func liftedCategory(for expressions: [ExprSyntax]) -> GroupCategory { + let categories = expressions.map(category(for:)) + + if categories.contains(where: \.isReference) { + return .neutral + } + + let specifiers = categories.compactMap(\.bindingSpecifier) + guard let first = specifiers.first else { + return .neutral + } + + if specifiers.allSatisfy({ $0.tokenKind == first.tokenKind }) { + return .binding(specifier: first) + } + + return .neutral + } +} + +private extension ExprSyntax { + var immediatePatternGroupChildren: [ExprSyntax]? { + if let tuple = `as`(TupleExprSyntax.self) { + return tuple.elements.map(\.expression) + } + + if let call = `as`(FunctionCallExprSyntax.self), + call.calledExpression.is(MemberAccessExprSyntax.self) { + return call.arguments.map(\.expression) + } + + return nil } } diff --git a/Tests/BuiltInRulesTests/PatternMatchingKeywordsRuleTests.swift b/Tests/BuiltInRulesTests/PatternMatchingKeywordsRuleTests.swift new file mode 100644 index 0000000000..218926334d --- /dev/null +++ b/Tests/BuiltInRulesTests/PatternMatchingKeywordsRuleTests.swift @@ -0,0 +1,131 @@ +@testable import SwiftLintBuiltInRules +import TestHelpers + +final class PatternMatchingKeywordsRuleTests: SwiftLintTestCase { + func testRegressionExamples() { + let triggering = [ + "switch foo { case (.yamlParsing(↓let x), .yamlParsing(↓let y)): break }", + "switch foo { case (.yamlParsing(↓var x), (.yamlParsing(↓var y), _)): break }", + """ + do {} catch Foo.outer(.inner(↓let x), .inner(↓let y)) {} + """, + ] + + let nonTriggering = [ + "switch foo { case (.yamlParsing(var x), (.yamlParsing(var y), z)): break }", + "switch foo { case (foo, let x): break }", + "if case (foo, let x) = value {}", + ] + + verifyRule( + PatternMatchingKeywordsRule.description.with( + nonTriggeringExamples: nonTriggering.map { Example($0) }, + triggeringExamples: triggering.map { Example($0) } + ) + ) + } + + func testLabeledAssociatedValues() { + let triggering = [ + "switch foo { case .foo(bar: ↓let lhs, baz: ↓let rhs): break }", + "switch foo { case .foo(bar: ↓var lhs, baz: ↓var rhs): break }", + "do {} catch .foo(bar: ↓let x, baz: ↓let y) {}", + ] + + let nonTriggering = [ + "switch foo { case let .foo(bar: lhs, baz: rhs): break }", + "switch foo { case var .foo(bar: lhs, baz: rhs): break }", + "switch foo { case .foo(bar: existingValue, baz: let x): break }", + "switch foo { case .foo(bar: let x, baz: existingValue): break }", + "do {} catch let .foo(bar: x, baz: y) {}", + ] + + verifyRule( + PatternMatchingKeywordsRule.description.with( + nonTriggeringExamples: nonTriggering.map { Example($0) }, + triggeringExamples: triggering.map { Example($0) } + ) + ) + } + + func testSingleBindingDoesNotTrigger() { + let examples = [ + "switch foo { case (let x, y): break }", + "switch foo { case .foo(let x, y): break }", + "switch foo { case (.foo(let x), y): break }", + ] + + verifyRule( + PatternMatchingKeywordsRule.description.with( + nonTriggeringExamples: examples.map { Example($0) } + ) + ) + } + + func testNeutralElementsDoNotBlockLift() { + let examples = [ + "switch foo { case (↓let x, ↓let y, _): break }", + "switch foo { case (↓let x, ↓let y, 1): break }", + "switch foo { case (↓let x, ↓let y, .foo): break }", + "switch foo { case (↓let x, ↓let y, s.t): break }", + "switch foo { case .foo(↓let x, ↓let y, _): break }", + "switch foo { case .foo(.bar(↓let x), .bar(↓let y), .baz): break }", + ] + + verifyRule( + PatternMatchingKeywordsRule.description.with( + triggeringExamples: examples.map { Example($0) } + ) + ) + } + + func testThreeAlternativeMultiPatternCase() { + let triggering = [ + "switch foo { case .foo(↓let x, ↓let y), .bar(↓let x, ↓let y), .baz(↓let x, ↓let y): break }", + ] + + let nonTriggering = [ + "switch foo { case let .foo(x, y), let .bar(x, y), let .baz(x, y): break }", + ] + + verifyRule( + PatternMatchingKeywordsRule.description.with( + nonTriggeringExamples: nonTriggering.map { Example($0) }, + triggeringExamples: triggering.map { Example($0) } + ) + ) + } + + func testForCaseWithEnumAssociatedValues() { + let triggering = [ + "for case .foo(↓let x, ↓let y) in values {}", + ] + + let nonTriggering = [ + "for case let .foo(x, y) in values {}", + "for case .foo(existingValue, let x) in values {}", + ] + + verifyRule( + PatternMatchingKeywordsRule.description.with( + nonTriggeringExamples: nonTriggering.map { Example($0) }, + triggeringExamples: triggering.map { Example($0) } + ) + ) + } + + func testSanityChecksForNonInterestingPatterns() { + let examples = [ + "switch foo { case _: break }", + "switch foo { case .foo: break }", + "switch foo { case 42: break }", + "switch foo { case existingValue: break }", + ] + + verifyRule( + PatternMatchingKeywordsRule.description.with( + nonTriggeringExamples: examples.map { Example($0) } + ) + ) + } +} From 779e7c4677861e234cfeb015fc9cb9c29dd6d827 Mon Sep 17 00:00:00 2001 From: Rodion Ivashkov Date: Tue, 10 Mar 2026 02:10:23 +0300 Subject: [PATCH 2/3] Fix code --- .../Configuration/Configuration+Parsing.swift | 4 ++-- .../ConfigurationTests+MultipleConfigs.swift | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Source/SwiftLintFramework/Configuration/Configuration+Parsing.swift b/Source/SwiftLintFramework/Configuration/Configuration+Parsing.swift index bf327accdb..e02ddac311 100644 --- a/Source/SwiftLintFramework/Configuration/Configuration+Parsing.swift +++ b/Source/SwiftLintFramework/Configuration/Configuration+Parsing.swift @@ -211,8 +211,8 @@ extension Configuration { if case .onlyConfiguration(let onlyRules) = parentConfiguration?.rulesMode { enabledInParentRules = onlyRules - } else if case .defaultConfiguration( - let parentDisabledRules, let parentOptInRules + } else if case let .defaultConfiguration( + parentDisabledRules, parentOptInRules ) = parentConfiguration?.rulesMode { enabledInParentRules = parentOptInRules disabledInParentRules = parentDisabledRules diff --git a/Tests/FileSystemAccessTests/ConfigurationTests+MultipleConfigs.swift b/Tests/FileSystemAccessTests/ConfigurationTests+MultipleConfigs.swift index 937647187b..603485a550 100644 --- a/Tests/FileSystemAccessTests/ConfigurationTests+MultipleConfigs.swift +++ b/Tests/FileSystemAccessTests/ConfigurationTests+MultipleConfigs.swift @@ -488,7 +488,7 @@ extension ConfigurationTests { configuration: Configuration, ruleType: any Rule.Type ) { - guard case .defaultConfiguration(let disabledRules, let optInRules) = configuration.rulesMode else { + guard case let .defaultConfiguration(disabledRules, optInRules) = configuration.rulesMode else { XCTFail("Configuration rulesMode was not the default") return } From 1dc7fedf89496ba3001cbf177f90ebe384d3fd41 Mon Sep 17 00:00:00 2001 From: Rodion Ivashkov Date: Tue, 10 Mar 2026 01:48:55 +0300 Subject: [PATCH 3/3] Update CHANGELOG.md --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 52cafe5b3e..9910520e0d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,9 @@ `optional_data_string_conversion` rule. [nadeemnali](https://github.com/nadeemnali) [#6359](https://github.com/realm/SwiftLint/issues/6359) +* Improve the opt-in `pattern_matching_keywords` rule by extending support + beyond `switch case` and refining nested pattern handling. + [GandaLF2006](https://github.com/GandaLF2006) ### Bug Fixes