|
| 1 | +# Return errors from `#expect(throws:)` |
| 2 | + |
| 3 | +* Proposal: [SWT-0006](0006-filename.md) |
| 4 | +* Authors: [Jonathan Grynspan](https://github.com/grynspan) |
| 5 | +* Status: **Awaiting review** |
| 6 | +* Bug: rdar://138235250 |
| 7 | +* Implementation: [swiftlang/swift-testing#780](https://github.com/swiftlang/swift-testing/pull/780) |
| 8 | +* Review: ([pitch](https://forums.swift.org/t/pitch-returning-errors-from-expect-throws/75567)) |
| 9 | + |
| 10 | +## Introduction |
| 11 | + |
| 12 | +Swift Testing includes overloads of `#expect()` and `#require()` that can be |
| 13 | +used to assert that some code throws an error. They are useful when validating |
| 14 | +that your code's failure cases are correctly detected and handled. However, for |
| 15 | +more complex validation cases, they aren't particularly ergonomic. This proposal |
| 16 | +seeks to resolve that issue by having these overloads return thrown errors for |
| 17 | +further inspection. |
| 18 | + |
| 19 | +## Motivation |
| 20 | + |
| 21 | +We offer three variants of `#expect(throws:)`: |
| 22 | + |
| 23 | +- One that takes an error type, and matches any error of the same type; |
| 24 | +- One that takes an error _instance_ (conforming to `Equatable`) and matches any |
| 25 | + error that compares equal to it; and |
| 26 | +- One that takes a trailing closure and allows test authors to write arbitrary |
| 27 | + validation logic. |
| 28 | + |
| 29 | +The third overload has proven to be somewhat problematic. First, it yields the |
| 30 | +error to its closure as an instance of `any Error`, which typically forces the |
| 31 | +developer to cast it before doing any useful comparisons. Second, the test |
| 32 | +author must return `true` to indicate the error matched and `false` to indicate |
| 33 | +it didn't, which can be both logically confusing and difficult to express |
| 34 | +concisely: |
| 35 | + |
| 36 | +```swift |
| 37 | +try #require { |
| 38 | + let potato = try Sack.randomPotato() |
| 39 | + try potato.turnIntoFrenchFries() |
| 40 | +} throws: { error in |
| 41 | + guard let error = error as PotatoError else { |
| 42 | + return false |
| 43 | + } |
| 44 | + guard case .potatoNotPeeled = error else { |
| 45 | + return false |
| 46 | + } |
| 47 | + return error.variety != .russet |
| 48 | +} |
| 49 | +``` |
| 50 | + |
| 51 | +The first impulse many test authors have here is to use `#expect()` in the |
| 52 | +second closure, but it doesn't return the necessary boolean value _and_ it can |
| 53 | +result in multiple issues being recorded in a test when there's really only one. |
| 54 | + |
| 55 | +## Proposed solution |
| 56 | + |
| 57 | +I propose deprecating [`#expect(_:sourceLocation:performing:throws:)`](https://developer.apple.com/documentation/testing/expect(_:sourcelocation:performing:throws:)) |
| 58 | +and [`#require(_:sourceLocation:performing:throws:)`](https://developer.apple.com/documentation/testing/require(_:sourcelocation:performing:throws:)) |
| 59 | +and modifying the other overloads so that, on success, they return the errors |
| 60 | +that were thrown. |
| 61 | + |
| 62 | +## Detailed design |
| 63 | + |
| 64 | +All overloads of `#expect(throws:)` and `#require(throws:)` will be updated to |
| 65 | +return an instance of the error type specified by their arguments, with the |
| 66 | +problematic overloads returning `any Error` since more precise type information |
| 67 | +is not statically available. The problematic overloads will also be deprecated: |
| 68 | + |
| 69 | +```diff |
| 70 | +--- a/Sources/Testing/Expectations/Expectation+Macro.swift |
| 71 | ++++ b/Sources/Testing/Expectations/Expectation+Macro.swift |
| 72 | ++@discardableResult |
| 73 | + @freestanding(expression) public macro expect<E, R>( |
| 74 | + throws errorType: E.Type, |
| 75 | + _ comment: @autoclosure () -> Comment? = nil, |
| 76 | + sourceLocation: SourceLocation = #_sourceLocation, |
| 77 | + performing expression: () async throws -> R |
| 78 | +-) |
| 79 | ++) -> E? where E: Error |
| 80 | + |
| 81 | ++@discardableResult |
| 82 | + @freestanding(expression) public macro require<E, R>( |
| 83 | + throws errorType: E.Type, |
| 84 | + _ comment: @autoclosure () -> Comment? = nil, |
| 85 | + sourceLocation: SourceLocation = #_sourceLocation, |
| 86 | + performing expression: () async throws -> R |
| 87 | +-) where E: Error |
| 88 | ++) -> E where E: Error |
| 89 | + |
| 90 | ++@discardableResult |
| 91 | + @freestanding(expression) public macro expect<E, R>( |
| 92 | + throws error: E, |
| 93 | + _ comment: @autoclosure () -> Comment? = nil, |
| 94 | + sourceLocation: SourceLocation = #_sourceLocation, |
| 95 | + performing expression: () async throws -> R |
| 96 | +-) where E: Error & Equatable |
| 97 | ++) -> E? where E: Error & Equatable |
| 98 | + |
| 99 | ++@discardableResult |
| 100 | + @freestanding(expression) public macro require<E, R>( |
| 101 | + throws error: E, |
| 102 | + _ comment: @autoclosure () -> Comment? = nil, |
| 103 | + sourceLocation: SourceLocation = #_sourceLocation, |
| 104 | + performing expression: () async throws -> R |
| 105 | +-) where E: Error & Equatable |
| 106 | ++) -> E where E: Error & Equatable |
| 107 | + |
| 108 | ++@available(*, deprecated, message: "Examine the result of '#expect(throws:)' instead.") |
| 109 | ++@discardableResult |
| 110 | + @freestanding(expression) public macro expect<R>( |
| 111 | + _ comment: @autoclosure () -> Comment? = nil, |
| 112 | + sourceLocation: SourceLocation = #_sourceLocation, |
| 113 | + performing expression: () async throws -> R, |
| 114 | + throws errorMatcher: (any Error) async throws -> Bool |
| 115 | +-) |
| 116 | ++) -> (any Error)? |
| 117 | + |
| 118 | ++@available(*, deprecated, message: "Examine the result of '#require(throws:)' instead.") |
| 119 | ++@discardableResult |
| 120 | + @freestanding(expression) public macro require<R>( |
| 121 | + _ comment: @autoclosure () -> Comment? = nil, |
| 122 | + sourceLocation: SourceLocation = #_sourceLocation, |
| 123 | + performing expression: () async throws -> R, |
| 124 | + throws errorMatcher: (any Error) async throws -> Bool |
| 125 | +-) |
| 126 | ++) -> any Error |
| 127 | +``` |
| 128 | + |
| 129 | +(More detailed information about the deprecations will be provided via DocC.) |
| 130 | + |
| 131 | +The `#expect(throws:)` overloads return an optional value that is `nil` if the |
| 132 | +expectation failed, while the `#require(throws:)` overloads return non-optional |
| 133 | +values and throw instances of `ExpectationFailedError` on failure (as before.) |
| 134 | + |
| 135 | +> [!NOTE] |
| 136 | +> Instances of `ExpectationFailedError` thrown by `#require(throws:)` on failure |
| 137 | +> are not returned as that would defeat the purpose of using `#require(throws:)` |
| 138 | +> instead of `#expect(throws:)`. |
| 139 | +
|
| 140 | +Test authors will be able to use the result of the above functions to verify |
| 141 | +that the thrown error is correct: |
| 142 | + |
| 143 | +```swift |
| 144 | +let error = try #require(throws: PotatoError.self) { |
| 145 | + let potato = try Sack.randomPotato() |
| 146 | + try potato.turnIntoFrenchFries() |
| 147 | +} |
| 148 | +#expect(error == .potatoNotPeeled) |
| 149 | +#expect(error.variety != .russet) |
| 150 | +``` |
| 151 | + |
| 152 | +The new code is more concise than the old code and avoids boilerplate casting |
| 153 | +from `any Error`. |
| 154 | + |
| 155 | +## Source compatibility |
| 156 | + |
| 157 | +In most cases, this change does not affect source compatibility. Swift does not |
| 158 | +allow forming references to macros at runtime, so we don't need to worry about |
| 159 | +type mismatches assigning one to some local variable. |
| 160 | + |
| 161 | +We have identified two scenarios where a new warning will be emitted. |
| 162 | + |
| 163 | +### Inferred return type from macro invocation |
| 164 | + |
| 165 | +The return type of the macro may be used by the compiler to infer the return |
| 166 | +type of an enclosing closure. If the return value is then discarded, the |
| 167 | +compiler may emit a warning: |
| 168 | + |
| 169 | +```swift |
| 170 | +func pokePotato(_ pPotato: UnsafePointer<Potato>) throws { ... } |
| 171 | + |
| 172 | +let potato = Potato() |
| 173 | +try await Task.sleep(for: .months(3)) |
| 174 | +withUnsafePointer(to: potato) { pPotato in |
| 175 | + // ^ ^ ^ ⚠️ Result of call to 'withUnsafePointer(to:_:)' is unused |
| 176 | + #expect(throws: PotatoError.rotten) { |
| 177 | + try pokePotato(pPotato) |
| 178 | + } |
| 179 | +} |
| 180 | +``` |
| 181 | + |
| 182 | +This warning can be suppressed by assigning the result of the macro invocation |
| 183 | +or the result of the function call to `_`: |
| 184 | + |
| 185 | +```swift |
| 186 | +withUnsafePointer(to: potato) { pPotato in |
| 187 | + _ = #expect(throws: PotatoError.rotten) { |
| 188 | + try pokePotato(pPotato) |
| 189 | + } |
| 190 | +} |
| 191 | +``` |
| 192 | + |
| 193 | +### Use of `#require(throws:)` in a generic context with `Never.self` |
| 194 | + |
| 195 | +If `#require(throws:)` (but not `#expect(throws:)`) is used in a generic context |
| 196 | +where the type of thrown error is a generic parameter, and the type is resolved |
| 197 | +to `Never`, there is no valid value for the invocation to return: |
| 198 | + |
| 199 | +```swift |
| 200 | +func wrapper<E>(throws type: E.Type, _ body: () throws -> Void) throws -> E { |
| 201 | + return try #require(throws: type) { |
| 202 | + try body() |
| 203 | + } |
| 204 | +} |
| 205 | +let error = try #require(throws: Never.self) { ... } |
| 206 | +``` |
| 207 | + |
| 208 | +We don't think this particular pattern is common (and outside of our own test |
| 209 | +target, I'd be surprised if anybody's attempted it yet.) However, we do need to |
| 210 | +handle it gracefully. If this pattern is encountered, Swift Testing will record |
| 211 | +an "API Misused" issue for the current test and advise the test author to switch |
| 212 | +to `#expect(throws:)` or to not pass `Never.self` here. |
| 213 | + |
| 214 | +## Integration with supporting tools |
| 215 | + |
| 216 | +N/A |
| 217 | + |
| 218 | +## Future directions |
| 219 | + |
| 220 | +- Adopting [typed throws](https://github.com/swiftlang/swift-evolution/blob/main/proposals/0413-typed-throws.md) |
| 221 | + to statically require that the error thrown from test code is of the correct |
| 222 | + type. |
| 223 | + |
| 224 | + If we adopted typed throws in the signatures of these macros, it would force |
| 225 | + adoption of typed throws in the code under test even when it may not be |
| 226 | + appropriate. For example, if we adopted typed throws, the following code would |
| 227 | + not compile: |
| 228 | + |
| 229 | + ```swift |
| 230 | + func cook(_ food: consuming some Food) throws { ... } |
| 231 | + |
| 232 | + let error: PotatoError? = #expect(throws: PotatoError.self) { |
| 233 | + var potato = Potato() |
| 234 | + potato.fossilize() |
| 235 | + try cook(potato) // 🛑 ERROR: Invalid conversion of thrown error type |
| 236 | + // 'any Error' to 'PotatoError' |
| 237 | + } |
| 238 | + ``` |
| 239 | + |
| 240 | + We believe it may be possible to overload these macros or their expansions so |
| 241 | + that the code sample above _does_ compile and behave as intended. We intend to |
| 242 | + experiment further with this idea and potentially revisit typed throws support |
| 243 | + in a future proposal. |
| 244 | + |
| 245 | +## Alternatives considered |
| 246 | + |
| 247 | +- Leaving the existing implementation and signatures in place. We've had |
| 248 | + sufficient feedback about the ergonomics of this API that we want to address |
| 249 | + the problem. |
| 250 | + |
| 251 | +- Having the return type of the macros be `any Error` and returning _any_ error |
| 252 | + that was thrown even on mismatch. This would make the ergonomics of the |
| 253 | + subsequent test code less optimal because the test author would need to cast |
| 254 | + the error to the appropriate type before inspecting it. |
| 255 | + |
| 256 | + There's a philosophical argument to be made here that if a mismatched error is |
| 257 | + thrown, then the test has already failed and is in an inconsistent state, so |
| 258 | + we should allow the test to fail rather than return what amounts to "bad |
| 259 | + output". |
| 260 | + |
| 261 | + If the test author wants to inspect any arbitrary thrown error, they can |
| 262 | + specify `(any Error).self` instead of a concrete error type. |
| 263 | + |
| 264 | +## Acknowledgments |
| 265 | + |
| 266 | +Thanks to the team and to [@jakepetroules](https://github.com/jakepetroules) for |
| 267 | +starting the discussion that ultimately led to this proposal. |
0 commit comments