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
This just improves the ability to quickly triage bugs in SendNonSendable. It
used to be this way, but in the process of doing some refactoring, I moved the
logging too late by mistake.
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.
We are going to visit all of the blocks anyways... so it makes sense to just run
them as part of the initializing of the block state. Otherwise, the logic is
buried in the dataflow which makes it harder for someone who is not familiar
with the pass to reason about it.
If we treat a begin_access as an assignment then in cases like below we assume
that a value was used before we reassign.
```
func testVarReassignStopActorDerived() async {
var closure = {}
await transferToMain(closure)
// This re-assignment shouldn't error.
closure = {} // (1)
}
```
rdar://117437299
We only ever used this cache of IDs to construct the function arg
partition. Since a Partition involves using memory, it makes sense to just cache
the Partition itself and eliminate the unnecessary second cache.
Semantically these are the only cases where we would want to force an address to
be merged. Some notes:
1. When we have a box, the project_box is considered to come from the box and is
considered non uniquely identified.
2. The only place this was really triggering was when we created temporary
partial applies to pass to a function when using address only types. Since this
temporaries are never reassigned to, there isn't any reason to require them to
be merged.
3. In the case where we have an alloc_stack grabbed by a partial_apply, we still
appropriately merge.
Importantly, we determine at the error stage if a specific value that is
transferred is within the same region of a value that is Actor derived. This
means that we can in a flow insensitive manner determine the values that are
actor derived and then via propagating those values into various regions
determine the issue... using our region analysis to handle the flow sensitivity.
One important case to reason about here is that since we are relying on the
region propagation for flow sensitivity is that this solves the var case for
us. A var when declared is never marked as actor derived. Var uses only become
actor derived if the var was merged into a region that contain is actor
derived. That means that re-assigning to a non-actor derived value, eliminates
the actor derived bit.
As part of this, I also discovered I could just get rid of the captured uniquely
identified array in favor of just passing in an actor derived flag.
rdar://115656589
In a future commit, I think I am going to change this to use a
SmallFrozenMultiMap so we can avoid the heap cost in most cases. For now though
I am just changing it to a std::multimap to make it cleaner and avoid having to
reason about when to freeze the SmallFrozenMultiMap.
This indirection is premature abstraction since the only two places we actually
use these callbacks is to print the accumulated output and to emit our
errors. With this change, I was able to simplify how print works and inline in
the actual diagnostic emission making it easier to understand the code.
Additionally, in a subsequent commit I am going to need to ensure that we do not
emit an error here if the source cause of our error is due to an actor. To
implement that I would need to change tryDiagnoseAsCallsite to signal to the
machinery that it needs to not emit requires and then fail further up as
well. Given that there really wasn't a need for this abstraction in the first
place and the amount of munging needed to express this, I decided to just make
this change and make it easier. If we truly need this flexibility, we can always
add it back later.
Transfer is the terminology that we are using for something be transferred
across an isolation boundary, not consume. This also eliminates confusion with
consume which is a term being used around ownership.
I did this by introducing a builder construct that has a SmallVector within it
that we append into instead of returning std::vector. I also used some passed in
SmallVectors in other places as well with large enough sizes that most times we
will not allocate.
Previously, we were not recognizing that a ref_element_addr from an actor object
is equivalent to the actor object and we shouldn't allow for it to be consumed.
rdar://115132118
This is a case of an instruction that we missed translating into the pseudo-IR.
This was easy to do since we are not yet performing an exhaustive switch over
all instruction kinds when translating. Earlier I did the first part of the
translation by switching from doing conditional is<INST> to switch but haven't
yet eliminated the default yet from the switch. I am fixing this now since with
my load [copy] change from earlier causes us to crash in the earlier unorganized
tests.
rdar://115367810
Really, we should just be using representative values here in general since it
serves the same purpose and makes it easier to trace back values. But this in
the short term makes the output easier to reason about.
Specifically:
1. Converted llvm::errs() -> llvm::dbgs() when using LLVM_DEBUG.
2. Converted a dump method on errs to be a print method on dbgs that is called from dump with print(llvm::dbgs()).
This ensures that we actually handle all instructions. I already found that we
are not handling ~106 cases... but to make this just a refactoring patch, I just
put in a default + a break + left in the error.
rdar://115367810
Previously before this patch, the logic for constructing TrackableSILValue was
split inbetween TrackableSILValue and the PartitionOpTranslator. A lot of
routines were also just utility routines that did not depend on the state in
either of the constructs.
With that in mind in this patch I:
1. Moved the utility functions that did not depend on internal state of those
functions into utility functions in the utility section.
2. Moved the logic in TrackableSILValue's constructor that depended on state
inside PartitionOpTranslator into PartitionOpTranslator itself.
3. Made TrackableSILValue a simple POD type used for storing state.
4. I changed capturedUIValues to just store within it a SILValue. It actually
does not care about isAliased or isSendable... it is just used as a way to check
if we have a uniquely identified value. In a forthcoming patch, I am going to
try to do similar things with the other maps that use TrackableSILValues since
by making the TrackableSILValues our keys/sets it allows for us to potentially
store multiple versions of the same TrackableSILValues if their aliased/sendable
state differ... which would lead to bugs.
Specifically, the two routines we were importing relatively were:
1. TypeChecker::conformsToProtocol. I moved this onto a helper routine on
SILType.
2. swift::findOriginalValueType(Expr *). This routine just looks through various
implicit conversions to find the original underlying type. I moved it to a
helper method on Expr.
