diff --git a/.swiftlint.yml b/.swiftlint.yml index 5c349312a8..4201a9a70f 100644 --- a/.swiftlint.yml +++ b/.swiftlint.yml @@ -47,6 +47,7 @@ disabled_rules: - todo - trailing_closure - type_contents_order + - unneeded_throws_rethrows - vertical_whitespace_between_cases # Configurations diff --git a/CHANGELOG.md b/CHANGELOG.md index 694500781c..c02e6e3ad0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -83,6 +83,10 @@ * Support deinitializers and subscripts in `function_body_length` rule. [SimplyDanny](https://github.com/SimplyDanny) +* Add opt-in `unneeded_throws_rethrows` rules that triggers when declarations + marked `throws`/`rethrows` never actually throw or call any throwing code. + [Tony Ngo](https://github.com/tonyskansf) + ### Bug Fixes * Improved error reporting when SwiftLint exits, because of an invalid configuration file diff --git a/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift b/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift index bea8255b9c..c759c75998 100644 --- a/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift +++ b/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift @@ -222,6 +222,7 @@ public let builtInRules: [any Rule.Type] = [ UnneededOverrideRule.self, UnneededParenthesesInClosureArgumentRule.self, UnneededSynthesizedInitializerRule.self, + UnneededThrowsRule.self, UnownedVariableCaptureRule.self, UntypedErrorInCatchRule.self, UnusedClosureParameterRule.self, diff --git a/Source/SwiftLintBuiltInRules/Rules/Lint/UnneededThrowsRule.swift b/Source/SwiftLintBuiltInRules/Rules/Lint/UnneededThrowsRule.swift new file mode 100644 index 0000000000..d34cdc04e5 --- /dev/null +++ b/Source/SwiftLintBuiltInRules/Rules/Lint/UnneededThrowsRule.swift @@ -0,0 +1,220 @@ +import SwiftLintCore +import SwiftSyntax + +@SwiftSyntaxRule(correctable: true, optIn: true) +struct UnneededThrowsRule: Rule { + var configuration = SeverityConfiguration(.warning) + + static let description = RuleDescription( + identifier: "unneeded_throws_rethrows", + name: "Unneeded (re)throws keyword", + description: "Non-throwing functions/properties/closures should not be marked as `throws` or `rethrows`.", + kind: .lint, + nonTriggeringExamples: UnneededThrowsRuleExamples.nonTriggeringExamples, + triggeringExamples: UnneededThrowsRuleExamples.triggeringExamples, + corrections: UnneededThrowsRuleExamples.corrections + ) +} + +private extension UnneededThrowsRule { + struct Scope { + var throwsClause: ThrowsClauseSyntax? + } + + final class Visitor: ViolationsSyntaxVisitor { + private var scopes = Stack() + + override var skippableDeclarations: [any DeclSyntaxProtocol.Type] { + [ + ProtocolDeclSyntax.self, + TypeAliasDeclSyntax.self, + EnumCaseDeclSyntax.self, + ] + } + + override func visit(_: FunctionParameterClauseSyntax) -> SyntaxVisitorContinueKind { + .skipChildren + } + + override func visit(_ node: InitializerDeclSyntax) -> SyntaxVisitorContinueKind { + scopes.openScope(with: node.signature.effectSpecifiers?.throwsClause) + return .visitChildren + } + + override func visitPost(_: InitializerDeclSyntax) { + if let closedScope = scopes.closeScope() { + validate( + scope: closedScope, + construct: "initializer" + ) + } + } + + override func visit(_ node: AccessorDeclSyntax) -> SyntaxVisitorContinueKind { + scopes.openScope(with: node.effectSpecifiers?.throwsClause) + return .visitChildren + } + + override func visitPost(_: AccessorDeclSyntax) { + if let closedScope = scopes.closeScope() { + validate( + scope: closedScope, + construct: "accessor" + ) + } + } + + override func visit(_ node: FunctionDeclSyntax) -> SyntaxVisitorContinueKind { + scopes.openScope(with: node.signature.effectSpecifiers?.throwsClause) + return .visitChildren + } + + override func visitPost(_: FunctionDeclSyntax) { + if let closedScope = scopes.closeScope() { + validate( + scope: closedScope, + construct: "body of this function" + ) + } + } + + override func visit(_ node: PatternBindingSyntax) -> SyntaxVisitorContinueKind { + if node.containsInitializerClause, let functionTypeSyntax = node.functionTypeSyntax { + scopes.openScope(with: functionTypeSyntax.effectSpecifiers?.throwsClause) + } + return .visitChildren + } + + override func visitPost(_ node: PatternBindingSyntax) { + if node.containsInitializerClause, node.functionTypeSyntax != nil { + if let closedScope = scopes.closeScope() { + validate( + scope: closedScope, + construct: "closure type" + ) + } + } + } + + override func visit(_ node: FunctionCallExprSyntax) -> SyntaxVisitorContinueKind { + if node.containsTaskDeclaration { + scopes.openScope() + } + return .visitChildren + } + + override func visitPost(_ node: FunctionCallExprSyntax) { + if node.containsTaskDeclaration { + scopes.closeScope() + } + } + + override func visit(_: DoStmtSyntax) -> SyntaxVisitorContinueKind { + scopes.openScope() + return .visitChildren + } + + override func visitPost(_ node: CodeBlockSyntax) { + if node.parent?.is(DoStmtSyntax.self) == true { + scopes.closeScope() + } + } + + override func visitPost(_ node: DoStmtSyntax) { + if node.catchClauses.contains(where: { $0.catchItems.isEmpty }) { + // All errors will be caught. + return + } + scopes.markCurrentScopeAsThrowing() + } + + override func visitPost(_ node: ForStmtSyntax) { + if node.tryKeyword != nil { + scopes.markCurrentScopeAsThrowing() + } + } + + override func visitPost(_ node: TryExprSyntax) { + if node.questionOrExclamationMark == nil { + scopes.markCurrentScopeAsThrowing() + } + } + + override func visitPost(_: ThrowStmtSyntax) { + scopes.markCurrentScopeAsThrowing() + } + + private func validate(scope: Scope, construct: String) { + guard let throwsClause = scope.throwsClause else { return } + violations.append( + ReasonedRuleViolation( + position: throwsClause.positionAfterSkippingLeadingTrivia, + reason: "Superfluous 'throws'; \(construct) does not throw any error", + correction: ReasonedRuleViolation.ViolationCorrection( + // Move start position back by 1 to include the space before the throwsClause + start: throwsClause.positionAfterSkippingLeadingTrivia.advanced(by: -1), + end: throwsClause.endPositionBeforeTrailingTrivia, + replacement: "" + ) + ) + ) + } + } +} + +private extension Stack where Element == UnneededThrowsRule.Scope { + mutating func markCurrentScopeAsThrowing() { + modifyLast { currentScope in + currentScope.throwsClause = nil + } + } + + mutating func openScope(with throwsClause: ThrowsClauseSyntax? = nil) { + push(UnneededThrowsRule.Scope(throwsClause: throwsClause)) + } + + @discardableResult + mutating func closeScope() -> Element? { + pop() + } +} + +private extension FunctionCallExprSyntax { + var containsTaskDeclaration: Bool { + children(viewMode: .sourceAccurate).contains { child in + child.as(DeclReferenceExprSyntax.self)?.baseName.tokenKind == .identifier("Task") + } + } +} + +private extension PatternBindingSyntax { + var containsInitializerClause: Bool { + initializer != nil + } + + var functionTypeSyntax: FunctionTypeSyntax? { + typeAnnotation?.type.baseFunctionTypeSyntax + } +} + +private extension TypeSyntax { + var baseFunctionTypeSyntax: FunctionTypeSyntax? { + switch Syntax(self).as(SyntaxEnum.self) { + case .functionType(let function): + function + case .optionalType(let optional): + optional.wrappedType.baseFunctionTypeSyntax + case .attributedType(let attributed): + attributed.baseType.baseFunctionTypeSyntax + case .tupleType(let tuple): + // It's hard to check for the necessity of throws keyword in multi-element tuples + if tuple.elements.count == 1 { + tuple.elements.first?.type.baseFunctionTypeSyntax + } else { + nil + } + default: + nil + } + } +} diff --git a/Source/SwiftLintBuiltInRules/Rules/Lint/UnneededThrowsRuleExamples.swift b/Source/SwiftLintBuiltInRules/Rules/Lint/UnneededThrowsRuleExamples.swift new file mode 100644 index 0000000000..59fd6738da --- /dev/null +++ b/Source/SwiftLintBuiltInRules/Rules/Lint/UnneededThrowsRuleExamples.swift @@ -0,0 +1,382 @@ +internal struct UnneededThrowsRuleExamples { + static let nonTriggeringExamples = [ + Example(""" + func foo() throws { + try bar() + } + """), + Example(""" + func foo() throws { + throw Example.failure + } + """), + Example(""" + func foo() throws(Example) { + throw Example.failure + } + """), + Example(""" + func foo(_ bar: () throws -> T) rethrows -> Int { + try items.map { try bar() } + } + """), + Example(""" + func foo() { + func bar() throws { + try baz() + } + try? bar() + } + """), + Example(""" + protocol Foo { + func bar() throws + } + """), + Example(""" + func foo() throws { + guard false else { + throw Example.failure + } + } + """), + Example(""" + func foo() throws { + do { try bar() } + catch { + throw Example.failure + } + } + """), + Example(""" + func foo() throws { + do { try bar() } + catch { + try baz() + } + } + """), + Example(""" + func foo() throws { + do { + throw Example.failure + } catch { + do { + throw Example.failure + } catch { + throw Example.failure + } + } + } + """), + Example(""" + func foo() throws { + switch bar { + case 1: break + default: try bar() + } + } + """), + Example(""" + var foo: Int { + get throws { + try bar + } + } + """), + Example(""" + func foo() throws { + let bar = Bar() + + if bar.boolean { + throw Example.failure + } + } + """), + Example(""" + func foo() throws -> Bar? { + Bar(try baz()) + } + """), + Example(""" + typealias Foo = () throws -> Void + """), + Example(""" + enum Foo { + case foo + case bar(() throws -> Void) + } + """), + Example(""" + func foo() async throws { + for try await item in items {} + } + """), + Example(""" + let foo: () throws -> Void + """), + Example(""" + let foo: @Sendable () throws -> Void + """), + Example(""" + let foo: (() throws -> Void)? + """), + Example(""" + func foo(_ bar: () throws -> Void = {}) {} + """), + Example(""" + func foo() async throws { + func foo() {} + for _ in 0.. Void> = S()"), + Example("let foo: (() throws -> Void, Int) = ({}, 1)"), + Example("let foo: (Int, () throws -> Void) = (1, {})"), + Example("let foo: (Int, Int, () throws -> Void) = (1, 1, {})"), + Example("let foo: () throws -> Void = { try bar() }"), + ] + + static let triggeringExamples = [ + Example("func foo() ↓throws {}"), + Example("let foo: () ↓throws -> Void = {}"), + Example("let foo: (() ↓throws -> Void) = {}"), + Example("let foo: (() ↓throws -> Void)? = {}"), + Example("let foo: @Sendable () ↓throws -> Void = {}"), + Example("func foo(bar: () throws -> Void) ↓rethrows {}"), + Example("init() ↓throws {}"), + Example(""" + func foo() ↓throws { + bar() + } + """), + Example(""" + func foo() ↓throws(Example) { + bar() + } + """), + Example(""" + func foo() { + func bar() ↓throws {} + bar() + } + """), + Example(""" + func foo() { + func bar() ↓throws { + baz() + } + bar() + } + """), + Example(""" + func foo() { + func bar() ↓throws { + baz() + } + try? bar() + } + """), + Example(""" + func foo() ↓throws { + func bar() ↓throws { + baz() + } + } + """), + Example(""" + func foo() ↓throws { + do { try bar() } + catch {} + } + """), + Example(""" + func foo() ↓throws { + do {} + catch {} + } + """), + Example(""" + func foo() ↓throws(Example) { + do {} + catch {} + } + """), + Example(""" + func foo() { + do { + func bar() ↓throws {} + try bar() + } catch {} + } + """), + Example(""" + func foo() ↓throws { + do { + try bar() + func baz() throws { try bar() } + try baz() + } catch {} + } + """), + Example(""" + func foo() ↓throws { + do { + try bar() + } catch { + do { + throw Example.failure + } catch {} + } + } + """), + Example(""" + func foo() ↓throws { + do { + try bar() + } catch { + do { + try bar() + func baz() ↓throws {} + try baz() + } catch {} + } + } + """), + Example(""" + func foo() ↓throws { + switch bar { + case 1: break + default: break + } + } + """), + Example(""" + func foo() ↓throws { + _ = try? bar() + } + """), + Example(""" + func foo() ↓throws { + Task { + try bar() + } + } + """), + Example(""" + func foo() throws { + try bar() + Task { + func baz() ↓throws {} + } + } + """), + Example(""" + var foo: Int { + get ↓throws { + 0 + } + } + """), + Example(""" + func foo() ↓throws { + do { try bar() } + catch Example.failure {} + catch is SomeError {} + catch {} + } + """), + ] + + static let corrections = [ + Example("func foo() ↓throws {}"): Example("func foo() {}"), + Example("init() ↓throws {}"): Example("init() {}"), + Example(""" + func foo() { + func bar() ↓throws {} + bar() + } + """): Example(""" + func foo() { + func bar() {} + bar() + } + """), + Example(""" + var foo: Int { + get ↓throws { + 0 + } + } + """): Example(""" + var foo: Int { + get { + 0 + } + } + """), + Example(""" + var foo: Int { + get ↓throws(Example) { + 0 + } + } + """): Example(""" + var foo: Int { + get { + 0 + } + } + """), + Example(""" + let foo: () ↓throws -> Void = {} + """): Example(""" + let foo: () -> Void = {} + """), + Example(""" + let foo: () ↓throws(Example) -> Void = {} + """): Example(""" + let foo: () -> Void = {} + """), + Example(""" + func foo() ↓throws { + do {} + catch {} + } + """): Example(""" + func foo() { + do {} + catch {} + } + """), + Example(""" + func foo() ↓throws(Example) { + do {} + catch {} + } + """): Example(""" + func foo() { + do {} + catch {} + } + """), + Example("func f() ↓throws /* comment */ {}"): Example("func f() /* comment */ {}"), + Example("func f() /* comment */ ↓throws /* comment */ {}"): Example("func f() /* comment */ /* comment */ {}"), + ] +} diff --git a/Tests/GeneratedTests/GeneratedTests_09.swift b/Tests/GeneratedTests/GeneratedTests_09.swift index 1d8dc7614d..19cd5dc595 100644 --- a/Tests/GeneratedTests/GeneratedTests_09.swift +++ b/Tests/GeneratedTests/GeneratedTests_09.swift @@ -127,6 +127,12 @@ final class UnneededSynthesizedInitializerRuleGeneratedTests: SwiftLintTestCase } } +final class UnneededThrowsRuleGeneratedTests: SwiftLintTestCase { + func testWithDefaultConfiguration() { + verifyRule(UnneededThrowsRule.description) + } +} + final class UnownedVariableCaptureRuleGeneratedTests: SwiftLintTestCase { func testWithDefaultConfiguration() { verifyRule(UnownedVariableCaptureRule.description) @@ -150,9 +156,3 @@ final class UnusedControlFlowLabelRuleGeneratedTests: SwiftLintTestCase { verifyRule(UnusedControlFlowLabelRule.description) } } - -final class UnusedDeclarationRuleGeneratedTests: SwiftLintTestCase { - func testWithDefaultConfiguration() { - verifyRule(UnusedDeclarationRule.description) - } -} diff --git a/Tests/GeneratedTests/GeneratedTests_10.swift b/Tests/GeneratedTests/GeneratedTests_10.swift index a5ce334fc5..648dde1162 100644 --- a/Tests/GeneratedTests/GeneratedTests_10.swift +++ b/Tests/GeneratedTests/GeneratedTests_10.swift @@ -7,6 +7,12 @@ @testable import SwiftLintCore import TestHelpers +final class UnusedDeclarationRuleGeneratedTests: SwiftLintTestCase { + func testWithDefaultConfiguration() { + verifyRule(UnusedDeclarationRule.description) + } +} + final class UnusedEnumeratedRuleGeneratedTests: SwiftLintTestCase { func testWithDefaultConfiguration() { verifyRule(UnusedEnumeratedRule.description) diff --git a/Tests/IntegrationTests/default_rule_configurations.yml b/Tests/IntegrationTests/default_rule_configurations.yml index aaeffb97b6..8f869b3a7a 100644 --- a/Tests/IntegrationTests/default_rule_configurations.yml +++ b/Tests/IntegrationTests/default_rule_configurations.yml @@ -1270,6 +1270,11 @@ unneeded_synthesized_initializer: meta: opt-in: false correctable: true +unneeded_throws_rethrows: + severity: warning + meta: + opt-in: true + correctable: true unowned_variable_capture: severity: warning meta: