Merge pull request #41828 from beccadax/a-breach-of-protocol-can-cause-a-conflict

Check protocols for selector conflicts

Author: beccadax

include/swift/AST/ASTContext.h

@@ -988,11 +988,11 @@ class ASTContext final {
   /// one.
   void loadExtensions(NominalTypeDecl *nominal, unsigned previousGeneration);
 
-  /// Load the methods within the given class that produce
+  /// Load the methods within the given type that produce
   /// Objective-C class or instance methods with the given selector.
   ///
-  /// \param classDecl The class in which we are searching for @objc methods.
-  /// The search only considers this class and its extensions; not any
+  /// \param tyDecl The type in which we are searching for @objc methods.
+  /// The search only considers this type and its extensions; not any
   /// superclasses.
   ///
   /// \param selector The selector to search for.
@@ -1010,7 +1010,11 @@ class ASTContext final {
   ///
   /// \param swiftOnly If true, only loads methods from imported Swift modules,
   /// skipping the Clang importer.
-  void loadObjCMethods(ClassDecl *classDecl, ObjCSelector selector,
+  ///
+  /// \note Passing a protocol is supported, but currently a no-op, because
+  /// Objective-C protocols cannot be extended in ways that make the ObjC method
+  /// lookup table relevant.
+  void loadObjCMethods(NominalTypeDecl *tyDecl, ObjCSelector selector,
                        bool isInstanceMethod, unsigned previousGeneration,
                        llvm::TinyPtrVector<AbstractFunctionDecl *> &methods,
                        bool swiftOnly = false);

include/swift/AST/Decl.h

@@ -3277,6 +3277,7 @@ class AssociatedTypeDecl : public AbstractTypeParamDecl {
 };
 
 class MemberLookupTable;
+class ObjCMethodLookupTable;
 class ConformanceLookupTable;
 
 // Kinds of pointer types.
@@ -3382,6 +3383,12 @@ class NominalTypeDecl : public GenericTypeDecl, public IterableDeclContext {
     getSatisfiedProtocolRequirementsForMember(const ValueDecl *Member,
                                               bool Sorted) const;
 
+  ObjCMethodLookupTable *ObjCMethodLookup = nullptr;
+
+  /// Create the Objective-C method lookup table, or return \c false if this
+  /// kind of type cannot have Objective-C methods.
+  bool createObjCMethodLookup();
+
   friend class ASTContext;
   friend class MemberLookupTable;
   friend class ConformanceLookupTable;
@@ -3534,6 +3541,24 @@ class NominalTypeDecl : public GenericTypeDecl, public IterableDeclContext {
 
   void setConformanceLoader(LazyMemberLoader *resolver, uint64_t contextData);
 
+  /// Look in this type and its extensions (but not any of its protocols or
+  /// superclasses) for declarations with a given Objective-C selector.
+  ///
+  /// Note that this can find methods, initializers, deinitializers,
+  /// getters, and setters.
+  ///
+  /// \param selector The Objective-C selector of the method we're
+  /// looking for.
+  ///
+  /// \param isInstance Whether we are looking for an instance method
+  /// (vs. a class method).
+  TinyPtrVector<AbstractFunctionDecl *> lookupDirect(ObjCSelector selector,
+                                                     bool isInstance);
+
+  /// Record the presence of an @objc method with the given selector. No-op if
+  /// the type is of a kind which cannot contain @objc methods.
+  void recordObjCMethod(AbstractFunctionDecl *method, ObjCSelector selector);
+
   /// Is this the decl for Optional<T>?
   bool isOptionalDecl() const;
 
@@ -3957,13 +3982,7 @@ using AncestryOptions = OptionSet<AncestryFlags>;
 /// The type of the decl itself is a MetatypeType; use getDeclaredType()
 /// to get the declared type ("Complex" in the above example).
 class ClassDecl final : public NominalTypeDecl {
-  class ObjCMethodLookupTable;
-
   SourceLoc ClassLoc;
-  ObjCMethodLookupTable *ObjCMethodLookup = nullptr;
-
-  /// Create the Objective-C member lookup table.
-  void createObjCMethodLookup();
 
   struct {
     /// The superclass decl and a bit to indicate whether the
@@ -4228,25 +4247,6 @@ class ClassDecl final : public NominalTypeDecl {
   /// the Objective-C runtime.
   StringRef getObjCRuntimeName(llvm::SmallVectorImpl<char> &buffer) const;
 
-  using NominalTypeDecl::lookupDirect;
-
-  /// Look in this class and its extensions (but not any of its protocols or
-  /// superclasses) for declarations with a given Objective-C selector.
-  ///
-  /// Note that this can find methods, initializers, deinitializers,
-  /// getters, and setters.
-  ///
-  /// \param selector The Objective-C selector of the method we're
-  /// looking for.
-  ///
-  /// \param isInstance Whether we are looking for an instance method
-  /// (vs. a class method).
-  TinyPtrVector<AbstractFunctionDecl *> lookupDirect(ObjCSelector selector,
-                                                     bool isInstance);
-
-  /// Record the presence of an @objc method with the given selector.
-  void recordObjCMethod(AbstractFunctionDecl *method, ObjCSelector selector);
-
   // Implement isa/cast/dyncast/etc.
   static bool classof(const Decl *D) {
     return D->getKind() == DeclKind::Class;

include/swift/AST/SourceFile.h

@@ -243,8 +243,8 @@ class SourceFile final : public FileUnit {
   std::vector<ObjCUnsatisfiedOptReq> ObjCUnsatisfiedOptReqs;
 
   /// A selector that is used by two different declarations in the same class.
-  /// Fields: classDecl, selector, isInstanceMethod.
-  using ObjCMethodConflict = std::tuple<ClassDecl *, ObjCSelector, bool>;
+  /// Fields: typeDecl, selector, isInstanceMethod.
+  using ObjCMethodConflict = std::tuple<NominalTypeDecl *, ObjCSelector, bool>;
 
   /// List of Objective-C member conflicts we have found during type checking.
   std::vector<ObjCMethodConflict> ObjCMethodConflicts;

lib/AST/ASTContext.cpp

@@ -1906,11 +1906,24 @@ void ASTContext::loadExtensions(NominalTypeDecl *nominal,
 }
 
 void ASTContext::loadObjCMethods(
-    ClassDecl *classDecl, ObjCSelector selector, bool isInstanceMethod,
+    NominalTypeDecl *tyDecl, ObjCSelector selector, bool isInstanceMethod,
     unsigned previousGeneration,
     llvm::TinyPtrVector<AbstractFunctionDecl *> &methods, bool swiftOnly) {
   PrettyStackTraceSelector stackTraceSelector("looking for", selector);
-  PrettyStackTraceDecl stackTraceDecl("...in", classDecl);
+  PrettyStackTraceDecl stackTraceDecl("...in", tyDecl);
+
+  // @objc protocols cannot have @objc extension members, so if we've recorded
+  // everything in the protocol definition, we've recorded everything. And we
+  // only ever use the ObjCSelector version of `NominalTypeDecl::lookupDirect()`
+  // on protocols in primary file typechecking, so we don't care about protocols
+  // that need to be loaded from files.
+  // TODO: Rework `ModuleLoader::loadObjCMethods()` to support protocols too if
+  //       selector-based `NominalTypeDecl::lookupDirect()` ever needs to work
+  //       in more situations.
+  ClassDecl *classDecl = dyn_cast<ClassDecl>(tyDecl);
+  if (!classDecl)
+    return;
+
   for (auto &loader : getImpl().ModuleLoaders) {
     // Ignore the Clang importer if we've been asked for Swift-only results.
     if (swiftOnly && loader.get() == getClangModuleLoader())

lib/AST/NameLookup.cpp

@@ -1125,10 +1125,10 @@ namespace {
 
 /// Class member lookup table, which is a member lookup table with a second
 /// table for lookup based on Objective-C selector.
-class ClassDecl::ObjCMethodLookupTable
+class swift::ObjCMethodLookupTable
         : public llvm::DenseMap<std::pair<ObjCSelector, char>,
                                 StoredObjCMethods>,
-          public ASTAllocated<ClassDecl::ObjCMethodLookupTable>
+          public ASTAllocated<ObjCMethodLookupTable>
 {};
 
 MemberLookupTable::MemberLookupTable(ASTContext &ctx) {
@@ -1483,23 +1483,27 @@ DirectLookupRequest::evaluate(Evaluator &evaluator,
                                       includeAttrImplements);
 }
 
-void ClassDecl::createObjCMethodLookup() {
+bool NominalTypeDecl::createObjCMethodLookup() {
   assert(!ObjCMethodLookup && "Already have an Objective-C member table");
+
+  // Most types cannot have ObjC methods.
+  if (!(isa<ClassDecl>(this) || isa<ProtocolDecl>(this)))
+    return false;
+
   auto &ctx = getASTContext();
   ObjCMethodLookup = new (ctx) ObjCMethodLookupTable();
 
   // Register a cleanup with the ASTContext to call the lookup table
   // destructor.
-  ctx.addCleanup([this]() {
-    this->ObjCMethodLookup->~ObjCMethodLookupTable();
-  });
+  ctx.addDestructorCleanup(*ObjCMethodLookup);
+
+  return true;
 }
 
 TinyPtrVector<AbstractFunctionDecl *>
-ClassDecl::lookupDirect(ObjCSelector selector, bool isInstance) {
-  if (!ObjCMethodLookup) {
-    createObjCMethodLookup();
-  }
+NominalTypeDecl::lookupDirect(ObjCSelector selector, bool isInstance) {
+  if (!ObjCMethodLookup && !createObjCMethodLookup())
+    return {};
 
   // If any modules have been loaded since we did the search last (or if we
   // hadn't searched before), look in those modules, too.
@@ -1514,11 +1518,10 @@ ClassDecl::lookupDirect(ObjCSelector selector, bool isInstance) {
   return stored.Methods;
 }
 
-void ClassDecl::recordObjCMethod(AbstractFunctionDecl *method,
-                                 ObjCSelector selector) {
-  if (!ObjCMethodLookup) {
-    createObjCMethodLookup();
-  }
+void NominalTypeDecl::recordObjCMethod(AbstractFunctionDecl *method,
+                                       ObjCSelector selector) {
+  if (!ObjCMethodLookup && !createObjCMethodLookup())
+    return;
 
   // Record the method.
   bool isInstanceMethod = method->isObjCInstanceMethod();

lib/ClangImporter/ImportDecl.cpp

@@ -4815,12 +4815,12 @@ namespace {
       decl->setIsDynamic(true);
 
       // If the declaration we attached the 'objc' attribute to is within a
-      // class, record it in the class.
+      // type, record it in the type.
       if (auto contextTy = decl->getDeclContext()->getDeclaredInterfaceType()) {
-        if (auto classDecl = contextTy->getClassOrBoundGenericClass()) {
+        if (auto tyDecl = contextTy->getNominalOrBoundGenericNominal()) {
           if (auto method = dyn_cast<AbstractFunctionDecl>(decl)) {
             if (name)
-              classDecl->recordObjCMethod(method, *name);
+              tyDecl->recordObjCMethod(method, *name);
           }
         }
       }

lib/Sema/TypeCheckDeclObjC.cpp

@@ -2008,9 +2008,9 @@ void markAsObjC(ValueDecl *D, ObjCReason reason,
       }
     }
 
-    // Record the method in the class, if it's a member of one.
-    if (auto classDecl = D->getDeclContext()->getSelfClassDecl()) {
-      classDecl->recordObjCMethod(method, selector);
+    // Record the method in the type, if it's a member of one.
+    if (auto tyDecl = D->getDeclContext()->getSelfNominalTypeDecl()) {
+      tyDecl->recordObjCMethod(method, selector);
     }
 
     // Record the method in the source file.
@@ -2406,11 +2406,11 @@ bool swift::diagnoseUnintendedObjCMethodOverrides(SourceFile &sf) {
 /// Retrieve the source file for the given Objective-C member conflict.
 static TinyPtrVector<AbstractFunctionDecl *>
 getObjCMethodConflictDecls(const SourceFile::ObjCMethodConflict &conflict) {
-  ClassDecl *classDecl = std::get<0>(conflict);
+  NominalTypeDecl *typeDecl = std::get<0>(conflict);
   ObjCSelector selector = std::get<1>(conflict);
   bool isInstanceMethod = std::get<2>(conflict);
 
-  return classDecl->lookupDirect(selector, isInstanceMethod);
+  return typeDecl->lookupDirect(selector, isInstanceMethod);
 }
 
 static ObjCAttr *getObjCAttrIfFromAccessNote(ValueDecl *VD) {
@@ -2447,6 +2447,7 @@ bool swift::diagnoseObjCMethodConflicts(SourceFile &sf) {
   // Diagnose each conflict.
   bool anyConflicts = false;
   for (const auto &conflict : localConflicts) {
+    NominalTypeDecl *tyDecl = std::get<0>(conflict);
     ObjCSelector selector = std::get<1>(conflict);
 
     auto methods = getObjCMethodConflictDecls(conflict);
@@ -2530,6 +2531,9 @@ bool swift::diagnoseObjCMethodConflicts(SourceFile &sf) {
                                      origDiagInfo.first, origDiagInfo.second,
                                      selector);
 
+      // Protocols weren't checked for selector conflicts in 5.0.
+      diag.warnUntilSwiftVersionIf(!isa<ClassDecl>(tyDecl), 6);
+
       auto objcAttr = getObjCAttrIfFromAccessNote(conflictingDecl);
       swift::softenIfAccessNote(conflictingDecl, objcAttr, diag);
       if (objcAttr)

test/ClangImporter/objc_bridging_custom.swift

@@ -159,8 +159,9 @@ protocol TestProto {
   @objc optional func testUnmigrated(a: NSRuncingMode, b: Refrigerator, c: NSCoding) // expected-note {{here}} {{none}}
   @objc optional func testPartialMigrated(a: NSRuncingMode, b: Refrigerator) // expected-note {{here}} {{none}}
 
-  @objc optional subscript(a a: Refrigerator) -> Refrigerator? { get } // expected-note {{here}} {{none}}
+  @objc optional subscript(a a: Refrigerator) -> Refrigerator? { get } // expected-note 2 {{here}} {{none}}
   @objc optional subscript(generic a: ManufacturerInfo<NSString>) -> ManufacturerInfo<NSString>? { get } // expected-note {{here}} {{none}}
+  // expected-warning@-1 {{subscript getter with Objective-C selector 'objectForKeyedSubscript:' conflicts with previous declaration with the same Objective-C selector; this is an error in Swift 6}}
 
   @objc optional var prop: Refrigerator? { get } // expected-note {{here}} {{none}}
   @objc optional var propGeneric: ManufacturerInfo<NSString>? { get } // expected-note {{here}} {{none}}

test/Constraints/common_type_objc.swift

@@ -6,11 +6,11 @@
 import Foundation
 
 @objc protocol P {
-  func foo(_ i: Int) -> Double
-  func foo(_ d: Double) -> Double
+  func foo(_ i: Int) -> Double // expected-note {{'foo' previously declared here}}
+  func foo(_ d: Double) -> Double // expected-warning {{method 'foo' with Objective-C selector 'foo:' conflicts with previous declaration with the same Objective-C selector; this is an error in Swift 6}}
 
-  @objc optional func opt(_ i: Int) -> Int
-  @objc optional func opt(_ d: Double) -> Int
+  @objc optional func opt(_ i: Int) -> Int // expected-note {{'opt' previously declared here}}
+  @objc optional func opt(_ d: Double) -> Int // expected-warning {{method 'opt' with Objective-C selector 'opt:' conflicts with previous declaration with the same Objective-C selector; this is an error in Swift 6}}
 }
 
 func testOptional(obj: P) {

test/Constraints/overload_filtering_objc.swift

@@ -9,8 +9,8 @@
 import Foundation
 
 @objc protocol P {
-  func foo(_ i: Int) -> Int
-  func foo(_ d: Double) -> Int
+  func foo(_ i: Int) -> Int // expected-note {{'foo' previously declared here}}
+  func foo(_ d: Double) -> Int // expected-warning {{method 'foo' with Objective-C selector 'foo:' conflicts with previous declaration with the same Objective-C selector; this is an error in Swift 6}}
 
   @objc optional func opt(_ i: Int) -> Int
   @objc optional func opt(double: Double) -> Int