From 7ee5a351be3ca80db26200efe968782b8b334efa Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Fri, 4 Oct 2024 17:37:27 -0400 Subject: [PATCH 1/8] Simplify the configuration option for synchronous test isolation. This PR replaces `Configuration.isMainActorIsolationEnforced` with a new property `Configuration.defaultIsolationContext` which can be set to any actor, not just the main actor. This property is then used as the isolation context for synchronous test functions that are not otherwise actor-isolated. The typical use case for this property would be to set it to `MainActor.shared` so that synchronous test functions run on the main actor, but a testing tool might want to instead run them all on another actor or serial executor for performance (or other) reasons. This change isn't just speculative: it allows us to simplify the macro-generated thunk for synchronous test functions so that it does not need to emit the body of the thunk twice (once for the main actor, once for non-isolated.) This change wasn't possible before `#isolation` was introduced, but we'd already added `Configuration.isMainActorIsolationEnforced` when that happened so the timing didn't line up. By rewriting this part of the macro expansion, we're able to toss a bunch of support code that isn't needed anymore. Yay! --- Sources/Testing/Running/Configuration.swift | 31 ++++++-- Sources/Testing/Test+Macro.swift | 46 ++---------- .../TestingMacros/TestDeclarationMacro.swift | 75 ++++--------------- .../TestDeclarationMacroTests.swift | 2 - Tests/TestingTests/RunnerTests.swift | 14 ++++ 5 files changed, 59 insertions(+), 109 deletions(-) diff --git a/Sources/Testing/Running/Configuration.swift b/Sources/Testing/Running/Configuration.swift index c359d4ee0..5702c7f93 100644 --- a/Sources/Testing/Running/Configuration.swift +++ b/Sources/Testing/Running/Configuration.swift @@ -105,14 +105,13 @@ public struct Configuration: Sendable { /// By default, the value of this property allows for a single iteration. public var repetitionPolicy: RepetitionPolicy = .once - // MARK: - Main actor isolation + // MARK: - Isolation context for synchronous tests -#if !SWT_NO_GLOBAL_ACTORS - /// Whether or not synchronous test functions need to run on the main actor. + /// The isolation context to use for synchronous test functions. /// - /// This property is available on platforms where UI testing is implemented. - public var isMainActorIsolationEnforced = false -#endif + /// If the value of this property is `nil`, synchronous test functions run in + /// an unspecified isolation context. + public var defaultIsolationContext: (any Actor)? = nil // MARK: - Time limits @@ -233,3 +232,23 @@ public struct Configuration: Sendable { /// The test case filter to which test cases should be filtered when run. public var testCaseFilter: TestCaseFilter = { _, _ in true } } + +// MARK: - Deprecated + +extension Configuration { +#if !SWT_NO_GLOBAL_ACTORS + @available(*, deprecated, message: "Set defaultIsolationContext instead.") + public var isMainActorIsolationEnforced: Bool { + get { + defaultIsolationContext === MainActor.shared + } + set { + if newValue { + defaultIsolationContext = MainActor.shared + } else { + defaultIsolationContext = nil + } + } + } +#endif +} diff --git a/Sources/Testing/Test+Macro.swift b/Sources/Testing/Test+Macro.swift index 637256b16..e546c0afc 100644 --- a/Sources/Testing/Test+Macro.swift +++ b/Sources/Testing/Test+Macro.swift @@ -504,36 +504,7 @@ extension Test { value } -#if !SWT_NO_GLOBAL_ACTORS -/// Invoke a function isolated to the main actor if appropriate. -/// -/// - Parameters: -/// - thenBody: The function to invoke, isolated to the main actor, if actor -/// isolation is required. -/// - elseBody: The function to invoke if actor isolation is not required. -/// -/// - Returns: Whatever is returned by `thenBody` or `elseBody`. -/// -/// - Throws: Whatever is thrown by `thenBody` or `elseBody`. -/// -/// `thenBody` and `elseBody` should represent the same function with differing -/// actor isolation. Which one is invoked depends on whether or not synchronous -/// test functions need to run on the main actor. -/// -/// - Warning: This function is used to implement the `@Test` macro. Do not call -/// it directly. -public func __ifMainActorIsolationEnforced( - _ thenBody: @Sendable @MainActor () async throws -> R, - else elseBody: @Sendable () async throws -> R -) async throws -> R where R: Sendable { - if Configuration.current?.isMainActorIsolationEnforced == true { - try await thenBody() - } else { - try await elseBody() - } -} -#else -/// Invoke a function. +/// Invoke a function isolated to the default isolation context. /// /// - Parameters: /// - body: The function to invoke. @@ -542,20 +513,17 @@ public func __ifMainActorIsolationEnforced( /// /// - Throws: Whatever is thrown by `body`. /// -/// This function simply invokes `body`. Its signature matches that of the same -/// function when `SWT_NO_GLOBAL_ACTORS` is not defined so that it can be used -/// during expansion of the `@Test` macro without knowing the value of that -/// compiler conditional on the target platform. +/// This function invokes `body` isolated to the default isolation context as +/// specified when configuring the test run. /// /// - Warning: This function is used to implement the `@Test` macro. Do not call /// it directly. -@inlinable public func __ifMainActorIsolationEnforced( - _: @Sendable () async throws -> R, - else body: @Sendable () async throws -> R +public func __withDefaultIsolationContext( + isolation: isolated (any Actor)? = #isolation, + _ body: (isolated (any Actor)?) async throws -> R ) async throws -> R where R: Sendable { - try await body() + try await body(Configuration.current?.defaultIsolationContext ?? isolation) } -#endif /// Run a test function as an `XCTestCase`-compatible method. /// diff --git a/Sources/TestingMacros/TestDeclarationMacro.swift b/Sources/TestingMacros/TestDeclarationMacro.swift index f23369027..7365bc5d7 100644 --- a/Sources/TestingMacros/TestDeclarationMacro.swift +++ b/Sources/TestingMacros/TestDeclarationMacro.swift @@ -172,62 +172,6 @@ public struct TestDeclarationMacro: PeerMacro, Sendable { return FunctionParameterClauseSyntax(parameters: parameterList) } - /// Create a closure capture list used to capture the arguments to a function - /// when calling it from its corresponding thunk function. - /// - /// - Parameters: - /// - parametersWithLabels: A sequence of tuples containing parameters to - /// the original function and their corresponding identifiers as used by - /// the thunk function. - /// - /// - Returns: A closure capture list syntax node representing the arguments - /// to the thunk function. - /// - /// We need to construct a capture list when calling a synchronous test - /// function because of the call to `__ifMainActorIsolationEnforced(_:else:)` - /// that we insert. That function theoretically captures all arguments twice, - /// which is not allowed for arguments marked `borrowing` or `consuming`. The - /// capture list forces those arguments to be copied, side-stepping the issue. - /// - /// - Note: We do not support move-only types as arguments yet. Instances of - /// move-only types cannot be used with generics, so they cannot be elements - /// of a `Collection`. - private static func _createCaptureListExpr( - from parametersWithLabels: some Sequence<(DeclReferenceExprSyntax, FunctionParameterSyntax)> - ) -> ClosureCaptureClauseSyntax { - let specifierKeywordsNeedingCopy: [TokenKind] = [.keyword(.borrowing), .keyword(.consuming),] - let closureCaptures = parametersWithLabels.lazy.map { label, parameter in - var needsCopy = false - if let parameterType = parameter.type.as(AttributedTypeSyntax.self) { - needsCopy = parameterType.specifiers.contains { specifier in - guard case let .simpleTypeSpecifier(specifier) = specifier else { - return false - } - return specifierKeywordsNeedingCopy.contains(specifier.specifier.tokenKind) - } - } - - if needsCopy { - return ClosureCaptureSyntax( - name: label.baseName, - equal: .equalToken(), - expression: CopyExprSyntax( - copyKeyword: .keyword(.copy).with(\.trailingTrivia, .space), - expression: label - ) - ) - } else { - return ClosureCaptureSyntax(expression: label) - } - } - - return ClosureCaptureClauseSyntax { - for closureCapture in closureCaptures { - closureCapture - } - } - } - /// The `static` keyword, if `typeName` is not `nil`. /// /// - Parameters: @@ -269,7 +213,6 @@ public struct TestDeclarationMacro: PeerMacro, Sendable { // needed, so it's lazy. let forwardedParamsExpr = _createForwardedParamsExpr(from: parametersWithLabels) let thunkParamsExpr = _createThunkParamsExpr(from: parametersWithLabels) - lazy var captureListExpr = _createCaptureListExpr(from: parametersWithLabels) // How do we call a function if we don't know whether it's `async` or // `throws`? Yes, we know if the keywords are on the function, but it could @@ -348,13 +291,21 @@ public struct TestDeclarationMacro: PeerMacro, Sendable { // main actor, it may still need to run main-actor-isolated depending on the // runtime configuration in the test process. if functionDecl.signature.effectSpecifiers?.asyncSpecifier == nil && !isMainActorIsolated { - thunkBody = """ - try await Testing.__ifMainActorIsolationEnforced { \(captureListExpr) in - \(thunkBody) - } else: { \(captureListExpr) in + if functionDecl.signature.parameterClause.parameters.tokens(viewMode: .sourceAccurate) + .map(\.tokenKind) + .contains(.keyword(.borrowing)) { + // BUG: rdar://137308488 The compiler cannot tell that this thunk calls + // its closure only once, so it emits a diagnostic about consuming the + // borrowed parameter. Work around the diagnostic by not emitting the + // isolation context hop. It's unfortunate but should rarely be a + // problem since we don't support move-only arguments anyway (see #543.) + } else { + thunkBody = """ + try await Testing.__withDefaultIsolationContext { _ in \(thunkBody) + } + """ } - """ } // Add availability guards if needed. diff --git a/Tests/TestingMacrosTests/TestDeclarationMacroTests.swift b/Tests/TestingMacrosTests/TestDeclarationMacroTests.swift index fffa06664..fef2da83c 100644 --- a/Tests/TestingMacrosTests/TestDeclarationMacroTests.swift +++ b/Tests/TestingMacrosTests/TestDeclarationMacroTests.swift @@ -325,8 +325,6 @@ struct TestDeclarationMacroTests { ("@Test @_unavailableFromAsync @MainActor func f() {}", nil, "MainActor.run"), ("@Test @available(*, noasync) func f() {}", nil, "__requiringTry"), ("@Test @_unavailableFromAsync func f() {}", nil, "__requiringTry"), - ("@Test(arguments: []) func f(i: borrowing Int) {}", nil, "copy"), - ("@Test(arguments: []) func f(_ i: borrowing Int) {}", nil, "copy"), ("@Test(arguments: []) func f(f: () -> String) {}", "(() -> String).self", nil), ("struct S {\n\t@Test func testF() {} }", nil, "__invokeXCTestCaseMethod"), ("struct S {\n\t@Test func testF() throws {} }", nil, "__invokeXCTestCaseMethod"), diff --git a/Tests/TestingTests/RunnerTests.swift b/Tests/TestingTests/RunnerTests.swift index bbc88949c..571e40bc2 100644 --- a/Tests/TestingTests/RunnerTests.swift +++ b/Tests/TestingTests/RunnerTests.swift @@ -824,6 +824,7 @@ final class RunnerTests: XCTestCase { } } + @available(*, deprecated) func testSynchronousTestFunctionRunsOnMainActorWhenEnforced() async { var configuration = Configuration() configuration.isMainActorIsolationEnforced = true @@ -836,6 +837,19 @@ final class RunnerTests: XCTestCase { await runTest(for: MainActorIsolationTests.self, configuration: configuration) } } + + func testSynchronousTestFunctionRunsInDefaultIsolationContext() async { + var configuration = Configuration() + configuration.defaultIsolationContext = MainActor.shared + await Self.$isMainActorIsolationEnforced.withValue(true) { + await runTest(for: MainActorIsolationTests.self, configuration: configuration) + } + + configuration.defaultIsolationContext = nil + await Self.$isMainActorIsolationEnforced.withValue(false) { + await runTest(for: MainActorIsolationTests.self, configuration: configuration) + } + } #endif @Suite(.hidden) struct DeprecatedVersionTests { From 6e7dbee248000890359efa39d19c12c174fa6e90 Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Sat, 5 Oct 2024 14:48:02 -0400 Subject: [PATCH 2/8] Instead of `__withDefaultIsolationContext`, expose a public `__defaultIsolationContext` property and use a local closure that the compiler can see is only called once, solving the consuming/borrowing problem. --- Sources/Testing/Test+Macro.swift | 21 ++++------------- .../TestingMacros/TestDeclarationMacro.swift | 23 +++++-------------- 2 files changed, 10 insertions(+), 34 deletions(-) diff --git a/Sources/Testing/Test+Macro.swift b/Sources/Testing/Test+Macro.swift index e546c0afc..02d54babd 100644 --- a/Sources/Testing/Test+Macro.swift +++ b/Sources/Testing/Test+Macro.swift @@ -504,25 +504,12 @@ extension Test { value } -/// Invoke a function isolated to the default isolation context. +/// The current default isolation context. /// -/// - Parameters: -/// - body: The function to invoke. -/// -/// - Returns: Whatever is returned by `body`. -/// -/// - Throws: Whatever is thrown by `body`. -/// -/// This function invokes `body` isolated to the default isolation context as -/// specified when configuring the test run. -/// -/// - Warning: This function is used to implement the `@Test` macro. Do not call +/// - Warning: This property is used to implement the `@Test` macro. Do not call /// it directly. -public func __withDefaultIsolationContext( - isolation: isolated (any Actor)? = #isolation, - _ body: (isolated (any Actor)?) async throws -> R -) async throws -> R where R: Sendable { - try await body(Configuration.current?.defaultIsolationContext ?? isolation) +public var __defaultIsolationContext: (any Actor)? { + Configuration.current?.defaultIsolationContext ?? #isolation } /// Run a test function as an `XCTestCase`-compatible method. diff --git a/Sources/TestingMacros/TestDeclarationMacro.swift b/Sources/TestingMacros/TestDeclarationMacro.swift index 7365bc5d7..eec6c2dd5 100644 --- a/Sources/TestingMacros/TestDeclarationMacro.swift +++ b/Sources/TestingMacros/TestDeclarationMacro.swift @@ -233,7 +233,7 @@ public struct TestDeclarationMacro: PeerMacro, Sendable { // If the function is noasync *and* main-actor-isolated, we'll call through // MainActor.run to invoke it. We do not have a general mechanism for // detecting isolation to other global actors. - lazy var isMainActorIsolated = !functionDecl.attributes(named: "MainActor", inModuleNamed: "Swift").isEmpty + lazy var isMainActorIsolated = !functionDecl.attributes(named: "MainActor", inModuleNamed: "_Concurrency").isEmpty var forwardCall: (ExprSyntax) -> ExprSyntax = { "try await Testing.__requiringTry(Testing.__requiringAwait(\($0)))" } @@ -288,24 +288,13 @@ public struct TestDeclarationMacro: PeerMacro, Sendable { } // If this function is synchronous and is not explicitly isolated to the - // main actor, it may still need to run main-actor-isolated depending on the - // runtime configuration in the test process. + // main actor, it should run in the configured default isolation context. if functionDecl.signature.effectSpecifiers?.asyncSpecifier == nil && !isMainActorIsolated { - if functionDecl.signature.parameterClause.parameters.tokens(viewMode: .sourceAccurate) - .map(\.tokenKind) - .contains(.keyword(.borrowing)) { - // BUG: rdar://137308488 The compiler cannot tell that this thunk calls - // its closure only once, so it emits a diagnostic about consuming the - // borrowed parameter. Work around the diagnostic by not emitting the - // isolation context hop. It's unfortunate but should rarely be a - // problem since we don't support move-only arguments anyway (see #543.) - } else { - thunkBody = """ - try await Testing.__withDefaultIsolationContext { _ in + thunkBody = """ + try await { (_: isolated (any Actor)?) async throws in \(thunkBody) - } - """ - } + }(__defaultIsolationContext) + """ } // Add availability guards if needed. From 46f79be3f0136d50cf95736d07de25419f3e39db Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Mon, 7 Oct 2024 10:31:56 -0400 Subject: [PATCH 3/8] Monday Jonathan disagrees strenuously with Saturday Jonathan --- .../FunctionDeclSyntaxAdditions.swift | 22 ++++++++++ .../TestingMacros/TestDeclarationMacro.swift | 41 ++++++++++++++++--- 2 files changed, 58 insertions(+), 5 deletions(-) diff --git a/Sources/TestingMacros/Support/Additions/FunctionDeclSyntaxAdditions.swift b/Sources/TestingMacros/Support/Additions/FunctionDeclSyntaxAdditions.swift index c7df36f18..f99a7f18b 100644 --- a/Sources/TestingMacros/Support/Additions/FunctionDeclSyntaxAdditions.swift +++ b/Sources/TestingMacros/Support/Additions/FunctionDeclSyntaxAdditions.swift @@ -26,6 +26,28 @@ extension FunctionDeclSyntax { .contains(.keyword(.mutating)) } + /// Whether or not this function is a `nonisolated` function. + var isNonisolated: Bool { + modifiers.lazy + .map(\.name.tokenKind) + .contains(.keyword(.nonisolated)) + } + + /// The `isolated` parameter of this function, if any. + var isolatedParameter: FunctionParameterSyntax? { + signature.parameterClause.parameters.first { parameter in + guard let type = parameter.type.as(AttributedTypeSyntax.self) else { + return false + } + return type.specifiers.contains { specifier in + guard case let .simpleTypeSpecifier(specifier) = specifier else { + return false + } + return specifier.specifier.tokenKind == .keyword(.isolated) + } + } + } + /// The name of this function including parentheses, parameter labels, and /// colons. var completeName: String { diff --git a/Sources/TestingMacros/TestDeclarationMacro.swift b/Sources/TestingMacros/TestDeclarationMacro.swift index eec6c2dd5..aec198266 100644 --- a/Sources/TestingMacros/TestDeclarationMacro.swift +++ b/Sources/TestingMacros/TestDeclarationMacro.swift @@ -287,13 +287,44 @@ public struct TestDeclarationMacro: PeerMacro, Sendable { thunkBody = "_ = \(forwardCall("\(functionDecl.name.trimmed)\(forwardedParamsExpr)"))" } - // If this function is synchronous and is not explicitly isolated to the - // main actor, it should run in the configured default isolation context. - if functionDecl.signature.effectSpecifiers?.asyncSpecifier == nil && !isMainActorIsolated { + // If this function is synchronous, is not explicitly nonisolated, and is + // not explicitly isolated to some actor, it should run in the configured + // default isolation context. If the suite type is an actor, this will cause + // a hop off the actor followed by an immediate hop back on, but otherwise + // should be harmless. + // + // We use a second, inner thunk function here instead of just adding the + // isolation parameter to the "real" thunk because adding it there prevents + // correct tuple desugaring of the "real" arguments to the thunk. + if functionDecl.signature.effectSpecifiers?.asyncSpecifier == nil && !isMainActorIsolated && !functionDecl.isNonisolated && functionDecl.isolatedParameter == nil { + // Get a unique name for this secondary thunk. We don't need it to be + // uniqued against functionDecl because it's interior to the "real" thunk, + // so its name can't conflict with any other names visible in this scope. + let isolationThunkName = context.makeUniqueName("") + + // Insert a (defaulted) isolated argument. If we emit a closure (or inner + // function) that captured the arguments to the "real" thunk, the compiler + // has trouble reasoning about the lifetime of arguments to that closure + // especially if those arguments are borrowed or consumed, which results + // in hard-to-avoid compile-time errors. Fortunately, forwarding the full + // argument list is straightforward. + let thunkParamsExprCopy = FunctionParameterClauseSyntax { + for thunkParam in thunkParamsExpr.parameters { + thunkParam + } + FunctionParameterSyntax( + modifiers: [DeclModifierSyntax(name: .keyword(.isolated))], + firstName: .wildcardToken(), + type: "isolated (any Actor)?" as TypeSyntax, + defaultValue: InitializerClauseSyntax(value: "Testing.__defaultIsolationContext" as ExprSyntax) + ) + } + thunkBody = """ - try await { (_: isolated (any Actor)?) async throws in + @Sendable func \(isolationThunkName)\(thunkParamsExprCopy) async throws { \(thunkBody) - }(__defaultIsolationContext) + } + try await \(isolationThunkName)\(forwardedParamsExpr) """ } From 5749ffac169d983be511453f7dfe67e77bc40720 Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Mon, 7 Oct 2024 10:35:24 -0400 Subject: [PATCH 4/8] Add some more fixture test coverage --- Tests/TestingTests/MiscellaneousTests.swift | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/Tests/TestingTests/MiscellaneousTests.swift b/Tests/TestingTests/MiscellaneousTests.swift index d8482c612..f04770846 100644 --- a/Tests/TestingTests/MiscellaneousTests.swift +++ b/Tests/TestingTests/MiscellaneousTests.swift @@ -99,12 +99,28 @@ struct SendableTests: Sendable { @Test(.hidden, arguments: FixtureData.zeroUpTo100) func parameterizedBorrowingAsync(i: borrowing Int) async {} + @MainActor + @Test(.hidden, arguments: FixtureData.zeroUpTo100) + func parameterizedBorrowingMainActor(i: borrowing Int) {} + + @available(*, noasync) + @Test(.hidden, arguments: FixtureData.zeroUpTo100) + func parameterizedBorrowingNoasync(i: borrowing Int) {} + @Test(.hidden, arguments: FixtureData.zeroUpTo100) func parameterizedConsuming(i: consuming Int) {} @Test(.hidden, arguments: FixtureData.zeroUpTo100) func parameterizedConsumingAsync(i: consuming Int) async { } + @MainActor + @Test(.hidden, arguments: FixtureData.zeroUpTo100) + func parameterizedConsumingMainActor(i: consuming Int) {} + + @available(*, noasync) + @Test(.hidden, arguments: FixtureData.zeroUpTo100) + func parameterizedConsumingNoasync(i: consuming Int) {} + @Test(.hidden, arguments: FixtureData.stringReturningClosureArray) func parameterizedAcceptingFunction(f: @Sendable () -> String) {} } From e2a075c8990f7f60829e2187bb4679e975dd58f7 Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Mon, 7 Oct 2024 10:41:38 -0400 Subject: [PATCH 5/8] Simplify the name of the instance created in the thunk (saves compile time) --- Sources/TestingMacros/TestDeclarationMacro.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/TestingMacros/TestDeclarationMacro.swift b/Sources/TestingMacros/TestDeclarationMacro.swift index aec198266..f596d1d9c 100644 --- a/Sources/TestingMacros/TestDeclarationMacro.swift +++ b/Sources/TestingMacros/TestDeclarationMacro.swift @@ -258,7 +258,7 @@ public struct TestDeclarationMacro: PeerMacro, Sendable { if functionDecl.isStaticOrClass { thunkBody = "_ = \(forwardCall("\(typeName).\(functionDecl.name.trimmed)\(forwardedParamsExpr)"))" } else { - let instanceName = context.makeUniqueName(thunking: functionDecl) + let instanceName = context.makeUniqueName("") let varOrLet = functionDecl.isMutating ? "var" : "let" thunkBody = """ \(raw: varOrLet) \(raw: instanceName) = \(forwardInit("\(typeName)()")) From 5c0490b3b6be9d26ffbdf7d592c08815d91aac36 Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Mon, 7 Oct 2024 10:49:00 -0400 Subject: [PATCH 6/8] Can't specify parameterized test arguments as isolated anyway, so strip that bit back out --- .../Additions/FunctionDeclSyntaxAdditions.swift | 15 --------------- Sources/TestingMacros/TestDeclarationMacro.swift | 5 +++-- Tests/TestingTests/RunnerTests.swift | 4 ++++ 3 files changed, 7 insertions(+), 17 deletions(-) diff --git a/Sources/TestingMacros/Support/Additions/FunctionDeclSyntaxAdditions.swift b/Sources/TestingMacros/Support/Additions/FunctionDeclSyntaxAdditions.swift index f99a7f18b..1990356da 100644 --- a/Sources/TestingMacros/Support/Additions/FunctionDeclSyntaxAdditions.swift +++ b/Sources/TestingMacros/Support/Additions/FunctionDeclSyntaxAdditions.swift @@ -33,21 +33,6 @@ extension FunctionDeclSyntax { .contains(.keyword(.nonisolated)) } - /// The `isolated` parameter of this function, if any. - var isolatedParameter: FunctionParameterSyntax? { - signature.parameterClause.parameters.first { parameter in - guard let type = parameter.type.as(AttributedTypeSyntax.self) else { - return false - } - return type.specifiers.contains { specifier in - guard case let .simpleTypeSpecifier(specifier) = specifier else { - return false - } - return specifier.specifier.tokenKind == .keyword(.isolated) - } - } - } - /// The name of this function including parentheses, parameter labels, and /// colons. var completeName: String { diff --git a/Sources/TestingMacros/TestDeclarationMacro.swift b/Sources/TestingMacros/TestDeclarationMacro.swift index f596d1d9c..b65cff6a1 100644 --- a/Sources/TestingMacros/TestDeclarationMacro.swift +++ b/Sources/TestingMacros/TestDeclarationMacro.swift @@ -291,12 +291,13 @@ public struct TestDeclarationMacro: PeerMacro, Sendable { // not explicitly isolated to some actor, it should run in the configured // default isolation context. If the suite type is an actor, this will cause // a hop off the actor followed by an immediate hop back on, but otherwise - // should be harmless. + // should be harmless. Note that we do not support specifying an `isolated` + // argument on a test function at this time. // // We use a second, inner thunk function here instead of just adding the // isolation parameter to the "real" thunk because adding it there prevents // correct tuple desugaring of the "real" arguments to the thunk. - if functionDecl.signature.effectSpecifiers?.asyncSpecifier == nil && !isMainActorIsolated && !functionDecl.isNonisolated && functionDecl.isolatedParameter == nil { + if functionDecl.signature.effectSpecifiers?.asyncSpecifier == nil && !isMainActorIsolated && !functionDecl.isNonisolated { // Get a unique name for this secondary thunk. We don't need it to be // uniqued against functionDecl because it's interior to the "real" thunk, // so its name can't conflict with any other names visible in this scope. diff --git a/Tests/TestingTests/RunnerTests.swift b/Tests/TestingTests/RunnerTests.swift index 571e40bc2..8005f6511 100644 --- a/Tests/TestingTests/RunnerTests.swift +++ b/Tests/TestingTests/RunnerTests.swift @@ -822,6 +822,10 @@ final class RunnerTests: XCTestCase { @Test(.hidden) @MainActor func asyncButRunsOnMainActor() async { XCTAssertTrue(Thread.isMainThread) } + + @Test(.hidden) nonisolated func runsNonisolated() { + XCTAssertFalse(Thread.isMainThread) + } } @available(*, deprecated) From 53942cde72cd4afb8ca776550af74a82e5fa6b3b Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Mon, 7 Oct 2024 10:53:59 -0400 Subject: [PATCH 7/8] More test coverage --- Tests/TestingTests/RunnerTests.swift | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Tests/TestingTests/RunnerTests.swift b/Tests/TestingTests/RunnerTests.swift index 8005f6511..f83b2736c 100644 --- a/Tests/TestingTests/RunnerTests.swift +++ b/Tests/TestingTests/RunnerTests.swift @@ -807,7 +807,11 @@ final class RunnerTests: XCTestCase { @TaskLocal static var isMainActorIsolationEnforced = false @Suite(.hidden) struct MainActorIsolationTests { - @Test(.hidden) func mustRunOnMainActor() { + @Test(.hidden) func mightRunOnMainActor() { + XCTAssertEqual(Thread.isMainThread, isMainActorIsolationEnforced) + } + + @Test(.hidden, arguments: 0 ..< 10) func mightRunOnMainActor(arg: Int) { XCTAssertEqual(Thread.isMainThread, isMainActorIsolationEnforced) } From 1c197b291b30c360f918d761e43d103277910ad2 Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Mon, 7 Oct 2024 11:44:33 -0400 Subject: [PATCH 8/8] Incorporate feedback --- Sources/Testing/Running/Configuration.swift | 10 +++++----- Sources/Testing/Test+Macro.swift | 4 ++-- Sources/TestingMacros/TestDeclarationMacro.swift | 4 ++-- Tests/TestingTests/RunnerTests.swift | 4 ++-- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/Sources/Testing/Running/Configuration.swift b/Sources/Testing/Running/Configuration.swift index 5702c7f93..89cb93a07 100644 --- a/Sources/Testing/Running/Configuration.swift +++ b/Sources/Testing/Running/Configuration.swift @@ -111,7 +111,7 @@ public struct Configuration: Sendable { /// /// If the value of this property is `nil`, synchronous test functions run in /// an unspecified isolation context. - public var defaultIsolationContext: (any Actor)? = nil + public var defaultSynchronousIsolationContext: (any Actor)? = nil // MARK: - Time limits @@ -237,16 +237,16 @@ public struct Configuration: Sendable { extension Configuration { #if !SWT_NO_GLOBAL_ACTORS - @available(*, deprecated, message: "Set defaultIsolationContext instead.") + @available(*, deprecated, message: "Set defaultSynchronousIsolationContext instead.") public var isMainActorIsolationEnforced: Bool { get { - defaultIsolationContext === MainActor.shared + defaultSynchronousIsolationContext === MainActor.shared } set { if newValue { - defaultIsolationContext = MainActor.shared + defaultSynchronousIsolationContext = MainActor.shared } else { - defaultIsolationContext = nil + defaultSynchronousIsolationContext = nil } } } diff --git a/Sources/Testing/Test+Macro.swift b/Sources/Testing/Test+Macro.swift index 02d54babd..891b37fe3 100644 --- a/Sources/Testing/Test+Macro.swift +++ b/Sources/Testing/Test+Macro.swift @@ -508,8 +508,8 @@ extension Test { /// /// - Warning: This property is used to implement the `@Test` macro. Do not call /// it directly. -public var __defaultIsolationContext: (any Actor)? { - Configuration.current?.defaultIsolationContext ?? #isolation +public var __defaultSynchronousIsolationContext: (any Actor)? { + Configuration.current?.defaultSynchronousIsolationContext ?? #isolation } /// Run a test function as an `XCTestCase`-compatible method. diff --git a/Sources/TestingMacros/TestDeclarationMacro.swift b/Sources/TestingMacros/TestDeclarationMacro.swift index b65cff6a1..2274a4788 100644 --- a/Sources/TestingMacros/TestDeclarationMacro.swift +++ b/Sources/TestingMacros/TestDeclarationMacro.swift @@ -292,7 +292,7 @@ public struct TestDeclarationMacro: PeerMacro, Sendable { // default isolation context. If the suite type is an actor, this will cause // a hop off the actor followed by an immediate hop back on, but otherwise // should be harmless. Note that we do not support specifying an `isolated` - // argument on a test function at this time. + // parameter on a test function at this time. // // We use a second, inner thunk function here instead of just adding the // isolation parameter to the "real" thunk because adding it there prevents @@ -317,7 +317,7 @@ public struct TestDeclarationMacro: PeerMacro, Sendable { modifiers: [DeclModifierSyntax(name: .keyword(.isolated))], firstName: .wildcardToken(), type: "isolated (any Actor)?" as TypeSyntax, - defaultValue: InitializerClauseSyntax(value: "Testing.__defaultIsolationContext" as ExprSyntax) + defaultValue: InitializerClauseSyntax(value: "Testing.__defaultSynchronousIsolationContext" as ExprSyntax) ) } diff --git a/Tests/TestingTests/RunnerTests.swift b/Tests/TestingTests/RunnerTests.swift index f83b2736c..356082356 100644 --- a/Tests/TestingTests/RunnerTests.swift +++ b/Tests/TestingTests/RunnerTests.swift @@ -848,12 +848,12 @@ final class RunnerTests: XCTestCase { func testSynchronousTestFunctionRunsInDefaultIsolationContext() async { var configuration = Configuration() - configuration.defaultIsolationContext = MainActor.shared + configuration.defaultSynchronousIsolationContext = MainActor.shared await Self.$isMainActorIsolationEnforced.withValue(true) { await runTest(for: MainActorIsolationTests.self, configuration: configuration) } - configuration.defaultIsolationContext = nil + configuration.defaultSynchronousIsolationContext = nil await Self.$isMainActorIsolationEnforced.withValue(false) { await runTest(for: MainActorIsolationTests.self, configuration: configuration) }