Specifically in terms of printing, if NonisolatedNonsendingByDefault is enabled,
we print out things as nonisolated/task-isolated and @concurrent/@concurrent
task-isolated. If said feature is disabled, we print out things as
nonisolated(nonsending)/nonisolated(nonsending) task-isolated and
nonisolated/task-isolated. This ensures in the default case, diagnostics do not
change and we always print out things to match the expected meaning of
nonisolated depending on the mode.
I also updated the tests as appropriate/added some more tests/added to the
SendNonSendable education notes information about this.
(cherry picked from commit 14634b6847)
Specifically, there is currently a bug in TypeCheckConcurrency.cpp where we do
not visit autoclosures. This causes us to never set the autoclosure's
ActorIsolation field like all other closures. For a long time we were able to
get away with this just by relying on the isolation of the decl context of the
autoclosure... but with the introduction of nonisolated(nonsending), we found
cases where the generated single curry autoclosure would necessarily be
different than its decl context (e.x.: a synchronous outer curry thunk that is
nonisolated that returns an inner curry thunk that is
nonisolated(nonsending)). This problem caused us to hit asserts later in the
compiler since the inner closure was actually nonisolated(nonsending), but we
were thinking that it should have been concurrent.
To work around this problem, I changed the type checker in
ced96aa5cd to explicitly set the isolation of
single curry thunk autoclosures when it generates them. The reason why we did
this is that it made it so that we did not have to have a potential large source
break in 6.2 by changing TypeCheckConcurrency.cpp to visit all autoclosures it
has not been visiting.
This caused a follow on issue where since we were now inferring the inner
autoclosure to have the correct isolation, in cases where we were creating a
double curry thunk for an access to a global actor isolated field of a
non-Sendable non-global actor isolated nominal type, we would have the outer
curry thunk have unspecified isolation instead of main actor isolation. An
example of this is the following:
```swift
class A {
var block: @MainActor () -> Void = {}
}
class B {
let a = A()
func d() {
a.block = c // Error! Passing task isolated 'self' to @MainActor closure.
}
@MainActor
func c() {}
}
```
This was unintentional. To work around this, this commit changes the type
checker to explicitly set the double curry thunk isolation to the correct value
when the type checker generates the double curry thunk in the same manner as it
does for single curry thunks and validates that if we do set the value to
something explicitly that it has the same value as the single curry thunk.
rdar://152522631
(cherry picked from commit c28490b527)
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
(cherry picked from commit f31236931b)
[6.2][send-non-sendable] Recurse to the full underlying value computation instead of just the object one when computing the underlying object of an address.
**Description**: This adds "task name" parameter to all task creating
functions.
This is done in a few ways, e.g. we can backdeploy this to 5.1 in APIs
which do not accept the `TaskExecutor` but it they do we provide a
version for 6.0+ etc. This was requested in the SE acceptable of this
proposal [Acceptance post
SE-0469](https://forums.swift.org/t/accepted-with-modifications-se-0469-task-naming/79438).
This moves all these declarations to gyb since going through them one by
one has become unmaintainable otherwise.
**Scope/Impact**: All task creation APIs now gain a new task name
parameter.
**Risk:** Medium, changes existing APIs rather than adding "even more
overloads" though this risk was discussed in the team and accepted. This
has a potential to be source breaking it someone used Task.init and
friends as function.
**Testing**: CI testing, source compatibility suite testing
**Reviewed by**:
**Original PR:**
- https://github.com/swiftlang/swift/pull/81107 build changes required
for this
- https://github.com/swiftlang/swift/pull/80984
**Radar:**
---------
Co-authored-by: Kuba Mracek <mracek@apple.com>
Otherwise, depending on the exact value that we perform the underlying look up
at... we will get different underlying values. To see this consider the
following SIL:
```sil
%1 = alloc_stack $MyEnum<T>
copy_addr %0 to [init] %1
%2 = unchecked_take_enum_data_addr %1, #MyEnum.some!enumelt
%3 = load [take] %2
%4 = project_box %3, 0
%5 = load_borrow %4
%6 = copy_value %5
```
If one were to perform an underlying object query on %4 or %3, one would get
back an underlying object of %1. In contrast, if one performed the same
operation on %5, then one would get back %3. The reason why this happens is that
we first see we have an object but that it is from a load_borrow so we need to
look through the load_borrow and perform the address underlying value
computation. When we do that, we find project_box to be the value. project_box
is special since it is the only address base we ever look through since from an
underlying object perspective, we want to consider the box to be the underlying
object rather than the projection. So thus we see that the result of the
underlying address computation is that the underlying address is from a load
[take]. Since we then pass in load [take] recursively into the underlying value
object computation, we just return load [take]. In contrast, the correct
behavior is to do the more general recurse that recognizes that we have a load
[take] and that we need to look through it and perform the address computation.
rdar://151598281
(cherry picked from commit 904ebc6784)
This matches send non sendable but importantly also makes it clear that we are
talking about something that doesn't conform to the Sendable protocol which is
capitalized.
rdar://151802975
(cherry picked from commit 3ed4059a60)
Otherwise, we can be inconsistent with isolations returned by other parts of the
code. Previously we were just treating it always as self + nom decl, which is
clearly wrong if a type is not self (e.x.: if it is an isolated parameter).
rdar://135459885
(cherry picked from commit 0ece31e4f6)
The specific issue was when we were walking instructions looking to see if there
was a partial apply escaping instruction, we were not including the user
itself. That means that if the user was the partial apply escaping instruction,
we would return that no escape occured.
rdar://149414471
(cherry picked from commit 6eee52fb01)
Specifically, we only do this if the base is a let or if it is a var but not
captured by reference.
rdar://149019222
(cherry picked from commit 23b6937cbc)
This also fixes a case where we would have inferred the wrong isolation (which
the assert caught). I think we missed this since it only comes up with final
classes.
rdar://142568522
I did the correct thing for indirect parameters, but did not do the correct
thing for direct parameters. This is now fixed with some tests to boot.
rdar://141631655
I also want to extend it and did not want to have to copy/paste this code into
multiple places.
The small test tweak occurs since I changed the initializer SILGen emission code
to set the declref field of SILFunctions to the actual decl ref which we did not
before. So we got a more specific diagnostic.
Find all the usages of `--enable-experimental-feature` or
`--enable-upcoming-feature` in the tests and replace some of the
`REQUIRES: asserts` to use `REQUIRES: swift-feature-Foo` instead, which
should correctly apply to depending on the asserts/noasserts mode of the
toolchain for each feature.
Remove some comments that talked about enabling asserts since they don't
apply anymore (but I might had miss some).
All this was done with an automated script, so some formatting weirdness
might happen, but I hope I fixed most of those.
There might be some tests that were `REQUIRES: asserts` that might run
in `noasserts` toolchains now. This will normally be because their
feature went from experimental to upcoming/base and the tests were not
updated.
This problem comes up with the following example:
```swift
class A {
var description = ""
}
class B {
let a = A()
func b() {
let asdf = ""
Task { @MainActor in
a.description = asdf // Sending 'asdf' risks causing data races
}
}
}
```
The specific issue is that the closure we generate actually includes an
implicit(any) parameter at the SIL level which occurs after the callee operand
but before the captures. This caused the captured variable index from the AST
and the one we compute from the partial_apply to differ by 1. So we need to
subtract 1 in such a case. That is why we used to print 'asdf' instead of 'a'
above.
DISCUSSION: This shows an interesting difference between SIL applied arg indices
and AST indices. SIL applied arg indices would include the implicit(any)
parameter since it is a parameter in the SIL function type. In contrast, this
doesn't show up in the formal AST parameters or captures. To make it easier to
reason about this, I added a new API to ApplySite called
ApplySite::getASTAppliedArgIndex and added large comments to
getASTAppliedArgIndex and getAppliedArgIndex that explains the issue.
rdar://136593706
https://github.com/swiftlang/swift/issues/76648
The reason that I am modifying this error is that in situations like the
following one can have a Sendable type that triggers this error since the box
containing the value is non-Sendable.
```
func methodConsuming(x: consuming SendableKlass) async throws {
try await withThrowingTaskGroup(of: Void.self) { group in
group.addTask { // expected-tns-warning {{passing closure as a 'sending' parameter risks causing data races between code in the current task and concurrent execution of the closure}}
useValue(x) // expected-tns-note {{closure captures reference to mutable var 'x' which is accessible to code in the current task}}
}
try await group.waitForAll()
}
}
```
rdar://133813644
(cherry picked from commit 36c2b3cc1330c07dcf9715f8ae88d31f9dba58c4)
Specifically:
I changed the main error message to focus on the closure and that the closure
is being accessed concurrently.
If we find that we captured a value that is the actual isolation source, we
emit that the capture is actually actor isolated.
If the captured value is in the same region as the isolated value but is not
isolated, we instead say that the value is accessible from *-isolated code or
code within the current task.
If we find multiple captures and we do not which is the actual value that was
in the same region before we formed the partial apply, we just emit a note on
the captures saying that the closure captures the value.
I changed the diagnostics from using the phrase "task-isolated" to use some
variant of accessible to code in the current task.
The idea is that in all situations we provide a breadcrumb that the user can
start investigating rather than just saying that the closure is "task-isolated".
From a preconcurrency perspective, I made it so that we apply the preconcurrency
behavior of all of the captures. This means that if one of the captures is
preconcurrency, we apply the preconcurrency restriction to the closure. This is
one step towards making it so that preconcurrency applies at the region level...
we just are not completely there yet.
rdar://133798044
Specifically, this API has some hard edges where instead of just returning an
invalid value to signal that we do not have self, we assert or return something
bogus. This commit just fixes our usage of that API to be correct.
rdar://132545626
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
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
Before this change in the following code, we would say that message is isolated to the actor instead of the global actor isolation of the actor's method:
```swift
class Message { ... }
actor MessageHolder {
@MainActor func hold(_ message: Message) { ... }
}
@MainActor
func sendMessage() async {
let messageHolder = MessageHolder()
let message = Message()
// We identified messageHolder.hold as being MessageHolder isolated
// instead of main actor isolated.
messageHolder.hold(message)
Task { @MainActor in print(message) }
}
```
I also used this as an opportunity to simplify the logic in this part of the
code. Specifically, I had made it so that multiple interesting cases were
handled in the same conditional statement in a manner that it made it hard to
know which cases were actually being handled and why it was correct. Now I split
that into two separate if statements with comments that make it clear what we
are actually trying to pattern match against.
rdar://130980933
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
This just means that I stopped treating it like an actor instance isolated
thing. This was fun to track down since ActorIsolation has a union in it that
was being misinterpreted, leading to memory corruption... my favorite! = ).
rdar://129256560
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
This ensures that we can properly compute isolation for generic types that
conform to AnyActor.
I found this by playing with test cases from the previous commit. We would not
find an actor type for the actor instance isolation and would fall back along an
incorrect path.
rdar://128021548
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
