swiftlang / swift

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

Symbol Graphs Contain Invalid Default Implementation Relationships #61285

Open theMomax opened 2 years ago

theMomax commented 2 years ago

Describe the bug

Members that are marked as throws and/or async are recorded as default implementation targets for protocol requirements without the respective keyword(s).

This is caused by the name-based comparison here:

  /// Claim a protocol `P`'s members as default implementation targets
  /// for `VD`.
  auto HandleProtocol = [=](const ProtocolDecl *P) {
    for (const auto *Member : P->getMembers()) {
      if (const auto *MemberVD = dyn_cast<ValueDecl>(Member)) {
        if (MemberVD->getName().compare(VD->getName()) == 0) { // <-- does not consider `throws` or `async` keywords
          recordEdge(Symbol(this, VD, nullptr),
                     Symbol(this, MemberVD, nullptr),
                     RelationshipKind::DefaultImplementationOf());

Steps To Reproduce

  1. Build documentation for the following code:
    
    public protocol E {
    func foo()
    }

public extension E { func foo() throws { } }

2. Inspect the symbol graph file, which should contain a relationship similar to this one:
```json
{
    "kind": "defaultImplementationOf",
    "source": "s:9MyLibrary1EPAAE3fooyyKF",
    "target": "s:9MyLibrary1EP3fooyyF"
}
  1. The default implementation is also reflected in the documentation archive:
image

Expected behavior The detection of default implementation should follow the rules applied by the Swift compiler, i.e. a member can only implement a protocol requirement if it requires a subset of the call modifiers (try, await) required by the protocol requirement.

E.g. in the example above, the implementation cannot implement the requirement because the former throws, whereas the latter doesn't:

public protocol E {
  func foo()
}

public extension E {
  func foo() throws { }
}

struct D: E { } // Error: Type 'D' does not conform to protocol 'E'

Environment (please fill out the following information)

slavapestov commented 11 months ago

The bug is a consequence of the fact that SymbolGraph::recordDefaultImplementationRelationships() seems to implement it's own thing here: https://github.com/apple/swift/blob/9ee5d6b34291ebfc89f57068ed89842285dc9688/lib/SymbolGraphGen/SymbolGraph.cpp#L410

TypeChecker::inferDefaultWitnesses() already computes this information, and records it in eg ProtocolDecl::getDefaultWitness().