Some typos and a few small wording changes #2
Conversation
This file contains 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
mattmassicotte
changed the title
mattmassicotte
commented
Mar 15, 2024
| @@ -10,7 +10,7 @@ | |||
|
|
|||
| ## Introduction | |||
|
|
|||
| This proposal provides the ability to explicitly specify actor-isolation or non-isolation of a closure, as well as providing a parameter attribute to guarantee that a closure parameter inherits the isolation of the context. It makes the isolation inheritance rules more uniform while also making a specific concurrency pattern less restrictive. | |||
| This proposal provides the ability to explicitly specify actor-isolation or non-isolation of a closure, as well as providing a parameter attribute to guarantee that a closure parameter inherits the isolation of the context. It makes the isolation inheritance rules more uniform, helps to better express intention at closure-creation time, and also makes integrating concurrency with non-Sendable types less restrictive. | |||
There was a problem hiding this comment.
I really appreciate you incorporating my original wording here, but at this point, I think the non-Sendable pattern is much less-impactful than the other changes.
mattmassicotte
commented
Mar 15, 2024
| @@ -45,7 +45,7 @@ Without a global actor isolation annotation, actor-isolation or non-isolation of | |||
|
|
|||
| Explicit annotation has the benefit of disabling inference rules and the potential that they lead to a formal isolation that is not preferred. For example, there are circumstances where it is beneficial to guarantee that a closure is `nonisolated` therefore knowing that its execution will hop off the current actor. Explicit annotation also offers the ability to identify a mismatch of intention, such as a case where the developer expected `nonisolated` but inference landed on actor-isolated, and the closure is mistakenly used in an isolated context. Using explicit annotation, the developer would receive a diagnostic about a `nonisolated` closure being used in an actor-isolated context which helpfully identifies this mismatch of intention. | |||
|
|
|||
| Additionally, there is a difference in how isolation inheritance behaves via the experimental attribute `@_inheritActorContext` (as used by `Task.init`) for isolated parameters vs actor isolation: global actor isolatation is inherited by `Task`'s initializer closure argument, whereas an actor-isolated parameter is not inherited. This makes it challenging to build intuition around how isolation inheritance works. It also makes the concurrency pattern impossible to allow a non-Sendable type to create a new Task that can mutate self. | |||
| Additionally, there is a difference in how isolation inheritance behaves via the experimental attribute `@_inheritActorContext` (as used by `Task.init`) for isolated parameters vs actor isolation: global actor isolation is inherited by `Task`'s initializer closure argument, whereas an actor-isolated parameter is not inherited. This makes it challenging to build intuition around how isolation inheritance works. It also makes it impossible to allow a non-Sendable type to create a new Task that can access self. | |||
There was a problem hiding this comment.
1 - Typo
2 - just clarify that the problem is worse that just mutating self
3 - simplify wording
mattmassicotte
commented
Mar 15, 2024
| @@ -80,7 +80,7 @@ Enable explicit specification of actor-isolation via an isolated parameter in a | |||
|
|
|||
| ```swift | |||
| actor A { | |||
| func isolate() { | |||
| nonisolated func isolate() { | |||
There was a problem hiding this comment.
I was a little confused here, because I think the same is semantically equivalent to just not using an isolated capture at all, isn't it?
I make this function nonisolated to really highlight the new capabilities. But please look at this closely, because I could be wrong!
mattmassicotte
commented
Mar 15, 2024
| @@ -90,7 +90,7 @@ actor A { | |||
|
|
|||
| ### Isolation inheritance | |||
|
|
|||
| Provide a formal replacement of the experimental parameter attribute `@_inheritActorContext` to resolve its ambiguity with closure isolation. Its replacement `@inheritsIsolation` changes the behavior so that it unconditionally and implicitly captures the isolation context (as opposed to currently in actor-isolated contexts it being conditional on whether you capture an isolated parameter or isolated capture or actor-isolated function, but guaranteed if the context is isolated to a global actor or `nonisolated`). | |||
| Provide a formal replacement of the experimental parameter attribute `@_inheritActorContext` to resolve its ambiguity with closure isolation. Currently, `@_inheritActorContext` actual context capture behavior is conditional on whether you capture an isolated parameter or isolated capture or actor-isolated function, but unconditional if the context is isolated to a global actor or `nonisolated`. Its replacement `@inheritsIsolation` changes the behavior so that it unconditionally and implicitly captures the isolation context. | |||
There was a problem hiding this comment.
I hope you don't mind this, but I extracted the large parenthesized backstory. I just think the backstory is important!
mattmassicotte
commented
Mar 15, 2024
| @@ -167,15 +167,15 @@ func isolate(d: D) { | |||
| } | |||
| ``` | |||
|
|
|||
| While it is technically possible to enqueue work on a remote distributed actor reference, the enqueue on such actor will always immediately crash. Because of that, we err on the side of disallowing such illegal code. [Future directions](#future-directions) discusses how this can be made more powerful when it is known that an actor is local. It is also worth noting the `da.whenLocal { isolated da in ... }` API which allows dynamically recovering an isolated distributed actor reference after it has dynamically been checked for locality. | |||
| While it is technically possible to enqueue work on a remote distributed actor reference, the enqueue on such an actor will always immediately crash. Because of that, we err on the side of disallowing such illegal code. [Future directions](#future-directions) discusses how this can be made more powerful when it is known that an actor is local. It is also worth noting the `da.whenLocal { isolated da in ... }` API which allows dynamically recovering an isolated distributed actor reference after it has dynamically been checked for locality. | |||
There was a problem hiding this comment.
Tiny typo - missing word.
mattmassicotte
commented
Mar 15, 2024
|
|
||
| ## Source compatibility | ||
|
|
||
| It is possible that existing code could have a closure that names a type-inferred parameter `nonisolated`: | ||
| ```swift | ||
| { nonisolated in print(nonisolated) } | ||
| ``` | ||
| but with this proposed change, `nonisolated` in this case would instead be interpretted as the contextual keyword specifying the formal isolation of the closure. Such code would then result in a compilation error when trying to use a parameter named `nonisolated`. | ||
| but with this proposed change, `nonisolated` in this case would instead be interpreted as the contextual keyword specifying the formal isolation of the closure. Such code would then result in a compilation error when trying to use a parameter named `nonisolated`. |
sophiapoirier
approved these changes
Apr 3, 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.
No description provided.