We want 'inout sending' parameters to have the semantics that not only are they
disconnected on return from the function but additionally they are guaranteed to
be in their own disconnected region on return. This implies that we must emit
errors when an 'inout sending' parameter or any element that is in the same
region as the current value within an 'inout sending' parameter is
returned. This commit contains a new diagnostic for RegionIsolation that adds
specific logic for detecting and emitting errors in these situations.
To implement this, we introduce 3 new diagnostics with each individual
diagnostic being slightly different to reflect the various ways that this error
can come up in source:
* Returning 'inout sending' directly:
```swift
func returnInOutSendingDirectly(_ x: inout sending NonSendableKlass) -> NonSendableKlass {
return x // expected-warning {{cannot return 'inout sending' parameter 'x' from global function 'returnInOutSendingDirectly'}}
// expected-note @-1 {{returning 'x' risks concurrent access since caller assumes that 'x' and the result of global function 'returnInOutSendingDirectly' can be safely sent to different isolation domains}}
}
```
* Returning a value in the same region as an 'inout sending' parameter. E.x.:
```swift
func returnInOutSendingRegionVar(_ x: inout sending NonSendableKlass) -> NonSendableKlass {
var y = x
y = x
return y // expected-warning {{cannot return 'y' from global function 'returnInOutSendingRegionVar'}}
// expected-note @-1 {{returning 'y' risks concurrent access to 'inout sending' parameter 'x' since the caller assumes that 'x' and the result of global function 'returnInOutSendingRegionVar' can be safely sent to different isolation domains}}
}
```
* Returning the result of a function or computed property that is in the same
region as the 'inout parameter'.
```swift
func returnInOutSendingViaHelper(_ x: inout sending NonSendableKlass) -> NonSendableKlass {
let y = x
return useNonSendableKlassAndReturn(y) // expected-warning {{cannot return result of global function 'useNonSendableKlassAndReturn' from global function 'returnInOutSendingViaHelper'}}
// expected-note @-1 {{returning result of global function 'useNonSendableKlassAndReturn' risks concurrent access to 'inout sending' parameter 'x' since the caller assumes that 'x' and the result of global function 'returnInOutSendingViaHelper' can be safely sent to different isolation domains}}
}
```
Additionally, I had to introduce a specific variant for each of these
diagnostics for cases where due to us being in a method, we are actually in our
caller causing the 'inout sending' parameter to be in the same region as an
actor isolated value:
* Returning 'inout sending' directly:
```swift
extension MyActor {
func returnInOutSendingDirectly(_ x: inout sending NonSendableKlass) -> NonSendableKlass {
return x // expected-warning {{cannot return 'inout sending' parameter 'x' from instance method 'returnInOutSendingDirectly'}}
// expected-note @-1 {{returning 'x' risks concurrent access since caller assumes that 'x' is not actor-isolated and the result of instance method 'returnInOutSendingDirectly' is 'self'-isolated}}
}
}
```
* Returning a value in the same region as an 'inout sending' parameter. E.x.:
```swift
extension MyActor {
func returnInOutSendingRegionLet(_ x: inout sending NonSendableKlass) -> NonSendableKlass {
let y = x
return y // expected-warning {{cannot return 'y' from instance method 'returnInOutSendingRegionLet'}}
// expected-note @-1 {{returning 'y' risks concurrent access to 'inout sending' parameter 'x' since the caller assumes that 'x' is not actor-isolated and the result of instance method 'returnInOutSendingRegionLet' is 'self'-isolated}}
}
}
```
* Returning the result of a function or computed property that is in the same region as the 'inout parameter'.
```swift
extension MyActor {
func returnInOutSendingViaHelper(_ x: inout sending NonSendableKlass) -> NonSendableKlass {
let y = x
return useNonSendableKlassAndReturn(y) // expected-warning {{cannot return result of global function 'useNonSendableKlassAndReturn' from instance method 'returnInOutSendingViaHelper'; this is an error in the Swift 6 language mode}}
// expected-note @-1 {{returning result of global function 'useNonSendableKlassAndReturn' risks concurrent access to 'inout sending' parameter 'x' since the caller assumes that 'x' is not actor-isolated and the result of instance method 'returnInOutSendingViaHelper' is 'self'-isolated}}
}
}
```
To implement this, I used two different approaches depending on whether or not
the returned value was generic or not.
* Concrete
In the case where we had a concrete value, I was able to in simple cases emit
diagnostics based off of the values returned by the return inst. In cases where
we phied together results due to multiple results in the same function, we
determine which of the incoming phied values caused the error by grabbing the
exit partition information of each of the incoming value predecessors and seeing
if an InOutSendingAtFunctionExit would emit an error.
* Generic
In the case of generic code, it is a little more interesting since the result is
a value stored in an our parameter instead of being a value directly returned by
a return inst. To work around this, I use PrunedLiveness to determine the last
values stored into the out parameter in the function to avoid having to do a
full dataflow. Then I take the exit blocks where we assign each of those values
and run the same check as we do in the direct phi case to emit the appropriate
error.
rdar://152454571
I am doing this so I can mark requires as being on a mutable non-Sendable base
from a Sendable value.
I also took this as an opportunity to compress the size of PartitionOp to be 24
bytes instead of 40 bytes.
In terms of the test suite the only difference is that we allow for non-Sendable
types to be returned from nonisolated functions. This is safe due to the rules
of rbi. We do still error when we return non-Sendable functions across isolation
boundaries though.
The reason that I am doing this now is that I am implementing a prototype that
allows for nonisolated functions to inherit isolation from their caller. This
would have required me to implement support both in Sema for results and
arguments in SIL. Rather than implement results in Sema, I just finished the
work of transitioning the result checking out of Sema and into SIL. The actual
prototype will land in a subsequent change.
rdar://127477211
I am doing this since I discovered that we are not printing certain errors as
early as we used to (due to the refactoring I did here), which makes it harder
to see the errors that we are emitting while processing individual instructions
and before we run the actual dataflow.
A nice side-effect of this is that it will make it easy to dump the error in the
debugger rather than having to wait until the point in the code where the normal
logging takes place.
These do not specifically have to do with PartitionUtils... they are really
logging options for the whole infrastructure, so it makes sense to have them in
the a different file.
Specifically:
1. We error now if one transfers an 'inout sending' parameter and does not
reinitialize it before the end of the function.
2. We error now if one merges an 'inout sending' parameter into an actor
isolated region and do not reinitialize it with a non-actor isolated value
before the end of the function.
rdar://126303739
Although I don't plan to bring over new assertions wholesale
into the current qualification branch, it's entirely possible
that various minor changes in main will use the new assertions;
having this basic support in the release branch will simplify that.
(This is why I'm adding the includes as a separate pass from
rewriting the individual assertions)
DISCUSSION: The analysis itself is unable to emit errors. So we achieve the same
functionality by in such cases emitting a partition op that signals to our user
that when they process that partition op they should emit an "unknown pattern"
error at the partition op's instructions.
I have wanted this for a long time, but I never got around to it.
We still only parse transferring... but this sets us up for adding the new
'sending' syntax by first validating that this internal change does not mess up
the current transferring impl since we want both to keep working for now.
rdar://128216574
This ensures that we can properly compute isolation for generic types that
conform to AnyActor.
I found this by playing with test cases from the previous commit. We would not
find an actor type for the actor instance isolation and would fall back along an
incorrect path.
rdar://128021548
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
The thinko was that rather than use the actual isolated parameter of a partial apply as the isolated parameter, I instead just used the last operand... :doh:. This fixes:
rdar://126297097
This is backing out an approach that I thought would be superior, but ended up
causing problems.
Originally, we mapped a region number to an immutable pointer set containing
Operand * where the region was tranferred. This worked great for a time... until
I began to need to propagate other information from the transferring code in the
analysis to the actual diagnostic emitter.
To be able to do that, my thought was to make a wrapper type around Operand
called TransferringOperand that contained the operand and the other information
I needed. This seemed to provide me what I wanted but I later found that since
the immutable pointer set was tracking TransferringOperands which were always
newly wrapped with an Operand *, we actually always created new pointer
sets. This is of course wasteful from a memory perspective, but also prevents me
from tracking transferring operand sets during the dataflow since we would never
converge.
In this commit, I fix that issue by again tracking just an Operand * in the
TransferringOperandSet and instead map each operand to a state structure which
we merge dataflow state into whenever we visit it. This provides us with
everything we need to in the next commit to including a region -> transferring
operand set equality check in our dataflow equations and always converge.
This ensures that we can efficiently iterate over the map which we will need to
do for equality queries.
I am going to add the equality queries in a subsequent commit. Just chopping off
a larger commit.
Specifically:
1. I copy the history that we have been tracking from the transferring operand
value at the transfer point. This is then available for use to emit diagnostics.
2. I added the ability for SILIsolationInfo to not only track the ActorIsolation
of an actor isolated value, but also if we have a value, we can track that as
well. Since we now track a value for task isolated and actor isolated
SILIsolationInfo, I just renamed the field to isolatedValue and moved it out of
the enum.
In a subsequent commit, I am going to wire it up to a few diagnostics.
rdar://123479934
We package all isolation history nodes from a single instruction by placing a
sequence boundary at the bottom. When ever we pop, we actually pop a PartitionOp
at a time meaning that we pop until we see a SequenceBoundary. Thus the sequence
boundary will always be the last element visited when popping meaning that it is
a convenient place to stick the SILLocation associated with the entire
PartitionOp. As a benefit, there was some unused space in IsolationHistory::Node
for that case since we were not using the std::variant field at all.
This means that I added an IsolationHistory field to Partition. Just upstreaming
the beginning part of this work. I added some unittests to exercise the code as
well. NOTE: This means that I did need to begin tracking an
IsolationHistoryFactory and propagating IsolationHistory in the pass
itself... but we do not use it for anything.
A quick overview of the design.
IsolationHistory is the head of an immutable directed acyclic graph. It is
actually represented as an immutable linked list with a special node that ties
in extra children nodes. The users of the information are expected to get a
SmallVectorImpl and process those sibling nodes afterwards. The reason why we
use an immutable approach is that it fits well with the problem and saves space
since different partitions could be pointing at the same linked list
node. Operations occur on an isolation history by pushing/popping nodes. It is
assumed that the user will push nodes in batches with a sequence boundary at the
bottom of the addition which signals to stop processing nodes.
Tieing this together, each Partition within it contains an IsolationHistory. As
the PartitionOpEvaluator applies PartitionOps to Partition in
PartitionOpEvaluator::apply, the evaluator also updates the isolation history in
the partition by first pushing a SequenceBoundary node and then pushing nodes
that will undo the operation that it is performing. This information is used by
the method Partition::popHistory. This pops linked list nodes from its history,
performing the operation in reverse until it hits a SequenceBoundary node.
This allows for one to rewind Partition history. And if one stashes an isolation
history as a target, one can even unwind a partition to what its state was at a
specific transfer point or earlier. Once we are at that point, we can begin
going one node back at a time and see when two values that we are searching for
no longer are apart of the same region. That is a place where we want to emit a
diagnostic. We then process until we find for both of our values history points
where they were the immediate reason why the two regions merge.
rdar://123479934
This should be NFC since the only case where I used this was with self... and I
found another way of doing that using the API I added in the previous commit.
ActorIsolation already has a "I have no value case": unspecified. Lets just use
that.
Just a mistake I made that I am trying to fix before anything further depends on
this code.
This issue can come up when a value is initially statically disconnected, but
after we performed dataflow, we discovered that it was actually actor isolated
at the transfer point, implying that we are not actually transferring.
Example:
```swift
@MainActor func testGlobalAndGlobalIsolatedPartialApplyMatch2() {
var ns = (NonSendableKlass(), NonSendableKlass())
// Regions: (ns.0, ns.1), {(mainActorIsolatedGlobal), @MainActor}
ns.0 = mainActorIsolatedGlobal
// Regions: {(ns.0, ns.1, mainActorIsolatedGlobal), @MainActor}
// This is not a transfer since ns is already main actor isolated.
let _ = { @MainActor in
print(ns)
}
useValue(ns)
}
```
To do this, I also added to SILFunction an actor isolation that SILGen puts on
the SILFunction during pre function visitation. We don't print it or serialize
it for now.
rdar://123474616
Just making PartitionUtils.h a little easier to walk through by moving more of
the impl into the .cpp file. This reduces the header from ~1500 lines to ~950
lines which is more managable. This is especially important since I am going
to be adding IsolationHistory to the header file which will expand it even
further.
Long term I would like to get region analysis and transfer non sendable out of
the business of directly interpreting the AST... but if we have to do it now, I
would rather us do it through a helper struct. At least the helper struct can be
modified later to work with additional SIL concurrency support when it is added.
What this does is really split the one dataflow we are performing into two
dataflows we perform at the same time. The first dataflow is the region dataflow
that we already have with transferring never occurring. The second dataflow is a
simple gen/kill dataflow where we gen on a transfer instruction and kill on
AssignFresh. What it tracks are regions where a specific element is transferred
and propagates the region until the element is given a new value. This of course
means that once the dataflow has converged, we have to emit an error not if the
value was transferred, but if any value in its region was transferred.
One needs to pass in the explicit flag to enable this as well as
-debug-flag=send-non-sendable. This makes it easier to debug the affect of
applying specific partition ops.
