Skip to content
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

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

Open
groue opened this issue Jun 17, 2024 · 5 comments
Assignees
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. concurrency Feature: umbrella label for concurrency language features

Comments

@groue
Copy link

groue commented Jun 17, 2024

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 groue added bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. triage needed This issue needs more specific labels labels Jun 17, 2024
@groue
Copy link
Author

groue commented Jul 9, 2024

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

@hborla hborla added concurrency Feature: umbrella label for concurrency language features and removed triage needed This issue needs more specific labels labels Jul 14, 2024
@hborla
Copy link
Member

hborla commented Jul 14, 2024

@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
Copy link
Author

groue commented Jul 14, 2024

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
Copy link
Member

hborla commented Jul 14, 2024

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.

@hborla hborla self-assigned this Jul 14, 2024
@groue
Copy link
Author

groue commented Jul 14, 2024

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. concurrency Feature: umbrella label for concurrency language features
Projects
None yet
Development

No branches or pull requests

2 participants