diff --git a/Documentation/Proposals/0006-return-errors-from-expect-throws.md b/Documentation/Proposals/0006-return-errors-from-expect-throws.md new file mode 100644 index 000000000..2f15c02ba --- /dev/null +++ b/Documentation/Proposals/0006-return-errors-from-expect-throws.md @@ -0,0 +1,267 @@ +# Return errors from `#expect(throws:)` + +* Proposal: [SWT-0006](0006-filename.md) +* Authors: [Jonathan Grynspan](https://github.com/grynspan) +* Status: **Awaiting review** +* Bug: rdar://138235250 +* Implementation: [swiftlang/swift-testing#780](https://github.com/swiftlang/swift-testing/pull/780) +* Review: ([pitch](https://forums.swift.org/t/pitch-returning-errors-from-expect-throws/75567)) + +## Introduction + +Swift Testing includes overloads of `#expect()` and `#require()` that can be +used to assert that some code throws an error. They are useful when validating +that your code's failure cases are correctly detected and handled. However, for +more complex validation cases, they aren't particularly ergonomic. This proposal +seeks to resolve that issue by having these overloads return thrown errors for +further inspection. + +## Motivation + +We offer three variants of `#expect(throws:)`: + +- One that takes an error type, and matches any error of the same type; +- One that takes an error _instance_ (conforming to `Equatable`) and matches any + error that compares equal to it; and +- One that takes a trailing closure and allows test authors to write arbitrary + validation logic. + +The third overload has proven to be somewhat problematic. First, it yields the +error to its closure as an instance of `any Error`, which typically forces the +developer to cast it before doing any useful comparisons. Second, the test +author must return `true` to indicate the error matched and `false` to indicate +it didn't, which can be both logically confusing and difficult to express +concisely: + +```swift +try #require { + let potato = try Sack.randomPotato() + try potato.turnIntoFrenchFries() +} throws: { error in + guard let error = error as PotatoError else { + return false + } + guard case .potatoNotPeeled = error else { + return false + } + return error.variety != .russet +} +``` + +The first impulse many test authors have here is to use `#expect()` in the +second closure, but it doesn't return the necessary boolean value _and_ it can +result in multiple issues being recorded in a test when there's really only one. + +## Proposed solution + +I propose deprecating [`#expect(_:sourceLocation:performing:throws:)`](https://developer.apple.com/documentation/testing/expect(_:sourcelocation:performing:throws:)) +and [`#require(_:sourceLocation:performing:throws:)`](https://developer.apple.com/documentation/testing/require(_:sourcelocation:performing:throws:)) +and modifying the other overloads so that, on success, they return the errors +that were thrown. + +## Detailed design + +All overloads of `#expect(throws:)` and `#require(throws:)` will be updated to +return an instance of the error type specified by their arguments, with the +problematic overloads returning `any Error` since more precise type information +is not statically available. The problematic overloads will also be deprecated: + +```diff +--- a/Sources/Testing/Expectations/Expectation+Macro.swift ++++ b/Sources/Testing/Expectations/Expectation+Macro.swift ++@discardableResult + @freestanding(expression) public macro expect( + throws errorType: E.Type, + _ comment: @autoclosure () -> Comment? = nil, + sourceLocation: SourceLocation = #_sourceLocation, + performing expression: () async throws -> R +-) ++) -> E? where E: Error + ++@discardableResult + @freestanding(expression) public macro require( + throws errorType: E.Type, + _ comment: @autoclosure () -> Comment? = nil, + sourceLocation: SourceLocation = #_sourceLocation, + performing expression: () async throws -> R +-) where E: Error ++) -> E where E: Error + ++@discardableResult + @freestanding(expression) public macro expect( + throws error: E, + _ comment: @autoclosure () -> Comment? = nil, + sourceLocation: SourceLocation = #_sourceLocation, + performing expression: () async throws -> R +-) where E: Error & Equatable ++) -> E? where E: Error & Equatable + ++@discardableResult + @freestanding(expression) public macro require( + throws error: E, + _ comment: @autoclosure () -> Comment? = nil, + sourceLocation: SourceLocation = #_sourceLocation, + performing expression: () async throws -> R +-) where E: Error & Equatable ++) -> E where E: Error & Equatable + ++@available(*, deprecated, message: "Examine the result of '#expect(throws:)' instead.") ++@discardableResult + @freestanding(expression) public macro expect( + _ comment: @autoclosure () -> Comment? = nil, + sourceLocation: SourceLocation = #_sourceLocation, + performing expression: () async throws -> R, + throws errorMatcher: (any Error) async throws -> Bool +-) ++) -> (any Error)? + ++@available(*, deprecated, message: "Examine the result of '#require(throws:)' instead.") ++@discardableResult + @freestanding(expression) public macro require( + _ comment: @autoclosure () -> Comment? = nil, + sourceLocation: SourceLocation = #_sourceLocation, + performing expression: () async throws -> R, + throws errorMatcher: (any Error) async throws -> Bool +-) ++) -> any Error +``` + +(More detailed information about the deprecations will be provided via DocC.) + +The `#expect(throws:)` overloads return an optional value that is `nil` if the +expectation failed, while the `#require(throws:)` overloads return non-optional +values and throw instances of `ExpectationFailedError` on failure (as before.) + +> [!NOTE] +> Instances of `ExpectationFailedError` thrown by `#require(throws:)` on failure +> are not returned as that would defeat the purpose of using `#require(throws:)` +> instead of `#expect(throws:)`. + +Test authors will be able to use the result of the above functions to verify +that the thrown error is correct: + +```swift +let error = try #require(throws: PotatoError.self) { + let potato = try Sack.randomPotato() + try potato.turnIntoFrenchFries() +} +#expect(error == .potatoNotPeeled) +#expect(error.variety != .russet) +``` + +The new code is more concise than the old code and avoids boilerplate casting +from `any Error`. + +## Source compatibility + +In most cases, this change does not affect source compatibility. Swift does not +allow forming references to macros at runtime, so we don't need to worry about +type mismatches assigning one to some local variable. + +We have identified two scenarios where a new warning will be emitted. + +### Inferred return type from macro invocation + +The return type of the macro may be used by the compiler to infer the return +type of an enclosing closure. If the return value is then discarded, the +compiler may emit a warning: + +```swift +func pokePotato(_ pPotato: UnsafePointer) throws { ... } + +let potato = Potato() +try await Task.sleep(for: .months(3)) +withUnsafePointer(to: potato) { pPotato in + // ^ ^ ^ ⚠️ Result of call to 'withUnsafePointer(to:_:)' is unused + #expect(throws: PotatoError.rotten) { + try pokePotato(pPotato) + } +} +``` + +This warning can be suppressed by assigning the result of the macro invocation +or the result of the function call to `_`: + +```swift +withUnsafePointer(to: potato) { pPotato in + _ = #expect(throws: PotatoError.rotten) { + try pokePotato(pPotato) + } +} +``` + +### Use of `#require(throws:)` in a generic context with `Never.self` + +If `#require(throws:)` (but not `#expect(throws:)`) is used in a generic context +where the type of thrown error is a generic parameter, and the type is resolved +to `Never`, there is no valid value for the invocation to return: + +```swift +func wrapper(throws type: E.Type, _ body: () throws -> Void) throws -> E { + return try #require(throws: type) { + try body() + } +} +let error = try #require(throws: Never.self) { ... } +``` + +We don't think this particular pattern is common (and outside of our own test +target, I'd be surprised if anybody's attempted it yet.) However, we do need to +handle it gracefully. If this pattern is encountered, Swift Testing will record +an "API Misused" issue for the current test and advise the test author to switch +to `#expect(throws:)` or to not pass `Never.self` here. + +## Integration with supporting tools + +N/A + +## Future directions + +- Adopting [typed throws](https://github.com/swiftlang/swift-evolution/blob/main/proposals/0413-typed-throws.md) + to statically require that the error thrown from test code is of the correct + type. + + If we adopted typed throws in the signatures of these macros, it would force + adoption of typed throws in the code under test even when it may not be + appropriate. For example, if we adopted typed throws, the following code would + not compile: + + ```swift + func cook(_ food: consuming some Food) throws { ... } + + let error: PotatoError? = #expect(throws: PotatoError.self) { + var potato = Potato() + potato.fossilize() + try cook(potato) // 🛑 ERROR: Invalid conversion of thrown error type + // 'any Error' to 'PotatoError' + } + ``` + + We believe it may be possible to overload these macros or their expansions so + that the code sample above _does_ compile and behave as intended. We intend to + experiment further with this idea and potentially revisit typed throws support + in a future proposal. + +## Alternatives considered + +- Leaving the existing implementation and signatures in place. We've had + sufficient feedback about the ergonomics of this API that we want to address + the problem. + +- Having the return type of the macros be `any Error` and returning _any_ error + that was thrown even on mismatch. This would make the ergonomics of the + subsequent test code less optimal because the test author would need to cast + the error to the appropriate type before inspecting it. + + There's a philosophical argument to be made here that if a mismatched error is + thrown, then the test has already failed and is in an inconsistent state, so + we should allow the test to fail rather than return what amounts to "bad + output". + + If the test author wants to inspect any arbitrary thrown error, they can + specify `(any Error).self` instead of a concrete error type. + +## Acknowledgments + +Thanks to the team and to [@jakepetroules](https://github.com/jakepetroules) for +starting the discussion that ultimately led to this proposal. diff --git a/Sources/Testing/Expectations/Expectation+Macro.swift b/Sources/Testing/Expectations/Expectation+Macro.swift index 5012c93ca..c8a691e85 100644 --- a/Sources/Testing/Expectations/Expectation+Macro.swift +++ b/Sources/Testing/Expectations/Expectation+Macro.swift @@ -142,6 +142,9 @@ public macro require( /// issues should be attributed. /// - expression: The expression to be evaluated. /// +/// - Returns: If the expectation passes, the instance of `errorType` that was +/// thrown by `expression`. If the expectation fails, the result is `nil`. +/// /// Use this overload of `#expect()` when the expression `expression` _should_ /// throw an error of a given type: /// @@ -158,7 +161,7 @@ public macro require( /// discarded. /// /// If the thrown error need only equal another instance of [`Error`](https://developer.apple.com/documentation/swift/error), -/// use ``expect(throws:_:sourceLocation:performing:)-1xr34`` instead. +/// use ``expect(throws:_:sourceLocation:performing:)-7du1h`` instead. /// /// ## Expressions that should never throw /// @@ -181,12 +184,13 @@ public macro require( /// fail when an error is thrown by `expression`, rather than to explicitly /// check that an error is _not_ thrown by it, do not use this macro. Instead, /// simply call the code in question and allow it to throw an error naturally. +@discardableResult @freestanding(expression) public macro expect( throws errorType: E.Type, _ comment: @autoclosure () -> Comment? = nil, sourceLocation: SourceLocation = #_sourceLocation, performing expression: () async throws -> R -) = #externalMacro(module: "TestingMacros", type: "ExpectMacro") where E: Error +) -> E? = #externalMacro(module: "TestingMacros", type: "ExpectMacro") where E: Error /// Check that an expression always throws an error of a given type, and throw /// an error if it does not. @@ -200,6 +204,8 @@ public macro require( /// issues should be attributed. /// - expression: The expression to be evaluated. /// +/// - Returns: The instance of `errorType` that was thrown by `expression`. +/// /// - Throws: An instance of ``ExpectationFailedError`` if `expression` does not /// throw a matching error. The error thrown by `expression` is not rethrown. /// @@ -219,16 +225,17 @@ public macro require( /// is thrown. Any value returned by `expression` is discarded. /// /// If the thrown error need only equal another instance of [`Error`](https://developer.apple.com/documentation/swift/error), -/// use ``require(throws:_:sourceLocation:performing:)-7v83e`` instead. +/// use ``require(throws:_:sourceLocation:performing:)-4djuw`` instead. /// /// If `expression` should _never_ throw, simply invoke the code without using /// this macro. The test will then fail if an error is thrown. +@discardableResult @freestanding(expression) public macro require( throws errorType: E.Type, _ comment: @autoclosure () -> Comment? = nil, sourceLocation: SourceLocation = #_sourceLocation, performing expression: () async throws -> R -) = #externalMacro(module: "TestingMacros", type: "RequireMacro") where E: Error +) -> E = #externalMacro(module: "TestingMacros", type: "RequireThrowsMacro") where E: Error /// Check that an expression never throws an error, and throw an error if it /// does. @@ -261,6 +268,10 @@ public macro require( /// issues should be attributed. /// - expression: The expression to be evaluated. /// +/// - Returns: If the expectation passes, the instance of `E` that was thrown by +/// `expression` and is equal to `error`. If the expectation fails, the result +/// is `nil`. +/// /// Use this overload of `#expect()` when the expression `expression` _should_ /// throw a specific error: /// @@ -276,13 +287,14 @@ public macro require( /// in the current task. Any value returned by `expression` is discarded. /// /// If the thrown error need only be an instance of a particular type, use -/// ``expect(throws:_:sourceLocation:performing:)-79piu`` instead. +/// ``expect(throws:_:sourceLocation:performing:)-1hfms`` instead. +@discardableResult @freestanding(expression) public macro expect( throws error: E, _ comment: @autoclosure () -> Comment? = nil, sourceLocation: SourceLocation = #_sourceLocation, performing expression: () async throws -> R -) = #externalMacro(module: "TestingMacros", type: "ExpectMacro") where E: Error & Equatable +) -> E? = #externalMacro(module: "TestingMacros", type: "ExpectMacro") where E: Error & Equatable /// Check that an expression always throws a specific error, and throw an error /// if it does not. @@ -293,6 +305,9 @@ public macro require( /// - sourceLocation: The source location to which recorded expectations and /// issues should be attributed. /// - expression: The expression to be evaluated. + +/// - Returns: The instance of `E` that was thrown by `expression` and is equal +/// to `error`. /// /// - Throws: An instance of ``ExpectationFailedError`` if `expression` does not /// throw a matching error. The error thrown by `expression` is not rethrown. @@ -313,13 +328,14 @@ public macro require( /// Any value returned by `expression` is discarded. /// /// If the thrown error need only be an instance of a particular type, use -/// ``require(throws:_:sourceLocation:performing:)-76bjn`` instead. +/// ``require(throws:_:sourceLocation:performing:)-7n34r`` instead. +@discardableResult @freestanding(expression) public macro require( throws error: E, _ comment: @autoclosure () -> Comment? = nil, sourceLocation: SourceLocation = #_sourceLocation, performing expression: () async throws -> R -) = #externalMacro(module: "TestingMacros", type: "RequireMacro") where E: Error & Equatable +) -> E = #externalMacro(module: "TestingMacros", type: "RequireMacro") where E: Error & Equatable // MARK: - Arbitrary error matching @@ -333,6 +349,9 @@ public macro require( /// - errorMatcher: A closure to invoke when `expression` throws an error that /// indicates if it matched or not. /// +/// - Returns: If the expectation passes, the error that was thrown by +/// `expression`. If the expectation fails, the result is `nil`. +/// /// Use this overload of `#expect()` when the expression `expression` _should_ /// throw an error, but the logic to determine if the error matches is complex: /// @@ -353,15 +372,17 @@ public macro require( /// discarded. /// /// If the thrown error need only be an instance of a particular type, use -/// ``expect(throws:_:sourceLocation:performing:)-79piu`` instead. If the thrown +/// ``expect(throws:_:sourceLocation:performing:)-1hfms`` instead. If the thrown /// error need only equal another instance of [`Error`](https://developer.apple.com/documentation/swift/error), -/// use ``expect(throws:_:sourceLocation:performing:)-1xr34`` instead. +/// use ``expect(throws:_:sourceLocation:performing:)-7du1h`` instead. +@available(*, deprecated, message: "Examine the result of '#expect(throws:)' instead.") +@discardableResult @freestanding(expression) public macro expect( _ comment: @autoclosure () -> Comment? = nil, sourceLocation: SourceLocation = #_sourceLocation, performing expression: () async throws -> R, throws errorMatcher: (any Error) async throws -> Bool -) = #externalMacro(module: "TestingMacros", type: "ExpectMacro") +) -> (any Error)? = #externalMacro(module: "TestingMacros", type: "ExpectMacro") /// Check that an expression always throws an error matching some condition, and /// throw an error if it does not. @@ -374,6 +395,8 @@ public macro require( /// - errorMatcher: A closure to invoke when `expression` throws an error that /// indicates if it matched or not. /// +/// - Returns: The error that was thrown by `expression`. +/// /// - Throws: An instance of ``ExpectationFailedError`` if `expression` does not /// throw a matching error. The error thrown by `expression` is not rethrown. /// @@ -398,18 +421,20 @@ public macro require( /// discarded. /// /// If the thrown error need only be an instance of a particular type, use -/// ``require(throws:_:sourceLocation:performing:)-76bjn`` instead. If the thrown error need +/// ``require(throws:_:sourceLocation:performing:)-7n34r`` instead. If the thrown error need /// only equal another instance of [`Error`](https://developer.apple.com/documentation/swift/error), -/// use ``require(throws:_:sourceLocation:performing:)-7v83e`` instead. +/// use ``require(throws:_:sourceLocation:performing:)-4djuw`` instead. /// /// If `expression` should _never_ throw, simply invoke the code without using /// this macro. The test will then fail if an error is thrown. +@available(*, deprecated, message: "Examine the result of '#require(throws:)' instead.") +@discardableResult @freestanding(expression) public macro require( _ comment: @autoclosure () -> Comment? = nil, sourceLocation: SourceLocation = #_sourceLocation, performing expression: () async throws -> R, throws errorMatcher: (any Error) async throws -> Bool -) = #externalMacro(module: "TestingMacros", type: "RequireMacro") +) -> any Error = #externalMacro(module: "TestingMacros", type: "RequireMacro") // MARK: - Exit tests @@ -425,7 +450,7 @@ public macro require( /// issues should be attributed. /// - expression: The expression to be evaluated. /// -/// - Returns: If the exit test passed, an instance of ``ExitTestArtifacts`` +/// - Returns: If the exit test passes, an instance of ``ExitTestArtifacts`` /// describing the state of the exit test when it exited. If the exit test /// fails, the result is `nil`. /// diff --git a/Sources/Testing/Expectations/ExpectationChecking+Macro.swift b/Sources/Testing/Expectations/ExpectationChecking+Macro.swift index eff01e5bf..ca452e2f8 100644 --- a/Sources/Testing/Expectations/ExpectationChecking+Macro.swift +++ b/Sources/Testing/Expectations/ExpectationChecking+Macro.swift @@ -824,7 +824,8 @@ public func __checkCast( /// Check that an expression always throws an error. /// /// This overload is used for `#expect(throws:) { }` invocations that take error -/// types. +/// types. It is disfavored so that `#expect(throws: Never.self)` preferentially +/// returns `Void`. /// /// - Warning: This function is used to implement the `#expect()` and /// `#require()` macros. Do not call it directly. @@ -835,7 +836,7 @@ public func __checkClosureCall( comments: @autoclosure () -> [Comment], isRequired: Bool, sourceLocation: SourceLocation -) -> Result where E: Error { +) -> Result where E: Error { if errorType == Never.self { __checkClosureCall( throws: Never.self, @@ -844,7 +845,7 @@ public func __checkClosureCall( comments: comments(), isRequired: isRequired, sourceLocation: sourceLocation - ) + ).map { _ in nil } } else { __checkClosureCall( performing: body, @@ -854,14 +855,15 @@ public func __checkClosureCall( comments: comments(), isRequired: isRequired, sourceLocation: sourceLocation - ) + ).map { $0 as? E } } } /// Check that an expression always throws an error. /// /// This overload is used for `await #expect(throws:) { }` invocations that take -/// error types. +/// error types. It is disfavored so that `#expect(throws: Never.self)` +/// preferentially returns `Void`. /// /// - Warning: This function is used to implement the `#expect()` and /// `#require()` macros. Do not call it directly. @@ -873,7 +875,7 @@ public func __checkClosureCall( isRequired: Bool, isolation: isolated (any Actor)? = #isolation, sourceLocation: SourceLocation -) async -> Result where E: Error { +) async -> Result where E: Error { if errorType == Never.self { await __checkClosureCall( throws: Never.self, @@ -883,7 +885,7 @@ public func __checkClosureCall( isRequired: isRequired, isolation: isolation, sourceLocation: sourceLocation - ) + ).map { _ in nil } } else { await __checkClosureCall( performing: body, @@ -894,7 +896,7 @@ public func __checkClosureCall( isRequired: isRequired, isolation: isolation, sourceLocation: sourceLocation - ) + ).map { $0 as? E } } } @@ -932,7 +934,7 @@ public func __checkClosureCall( comments: comments(), isRequired: isRequired, sourceLocation: sourceLocation - ) + ).map { _ in } } /// Check that an expression never throws an error. @@ -969,7 +971,7 @@ public func __checkClosureCall( comments: comments(), isRequired: isRequired, sourceLocation: sourceLocation - ) + ).map { _ in } } // MARK: - Matching instances of equatable errors @@ -988,7 +990,7 @@ public func __checkClosureCall( comments: @autoclosure () -> [Comment], isRequired: Bool, sourceLocation: SourceLocation -) -> Result where E: Error & Equatable { +) -> Result where E: Error & Equatable { __checkClosureCall( performing: body, throws: { true == (($0 as? E) == error) }, @@ -997,7 +999,7 @@ public func __checkClosureCall( comments: comments(), isRequired: isRequired, sourceLocation: sourceLocation - ) + ).map { $0 as? E } } /// Check that an expression always throws an error. @@ -1015,7 +1017,7 @@ public func __checkClosureCall( isRequired: Bool, isolation: isolated (any Actor)? = #isolation, sourceLocation: SourceLocation -) async -> Result where E: Error & Equatable { +) async -> Result where E: Error & Equatable { await __checkClosureCall( performing: body, throws: { true == (($0 as? E) == error) }, @@ -1025,7 +1027,7 @@ public func __checkClosureCall( isRequired: isRequired, isolation: isolation, sourceLocation: sourceLocation - ) + ).map { $0 as? E } } // MARK: - Arbitrary error matching @@ -1044,10 +1046,11 @@ public func __checkClosureCall( comments: @autoclosure () -> [Comment], isRequired: Bool, sourceLocation: SourceLocation -) -> Result { +) -> Result<(any Error)?, any Error> { var errorMatches = false var mismatchExplanationValue: String? = nil var expression = expression + var caughtError: (any Error)? do { let result = try body() @@ -1057,6 +1060,7 @@ public func __checkClosureCall( } mismatchExplanationValue = explanation } catch { + caughtError = error expression = expression.capturingRuntimeValues(error) let secondError = Issue.withErrorRecording(at: sourceLocation) { errorMatches = try errorMatcher(error) @@ -1075,7 +1079,7 @@ public func __checkClosureCall( comments: comments(), isRequired: isRequired, sourceLocation: sourceLocation - ) + ).map { caughtError } } /// Check that an expression always throws an error. @@ -1093,10 +1097,11 @@ public func __checkClosureCall( isRequired: Bool, isolation: isolated (any Actor)? = #isolation, sourceLocation: SourceLocation -) async -> Result { +) async -> Result<(any Error)?, any Error> { var errorMatches = false var mismatchExplanationValue: String? = nil var expression = expression + var caughtError: (any Error)? do { let result = try await body() @@ -1106,6 +1111,7 @@ public func __checkClosureCall( } mismatchExplanationValue = explanation } catch { + caughtError = error expression = expression.capturingRuntimeValues(error) let secondError = await Issue.withErrorRecording(at: sourceLocation) { errorMatches = try await errorMatcher(error) @@ -1124,7 +1130,7 @@ public func __checkClosureCall( comments: comments(), isRequired: isRequired, sourceLocation: sourceLocation - ) + ).map { caughtError } } // MARK: - Exit tests diff --git a/Sources/Testing/Issues/Issue+Recording.swift b/Sources/Testing/Issues/Issue+Recording.swift index e13099eaf..8a80e4467 100644 --- a/Sources/Testing/Issues/Issue+Recording.swift +++ b/Sources/Testing/Issues/Issue+Recording.swift @@ -29,11 +29,19 @@ extension Issue { func record(configuration: Configuration? = nil) -> Self { // If this issue is a caught error of kind SystemError, reinterpret it as a // testing system issue instead (per the documentation for SystemError.) - if case let .errorCaught(error) = kind, let error = error as? SystemError { - var selfCopy = self - selfCopy.kind = .system - selfCopy.comments.append(Comment(rawValue: String(describingForTest: error))) - return selfCopy.record(configuration: configuration) + if case let .errorCaught(error) = kind { + // TODO: consider factoring this logic out into a protocol + if let error = error as? SystemError { + var selfCopy = self + selfCopy.kind = .system + selfCopy.comments.append(Comment(rawValue: String(describingForTest: error))) + return selfCopy.record(configuration: configuration) + } else if let error = error as? APIMisuseError { + var selfCopy = self + selfCopy.kind = .apiMisused + selfCopy.comments.append(Comment(rawValue: String(describingForTest: error))) + return selfCopy.record(configuration: configuration) + } } // If this issue matches via the known issue matcher, set a copy of it to be diff --git a/Sources/Testing/Support/Additions/ResultAdditions.swift b/Sources/Testing/Support/Additions/ResultAdditions.swift index f14f68c85..9a2e6ea5a 100644 --- a/Sources/Testing/Support/Additions/ResultAdditions.swift +++ b/Sources/Testing/Support/Additions/ResultAdditions.swift @@ -31,16 +31,27 @@ extension Result { /// /// - Warning: This function is used to implement the `#expect()` and /// `#require()` macros. Do not call it directly. - @inlinable public func __expected() -> Success where Success == T? { + @discardableResult @inlinable public func __expected() -> Success where Success == T? { try? get() } /// Handle this instance as if it were returned from a call to `#require()`. /// + /// This overload of `__require()` assumes that the result cannot actually be + /// `nil` on success. The optionality is part of our ABI contract for the + /// `__check()` function family so that we can support uninhabited types and + /// "soft" failures. + /// + /// If the value really is `nil` (e.g. we're dealing with `Never`), the + /// testing library throws an error representing an issue of kind + /// ``Issue/Kind-swift.enum/apiMisused``. + /// /// - Warning: This function is used to implement the `#expect()` and /// `#require()` macros. Do not call it directly. - @inlinable public func __required() throws -> T where Success == T? { - // TODO: handle edge case where the value is nil (see #780) - try get()! + @discardableResult public func __required() throws -> T where Success == T? { + guard let result = try get() else { + throw APIMisuseError(description: "Could not unwrap 'nil' value of type Optional<\(T.self)>. Consider using #expect() instead of #require() here.") + } + return result } } diff --git a/Sources/Testing/Support/SystemError.swift b/Sources/Testing/Support/SystemError.swift index d68b9c241..d2d4809e3 100644 --- a/Sources/Testing/Support/SystemError.swift +++ b/Sources/Testing/Support/SystemError.swift @@ -21,3 +21,16 @@ struct SystemError: Error, CustomStringConvertible { var description: String } + +/// A type representing misuse of testing library API. +/// +/// When an error of this type is thrown and caught by the testing library, it +/// is recorded as an issue of kind ``Issue/Kind/apiMisused`` rather than +/// ``Issue/Kind/errorCaught(_:)``. +/// +/// This type is not part of the public interface of the testing library. +/// External callers should generally record issues by throwing their own errors +/// or by calling ``Issue/record(_:sourceLocation:)``. +struct APIMisuseError: Error, CustomStringConvertible { + var description: String +} diff --git a/Sources/Testing/Testing.docc/AvailabilityStubs/ExpectComplexThrows.md b/Sources/Testing/Testing.docc/AvailabilityStubs/ExpectComplexThrows.md new file mode 100644 index 000000000..755bf5089 --- /dev/null +++ b/Sources/Testing/Testing.docc/AvailabilityStubs/ExpectComplexThrows.md @@ -0,0 +1,28 @@ +# ``expect(_:sourceLocation:performing:throws:)`` + + + +@Metadata { + @Available(Swift, introduced: 6.0, deprecated: 999.0) + @Available(Xcode, introduced: 16.0, deprecated: 999.0) +} + +@DeprecationSummary { + Examine the result of ``expect(throws:_:sourceLocation:performing:)-7du1h`` or + ``expect(throws:_:sourceLocation:performing:)-1hfms`` instead: + + ```swift + let error = #expect(throws: FoodTruckError.self) { + ... + } + #expect(error?.napkinCount == 0) + ``` +} diff --git a/Sources/Testing/Testing.docc/AvailabilityStubs/RequireComplexThrows.md b/Sources/Testing/Testing.docc/AvailabilityStubs/RequireComplexThrows.md new file mode 100644 index 000000000..ff42dc40f --- /dev/null +++ b/Sources/Testing/Testing.docc/AvailabilityStubs/RequireComplexThrows.md @@ -0,0 +1,28 @@ +# ``require(_:sourceLocation:performing:throws:)`` + + + +@Metadata { + @Available(Swift, introduced: 6.0, deprecated: 999.0) + @Available(Xcode, introduced: 16.0, deprecated: 999.0) +} + +@DeprecationSummary { + Examine the result of ``require(throws:_:sourceLocation:performing:)-7n34r`` + or ``require(throws:_:sourceLocation:performing:)-4djuw`` instead: + + ```swift + let error = try #require(throws: FoodTruckError.self) { + ... + } + #expect(error.napkinCount == 0) + ``` +} diff --git a/Sources/Testing/Testing.docc/Expectations.md b/Sources/Testing/Testing.docc/Expectations.md index ce92824c1..fd3b0070d 100644 --- a/Sources/Testing/Testing.docc/Expectations.md +++ b/Sources/Testing/Testing.docc/Expectations.md @@ -65,11 +65,11 @@ the test when the code doesn't satisfy a requirement, use ### Checking that errors are thrown - -- ``expect(throws:_:sourceLocation:performing:)-79piu`` -- ``expect(throws:_:sourceLocation:performing:)-1xr34`` +- ``expect(throws:_:sourceLocation:performing:)-1hfms`` +- ``expect(throws:_:sourceLocation:performing:)-7du1h`` - ``expect(_:sourceLocation:performing:throws:)`` -- ``require(throws:_:sourceLocation:performing:)-76bjn`` -- ``require(throws:_:sourceLocation:performing:)-7v83e`` +- ``require(throws:_:sourceLocation:performing:)-7n34r`` +- ``require(throws:_:sourceLocation:performing:)-4djuw`` - ``require(_:sourceLocation:performing:throws:)`` ### Confirming that asynchronous events occur diff --git a/Sources/Testing/Testing.docc/MigratingFromXCTest.md b/Sources/Testing/Testing.docc/MigratingFromXCTest.md index bf0b43e34..133daa49c 100644 --- a/Sources/Testing/Testing.docc/MigratingFromXCTest.md +++ b/Sources/Testing/Testing.docc/MigratingFromXCTest.md @@ -326,7 +326,7 @@ their equivalents in the testing library: | `XCTAssertLessThanOrEqual(x, y)` | `#expect(x <= y)` | | `XCTAssertLessThan(x, y)` | `#expect(x < y)` | | `XCTAssertThrowsError(try f())` | `#expect(throws: (any Error).self) { try f() }` | -| `XCTAssertThrowsError(try f()) { error in … }` | `#expect { try f() } throws: { error in return … }` | +| `XCTAssertThrowsError(try f()) { error in … }` | `let error = #expect(throws: (any Error).self) { try f() }` | | `XCTAssertNoThrow(try f())` | `#expect(throws: Never.self) { try f() }` | | `try XCTUnwrap(x)` | `try #require(x)` | | `XCTFail("…")` | `Issue.record("…")` | diff --git a/Sources/Testing/Testing.docc/testing-for-errors-in-swift-code.md b/Sources/Testing/Testing.docc/testing-for-errors-in-swift-code.md index 5113202d0..c25a650fe 100644 --- a/Sources/Testing/Testing.docc/testing-for-errors-in-swift-code.md +++ b/Sources/Testing/Testing.docc/testing-for-errors-in-swift-code.md @@ -26,7 +26,7 @@ If the code throws an error, then your test fails. To check that the code under test throws a specific error, or to continue a longer test function after the code throws an error, pass that error as the -first argument of ``expect(throws:_:sourceLocation:performing:)-1xr34``, and +first argument of ``expect(throws:_:sourceLocation:performing:)-7du1h``, and pass a closure that calls the code under test: ```swift @@ -65,4 +65,23 @@ the error to `Never`: If the closure throws _any_ error, the testing library records an issue. If you need the test to stop when the code throws an error, include the code inline in the test function instead of wrapping it in a call to -``expect(throws:_:sourceLocation:performing:)-1xr34``. +``expect(throws:_:sourceLocation:performing:)-7du1h``. + +## Inspect an error thrown by your code + +When you use `#expect(throws:)` or `#require(throws:)` and the error matches the +expectation, it is returned to the caller so that you can perform additional +validation. If the expectation fails because no error was thrown or an error of +a different type was thrown, `#expect(throws:)` returns `nil`: + +```swift +@Test func cannotAddMarshmallowsToPizza() throws { + let error = #expect(throws: PizzaToppings.InvalidToppingError.self) { + try Pizza.current.add(topping: .marshmallows) + } + #expect(error?.topping == .marshmallows) + #expect(error?.reason == .dessertToppingOnly) +} +``` + +If you aren't sure what type of error will be thrown, pass `(any Error).self`. diff --git a/Sources/TestingMacros/ConditionMacro.swift b/Sources/TestingMacros/ConditionMacro.swift index 341b27d7d..3d2013c69 100644 --- a/Sources/TestingMacros/ConditionMacro.swift +++ b/Sources/TestingMacros/ConditionMacro.swift @@ -332,6 +332,35 @@ public struct NonOptionalRequireMacro: RefinedConditionMacro { } } +/// A type describing the expansion of the `#require(throws:)` macro. +/// +/// This macro makes a best effort to check if the type argument is `Never.self` +/// (as we only have the syntax tree here) and diagnoses it as redundant if so. +/// See also ``RequireThrowsNeverMacro`` which is used when full type checking +/// is contextually available. +/// +/// This type is otherwise exactly equivalent to ``RequireMacro``. +public struct RequireThrowsMacro: RefinedConditionMacro { + public typealias Base = RequireMacro + + public static func expansion( + of macro: some FreestandingMacroExpansionSyntax, + in context: some MacroExpansionContext + ) throws -> ExprSyntax { + if let argument = macro.arguments.first { + let argumentTokens: [String] = argument.expression.tokens(viewMode: .fixedUp).lazy + .filter { $0.tokenKind != .period } + .map(\.textWithoutBackticks) + if argumentTokens == ["Swift", "Never", "self"] || argumentTokens == ["Never", "self"] { + context.diagnose(.requireThrowsNeverIsRedundant(argument.expression, in: macro)) + } + } + + // Perform the normal macro expansion for #require(). + return try RequireMacro.expansion(of: macro, in: context) + } +} + /// A type describing the expansion of the `#require(throws:)` macro when it is /// passed `Never.self`, which is redundant. /// diff --git a/Sources/TestingMacros/Support/Additions/MacroExpansionContextAdditions.swift b/Sources/TestingMacros/Support/Additions/MacroExpansionContextAdditions.swift index 4539ed04d..ca0137b5d 100644 --- a/Sources/TestingMacros/Support/Additions/MacroExpansionContextAdditions.swift +++ b/Sources/TestingMacros/Support/Additions/MacroExpansionContextAdditions.swift @@ -80,6 +80,34 @@ extension MacroExpansionContext { // MARK: - extension MacroExpansionContext { + /// Whether or not our generated warnings are suppressed in the current + /// lexical context. + /// + /// The value of this property is `true` if the current lexical context + /// contains a node with the `@_semantics("testing.macros.nowarnings")` + /// attribute applied to it. + /// + /// - Warning: This functionality is not part of the public interface of the + /// testing library. It may be modified or removed in a future update. + var areWarningsSuppressed: Bool { +#if DEBUG + for lexicalContext in self.lexicalContext { + guard let lexicalContext = lexicalContext.asProtocol((any WithAttributesSyntax).self) else { + continue + } + for attribute in lexicalContext.attributes { + if case let .attribute(attribute) = attribute, + attribute.attributeNameText == "_semantics", + case let .string(argument) = attribute.arguments, + argument.representedLiteralValue == "testing.macros.nowarnings" { + return true + } + } + } +#endif + return false + } + /// Emit a diagnostic message. /// /// - Parameters: @@ -87,23 +115,27 @@ extension MacroExpansionContext { /// arguments to `Diagnostic.init()` are derived from the message's /// `syntax` property. func diagnose(_ message: DiagnosticMessage) { - diagnose( - Diagnostic( - node: message.syntax, - position: message.syntax.positionAfterSkippingLeadingTrivia, - message: message, - fixIts: message.fixIts - ) - ) + diagnose(CollectionOfOne(message)) } /// Emit a sequence of diagnostic messages. /// /// - Parameters: /// - messages: The diagnostic messages to emit. - func diagnose(_ messages: some Sequence) { + func diagnose(_ messages: some Collection) { + lazy var areWarningsSuppressed = areWarningsSuppressed for message in messages { - diagnose(message) + if message.severity == .warning && areWarningsSuppressed { + continue + } + diagnose( + Diagnostic( + node: message.syntax, + position: message.syntax.positionAfterSkippingLeadingTrivia, + message: message, + fixIts: message.fixIts + ) + ) } } diff --git a/Sources/TestingMacros/TestingMacrosMain.swift b/Sources/TestingMacros/TestingMacrosMain.swift index ebc62d660..c6904a6e7 100644 --- a/Sources/TestingMacros/TestingMacrosMain.swift +++ b/Sources/TestingMacros/TestingMacrosMain.swift @@ -24,6 +24,7 @@ struct TestingMacrosMain: CompilerPlugin { RequireMacro.self, AmbiguousRequireMacro.self, NonOptionalRequireMacro.self, + RequireThrowsMacro.self, RequireThrowsNeverMacro.self, ExitTestExpectMacro.self, ExitTestRequireMacro.self, diff --git a/Tests/TestingMacrosTests/ConditionMacroTests.swift b/Tests/TestingMacrosTests/ConditionMacroTests.swift index 7ede6233c..9f1201367 100644 --- a/Tests/TestingMacrosTests/ConditionMacroTests.swift +++ b/Tests/TestingMacrosTests/ConditionMacroTests.swift @@ -354,6 +354,8 @@ struct ConditionMacroTests { @Test("#require(throws: Never.self) produces a diagnostic", arguments: [ + "#requireThrows(throws: Swift.Never.self)", + "#requireThrows(throws: Never.self)", "#requireThrowsNever(throws: Never.self)", ] ) diff --git a/Tests/TestingMacrosTests/TestSupport/Parse.swift b/Tests/TestingMacrosTests/TestSupport/Parse.swift index fcb0215bc..e6b36e3b2 100644 --- a/Tests/TestingMacrosTests/TestSupport/Parse.swift +++ b/Tests/TestingMacrosTests/TestSupport/Parse.swift @@ -23,6 +23,7 @@ fileprivate let allMacros: [String: any Macro.Type] = [ "require": RequireMacro.self, "requireAmbiguous": AmbiguousRequireMacro.self, // different name needed only for unit testing "requireNonOptional": NonOptionalRequireMacro.self, // different name needed only for unit testing + "requireThrows": RequireThrowsMacro.self, // different name needed only for unit testing "requireThrowsNever": RequireThrowsNeverMacro.self, // different name needed only for unit testing "expectExitTest": ExitTestRequireMacro.self, // different name needed only for unit testing "requireExitTest": ExitTestRequireMacro.self, // different name needed only for unit testing diff --git a/Tests/TestingTests/IssueTests.swift b/Tests/TestingTests/IssueTests.swift index 53fe92b84..631ff0c54 100644 --- a/Tests/TestingTests/IssueTests.swift +++ b/Tests/TestingTests/IssueTests.swift @@ -491,6 +491,7 @@ final class IssueTests: XCTestCase { }.run(configuration: .init()) } + @available(*, deprecated) func testErrorCheckingWithExpect() async throws { let expectationFailed = expectation(description: "Expectation failed") expectationFailed.isInverted = true @@ -539,6 +540,7 @@ final class IssueTests: XCTestCase { await fulfillment(of: [expectationFailed], timeout: 0.0) } + @available(*, deprecated) func testErrorCheckingWithExpect_Mismatching() async throws { let expectationFailed = expectation(description: "Expectation failed") expectationFailed.expectedFulfillmentCount = 13 @@ -663,6 +665,7 @@ final class IssueTests: XCTestCase { await fulfillment(of: [expectationFailed], timeout: 0.0) } + @available(*, deprecated) func testErrorCheckingWithExpectAsync() async throws { let expectationFailed = expectation(description: "Expectation failed") expectationFailed.isInverted = true @@ -706,6 +709,7 @@ final class IssueTests: XCTestCase { await fulfillment(of: [expectationFailed], timeout: 0.0) } + @available(*, deprecated) func testErrorCheckingWithExpectAsync_Mismatching() async throws { let expectationFailed = expectation(description: "Expectation failed") expectationFailed.expectedFulfillmentCount = 13 @@ -822,6 +826,7 @@ final class IssueTests: XCTestCase { await fulfillment(of: [expectationFailed], timeout: 0.0) } + @available(*, deprecated) func testErrorCheckingWithExpect_ThrowingFromErrorMatcher() async throws { let errorCaught = expectation(description: "Error matcher's error caught") let expectationFailed = expectation(description: "Expectation failed") @@ -849,6 +854,7 @@ final class IssueTests: XCTestCase { await fulfillment(of: [errorCaught, expectationFailed], timeout: 0.0) } + @available(*, deprecated) func testErrorCheckingWithExpectAsync_ThrowingFromErrorMatcher() async throws { let errorCaught = expectation(description: "Error matcher's error caught") let expectationFailed = expectation(description: "Expectation failed") @@ -876,6 +882,7 @@ final class IssueTests: XCTestCase { await fulfillment(of: [errorCaught, expectationFailed], timeout: 0.0) } + @available(*, deprecated) func testErrorCheckingWithRequire_ThrowingFromErrorMatcher() async throws { let errorCaught = expectation(description: "Error matcher's error caught") let expectationFailed = expectation(description: "Expectation failed") @@ -904,6 +911,7 @@ final class IssueTests: XCTestCase { await fulfillment(of: [errorCaught, expectationFailed], timeout: 0.0) } + @available(*, deprecated) func testErrorCheckingWithRequireAsync_ThrowingFromErrorMatcher() async throws { let errorCaught = expectation(description: "Error matcher's error caught") let expectationFailed = expectation(description: "Expectation failed") @@ -932,6 +940,77 @@ final class IssueTests: XCTestCase { await fulfillment(of: [errorCaught, expectationFailed], timeout: 0.0) } + func testErrorCheckingWithExpect_ResultValue() throws { + let error = #expect(throws: MyDescriptiveError.self) { + throw MyDescriptiveError(description: "abc123") + } + #expect(error?.description == "abc123") + } + + func testErrorCheckingWithRequire_ResultValue() async throws { + let error = try #require(throws: MyDescriptiveError.self) { + throw MyDescriptiveError(description: "abc123") + } + #expect(error.description == "abc123") + } + + func testErrorCheckingWithExpect_ResultValueIsNever() async throws { + let error: Never? = #expect(throws: Never.self) { + throw MyDescriptiveError(description: "abc123") + } + #expect(error == nil) + } + + func testErrorCheckingWithRequire_ResultValueIsNever() async throws { + let errorCaught = expectation(description: "Error caught") + errorCaught.isInverted = true + let apiMisused = expectation(description: "API misused") + let expectationFailed = expectation(description: "Expectation failed") + expectationFailed.isInverted = true + + var configuration = Configuration() + configuration.eventHandler = { event, _ in + guard case let .issueRecorded(issue) = event.kind else { + return + } + if case .errorCaught = issue.kind { + errorCaught.fulfill() + } else if case .apiMisused = issue.kind { + apiMisused.fulfill() + } else { + expectationFailed.fulfill() + } + } + + await Test { + func f(_ type: E.Type) throws -> E where E: Error { + try #require(throws: type) {} + } + try f(Never.self) + }.run(configuration: configuration) + + await fulfillment(of: [errorCaught, apiMisused, expectationFailed], timeout: 0.0) + } + + @_semantics("testing.macros.nowarnings") + func testErrorCheckingWithRequire_ResultValueIsNever_VariousSyntaxes() throws { + // Basic expressions succeed and don't diagnose. + #expect(throws: Never.self) {} + try #require(throws: Never.self) {} + + // Casting to specific types succeeds and doesn't diagnose. + let _: Void = try #require(throws: Never.self) {} + let _: Any = try #require(throws: Never.self) {} + + // Casting to any Error throws an API misuse error because Never cannot be + // instantiated. NOTE: inner function needed for lexical context. + @_semantics("testing.macros.nowarnings") + func castToAnyError() throws { + let _: any Error = try #require(throws: Never.self) {} + } + #expect(throws: APIMisuseError.self, performing: castToAnyError) + } + func testFail() async throws { var configuration = Configuration() configuration.eventHandler = { event, _ in diff --git a/Tests/TestingTests/Traits/TagListTests.swift b/Tests/TestingTests/Traits/TagListTests.swift index 29b8e3909..1ec8d1248 100644 --- a/Tests/TestingTests/Traits/TagListTests.swift +++ b/Tests/TestingTests/Traits/TagListTests.swift @@ -171,7 +171,7 @@ struct TagListTests { func noTagColorsReadFromBadPath(tagColorJSON: String) throws { var tagColorJSON = tagColorJSON tagColorJSON.withUTF8 { tagColorJSON in - #expect(throws: (any Error).self) { + _ = #expect(throws: (any Error).self) { _ = try JSON.decode(Tag.Color.self, from: .init(tagColorJSON)) } }