swiftlang / swift

The Swift Programming Language
https://swift.org
Apache License 2.0
67.49k stars 10.35k forks source link

False positive warning during the code analysis performed by SE-0430 "transferring" parameters and result values #73315

Open groue opened 5 months ago

groue commented 5 months ago

Description

The compiler emits false positive warnings whenever a value is transferred twice.

Reproduction

func schedule(_ action: transferring @escaping () -> Void) { }

// OK
func f1(_ closure: transferring @escaping () -> Void) {
    schedule {
        closure()
    }
}

// Compiler warning
func f2(_ closure: transferring @escaping () -> Void) {
    schedule {
        schedule {
//      `- warning: task-isolated value of type '() -> Void'
//         passed as a strongly transferred parameter
            closure()
        }
    }
}

Expected behavior

The compiler emits no diagnostic

Environment

$ /Library/Developer/Toolchains/swift-6.0-DEVELOPMENT-SNAPSHOT-2024-04-22-a.xctoolchain/usr/bin/swift --version
Apple Swift version 6.0-dev (LLVM 94447cf359772fc, Swift f36098a14a0d48b)
Target: arm64-apple-macosx14.0

Additional information

I discovered this issue while exploring the possibilities evoked in this forum post.

I assumed that DispatchQueue.async WILL be modified so that it accepts a transferred closure instead of a Sendable closure.

I have some code that performs double async dispatch in order to avoid thread explosion (technique discussed in this forum post).

And that's how I had to transfer a closure twice, saw a warning, and opened this issue.

groue commented 5 months ago

I have similar warnings in (real) code that involves continuation, as below:

// Definition of asyncRead used below
func asyncRead(_ value: transferring @escaping (Result<Database, Error>) -> Void)

public func read<T>(
    _ value: transferring @escaping (Database) throws -> transferring T
) async throws -> transferring T
{
  try await withUnsafeThrowingContinuation { continuation in
    asyncRead { result in
//  `- warning: task-isolated value of type
//     '(Result<Database, any Error>) -> Void' passed
//     as a strongly transferred parameter.
      do {
        try continuation.resume(returning: value(result.get()))
      } catch {
        continuation.resume(throwing: error)
      }
    }
  }
}

I'm playing with swift-6.0-DEVELOPMENT-SNAPSHOT-2024-04-22-a from Xcode, which means that it's difficult to know what's happening. To get accurate compiler diagnostics, I compile in the terminal with /Library/Developer/Toolchains/swift-6.0-DEVELOPMENT-SNAPSHOT-2024-04-22-a.xctoolchain/usr/bin/swift build. But I don't know what is the actual definition of withUnsafeThrowingContinuation.

My goal here is to help users use non-sendable values as freely as possible, both as captured inputs, and output of read. Since read guarantees that its value closure is executed only once, and that it does not use the produced values in any way, I believe that using transferring is correct.

I don't really know if I'm facing compiler issues, or if I misunderstand the transferring feature, or if I need yet another compiler feature that is not there yet.

groue commented 4 months ago

Hello,

This issue is still there in Xcode 16 beta 1 (with transferring renamed to sending).

I still assume that this warning is a false positive. Please somebody chime in if I'm wrong, so that I do not ship an incorrect workaround 🙏

My current workaround involves an unchecked sendable wrapper:

func schedule(_ action: sending @escaping () -> Void) { }

// OK
func f1(_ closure: sending @escaping () -> Void) {
    schedule {
        closure()
    }
}

// Compiler warning
func f2(_ closure: sending @escaping () -> Void) {
    schedule {
        schedule {
//      `- warning: task-isolated value of type '() -> Void'
//         passed as a strongly transferred parameter
            closure()
        }
    }
}

// MARK: - Workaround

/// Helps compiler accept valid code it can't validate.
struct UncheckedSendableWrapper<Value>: @unchecked Sendable {
    let value: Value
}

// No compiler warning
func f3(_ closure: sending @escaping () -> Void) {
    schedule {
        // TODO: remove wrapper when <https://github.com/apple/swift/issues/73315> is fixed.
        let wrapper = UncheckedSendableWrapper(value: closure)
        schedule {
            wrapper.value()
        }
    }
}
groue commented 4 months ago

Here are two other snippets that reveal a variant of the warning. Maybe this is another compiler issue.

The first snippet turns a call to DispatchQueue.async(execute:) into an async function that returns when the submitted closure has completed. I believe this is a valid use case (and I actually need it). I wonder if it should be supported without any workaround such as UncheckedSendableWrapper:

// Similar to a future `DispatchQueue.async(execute:)`, according to
// <https://forums.swift.org/t/how-can-i-use-region-based-isolation/71426/5>
func async(execute work: sending @escaping () -> Void) { }

func awaitCompletion(_ closure: sending @escaping () -> Void) async {
    await withUnsafeContinuation { continuation in
        // ⚠️ Task-isolated value of type '() -> Void' passed as a strongly
        // transferred parameter; later accesses could race; this is an
        // error in the Swift 6 language mode.
        async(execute: {
            closure()
            continuation.resume()
        })
    }
}

A minimal sample code that reproduces this warning:

// Minimal
func async(execute work: sending @escaping () -> Void) { }
func f(_ fn: () -> Void) { }

func noWarning(_ closure: sending @escaping () -> Void) async {
    // No warning
    async(execute: closure)
}

func warning1(_ closure: sending @escaping () -> Void) async {
    f {
        // ⚠️ Sending 'closure' risks causing data races; this is an error
        // in the Swift 6 language mode
        async(execute: closure)
    }
}

func warning2(_ closure: sending @escaping () -> Void) async {
    f {
        // ⚠️ Task-isolated value of type '() -> Void' passed as a strongly
        // transferred parameter; later accesses could race; this is an
        // error in the Swift 6 language mode
        async(execute: {
            closure()
        })
    }
}

Note that withUnsafeContinuation and f have a non-escaping input closure. Is it really possible that a race can occur?

groue commented 4 months ago

Case closed: those warnings are currently expected. See this forum post by @rjmccall.