A non-Sendable value projected directly out of a Sendable access base
(e.g. reading the non-Sendable field of an @unchecked Sendable struct
stored on an actor) was unconditionally inheriting the actor-instance
isolation of the underlying access base in
RegionAnalysisValueMap::getTrackableValueHelper. Because the value is
reached *through* a Sendable value, it should remain disconnected, not
actor-isolated.
Concretely, given:
class Obj: Equatable { static func ==(lhs: Obj, rhs: Obj) -> Bool { true } }
struct W: @unchecked Sendable { let v: Obj }
actor A { let w = W(v: Obj()) }
extension A: Equatable {
static func ==(lhs: A, rhs: A) -> Bool { lhs.w.v == rhs.w.v }
}
the projected addresses 'lhs.w.v' and 'rhs.w.v' were inferred as
'lhs'-isolated and 'rhs'-isolated, and merging them at the Obj.== apply
produced a spurious cross-isolation region-merge warning on valid code.
llvm::cl::opt flags are compiled out in non-asserts builds, making these
debug flags unavailable in an important category of use cases — debugging
a release compiler in lldb. By promoting them to Swift frontend flags stored
in DiagnosticOptions/SILOptions, they are available in all build
configurations.
The three existing flags are migrated:
-diagnostics-assert-on-error
-diagnostics-assert-on-warning
-sil-region-isolation-assert-on-unknown-pattern
(backing field renamed to AbortOnUnknownRegionIsolationPatternError)
A new flag is added:
-diagnostics-assert-on-group <group>
Traps when any diagnostic belonging to the named group is emitted,
allowing targeted breakpoints on a single diagnostic group rather than
all errors or all warnings.
The assert-on-{error,warning,group} flags are intentionally kept separate
from the normal diagnostic suppression/escalation machinery so that they
remain useful while other diagnostics are also being emitted.
Tests are added for all four flags.
A closure that captures a `@dynamic_self` metatype produces a
`partial_apply` with `self` appearing as a type-defs operand. The region
analysis was iterating `pai->getAllOperands()` and feeding every operand
to `translateSILMultiAssign`, which then merged `self`'s region with the
PA's region even though no runtime value of `self` is captured.
For `sending`-parameter closures this manifested as a bogus "pattern
that the region-based isolation checker does not understand how to
check" diagnostic, because the spurious merge violated the
region-disjoint invariant required by `sending`.
Switch to `ApplySite(pai).getArgumentOperands()`, matching
`translateSILPartialApplyAsyncLetBegin` and
`translateIsolatedPartialApply`, which already use that idiom and so
were never affected. The apply-site code in
`translateNonIsolationCrossingSILApply` likewise filters
`isTypeDependent()` operands.
When init_existential_addr has a formal concrete type involving an opened
archetype (e.g., the iterator type in a nonisolated actor init that iterates
over any Sequence<Int>), the AssignDirect path calls getConformanceIsolation()
and introduces a task-isolated isolated-conformance region. In a nonisolated
actor init, parameters are 'self'-isolated, and merging 'self'-isolated with
task-isolated-conformance triggers a RegionIsolationUnknownPattern false
positive ("pattern that the region-based isolation checker does not understand
how to check").
The reproducer is:
actor Foo {
init(sequence: any Sequence<Int>) {
for element in sequence { // error: pattern that the region-based
_ = element // isolation checker does not understand
}
}
}
Treating the conformance as an independent task-isolated conformance is
incorrect. According to SE-0470, the iterator's conformance to IteratorProtocol
is a dependent conformance on the conformance to Sequence which implies that the
former conformance must be isolated to the same isolation domain as the latter
conformance.
The fix detects this situation by calling
getFormalConcreteType()->hasOpenedExistential() and calls
translateSILMultiAssign over all operands (primary + type-dependent) without a
conformance isolation override. This places the init_existential_addr in the
same region as its type-dependent operand producing value (the
open_existential_addr), reflecting the dependent conformance relationship.
Found the same issue in init_existential_ref (class-bound existentials) and
init_existential_value (opaque-values mode) and applied the same fix.
Tests cover all three instruction variants at the Swift level and as partition
op translation SIL tests.
rdar://176882987
We cannot use spare bits or other overlapping storage layout tricks with fundamentally
address-only enums, and we can take advantage of this to do borrowing switches or other
in-place projections without copying the value. However, for resilient enums, the
implementation may use spare bit packing, but the type must be handled address-only
outside of its defining module, and we didn't have a way to express that with
borrowing switch. Optimization passes have also been running into problems with the
complexity that we were using `unchecked_take_enum_data_addr` sometimes as a pure
operation. This patch splits the instruction into three:
- `unchecked_inplace_enum_data_addr` represents a nondestructive in-place enum
projection. It is only allowed for enums whose projection operation is
nondestructive.
- `unchecked_take_enum_data_addr` represents a destructive enum projection,
invalidating the enum and leaving the payload to be further consumed.
This matches the current instruction's semantics.
- `unchecked_borrow_enum_data_addr` represents a borrowing enum projection.
The instruction takes a second operand for "scratch" space, which the
enum representation may be copied into in order to avoid invalidating the
enum value, so the result is dependent on the lifetime of both the
original enum and the scratch buffer. This allows for borrowing switches
over resilient enums.
`unchecked_borrow_enum_data_addr` is implemented by taking advantage of the
"address-only enums can't do spare bit optimization" property at runtime.
We inspect the operand type's bitwise-borrowability from its metadata. If
the type is bitwise-borrowable, then we are allowed to bitwise-copy the
enum to the scratch space and apply the projection to the scratch space,
preserving the original value. If the type is not bitwise-borrowable, then
we cannot use spare bit optimization in its layout, so we apply the
projection in-place.
Fixes rdar://174952822.
Specifically, we previously used translateSILMultiAssign with checked_cast_br
and checked_cast_addr_br and relied upon its behavior around injecting an
overriding isolation to work.
This caused a bunch of problems when dealing with isolated conformances
introduced by checked_cast_br and checked_cast_addr_br.
In this commit, I fix these issues by:
1. Making it so that we actually represent in SILIsolationInfo that a phi
argument from a checked_cast_br can have disconnected or an isolated conformance
isolation. We previously just returned an invalid isolation. This worked since
we took advantage of a quirk in translateSILMultiAssign that overrode the
isolation of certain results. This quirk breaks the abstraction boundary that
SILIsolationInfo decides the isolation of elements and RegionAnalysis just
propagates those around. I want to remove this behavior but that needs to occur
in a future commit since I am trying to be more surgical with this change.
2. I changed checked_cast_br and checked_cast_addr_br to not use
translateSILMultiAssign so that I have more control and can avoid the behavior
quirk. This had a nice benefit that we now properly represent for
checked_cast_br that the merge in between the parent value and the cast value
only happens on the success path.
3. I added tests for isolated conformances and casting to give us more
confidence that the behavior is correct. We previously had pretty minimal tests.
4. I added partition op translation and silisolation info tests for these two
casting instructions to increase code coverage. I want to increase it more, but
this is a good first step.
rdar://172941821
Specifically, previously if we had a non-Sendable field in a non-Sendable parent
class, we would perform an assign direct. This is not safe anymore since we are
being much more stringent about mismerges.
Instead, the correct thing to do is if the field has a differing explicit
isolation (not its region isolation, but the isolation of the element) from its
parent nominal type, we require the parent operand type and assign fresh the
ref_element_addr. This ensures that they are in different regions and we do not
have the same region with differing isolations within it.
The reason why this is safe is that The field will be isolated to whatever
actor isolation it has at the AST level. So any time we escape it or send
it, we will properly get an error. Any other ref_element_addr to the same
field will be a different region, but that doesn't matter since they all
will still be isolated ot the same actor isolation meaning merges will not
cause problems.
I am going to be fixing a similar issue with structs but it is more complicated,
so I thought I would fix this now.
Introduce the IncompatibleRegionMerge partition op error type to detect merges
between regions with incompatible isolation.
Previously, these merge failures produced generic "unknown pattern" errors. Now
we detect them during partition evaluation and emit context-specific diagnostics
like "cannot assign X to Y" or "cannot pass X to function Y", explaining the
specific operation that caused the isolation conflict.
This is part of a larger effort to move isolation-related diagnostics from the
AST level to RegionIsolation at the SIL level. This is important because it
enables flow isolation to support global actor isolated nominal types.
NOTE: We are going to emit errors that we are not going to emit eventually. I am
just updating so I can track that they are all resolved.
Restructure the isolation info lattice from a linear progression to a
diamond-shaped lattice that properly models merge failures:
Old: Unknown -> Disconnected -> Task -> Actor
New: Disconnected ---> Task ---> Invalid
\--> Actor -/
This cleanup prepares the isolation lattice for better error handling when
incompatible regions are merged.
Previously, it was incorrect and not a true lattice and prevented us from
modeling merge errors in between Task and Actor isolation. We need to model this
so that we can properly emit errors when we suppress the AST from emitting these
errors during flow isolation. We are also going to move these sorts of errors
completely from the AST to Region Analysis to simplify things.
NOTE: This causes us to emit some more "unknown pattern" errors. I updated the
tests with that so in the next series of commits, we can validate they all go away.
Complete the transition to the full translateSILMultiAssign signature by
updating all remaining call sites to pass explicit indirect results. We just
pass through mechanically an empty ArrayRef<Operand *>, so nothing semantic is
changing.
Arguably some of these entrypoints should use actual indirect parameters
(looking at use store_borrow), but this at least makes progress.
Previously, region-based isolation was not diagnosing cases where a non-Sendable
value projected from a Sendable base that was MainActor isolated. For example:
```swift
@MainActor
struct MainActorBox {
var value: NonSendableKlass?
mutating func take() -> sending NonSendableKlass {
if let value {
self.value = nil
return value // Should warn: main actor-isolated value cannot be 'sending'
} else {
preconditionFailure("Consumed twice")
}
}
}
```
This was caused by two issues:
1. A logic bug in `AddressBaseComputingVisitor::visitAll` where we overwrote an
already-found Sendable value when recursing. The visitor should only record
the first Sendable value found, not subsequent ones. This caused us to
incorrectly return the `@MainActor` `MainActorBox` as the base instead of
emitting a require for the projected value.
2. struct_element_addr being transparent unconditionally causing us to not even
emit a require at all.
This commit has two parts:
1. I fixed the first two issues by fixing the logic bug and by making
struct_element_addr only transparent if its operand and result are
non-Sendable. I added logic that we have used in other such cases to handle
non-Sendable operand/Sendable result as well as Sendable operand and
non-Sendable result. I then added the same support for tuple_element_addr and
unchecked_take_enum_data_addr since they suffered from a similar problem.
2. Adding the logic to handle variants where the operand was non-Sendable and
the result was Sendable, caused a bunch of tests to break so I had to fix
that.
To fix the broken test cases, I had to make the compiler smarter about how it
was inserting this require. To do this:
1. I checked if the access base of the projection was actually immutable or if
we are an alloc_stack that there was a prefix path in the projection path of
immutable projections that end in the alloc_stack. In such a case, we know
that we do not need to require that the operand is available in the current
isolation domain since we will never write to that piece of memory and once
we have loaded the result from memory, we know that the value is Sendable so
nothing bad can happen.
2. If the access base of the projection was mutable, I used the same logic that
we used for alloc_box that were non-Sendable that stored a Sendable thing by
changing operand to be a `require [mutable_base_of_sending]` on the result of
the projection instead of requiring the projection's operand. This ensures
that we handled important flow sensitive cases.
rdar://169626088
For stores to unaliased, non-aggregate-projected destinations, switch from
translateSILAssignDirect(destValue, src) to translateSILAssignIndirect(dest, src).
This passes the Operand* rather than just the SILValue, enabling proper tracking
of the destination address in region analysis.
First in a series migrating instruction handlers to indirect assignment.
NOTE: I originally thought that pack_element_set would also have this property,
but alloc_pack today is always assumed to be to be MayAlias and thus we emit a
merge. I think this is fine since one can never actually assign over a pack
variable like one can other things (that is I think they are generally just used
for marshalling and the like). If I am wrong, I can just fix it later.
rdar://156024613
https://github.com/swiftlang/swift/issues/83121
Explicitly pass an empty indirect results array to translateSILMultiAssign when
translating partial_apply instructions, since partial_apply does not produce
indirect results. This transitions the call site from the backward-compatible
overload to the full signature introduced in bc8eadad12f.
Add two template overloads of translateSILAssignIndirect that wrap
translateSILMultiAssign for the common case of assigning to a single
indirect parameter destination:
1. A generic template accepting any collection of source operands
2. A specialization for a single source operand
These simplify call sites that only need to handle one destination.
Add a new IndirectResultsRangeTy template parameter and indirectResultAddresses
parameter to translateSILMultiAssign, allowing it to process both direct result
values and indirect result operands.
I also added overload that takes only direct results for backward
compatibility. I am going to get rid of this eventually once we have finished
the transition.
It previously just accepted ArrayRef<TrackableValue>. I am going to be using
this with a transform + concat range in a subsequent change to pass a combined
ArrayRef<TrackableValue> + ArrayRef<std::pair<Operand, TrackableValue>>.second.
Since after address lowering, `Borrow` can remain loadable with a known-
layout address-only referent, we need instructions that handle three
forms:
- borrow and referent are both loadable values
- borrow is a value, but referent is address-only
- borrow and referent are both address-only
begin_dealloc_ref is only lookthrough its first parameter. We want to treat the
other parameter as just a require. This hit an assert in the SIL test cases in
the next commit.
Split the `Assign` partition operation into `AssignDirect` and `AssignIndirect`
to properly handle the case where a value in memory is overwritten.
Previously, when a store instruction overwrote a value in memory (e.g.,
`store %new to [assign] %addr`), we would simply update %addr's region to
match %new's region. This caused us to lose information about the original
value that was in %addr, which could include actor or task isolation info.
For example:
%addr = alloc_stack
store %task_local to [init] %addr // %addr's region is task-isolated
store %new_value to [assign] %addr // Previously: lost task isolation!
Now, `AssignIndirect` creates a new representative value (identified by the
store's destination operand) for the overwritten value and merges it into
the destination's region before reassigning. This preserves the region's
isolation properties.
`AssignDirect` continues to be used for direct results where the SSA value
itself is new (e.g., struct_extract results, pack_element_get addresses).
The actual implementation change for AssignIndirect will come in a follow-up
commit. This commit only refactors code in preparation for that.
This commit systematically replaces all calls to `SILIsolationInfo::isNonSendableType(type, fn)`
and `SILIsolationInfo::isSendableType(type, fn)` with their value-based equivalents
`SILIsolationInfo::isNonSendable(value)` and `SILIsolationInfo::isSendable(value)`.
This refactoring enables more precise Sendability analysis for captured values
in closures, which is a prerequisite for treating inferred-immutable weak
captures as Sendable, a modification I will be making a subsequent commit.
I made the type-based `isSendableType(type, fn)` methods private to prevent
future misuse. The only place where isSendableType was needed to be used outside
of SILIsolationInfo itself was when checking the fields of a box. Rather than
exposing the API for that one purpose, I added two APIs specifically for that
use case.
This instruction can be used to disable ownership verification on it's result and
will be allowed only in raw SIL.
Sometimes SILGen can produce invalid ownership SSA, that cannot be resolved until
mandatory passes run. We have a few ways to piecewise disable verification.
With unchecked_ownership instruction we can provide a uniform way to disable ownership
verification for a value.
This instruction converts Builtin.ImplicitActor to Optional<any Actor>. In the
process of doing so, it masks out the bits we may have stolen from the witness
table pointer of Builtin.ImplicitActor. The bits that we mask out are the bottom
two bits of the top nibble of the TBI space on platforms that support TBI (that
is bit 60,61 on arm64). On platforms that do not support TBI, we just use the
bottom two tagged pointer bits (0,1).
By using an instruction, we avoid having to represent the bitmasking that we are
performing at the SIL level and can instead just make the emission of the
bitmasking an IRGen detail. It also allows us to move detection if we are
compiling for AArch64 to be an IRGen flag instead of a LangOpts flag.
The instruction is a guaranteed forwarding instruction since we want to treat
its result as a borrowed projection from the Builtin.ImplicitActor.
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 previously were not "unsending" inout sending parameters after sending them
so they could not be used again in the caller and could not be forwarded into
other 'inout sending' parameters. While looking at the code I
realized it was pretty obtuse/confusing so I cleaned up the logic and fixed a
few other issues in the process.
Now we follow the following pattern in the non-isolation crossing case:
1. We first require the callee operand.
2. We then merge/require all of the non-explicitly sent parameters.
3. We then through all of the parameters and require/send all of the sending parameters.
4. At the end of processing, we unsend all of the sending parameters that were
'inout sending' parameters.
In the case of isolation crossing applies we:
1. Require all parameters that are not explicitly marked as sending and then
send them all at once. We are really just saving a little work by not merging
them into one large region and then requiring/sending that region once.
2. Then for each sending parameter, we require/send them one by one interleaving
the requires/sends. This ensures that if a value is passed to different
explicitly sending parameters, we get an error.
3. Then once we have finished processing results, we perform an undo send on all
of the 'inout sending' params.
rdar://154440896
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
Michael Gottesman
Ben Cohen
Konrad Malawski
Joe Groff
Michael Gottesman
Meghana Gupta
Janat Baig
Anthony Latsis
Doug Gregor