Just trying to improve logging to speed up triaging further. This is useful so
that I can quickly find specific closures we process by using the closure
numbering (e.x.: closure #1 in XXXX).
In this part of the code, we are attempting to merge all of the operands into
the same region and then assigning all non-Sendable results of the function to
that same region. The problem that was occuring here was a thinko due to the
control flow of the code here not separating nicely the case of whether or not
we had operands or not. Previously this did not matter, since we just used the
first result in such a case... but since we changed to assign to the first
operand element in some cases, it matters now. To fix this, I split the confused
logic into two different easy to follow control paths... one if we have operands
and one where we do not have an operand. In the case where we have a first
operand, we merge our elements into its region. If we do not have any operands,
then we just perform one large region assign fresh.
This was not exposed by code that used non-coroutines since in SIL only
coroutines today have multiple results.
rdar://132767643
We have found certain cases due to the requestified typechecker, a type is
initially Sendable and then is later non-Sendable. This can be seen by the
attached test case where the first time one calls isNonSendableType on the test
value, one would get that it is Sendable and then the second time one would get
it was non-Sendable. The result of this is that the pass gets into an
inconsistent state.
This patch is a small patch that makes the pass more permissive in the face of
such an error by making it so that we do not ignore Sendable results of
instructions (that is we make sure to track a value for them), so we do not
break invariants.
The longer term better fix is to make it so that we have a cache in the pass for
this query that way we just always use the first answer returned from the
typechecker and cache that. If the typechecker has such a bug, we may get bogus
results, but we at least do not break invariants.
As an example of this type of behavior, in the test case in this patch, we first
find the Sendable conformance of MySubClass and then the typechecker after doing
some more type checking while performing that query, the second time finds the
inherited non-Sendable conformance of MyParentClass causing MySubClass to be
considered to be non-Sendable.
rdar://132347404
This will let me know the exact source operand used instead of the source value
representative. This will ensure that the name associated with the diagnostic is
not of the representative value, but the actual value that was the source of the
assign.
This is an NFCI commit that is an algebraic refactor.
This is just moving up the declaration in the chain of dependencies so that I
can write logic in PartitionUtils.h using it. I also added entrypoints to lookup
the ReprensetativeValue for our various emitters.
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
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).
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
This asserts only option is an option to make it quicker/easier to triage
unknown pattern match errors by aborting when we emit it (allowing one to
immediately drop into the debugger at that point).
Previously, it only happened for errors in RegionAnalysis not in
TransferNonSendable itself.
Otherwise, we will have differing isolation from other parameters since
the isolations will look different since one will have the .none value
as an instance and the other will not have one and instead will rely on
the AST isolation info. That is the correct behavior here since we do
not actually have an actor here.
I also removed some undefined behavior in the merging code. The way the
code should work is that we should check if the merge fails and in such
a case emit an unknown pattern error... instead of not checking
appropriately on the next iteration and hitting undefined behavior.
rdar://130396399
TLDR:
The reason why I am doing this is it ensures that temporary store_borrow that we
create when materializing a value before were treated as uses. So we would error
on this:
```swift
@MainActor func transferToMain<T>(_ t: T) async {}
func test() async {
let x = NonSendableKlass()
await transferToMain(x)
await transferToMain(x)
}
```
----
store_borrow is an instruction intended to be used to initialize temporary
alloc_stack with borrows. Since it is a temporary, we do not want to error on
the temporaries initialization... instead, we want to error on the use of the
temporary parameter.
This is achieved by making it so that store_borrow still performs an
assign/merge, but does not require that src/dest be alive. So the regions still
merge (yielding diagnostics for later uses).
It also required me to make it so that PartitionOp::{Assign,Merge} do not
require by default. Instead, we want the individual operations to always emit a
PartitionOp::Require explicitly (which they already did).
One thing to be aware of is that when it comes to diagnostics, we already know
how to find a temporaries original value and how to handle that. So this is the
last part of making store_borrow behave nicely.
rdar://129237675
Although I don't plan to bring over new assertions wholesale
into the current qualification branch, it's entirely possible
that various minor changes in main will use the new assertions;
having this basic support in the release branch will simplify that.
(This is why I'm adding the includes as a separate pass from
rewriting the individual assertions)
It indicates that the value's lifetime continues to at least this point.
The boundary formed by all consuming uses together with these
instructions will encompass all uses of the value.
I also fixed an issue that I found where we were not substituting SILResultInfo
flags which was causing us to drop when substituting sil_sending. I added a
SILVerifier check to make sure that we do not break this again.
The reason that I am doing this is it ensures that if we have a region isolation
merge failure due to a mismatch in between the actual args in the region and the
propagated callee isolation, we see it immediately when we translate the apply
into the pseudo-IR instead of later when we perform the actual diagnostic
emission. This makes it far easier to diagnose these issues since we get an
unknown pattern very early which can be asserted on via the option
-sil-region-isolation-assert-on-unknown-pattern.
This fixes a few issues I missed in the past bit of commits.
I need to fix one issue around async let, but I am going to fix it when I do a
sweep across async let.
The reason why we are doing this is that otherwise, we have that the alloc_stack
formed for the result is disconnected and despite the fact that we merge it into
the actor region of the class method, we do not have that the alloc_stack
specifically is marked when we attempt to squelch Please.
This patch fixes that problem by detecting when an alloc_stack is being used as
a temporary for an out parameter and makes the alloc_stack initially isolated as
appropriate. It only does this in the specific cases where we can pattern match
it which in my limited testing has handled everything.
When merging SILIsolationInfo for regions, we want to drop
nonisolated(unsafe). This is important since nonisolated(unsafe) should only
apply to the specific "value" that it belongs to, not the entire region.
This creates a problem since in a few places in the code base we initialize a
value (producing a disconnected value) and then initialize it by merging in an
actor isolation. This no longer work since we will then always have
nonisolated(unsafe) stripped, so no values would ever be considered to be
nonisolated(unsafe). After analyzing the use case, I realized that these were
just initialization patterns and in this commit, I added a specific
initialization operation called SILIsolationInfo::initializeTrackableValue and
eliminated those calls to SILIsolationInfo::mergeIsolationRegionInfo.
Since SILIsolationInfo no longer has any merge operation on it, I then
eliminated that code in this commit. This completes the behavior split that I
put into the type system in the last commit. Specifically, I defined a
composition type called SILDynamicMergedIsolationInfo. It represents a
SILIsolationInfo that has been merged... that is why I called it the
DynamicMergedIsolationInfo. It could probably use a better name = (.
This fixes one of the last weird test case that I wrote where we were not letting through valid
nonisolated(unsafe) code.
At the same time, I discovered an additional issue (which can be seen in the
TODOs in this commit), where we are being too conservative around a non-Sendable
class var field. I am going to fix that in the next commit.
rdar://128299305
DISCUSSION: The analysis itself is unable to emit errors. So we achieve the same
functionality by in such cases emitting a partition op that signals to our user
that when they process that partition op they should emit an "unknown pattern"
error at the partition op's instructions.
I have wanted this for a long time, but I never got around to it.
I made sure we match what we get without region isolation by turning off region
isolation in one of the test runs on the test for this.
There is one problem where for non-final classes with nonisolated(unsafe) var
fields, we currently do not properly squelch since I need to do more
infrastructure work. I am going to do that in the next commit.
rdar://128299305
The design change here is that instead of just initializing the regionInfo with
disconnected, we set it as .none and if we see .none, just return a newly
construct disconnected isolation region info when getIsolationRegionInfo() is
called.
This enables us to provide a setIsolationRegionInfo() helper for
RegionAnalysisValueMap::getTrackableValue that does not perform a merge. This is
important since for nonisolated(unsafe), we want to not have nonisolated(unsafe)
propagate through merging. So if we use merging to initialize the internal
regionInfo state of a SILIsolationInfo, we will never have a SILIsolationInfo
with that bit set since it will be lost in the merge. So we need some sort of
other assignment operator. Noting that we should only compute a value's
SILIsolationInfo once in RegionAnalysisValueMap before we cache it in the map,
it made sense to just represent it as an optional that way we can guarantee that
the regionInfo is only ever set exactly once by that routine.
There is no guarantee that these other helpers properly model lookthrough as our
model does. This ensures that this routine is always in sync with how we define
lookthrough in our model.
The problem with the old approach can be seen in how we handled move_value. The
model and the later code knew correctly that they should not look through
move_value that is marked as [var_decl]. But this other analysis code did not.
This with the tree today should not have any impact. But with the fix I am doing
now (fixing nonisolated(unsafe)) and later isolation history this will become a
problem.
We still only parse transferring... but this sets us up for adding the new
'sending' syntax by first validating that this internal change does not mess up
the current transferring impl since we want both to keep working for now.
rdar://128216574
This is safe since:
1. We transfer in the non-Sendable parameter into the global actor isolation
region so we know that we will not use the non-Sendable paramter again except on
that actor.
2. Since the closure is global actor isolated, we know that despite the fact
that it is Sendable, it will only ever be executed serially on said global actor
implying that we do not need to worry about different executions of the Sendable
closure running concurrently with each other.
rdar://125200006
As part of this I went through how we handled inference and rather than using a
grab-bag getActorIsolation that was confusing to use, I created split APIs for
specific use cases (actor instance, global actor, just an apply expr crossing)
that makes it clearer inside the SILIsolationInfo::get* APIs what we are
actually trying to model. I found a few issues as a result and fixed most of
them if they were small. I also fixed one bigger one around computed property
initializers in the next commit. There is a larger change I didn't fix around allowing function
ref/partial_apply with isolated self parameters have a delayed flow sensitive
actor isolation... this will be fixed in a subsequent commit.
This also fixes a bunch of cases where we were printing actor-isolated instead
of 'self' isolated.
rdar://127295657
This ensures that when we process, we consider load/load_borrow's result to be
equivalent to its operand. This ensures that a load/load_borrow cannot act as a
use of its operand preventing spurious diagnostics.
The reason why we do this is that we want to treat this as a separate value from
their operand since they are the result of defining a new value.
This has a few nice side-effects, one of which is that if a let results in just
a begin_borrow [var_decl], we emit names for it.
I also did a little work around helping variable name utils to lookup names from
applies that are fed into a {move_value,begin_borrow} [var_decl] which then has
the debug_value we are searching for.
This is backing out an approach that I thought would be superior, but ended up
causing problems.
Originally, we mapped a region number to an immutable pointer set containing
Operand * where the region was tranferred. This worked great for a time... until
I began to need to propagate other information from the transferring code in the
analysis to the actual diagnostic emitter.
To be able to do that, my thought was to make a wrapper type around Operand
called TransferringOperand that contained the operand and the other information
I needed. This seemed to provide me what I wanted but I later found that since
the immutable pointer set was tracking TransferringOperands which were always
newly wrapped with an Operand *, we actually always created new pointer
sets. This is of course wasteful from a memory perspective, but also prevents me
from tracking transferring operand sets during the dataflow since we would never
converge.
In this commit, I fix that issue by again tracking just an Operand * in the
TransferringOperandSet and instead map each operand to a state structure which
we merge dataflow state into whenever we visit it. This provides us with
everything we need to in the next commit to including a region -> transferring
operand set equality check in our dataflow equations and always converge.
