Skip to content

confirmation() does not work from @MainActor tests #622

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.

Sign up for GitHub

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

Jump to bottom
Closed
macguru opened this issue Aug 20, 2024 · 2 comments · Fixed by #624
Closed

confirmation() does not work from @MainActor tests #622

macguru opened this issue Aug 20, 2024 · 2 comments · Fixed by #624
Assignees
@grynspan
Labels
bug 🪲 Something isn't working concurrency 🔀 Swift concurrency/sendability issues
Milestone
Swift 6.1

Comments

@macguru
Copy link

macguru commented Aug 20, 2024 •
edited
Loading

Description

The following test function will not compile in the Swift 6 language mode:

@MainActor @Test func example() async {
	await confirmation { _ in } // Error: Sending main actor-isolated value of type '(Confirmation) async -> ()' with later accesses to nonisolated context risks causing data races
}

Expected behavior

I think this should just work. confirmation() should take (and respect) an additional isolation argument isolation: isolated (any Actor)? = #isolation.

Proposed new signature (also discussed here):

func confirmation<R>(
    _ comment: Testing.Comment? = nil,
    expectedCount: Int = 1,
    isolation: isolated (any Actor)? = #isolation,
    sourceLocation: Testing.SourceLocation = #_sourceLocation,
    _ body: (Testing.Confirmation) async throws -> R
) async rethrows -> R

Actual behavior

No response

Steps to reproduce

No response

swift-testing version/commit hash

Xcode 16 beta 5

Swift & OS version (output of swift --version ; uname -a)

swift-driver version: 1.113 Apple Swift version 6.0 (swiftlang-6.0.0.7.6 clang-1600.0.24.1)
Target: arm64-apple-macosx15.0
Darwin Mac.localdomain 24.0.0 Darwin Kernel Version 24.0.0: Wed Aug 7 03:08:56 PDT 2024; root:xnu-11215.1.9~22/RELEASE_ARM64_T6020 arm64
@macguru macguru added the bug 🪲 Something isn't working label Aug 20, 2024
@grynspan grynspan added concurrency 🔀 Swift concurrency/sendability issues swift-6.1 labels Aug 20, 2024
@grynspan grynspan self-assigned this Aug 20, 2024
@grynspan
Copy link
Contributor

Yes, we need to add an isolation argument. :) Thanks for filing!

grynspan added a commit that referenced this issue Aug 20, 2024
@grynspan
Add isolation argument to functions taking non-sendable async closu…
5c320f2 
…res.

This PR adds an `isolation` parameter to several API functions that take `async`
closures that are not required to be sendable. As well, it adds `sending` to
those closures' return types where appropriate.

This change is necessary in Swift 6 because a non-sendable async closure could
be called in the wrong isolation domain otherwise. In particular, if the caller
is `@MainActor`-isolated, the closure will not be called on the main actor and
will therefore always hop, and a concurrency error occurs:

```swift
@mainactor func f() async {
  await confirmation {
    // this inner closure is not actor-isolated, but can't be sent across
    // isolation domains.
  }
}
```

`withKnownIssue()` and `confirmation()` are affected, as are several async
overloads of the internal-but-public `__check()` function family.

This change is necessary for correctness, but is potentially source-breaking if
calling code directly references the modified functions by full name.

Resolves #622.
@grynspan grynspan mentioned this issue Aug 20, 2024
Merged
2 tasks
@grynspan
Copy link
Contributor

Tracked internally as rdar://134375046.

grynspan added a commit that referenced this issue Aug 26, 2024
@grynspan
Add isolation argument to functions taking non-sendable async closu…
86f2ad2 
…res. (#624)

This PR adds an `isolation` parameter to several API functions that take
`async` closures that are not required to be sendable. As well, it adds
`sending` to those closures' return types where appropriate.

This change is necessary in Swift 6 because a non-sendable async closure
could be called in the wrong isolation domain otherwise. In particular,
if the caller is `@MainActor`-isolated, the closure will not be called
on the main actor and will therefore always hop, and a concurrency error
occurs:

```swift
@mainactor func f() async {
  await confirmation {
    // this inner closure is not actor-isolated, but can't be sent across
    // isolation domains.
  }
}
```

`withKnownIssue()` and `confirmation()` are affected, as are several
async overloads of the internal-but-public `__check()` function family.

This change is necessary for correctness, but is potentially
source-breaking if calling code directly references the modified
functions by full name.

Resolves #622.

### Checklist:

- [x] Code and documentation should follow the style of the [Style
Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md).
- [x] If public symbols are renamed or modified, DocC references should
be updated.
@grynspan grynspan added this to the Swift 6.1 milestone Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@grynspan grynspan

Labels
bug 🪲 Something isn't working concurrency 🔀 Swift concurrency/sendability issues
Projects
None yet
Milestone
Swift 6.1
Development

Successfully merging a pull request may close this issue.

Add isolation argument to functions taking non-sendable async closures. swiftlang/swift-testing
2 participants
@macguru @grynspan