[6.0][transfer-non-sendable] Teach SILIsolationInfo how to handle "look through instructions" when finding actor instances. #74677
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
(cherry picked from commit baec06e)
…rough instructions" when finding actor instances.
From the perspective of the IR, we are changing SILIsolationInfo such that
inferring an actor instance means looking at equivalence classes of values where
we consider operands to look through instructions to be equivalent to their dest
value. The result is that cases where the IR maybe puts in a copy_value or the
like, we consider the copy_value to have the same isolation info as using the
actor directly. This prevents a class of crashes due to merge failings. Example:
```swift
actor MyActor {
init() async {
init(ns: NonSendableKlass) async {
self.k = NonSendableKlass()
self.helper(ns)
}
func helper(_ newK: NonSendableKlass) {}
}
```
Incidently, we already had a failing test case from this behavior rather than
the one that was the original genesis. Specifically:
1. If a function's SILIsolationInfo is the same as the isolation info of a
SILValue, we assume that no transfer actually occurs.
2. Since we were taking too static of a view of actor instances when comparing,
we would think that a SILIsolationInfo of a #isolation parameter to as an
argument would be different than the ambient's function isolation which is also
that same one. So we would emit a transfer non transferrable error if we pass in
any parameters of the ambient function into another isolated function. Example:
```swift
actor Test {
@TaskLocal static var local: Int?
func withTaskLocal(isolation: isolated (any Actor)? = #isolation,
_ body: (consuming NonSendableValue, isolated (any Actor)?) -> Void) async {
Self.$local.withValue(12) {
// We used to get these errors here since we thought that body's isolation
// was different than the body's isolation.
//
// warning: sending 'body' risks causing data races
// note: actor-isolated 'body' is captured by a actor-isolated closure...
body(NonSendableValue(), isolation)
}
}
}
```
rdar://129400019
(cherry picked from commit c01f551)
|
@swift-ci test |
tbkka
approved these changes
Jun 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation: This PR fixes an issue where when we were trying to identify the specific actor instance that was being identified and we were not looking through instructions like copy_value/move_value which we should have. So we were saying that two SILIsolationInfo that should be identical were actually different since the underlying value was different.
By doing this, we fix two issues at least:
the problem here was that we viewed the isolation of helper to be different than the isolation of ns even though they are the same.
The problem in this case is that body is considered isolated to
#isolationbut there is a copy on it so we think it has different isolation from the closure whose isolation is straight upisolation.Radars:
Original PRs:
Risk: Low.
Testing: Added tests to the compiler.
Reviewer: @ktoso