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.
Add two LLVM statistics to RequireLiveness::process() in SendNonSendable.cpp:
- NumRequireLivenessProcess: counts calls to RequireLiveness::process()
- NumRequireLivenessInstScans: counts instructions scanned in block loops
Add a scale test documenting the known-bad O(N²) behavior:
- N non-Sendable values are each sent to a @MainActor function, producing N errors
- RequireLiveness::process() is called once per error (N times)
- Each call scans the `if cond` successor block, which contains O(N) instructions
- Total instruction scans: O(N²)
The test uses --invert-result to document this as a known regression, not a pass.
When a closure passed to a `sending` parameter captures a
`nonisolated(unsafe)` value, the diagnostic inferrer was incorrectly
including it in the list of non-Sendable captures. This caused the wrong
diagnostic to fire (e.g. "closure captures 'x'" instead of "sending 'y'
risks causing data races") because the inferrer believed the closure had
problematic captures when it did not.
Add an `isUnsafeNonIsolated()` guard in both
`UseAfterSendDiagnosticInferrer::initForSendingPartialApply` and
`SentNeverSendableDiagnosticEmitter::initForSendingPartialApply` so that
`nonisolated(unsafe)` captures are skipped, matching the user's intent
to opt out of isolation checking for those values.
Previously we missed diagnosing inout sending parameters returned through
indirect out results when the result was marked sending. Unlike normal indirect
out parameters which are task-isolated, a sending indirect out parameter is
disconnected. This edge case was missed by our normal handling which only
checked for task-isolated indirect out parameters.
rdar://171853077
SIL functions should always have actor isolation set, otherwise
it could lead to wrong deductions in optimization passes like
`SendNonSendable` or `OptimizeHopToExecutor`.
This is a first step to move isolation assignment into
`SILFunction::create`.
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
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.
FlowIsolation was digging into ASTContext::Diags manually and extracting
SILLocations from SIL constructs. Rather than do this, refactor the diagnostic
helpers I used in SendNonSendable into DiagnosticHelpers.h and use them in both
passes.
I did this by:
1. Standardizing the constructors of diagnostic emitters to take the same
parameters.
2. Changed the names of emitters so that they match the names of errors so I can
use the XMACRO.
3. Changed the various switches that needed to be edited previously for verbatim
and non-verbatim errors to be driven by the XMACROS now. This required the
former two changes to compile successfully.
The result is that adding new errors is much simpler and easier to do.
Add REGIONBASEDISOLATION_LOG statements to all emitUnknownPatternError()
methods in SendNonSendable.cpp to emit:
Emitting Error. DiagnosticEmission Error: Unknown Code Pattern.
Emitter: $EMITTER_NAME
Inst: $INST
Location: $FILE:$LINE
This enables one to easily understand if an unknown code pattern error is coming
from RegionAnalysis itself or from SendNonSendable.
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.
I am doing general work in this area to fix a larger bug. As I brought up tests
for the new larger bug, I noticed some small improvements that I could make at
the same time. In this case, I am porting a diagnostic that already exists for
sending to closures to have a similar form for isolated closures.
The diagnostic occurs when we error on escaping a non-Sendable box that contains
a Sendable var. We would emit an error saying that 'x' (the Sendable value) is
isolated in some way and that is why we are emitting an error. This is of course
incorrect since we are erroring due to the mutable box that contains the
sendable type being mutatable from multiple isolation domains. To see this in
action, we used to emit the following diagnostic in this case:
```swift
func testMutableNoncopyableSendableStructWithEscapingMainActorAsync() {
var x = NoncopyableStructSendable()
x = NoncopyableStructSendable()
let _ = {
escapingAsyncUse { @MainActor in
useValue(x) // expected-error {{sending 'x' risks causing data races}}
// expected-note @-1 {{task-isolated 'x' is captured by a main actor-isolated closure. main actor-isolated uses in closure may race against later nonisolated uses}}
}
}
}
```
After this change, we emit the following diagnostic which emphasizes the
mutability of 'x' and how it is referenceable from multiple concurrency domains.
```
test.swift:48:7: error: sending 'x' risks causing data races [#SendingRisksDataRace]
46 | let _ = {
47 | escapingAsyncUse { @MainActor in
48 | useValue(x)
| |- error: sending 'x' risks causing data races [#SendingRisksDataRace]
| `- note: main actor-isolated closure captures reference to mutable 'x' which remains modifiable by code in the current task
59 | }
```
rdar://166417084
This ensures that in cases like the following:
+func testNoncopyableNonsendableStructWithNonescapingMainActorAsync() {
+ let x = NoncopyableStructNonsendable() <=========
+ let _ = {
+ nonescapingAsyncClosure { @MainActor in
+ useValueNoncopyable(x) // expected-warning {{sending 'x' risks causing data races}}
+ // expected-note @-1 {{task-isolated 'x' is captured by a main actor-isolated closure. main actor-isolated uses in closure may race against later nonisolated uses}}
+ }
+ }
+}
We emit the diagnostic on the use instead of the <=====.
rdar://166347485
I have noticed over time when working on the command line, we often times
highlight too large of an expression due to the locations provided to us by
earlier parts of the compiler. This isn't technically necessary and the
following doesn't look nice... so remove it.
```
test5.swift:171:16: error: sending 'x' risks causing data races [#SendingRisksDataRace]
169 | let _ = {
170 | nonescapingAsyncUse { @MainActor in
171 | _ _ _ _ _u_s_e_V_a_l_u_e_(_x_)
| |- error: sending 'x' risks causing data races [#SendingRisksDataRace]
| `- note: task-isolated 'x' is captured by a main actor-isolated closure. main actor-isolated uses in closure may race against later nonisolated uses
172 | }
173 | }
```
The caller is allowed to assume that the 'inout sending' parameters are not in
the same region on return so can be sent to different isolation domains safely.
To enforce that we have to ensure on return that the two are /actually/ not in
the same region.
rdar://138519484
I am going to be adding support to passes for emitting IsolationHistory behind a
flag. As part of this, we need to store the state of the partition that created
the error when the error is emitted. A partition stores heap memory so it makes
sense to make these types noncopyable types so we just move the heap memory
rather than copying it all over the place.
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
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.
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.
The reason I am doing this is that we have gotten reports about certain test
cases where we are emitting errors about self being captured in isolated
closures where the sourceloc is invalid. The reason why this happened is that
the decl returned by getIsolationCrossing did not have a SourceLoc since self
was being used implicitly.
In this commit I fix that issue by using SIL level information instead of AST
level information. This guarantees that we get an appropriate SourceLoc. As an
additional benefit, this fixed some extant errors where due to some sort of bug
in the AST, we were saying that a value was nonisolated when it was actor
isolated in some of the error msgs.
rdar://151955519
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.
Previously, when we saw any Sendable type and attempted to look up an underlying
tracked value, we just bailed. This caused an issue in situations like the
following where we need to emit an error:
```swift
func test() {
var x = 5
Task.detached { x += 1 }
print(x)
}
```
The problem with the above example is that despite value in x being Sendable,
'x' is actually in a non-Sendable box. We are passing that non-Sendable box into
the detached task by reference causing a race against the read from the
non-Sendable box later in the function. In SE-0414, this is explicitly banned in
the section called "Accessing Sendable fields of non-Sendable types after weak
transferring". In this example, the box is the non-Sendable type and the value
stored in the box is the Sendable field.
To properly represent this, we need to change how the underlying object part of
our layering returns underlying objects and vends TrackableValues to the actual
analysis for addresses. NOTE: We leave the current behavior alone for SIL
objects.
By doing this, in situations like the above, despite have a Sendable value (the
integer), we are able to ensure that we require that the non-Sendable box
containing the integer is not used after we have sent it into the other Task
despite us not actually using the box directly.
Below I describe the representation change in more detail and describe the
various cases here. In this commit, I only change the representation and do not
actually use the new base information. I do that in the next commit to make this
change easier for others to read and review. I made sure that change was NFC by
leaving RegionAnalysis.cpp:727 returning an optional.none if the value found was
a Sendable value.
----
The way we modify the representation is that we instead of just returning a
single TrackedValue return a pair of tracked values, one for the base and one
for the "value". We return this pair in what is labeled a
"TrackableValueLookupResult":
```c++
struct TrackableValueLookupResult {
TrackableValue value;
std::optional<TrackableValue> base;
TrackableValueLookupResult(TrackableValue value)
: value(value), base() {}
TrackableValueLookupResult(TrackableValue value, TrackableValue base)
: value(value), base(base) {}
};
```
In the case where we are accessing a projection path out of a non-Sendable type
that contains all non-Sendable fields, we do not do anything different than we
did previously. We just walk up from use->def until we find the access path base
which we use as the representative of the leaf of the chain and return
TrackableValueLookupResult(access path base).
In the case where we are accessing a Sendable leaf type projected from a
non-Sendable base, we store the leaf type as our value and return the actual
non-Sendable base in TrackableValueLookupResult. Importantly this ensures that
even though our Sendable value will be ignored by the rest of the analysis, the
rest of the analysis will ensure that our base is required if our base is a var
that had been escaped into a closure by reference.
In the case where we are accessing a non-Sendable leaf type projected from a
Sendable type (which we may have continued to be projected subsequently out of
additional Sendable types or a non-Sendable type), we make the last type on the
projection path before the Sendable type, the value of the leaf type. We return
the eventual access path base as our underlying value base. The logic here is
that since we are dealing with access paths, our access path can only consist of
projections into a recursive value type (e.x.: struct/tuple/enum... never a
class). The minute that we hit a pointer or a class, we will no longer be along
the access path since we will be traversing a non-contiguous piece of
memory (consider a class vs the class's storage) and the traversal from use->def
will stop. Thus, we know that there are only two ways we can get a field in that
value type to be Sendable and have a non-Sendable field:
1. The struct can be @unchecked Sendable. In such a case, we want to treat the
leaf field as part of its own disconnected region.
2. The struct can be global actor isolated. In such a case, we want to treat the
leaf field as part of the global actor's region rather than whatever actor.
The reason why we return the eventual access path base as our tracked value base
is that we want to ensure that if the var value had been escaped by reference,
we can require that the var not be sent since we are going to attempt to access
state from the var in order to get the global actor guarded struct that we are
going to attempt to extract our non-Sendable leaf value out of.
Otherwise, when one diagnoses code like the following:
```
Task {
{
use($0)
}(x)
}
```
One gets that $0 was captured instead of x. Unfortunately, since function
parameters do not have locations associated with them, we do not mark x
itself... instead, we mark the closure... which is unfortunate.
The reason why I am doing this is that in certain cases the AST captures indices
will never actually line up with partial apply capture indices since we seem to
"smush" together closures and locally defined functions.
NOTE: The reason for the really small amount of test changes is that this change
does not change the actual output by design. The only cases I had to change were
a case where we began to emit a better diagnostic and also where I added code
coverage around _ and let _ since those require ignored_use to be implemented so
that they would be diagnosed (previously we just did not emit anything so we
couldn't emit the diagnostic at the SIL level).
rdar://142661388
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
Michael Gottesman
Michael Gottesman
Pavel Yaskevich
Pavel Yaskevich
Anthony Latsis
Hamish Knight
Doug Gregor
Anthony
Slava Pestov