Skip to content

Simplify the configuration option for synchronous test isolation. #747

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Oct 7, 2024
31 changes: 25 additions & 6 deletions Sources/Testing/Running/Configuration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 defaultSynchronousIsolationContext: (any Actor)? = nil

// MARK: - Time limits

Expand Down Expand Up @@ -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 defaultSynchronousIsolationContext instead.")
public var isMainActorIsolationEnforced: Bool {
get {
defaultSynchronousIsolationContext === MainActor.shared
}
set {
if newValue {
defaultSynchronousIsolationContext = MainActor.shared
} else {
defaultSynchronousIsolationContext = nil
}
}
}
#endif
}
53 changes: 4 additions & 49 deletions Sources/Testing/Test+Macro.swift
Original file line number Diff line number Diff line change
Expand Up @@ -504,58 +504,13 @@ extension Test {
value
}

#if !SWT_NO_GLOBAL_ACTORS
/// Invoke a function isolated to the main actor if appropriate.
/// The current default isolation context.
///
/// - 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<R>(
_ 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.
///
/// - Parameters:
/// - body: The function to invoke.
///
/// - Returns: Whatever is returned by `body`.
///
/// - 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.
///
/// - 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.
@inlinable public func __ifMainActorIsolationEnforced<R>(
_: @Sendable () async throws -> R,
else body: @Sendable () async throws -> R
) async throws -> R where R: Sendable {
try await body()
public var __defaultSynchronousIsolationContext: (any Actor)? {
Configuration.current?.defaultSynchronousIsolationContext ?? #isolation
}
#endif

/// Run a test function as an `XCTestCase`-compatible method.
///
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,13 @@ 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 name of this function including parentheses, parameter labels, and
/// colons.
var completeName: String {
Expand Down
104 changes: 38 additions & 66 deletions Sources/TestingMacros/TestDeclarationMacro.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand All @@ -290,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)))"
}
Expand All @@ -315,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)()"))
Expand Down Expand Up @@ -344,16 +287,45 @@ 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 may still need to run main-actor-isolated depending on the
// runtime configuration in the test process.
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. Note that we do not support specifying an `isolated`
// 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
// correct tuple desugaring of the "real" arguments to the thunk.
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.
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.__defaultSynchronousIsolationContext" as ExprSyntax)
)
}

thunkBody = """
try await Testing.__ifMainActorIsolationEnforced { \(captureListExpr) in
\(thunkBody)
} else: { \(captureListExpr) in
@Sendable func \(isolationThunkName)\(thunkParamsExprCopy) async throws {
\(thunkBody)
}
try await \(isolationThunkName)\(forwardedParamsExpr)
"""
}

Expand Down
2 changes: 0 additions & 2 deletions Tests/TestingMacrosTests/TestDeclarationMacroTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down
16 changes: 16 additions & 0 deletions Tests/TestingTests/MiscellaneousTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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) {}
}
Expand Down
24 changes: 23 additions & 1 deletion Tests/TestingTests/RunnerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand All @@ -822,8 +826,13 @@ final class RunnerTests: XCTestCase {
@Test(.hidden) @MainActor func asyncButRunsOnMainActor() async {
XCTAssertTrue(Thread.isMainThread)
}

@Test(.hidden) nonisolated func runsNonisolated() {
XCTAssertFalse(Thread.isMainThread)
}
}

@available(*, deprecated)
func testSynchronousTestFunctionRunsOnMainActorWhenEnforced() async {
var configuration = Configuration()
configuration.isMainActorIsolationEnforced = true
Expand All @@ -836,6 +845,19 @@ final class RunnerTests: XCTestCase {
await runTest(for: MainActorIsolationTests.self, configuration: configuration)
}
}

func testSynchronousTestFunctionRunsInDefaultIsolationContext() async {
var configuration = Configuration()
configuration.defaultSynchronousIsolationContext = MainActor.shared
await Self.$isMainActorIsolationEnforced.withValue(true) {
await runTest(for: MainActorIsolationTests.self, configuration: configuration)
}

configuration.defaultSynchronousIsolationContext = nil
await Self.$isMainActorIsolationEnforced.withValue(false) {
await runTest(for: MainActorIsolationTests.self, configuration: configuration)
}
}
#endif

@Suite(.hidden) struct DeprecatedVersionTests {
Expand Down