Skip to content

[transfer-non-sendable] Teach SILIsolationInfo how to handle "look through instructions" when finding actor instances. #74673

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

Conversation

gottesmm
Copy link
Contributor

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:

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:

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

gottesmm added 2 commits June 24, 2024 18:16
…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
@gottesmm gottesmm requested a review from ktoso as a code owner June 25, 2024 01:16
@gottesmm
Copy link
Contributor Author

@swift-ci smoke test


// TODO: This crashes the compiler since we attempt to hop_to_executor to the
// value that is materialized onto disk.
//
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While doing this list of look through, I wanted to see how having consuming/borrowing as actor self worked... and I found in the consuming case we crashed. I am going to fix that in a different PR. But I thought it was worth putting in tree.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, thanks for followup then as well :)

Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@gottesmm gottesmm merged commit dcd348f into swiftlang:main Jun 25, 2024
3 checks passed
@gottesmm gottesmm deleted the pr-820f4317d3273ec87c02ce9d16e1dc14a759dd40 branch June 25, 2024 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants