diff --git a/.swiftlint.yml b/.swiftlint.yml index dd7139c91..75238a677 100644 --- a/.swiftlint.yml +++ b/.swiftlint.yml @@ -46,6 +46,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 952a8e4b7..86ebf5650 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -423,6 +423,10 @@ [p4checo](https://github.com/p4checo) [#5965](https://github.com/realm/SwiftLint/issues/5965) +* Add opt-in `unneeded_throws_rethrows` rule that triggers when declarations + marked `throws`/`rethrows` never actually throw or call any throwing code. + [Tony Ngo](https://github.com/tonyskansf) + ### Bug Fixes * Fix `no_extension_access_modifier` rule incorrectly triggering for `nonisolated extension`. diff --git a/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift b/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift index 1c13be34b..bda21e8ec 100644 --- a/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift +++ b/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift @@ -224,6 +224,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 000000000..41eb85ebe --- /dev/null +++ b/Source/SwiftLintBuiltInRules/Rules/Lint/UnneededThrowsRule.swift @@ -0,0 +1,242 @@ +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, + ] + } + + 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 let lintableFunctionType = node.lintableFunctionType { + scopes.openScope(with: lintableFunctionType.effectSpecifiers?.throwsClause) + } + return .visitChildren + } + + override func visitPost(_ node: PatternBindingSyntax) { + if node.lintableFunctionType != nil { + if let closedScope = scopes.closeScope() { + validate( + scope: closedScope, + construct: "closure type" + ) + } + } + } + + override func visit(_ node: ClosureExprSyntax) -> SyntaxVisitorContinueKind { + if let throwsClause = node.signature?.effectSpecifiers?.throwsClause { + scopes.openScope(with: throwsClause) + } + return .visitChildren + } + + override func visitPost(_ node: ClosureExprSyntax) { + if node.signature?.effectSpecifiers?.throwsClause != nil, let closedScope = scopes.closeScope() { + validate( + scope: closedScope, + construct: "body of this closure" + ) + } + } + + override func visit(_ node: FunctionCallExprSyntax) -> SyntaxVisitorContinueKind { + if node.containsClosureDeclaration { + scopes.openScope() + } + return .visitChildren + } + + override func visitPost(_ node: FunctionCallExprSyntax) { + if node.containsClosureDeclaration { + 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: \.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( + .init( + position: throwsClause.positionAfterSkippingLeadingTrivia, + reason: "Superfluous 'throws'; \(construct) does not throw any error", + correction: .init( + // Move start position back by 1 to include the space before the keyword. + 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 containsClosureDeclaration: Bool { + children(viewMode: .sourceAccurate).contains { $0.is(ClosureExprSyntax.self) } + } +} + +private extension PatternBindingSyntax { + private var hasNonReferenceInitializer: Bool { + ![.declReferenceExpr, .memberAccessExpr, .functionCallExpr, nil].contains(initializer?.value.kind) + } + + private var isLetBinding: Bool { + parent?.as(PatternBindingListSyntax.self)? + .parent?.as(VariableDeclSyntax.self)? + .bindingSpecifier.tokenKind == .keyword(.let) + } + + var lintableFunctionType: FunctionTypeSyntax? { + guard isLetBinding, hasNonReferenceInitializer else { + return nil + } + return 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 000000000..49c8bb525 --- /dev/null +++ b/Source/SwiftLintBuiltInRules/Rules/Lint/UnneededThrowsRuleExamples.swift @@ -0,0 +1,394 @@ +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() }"), + Example("let foo: () throws -> Void = bar"), + Example("var foo: () throws -> Void = {}"), + Example("let x = { () throws -> Void in try baz() }"), + ] + + 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 {} + } + """), + Example(""" + func foo() ↓throws { + bar(1) { + try baz() + } + } + """), + Example("let x = { () ↓throws -> Void in baz() }"), + ] + + 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 */ {}"), + Example("let foo: @Sendable () ↓throws -> Void = {}"): Example("let foo: @Sendable () -> Void = {}"), + ] +} diff --git a/Tests/GeneratedTests/GeneratedTests_09.swift b/Tests/GeneratedTests/GeneratedTests_09.swift index b7081609b..386970cf9 100644 --- a/Tests/GeneratedTests/GeneratedTests_09.swift +++ b/Tests/GeneratedTests/GeneratedTests_09.swift @@ -139,6 +139,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 UntypedErrorInCatchRuleGeneratedTests: SwiftLintTestCase { verifyRule(UntypedErrorInCatchRule.description) } } - -final class UnusedClosureParameterRuleGeneratedTests: SwiftLintTestCase { - func testWithDefaultConfiguration() { - verifyRule(UnusedClosureParameterRule.description) - } -} diff --git a/Tests/GeneratedTests/GeneratedTests_10.swift b/Tests/GeneratedTests/GeneratedTests_10.swift index ec711592c..017096b03 100644 --- a/Tests/GeneratedTests/GeneratedTests_10.swift +++ b/Tests/GeneratedTests/GeneratedTests_10.swift @@ -7,6 +7,12 @@ @testable import SwiftLintCore import TestHelpers +final class UnusedClosureParameterRuleGeneratedTests: SwiftLintTestCase { + func testWithDefaultConfiguration() { + verifyRule(UnusedClosureParameterRule.description) + } +} + final class UnusedControlFlowLabelRuleGeneratedTests: SwiftLintTestCase { func testWithDefaultConfiguration() { verifyRule(UnusedControlFlowLabelRule.description) diff --git a/Tests/IntegrationTests/default_rule_configurations.yml b/Tests/IntegrationTests/default_rule_configurations.yml index 6a0bb3bd7..392a9b678 100644 --- a/Tests/IntegrationTests/default_rule_configurations.yml +++ b/Tests/IntegrationTests/default_rule_configurations.yml @@ -1287,6 +1287,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: