From 588f3a8f1cfb9faaa601dfa4beea1173206088a6 Mon Sep 17 00:00:00 2001 From: JP Simard Date: Fri, 20 Jun 2025 10:45:18 -0400 Subject: [PATCH 1/5] Migrate StatementPositionRule from SourceKit to SwiftSyntax Convert StatementPositionRule to use SwiftSyntax instead of SourceKit for improved performance and more accurate positioning validation. The SwiftSyntax implementation: - Uses ViolationsSyntaxVisitor and ViolationsSyntaxRewriter patterns - Validates else/catch keyword positioning with proper indentation checks - Supports both default (same line) and uncuddled (next line) modes - Correctly handles trailing/leading trivia for whitespace validation - Implements corrections using explicit rewriter for both statement types - Extracts common validation logic to reduce code duplication - Skip correcting when there are comments --- CHANGELOG.md | 1 + .../Rules/Style/StatementPositionRule.swift | 346 ++++++++++-------- .../Style/StatementPositionRuleExamples.swift | 322 ++++++++++++++++ .../StatementPositionRuleTests.swift | 188 +++++++++- 4 files changed, 696 insertions(+), 161 deletions(-) create mode 100644 Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRuleExamples.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index cc3c23998f..9713a92a2f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,7 @@ * `file_header` * `file_length` * `line_length` + * `statement_position` * `trailing_whitespace` * `vertical_whitespace` diff --git a/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRule.swift b/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRule.swift index 342037995b..5d4f78eb39 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRule.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRule.swift @@ -1,7 +1,7 @@ -import Foundation -import SourceKittenFramework +import SwiftSyntax -struct StatementPositionRule: CorrectableRule { +@SwiftSyntaxRule(explicitRewriter: true) +struct StatementPositionRule: Rule { var configuration = StatementPositionConfiguration() static let description = RuleDescription( @@ -9,25 +9,9 @@ struct StatementPositionRule: CorrectableRule { name: "Statement Position", description: "Else and catch should be on the same line, one space after the previous declaration", kind: .style, - nonTriggeringExamples: [ - Example("} else if {"), - Example("} else {"), - Example("} catch {"), - Example("\"}else{\""), - Example("struct A { let catchphrase: Int }\nlet a = A(\n catchphrase: 0\n)"), - Example("struct A { let `catch`: Int }\nlet a = A(\n `catch`: 0\n)"), - ], - triggeringExamples: [ - Example("↓}else if {"), - Example("↓} else {"), - Example("↓}\ncatch {"), - Example("↓}\n\t catch {"), - ], - corrections: [ - Example("↓}\n else {"): Example("} else {"), - Example("↓}\n else if {"): Example("} else if {"), - Example("↓}\n catch {"): Example("} catch {"), - ] + nonTriggeringExamples: StatementPositionRuleExamples.nonTriggeringExamples, + triggeringExamples: StatementPositionRuleExamples.triggeringExamples, + corrections: StatementPositionRuleExamples.corrections ) static let uncuddledDescription = RuleDescription( @@ -36,175 +20,217 @@ struct StatementPositionRule: CorrectableRule { description: "Else and catch should be on the next line, with equal indentation to the " + "previous declaration", kind: .style, - nonTriggeringExamples: [ - Example(" }\n else if {"), - Example(" }\n else {"), - Example(" }\n catch {"), - Example(" }\n\n catch {"), - Example("\n\n }\n catch {"), - Example("\"}\nelse{\""), - Example("struct A { let catchphrase: Int }\nlet a = A(\n catchphrase: 0\n)"), - Example("struct A { let `catch`: Int }\nlet a = A(\n `catch`: 0\n)"), - ], - triggeringExamples: [ - Example("↓ }else if {"), - Example("↓}\n else {"), - Example("↓ }\ncatch {"), - Example("↓}\n\t catch {"), - ], - corrections: [ - Example(" }else if {"): Example(" }\n else if {"), - Example("}\n else {"): Example("}\nelse {"), - Example(" }\ncatch {"): Example(" }\n catch {"), - Example("}\n\t catch {"): Example("}\ncatch {"), - ] + nonTriggeringExamples: StatementPositionRuleExamples.uncuddledNonTriggeringExamples, + triggeringExamples: StatementPositionRuleExamples.uncuddledTriggeringExamples, + corrections: StatementPositionRuleExamples.uncuddledCorrections ) +} - func validate(file: SwiftLintFile) -> [StyleViolation] { - switch configuration.statementMode { - case .default: - return defaultValidate(file: file) - case .uncuddledElse: - return uncuddledValidate(file: file) +// MARK: - Shared Validation Logic + +private struct StatementValidation { + let hasLeadingNewline: Bool + let hasTrailingContent: Bool + let expectedIndentation: Int + let actualIndentation: Int + let isSingleSpace: Bool + let hasCommentsBetween: Bool + + init(keyword: TokenSyntax, previousToken: TokenSyntax) { + self.hasLeadingNewline = keyword.leadingTrivia.contains { piece in + switch piece { + case .newlines, .carriageReturns, .carriageReturnLineFeeds: + return true + default: + return false + } } + self.hasTrailingContent = !previousToken.trailingTrivia.isEmpty + self.expectedIndentation = Self.calculateIndentation(previousToken.leadingTrivia) + self.actualIndentation = Self.calculateIndentation(keyword.leadingTrivia) + self.isSingleSpace = previousToken.trailingTrivia.isSingleSpace + + // Check for comments between closing brace and keyword + self.hasCommentsBetween = previousToken.trailingTrivia.contains(where: \.isComment) || + keyword.leadingTrivia.contains(where: \.isComment) } - func correct(file: SwiftLintFile) -> Int { - switch configuration.statementMode { - case .default: - defaultCorrect(file: file) - case .uncuddledElse: - uncuddledCorrect(file: file) + private static func calculateIndentation(_ trivia: Trivia) -> Int { + var indentation = 0 + for piece in trivia.reversed() { + switch piece { + case .spaces(let count): + indentation += count + case .tabs(let count): + indentation += count * 4 // Assuming 1 tab = 4 spaces + case .newlines, .carriageReturns, .carriageReturnLineFeeds: + break + default: + continue + } } + return indentation } -} -// Default Behaviors -private extension StatementPositionRule { - // match literal '}' - // followed by 1) nothing, 2) two+ whitespace/newlines or 3) newlines or tabs - // followed by 'else' or 'catch' literals - static let defaultPattern = "\\}(?:[\\s\\n\\r]{2,}|[\\n\\t\\r]+)?\\b(else|catch)\\b" - - func defaultValidate(file: SwiftLintFile) -> [StyleViolation] { - defaultViolationRanges(in: file, matching: Self.defaultPattern).compactMap { range in - StyleViolation(ruleDescription: Self.description, - severity: configuration.severity, - location: Location(file: file, characterOffset: range.location)) - } + func isValidForDefaultMode() -> Bool { + !hasLeadingNewline && isSingleSpace } - func defaultViolationRanges(in file: SwiftLintFile, matching pattern: String) -> [NSRange] { - file.match(pattern: pattern).filter { _, syntaxKinds in - syntaxKinds.starts(with: [.keyword]) - }.compactMap { $0.0 } + func isValidForUncuddledMode() -> Bool { + hasLeadingNewline && !hasTrailingContent && actualIndentation == expectedIndentation } +} + +// MARK: - Visitor + +private extension StatementPositionRule { + final class Visitor: ViolationsSyntaxVisitor { + override func visitPost(_ node: IfExprSyntax) { + guard let elseKeyword = node.elseKeyword else { return } + validateStatement(keyword: elseKeyword) + } - func defaultCorrect(file: SwiftLintFile) -> Int { - let violations = defaultViolationRanges(in: file, matching: Self.defaultPattern) - let matches = file.ruleEnabled(violatingRanges: violations, for: self) - if matches.isEmpty { - return 0 + override func visitPost(_ node: DoStmtSyntax) { + for catchClause in node.catchClauses { + validateStatement(keyword: catchClause.catchKeyword) + } } - let regularExpression = regex(Self.defaultPattern) - var contents = file.contents - for range in matches.reversed() { - contents = regularExpression.stringByReplacingMatches(in: contents, options: [], range: range, - withTemplate: "} $1") + + private func validateStatement(keyword: TokenSyntax) { + guard let previousToken = keyword.previousToken(viewMode: .sourceAccurate), + previousToken.tokenKind == .rightBrace else { return } + + let validation = StatementValidation(keyword: keyword, previousToken: previousToken) + let isValid = configuration.statementMode == .default ? + validation.isValidForDefaultMode() : + validation.isValidForUncuddledMode() + + if !isValid { + let description = configuration.statementMode == .default ? + StatementPositionRule.description : + StatementPositionRule.uncuddledDescription + + violations.append( + ReasonedRuleViolation( + position: previousToken.positionAfterSkippingLeadingTrivia, + reason: description.description, + severity: configuration.severity + ) + ) + } } - file.write(contents) - return matches.count } } -// Uncuddled Behaviors +// MARK: - Rewriter + private extension StatementPositionRule { - func uncuddledValidate(file: SwiftLintFile) -> [StyleViolation] { - uncuddledViolationRanges(in: file).compactMap { range in - StyleViolation(ruleDescription: Self.uncuddledDescription, - severity: configuration.severity, - location: Location(file: file, characterOffset: range.location)) - } - } + final class Rewriter: ViolationsSyntaxRewriter { + override func visit(_ node: IfExprSyntax) -> ExprSyntax { + guard let elseKeyword = node.elseKeyword else { + return super.visit(node) + } - // match literal '}' - // preceded by whitespace (or nothing) - // followed by 1) nothing, 2) two+ whitespace/newlines or 3) newlines or tabs - // followed by newline and the same amount of whitespace then 'else' or 'catch' literals - static let uncuddledPattern = "([ \t]*)\\}(\\n+)?([ \t]*)\\b(else|catch)\\b" + if let corrected = correctIfStatement(node: node, elseKeyword: elseKeyword) { + return super.visit(corrected) + } - static let uncuddledRegex = regex(uncuddledPattern, options: []) + return super.visit(node) + } - static func uncuddledMatchValidator(contents: StringView) -> ((NSTextCheckingResult) - -> NSTextCheckingResult?) { - { match in - if match.numberOfRanges != 5 { - return match - } - if match.range(at: 2).length == 0 { - return match + override func visit(_ node: DoStmtSyntax) -> StmtSyntax { + var newNode = node + var newCatchClauses: [CatchClauseSyntax] = [] + var needsBodyUpdate = false + + for catchClause in node.catchClauses { + if let corrected = correctCatchStatement( + catchClause: catchClause, + keyword: catchClause.catchKeyword + ) { + newCatchClauses.append(corrected) + needsBodyUpdate = true + } else { + newCatchClauses.append(catchClause) } - let range1 = match.range(at: 1) - let range2 = match.range(at: 3) - let whitespace1 = contents.string.substring(from: range1.location, length: range1.length) - let whitespace2 = contents.string.substring(from: range2.location, length: range2.length) - if whitespace1 == whitespace2 { - return nil - } - return match } - } - static func uncuddledMatchFilter(contents: StringView, - syntaxMap: SwiftLintSyntaxMap) -> ((NSTextCheckingResult) -> Bool) { - { match in - let range = match.range - guard let matchRange = contents.NSRangeToByteRange(start: range.location, - length: range.length) else { - return false + // Update the body's closing brace if needed + if needsBodyUpdate { + var newBody = newNode.body + if configuration.statementMode == .default { + newBody.rightBrace = newBody.rightBrace.with(\.trailingTrivia, .space) + } else { + // Uncuddled mode - remove trailing trivia + newBody.rightBrace = newBody.rightBrace.with(\.trailingTrivia, []) + } + newNode.body = newBody } - return syntaxMap.kinds(inByteRange: matchRange) == [.keyword] + + newNode.catchClauses = CatchClauseListSyntax(newCatchClauses) + return super.visit(newNode) } - } - func uncuddledViolationRanges(in file: SwiftLintFile) -> [NSRange] { - let contents = file.stringView - let syntaxMap = file.syntaxMap - let matches = Self.uncuddledRegex.matches(in: file) - let validator = Self.uncuddledMatchValidator(contents: contents) - let filterMatches = Self.uncuddledMatchFilter(contents: contents, syntaxMap: syntaxMap) + private func correctIfStatement(node: IfExprSyntax, elseKeyword: TokenSyntax) -> IfExprSyntax? { + guard let previousToken = elseKeyword.previousToken(viewMode: .sourceAccurate), + previousToken.tokenKind == .rightBrace else { return nil } - return matches.compactMap(validator).filter(filterMatches).map(\.range) - } + let validation = StatementValidation(keyword: elseKeyword, previousToken: previousToken) + let needsCorrection = configuration.statementMode == .default ? + !validation.isValidForDefaultMode() : + !validation.isValidForUncuddledMode() - func uncuddledCorrect(file: SwiftLintFile) -> Int { - var contents = file.contents - let syntaxMap = file.syntaxMap - let matches = Self.uncuddledRegex.matches(in: file) - let validator = Self.uncuddledMatchValidator(contents: file.stringView) - let filterRanges = Self.uncuddledMatchFilter(contents: file.stringView, syntaxMap: syntaxMap) - let validMatches = matches.compactMap(validator).filter(filterRanges) - .filter { file.ruleEnabled(violatingRanges: [$0.range], for: self).isNotEmpty } - if validMatches.isEmpty { - return 0 - } - for match in validMatches.reversed() { - let range1 = match.range(at: 1) - let range2 = match.range(at: 3) - let newlineRange = match.range(at: 2) - var whitespace = contents.bridge().substring(with: range1) - let newLines: String - if newlineRange.location != NSNotFound { - newLines = contents.bridge().substring(with: newlineRange) - } else { - newLines = "" + guard needsCorrection else { return nil } + + // Skip correction if there are comments between brace and keyword + guard !validation.hasCommentsBetween else { return nil } + + numberOfCorrections += 1 + + if configuration.statementMode == .default { + // Update the right brace trailing trivia + var newBody = node.body + newBody.rightBrace = newBody.rightBrace.with(\.trailingTrivia, .space) + let newNode = node.with(\.body, newBody) + + // Update the else keyword leading trivia + return newNode.with(\.elseKeyword, elseKeyword.with(\.leadingTrivia, [])) } - if !whitespace.hasPrefix("\n") && newLines != "\n" { - whitespace.insert("\n", at: whitespace.startIndex) + // Uncuddled mode + let newTrivia = Trivia.newline + .spaces(validation.expectedIndentation) + + // Update the right brace trailing trivia + var newBody = node.body + newBody.rightBrace = newBody.rightBrace.with(\.trailingTrivia, []) + let newNode = node.with(\.body, newBody) + + // Update the else keyword leading trivia + return newNode.with(\.elseKeyword, elseKeyword.with(\.leadingTrivia, newTrivia)) + } + + private func correctCatchStatement(catchClause: CatchClauseSyntax, keyword: TokenSyntax) -> CatchClauseSyntax? { + guard let previousToken = keyword.previousToken(viewMode: .sourceAccurate), + previousToken.tokenKind == .rightBrace else { return nil } + + let validation = StatementValidation(keyword: keyword, previousToken: previousToken) + let needsCorrection = configuration.statementMode == .default ? + !validation.isValidForDefaultMode() : + !validation.isValidForUncuddledMode() + + guard needsCorrection else { return nil } + + // Skip correction if there are comments between brace and keyword + guard !validation.hasCommentsBetween else { return nil } + + numberOfCorrections += 1 + + if configuration.statementMode == .default { + // For default mode, just update the keyword + return catchClause.with(\.catchKeyword, keyword.with(\.leadingTrivia, [])) } - contents = contents.bridge().replacingCharacters(in: range2, with: whitespace) + // Uncuddled mode + let newTrivia = Trivia.newline + .spaces(validation.expectedIndentation) + return catchClause.with(\.catchKeyword, keyword.with(\.leadingTrivia, newTrivia)) } - file.write(contents) - return validMatches.count } } diff --git a/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRuleExamples.swift b/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRuleExamples.swift new file mode 100644 index 0000000000..8c182649f1 --- /dev/null +++ b/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRuleExamples.swift @@ -0,0 +1,322 @@ +struct StatementPositionRuleExamples { + static let nonTriggeringExamples = [ + // Single line examples + Example("if true { } else if false { }"), + Example("if true { } else { }"), + Example("do { } catch { }"), + Example("do { let a = 1 } catch let error { }"), + Example("\"}else{\""), + Example(""" + struct A { let catchphrase: Int } + let a = A( + catchphrase: 0 + ) + """), + Example(""" + struct A { let `catch`: Int } + let a = A( + `catch`: 0 + ) + """), + // Multi-line examples + Example(""" + if true { + foo() + } else { + bar() + } + """), + Example(""" + if true { + foo + } else if true { + bar() + } else { + return + } + """), + Example(""" + do { + foo() + } catch { + bar() + } + """), + Example(""" + do { + foo() + } catch let error { + bar() + } catch { + return + } + """), + Example(""" + struct A { let catchphrase: Int } + let a = A( + catchphrase: 0 + ) + """), + Example(""" + struct A { let `catch`: Int } + let a = A( + `catch`: 0 + ) + """), + ] + + static let triggeringExamples = [ + // Single line examples + Example("if true { ↓}else if false { }"), + Example("if true { ↓} else { }"), + Example(""" + do { ↓} + catch { } + """), + Example(""" + do { + let a = 1 + ↓} + \t catch { } + """), + // Multi-line examples + Example(""" + if true { + foo() + ↓}else { + bar() + } + """), + Example(""" + if true { + foo() + ↓} else if true { + bar() + } + """), + Example(""" + if true { + foo() + ↓} + else { + bar() + } + """), + Example(""" + do { + foo() + ↓}catch { + bar() + } + """), + Example(""" + do { + foo() + ↓} catch { + bar() + } + """), + Example(""" + do { + foo() + ↓} + catch { + bar() + } + """), + ] + + static let corrections = [ + // Single line examples + Example(""" + if true { ↓} + else { } + """): Example("if true { } else { }"), + Example(""" + if true { ↓} + else if false { } + """): Example("if true { } else if false { }"), + Example(""" + do { ↓} + catch { } + """): Example("do { } catch { }"), + // Multi-line examples + Example(""" + if true { + foo() + ↓} + else { + bar() + } + """): Example(""" + if true { + foo() + } else { + bar() + } + """), + Example(""" + if true { + foo() + ↓} else if true { + bar() + } + """): Example(""" + if true { + foo() + } else if true { + bar() + } + """), + Example(""" + do { + foo() + ↓} + catch { + bar() + } + """): Example(""" + do { + foo() + } catch { + bar() + } + """), + Example(""" + do { + foo() + ↓} catch { + bar() + } + """): Example(""" + do { + foo() + } catch { + bar() + } + """), + ] + + // MARK: - Uncuddled Examples + + static let uncuddledNonTriggeringExamples = [ + Example(""" + if true { + } + else if false { + } + """), + Example(""" + if condition { + } + else { + } + """), + Example(""" + do { + } + catch { + } + """), + Example(""" + do { + } + + catch { + } + """), + Example(""" + do { + + + } + catch { + } + """), + Example("\"}\nelse{\""), + Example(""" + struct A { let catchphrase: Int } + let a = A( + catchphrase: 0 + ) + """), + Example(""" + struct A { let `catch`: Int } + let a = A( + `catch`: 0 + ) + """), + ] + + static let uncuddledTriggeringExamples = [ + Example(""" + if true { + ↓}else if false { + } + """), + Example(""" + if condition { + ↓} + else { + } + """), + Example(""" + do { + ↓} + catch { + } + """), + Example(""" + do { + ↓} + \t catch { + } + """), + ] + + static let uncuddledCorrections = [ + Example(""" + if true { + }else if false { + } + """): Example(""" + if true { + } + else if false { + } + """), + Example(""" + if condition { + } + else { + } + """): Example(""" + if condition { + } + else { + } + """), + Example(""" + do { + } + catch { + } + """): Example(""" + do { + } + catch { + } + """), + Example(""" + do { + } + \t catch { + } + """): Example(""" + do { + } + catch { + } + """), + ] +} diff --git a/Tests/BuiltInRulesTests/StatementPositionRuleTests.swift b/Tests/BuiltInRulesTests/StatementPositionRuleTests.swift index ab4b81f353..565b6fd57a 100644 --- a/Tests/BuiltInRulesTests/StatementPositionRuleTests.swift +++ b/Tests/BuiltInRulesTests/StatementPositionRuleTests.swift @@ -2,8 +2,194 @@ import TestHelpers final class StatementPositionRuleTests: SwiftLintTestCase { + let nonTriggeringExamples = [ + Example(""" + if true { + foo() + } + else { + bar() + } + """), + Example(""" + if true { + foo() + } + else if true { + bar() + } + else { + return + } + """), + Example(""" + if true { foo() } + else { bar() } + """), + Example(""" + if true { foo() } + else if true { bar() } + else { return } + """), + Example(""" + do { + foo() + } + catch { + bar() + } + """), + Example(""" + do { + foo() + } + catch { + bar() + } + catch { + return + } + """), + Example(""" + do { foo() } + catch { bar() } + """), + Example(""" + do { foo() } + catch { bar() } + catch { return } + """), + ] + + let triggeringExamples = [ + Example(""" + if true { + foo() + ↓} else { + bar() + } + """), + Example(""" + if true { + foo() + ↓} else if true { + bar() + ↓} else { + return + } + """), + Example(""" + if true { + foo() + ↓} + else { + bar() + } + """), + Example(""" + do { + foo() + ↓} catch { + bar() + } + """), + Example(""" + do { + foo() + ↓} catch let error { + bar() + ↓} catch { + return + } + """), + Example(""" + do { + foo() + ↓} + catch { + bar() + } + """), + ] + + let corrections = [ + Example(""" + if true { + foo() + ↓} + else { + bar() + } + """): + Example(""" + if true { + foo() + } + else { + bar() + } + """), + Example(""" + if true { + foo() + ↓} else if true { + bar() + ↓} else { + bar() + } + """): + Example(""" + if true { + foo() + } + else if true { + bar() + } + else { + bar() + } + """), + Example(""" + do { + foo() + ↓} catch { + bar() + } + """): + Example(""" + do { + foo() + } + catch { + bar() + } + """), + Example(""" + do { + foo() + ↓} + catch { + bar() + } + """): + Example(""" + do { + foo() + } + catch { + bar() + } + """), + ] + func testStatementPositionUncuddled() { let configuration = ["statement_mode": "uncuddled_else"] - verifyRule(StatementPositionRule.uncuddledDescription, ruleConfiguration: configuration) + + let description = StatementPositionRule.uncuddledDescription + .with(nonTriggeringExamples: nonTriggeringExamples) + .with(triggeringExamples: triggeringExamples) + .with(corrections: corrections) + + verifyRule(description, ruleConfiguration: configuration) } } From 85241282af2dc1b3e9c092c8126ce4cac742c553 Mon Sep 17 00:00:00 2001 From: JP Simard Date: Tue, 24 Jun 2025 20:45:56 -0400 Subject: [PATCH 2/5] Address code review feedback --- .../Rules/Style/StatementPositionRule.swift | 112 ++++++++++-------- .../Style/StatementPositionRuleExamples.swift | 18 +++ 2 files changed, 79 insertions(+), 51 deletions(-) diff --git a/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRule.swift b/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRule.swift index 5d4f78eb39..4c55695a24 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRule.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRule.swift @@ -57,6 +57,8 @@ private struct StatementValidation { private static func calculateIndentation(_ trivia: Trivia) -> Int { var indentation = 0 + // Traverse the trivia in reverse because we need to calculate the indentation + // of the *last* line in the trivia, not the first. for piece in trivia.reversed() { switch piece { case .spaces(let count): @@ -81,6 +83,25 @@ private struct StatementValidation { } } +// MARK: - Shared Helpers + +private extension StatementPositionRule { + static func validateAndPrepareCorrection( + keyword: TokenSyntax, + configuration: StatementPositionConfiguration + ) -> (previousToken: TokenSyntax, validation: StatementValidation, needsCorrection: Bool)? { + guard let previousToken = keyword.previousToken(viewMode: .sourceAccurate), + previousToken.tokenKind == .rightBrace else { return nil } + + let validation = StatementValidation(keyword: keyword, previousToken: previousToken) + let needsCorrection = configuration.statementMode == .default ? + !validation.isValidForDefaultMode() : + !validation.isValidForUncuddledMode() + + return (previousToken, validation, needsCorrection) + } +} + // MARK: - Visitor private extension StatementPositionRule { @@ -97,27 +118,22 @@ private extension StatementPositionRule { } private func validateStatement(keyword: TokenSyntax) { - guard let previousToken = keyword.previousToken(viewMode: .sourceAccurate), - previousToken.tokenKind == .rightBrace else { return } - - let validation = StatementValidation(keyword: keyword, previousToken: previousToken) - let isValid = configuration.statementMode == .default ? - validation.isValidForDefaultMode() : - validation.isValidForUncuddledMode() - - if !isValid { - let description = configuration.statementMode == .default ? - StatementPositionRule.description : - StatementPositionRule.uncuddledDescription - - violations.append( - ReasonedRuleViolation( - position: previousToken.positionAfterSkippingLeadingTrivia, - reason: description.description, - severity: configuration.severity - ) + guard let result = StatementPositionRule.validateAndPrepareCorrection( + keyword: keyword, + configuration: configuration + ), result.needsCorrection else { return } + + let description = configuration.statementMode == .default ? + StatementPositionRule.description : + StatementPositionRule.uncuddledDescription + + violations.append( + ReasonedRuleViolation( + position: result.previousToken.positionAfterSkippingLeadingTrivia, + reason: description.description, + severity: configuration.severity ) - } + ) } } } @@ -141,30 +157,30 @@ private extension StatementPositionRule { override func visit(_ node: DoStmtSyntax) -> StmtSyntax { var newNode = node var newCatchClauses: [CatchClauseSyntax] = [] - var needsBodyUpdate = false + var bodyUpdated = false - for catchClause in node.catchClauses { + for (index, catchClause) in node.catchClauses.enumerated() { if let corrected = correctCatchStatement( catchClause: catchClause, keyword: catchClause.catchKeyword ) { newCatchClauses.append(corrected) - needsBodyUpdate = true - } else { - newCatchClauses.append(catchClause) - } - } - // Update the body's closing brace if needed - if needsBodyUpdate { - var newBody = newNode.body - if configuration.statementMode == .default { - newBody.rightBrace = newBody.rightBrace.with(\.trailingTrivia, .space) + // Update the body's closing brace only for the first catch that needs correction + if !bodyUpdated && index == 0 { + var newBody = newNode.body + if configuration.statementMode == .default { + newBody.rightBrace = newBody.rightBrace.with(\.trailingTrivia, .space) + } else { + // Uncuddled mode - remove trailing trivia + newBody.rightBrace = newBody.rightBrace.with(\.trailingTrivia, []) + } + newNode.body = newBody + bodyUpdated = true + } } else { - // Uncuddled mode - remove trailing trivia - newBody.rightBrace = newBody.rightBrace.with(\.trailingTrivia, []) + newCatchClauses.append(catchClause) } - newNode.body = newBody } newNode.catchClauses = CatchClauseListSyntax(newCatchClauses) @@ -172,15 +188,12 @@ private extension StatementPositionRule { } private func correctIfStatement(node: IfExprSyntax, elseKeyword: TokenSyntax) -> IfExprSyntax? { - guard let previousToken = elseKeyword.previousToken(viewMode: .sourceAccurate), - previousToken.tokenKind == .rightBrace else { return nil } - - let validation = StatementValidation(keyword: elseKeyword, previousToken: previousToken) - let needsCorrection = configuration.statementMode == .default ? - !validation.isValidForDefaultMode() : - !validation.isValidForUncuddledMode() + guard let result = StatementPositionRule.validateAndPrepareCorrection( + keyword: elseKeyword, + configuration: configuration + ), result.needsCorrection else { return nil } - guard needsCorrection else { return nil } + let validation = result.validation // Skip correction if there are comments between brace and keyword guard !validation.hasCommentsBetween else { return nil } @@ -209,15 +222,12 @@ private extension StatementPositionRule { } private func correctCatchStatement(catchClause: CatchClauseSyntax, keyword: TokenSyntax) -> CatchClauseSyntax? { - guard let previousToken = keyword.previousToken(viewMode: .sourceAccurate), - previousToken.tokenKind == .rightBrace else { return nil } - - let validation = StatementValidation(keyword: keyword, previousToken: previousToken) - let needsCorrection = configuration.statementMode == .default ? - !validation.isValidForDefaultMode() : - !validation.isValidForUncuddledMode() + guard let result = StatementPositionRule.validateAndPrepareCorrection( + keyword: keyword, + configuration: configuration + ), result.needsCorrection else { return nil } - guard needsCorrection else { return nil } + let validation = result.validation // Skip correction if there are comments between brace and keyword guard !validation.hasCommentsBetween else { return nil } diff --git a/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRuleExamples.swift b/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRuleExamples.swift index 8c182649f1..2e2dee4445 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRuleExamples.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRuleExamples.swift @@ -124,6 +124,24 @@ struct StatementPositionRuleExamples { bar() } """), + // Comments don't prevent violation detection + Example(""" + if true { + foo() + ↓} // comment + else { + bar() + } + """), + Example(""" + do { + foo() + ↓} + // comment + catch { + bar() + } + """), ] static let corrections = [ From 7e87636882b470aa69657459a8067782916dc501 Mon Sep 17 00:00:00 2001 From: JP Simard Date: Wed, 25 Jun 2025 08:19:37 -0400 Subject: [PATCH 3/5] Add tests that validate that there is no correction --- .../Style/StatementPositionRuleExamples.swift | 17 +++++++++++++++++ Source/SwiftLintCore/Models/Example.swift | 7 ++++++- Tests/TestHelpers/TestHelpers.swift | 8 +++++--- 3 files changed, 28 insertions(+), 4 deletions(-) diff --git a/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRuleExamples.swift b/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRuleExamples.swift index 2e2dee4445..c96bfaf594 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRuleExamples.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRuleExamples.swift @@ -213,6 +213,23 @@ struct StatementPositionRuleExamples { bar() } """), + Example(""" + do { + foo() + ↓} + // comment + catch { + bar() + } + """): Example(""" + do { + foo() + ↓} + // comment + catch { + bar() + } + """, allowsViolationsInCorrections: true), ] // MARK: - Uncuddled Examples diff --git a/Source/SwiftLintCore/Models/Example.swift b/Source/SwiftLintCore/Models/Example.swift index a91d023198..a48d31de42 100644 --- a/Source/SwiftLintCore/Models/Example.swift +++ b/Source/SwiftLintCore/Models/Example.swift @@ -41,6 +41,8 @@ public struct Example: Sendable { /// Specifies whether the test example should be the only example run during the current test case execution. package var isFocused: Bool + /// If violations are allowed in this example when correcting. + package var allowsViolationsInCorrections: Bool } public extension Example { @@ -59,6 +61,7 @@ public extension Example { /// Defaults to the file where this initializer is called. /// - line: The line in the file where the example is located. /// Defaults to the line where this initializer is called. + /// - allowsViolationsInCorrections: If violations are allowed in this example when correcting. init(_ code: String, configuration: [String: any Sendable]? = nil, testMultiByteOffsets: Bool = true, @@ -68,7 +71,8 @@ public extension Example { testOnLinux: Bool = true, file: StaticString = #filePath, line: UInt = #line, - excludeFromDocumentation: Bool = false) { + excludeFromDocumentation: Bool = false, + allowsViolationsInCorrections: Bool = false) { self.code = code self.configuration = configuration self.testMultiByteOffsets = testMultiByteOffsets @@ -80,6 +84,7 @@ public extension Example { self.testWrappingInString = testWrappingInString self.testDisableCommand = testDisableCommand self.isFocused = false + self.allowsViolationsInCorrections = allowsViolationsInCorrections } /// Returns the same example, but with the `code` that is passed in diff --git a/Tests/TestHelpers/TestHelpers.swift b/Tests/TestHelpers/TestHelpers.swift index bc4cc1ca07..480d1a2941 100644 --- a/Tests/TestHelpers/TestHelpers.swift +++ b/Tests/TestHelpers/TestHelpers.swift @@ -203,16 +203,18 @@ private extension Configuration { let collector = Linter(file: file, configuration: self, compilerArguments: compilerArguments) let linter = collector.collect(into: storage) let corrections = linter.correct(using: storage) + let beforeCode = expected.allowsViolationsInCorrections ? before.removingViolationMarkers().code : before.code + let expectedCode = expected.allowsViolationsInCorrections ? expected.removingViolationMarkers().code : expected.code XCTAssertGreaterThanOrEqual( corrections.count, - before.code != expected.code ? 1 : 0, + beforeCode != expectedCode ? 1 : 0, #function + ".expectedLocationsEmpty", file: before.file, line: before.line ) XCTAssertEqual( file.contents, - expected.code, + expectedCode, #function + ".file contents", file: before.file, line: before.line) let path = file.path! @@ -220,7 +222,7 @@ private extension Configuration { let corrected = try String(contentsOfFile: path, encoding: .utf8) XCTAssertEqual( corrected, - expected.code, + expectedCode, #function + ".corrected file equals expected", file: before.file, line: before.line) } catch { From 30348c8d2f60361726cdec32e556239695eaa22d Mon Sep 17 00:00:00 2001 From: JP Simard Date: Wed, 25 Jun 2025 10:33:55 -0400 Subject: [PATCH 4/5] fixup! Add tests that validate that there is no correction --- Tests/TestHelpers/TestHelpers.swift | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/Tests/TestHelpers/TestHelpers.swift b/Tests/TestHelpers/TestHelpers.swift index 480d1a2941..5e614171b5 100644 --- a/Tests/TestHelpers/TestHelpers.swift +++ b/Tests/TestHelpers/TestHelpers.swift @@ -203,8 +203,15 @@ private extension Configuration { let collector = Linter(file: file, configuration: self, compilerArguments: compilerArguments) let linter = collector.collect(into: storage) let corrections = linter.correct(using: storage) - let beforeCode = expected.allowsViolationsInCorrections ? before.removingViolationMarkers().code : before.code - let expectedCode = expected.allowsViolationsInCorrections ? expected.removingViolationMarkers().code : expected.code + let beforeCode: String + let expectedCode: String + if expected.allowsViolationsInCorrections { + beforeCode = before.removingViolationMarkers().code + expectedCode = expected.removingViolationMarkers().code + } else { + beforeCode = before.code + expectedCode = expected.code + } XCTAssertGreaterThanOrEqual( corrections.count, beforeCode != expectedCode ? 1 : 0, From 8aed5da2a59d39072a4383eae3d56138f3b5c1f3 Mon Sep 17 00:00:00 2001 From: JP Simard Date: Wed, 25 Jun 2025 10:54:21 -0400 Subject: [PATCH 5/5] Move violation location to the else/catch keyword Instead of at the preceding closing brace. --- .../Rules/Style/StatementPositionRule.swift | 2 +- .../Style/StatementPositionRuleExamples.swift | 82 +++++++++---------- .../StatementPositionRuleTests.swift | 34 ++++---- 3 files changed, 59 insertions(+), 59 deletions(-) diff --git a/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRule.swift b/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRule.swift index 4c55695a24..3d14371b4e 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRule.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRule.swift @@ -129,7 +129,7 @@ private extension StatementPositionRule { violations.append( ReasonedRuleViolation( - position: result.previousToken.positionAfterSkippingLeadingTrivia, + position: keyword.positionAfterSkippingLeadingTrivia, reason: description.description, severity: configuration.severity ) diff --git a/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRuleExamples.swift b/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRuleExamples.swift index c96bfaf594..79ffe46ba1 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRuleExamples.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Style/StatementPositionRuleExamples.swift @@ -67,60 +67,60 @@ struct StatementPositionRuleExamples { static let triggeringExamples = [ // Single line examples - Example("if true { ↓}else if false { }"), - Example("if true { ↓} else { }"), + Example("if true { }↓else if false { }"), + Example("if true { } ↓else { }"), Example(""" - do { ↓} - catch { } + do { } + ↓catch { } """), Example(""" do { let a = 1 - ↓} - \t catch { } + } + \t ↓catch { } """), // Multi-line examples Example(""" if true { foo() - ↓}else { + }↓else { bar() } """), Example(""" if true { foo() - ↓} else if true { + } ↓else if true { bar() } """), Example(""" if true { foo() - ↓} - else { + } + ↓else { bar() } """), Example(""" do { foo() - ↓}catch { + }↓catch { bar() } """), Example(""" do { foo() - ↓} catch { + } ↓catch { bar() } """), Example(""" do { foo() - ↓} - catch { + } + ↓catch { bar() } """), @@ -128,17 +128,17 @@ struct StatementPositionRuleExamples { Example(""" if true { foo() - ↓} // comment - else { + } // comment + ↓else { bar() } """), Example(""" do { foo() - ↓} + } // comment - catch { + ↓catch { bar() } """), @@ -147,23 +147,23 @@ struct StatementPositionRuleExamples { static let corrections = [ // Single line examples Example(""" - if true { ↓} - else { } + if true { } + ↓else { } """): Example("if true { } else { }"), Example(""" - if true { ↓} - else if false { } + if true { } + ↓else if false { } """): Example("if true { } else if false { }"), Example(""" - do { ↓} - catch { } + do { } + ↓catch { } """): Example("do { } catch { }"), // Multi-line examples Example(""" if true { foo() - ↓} - else { + } + ↓else { bar() } """): Example(""" @@ -176,7 +176,7 @@ struct StatementPositionRuleExamples { Example(""" if true { foo() - ↓} else if true { + } ↓else if true { bar() } """): Example(""" @@ -189,8 +189,8 @@ struct StatementPositionRuleExamples { Example(""" do { foo() - ↓} - catch { + } + ↓catch { bar() } """): Example(""" @@ -203,7 +203,7 @@ struct StatementPositionRuleExamples { Example(""" do { foo() - ↓} catch { + } ↓catch { bar() } """): Example(""" @@ -216,17 +216,17 @@ struct StatementPositionRuleExamples { Example(""" do { foo() - ↓} + } // comment - catch { + ↓catch { bar() } """): Example(""" do { foo() - ↓} + } // comment - catch { + ↓catch { bar() } """, allowsViolationsInCorrections: true), @@ -286,25 +286,25 @@ struct StatementPositionRuleExamples { static let uncuddledTriggeringExamples = [ Example(""" if true { - ↓}else if false { + }↓else if false { } """), Example(""" if condition { - ↓} - else { + } + ↓else { } """), Example(""" do { - ↓} - catch { + } + ↓catch { } """), Example(""" do { - ↓} - \t catch { + } + \t ↓catch { } """), ] diff --git a/Tests/BuiltInRulesTests/StatementPositionRuleTests.swift b/Tests/BuiltInRulesTests/StatementPositionRuleTests.swift index 565b6fd57a..ce8037bdb5 100644 --- a/Tests/BuiltInRulesTests/StatementPositionRuleTests.swift +++ b/Tests/BuiltInRulesTests/StatementPositionRuleTests.swift @@ -65,48 +65,48 @@ final class StatementPositionRuleTests: SwiftLintTestCase { Example(""" if true { foo() - ↓} else { + } ↓else { bar() } """), Example(""" if true { foo() - ↓} else if true { + } ↓else if true { bar() - ↓} else { + } ↓else { return } """), Example(""" if true { foo() - ↓} - else { + } + ↓else { bar() } """), Example(""" do { foo() - ↓} catch { + } ↓catch { bar() } """), Example(""" do { foo() - ↓} catch let error { + } ↓catch let error { bar() - ↓} catch { + } ↓catch { return } """), Example(""" do { foo() - ↓} - catch { + } + ↓catch { bar() } """), @@ -116,8 +116,8 @@ final class StatementPositionRuleTests: SwiftLintTestCase { Example(""" if true { foo() - ↓} - else { + } + ↓else { bar() } """): @@ -132,9 +132,9 @@ final class StatementPositionRuleTests: SwiftLintTestCase { Example(""" if true { foo() - ↓} else if true { + } ↓else if true { bar() - ↓} else { + } ↓else { bar() } """): @@ -152,7 +152,7 @@ final class StatementPositionRuleTests: SwiftLintTestCase { Example(""" do { foo() - ↓} catch { + } ↓catch { bar() } """): @@ -167,8 +167,8 @@ final class StatementPositionRuleTests: SwiftLintTestCase { Example(""" do { foo() - ↓} - catch { + } + ↓catch { bar() } """):