diff --git a/CHANGELOG.md b/CHANGELOG.md index d9f0544d7..92ef8ddce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -80,6 +80,12 @@ [SimplyDanny](https://github.com/SimplyDanny) [#4843](https://github.com/realm/SwiftLint/issues/4843) +* Add new `unhandled_throwing_task` rule that triggers when a Task with an + implicit error type has unhandled trys or errors thrown inside its body. + This results in errors being silently discarded, which may be unexpected. + [kylebshr](https://github.com/kylebshr) + [#4958](https://github.com/realm/SwiftLint/pull/4958) + #### Bug Fixes * Fix `lower_acl_than_parent` rule rewriter by preserving leading whitespace. diff --git a/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift b/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift index 71b88767b..b3f6a1a45 100644 --- a/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift +++ b/Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift @@ -204,6 +204,7 @@ public let builtInRules: [Rule.Type] = [ TypesafeArrayInitRule.self, UnavailableConditionRule.self, UnavailableFunctionRule.self, + UnhandledThrowingTaskRule.self, UnneededBreakInSwitchRule.self, UnneededParenthesesInClosureArgumentRule.self, UnownedVariableCaptureRule.self, diff --git a/Source/SwiftLintBuiltInRules/Rules/Lint/UnhandledThrowingTaskRule.swift b/Source/SwiftLintBuiltInRules/Rules/Lint/UnhandledThrowingTaskRule.swift new file mode 100644 index 000000000..49b6818d9 --- /dev/null +++ b/Source/SwiftLintBuiltInRules/Rules/Lint/UnhandledThrowingTaskRule.swift @@ -0,0 +1,267 @@ +import SwiftSyntax + +struct UnhandledThrowingTaskRule: ConfigurationProviderRule, SwiftSyntaxRule { + var configuration = SeverityConfiguration(.error) + + static let description = RuleDescription( + identifier: "unhandled_throwing_task", + name: "Unhandled Throwing Task", + description: """ + Errors thrown inside this task are not handled, which may be unexpected. \ + Handle errors inside the task, or use `try await` to access the Tasks value and handle errors. \ + See this forum thread for more details: \ + https://forums.swift.org/t/task-initializer-with-throwing-closure-swallows-error/56066) + """, + kind: .lint, + nonTriggeringExamples: [ + Example(""" + Task { + try await myThrowingFunction() + } + """), + Example(""" + Task { + try? await myThrowingFunction() + } + """), + Example(""" + Task { + try! await myThrowingFunction() + } + """), + Example(""" + Task { + let text = try myThrowingFunction() + return text + } + """), + Example(""" + Task { + do { + try myThrowingFunction() + } catch let e { + print(e) + } + } + """), + Example(""" + func someFunction() throws { + Task { + anotherFunction() + do { + try myThrowingFunction() + } catch { + print(error) + } + } + + try something() + } + """), + Example(""" + let task = Task { + try await myThrowingFunction() + } + """), + Example(""" + var task = Task { + try await myThrowingFunction() + } + """), + Example(""" + try await Task { + try await myThrowingFunction() + }.value + """), + Example(""" + executor.task = Task { + try await isolatedOpen(.init(executor.asUnownedSerialExecutor())) + } + """) + ], + triggeringExamples: [ + Example(""" + ↓Task { + try await myThrowingFunction() + } + """), + Example(""" + ↓Task { + let text = try myThrowingFunction() + return text + } + """), + Example(""" + ↓Task { + do { + try myThrowingFunction() + } + } + """), + Example(""" + ↓Task { + do { + try myThrowingFunction() + } catch let e as FooError { + print(e) + } + } + """), + Example(""" + ↓Task { + do { + throw FooError.bar + } + } + """), + Example(""" + ↓Task { + throw FooError.bar + } + """), + Example(""" + ↓Task<_, _> { + throw FooError.bar + } + """), + Example(""" + ↓Task { + throw FooError.bar + } + """), + Example(""" + ↓Task { + do { + try foo() + } catch { + try bar() + } + } + """), + Example(""" + ↓Task { + do { + try foo() + } catch { + throw BarError() + } + } + """) + ] + ) + + func makeVisitor(file: SwiftLintFile) -> ViolationsSyntaxVisitor { + Visitor(viewMode: .sourceAccurate) + } +} + +private extension UnhandledThrowingTaskRule { + final class Visitor: ViolationsSyntaxVisitor { + override func visitPost(_ node: FunctionCallExprSyntax) { + if node.hasViolation { + violations.append(node.calledExpression.positionAfterSkippingLeadingTrivia) + } + } + } +} + +private extension FunctionCallExprSyntax { + var hasViolation: Bool { + isTaskWithImplicitErrorType && + doesThrow && + !(isAssigned || isValueAccessed) + } + + var isTaskWithImplicitErrorType: Bool { + if let typeIdentifier = calledExpression.as(IdentifierExprSyntax.self), + typeIdentifier.identifier.text == "Task" { + return true + } + + if let specializedExpression = calledExpression.as(SpecializeExprSyntax.self), + let typeIdentifier = specializedExpression.expression.as(IdentifierExprSyntax.self), + typeIdentifier.identifier.text == "Task", + let lastGeneric = specializedExpression.genericArgumentClause + .arguments.last?.argumentType.as(SimpleTypeIdentifierSyntax.self), + lastGeneric.typeName == "_" { + return true + } + + return false + } + + var isAssigned: Bool { + guard let parent else { + return false + } + + if parent.is(InitializerClauseSyntax.self) { + return true + } + + if let list = parent.as(ExprListSyntax.self), + list.contains(where: { $0.is(AssignmentExprSyntax.self) }) { + return true + } + + return false + } + + var isValueAccessed: Bool { + guard let parent = parent?.as(MemberAccessExprSyntax.self) else { + return false + } + + return parent.name.text == "value" + } + + var doesThrow: Bool { + ThrowsVisitor(viewMode: .sourceAccurate) + .walk(tree: self, handler: \.doesThrow) + } +} + +/// If the `doesThrow` property is true after visiting, then this node throws an error that is "unhandled." +/// Try statements inside a `do` with a `catch` that handles all errors will not be marked as throwing. +private final class ThrowsVisitor: SyntaxVisitor { + var doesThrow = false + + override func visit(_ node: DoStmtSyntax) -> SyntaxVisitorContinueKind { + // No need to continue traversing if we already throw. + if doesThrow { + return .skipChildren + } + + // If there are no catch clauses, visit children to see if there are any try expressions. + guard let lastCatchClause = node.catchClauses?.last else { + return .visitChildren + } + + let catchItems = lastCatchClause.catchItems ?? [] + + // If we have a value binding pattern, only an IdentifierPatternSyntax will catch + // any error; if it's not an IdentifierPatternSyntax, we need to visit children. + if let pattern = catchItems.last?.pattern?.as(ValueBindingPatternSyntax.self), + !pattern.valuePattern.is(IdentifierPatternSyntax.self) { + return .visitChildren + } + + // Check the catch clause tree for unhandled throws. + if ThrowsVisitor(viewMode: .sourceAccurate).walk(tree: lastCatchClause, handler: \.doesThrow) { + doesThrow = true + } + + // We don't need to visit children of the `do` node, since all errors are handled by the catch. + return .skipChildren + } + + override func visitPost(_ node: TryExprSyntax) { + if node.questionOrExclamationMark == nil { + doesThrow = true + } + } + + override func visitPost(_ node: ThrowStmtSyntax) { + doesThrow = true + } +} diff --git a/Tests/GeneratedTests/GeneratedTests.swift b/Tests/GeneratedTests/GeneratedTests.swift index 3b5ea994c..62efdac74 100644 --- a/Tests/GeneratedTests/GeneratedTests.swift +++ b/Tests/GeneratedTests/GeneratedTests.swift @@ -1220,6 +1220,12 @@ class UnavailableFunctionRuleGeneratedTests: SwiftLintTestCase { } } +class UnhandledThrowingTaskRuleGeneratedTests: SwiftLintTestCase { + func testWithDefaultConfiguration() { + verifyRule(UnhandledThrowingTaskRule.description) + } +} + class UnneededBreakInSwitchRuleGeneratedTests: SwiftLintTestCase { func testWithDefaultConfiguration() { verifyRule(UnneededBreakInSwitchRule.description)