[region-isolation] When inferring isolation for an argument, handle non-self isolated parameters as well as self parameters that are actor isolated. #73556
Conversation
This file contains hidden or 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
gottesmm
requested review from
ktoso,
hborla,
slavapestov and
xedin
as code owners
|
@swift-ci smoke test |
…e next commit. Specifically, I added a few helper methods and improved the logging printing. This all makes the next commit a more focused commit.
…on-self isolated parameters as well as self parameters that are actor isolated. As part of this I went through how we handled inference and rather than using a grab-bag getActorIsolation that was confusing to use, I created split APIs for specific use cases (actor instance, global actor, just an apply expr crossing) that makes it clearer inside the SILIsolationInfo::get* APIs what we are actually trying to model. I found a few issues as a result and fixed most of them if they were small. I also fixed one bigger one around computed property initializers in the next commit. There is a larger change I didn't fix around allowing function ref/partial_apply with isolated self parameters have a delayed flow sensitive actor isolation... this will be fixed in a subsequent commit. This also fixes a bunch of cases where we were printing actor-isolated instead of 'self' isolated. rdar://127295657
rdar://127844737
…use when printing SIL instructions.
gottesmm
force-pushed
the
rdar127295657_127844737
branch
from
82e26a5 to
085f3d7
Compare
|
@swift-ci smoke test |
… a header. This works around a layering violation where libSwiftBasic includes parts of the AST even though it shouldn't.
|
@swift-ci smoke test |
…g isolation of a sil_isolated parameter. It is unnecessary and seems to be slightly out of sync sometimes around closures.
|
@swift-ci smoke test |
…ctor types. I also added some docs to SIL.rst about sil_isolated as well.
|
@swift-ci smoke test |
gottesmm
commented
May 12, 2024
The specific problem was that the AST was looking for Actor/AnyActor in _Concurrency... but I named the module of the test borrowing (for some reason). So the machinery was failing to think that my stubbed out protocols where the true known protocols. By changing the module name to _Concurrency, everything worked out.
|
@swift-ci smoke test |
ktoso
reviewed
May 13, 2024
| ``@sil_isolated``. An ``@sil_isolated`` parameter must be one of: | ||
|
|
||
| - An actor or any actor type. | ||
| - A generic type that conforms to Actor or AnyActor. |
There was a problem hiding this comment.
AnyActor being a marker protocol, I am wondering if this means Actor or DistributedActor in practice?
There was a problem hiding this comment.
Not sure. What is important is that both have the semantics of Actors (namely they can create isolation).
ktoso
reviewed
May 13, 2024
| @@ -122,18 +181,29 @@ class SILIsolationInfo { | |||
|
|
|||
| /// If set this is the SILValue that represents the actor instance that we | |||
| /// derived isolatedValue from. | |||
| SILValue actorInstance; | |||
| /// | |||
| /// If set to (SILValue(), 1), then we are in an | |||
There was a problem hiding this comment.
I'll fix this in a follow on commit.
ktoso
reviewed
May 13, 2024
| @@ -97,7 +97,7 @@ public actor MyActor: MyProto { | |||
| func g(ns1: NS1) async { | |||
| await nonisolatedAsyncFunc1(ns1) // expected-targeted-and-complete-warning{{passing argument of non-sendable type 'NS1' outside of actor-isolated context may introduce data races}} | |||
| // expected-tns-warning @-1 {{sending 'ns1' risks causing data races}} | |||
| // expected-tns-note @-2 {{sending actor-isolated 'ns1' to nonisolated global function 'nonisolatedAsyncFunc1' risks causing data races between nonisolated and actor-isolated uses}} | |||
| // expected-tns-note @-2 {{sending 'self'-isolated 'ns1' to nonisolated global function 'nonisolatedAsyncFunc1' risks causing data races between nonisolated and 'self'-isolated uses}} | |||
There was a problem hiding this comment.
nice diagnostic improvement :)
This was referenced May 13, 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.
[region-isolation] When inferring isolation for an argument, handle non-self isolated parameters as well as self parameters that are actor isolated.
As part of this I went through how we handled inference and rather than using a
grab-bag getActorIsolation that was confusing to use, I created split APIs for
specific use cases (actor instance, global actor, just an apply expr crossing)
that makes it clearer inside the SILIsolationInfo::get* APIs what we are
actually trying to model. I found a few issues as a result and fixed most of
them if they were small. I also fixed one bigger one around computed property
initializers in the next commit. There is a larger change I didn't fix around allowing function
ref/partial_apply with isolated self parameters have a delayed flow sensitive
actor isolation... this will be fixed in a subsequent commit.
This also fixes a bunch of cases where we were printing actor-isolated instead
of 'self' isolated.
rdar://127295657
NOTE: The third commit is the commit that fixes inferred isolation in property initializers in actors.
rdar://127844737