swiftlang / swift

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

Incorrect resolution of sync/async overload when async overload is provided as the default implementation of a protocol requirement #74469

Open groue opened 3 months ago

groue commented 3 months ago

Description

At runtime, the program calls the sync overload when it should call the async overload that is provided as the default implementation of a protocol requirement.

To workaround this compiler bug, the developer needs to remove the default protocol implementation, and have each and every concrete type repeat the code that could have been shared as a default implementation.

This workaround is acceptable for internal protocols, because the list of concrete types is known.

This workaround is not acceptable for public protocols, because this means that clients MUST copy some boilerplate code instead of enjoying a default implementation.

Reproduction

import XCTest

protocol Base {
    func f() -> String
    func f() async -> String
}

protocol Derived: Base { }

extension Derived {
    func f() async -> String { "Async" }
}

struct Bug: Derived {
    func f() -> String { "Sync" }
}

struct Correct: Derived {
    func f() -> String { "Sync" }
    func f() async -> String { "Async" }
}

final class MyTests: XCTestCase {
    func testBug() async {
        // ❌ XCTAssertEqual failed: ("Base") is not equal to ("Derived")
        let base: some Base = Bug()
        let value = await base.f()
        XCTAssertEqual(value, "Async")
    }

    func testCorrect() async {
        // ✅
        let base: some Base = Correct()
        let value = await base.f()
        XCTAssertEqual(value, "Async")
    }
}

Expected behavior

The testBug test should pass.

Environment

swift-driver version: 1.109.2 Apple Swift version 6.0 (swiftlang-6.0.0.3.300 clang-1600.0.20.10) Target: arm64-apple-macosx14.0

Additional information

This is not a naive bug report about "wrong" dynamic dispatch. The compiler has all the required static information to choose the correct overload.

See #74459 for a related issue (compiler is not serious enough about the async overload).

groue commented 2 months ago

Bug still present in Xcode 16.0 beta 3 (16A5202i)

hborla commented 2 months ago

@groue I can't reproduce the issue. I ran this code (which eliminated XCTest dependency) and both assertions passed. What am I missing?

protocol Base {
  func f() -> String
  func f() async -> String
}

protocol Derived: Base { }

extension Derived {
  func f() async -> String { "Async" }
}

struct Bug: Derived {
  func f() -> String { "Sync" }
}

struct Correct: Derived {
  func f() -> String { "Sync" }
  func f() async -> String { "Async" }
}

func testBug() async {
  let base: some Base = Bug()
  let value = await base.f()
  assert(value == "Async")
}

func testCorrect() async {
  let base: some Base = Correct()
  let value = await base.f()
  assert(value == "Async")
}

await testBug()
await testCorrect()

I noticed your comment also doesn't really reflect what the test case is doing

   `// ❌ XCTAssertEqual failed: ("Base") is not equal to ("Derived")`

The test is not testing these strings. Can you please clarify what behavior you were seeing? I'm guessing you were seeing value in testBug as "Sync" but I'd like to clarify that.

groue commented 2 months ago

Hello @hborla,

Sorry about the misleading comment. The ❌ mark just shows the test that fails.

And indeed I can reproduce, in both Xcode 15.4 and Xcode 16 beta 3:

% swiftc -version
swift-driver version: 1.111.2 Apple Swift version 6.0 (swiftlang-6.0.0.5.15 clang-1600.0.22.6)
Target: arm64-apple-macosx14.0

% swift repl     
Welcome to Apple Swift version 6.0 (swiftlang-6.0.0.5.15 clang-1600.0.22.6).
Type :help for assistance.
  1> protocol Base { 
  2.   func f() -> String 
  3.   func f() async -> String 
  4. } 
  5.  
  6. protocol Derived: Base { } 
  7.  
  8. extension Derived { 
  9.   func f() async -> String { "Async" } 
 10. } 
 11.  
 12. struct Bug: Derived { 
 13.   func f() -> String { "Sync" } 
 14. } 
 15.  
 16. struct Correct: Derived { 
 17.   func f() -> String { "Sync" } 
 18.   func f() async -> String { "Async" } 
 19. } 
 20.  
 21. func testBug() async { 
 22.   let base: some Base = Bug() 
 23.   let value = await base.f() 
 24.   assert(value == "Async") 
 25. } 
 26.  
 27. func testCorrect() async { 
 28.   let base: some Base = Correct() 
 29.   let value = await base.f() 
 30.   assert(value == "Async") 
 31. } 
 32.  
 33. await testBug() 
 34. await testCorrect()
__lldb_expr_1/repl.swift:24: Assertion failed
2024-07-14 07:52:26.094610+0200 repl_swift[92906:18786106] __lldb_expr_1/repl.swift:24: Assertion failed

Chosing the async overload, when available, is important. I'll repeat what I said in #74459:

[...] blocking the cooperative thread pool can lead to deadlock. API designers who provide an async overload rely on the compiler to help them not blocking the cooperative thread pool.

If we agree on this premise, I hope fixing that this will not require an evolution proposal as well 😅 But maybe this is already fixed, and the fix is just waiting to ship?

hborla commented 2 months ago

I agree that the behavior you're seeing is a bug. I tried it with a main compiler and I reproduced the issue. I figured out the difference in my other setup (which had a more recent 6.0 compiler) is I was building in release, which didn't reproduce the issue, and switching to a debug build reproduces there too. In any case, thank you for the report; I'll figure out what's going wrong.

groue commented 2 months ago

Thank you!