diff --git a/.swiftlint.yml b/.swiftlint.yml index f8a65c8d9..d77c440b5 100644 --- a/.swiftlint.yml +++ b/.swiftlint.yml @@ -44,6 +44,7 @@ disabled_rules: - self_binding - sorted_enum_cases - strict_fileprivate + - superfluous_else - switch_case_on_newline - trailing_closure - type_contents_order diff --git a/CHANGELOG.md b/CHANGELOG.md index d3a6f1f77..6958aac95 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,11 @@ #### Enhancements +* Add new `superfluous_else` rule that triggers on `if`-statements when an + attached `else`-block can be removed, because all branches of the previous + `if`-block(s) would certainly exit the current scope already. + [SimplyDanny](https://github.com/SimplyDanny) + * Add `sorted_enum_cases` rule which warns when enum cases are not sorted. [kimdv](https://github.com/kimdv) diff --git a/Source/SwiftLintFramework/Models/PrimaryRuleList.swift b/Source/SwiftLintFramework/Models/PrimaryRuleList.swift index 1e1388bb6..68091edd3 100644 --- a/Source/SwiftLintFramework/Models/PrimaryRuleList.swift +++ b/Source/SwiftLintFramework/Models/PrimaryRuleList.swift @@ -187,6 +187,7 @@ let builtInRules: [Rule.Type] = [ StrictFilePrivateRule.self, StrongIBOutletRule.self, SuperfluousDisableCommandRule.self, + SuperfluousElseRule.self, SwitchCaseAlignmentRule.self, SwitchCaseOnNewlineRule.self, SyntacticSugarRule.self, diff --git a/Source/SwiftLintFramework/Rules/Style/SuperfluousElseRule.swift b/Source/SwiftLintFramework/Rules/Style/SuperfluousElseRule.swift new file mode 100644 index 000000000..88e127642 --- /dev/null +++ b/Source/SwiftLintFramework/Rules/Style/SuperfluousElseRule.swift @@ -0,0 +1,152 @@ +import SwiftSyntax + +struct SuperfluousElseRule: SwiftSyntaxRule, ConfigurationProviderRule, OptInRule { + var configuration = SeverityConfiguration(.warning) + + init() {} + + static var description = RuleDescription( + identifier: "superfluous_else", + name: "Superfluous Else", + description: "Else branches should be avoided when the previous if-block exits the current scope", + kind: .style, + nonTriggeringExamples: [ + Example(""" + if i > 0 { + // comment + } else if i < 12 { + return 2 + } else { + return 3 + } + """), + Example(""" + if i > 0 { + let a = 1 + if a > 1 { + // comment + } else { + return 1 + } + // comment + } else { + return 3 + } + """), + Example(""" + if i > 0 { + if a > 1 { + return 1 + } + } else { + return 3 + } + """), + Example(""" + if i > 0 { + if a > 1 { + if a > 1 { + // comment + } else { + return 1 + } + } + } else { + return 3 + } + """, excludeFromDocumentation: true) + ], + triggeringExamples: [ + Example(""" + ↓if i > 0 { + return 1 + // comment + } else { + return 2 + } + """), + Example(""" + ↓if i > 0 { + return 1 + } else ↓if i < 12 { + return 2 + } else if i > 18 { + return 3 + } + """), + Example(""" + ↓if i > 0 { + ↓if i < 12 { + return 5 + } else { + ↓if i > 11 { + return 6 + } else { + return 7 + } + } + } else ↓if i < 12 { + return 2 + } else ↓if i < 24 { + return 8 + } else { + return 3 + } + """) + ] + ) + + func makeVisitor(file: SwiftLintFile) -> ViolationsSyntaxVisitor { + Visitor(viewMode: .sourceAccurate) + } +} + +private class Visitor: ViolationsSyntaxVisitor { + override var skippableDeclarations: [DeclSyntaxProtocol.Type] { [ProtocolDeclSyntax.self] } + + override func visitPost(_ node: IfExprSyntax) { + if node.violatesRule { + violations.append(node.ifKeyword.positionAfterSkippingLeadingTrivia) + } + } +} + +private extension IfExprSyntax { + var violatesRule: Bool { + if elseKeyword == nil { + return false + } + let thenBodyReturns = lastStatementReturns(in: body) + if thenBodyReturns, let parent = parent?.as(IfExprSyntax.self) { + return parent.violatesRule + } + return thenBodyReturns + } + + private var returnsInAllBranches: Bool { + guard lastStatementReturns(in: body) else { + return false + } + if case let .ifExpr(nestedIfStmt) = elseBody { + return nestedIfStmt.returnsInAllBranches + } + if case let .codeBlock(block) = elseBody { + return lastStatementReturns(in: block) + } + return false + } + + private func lastStatementReturns(in block: CodeBlockSyntax) -> Bool { + guard let lastItem = block.statements.last?.as(CodeBlockItemSyntax.self)?.item else { + return false + } + if lastItem.is(ReturnStmtSyntax.self) { + return true + } + if let exprStmt = lastItem.as(ExpressionStmtSyntax.self), + let lastIfStmt = exprStmt.expression.as(IfExprSyntax.self) { + return lastIfStmt.returnsInAllBranches + } + return false + } +} diff --git a/Tests/GeneratedTests/GeneratedTests.swift b/Tests/GeneratedTests/GeneratedTests.swift index b7865c5cd..9c37d5623 100644 --- a/Tests/GeneratedTests/GeneratedTests.swift +++ b/Tests/GeneratedTests/GeneratedTests.swift @@ -1106,6 +1106,12 @@ class SuperfluousDisableCommandRuleGeneratedTests: XCTestCase { } } +class SuperfluousElseRuleGeneratedTests: XCTestCase { + func testWithDefaultConfiguration() { + verifyRule(SuperfluousElseRule.description) + } +} + class SwitchCaseAlignmentRuleGeneratedTests: XCTestCase { func testWithDefaultConfiguration() { verifyRule(SwitchCaseAlignmentRule.description)