Clang importer: centralize the "suppress declaration import" logic.

The Swift name lookup tables and the complete Objective-C "container"
to Swift DeclContext mapping code used similar-but-different logic to
determine when to suppress a declaration (e.g., when suppressing the
accessors for a property). Centralize the logic so we get the same
behavior in both places.

Author: DougGregor

lib/ClangImporter/ClangImporter.cpp

@@ -2984,6 +2984,27 @@ isAccessibilityConformingContext(const clang::DeclContext *ctx) {
 
 }
 
+/// Determine whether the given method potentially conflicts with the
+/// setter for a property in the given protocol.
+static bool
+isPotentiallyConflictingSetter(const clang::ObjCProtocolDecl *proto,
+                               const clang::ObjCMethodDecl *method) {
+  auto sel = method->getSelector();
+  if (sel.getNumArgs() != 1)
+    return false;
+
+  clang::IdentifierInfo *setterID = sel.getIdentifierInfoForSlot(0);
+  if (!setterID || !setterID->getName().startswith("set"))
+    return false;
+
+  for (auto *prop : proto->properties()) {
+    if (prop->getSetterName() == sel)
+      return true;
+  }
+
+  return false;
+}
+
 bool ClangImporter::Implementation::shouldSuppressDeclImport(
        const clang::Decl *decl) {
   if (auto objcMethod = dyn_cast<clang::ObjCMethodDecl>(decl)) {
@@ -2993,13 +3014,49 @@ bool ClangImporter::Implementation::shouldSuppressDeclImport(
     //
     // Note that this is suppressed for certain accessibility declarations,
     // which are imported as getter/setter pairs and not properties.
-    return objcMethod->isPropertyAccessor() && !isAccessibilityDecl(objcMethod);
+    if (objcMethod->isPropertyAccessor()) {
+      // Suppress the import of this method when the corresponding
+      // property is not suppressed.
+      return !shouldSuppressDeclImport(
+               objcMethod->findPropertyDecl(/*checkOverrides=*/false));
+    }
+
+    // If the method was declared within a protocol, check that it
+    // does not conflict with the setter of a property.
+    if (auto proto = dyn_cast<clang::ObjCProtocolDecl>(decl->getDeclContext()))
+      return isPotentiallyConflictingSetter(proto, objcMethod);
+
+    return false;
   }
 
   if (auto objcProperty = dyn_cast<clang::ObjCPropertyDecl>(decl)) {
     // Suppress certain accessibility properties; they're imported as
     // getter/setter pairs instead.
-    return isAccessibilityDecl(objcProperty);
+    if (isAccessibilityDecl(objcProperty))
+      return true;
+
+    // Check whether there is a superclass method for the getter that
+    // is *not* suppressed, in which case we will need to suppress
+    // this property.
+    auto dc = objcProperty->getDeclContext();
+    auto objcClass = dyn_cast<clang::ObjCInterfaceDecl>(dc);
+    if (!objcClass) {
+      if (auto objcCategory = dyn_cast<clang::ObjCCategoryDecl>(dc))
+        objcClass = objcCategory->getClassInterface();
+    }
+
+    if (objcClass) {
+      if (auto objcSuperclass = objcClass->getSuperClass()) {
+        if (auto getterMethod
+              = objcSuperclass->lookupInstanceMethod(
+                  objcProperty->getGetterName())) {
+          if (!shouldSuppressDeclImport(getterMethod))
+            return true;
+        }
+      }
+    }
+
+    return false;
   }
 
   return false;

lib/ClangImporter/ImportDecl.cpp

@@ -4197,25 +4197,6 @@ namespace {
       }
     }
 
-    static bool
-    isPotentiallyConflictingSetter(const clang::ObjCProtocolDecl *proto,
-                                   const clang::ObjCMethodDecl *method) {
-      auto sel = method->getSelector();
-      if (sel.getNumArgs() != 1)
-        return false;
-
-      clang::IdentifierInfo *setterID = sel.getIdentifierInfoForSlot(0);
-      if (!setterID || !setterID->getName().startswith("set"))
-        return false;
-
-      for (auto *prop : proto->properties()) {
-        if (prop->getSetterName() == sel)
-          return true;
-      }
-
-      return false;
-    }
-
     /// Import members of the given Objective-C container and add them to the
     /// list of corresponding Swift members.
     void importObjCMembers(const clang::ObjCContainerDecl *decl,
@@ -4251,27 +4232,9 @@ namespace {
               members.push_back(alternate);
           }
 
-          // Import explicit properties as instance properties, not as separate
-          // getter and setter methods.
-          if (!Impl.isAccessibilityDecl(objcMethod)) {
-            // If this member is a method that is a getter or setter for a
-            // propertythat was imported, don't add it to the list of members
-            // so it won't be found by name lookup. This eliminates the
-            // ambiguity between property names and getter names (by choosing
-            // to only have a variable).
-            if (objcMethod->isPropertyAccessor()) {
-              auto prop = objcMethod->findPropertyDecl(/*checkOverrides=*/false);
-              assert(prop);
-              (void)Impl.importDecl(const_cast<clang::ObjCPropertyDecl *>(prop));
-              // We may have attached this member to an existing property even
-              // if we've failed to import a new property.
-              if (cast<FuncDecl>(member)->isAccessor())
-                continue;
-            } else if (auto *proto = dyn_cast<clang::ObjCProtocolDecl>(decl)) {
-              if (isPotentiallyConflictingSetter(proto, objcMethod))
-                continue;
-            }
-          }
+          // If this declaration shouldn't be visible, don't add it to
+          // the list.
+          if (Impl.shouldSuppressDeclImport(objcMethod)) continue;
         }
 
         members.push_back(member);

test/IDE/Inputs/swift_name_objc.h

@@ -45,6 +45,12 @@ SWIFT_NAME(SomeProtocol)
 @end
 
 @protocol SNCollision
+@property (readonly,nonnull) id reqSetter;
+- (void)setReqSetter:(nonnull id)bar;
+
+@property (readonly,nonnull) id optSetter;
+@optional
+- (void)setOptSetter:(nonnull id)bar;
 @end
 
 @protocol NSAccessibility

test/IDE/dump_swift_lookup_tables_objc.swift

@@ -71,8 +71,12 @@
 // CHECK-NEXT:     NSErrorImports: -[NSErrorImports methodWithFloat:error:]
 // CHECK-NEXT:   objectAtIndexedSubscript:
 // CHECK-NEXT:     SNSomeClass: -[SNSomeClass objectAtIndexedSubscript:]
+// CHECK-NEXT:   optSetter:
+// CHECK-NEXT:     SNCollision: SNCollision.optSetter
 // CHECK-NEXT:   protoInstanceMethodWithX:
 // CHECK-NEXT:     SNSomeProtocol: -[SNSomeProtocol protoInstanceMethodWithX:y:]
+// CHECK-NEXT:   reqSetter:
+// CHECK-NEXT:     SNCollision: SNCollision.reqSetter
 // CHECK-NEXT:   setAccessibilityFloat:
 // CHECK-NEXT:     NSAccessibility: -[NSAccessibility setAccessibilityFloat:]
 // CHECK-NEXT:   subscript: