C++ Interop: import namespaces redecls as separate extensions

Previously a namespace declaration was imported along with all of its redeclarations, and their members were added to a single Swift extension. This was problematic when a single namespace is declared in multiple modules – the extension belonged to only one of them.
For an example of this, try printing a module interface for `std.string`/`std.iosfwd` – it will be empty, even though the declarations from those modules are actually imported into Swift correctly.

This change makes sure that when we're importing different redeclarations of the same namespace, we're adding them as separate extensions to appropriate modules.

Author: egorzhdan

lib/ClangImporter/ClangImporter.cpp

@@ -2870,6 +2870,11 @@ void ClangImporter::lookupTypeDecl(
           continue;
         }
         auto *imported = Impl.importDecl(clangDecl, Impl.CurrentVersion);
+
+        // Namespaces are imported as extensions for enums.
+        if (auto ext = dyn_cast_or_null<ExtensionDecl>(imported)) {
+          imported = ext->getExtendedNominal();
+        }
         if (auto *importedType = dyn_cast_or_null<TypeDecl>(imported)) {
           foundViaClang = true;
           receiver(importedType);
@@ -3836,11 +3841,22 @@ void ClangImporter::Implementation::lookupValue(
     // If the entry is not visible, skip it.
     if (!isVisibleClangEntry(entry)) continue;
 
-    ValueDecl *decl;
+    ValueDecl *decl = nullptr;
     // If it's a Clang declaration, try to import it.
     if (auto clangDecl = entry.dyn_cast<clang::NamedDecl *>()) {
-      decl = cast_or_null<ValueDecl>(
-          importDeclReal(clangDecl->getMostRecentDecl(), CurrentVersion));
+      bool isNamespace = isa<clang::NamespaceDecl>(clangDecl);
+      Decl *realDecl =
+          importDeclReal(clangDecl->getMostRecentDecl(), CurrentVersion,
+                         /*useCanonicalDecl*/ !isNamespace);
+
+      if (isNamespace) {
+        if (auto extension = cast_or_null<ExtensionDecl>(realDecl))
+          realDecl = extension->getExtendedNominal();
+      }
+
+      if (!realDecl)
+        continue;
+      decl = cast<ValueDecl>(realDecl);
       if (!decl) continue;
     } else if (!name.isSpecial()) {
       // Try to import a macro.

lib/ClangImporter/ImportDecl.cpp

@@ -2469,73 +2469,98 @@ namespace {
     }
 
     Decl *VisitNamespaceDecl(const clang::NamespaceDecl *decl) {
-      // If we have a name for this declaration, use it.
-      Optional<ImportedName> correctSwiftName;
-      auto importedName = importFullName(decl, correctSwiftName);
-      if (!importedName) return nullptr;
-
-      auto extensionDC =
-          Impl.importDeclContextOf(decl, importedName.getEffectiveContext());
-      if (!extensionDC)
-        return nullptr;
-
-      SourceLoc loc = Impl.importSourceLoc(decl->getBeginLoc());
       DeclContext *dc = nullptr;
       // If this is a top-level namespace, don't put it in the module we're
       // importing, put it in the "__ObjC" module that is implicitly imported.
       // This way, if we have multiple modules that all open the same namespace,
       // we won't import multiple enums with the same name in swift.
-      if (extensionDC->getContextKind() == DeclContextKind::FileUnit)
+      if (!decl->getParent()->isNamespace())
         dc = Impl.ImportedHeaderUnit;
       else {
         // This is a nested namespace, we need to find its extension decl
         // context and then use that to find the parent enum. It's important
         // that we add this to the parent enum (in the "__ObjC" module) and not
         // to the extension.
         auto parentNS = cast<clang::NamespaceDecl>(decl->getParent());
-        auto parent = Impl.importDecl(parentNS, getVersion());
+        auto parent =
+            Impl.importDecl(parentNS, getVersion(), /*UseCanonicalDecl*/ false);
         // Sometimes when the parent namespace is imported, this namespace
         // also gets imported. If that's the case, then the parent namespace
         // will be an enum (because it was able to be fully imported) in which
         // case we need to bail here.
-        auto cachedResult =
-            Impl.ImportedDecls.find({decl->getCanonicalDecl(), getVersion()});
+        auto cachedResult = Impl.ImportedDecls.find({decl, getVersion()});
         if (cachedResult != Impl.ImportedDecls.end())
           return cachedResult->second;
         dc = cast<ExtensionDecl>(parent)
                  ->getExtendedType()
                  ->getEnumOrBoundGenericEnum();
       }
-      auto *enumDecl = Impl.createDeclWithClangNode<EnumDecl>(
-          decl, AccessLevel::Public, loc,
-          importedName.getDeclName().getBaseIdentifier(),
-          Impl.importSourceLoc(decl->getLocation()), None, nullptr, dc);
-      if (isa<clang::NamespaceDecl>(decl->getParent()))
-        cast<EnumDecl>(dc)->addMember(enumDecl);
-
-      // We are creating an extension, so put it at the top level. This is done
-      // after creating the enum, though, because we may need the correctly
-      // nested decl context above when creating the enum.
-      while (extensionDC->getParent() &&
-             extensionDC->getContextKind() != DeclContextKind::FileUnit)
-        extensionDC = extensionDC->getParent();
-
-      auto *extension = ExtensionDecl::create(Impl.SwiftContext, loc, nullptr,
-                                              {}, extensionDC, nullptr, decl);
-      Impl.SwiftContext.evaluator.cacheOutput(ExtendedTypeRequest{extension},
-                                              enumDecl->getDeclaredType());
-      Impl.SwiftContext.evaluator.cacheOutput(ExtendedNominalRequest{extension},
-                                              std::move(enumDecl));
-      // Keep track of what members we've already added so we don't add the same
-      // member twice. Note: we can't use "ImportedDecls" for this because we
-      // might import a decl that we don't add (for example, if it was a
-      // parameter to another decl).
-      SmallPtrSet<Decl *, 16> addedMembers;
+
+      EnumDecl *enumDecl = nullptr;
+      // Try to find an already created enum for this namespace.
       for (auto redecl : decl->redecls()) {
-        // This will be reset as the EnumDecl after we return from
-        // VisitNamespaceDecl.
-        Impl.ImportedDecls[{redecl->getCanonicalDecl(), getVersion()}] =
-            extension;
+        auto extension = Impl.ImportedDecls.find({redecl, getVersion()});
+        if (extension != Impl.ImportedDecls.end()) {
+          enumDecl = cast<EnumDecl>(
+              cast<ExtensionDecl>(extension->second)->getExtendedNominal());
+          break;
+        }
+      }
+      // If we're seeing this namespace for the first time, we need to create a
+      // new enum in the "__ObjC" module.
+      if (!enumDecl) {
+        // If we don't have a name for this declaration, bail.
+        Optional<ImportedName> correctSwiftName;
+        auto importedName = importFullName(decl, correctSwiftName);
+        if (!importedName)
+          return nullptr;
+
+        enumDecl = Impl.createDeclWithClangNode<EnumDecl>(
+            decl, AccessLevel::Public,
+            Impl.importSourceLoc(decl->getBeginLoc()),
+            importedName.getDeclName().getBaseIdentifier(),
+            Impl.importSourceLoc(decl->getLocation()), None, nullptr, dc);
+        if (isa<clang::NamespaceDecl>(decl->getParent()))
+          cast<EnumDecl>(dc)->addMember(enumDecl);
+      }
+
+      for (auto redecl : decl->redecls()) {
+        if (Impl.ImportedDecls.find({redecl, getVersion()}) !=
+            Impl.ImportedDecls.end())
+          continue;
+
+        Optional<ImportedName> correctSwiftName;
+        auto importedName = importFullName(redecl, correctSwiftName);
+        if (!importedName)
+          continue;
+
+        auto extensionDC = Impl.importDeclContextOf(
+            redecl, importedName.getEffectiveContext());
+        if (!extensionDC)
+          continue;
+
+        // We are creating an extension, so put it at the top level. This is
+        // done after creating the enum, though, because we may need the
+        // correctly nested decl context above when creating the enum.
+        while (extensionDC->getParent() &&
+               extensionDC->getContextKind() != DeclContextKind::FileUnit)
+          extensionDC = extensionDC->getParent();
+
+        auto *extension = ExtensionDecl::create(
+            Impl.SwiftContext, Impl.importSourceLoc(decl->getBeginLoc()),
+            nullptr, {}, extensionDC, nullptr, redecl);
+        enumDecl->addExtension(extension);
+        Impl.ImportedDecls[{redecl, getVersion()}] = extension;
+
+        Impl.SwiftContext.evaluator.cacheOutput(ExtendedTypeRequest{extension},
+                                                enumDecl->getDeclaredType());
+        Impl.SwiftContext.evaluator.cacheOutput(ExtendedNominalRequest{extension},
+                                                std::move(enumDecl));
+        // Keep track of what members we've already added so we don't add the
+        // same member twice. Note: we can't use "ImportedDecls" for this
+        // because we might import a decl that we don't add (for example, if it
+        // was a parameter to another decl).
+        SmallPtrSet<Decl *, 16> addedMembers;
 
         // Insert these backwards into "namespaceDecls" so we can pop them off
         // the end without loosing order.
@@ -2566,7 +2591,8 @@ namespace {
             continue;
           }
 
-          auto member = Impl.importDecl(nd, getVersion());
+          bool useCanonicalDecl = !isa<clang::NamespaceDecl>(nd);
+          auto member = Impl.importDecl(nd, getVersion(), useCanonicalDecl);
           if (!member || addedMembers.count(member) ||
               isa<clang::NamespaceDecl>(nd))
             continue;
@@ -2581,10 +2607,7 @@ namespace {
         }
       }
 
-      if (!extension->getMembers().empty())
-        enumDecl->addExtension(extension);
-
-      return enumDecl;
+      return Impl.ImportedDecls[{decl, getVersion()}];
     }
 
     Decl *VisitUsingDirectiveDecl(const clang::UsingDirectiveDecl *decl) {
@@ -9235,7 +9258,8 @@ DeclContext *ClangImporter::Implementation::importDeclContextImpl(
   // Category decls with same name can be merged and using canonical decl always
   // leads to the first category of the given name. We'd like to keep these
   // categories separated.
-  auto useCanonical = !isa<clang::ObjCCategoryDecl>(decl);
+  auto useCanonical =
+      !isa<clang::ObjCCategoryDecl>(decl) && !isa<clang::NamespaceDecl>(decl);
   auto swiftDecl = importDeclForDeclContext(ImportingDecl, decl->getName(),
                                             decl, CurrentVersion, useCanonical);
   if (!swiftDecl)

lib/ClangImporter/ImporterImpl.h

@@ -881,10 +881,11 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation
   ///
   /// \returns The imported declaration, or null if this declaration could
   /// not be represented in Swift.
-  Decl *importDeclReal(const clang::NamedDecl *ClangDecl, Version version) {
+  Decl *importDeclReal(const clang::NamedDecl *ClangDecl, Version version,
+                       bool useCanonicalDecl = true) {
     return importDeclAndCacheImpl(ClangDecl, version,
                                   /*SuperfluousTypedefsAreTransparent=*/false,
-                                  /*UseCanonicalDecl*/true);
+                                  /*UseCanonicalDecl*/ useCanonicalDecl);
   }
 
   /// Import a cloned version of the given declaration, which is part of

lib/ClangImporter/SwiftLookupTable.h

@@ -197,11 +197,10 @@ class EffectiveClangContext {
       DC = omDecl->getCanonicalDecl();
     } else if (auto fDecl = dyn_cast<clang::FunctionDecl>(dc)) {
       DC = fDecl->getCanonicalDecl();
-    } else if (auto nsDecl = dyn_cast<clang::NamespaceDecl>(dc)) {
-      DC = nsDecl->getCanonicalDecl();
     } else {
       assert(isa<clang::TranslationUnitDecl>(dc) ||
              isa<clang::LinkageSpecDecl>(dc) ||
+             isa<clang::NamespaceDecl>(dc) ||
              isa<clang::ObjCContainerDecl>(dc) &&
                  "No other kinds of effective Clang contexts");
       DC = dc;

test/Interop/Cxx/namespace/Inputs/module.modulemap

@@ -22,6 +22,17 @@ module FreeFunctionsSecondHeader {
   requires cplusplus
 }
 
+module Submodules {
+  module SubmoduleA {
+    header "submodule-a.h"
+    requires cplusplus
+  }
+  module SubmoduleB {
+    header "submodule-b.h"
+    requires cplusplus
+  }
+}
+
 module Templates {
   header "templates.h"
   requires cplusplus

test/Interop/Cxx/namespace/Inputs/submodule-a.h

@@ -0,0 +1,16 @@
+#ifndef TEST_INTEROP_CXX_NAMESPACE_INPUTS_SUBMODULE_A_H
+#define TEST_INTEROP_CXX_NAMESPACE_INPUTS_SUBMODULE_A_H
+
+namespace NS1 {
+
+namespace NS2 {
+
+struct BasicDeepA {};
+
+} // namespace NS2
+
+struct BasicA {};
+
+} // namespace NS1
+
+#endif // TEST_INTEROP_CXX_NAMESPACE_INPUTS_SUBMODULE_A_H

test/Interop/Cxx/namespace/Inputs/submodule-b.h

@@ -0,0 +1,16 @@
+#ifndef TEST_INTEROP_CXX_NAMESPACE_INPUTS_SUBMODULE_B_H
+#define TEST_INTEROP_CXX_NAMESPACE_INPUTS_SUBMODULE_B_H
+
+namespace NS1 {
+
+namespace NS2 {
+
+struct BasicDeepB {};
+
+} // namespace NS2
+
+struct BasicB {};
+
+} // namespace NS1
+
+#endif // TEST_INTEROP_CXX_NAMESPACE_INPUTS_SUBMODULE_B_H

test/Interop/Cxx/namespace/classes-module-interface.swift

@@ -1,7 +1,7 @@
 // RUN: %target-swift-ide-test -print-module -module-to-print=Classes -I %S/Inputs -source-filename=x -enable-cxx-interop | %FileCheck %s
 
 // CHECK-NOT: extension
-// CHECK: extension ClassesNS1.ClassesNS2 {
+// CHECK: extension ClassesNS1 {
 // CHECK:   struct BasicStruct {
 // CHECK:     init()
 // CHECK:     mutating func basicMember() -> UnsafePointer<CChar>!
@@ -13,7 +13,7 @@
 // CHECK: }
 // CHECK-NOT: extension
 
-// CHECK: extension ClassesNS1 {
+// CHECK: extension ClassesNS1.ClassesNS2 {
 // CHECK:   struct BasicStruct {
 // CHECK:     init()
 // CHECK:     mutating func basicMember() -> UnsafePointer<CChar>!

test/Interop/Cxx/namespace/free-functions-module-interface.swift

@@ -1,24 +1,36 @@
 // RUN: %target-swift-ide-test -print-module -module-to-print=FreeFunctions -I %S/Inputs -source-filename=x -enable-cxx-interop | %FileCheck %s
 
 // CHECK-NOT: extension
+// CHECK: extension FunctionsNS1 {
+// CHECK:   static func basicFunctionTopLevel() -> UnsafePointer<CChar>!
+// CHECK:   static func definedOutOfLine() -> UnsafePointer<CChar>!
+// CHECK:   static func forwardDeclared() -> UnsafePointer<CChar>!
+// CHECK: }
+// CHECK-NOT: extension
+
+// CHECK: extension FunctionsNS1.FunctionsNS2 {
+// CHECK:   static func basicFunctionSecondLevel() -> UnsafePointer<CChar>!
+// CHECK: }
+// CHECK-NOT: extension
+
 // CHECK: extension FunctionsNS1.FunctionsNS2.FunctionsNS3 {
 // CHECK:   static func basicFunctionLowestLevel() -> UnsafePointer<CChar>!
 // CHECK: }
 // CHECK-NOT: extension
 
+// CHECK: extension FunctionsNS1 {
+// CHECK:   static func definedInDefs() -> UnsafePointer<CChar>!
+// CHECK: }
+// CHECK-NOT: extension
+
 // CHECK: extension FunctionsNS1 {
 // CHECK:   static func sameNameInChild() -> UnsafePointer<CChar>!
 // CHECK:   static func sameNameInSibling() -> UnsafePointer<CChar>!
-// CHECK:   static func definedInDefs() -> UnsafePointer<CChar>!
-// CHECK:   static func forwardDeclared() -> UnsafePointer<CChar>!
-// CHECK:   static func basicFunctionTopLevel() -> UnsafePointer<CChar>!
-// CHECK:   static func definedOutOfLine() -> UnsafePointer<CChar>!
 // CHECK: }
 // CHECK-NOT: extension
 
 // CHECK: extension FunctionsNS1.FunctionsNS2 {
 // CHECK:   static func sameNameInChild() -> UnsafePointer<CChar>!
-// CHECK:   static func basicFunctionSecondLevel() -> UnsafePointer<CChar>!
 // CHECK: }
 // CHECK-NOT: extension
 

test/Interop/Cxx/namespace/free-functions-second-header-module-interface.swift

@@ -2,6 +2,5 @@
 
 // TODO: This file doesn't really test anything because functions need not be defined.
 // CHECK: extension FunctionsNS1 {
-// CHECK-NOT: extension
 // CHECK:   static func definedInDefs() -> UnsafePointer<CChar>!
 // CHECK: }

test/Interop/Cxx/namespace/submodules-module-interface.swift

@@ -0,0 +1,28 @@
+// RUN: %target-swift-ide-test -print-module -module-to-print=Submodules.SubmoduleA -I %S/Inputs -source-filename=x -enable-cxx-interop | %FileCheck %s -check-prefix=CHECK-A
+// RUN: %target-swift-ide-test -print-module -module-to-print=Submodules.SubmoduleB -I %S/Inputs -source-filename=x -enable-cxx-interop | %FileCheck %s -check-prefix=CHECK-B
+
+// CHECK-A-NOT: extension
+// CHECK-A: extension NS1 {
+// CHECK-A:   struct BasicA {
+// CHECK-A:   }
+// CHECK-A: }
+// CHECK-A-NOT: extension
+
+// CHECK-A: extension NS1.NS2 {
+// CHECK-A:   struct BasicDeepA {
+// CHECK-A:   }
+// CHECK-A: }
+// CHECK-A-NOT: extension
+
+// CHECK-B-NOT: extension
+// CHECK-B: extension NS1 {
+// CHECK-B:   struct BasicB {
+// CHECK-B:   }
+// CHECK-B: }
+// CHECK-B-NOT: extension
+
+// CHECK-B: extension NS1.NS2 {
+// CHECK-B:   struct BasicDeepB {
+// CHECK-B:   }
+// CHECK-B: }
+// CHECK-B-NOT: extension