Skip to content

[6.0][sending] Change CheckedContinuation/AsyncThrowingStream.Continuation APIs to use sending parameters and results. #74097

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
merged 10 commits into from
Jun 4, 2024

Conversation

gottesmm
Copy link
Contributor

@gottesmm gottesmm commented Jun 3, 2024

Explanation: This PR contains a bunch of work that culminates in adding sending to CheckedContinuation/Async{Throwing,}Stream.Continuation APIs. It also changes withCheckedContinuation to have a sending return value.

To implement this, I had to fix a bunch of blocking issues:

  • 7bb76be: I found that we were not properly reconstructing sending parameters during type reconstruction. This caused asserts to fire when compiling these APIs with sending parameters.
  • 7500deb: I discovered that we were dealing with isolated parameters in a specific part of region isolation by just checking for self. This is incorrect and we needed to check for an isolated parameter. This is especially important when processes isolated closures passed to withCheckedContinuation. I discovered that without this the tests I wrote for withCheckedContinuation failed.
  • 4b4583f: In this one, I discovered that in certain cases we were not properly handling global actor instances (e.x.: an instance of MainActor) which were passed to a closure as an isolated parameter. We were treating them as actor instances, but we needed to recognize them as global actor isolation to get the appropriate diagnostics.
  • 36ac58f and 5df0978. These two commits were dealing with problems that I found when handling sending with indirect results. The specific problem was that we were not handling indirect result alloc_stacks appropriately.
  • 3510f29: In this commit, I changed Builtin.withUnsafe{,Throwing}Continuation to return a sending result. This was a source stable change since we can always return a sending T as a T so even though I didn't change the actual withCheckedContinuation APIs yet, everything still worked. But this setup the patch series for future work.
  • c0a2fcc & cab61e8: These two changes have to do with mangling changes that result for sending. Specifically, originally I was going to use silgen_name for these APIs but I discovered that withCheckedContinuation relied on @backDeploy that does not support silgen_name. So I decided to just move forward some work I already needed to do for mangling so that I did not need to use silgen_name for these APIs. The specific mangling changes is that sending on a top level function does not affect mangling, but in recursive mangling contexts, we do mangle it in. This ensures that we do not mangling sending into: fun c foo(_ x: sending T) { ... }, but we do mangle it into: let f: (sending T) -> (). It also ensures that we do not mangle __shared into the mangling if __shared is combined with sending. This ensures that our adopters do not need to silgen_name if they use this workaround to avoid needing to use borrowing sending.
  • 75433a3: In this commit, I made it so that sending/transferring can always be parsed but their semantic effects are triggered instead by region isolation. This was done by changing all semantic changes that relied on the SendingArgsAndResults feature to instead rely on the RegionBasedIsolation feature and then to change {Sending,Transferring}ArgsAndResults to be normal LANGUAGE_FEATURES. This ensured that I did not need to change swift's CMake to pass in SendingArgsAndResults to the concurrency library so that I could make this actual change.
  • 1a6f055: The final penultimate change. This is where I actually made the API change to the Concurrency library. I made it so that CheckedContinuation.resume takes a sending parameter, Async{Throwing,}Stream.yield takes a sending parameter, and withCheckedContinuation returns a transferring parameter. Due to the previous changes, this is an ABI neutral change.

Radars:

  • rdar://120420024
  • rdar://128961672
  • rdar://129116141
  • rdar://127383107

Original PRs:

Risk: Low. Just affects sending.
Testing: Added tests to the test suite.
Reviewer: N/A

gottesmm added 10 commits June 3, 2024 09:36
… use the isolated parameter, not self.

This was ok in the small since most of the time we were processing a self
parameter as isolated... but that isn't always true...

(cherry picked from commit 1d8ea84)
…actors as global actors rather than actor instance.

The specific example I ran into was in sendable_continuation.swift where we were
passing in the @mainactor instance as a sil_isolated parameter. We were thinking
it was an actor instance so we emitted the wrong message.

