Warn about async-imported selector conflicts

lib/Sema/TypeCheckDeclObjC.cpp

@@ -2419,13 +2419,6 @@ bool swift::diagnoseUnintendedObjCMethodOverrides(SourceFile &sf) {
   return diagnosedAny;
 }
 
-/// Retrieve the source file for the given Objective-C member conflict.
-static TinyPtrVector<AbstractFunctionDecl *>
-getObjCMethodConflictDecls(const SourceFile::ObjCMethodConflict &conflict) {
-  return conflict.typeDecl->lookupDirect(conflict.selector,
-                                         conflict.isInstanceMethod);
-}
-
 static ObjCAttr *getObjCAttrIfFromAccessNote(ValueDecl *VD) {
   if (auto objc = VD->getAttrs().getAttribute<ObjCAttr>())
     if (objc->getAddedByAccessNote())
@@ -2437,95 +2430,138 @@ static ObjCAttr *getObjCAttrIfFromAccessNote(ValueDecl *VD) {
   return nullptr;
 }
 
+static bool hasCustomObjCName(AbstractFunctionDecl *afd) {
+  if (auto objc = afd->getAttrs().getAttribute<ObjCAttr>())
+    return objc->hasName();
+  return false;
+}
+
+/// Retrieve the methods involved in a specific Objective-C selector
+/// conflict. The list will be sorted so that the first method is the "best" one
+/// and the others can be diagnosed as conflicts with that one.
+static TinyPtrVector<AbstractFunctionDecl *>
+getObjCMethodConflictDecls(const SourceFile::ObjCMethodConflict &conflict) {
+  // Look up all methods involved in the conflict.
+  auto methods = conflict.typeDecl->lookupDirect(conflict.selector,
+                                                 conflict.isInstanceMethod);
+
+  // Erase any invalid or stub declarations. We don't want to complain about
+  // them, because we might already have complained about redeclarations
+  // based on Swift matching.
+  llvm::erase_if(methods, [](AbstractFunctionDecl *afd) -> bool {
+    if (afd->isInvalid())
+      return true;
+
+    if (auto ad = dyn_cast<AccessorDecl>(afd))
+      return ad->getStorage()->isInvalid();
+
+    if (auto *ctor = dyn_cast<ConstructorDecl>(afd)) {
+      if (ctor->hasStubImplementation())
+        return true;
+    }
+    return false;
+  });
+
+  // Sort the conflicting methods from the "strongest" claim to the "weakest".
+  // This puts the "best" method at methods.front() so that others will be
+  // diagnosed as conflicts with that one, and it helps ensure that individual
+  // methods in a conflict set are diagnosed in a deterministic order.
+  llvm::stable_sort(methods,
+                    [](AbstractFunctionDecl *a, AbstractFunctionDecl *b) {
+    #define RULE(aCriterion, bCriterion) do { \
+      bool _aCriterion = (aCriterion), _bCriterion = (bCriterion); \
+      if (!_aCriterion && _bCriterion) \
+        return false; \
+      if (_aCriterion && !_bCriterion) \
+        return true; \
+    } while (0)
+
+    // Is one of these from Objective-C and the other from Swift?
+    // NOTE: Inserting another rule above this will break the hasClangNode()
+    // filtering below.
+    RULE(a->hasClangNode(),
+         b->hasClangNode());
+
+    // Is one of these async and the other not?
+    RULE(a->hasAsync(),
+         b->hasAsync());
+
+    // Is one of these explicit and the other from an access note?
+    RULE(!getObjCAttrIfFromAccessNote(a),
+         !getObjCAttrIfFromAccessNote(b));
+
+    // Is one of these from the main decl and the other an extension?
+    RULE(!isa<ExtensionDecl>(a->getDeclContext()),
+         !isa<ExtensionDecl>(b->getDeclContext()));
+
+    // Does one of these use plain @objc and the other @objc(selector)?
+    RULE(!hasCustomObjCName(a),
+         !hasCustomObjCName(b));
+
+    // Neither has a "stronger" claim, so just try to put them in some sort of
+    // consistent order.
+    OrderDeclarations ordering;
+    return ordering(a, b);
+
+    #undef RULE
+  });
+
+  // If the best method is imported from ObjC, eliminate any other imported ObjC
+  // methods. Selector conflicts between imported ObjC methods are spurious;
+  // they're just the same ObjC method being imported under different names with
+  // different ImportNameVersions.
+  if (!methods.empty() && methods.front()->hasClangNode())
+    llvm::erase_if(methods, [&](AbstractFunctionDecl *afd) {
+      return afd != methods.front() && afd->hasClangNode();
+    });
+
+  return methods;
+}
+
 bool swift::diagnoseObjCMethodConflicts(SourceFile &sf) {
   // If there were no conflicts, we're done.
   if (sf.ObjCMethodConflicts.empty())
     return false;
 
   auto &Ctx = sf.getASTContext();
   DiagnosticStateRAII diagState(Ctx.Diags);
-  OrderDeclarations ordering;
 
-  // Sort the set of conflicts so we get a deterministic order for
-  // diagnostics. We use the first conflicting declaration in each set to
-  // perform the sort.
-  llvm::SmallVector<SourceFile::ObjCMethodConflict, 4> localConflicts;
-  llvm::copy(sf.ObjCMethodConflicts, std::back_inserter(localConflicts));
-  std::sort(localConflicts.begin(), localConflicts.end(),
-            [&](const SourceFile::ObjCMethodConflict &lhs,
-                const SourceFile::ObjCMethodConflict &rhs) {
-              return ordering(getObjCMethodConflictDecls(lhs)[1],
-                              getObjCMethodConflictDecls(rhs)[1]);
-            });
-
-  // Diagnose each conflict.
-  bool anyConflicts = false;
-  for (const auto &conflict : localConflicts) {
+  // Build a list of all the conflicts and the methods involved in them.
+  using ConflictSet = std::pair<SourceFile::ObjCMethodConflict,
+                                TinyPtrVector<AbstractFunctionDecl *>>;
+  llvm::SmallVector<ConflictSet, 4> conflictSets;
+  for (auto conflict : sf.ObjCMethodConflicts) {
     auto methods = getObjCMethodConflictDecls(conflict);
-
-    // Erase any invalid or stub declarations. We don't want to complain about
-    // them, because we might already have complained about redeclarations
-    // based on Swift matching.
-    llvm::erase_if(methods, [](AbstractFunctionDecl *afd) -> bool {
-      if (afd->isInvalid())
-        return true;
-
-      if (auto ad = dyn_cast<AccessorDecl>(afd))
-        return ad->getStorage()->isInvalid();
-
-      if (auto *ctor = dyn_cast<ConstructorDecl>(afd)) {
-        if (ctor->hasStubImplementation())
-          return true;
-      }
-      return false;
-    });
-
     if (methods.size() < 2)
       continue;
+    conflictSets.emplace_back(conflict, methods);
+  }
+
+  // Sort the set of conflicts so the different conflict sets are diagnosed in
+  // the same order. We use the first conflicting declaration in each set to
+  // perform the sort.
+  llvm::stable_sort(conflictSets,
+                    [](const ConflictSet &lhs, const ConflictSet &rhs) {
+                      OrderDeclarations ordering;
+                      return ordering(lhs.second[1], rhs.second[1]);
+                    });
 
+  // Diagnose each conflict.
+  bool anyConflicts = false;
+  for (const auto &conflictSet : conflictSets) {
     // Diagnose the conflict.
     anyConflicts = true;
+
+    const auto &conflict = conflictSet.first;
+    const auto &methods = conflictSet.second;
 
-    /// If true, \p a has a "weaker" claim on the selector than \p b, and the
-    /// conflict diagnostic should appear on \p a instead of \p b.
-    auto areBackwards =
-        [&](AbstractFunctionDecl *a, AbstractFunctionDecl *b) -> bool {
-      #define RULE(aCriterion, bCriterion) do { \
-        bool _aCriterion = (aCriterion), _bCriterion = (bCriterion); \
-        if (!_aCriterion && _bCriterion) \
-          return true; \
-        if (_aCriterion && !_bCriterion) \
-          return false; \
-      } while (0)
-
-      // Is one of these from an access note?
-      RULE(getObjCAttrIfFromAccessNote(a),
-           getObjCAttrIfFromAccessNote(b));
-
-      // Is one of these from the main declaration?
-      RULE(!isa<ExtensionDecl>(a->getDeclContext()),
-           !isa<ExtensionDecl>(b->getDeclContext()));
-
-      // Are these from different source files? If so, fall back to the order in
-      // which the declarations were type checked.
-      // FIXME: This is gross and nondeterministic.
-      if (a->getParentSourceFile() != b->getParentSourceFile())
-        return false;
-
-      // Handle them in source order.
-      return !ordering(a, b);
-
-      #undef RULE
-    };
-
-    MutableArrayRef<AbstractFunctionDecl *> methodsRef(methods);
-    if (areBackwards(methods[0], methods[1]))
-      std::swap(methodsRef[0], methodsRef[1]);
-
+    ArrayRef<AbstractFunctionDecl *> methodsRef(methods);
     auto originalMethod = methods.front();
-    auto conflictingMethods = methodsRef.slice(1);
-
     auto origDiagInfo = getObjCMethodDiagInfo(originalMethod);
+    bool originalIsImportedAsync = originalMethod->hasClangNode() &&
+                                     originalMethod->hasAsync();
+
+    auto conflictingMethods = methodsRef.slice(1);
     for (auto conflictingDecl : conflictingMethods) {
       auto diagInfo = getObjCMethodDiagInfo(conflictingDecl);
 
@@ -2534,16 +2570,24 @@ bool swift::diagnoseObjCMethodConflicts(SourceFile &sf) {
         if (auto accessor = dyn_cast<AccessorDecl>(originalMethod))
           originalDecl = accessor->getStorage();
 
+      // In Swift 5.7, we discovered cases which inadvertently bypassed selector
+      // conflict checking and have to be diagnosed as warnings in Swift 5:
+
+      // * Selectors for imported methods with async variants.
+      bool breakingInSwift5 = originalIsImportedAsync;
+
+      // * Protocol requirements
+      if (!isa<ClassDecl>(conflict.typeDecl))
+        breakingInSwift5 = true;
+
       bool redeclSame = (diagInfo == origDiagInfo);
       auto diag = Ctx.Diags.diagnose(conflictingDecl,
                                      redeclSame ? diag::objc_redecl_same
                                                 : diag::objc_redecl,
                                      diagInfo.first, diagInfo.second,
                                      origDiagInfo.first, origDiagInfo.second,
                                      conflict.selector);
-
-      // Protocols weren't checked for selector conflicts in 5.0.
-      diag.warnUntilSwiftVersionIf(!isa<ClassDecl>(conflict.typeDecl), 6);
+      diag.warnUntilSwiftVersionIf(breakingInSwift5, 6);
 
       auto objcAttr = getObjCAttrIfFromAccessNote(conflictingDecl);
       swift::softenIfAccessNote(conflictingDecl, objcAttr, diag);

test/Concurrency/Inputs/Delegate.h

@@ -21,4 +21,10 @@
 -(void)myAsyncMethod:(void (^ _Nullable)(NSError * _Nullable, NSString * _Nullable))completionHandler;
 @end
 
+@interface Delegate (SwiftImpls)
+
+- (void)makeRequestFromSwift:(Request * _Nonnull)request completionHandler:(void (^ _Nullable)(void))handler;
+
+@end
+
 #endif

test/Concurrency/objc_async_overload.swift

@@ -53,3 +53,10 @@ extension Delegate {
     }
   }
 }
+
+// rdar://95887113 - Implementing an ObjC category method in Swift is not strictly valid, but should be tolerated
+
+extension Delegate {
+  @objc public func makeRequest(fromSwift: Request, completionHandler: (() -> Void)?) {}
+  // expected-warning@-1 {{method 'makeRequest(fromSwift:completionHandler:)' with Objective-C selector 'makeRequestFromSwift:completionHandler:' conflicts with method 'makeRequest(fromSwift:)' with the same Objective-C selector; this is an error in Swift 6}}
+}

test/decl/Inputs/objc_redeclaration_multi_2.swift

@@ -5,7 +5,7 @@ extension Redecl1 {
   @objc(init)
   func initialize() { } // expected-error{{method 'initialize()' with Objective-C selector 'init' conflicts with initializer 'init()' with the same Objective-C selector}}
 
-  @objc func method2() { } // expected-error{{method 'method2()' with Objective-C selector 'method2' conflicts with method 'method2_alias()' with the same Objective-C selector}}
+  @objc func method2() { } // expected-note{{method 'method2()' declared here}}
 }
 
 @objc class Redecl2 {

test/decl/objc_redeclaration.swift

@@ -26,7 +26,7 @@ extension Redecl1 {
   @objc var method1_var_alias: Int {
     @objc(method1) get { return 5 } // expected-error{{getter for 'method1_var_alias' with Objective-C selector 'method1' conflicts with method 'method1()' with the same Objective-C selector}}
 
-    @objc(method2:) set { } // expected-note{{setter for 'method1_var_alias' declared here}}
+    @objc(method2:) set { } // expected-error{{setter for 'method1_var_alias' with Objective-C selector 'method2:' conflicts with method 'method2' with the same Objective-C selector}}
   }
 
   @objc subscript (i: Int) -> Redecl1 {
@@ -37,7 +37,7 @@ extension Redecl1 {
 
 extension Redecl1 {
   @objc
-  func method2(_ x: Int) { } // expected-error{{method 'method2' with Objective-C selector 'method2:' conflicts with setter for 'method1_var_alias' with the same Objective-C selector}}
+  func method2(_ x: Int) { } // expected-note{{method 'method2' declared here}}
 
   @objc(objectAtIndexedSubscript:)
   func indexed(_ x: Int) { } // expected-error{{method 'indexed' with Objective-C selector 'objectAtIndexedSubscript:' conflicts with subscript getter with the same Objective-C selector}}

test/decl/objc_redeclaration_multi.swift

@@ -16,5 +16,5 @@ extension Redecl2 {
 }
 
 extension Redecl1 {
-  @objc(method2) func method2_alias() { } // expected-note{{method 'method2_alias()' declared here}}
+  @objc(method2) func method2_alias() { } // expected-error{{method 'method2_alias()' with Objective-C selector 'method2' conflicts with method 'method2()' with the same Objective-C selector}}
 }

test/decl/protocol/objc.swift

@@ -261,7 +261,7 @@ class C8SubRW2: C8Base {
 
 @available(macOS 12, iOS 15, tvOS 15, watchOS 8, *)
 @objc protocol P9 {
-  @objc(custom:) func f(_: Any) // expected-note 2 {{method 'f' declared here}}
-  @objc(custom:) func g(_: Any) // expected-warning {{method 'g' with Objective-C selector 'custom:' conflicts with method 'f' with the same Objective-C selector; this is an error in Swift 6}}
-  @objc(custom:) func h() async // expected-warning {{method 'h()' with Objective-C selector 'custom:' conflicts with method 'f' with the same Objective-C selector; this is an error in Swift 6}}
+  @objc(custom:) func f(_: Any) // expected-warning {{method 'f' with Objective-C selector 'custom:' conflicts with method 'h()' with the same Objective-C selector; this is an error in Swift 6}}
+  @objc(custom:) func g(_: Any) // expected-warning {{method 'g' with Objective-C selector 'custom:' conflicts with method 'h()' with the same Objective-C selector; this is an error in Swift 6}}
+  @objc(custom:) func h() async // expected-note 2 {{method 'h()' declared here}}
 }