We are creating/relying on a contract between the AST and SIL... that SILDeclRef
should accurately describe the method/accessor that a class_method is from. By
doing this we eliminate pattern matching on the AST which ties this code too
tightly to the AST and makes it brittle in the face of AST changes. This also
fixes an issue where we were not handling setters correctly.
I am doing this now since it is natural to fix it along side fixing the
ref_element_addr issue in the previous commit since they are effectively doing
the same thing.
rdar://153207557
The two pieces of code are fundamentally doing the same thing so I can reuse the
code. I am doing the refactoring as a separate change so that it is easier to
review.
Specifically given a nominal type like the following:
```swift
@MainActor
struct Foo {
@CustomActor var ns: NonSendableKlass
}
```
the isolation for ns should be CustomActor not MainActor.
rdar://160603379
The previous commit in this PR exposed that we were not handling this correctly.
Specifically, we incorrectly started to error in SwiftFoundation.
rdar://162629359
Specifically, this code was added because otherwise we would in swift 5 +
strict-concurrency mode emit two warnings, one at the AST level and one at the
SIL level. Once we are in swift-6 mode, this does not happen since we stop
compiling at the AST level since we will emit the AST level diagnostic as an
error.
To do this, we tried to pattern match what the AST was erroring upon and treat
the parameter as disconnected instead of being isolated. Sadly, this resulted in
us treating certain closure cases incorrectly and not emit a diagnostic
(creating a concurrency hole).
Given that this behavior results in a bad diagnostic only to avoid emitting two
diagnostics in a mode which is not going to last forever... it really doesn't
make sense to keep it. We really need a better way to handle these sorts of
issues. Perhaps a special semantic parameter put on the function that squelches
certain errors. But that is something for another day. The specific case it
messes up is:
```
class NonSendable {
func action() async {}
}
@MainActor
final class Foo {
let value = NonSendable()
func perform() {
Task { [value] in
await value.action() // Should emit error but do not.
}
}
}
```
In this case, we think that value is sending... when it isnt and we should emit
an error.
rdar://146378329
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
Centralize the logic for figuring out the conformances for the various
init_existential* instructions in a SILIsolationInfo static method, and
always go through that when handling "assign" semantics. This way, we
can use CONSTANT_TRANSLATION again for these instructions, or a simpler
decision process between Assign and LookThrough.
The actually undoes a small change made earlier when we stopped looking
through `init_existential_value` instructions. Now we do when there are
no isolated conformances.
Better match the style of SILIsolationInfo by moving the code for determining
SILIsolationInfo from conformances or dynamic casts to existentials into
static `getXYZ` methods on SILIsolationInfo.
Other than adding an assertion regarding disconnected regions, no
intended functionality change.
When we introduce isolation due to a (potential) isolated conformance,
keep track of the protocol to which the conformance could be
introduced. Use this information for two reasons:
1. Downgrade the error to a warning in Swift < 7, because we are newly
diagnosing these
2. Add a note indicating where the isolated conformance could be introduced.
Specifically in terms of printing, if NonisolatedNonsendingByDefault is enabled,
we print out things as nonisolated/task-isolated and @concurrent/@concurrent
task-isolated. If said feature is disabled, we print out things as
nonisolated(nonsending)/nonisolated(nonsending) task-isolated and
nonisolated/task-isolated. This ensures in the default case, diagnostics do not
change and we always print out things to match the expected meaning of
nonisolated depending on the mode.
I also updated the tests as appropriate/added some more tests/added to the
SendNonSendable education notes information about this.
I am doing this so that I can change how we emit the diagnostics just for
SendNonSendable depending on if NonisolatedNonsendingByDefault is enabled
without touching the rest of the compiler.
This does not actually change any of the actual output though.
We were effectively working around this previously at the SIL level. This caused
us not to obey the semantics of the actual evolution proposal. As an example of
this, in the following, x should not be considered main actor isolated:
```swift
nonisolated(nonsending) func useValue<T>(_ t: T) async {}
@MainActor func test() async {
let x = NS()
await useValue(x)
print(x)
}
```
we should just consider this to be a merge and since useValue does not have any
MainActor isolated parameters, x should not be main actor isolated and we should
not emit an error here.
I also fixed a separate issue where we were allowing for parameters of
nonisolated(nonsending) functions to be passed to @concurrent functions. We
cannot allow for this to happen since the nonisolated(nonsending) parameters
/could/ be actor isolated. Of course, we have lost that static information at
this point so we cannot allow for it. Given that we have the actual dynamic
actor isolation information, we could dynamically allow for the parameters to be
passed... but that is something that is speculative and is definitely outside of
the scope of this patch.
rdar://154139237
In this case, what is happening is that in SILGen, we insert implicit
DistributedActor.asLocalActor calls to convert a distributed actor to its local
any Actor typed form. The intention is that the actor parameter and result are
considered the same... but there is nothing at the SIL level to enforce that. In
this commit, I change ActorInstance (the utility that defines actor identity at
a value level) to look through such a call.
I implemented this by just recognizing the decl directly. We already do this in
parts of SILGen, so I don't really see a problem with doing this. It also
provides a nice benefit that we do not have to modify SILFunctionType to
represent this or put a @_semantic attribute on the getter.
NOTE: Generally, Sema prevents us from mixing together different actors. In this
case, Sema does not help us since this call is inserted implicitly by the
distributed actor implementation in SILGen. So this is not a problem in general.
rdar://152436817
Otherwise, we can be inconsistent with isolations returned by other parts of the
code. Previously we were just treating it always as self + nom decl, which is
clearly wrong if a type is not self (e.x.: if it is an isolated parameter).
rdar://135459885
This also fixes a case where we would have inferred the wrong isolation (which
the assert caught). I think we missed this since it only comes up with final
classes.
rdar://142568522
Introduce a new experimental feature StrictSendableMetatypes that stops
treating all metatypes as `Sendable`. Instead, metatypes of generic
parameters and existentials are only considered Sendable if their
corresponding instance types are guaranteed to be Sendable.
Start with enforcing this property within region isolation. Track
metatype creation instructions and put them in the task's isolation
domain, so that transferring them into another isolation domain
produces a diagnostic. As an example:
func f<T: P>(_: T.Type) {
let x: P.Type = T.self
Task.detached {
x.someStaticMethod() // oops, T.Type is not Sendable
}
}
I did the correct thing for indirect parameters, but did not do the correct
thing for direct parameters. This is now fixed with some tests to boot.
rdar://141631655
This makes it so that one does not need to deal with the differences in text in
between the task isolated case and the actor isolated case. This is done by
swallowing the entire part of this message in one method rather than having the
caller do the work.
The reason why is that we want to distinguish inbetween SILFunction's that are
marked as unspecified by SILGen and those that are parsed from textual SIL that
do not have any specified isolation. This will make it easier to write nice
FileCheck tests against SILGen output on what is the inferred isolation for
various items.
NFCI.
Also, move this rule from the computation of lowered captures in SIL,
to the computation of AST captures in Sema. This allows us to
correctly handle the case where an async function nests inside a
sync function. It also removes a special case that was added recently
to cope with a generic `self` type.
Fixes rdar://129366819.
The reason that I am changing this code is that getWithIsolationCrossing is a
bad API that was being used to infer actor isolation straight from an ApplyExpr
without adding an actor instance. This can cause us to reject programs
unnecessarily if we in other parts of the code correctly infer the SILValue
actor instance for the isolation.
Rather than allow for that, I am removing this code and I improved the rest of
the pattern matching here to ensure that we handled that with the normal actor
instance inferring code. This will prevent this type of mismerge from happening
by mistake. I fixed up the changes in the test cases.
The only usage of this left is for ApplyIsolationCrossings parsed straight from
SIL that we use only when testing. This is safe since if a test writer is using
the parsed SIL in this manner, they can make sure that mismerges do not happen.
Closures generally only inherit actor instance isolation if they directly
capture state from the actor instance. In this case, for some reason that is not
true, so we hit an assert that assumes that we will only see a global actor
isolated isolation.
Region Isolation should be able to handle code even if the closure isolation
invariant is violated by the frontend. So to do this, I am introducing a new
singleton actor instance to represent the isolation of a defer or closure
created in an actor instance isolated method. The reason why I am using a
singleton is that closures and defer are not methods so we do not actually know
which parameter is 'self' since it isn't in the abi. But we still need some
value to represent the captured values as belonging to. To square this circle, I
just did what we have done in a similar situation where we did not have a value:
(ActorAccessorInit). In that case, we just use a sentinel to represent the
instance (NOTE: This is represented just via a kind so ActorInstances that are
operator== equal will not &value equal since we are just using a kind).
Otherwise, in cases like the following, we look through the load to x.boolean
and think that the closure is actually capturing x instead of y:
```swift
func testBooleanCapture(_ x: inout NonSendableKlass) {
let y = x.boolean
Task.detached { @MainActor [z = y] in
print(z)
}
}
```
rdar://131369987
We are already using this routine in other parts of TransferNonSendable to
ensure that we look through common insts that SILGen inserts that do not change
the actual underlying actor instance that we are using. In this case, I added
support for casts, optional formation, optional extraction, existential ref
initialization.
As an example of where this came up is the following test case where we fail to
look through an init_existential_ref.
```swift
public actor MyActor {
private var intDict: [Int: Int] = [:]
public func test() async {
await withTaskGroup(of: Void.self) { taskGroup in
for (_, _) in intDict {}
await taskGroup.waitForAll() // Isolation merge failure happens here
}
}
}
```
I also added the ability to at the SIL level actual test out this merge
condition using the analysis test runner. I used this to validate that this
functionality works as expected in a precise way.
rdar://130113744
Before we wouldn't print them in all situations and even more so a few of the
printing routines did not have it at all. This just adds a centralized
SILIsolationInfo::dumpOptions() method and then goes through all of the printing
helpers and changes them to use them as appropriate.
Given a function or a partial_apply with an isolated parameter, we do not know
immediately what the actual isolation is of the function or partial_apply since
we do not know which instance will be applied to the function or partial_apply.
In this commit, I introduce a new bit into SILIsolationInfo that tracks this
information upon construction and allows for it to merge with ownership that has
the appropriate type and a specific instance. Since the values that created the
two isolations, will be in the same region this should ensure that the value is
only ever in a flow sensitive manner in a region with only one actor instance
(since regions with isolations with differing actor instances are illegal).
Before this change in the following code, we would say that message is isolated to the actor instead of the global actor isolation of the actor's method:
```swift
class Message { ... }
actor MessageHolder {
@MainActor func hold(_ message: Message) { ... }
}
@MainActor
func sendMessage() async {
let messageHolder = MessageHolder()
let message = Message()
// We identified messageHolder.hold as being MessageHolder isolated
// instead of main actor isolated.
messageHolder.hold(message)
Task { @MainActor in print(message) }
}
```
I also used this as an opportunity to simplify the logic in this part of the
code. Specifically, I had made it so that multiple interesting cases were
handled in the same conditional statement in a manner that it made it hard to
know which cases were actually being handled and why it was correct. Now I split
that into two separate if statements with comments that make it clear what we
are actually trying to pattern match against.
rdar://130980933
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
