Increase determinism of selector conflict errors

Refactor ObjC conflict diagnosis code to sort conflict data more thoroughly, filter out unwanted declarations earlier, and just generally behave in ways that are more likely to work correctly.

This change increases the determinism of the ordering of diagnostics and the selection of the “correct” declaration that the others are considered to conflict with, increasing my confidence that the diagnostics will work correctly in untested corner cases or if the compiler is refactored so that declarations are recorded in a different order. It also adds a new selection rule—@objc without vs. with explicit selector—that I believe will slightly improve the diagnostics we produce. And it replaces a lot of really dodgy-looking logic that may have only worked reliably when a conflict involved exactly two methods.

Author: beccadax

lib/Sema/TypeCheckDeclObjC.cpp

@@ -2257,12 +2257,6 @@ namespace {
   /// Produce a deterministic ordering of the given declarations.
   struct OrderDeclarations {
     bool operator()(ValueDecl *lhs, ValueDecl *rhs) const {
-      // If one declaration is imported from ObjC and the other is native Swift,
-      // put the imported Clang one first.
-      if (lhs->hasClangNode() != rhs->hasClangNode()) {
-        return lhs->hasClangNode();
-      }
-
       // If the declarations come from different modules, order based on the
       // module.
       ModuleDecl *lhsModule = lhs->getDeclContext()->getParentModule();
@@ -2425,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())
@@ -2443,99 +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()));
-
-      // Is one of these imported from Objective-C?
-      RULE(a->hasClangNode(),
-           b->hasClangNode());
-
-      // 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);
 
@@ -2544,17 +2570,13 @@ bool swift::diagnoseObjCMethodConflicts(SourceFile &sf) {
         if (auto accessor = dyn_cast<AccessorDecl>(originalMethod))
           originalDecl = accessor->getStorage();
 
-      // Conflicts between two imported ObjC methods are caused by the same
-      // clang decl having several Swift names.
-      if (originalDecl->hasClangNode() && conflictingDecl->hasClangNode())
-        continue;
+      // In Swift 5.7, we discovered cases which inadvertently bypassed selector
+      // conflict checking and have to be diagnosed as warnings in Swift 5:
 
-      bool breakingInSwift5 = false;
-      // Imported ObjC async methods weren't fully checked for selector
-      // conflicts until 5.7.
-      if (originalMethod->hasClangNode() && originalMethod->hasAsync())
-        breakingInSwift5 = true;
-      // Protocols weren't checked for selector conflicts in 5.0.
+      // * Selectors for imported methods with async variants.
+      bool breakingInSwift5 = originalIsImportedAsync;
+      
+      // * Protocol requirements
       if (!isa<ClassDecl>(conflict.typeDecl))
         breakingInSwift5 = true;
 

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}}
 }