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

@isolated(any) function types #2357

Merged
merged 10 commits into from Mar 27, 2024
Merged

Conversation

rjmccall
Copy link
Member

@rjmccall rjmccall commented Mar 9, 2024

No description provided.

@ktoso ktoso self-requested a review March 9, 2024 04:25
proposals/NNNN-isolated-any-functions.md Show resolved Hide resolved
tasks:
- `Task.init`
- `Task.detached`
- `TaskGroup.add`
Copy link
Member

Choose a reason for hiding this comment

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

Similar to the other proposal I wrote I guess; if we want to list them all, we have to list them all:

Suggested change
- `TaskGroup.add`
- `TaskGroup.addTask(UnlessCancelled)`

(or, just say "all")

- `Task.init`
- `Task.detached`
- `TaskGroup.add`
- `ThrowingTaskGroup.add`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `ThrowingTaskGroup.add`
- `ThrowingTaskGroup.addTask(UnlessCancelled)`

- `Task.detached`
- `TaskGroup.add`
- `ThrowingTaskGroup.add`
- `DiscardingTaskGroup.add`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `DiscardingTaskGroup.add`
- `DiscardingTaskGroup.addTask(UnlessCancelled)`

- `TaskGroup.add`
- `ThrowingTaskGroup.add`
- `DiscardingTaskGroup.add`
- `ThrowingDiscardingTaskGroup.add`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `ThrowingDiscardingTaskGroup.add`
- `ThrowingDiscardingTaskGroup.addTask(UnlessCancelled)`

The Throwing... technically shall become deprecated/removed if we manage to make typed throws happen for them 🤔 There's another proposal in flight for this

Swift reserves the right to optimize the execution of tasks to avoid
"unnecessary" isolation changes, such as when an isolated `async` function
starts by calling a function with different isolation. In general, this
includes optimizing where the task initially starts executing. As an
Copy link
Member

Choose a reason for hiding this comment

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

This is probably worthy of an example as that's the "we'll reorder things" case that people should watch out for.

Maybe a bit here?

Suggested change
includes optimizing where the task initially starts executing. As an
includes optimizing where the task initially starts executing:
```swift
@MainActor class Helper {
let other: OtherActor
func help() {
Task { await other.work() } // *implicitly* starts on MainActor, however
Task { await other.work() } // this may be optimized away and start on `other` immediately
}
\``` // FIXME: can't suggest change with a \``` inside it, remove the \
In this example, `Task` is implicitly inheriting the enclosing context's MainActor isolation, however the task immediately calls code on another actor. The Swift runtime reserves the right to optimize away the initial "starting the task" enqueue on the MainActor, and instead enqueue on the `other` immediately.
As an

Did I get the semantics right of what you're proposing here?

@rjmccall rjmccall marked this pull request as ready for review March 18, 2024 17:36
Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

Looking good; just a handful of comments.

- It can also be inferred from context in a number of ways, such as
if the function is a method of a type with that attribute (which
itself can be inferred in a number of ways).
- Additionally, a closure passed directly to the `Task` initializer
Copy link
Member

Choose a reason for hiding this comment

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

A small drafting suggestion: the bullets here are interspersing the kinds of isolation with the ways in which isolation is inferred, making it harder to keep both separate in the reader's mind. Perhaps lay out the kinds of isolation here, and move the "inference" bits until later?

closure expression (including an implicit autoclosure), a function
reference, or a partial application of a method reference, the
resulting function is dynamically isolated to the isolation of the
function or closure. This looks through non-instrumental differences
Copy link
Member

Choose a reason for hiding this comment

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

"non-instrumental differences" isn't a phrase I've ever seen us use. Perhaps "looks though syntax that has no effect on the semantics of the expression, such as parentheses"?

Comment on lines 326 to 327
- it is a reference to a capture or immutable binding immediately
initialized with a derivation of `E`; or
Copy link
Member

Choose a reason for hiding this comment

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

IIRC, the current isolation analysis for isolated parameters does not look through an "immutable binding immediately initialized with a derivation of E", e.g.,

actor A {
  func g() { }
}

func f(actor: isolated A) {
  let a = actor
  a.g() // error: a is not considered to be in the same isolation domain, call needs to be asynchronous
}

If we want to generalize that, great, but we should do so consistently rather than make it a rule specific to .isolation on an @isolated(any) type.


(This uses the `isolated` capture modifier proposed in the draft
[closure isolation control][isolated-captures] proposal. It would be
possible but significantly more awkward to get this effect with that
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
possible but significantly more awkward to get this effect with that
possible but significantly more awkward to get this effect without that

Comment on lines +399 to +402
- it comes from a closure expression that is only *implicitly* isolated
to an actor (that is, it has neither an explicit `isolated` capture
nor a global actor attribute). This can currently only happen with
`Task {}`.
Copy link
Member

Choose a reason for hiding this comment

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

This came up in the pitch thread, and you answered it there. The proposal would be stronger if it included this justification.

with the closure isolation pitch.  We're considering this proposal
knowing that some things can't be expressed yet without that pitch.
As long as we add something vaguely like that pitch in the future,
this is how the rules for isolated(any) should apply to it.
@DougGregor DougGregor merged commit bac1faa into apple:main Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants