Skip to content

[6.0][region-isolation] Improve the error we emit for closure literals captured as a sending parameter. #75887

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 22, 2024

Conversation

gottesmm
Copy link
Contributor

@gottesmm gottesmm commented Aug 14, 2024

Explanation: [6.0][region-isolation] Improve the error we emit for closure literals captured as a sending parameter.

Before region isolation and sending closures, Task.init took an @Sendable closure. If non-Sendable state was captured in the closure, we emitted a specific error on the captured value. This provided a discoverable bread crumb that the user could use to trace back what the exact problem was (noting that takeSendableClosure is where Task.init) would be:

tmp.swift:8:11: error: capture of 'x' with non-sendable type 'NonSendableKlass' in a `@Sendable` closure
 1 | 
 2 | class NonSendableKlass {}
   |       `- note: class 'NonSendableKlass' does not conform to the 'Sendable' protocol
 3 | 
 4 | func takeSendableClosure(_ x: @Sendable () -> ()) {}
   :
 6 | func foo(_ x: NonSendableKlass) {
 7 |   takeSendableClosure {
 8 |     print(x)
   |           `- error: capture of 'x' with non-sendable type 'NonSendableKlass' in a `@Sendable` closure
 9 |   }
10 | }

When we introduced region isolation and changed Task.init to take a sending closure, we regressed this behavior unintentionally. With region isolation and a sending closure the above error is emitted instead:

tmp.swift:5:3: error: task-isolated value of type '() async -> ()' passed as a strongly transferred parameter; later accesses could race
3 | 
4 | func foo(_ x: NonSendableKlass) async {
5 |   Task {
  |   `- error: task-isolated value of type '() async -> ()' passed as a strongly transferred parameter; later accesses could race
6 |     print(x)
7 |   }

this doesn't provide a breadcrumb for the user to work back from anymore. With this change, we ameliorate this situation by introducing a special diagnostic that ensures that the user always has a breadcrumb. With this change, we now get the following error message:

tmp.swift:5:8: error: passing closure as a 'sending' parameter risks causing data races between code in the current task and concurrent execution of the closure
3 | 
4 | func foo(_ x: NonSendableKlass) {
5 |   Task {
  |        `- error: passing closure as a 'sending' parameter risks causing data races between code in the current task and concurrent execution of the closure
6 |     print(x)
  |           `- note: closure captures 'x' which is accessible to code in the current task
7 |   }
8 | }

Radars:

  • rdar://133798044

Original PRs:

Risk: Low. This does not impact where we emit errors... it just changes the actual error that we are emitting. Also, I pattern matched specifically against closure literals passed as a sending parameter, so if I did introduce a problem, it will occur narrowly just in this case (a case that we were already going to reject anyways). So the potential fallout is not large. Also this only applies to swift 6 and swift 5 + strict-concurrency.

…tured as a sending parameter.

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
(cherry picked from commit 4bb2e4f)

Conflicts:
	include/swift/AST/DiagnosticsSIL.def
	lib/SILOptimizer/Mandatory/TransferNonSendable.cpp
	test/Concurrency/concurrent_value_checking.swift
	test/Concurrency/isolated_parameters.swift
	test/Concurrency/sendable_preconcurrency.swift
	test/Concurrency/sendable_without_preconcurrency.swift
	test/Concurrency/sendable_without_preconcurrency_2.swift
	test/Concurrency/transfernonsendable.swift
	test/Concurrency/transfernonsendable_global_actor_sending.swift
@gottesmm gottesmm requested a review from a team as a code owner August 14, 2024 17:52
Copy link
Contributor

@atrick atrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm
Copy link
Contributor Author

@swift-ci test macOS platform

@gottesmm gottesmm merged commit b163fed into swiftlang:release/6.0 Aug 22, 2024
5 checks passed
@gottesmm gottesmm deleted the release/6.0-rdar133798044 branch August 22, 2024 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants