[ClangImporter] If a protocol and a class have the same name, add "Protocol".

jrose-apple · jrose-apple · commit a4ea92742680 · 2014-06-11T23:00:00.000Z
.../if/ the protocol and the class are from the same top-level Clang module.
If not, the protocol is /not/ renamed, and users will have to disambiguate
with module qualification.

This kills our hardcoded "RenamedProtocols" list; it turns out this pattern
is more common than we thought /and/ leads to cross-referencing issues.

&lt;rdar://problem/16206627&gt;

Swift SVN r18809
diff --git a/include/swift/Basic/LLVM.h b/include/swift/Basic/LLVM.h
@@ -29,6 +29,7 @@ namespace llvm {
   template <typename T, unsigned N> class SmallPtrSet;
   template <typename T> class SmallVectorImpl;
   template <typename T, unsigned N> class SmallVector;
+  template <unsigned N> class SmallString;
   template<typename T> class ArrayRef;
   template<typename T> class MutableArrayRef;
   template<typename T> class TinyPtrVector;
@@ -52,6 +53,7 @@ namespace swift {
   // Containers
   using llvm::SmallPtrSetImpl;
   using llvm::SmallPtrSet;
+  using llvm::SmallString;
   using llvm::StringRef;
   using llvm::Twine;
   using llvm::SmallVectorImpl;
diff --git a/include/swift/ClangImporter/RenamedProtocols.def b/include/swift/ClangImporter/RenamedProtocols.def
diff --git a/lib/ClangImporter/ClangImporter.cpp b/lib/ClangImporter/ClangImporter.cpp
@@ -554,6 +554,10 @@ ClangImporter::Implementation::getClangSubmoduleForDecl(
     actual = TD->getDefinition();
     if (!actual && !allowForwardDeclaration)
       return Nothing;
+  } else if (auto OPD = dyn_cast<clang::ObjCProtocolDecl>(D)) {
+    actual = OPD->getDefinition();
+    if (!actual && !allowForwardDeclaration)
+      return Nothing;
   }
 
   if (!actual)
@@ -1224,22 +1228,6 @@ void ClangImporter::lookupValue(Identifier name, VisibleDeclConsumer &consumer){
   auto &pp = Impl.Instance->getPreprocessor();
   auto &sema = Impl.Instance->getSema();
 
-  // If the given name matches one of a special set of renamed
-  // protocol names, perform protocol lookup. For example, the
-  // NSObject protocol is named NSObjectProto so that it does not
-  // conflict with the NSObject class.
-  // FIXME: It would be better to put protocols into a submodule, so
-  // that normal name lookup would prefer the class (NSObject) but
-  // protocols would be visible with, e.g., protocols.NSObject.
-  auto lookupNameKind = clang::Sema::LookupOrdinaryName;
-  if (false) { }
-#define RENAMED_PROTOCOL(ObjCName, SwiftName)             \
-  else if (name.str().equals(#SwiftName)) {               \
-    name = Impl.SwiftContext.getIdentifier(#ObjCName);    \
-    lookupNameKind = clang::Sema::LookupObjCProtocolName; \
-  }
-#include "swift/ClangImporter/RenamedProtocols.def"
-
   // Map the name. If we can't represent the Swift name in Clang, bail out now.
   auto clangName = Impl.importName(name);
   if (!clangName)
@@ -1255,73 +1243,71 @@ void ClangImporter::lookupValue(Identifier name, VisibleDeclConsumer &consumer){
     }
   }
 
-  // Perform name lookup into the global scope.
-  // FIXME: Map source locations over.
-  clang::LookupResult lookupResult(sema, clangName, clang::SourceLocation(),
-                                   lookupNameKind);
   bool FoundType = false;
   bool FoundAny = false;
-  if (sema.LookupName(lookupResult, /*Scope=*/0)) {
+  auto processResults = [&](clang::LookupResult &result) {
     // FIXME: Filter based on access path? C++ access control?
-    for (auto decl : lookupResult) {
-      if (auto swiftDecl = Impl.importDeclReal(decl->getUnderlyingDecl()))
+    for (auto decl : result) {
+      if (auto swiftDecl = Impl.importDeclReal(decl->getUnderlyingDecl())) {
         if (auto valueDecl = dyn_cast<ValueDecl>(swiftDecl)) {
           // If the importer gave us a declaration from the stdlib, make sure
           // it does not show up in the lookup results for the imported module.
           if (valueDecl->getDeclContext()->isModuleScopeContext() &&
               valueDecl->getModuleContext() == Impl.getStdlibModule())
             continue;
+          // Check that we didn't pick up something with a remapped name.
+          if (valueDecl->getName() != name)
+            continue;
 
           consumer.foundDecl(valueDecl, DeclVisibilityKind::VisibleAtTopLevel);
           FoundType = FoundType || isa<TypeDecl>(valueDecl);
           FoundAny = true;
         }
+      }
     }
-  }
+  };
+
+  // Perform name lookup into the global scope.
+  // FIXME: Map source locations over.
+  clang::LookupResult lookupResult(sema, clangName, clang::SourceLocation(),
+                                   clang::Sema::LookupOrdinaryName);
+  if (sema.LookupName(lookupResult, /*Scope=*/nullptr))
+    processResults(lookupResult);
 
-  if (lookupNameKind == clang::Sema::LookupOrdinaryName && !FoundType) {
+  if (!FoundType) {
     // Look up a tag name if we did not find a type with this name already.
     // We don't want to introduce multiple types with same name.
     lookupResult.clear(clang::Sema::LookupTagName);
-    if (sema.LookupName(lookupResult, /*Scope=*/0)) {
-      // FIXME: Filter based on access path? C++ access control?
-      for (auto decl : lookupResult) {
-        if (auto swiftDecl = Impl.importDeclReal(decl->getUnderlyingDecl()))
-          if (auto valueDecl = dyn_cast<ValueDecl>(swiftDecl)) {
-            consumer.foundDecl(valueDecl, DeclVisibilityKind::VisibleAtTopLevel);
-            FoundAny = true;
-          }
-      }
-    }
+    if (sema.LookupName(lookupResult, /*Scope=*/nullptr))
+      processResults(lookupResult);
   }
 
-  if (lookupNameKind == clang::Sema::LookupOrdinaryName && !FoundAny) {
-    // Look up a protocol name if we did not find anything with this
-    // name already.
+  // Look up protocol names as well.
+  lookupResult.clear(clang::Sema::LookupObjCProtocolName);
+  if (sema.LookupName(lookupResult, /*Scope=*/nullptr)) {
+    processResults(lookupResult);
+
+  } else if (!FoundAny && name.str().endswith(SWIFT_PROTOCOL_SUFFIX)) {
+    auto noProtoNameStr = name.str().drop_back(strlen(SWIFT_PROTOCOL_SUFFIX));
+    auto protoIdent = &Impl.getClangASTContext().Idents.get(noProtoNameStr);
     lookupResult.clear(clang::Sema::LookupObjCProtocolName);
-    if (sema.LookupName(lookupResult, /*Scope=*/0)) {
-      // FIXME: Filter based on access path? C++ access control?
-      for (auto decl : lookupResult) {
-        if (auto swiftDecl = Impl.importDeclReal(decl->getUnderlyingDecl())) {
-          if (auto valueDecl = dyn_cast<ValueDecl>(swiftDecl)) {
-            consumer.foundDecl(valueDecl, DeclVisibilityKind::VisibleAtTopLevel);
-            FoundAny = true;
-          }
-        }
-      }
-    }
+    lookupResult.setLookupName(protoIdent);
+
+    if (sema.LookupName(lookupResult, /*Scope=*/nullptr))
+      processResults(lookupResult);
+
+    lookupResult.setLookupName(clangName);
   }
 
   // If we *still* haven't found anything, try looking for '<name>Ref'.
   // Eventually, this should be optimized by recognizing this case when
   // generating the clang module.
-  if (lookupNameKind == clang::Sema::LookupOrdinaryName &&
-      !FoundAny && clangID && Impl.SwiftContext.LangOpts.ImportCFTypes) {
+  if (!FoundAny && clangID && Impl.SwiftContext.LangOpts.ImportCFTypes) {
     lookupResult.clear(clang::Sema::LookupOrdinaryName);
 
     llvm::SmallString<128> buffer;
     buffer += clangID->getName();
-    buffer += "Ref";
+    buffer += SWIFT_CFTYPE_SUFFIX;
     auto refIdent = &Impl.Instance->getASTContext().Idents.get(buffer.str());
     lookupResult.setLookupName(refIdent);
 
diff --git a/lib/ClangImporter/ImportDecl.cpp b/lib/ClangImporter/ImportDecl.cpp
@@ -32,6 +32,7 @@
 #include "clang/AST/DeclVisitor.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Lex/Preprocessor.h"
+#include "clang/Sema/Lookup.h"
 
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/Statistic.h"
@@ -986,7 +987,7 @@ namespace {
       theClass->setCheckedInheritanceClause();
       theClass->setAddedImplicitInitializers(); // suppress all initializers
       theClass->setForeign(true);
-      addObjCAttribute(theClass, ObjCSelector(Impl.SwiftContext, 0, className));
+      addObjCAttribute(theClass, className);
       Impl.registerExternalDecl(theClass);
 
       SmallVector<ProtocolDecl *, 4> protocols;
@@ -1028,7 +1029,7 @@ namespace {
 
         // Import 'typedef struct __Blah *BlahRef;' as a CF type.
         if (!SwiftType && Impl.SwiftContext.LangOpts.ImportCFTypes &&
-            Decl->getName().endswith("Ref")) {
+            Decl->getName().endswith(SWIFT_CFTYPE_SUFFIX)) {
           if (auto pointee = CFPointeeInfo::classifyTypedef(Decl)) {
             // If the pointee is a record, consider creating a class type.
             if (pointee.isRecord()) {
@@ -1960,6 +1961,14 @@ namespace {
       }
     }
 
+    /// Add an @objc(name) attribute with the given, optional name expressed as
+    /// selector.
+    ///
+    /// The importer should use this rather than adding the attribute directly.
+    void addObjCAttribute(ValueDecl *decl, Identifier name) {
+      addObjCAttribute(decl, ObjCSelector(Impl.SwiftContext, 0, name));
+    }
+
     Decl *VisitObjCMethodDecl(const clang::ObjCMethodDecl *decl) {
       auto dc = Impl.importDeclContextOf(decl);
       if (!dc)
@@ -3812,19 +3821,8 @@ namespace {
     }
 
     Decl *VisitObjCProtocolDecl(const clang::ObjCProtocolDecl *decl) {
-      // Form the protocol name, using the renaming table when necessary.
-      Identifier name;
-      Identifier origName = Impl.importName(decl->getDeclName());
-      if (false) { }
-#define RENAMED_PROTOCOL(ObjCName, SwiftName)                  \
-      else if (decl->getName().equals(#ObjCName)) {            \
-        name = Impl.SwiftContext.getIdentifier(#SwiftName);    \
-      }
-#include "swift/ClangImporter/RenamedProtocols.def"
-      else {
-        name = origName;
-      }
-
+      clang::DeclarationName clangName = decl->getDeclName();
+      Identifier name = Impl.importName(clangName);
       if (name.empty())
         return nullptr;
 
@@ -3843,6 +3841,44 @@ namespace {
 
       decl = decl->getDefinition();
 
+      // Test to see if there is a value with the same name as the protocol
+      // in the same module.
+      // FIXME: This will miss macros.
+      auto clangModule = Impl.getClangSubmoduleForDecl(decl);
+      if (clangModule.hasValue() && clangModule.getValue())
+        clangModule = clangModule.getValue()->getTopLevelModule();
+
+      auto isInSameModule = [&](const clang::Decl *D) -> bool {
+        auto declModule = Impl.getClangSubmoduleForDecl(D);
+        if (!declModule.hasValue())
+          return false;
+        return *clangModule == declModule.getValue()->getTopLevelModule();
+      };
+
+
+      bool hasConflict = false;
+      clang::LookupResult lookupResult(Impl.getClangSema(), clangName,
+                                       clang::SourceLocation(),
+                                       clang::Sema::LookupOrdinaryName);
+      if (Impl.getClangSema().LookupName(lookupResult, /*scope=*/nullptr)) {
+        hasConflict = std::any_of(lookupResult.begin(), lookupResult.end(),
+                                  isInSameModule);
+      }
+      if (!hasConflict) {
+        lookupResult.clear(clang::Sema::LookupTagName);
+        if (Impl.getClangSema().LookupName(lookupResult, /*scope=*/nullptr)) {
+          hasConflict = std::any_of(lookupResult.begin(), lookupResult.end(),
+                                    isInSameModule);
+        }
+      }
+
+      Identifier origName = name;
+      if (hasConflict) {
+        SmallString<64> nameBuf{name.str()};
+        nameBuf += SWIFT_PROTOCOL_SUFFIX;
+        name = Impl.SwiftContext.getIdentifier(nameBuf.str());
+      }
+
       auto dc = Impl.importDeclContextOf(decl);
       if (!dc)
         return nullptr;
@@ -3860,7 +3896,7 @@ namespace {
                                    name,
                                    None);
       result->computeType();
-      addObjCAttribute(result, ObjCSelector(Impl.SwiftContext, 0, origName));
+      addObjCAttribute(result, origName);
 
       if (declaredNative)
         markMissingSwiftDecl(result);
@@ -3951,7 +3987,7 @@ namespace {
           result->setSuperclass(nsObjectTy);
           result->setCheckedInheritanceClause();
           result->setAddedImplicitInitializers();
-          addObjCAttribute(result, ObjCSelector(Impl.SwiftContext, 0, name));
+          addObjCAttribute(result, name);
           Impl.registerExternalDecl(result);
           return result;
         }
@@ -3990,7 +4026,7 @@ namespace {
       Impl.ImportedDecls[decl->getCanonicalDecl()] = result;
       result->setCircularityCheck(CircularityCheck::Checked);
       result->setAddedImplicitInitializers();
-      addObjCAttribute(result, ObjCSelector(Impl.SwiftContext, 0, name));
+      addObjCAttribute(result, name);
 
       if (declaredNative)
         markMissingSwiftDecl(result);
diff --git a/lib/ClangImporter/ImporterImpl.h b/lib/ClangImporter/ImporterImpl.h
@@ -185,6 +185,9 @@ enum class SpecialMethodKind {
 
 #define SWIFT_NATIVE_ANNOTATION_STRING "__swift native"
 
+#define SWIFT_PROTOCOL_SUFFIX "Protocol"
+#define SWIFT_CFTYPE_SUFFIX "Ref"
+
 /// Describes whether to classify a factory method as an initializer.
 enum class FactoryAsInitKind {
   /// Infer based on name and type (the default).
diff --git a/lib/PrintAsObjC/PrintAsObjC.cpp b/lib/PrintAsObjC/PrintAsObjC.cpp
@@ -87,14 +87,12 @@ class ObjCPrinter : private DeclVisitor<ObjCPrinter>,
     os << " <";
     interleave(protosToPrint,
                [this](const ProtocolDecl *PD) {
-                 auto Name = PD->getName().str();
-                 // Remap back to the original protocol name that may
-                 // have been changed during import.
-                 Name = llvm::StringSwitch<StringRef>(Name)
-#define RENAMED_PROTOCOL(ObjCName, SwiftName) .Case(#SwiftName, #ObjCName)
-#include "swift/ClangImporter/RenamedProtocols.def"
-                  .Default(Name);
-                 os << Name;
+                 if (PD->hasClangNode()) {
+                   SmallString<64> buf;
+                   os << PD->getObjCRuntimeName(buf);
+                 } else {
+                   os << PD->getName().str();
+                  }
                },
                [this] { os << ", "; });
     os << ">";
diff --git a/test/ClangModules/Inputs/custom-modules/ObjCParseExtras.h b/test/ClangModules/Inputs/custom-modules/ObjCParseExtras.h
@@ -60,6 +60,10 @@ __attribute__((objc_root_class))
 - (void)setObject:(id)obj atIndexedSubscript:(int)i;
 @end
 
+@protocol ProtoOrClass
+@property int thisIsTheProto;
+@end
+
 
 #pragma mark Constant global properties
 
diff --git a/test/ClangModules/Inputs/custom-modules/ObjCParseExtrasToo.h b/test/ClangModules/Inputs/custom-modules/ObjCParseExtrasToo.h
@@ -0,0 +1,13 @@
+@import ObjCParseExtrasTooHelper;
+
+@interface ProtoOrClass
+@property int thisClassHasAnAwfulName;
+@end
+
+
+@protocol ClassInHelper
+@end
+
+@interface ProtoInHelper
+- (instancetype)init;
+@end
diff --git a/test/ClangModules/Inputs/custom-modules/ObjCParseExtrasTooHelper.h b/test/ClangModules/Inputs/custom-modules/ObjCParseExtrasTooHelper.h
@@ -0,0 +1,6 @@
+@interface ClassInHelper
+- (instancetype)init;
+@end
+
+@protocol ProtoInHelper
+@end
diff --git a/test/ClangModules/Inputs/custom-modules/module.map b/test/ClangModules/Inputs/custom-modules/module.map
diff --git a/test/ClangModules/objc_parse.swift b/test/ClangModules/objc_parse.swift
