This involves me changing where we emit the error. Instead of us emitting the
error early in the evaluator, we emit it later when we emit diagnostics for all
other transferring things.
This is NFCI.
This commit makes it so that we treat values captured by an actor isolated
closure as being transferred to that closure. I also introduced a new diagnostic
for these warnings that puts the main warning on the capture point of the value
so the user is able to see the actual capture that causes the transfer to occur:
```swift
nonisolated func testLocal2() async {
let l = NonSendableKlass()
// This is not safe since we use l later.
self.assumeIsolated { isolatedSelf in
isolatedSelf.ns = l
}
useValue(l) // expected-note {{access here could race}}
}
```
```
test.swift:74:14: warning: main actor-isolated closure captures value of non-Sendable type 'NonSendableKlass' from nonisolated context; later accesses to value could race
useValue(x) // expected-warning {{main actor-isolated closure captures value of non-Sendable type 'NonSendableKlass' from nonisolated context; later accesses to value could race}}
^
test.swift:76:12: note: access here could race
useValue(x) // expected-note {{access here could race}}
^
```
One thing to keep in mind is that if we have a function argument being captured
in this way, we still emit the "call site passes `self`" error. I am going to
begin cleaning that up in the next commit in this PR so that we emit a better
error here. But it makes sense to split these into two separate commits since
they are doing different things.
rdar://121345525
Before the previous patch, we were just getting lucky on macOS due to UB. Now
that the UB is fixed, we correctly crash without this commit since we were not
pattern matching the simple case of a local value that was transferred and used
later.
llvm::Optional<T> used to make it so that in asserts builds if one dereferenced the optional and nothing was there, one would get an assert. std::optional<T> does not have that property.
The specific semantics is if we assign into a transferring parameter's field,
then we "merge" src's value into the transferring parameter, so we
conservatively leave the region of the transferring parameter alone. If we
assign over the entire transferring parameter, we perform an assign fresh since
any value that used to be in the transferring parameter cannot reference
anything in its new value since they are all gone.
In the fullness of time, AddressLowering should never see such
instructions because they will be eliminated after diagnostics. Until
that time, though they need to be handled.
Before this commit, this was done at the beginning of TransferNonSendable. I
thought that those checks would be sufficient to ensure that
RegionAnalysisFunctionInfo was not created for functions that we do not
support. Turns out when we perform certain forms of verification, we force all
function analyses to be created for all functions meaning that we would create a
RegionAnalysisFunctionInfo for such an unsupported function causing us to hit
asserts.
In this commit, I move the check to whether or not we support a function into
RegionAnalysisFunctionInfo itself and use that to determine if we should run
TransferNonSendable. This additionally allows me to change
RegionAnalysisFunctionInfo so that one can construct one for an unsupported
function... as long as one doesn't actually touch any of its methods. If one
does, I put in an assert so we will know that operator error has occured.
NFCI. This is just a pure refactor of the analysis part of TransferNonSendable
into a separate SIL level analysis so it can be reused by other passes.
The reason that I am committing this earlier is that I am working concurrently
on other patches that change TransferNonSendable itself and I want to avoid
issues when rebasing those patches. Getting this patch into tree earlier avoids
that.
This is in preparation for adding a new flow sensitive initialization pass that
combines region based analysis with the current flow sensitive isolation's
diagnostic emitter. The idea is that we want to preserve the diagnostics from
that pass rather than try to make our own as an initial step.
in LowerHopToActor.
In order to project an optional executor value from an optional actor value,
the executor lowering needs to modify the CFG. This is done by splitting
LowerHopToActor into two passes. The first pass records all actor operands
of hop_to_executor and extract_executor instructions and records the
dominating instruction that will derive the executor value. The second pass
iterates over the multi-map of dominating instructions, derives the executor
value, and rewrites the operands of all reachable hop_to_executor and
extract_executor instructions to reuse that executor value.
The dependent 'value' may be marked 'nonescaping', which guarantees that the
lifetime dependence is statically enforceable. In this case, the compiler
must be able to follow all values forwarded from the dependent 'value', and
recognize all final (non-forwarded, non-escaping) use points. This implies
that `findPointerEscape` is false. A diagnostic pass checks that the
incoming SIL to verify that these use points are all initially within the
'base' lifetime. Regular 'mark_dependence' semantics ensure that
optimizations cannot violate the lifetime dependence after diagnostics.
NOTE: This required me to stop using swift::getUnderlyingObject from
getUnderlyingTrackedObject since when it stripsCasts it looks through
unchecked_trivial_bit_cast... but we only want to do that if both the operand
and result of the instruction are non-Sendable. To fix this I inlined
getUnderlyingObject's impl and removed that part of stripCasts.
NOTE: I am just adding coverage that we support these instructions. One can only
use this with Error today and Error is always Sendable. So this is just going
for completeness.
Semantically a mark_dependence returns a value that is equal to its first
parameter with the extra semantics that any destroys of the 2nd operand cannot
occur before any uses of the result of the instruction. From a region
perspective this suggests that the instruction should be an assign from the
first operand onto the result and act as a require on the result. Semantically
the requirement that the 2nd operand cannot be destroyed before any uses of the
result does not expose any memory or state from the first operand implying that
we don't need to merge it into the result region. The restriction is purely to
tell the optimizer what it can/cannot do rather.
We already only supported Ownership SSA since we run early in the pipeline
before OSSA is lowered. This just formalizes this behavior. I am marking these
instructions as Asserting (even though we will never see them) so I can
semantically be sure that all of the instructions are covered without using an
"unsupported" like moniker that I fear will lead to new instructions being added
as unsupported. Better to have a semantic thing for new instruction adders to
use.
This involved fixing a few different bugs.
1. We were just performing dataflow by setting that only the initial block needs
to be updated. This means that if there isn’t anything in the initial dataflow
block, we won’t visit any successor blocks. Instead the correct thing to do here
is to visit all blocks in the initial round.
2. I also needed to fix a separate issue where we were updating our union-find
data structure incorrectly as found by an assert on transfernonsendable.swift
that was triggered once I fixed 1. Put simply, we needed to set a new max label
+ 1 when our new max element is less than or equal to the old max label + 1…
before we just did less than so if we had a new max element that is the same as
our fresh label, we wouldn’t increment the fresh label.
rdar://119584497
Mark the result of a move-only addressor as unresolved. The pointed-at value
cannot be consumed so ensure that only [read] or [modify] accesses are
performed. Update the move-only checker to recognize code patterns
from addressors.
