Handle conformance with multiple protocol requirements with the same selector.

When concurrency is enabled, the same Objective-C method on a protocol can
be imported as two different protocol requirements, both of which have the
same selector. When performing conformance checking, only treat a
given @objc protocol requirement as "unsatisfied" by a conformance if
none of the requirements with the same Objective-C selector (+
instance/class designation) are satisfied.

Author: DougGregor

lib/Sema/TypeCheckDeclObjC.cpp

@@ -2372,12 +2372,22 @@ bool swift::diagnoseObjCUnsatisfiedOptReqConflicts(SourceFile &sf) {
     if (conflicts.empty())
       continue;
 
+    auto bestConflict = conflicts[0];
+    for (auto conflict : conflicts) {
+      if (conflict->getName().isCompoundName() &&
+          conflict->getName().getArgumentNames().size() ==
+            req->getName().getArgumentNames().size()) {
+        bestConflict = conflict;
+        break;
+      }
+    }
+
     // Diagnose the conflict.
     auto reqDiagInfo = getObjCMethodDiagInfo(unsatisfied.second);
-    auto conflictDiagInfo = getObjCMethodDiagInfo(conflicts[0]);
+    auto conflictDiagInfo = getObjCMethodDiagInfo(bestConflict);
     auto protocolName
       = cast<ProtocolDecl>(req->getDeclContext())->getName();
-    Ctx.Diags.diagnose(conflicts[0],
+    Ctx.Diags.diagnose(bestConflict,
                        diag::objc_optional_requirement_conflict,
                        conflictDiagInfo.first,
                        conflictDiagInfo.second,
@@ -2387,9 +2397,9 @@ bool swift::diagnoseObjCUnsatisfiedOptReqConflicts(SourceFile &sf) {
                        protocolName);
 
     // Fix the name of the witness, if we can.
-    if (req->getName() != conflicts[0]->getName() &&
-        req->getKind() == conflicts[0]->getKind() &&
-        isa<AccessorDecl>(req) == isa<AccessorDecl>(conflicts[0])) {
+    if (req->getName() != bestConflict->getName() &&
+        req->getKind() == bestConflict->getKind() &&
+        isa<AccessorDecl>(req) == isa<AccessorDecl>(bestConflict)) {
       // They're of the same kind: fix the name.
       unsigned kind;
       if (isa<ConstructorDecl>(req))
@@ -2402,29 +2412,29 @@ bool swift::diagnoseObjCUnsatisfiedOptReqConflicts(SourceFile &sf) {
         llvm_unreachable("unhandled @objc declaration kind");
       }
 
-      auto diag = Ctx.Diags.diagnose(conflicts[0],
+      auto diag = Ctx.Diags.diagnose(bestConflict,
                                      diag::objc_optional_requirement_swift_rename,
                                      kind, req->getName());
 
       // Fix the Swift name.
-      fixDeclarationName(diag, conflicts[0], req->getName());
+      fixDeclarationName(diag, bestConflict, req->getName());
 
       // Fix the '@objc' attribute, if needed.
-      if (!conflicts[0]->canInferObjCFromRequirement(req))
-        fixDeclarationObjCName(diag, conflicts[0],
-                               conflicts[0]->getObjCRuntimeName(),
+      if (!bestConflict->canInferObjCFromRequirement(req))
+        fixDeclarationObjCName(diag, bestConflict,
+                               bestConflict->getObjCRuntimeName(),
                                req->getObjCRuntimeName(),
                                /*ignoreImpliedName=*/true);
     }
 
     // @nonobjc will silence this warning.
     bool hasExplicitObjCAttribute = false;
-    if (auto objcAttr = conflicts[0]->getAttrs().getAttribute<ObjCAttr>())
+    if (auto objcAttr = bestConflict->getAttrs().getAttribute<ObjCAttr>())
       hasExplicitObjCAttribute = !objcAttr->isImplicit();
     if (!hasExplicitObjCAttribute)
-      Ctx.Diags.diagnose(conflicts[0], diag::req_near_match_nonobjc, true)
+      Ctx.Diags.diagnose(bestConflict, diag::req_near_match_nonobjc, true)
         .fixItInsert(
-          conflicts[0]->getAttributeInsertionLoc(/*forModifier=*/false),
+          bestConflict->getAttributeInsertionLoc(/*forModifier=*/false),
           "@nonobjc ");
 
     Ctx.Diags.diagnose(getDeclContextLoc(unsatisfied.first),

lib/Sema/TypeCheckProtocol.cpp

@@ -1525,7 +1525,9 @@ class swift::MultiConformanceChecker {
     NormalProtocolConformance *conformance, bool issueFixit);
 
   /// Determine whether the given requirement was left unsatisfied.
-  bool isUnsatisfiedReq(NormalProtocolConformance *conformance, ValueDecl *req);
+  bool isUnsatisfiedReq(
+      ConformanceChecker &checker, NormalProtocolConformance *conformance,
+      ValueDecl *req);
 public:
   MultiConformanceChecker(ASTContext &ctx) : Context(ctx) {}
 
@@ -1551,17 +1553,34 @@ class swift::MultiConformanceChecker {
 };
 
 bool MultiConformanceChecker::
-isUnsatisfiedReq(NormalProtocolConformance *conformance, ValueDecl *req) {
+isUnsatisfiedReq(ConformanceChecker &checker,
+                 NormalProtocolConformance *conformance, ValueDecl *req) {
   if (conformance->isInvalid()) return false;
   if (isa<TypeDecl>(req)) return false;
 
   auto witness = conformance->hasWitness(req)
                      ? conformance->getWitnessUncached(req).getDecl()
                      : nullptr;
 
-  // An optional requirement might not have a witness...
-  if (!witness)
+  if (!witness) {
+    // If another @objc requirement refers to the same Objective-C
+    // method, this requirement isn't unsatisfied.
+    if (conformance->getProtocol()->isObjC() &&
+        isa<AbstractFunctionDecl>(req)) {
+      auto funcReq = cast<AbstractFunctionDecl>(req);
+      auto key = checker.getObjCMethodKey(funcReq);
+      for (auto otherReq : checker.getObjCRequirements(key)) {
+        if (otherReq == req)
+          continue;
+
+        if (conformance->hasWitness(otherReq))
+          return false;
+      }
+    }
+
+    // An optional requirement might not have a witness.
     return req->getAttrs().hasAttribute<OptionalAttr>();
+  }
 
   // If the witness lands within the declaration context of the conformance,
   // record it as a "covered" member.
@@ -1586,13 +1605,26 @@ void MultiConformanceChecker::checkAllConformances() {
       continue;
     // Check whether there are any unsatisfied requirements.
     auto proto = conformance->getProtocol();
+    Optional<ConformanceChecker> checker;
+    auto getChecker = [&] () -> ConformanceChecker& {
+      if (checker)
+        return *checker;
+
+      if (!AllUsedCheckers.empty() &&
+          AllUsedCheckers.back().Conformance == conformance)
+        return AllUsedCheckers.back();
+
+      checker.emplace(getASTContext(), conformance, MissingWitnesses);
+      return *checker;
+    };
+
     for (auto member : proto->getMembers()) {
       auto req = dyn_cast<ValueDecl>(member);
       if (!req || !req->isProtocolRequirement()) continue;
 
       // If the requirement is unsatisfied, we might want to warn
       // about near misses; record it.
-      if (isUnsatisfiedReq(conformance, req)) {
+      if (isUnsatisfiedReq(getChecker(), conformance, req)) {
         UnsatisfiedReqs.push_back(req);
         continue;
       }
@@ -3100,10 +3132,108 @@ filterProtocolRequirements(
   return Filtered;
 }
 
+/// Prune the set of missing witnesses for the given conformance, eliminating
+/// any requirements that do not actually need to satisfied.
+static ArrayRef<MissingWitness> pruneMissingWitnesses(
+    ConformanceChecker &checker,
+    ProtocolDecl *proto,
+    NormalProtocolConformance *conformance,
+    ArrayRef<MissingWitness> missingWitnesses,
+    SmallVectorImpl<MissingWitness> &scratch) {
+  if (missingWitnesses.empty())
+    return missingWitnesses;
+
+  // For an @objc protocol defined in Objective-C, the Clang importer might
+  // have imported the same underlying Objective-C declaration as more than
+  // one Swift declaration. If we aren't in an imported @objc protocol, there
+  // is nothing to do.
+  if (!proto->isObjC())
+    return missingWitnesses;
+
+  // Consider each of the missing witnesses to remove any that should not
+  // longer be considered "missing".
+  llvm::SmallDenseSet<ConformanceChecker::ObjCMethodKey>
+      alreadyReportedAsMissing;
+  bool removedAny = false;
+  for (unsigned missingWitnessIdx : indices(missingWitnesses)) {
+    const auto &missingWitness = missingWitnesses[missingWitnessIdx];
+
+    // Local function to skip this particular witness.
+    auto skipWitness = [&] {
+      if (removedAny)
+        return;
+
+      // This is the first witness we skipped. Copy all of the earlier
+      // missing witnesses over.
+      scratch.clear();
+      scratch.append(
+          missingWitnesses.begin(),
+          missingWitnesses.begin() + missingWitnessIdx);
+      removedAny = true;
+    };
+
+    // Local function to add this particular witness.
+    auto addWitness = [&] {
+      if (removedAny)
+        scratch.push_back(missingWitness);
+    };
+
+    // We only care about functions
+    auto funcRequirement = dyn_cast<AbstractFunctionDecl>(
+        missingWitness.requirement);
+    if (!funcRequirement) {
+      addWitness();
+      continue;
+    }
+
+    // ... whose selector is one that maps to multiple requirement declarations.
+    auto key = checker.getObjCMethodKey(funcRequirement);
+    auto matchingRequirements = checker.getObjCRequirements(key);
+    if (matchingRequirements.size() < 2) {
+      addWitness();
+      continue;
+    }
+
+    // If we have already reported a function with this selector as missing,
+    // don't do it again.
+    if (!alreadyReportedAsMissing.insert(key).second) {
+      skipWitness();
+      continue;
+    }
+
+    // If there is a witness for any of the *other* requirements with this
+    // same selector, don't report it.
+    bool foundOtherWitness = false;
+    for (auto otherReq : matchingRequirements) {
+      if (otherReq == funcRequirement)
+        continue;
+
+      if (conformance->getWitness(otherReq)) {
+        foundOtherWitness = true;
+        break;
+      }
+    }
+
+    if (foundOtherWitness)
+      skipWitness();
+    else
+      addWitness();
+  }
+
+  if (removedAny)
+    return scratch;
+
+  return missingWitnesses;
+}
+
 bool ConformanceChecker::
 diagnoseMissingWitnesses(MissingWitnessDiagnosisKind Kind) {
   auto LocalMissing = getLocalMissingWitness();
 
+  SmallVector<MissingWitness, 4> MissingWitnessScratch;
+  LocalMissing = pruneMissingWitnesses(
+      *this, Proto, Conformance, LocalMissing, MissingWitnessScratch);
+
   // If this conformance has nothing to complain, return.
   if (LocalMissing.empty())
     return false;
@@ -4451,6 +4581,41 @@ void ConformanceChecker::checkConformance(MissingWitnessDiagnosisKind Kind) {
   }
 }
 
+/// Retrieve the Objective-C method key from the given function.
+auto ConformanceChecker::getObjCMethodKey(AbstractFunctionDecl *func)
+    -> ObjCMethodKey {
+  return ObjCMethodKey(func->getObjCSelector(), func->isInstanceMember());
+}
+
+/// Retrieve the Objective-C requirements in this protocol that have the
+/// given Objective-C method key.
+ArrayRef<AbstractFunctionDecl *>
+ConformanceChecker::getObjCRequirements(ObjCMethodKey key) {
+  auto proto = Conformance->getProtocol();
+  if (!proto->isObjC())
+    return { };
+
+  // Fill in the data structure if we haven't done so yet.
+  if (!computedObjCMethodRequirements) {
+    for (auto requirement : proto->getSemanticMembers()) {
+      auto funcRequirement = dyn_cast<AbstractFunctionDecl>(requirement);
+      if (!funcRequirement)
+        continue;
+
+      objcMethodRequirements[getObjCMethodKey(funcRequirement)]
+          .push_back(funcRequirement);
+    }
+
+    computedObjCMethodRequirements = true;
+  }
+
+  auto known = objcMethodRequirements.find(key);
+  if (known == objcMethodRequirements.end())
+    return { };
+
+  return known->second;
+}
+
 void swift::diagnoseConformanceFailure(Type T,
                                        ProtocolDecl *Proto,
                                        DeclContext *DC,

lib/Sema/TypeCheckProtocol.h

@@ -678,6 +678,12 @@ class DelayedMissingWitnesses : public MissingWitnessesBase {
 /// This helper class handles most of the details of checking whether a
 /// given type (\c Adoptee) conforms to a protocol (\c Proto).
 class ConformanceChecker : public WitnessChecker {
+public:
+  /// Key that can be used to uniquely identify a particular Objective-C
+  /// method.
+  using ObjCMethodKey = std::pair<ObjCSelector, char>;
+
+private:
   friend class MultiConformanceChecker;
   friend class AssociatedTypeInference;
 
@@ -712,6 +718,14 @@ class ConformanceChecker : public WitnessChecker {
   /// Whether we checked the requirement signature already.
   bool CheckedRequirementSignature = false;
 
+  /// Mapping from Objective-C methods to the set of requirements within this
+  /// protocol that have the same selector and instance/class designation.
+  llvm::SmallDenseMap<ObjCMethodKey, TinyPtrVector<AbstractFunctionDecl *>, 4>
+    objcMethodRequirements;
+
+  /// Whether objcMethodRequirements has been computed.
+  bool computedObjCMethodRequirements = false;
+
   /// Retrieve the associated types that are referenced by the given
   /// requirement with a base of 'Self'.
   ArrayRef<AssociatedTypeDecl *> getReferencedAssociatedTypes(ValueDecl *req);
@@ -827,6 +841,13 @@ class ConformanceChecker : public WitnessChecker {
   /// Check the entire protocol conformance, ensuring that all
   /// witnesses are resolved and emitting any diagnostics.
   void checkConformance(MissingWitnessDiagnosisKind Kind);
+
+  /// Retrieve the Objective-C method key from the given function.
+  ObjCMethodKey getObjCMethodKey(AbstractFunctionDecl *func);
+
+  /// Retrieve the Objective-C requirements in this protocol that have the
+  /// given Objective-C method key.
+  ArrayRef<AbstractFunctionDecl *> getObjCRequirements(ObjCMethodKey key);
 };
 
 /// Captures the state needed to infer associated types.

test/decl/protocol/conforms/objc_async.swift

@@ -0,0 +1,52 @@
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck -I %S/Inputs/custom-modules -enable-experimental-concurrency %s -verify -verify-ignore-unknown
+
+// REQUIRES: objc_interop
+import Foundation
+import ObjCConcurrency
+
+// Conform via async method
+class C1: ConcurrentProtocol {
+  func askUser(toSolvePuzzle puzzle: String) async throws -> String? { nil }
+
+  func askUser(toJumpThroughHoop hoop: String) async -> String { "hello" }
+}
+
+// Conform via completion-handler method
+class C2: ConcurrentProtocol {
+  func askUser(toSolvePuzzle puzzle: String, completionHandler: ((String?, Error?) -> Void)?) {
+    completionHandler?("hello", nil)
+  }
+
+  func askUser(toJumpThroughHoop hoop: String, completionHandler: ((String) -> Void)?) {
+    completionHandler?("hello")
+  }
+}
+
+// Conform via both; this is an error
+class C3: ConcurrentProtocol {
+  // expected-note@+1{{method 'askUser(toSolvePuzzle:)' declared here}}
+  func askUser(toSolvePuzzle puzzle: String) async throws -> String? { nil }
+
+  // expected-error@+1{{'askUser(toSolvePuzzle:completionHandler:)' with Objective-C selector 'askUserToSolvePuzzle:completionHandler:' conflicts with method 'askUser(toSolvePuzzle:)' with the same Objective-C selector}}
+  func askUser(toSolvePuzzle puzzle: String, completionHandler: ((String?, Error?) -> Void)?) {
+    completionHandler?("hello", nil)
+  }
+}
+
+// Conform but forget to supply either. Also an error.
+// FIXME: Suppress one of the notes?
+class C4: ConcurrentProtocol { // expected-error{{type 'C4' does not conform to protocol 'ConcurrentProtocol'}}
+}
+
+class C5 {
+}
+
+extension C5: ConcurrentProtocol {
+  func askUser(toSolvePuzzle puzzle: String, completionHandler: ((String?, Error?) -> Void)?) {
+    completionHandler?("hello", nil)
+  }
+
+  func askUser(toJumpThroughHoop hoop: String, completionHandler: ((String) -> Void)?) {
+    completionHandler?("hello")
+  }
+}