Sema: Make @concurrent not imply async on closures
#83094
Conversation
AnthonyLatsis
requested review from
ktoso,
hborla,
xedin and
slavapestov
as code owners
|
@swift-ci please smoke test macOS |
| _ = { @concurrent () -> Int in } | ||
| // expected-error@-1:9 {{cannot use @concurrent on non-async closure}}{{none}} | ||
| _ = { @concurrent () async in } | ||
| _ = { @concurrent in await awaitMe() } |
There was a problem hiding this comment.
Presence of throws precludes effect inference from the body, might be good to add a test case where we have @concurrent + throws without async, to make sure that everything works as expected.
|
@swift-ci please smoke test |
The present approach is not prudent because `@concurrent` synchronous
functions, a natural extension, are a likely-to-happen future direction,
whereas the current inference rule is entirely grounded on `@concurrent`
being exclusive to async functions.
If we were to ship this rule, we would have to keep the promise for
backwards compatibility when implementing the aforementioned future
direction, replacing one inconsistency with another, and possibly
introducing new bug-prone expression checking code.
```swift
func foo(_: () -> Void) {}
func foo(_: () async -> Void) {}
// In a future without this change and `@concurrent` synchronous
// functions accepted, the first call resolves to the first overload,
// and the second call resolves to the second, despite `@concurrent` no
// longer implying `async`.
foo { }
foo { @Concurrent in }
```
This change also drops the fix-it for removing `@concurrent` when used
on a synchronous closure. With the inference rule gone, and the
diagnosis delayed until after solution application, this error raises a
fairly balanced choice between removing the attribute and being
explicit about the effect, where a unilateral suggestion is quite
possibly more harmful than useful.
|
@swift-ci please smoke test |
|
@swift-ci please test source compatibility |
@AnthonyLatsis if you have a spare moment, i'm curious if you could share what your current mental model for such a construct is. what would an |
|
Think of a synchronous actor-isolated function. Similarly, a |
The present approach is not prudent because
@concurrentsynchronous functions, a natural extension, are a likely-to-happen future direction, whereas the current inference rule is entirely grounded on@concurrentbeing exclusive to async functions.If we were to ship this rule, we would have to keep the promise for backwards compatibility when implementing the aforementioned future direction, replacing one inconsistency with another, and possibly introducing new bug-prone expression checking code.
This change also drops the fix-it for removing
@concurrentwhen used on a synchronous closure. With the inference rule gone, and the diagnosis delayed until after solution application, this error raises a fairly balanced choice between removing the attribute and being explicit about the effect, where a unilateral suggestion is quite possibly more harmful than useful.