[Concurrency] Implement type checking for 'async let' declarations. #34573

DougGregor · 2020-11-04T16:07:24Z

Implement parsing and type checking for 'async let' declarations. 'async let' allows one to create child tasks that execute concurrently with the current task. This covers:

Declaring async let declarations within asynchronous functions
Ensuring that access to an async let declaration is covered by await (and try, if the initializer could throw an error)
Treating the initializer as a concurrently-executing child task, by injecting it into an escaping autoclosure, which (e.g.) takes it out of the actor isolation domain

Covers the type checking portion of rdar://70101851.

test/decl/var/async_let.swift

rintaro · 2020-11-05T01:12:08Z

Could you add test cases for multiple patterns? like

async let veggies = try chopVegetables(), meat = marinateMeat()

Edit: I see you have async let (_, _) = (1, 2), y2 = 7, but this is a (different?) error case.

Perform very basic semantic analysis for their well-formedness.

Extend effects checking to ensure that each reference to a variable bound by an 'async let' is covered by an "await" expression and occurs in a suitable context.

…all. The initializer of an 'async let' is executed as a separate child task that will run concurrently with the main body of the function. Model the semantics of this operation by wrapping the initializer in an async, escaping autoclosure (representing the evaluation of the child task), and then a call to that autoclosure (to This is useful both for actor isolation checking, which needs to treat the initializer as executing in concurrent code, and also (eventually) for code generation, which needs to have that code in a closure so that it can be passed off to the task-creation functions. There are a number of issues with this implementation producing extraneous diagnostics due to this closure transformation, which will be addressed in a follow-up commit.

Customize diagnostics when an 'await' is missing in an 'async let' initializer. While here, fix the coverage checking so we also diagnose a missing 'try'.

…itializer. When an 'async let' initializer can throw, any access to one of the variables in the 'async let' can also throw, so require such accesses to be annotated with 'try'.

DougGregor · 2020-11-05T08:10:08Z

Could you add test cases for multiple patterns? like
async let veggies = try chopVegetables(), meat = marinateMeat()

Sure, I've added it, plus a bunch of other tests.

DougGregor · 2020-11-05T08:10:15Z

@swift-ci please smoke test

ktoso

Looking great, just some small inline comments 👍

include/swift/AST/Decl.h

ktoso · 2020-11-05T08:23:16Z

test/Concurrency/async_let_isolation.swift

+
+  func testAsyncLetIsolation() async {
+    async let x = self.synchronous()
+    // expected-error @-1{{actor-isolated instance method 'synchronous()' is unsafe to reference in code that may execute concurrently}}


Hmm... this one is interesting 🤔 Took me quite a moment to stare at it and ponder if the error is right or not. I guess it is correct, the reasoning is a bit non trivial to get to that perhaps though?

Sanity checking first then:

So we're saying that:

The (async initializer) closure is permitted but not required to have async function type.

So okey, it can indeed be the synchronous() function. That would mean we have a child task which will invoke this synchronous actor function; I guess that's where I first stumbled already -- there model wise there isn't really "synchronous actor function" because there are no "actor functions" but we only talk about "async functions being actor isolated"...?

So okey, async let = synchronous thing is okey... Then, we also say:

The closure executes on whatever actor it is restricted to, if it is restricted, or else on an unspecified executor.

So... I interpret this wording as "the closure executes on the target we invoke"? And since this function is synchronous... it is not associated with this actor (?!) so it would execute on an unspecified executor and that explains the current diagnostic?

Is my analysis so far correct?

--

Given that... that makes is a bit weird... I guess it was a bit surprising, but it makes sense with those rules. Not sure what a better error message would be -- explaining all the "why?" maybe 🤔

I agree that we could do better with the "why?" here, e.g., we could call out that we're in a child task.

test/expr/unary/async_await.swift

test/decl/var/async_let.swift

kastiglione · 2020-11-05T16:44:42Z

test/decl/var/async_let.swift

+
+// REQUIRES: concurrency
+
+async let x = 1 // okay


Curious: why allow non-async initializers?

The work you want to run concurrently could be synchronous work. Why not?

I didn't think that one through, sorry. I assumed (I don't know why) that concurrent code would be via async functions.

kastiglione · 2020-11-05T16:48:14Z

test/expr/unary/async_await.swift

+  async let x = await getInt()
+  print(x) // expected-error{{reference to async let 'x' is not marked with 'await'}}
+  print(await x)


Compared to the proposals and other docs, this is the first I've seen await being used (required?) on an async let initializer. Is the await keyword going to be required for initializers that call async functions? Are there two suspension points, at the initializer and at the subsequent read (on line 161)?

I would think the await would only be required in a case like this:

async let y = getY() async let x = await calculateX(y)

I (intentionally) deviated from the proposal document, because as I was writing the tests I felt like skipping the await was arbitrary and confusing.

oh, why arbitrary to not have it? My feeling was the opposite, I thought that adding the await reads like a suspension point, but if it's not a suspension point then the await seems arbitrary. And as I mentioned, the two awaits also struck me as potentially confusing. I'm not arguing a position, just wanted to understand this use, and to share my perspective, for what it's worth.

We should take this over to the forums. You're right that the async let itself isn't necessarily a suspension point, so in that sense, the await might be misleading. On the other hand, it tells you whether you have asynchronous code within the initializer, which... might?... be interesting.

DougGregor marked this pull request as draft November 4, 2020 16:07

ktoso reviewed Nov 5, 2020

View reviewed changes

test/decl/var/async_let.swift Show resolved Hide resolved

DougGregor force-pushed the async-let branch from 6a40e0f to c4f4f81 Compare November 5, 2020 00:20

DougGregor added 4 commits November 4, 2020 17:31

[Concurrency] Parse 'async let' declarations.

7f73877

Perform very basic semantic analysis for their well-formedness.

[Concurrency] Require references to 'async let' to have an 'await'.

9722df8

Extend effects checking to ensure that each reference to a variable bound by an 'async let' is covered by an "await" expression and occurs in a suitable context.

[Concurrency] Diagnose 'async let' declarations in non-async contexts.

dc4a119

DougGregor force-pushed the async-let branch from 05ca518 to 58b590a Compare November 5, 2020 01:32

DougGregor added 3 commits November 4, 2020 23:18

[Concurrency] Don't diagnose missing "self." in async let autoclosures.

33cfbbd

[Concurrency] Improve await/try checking in 'async let' initializers.

3223cf1

Customize diagnostics when an 'await' is missing in an 'async let' initializer. While here, fix the coverage checking so we also diagnose a missing 'try'.

[Concurrency] Require 'try' on accessing 'async let' with throwing in…

779a36b

…itializer. When an 'async let' initializer can throw, any access to one of the variables in the 'async let' can also throw, so require such accesses to be annotated with 'try'.

DougGregor changed the title ~~[Concurrency] Parse 'async let' declarations.~~ [Concurrency] Implement type checking for 'async let' declarations. Nov 5, 2020

DougGregor marked this pull request as ready for review November 5, 2020 08:07

[Concurrency] Add an async let example with multiple initializer clauses

290f9a5

ktoso approved these changes Nov 5, 2020

View reviewed changes

[Concurrency] Fix a comment.

4017888

DougGregor merged commit 203d2c2 into swiftlang:main Nov 5, 2020

DougGregor deleted the async-let branch November 5, 2020 16:44

kastiglione reviewed Nov 5, 2020

View reviewed changes

[Concurrency] Implement type checking for 'async let' declarations. #34573

[Concurrency] Implement type checking for 'async let' declarations. #34573

Uh oh!

Conversation

DougGregor commented Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

rintaro commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DougGregor commented Nov 5, 2020

Uh oh!

DougGregor commented Nov 5, 2020

Uh oh!

ktoso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kastiglione Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DougGregor commented Nov 4, 2020 •

edited

Loading

rintaro commented Nov 5, 2020 •

edited

Loading

kastiglione Nov 5, 2020 •

edited

Loading