Skip to content
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

Add isolation argument to functions taking non-sendable async closures. #624

Merged
merged 4 commits into from
Aug 26, 2024

Conversation

grynspan
Copy link
Contributor

@grynspan grynspan commented Aug 20, 2024

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:

@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:

  • Code and documentation should follow the style of the Style Guide.
  • If public symbols are renamed or modified, DocC references should be updated.

…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 added bug Something isn't working concurrency Swift concurrency/sendability issues swift-6.1 labels Aug 20, 2024
@grynspan grynspan self-assigned this Aug 20, 2024
@grynspan
Copy link
Contributor Author

@swift-ci test

@grynspan
Copy link
Contributor Author

@swift-ci test

@grynspan
Copy link
Contributor Author

@swift-ci test

@grynspan grynspan requested review from gottesmm and hborla August 21, 2024 21:07
@macguru
Copy link

macguru commented Aug 22, 2024

I'm really not an expert on this, but I just stumbled across @_inheritActorContext in the Closure isolation control proposal, and am wondering what the tradeoff between that and this pull request's solution is.

As far as I understand, the following change should provide the same behavior to clients, with the benefit of not adding an additional argument to the signature:

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

Then again, @_inheritActorContext is experimental, it's use "strongly discouraged" and there is maybe more flexibility in having an (any Actor)? inside the function bodies…

@grynspan
Copy link
Contributor Author

@_inheritActorContext is not the "modern" way to propagate actor isolation into a closure, no. :)

@grynspan
Copy link
Contributor Author

@swift-ci test

@grynspan grynspan merged commit cac6314 into main Aug 26, 2024
3 checks passed
@grynspan grynspan deleted the jgrynspan/add-isolation-argument-to-various-functions branch August 26, 2024 18:21
grynspan added a commit that referenced this pull request Aug 26, 2024
…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
bradleymackey added a commit to bradleymackey/vault-ios that referenced this pull request Oct 3, 2024
- Initial migration of model types that use `@MainActor`
- There's a few issues in the Swift Testing library surrounding global
actors ([see
here](swiftlang/swift-testing#624)), so we will
likely wait for Swift 6.1 before migrating most of the code in
`VaultFeedTests` and `VaultiOSTests`, the only 2 packages left that have
XCTests in them.
- Resolves #330
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working concurrency Swift concurrency/sendability issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

confirmation() does not work from @MainActor tests
3 participants