(cherry picked from commit 0cd0868)
…pplies merged region as a value, just propagate its isolation.

The reason that I am doing this is it ensures that if we have a region isolation
merge failure due to a mismatch in between the actual args in the region and the
propagated callee isolation, we see it immediately when we translate the apply
into the pseudo-IR instead of later when we perform the actual diagnostic
emission. This makes it far easier to diagnose these issues since we get an
unknown pattern very early which can be asserted on via the option
-sil-region-isolation-assert-on-unknown-pattern.

(cherry picked from commit b7249e7)
…ce to infer disconnected if the alloc stack is used as a sending indirect result.

I also fixed an issue that I found where we were not substituting SILResultInfo
flags which was causing us to drop when substituting sil_sending. I added a
SILVerifier check to make sure that we do not break this again.

(cherry picked from commit c7124e4)
…ng into vars/storage.

We want to ensure that functions/methods themselves do not have sending mangled
into their names, but we do want sending mangled in non-top level positions. For
example: we do not want to mangle sending into a function like the following:

```swift
// We don't want to mangle this.
func test(_ x: sending NonSendableKlass) -> ()
```

But when  it comes  to actually  storing functions  into memory,  we do  want to
distinguish in  between function values  that use sending  vs those that  do not
since we do not want to allow for  them to alias. Thus we want to mangle sending
into things like the following:

```swift
// We want to distinguish in between Array<(sending T) -> ()> and
// Array((T) -> ()>
let a = Array<(sending T) -> ()>

// We want to distinguish in between a global contianing (sending T) -> () and a
// global containing (T) -> ().
var global: (sending T) -> ()
```

This commit achieves that by making changes to the ASTMangler in getDeclType
which causes getDeclType to set a flag that says that we have not yet recursed
through the system and thus should suppress the printing of sendable. Once we
get further into the system and recurse, that flag is by default set to true, so
we get the old sending parameter without having to update large amounts of code.

rdar://127383107
(cherry picked from commit 2bf497d)
…aired with sending.

I am doing this b/c we are going to ban borrowing sending so that we can leave
open that space for further design. In the short term, we need the ability to
create +0 sending parameters without messing with mangling. By special casing
this, we get what we want.

rdar://129116141
(cherry picked from commit a890c60)
…E instead of an UPCOMING_FEATURE.

TLDR: This makes it so that we always can parse sending/transferring but changes
the semantic language effects to be keyed on RegionBasedIsolation instead.

----

The key thing that makes this all work is that I changed all of the "special"
semantic changes originally triggered on *ArgsAndResults to now be triggered
based on RegionBasedIsolation being enabled. This makes a lot of sense since we
want these semantic changes specifically to be combined with the checkers that
RegionBasedIsolation turns on. As a result, even though this causes these two
features to always be enabled, we just parse it but we do not use it for
anything semantically.

rdar://128961672
(cherry picked from commit 88729b9)

Conflicts:
	include/swift/Basic/Features.def
	lib/DriverTool/sil_opt_main.cpp
… APIs to use sending parameters and results.

Specifically:

1. CheckedContinuation.resume now takes a sending parameter.
2. Async{Throwing,}Stream.yield takes a sending parameter.
3. withCheckedContinuation returns a transferring parameter.

Importantly due to the previous changes around mangling, this is a mangling
neutral change.

rdar://120420024
(cherry picked from commit ce1dd43)
@gottesmm gottesmm requested a review from a team as a code owner June 3, 2024 16:40
@gottesmm
Copy link
Contributor Author

gottesmm commented Jun 3, 2024

@swift-ci test

@gottesmm gottesmm merged commit 5b91fd8 into swiftlang:release/6.0 Jun 4, 2024
5 checks passed
@gottesmm gottesmm deleted the release/6.0-128961672 branch June 4, 2024 00:24
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