Merge pull request #76756 from tshortli/member-import-visibility-cxx

SE-0444: Fix interactions with Cxx interop

Author: tshortli

include/swift/AST/ClangModuleLoader.h

@@ -147,7 +147,7 @@ class ClangModuleLoader : public ModuleLoader {
   virtual void dumpSwiftLookupTables() const = 0;
 
   /// Returns the module that contains imports and declarations from all loaded
-  /// Objective-C header files.
+  /// header files.
   virtual ModuleDecl *getImportedHeaderModule() const = 0;
 
   /// Retrieves the Swift wrapper for the given Clang module, creating

include/swift/AST/Decl.h

@@ -979,6 +979,12 @@ class alignas(1 << DeclAlignInBits) Decl : public ASTAllocated<Decl> {
   LLVM_READONLY
   ModuleDecl *getModuleContext() const;
 
+  /// Retrieve the module in which this declaration would be found by name
+  /// lookup queries. The result can differ from that of `getModuleContext()`
+  /// when the decl was imported via Cxx interop.
+  LLVM_READONLY
+  ModuleDecl *getModuleContextForNameLookup() const;
+
   /// getASTContext - Return the ASTContext that this decl lives in.
   LLVM_READONLY
   ASTContext &getASTContext() const {

include/swift/AST/Module.h

@@ -614,6 +614,9 @@ class ModuleDecl
   void findDeclaredCrossImportOverlaysTransitive(
       SmallVectorImpl<ModuleDecl *> &overlays);
 
+  /// Returns true if this module is the Clang header import module.
+  bool isClangHeaderImportModule() const;
+
   /// Convenience accessor for clients that know what kind of file they're
   /// dealing with.
   SourceFile &getMainSourceFile() const;

lib/AST/Decl.cpp

@@ -770,6 +770,42 @@ ModuleDecl *Decl::getModuleContext() const {
   return getDeclContext()->getParentModule();
 }
 
+/// If `decl` is an imported Cxx decl, returns the actual module that the decl
+/// was imported from. This is necessary to compensate for the way that
+/// Cxx `namespace` declarations are imported. Since namespaces can span
+/// multiple Clang modules, the corresponding decls and all of their members get
+/// associated with the Clang imported header module (named `__ObjC`). This
+/// utility reaches through the Clang AST to find the actual module that members
+/// of namespaces originated from.
+static ModuleDecl *getModuleContextForNameLookupForCxxDecl(const Decl *decl) {
+  auto &ctx = decl->getASTContext();
+  if (!ctx.LangOpts.EnableCXXInterop)
+    return nullptr;
+
+  if (!decl->hasClangNode())
+    return nullptr;
+
+  auto parentModule = decl->getModuleContext();
+
+  // We only need to look for the real parent module when the existing parent
+  // is the imported header module.
+  if (!parentModule->isClangHeaderImportModule())
+    return nullptr;
+
+  auto clangModule = decl->getClangDecl()->getOwningModule();
+  if (!clangModule)
+    return nullptr;
+
+  return ctx.getClangModuleLoader()->getWrapperForModule(clangModule);
+}
+
+ModuleDecl *Decl::getModuleContextForNameLookup() const {
+  if (auto parentModule = getModuleContextForNameLookupForCxxDecl(this))
+    return parentModule;
+
+  return getModuleContext();
+}
+
 /// Retrieve the diagnostic engine for diagnostics emission.
 DiagnosticEngine &Decl::getDiags() const {
   return getASTContext().Diags;
@@ -3946,7 +3982,7 @@ ValueDecl::getSatisfiedProtocolRequirements(bool Sorted) const {
 std::optional<AttributedImport<ImportedModule>>
 ValueDecl::findImport(const DeclContext *fromDC) const {
   // If the type is from the current module, there's no import.
-  auto module = getModuleContext();
+  auto module = getModuleContextForNameLookup();
   if (module == fromDC->getParentModule())
     return std::nullopt;
 

lib/AST/Module.cpp

@@ -2471,6 +2471,14 @@ bool ModuleDecl::getRequiredBystandersIfCrossImportOverlay(
   return false;
 }
 
+bool ModuleDecl::isClangHeaderImportModule() const {
+  auto importer = getASTContext().getClangModuleLoader();
+  if (!importer)
+    return false;
+
+  return this == importer->getImportedHeaderModule();
+}
+
 namespace {
 struct OverlayFileContents {
   struct Module {

lib/AST/NameLookup.cpp

@@ -2504,7 +2504,7 @@ bool DeclContext::lookupQualified(Type type,
 }
 
 bool DeclContext::isDeclImported(const Decl *decl) const {
-  auto declModule = decl->getDeclContext()->getParentModule();
+  auto declModule = decl->getModuleContextForNameLookup();
   return getASTContext().getImportCache().isImportedBy(declModule, this);
 }
 

lib/PrintAsClang/PrintAsClang.cpp

@@ -404,9 +404,7 @@ writeImports(raw_ostream &out, llvm::SmallPtrSetImpl<ImportModuleTy> &imports,
     if (bridgingHeader.empty())
       return import != &M && import->getName() == M.getName();
 
-    auto importer = static_cast<ClangImporter *>(
-        import->getASTContext().getClangModuleLoader());
-    return import == importer->getImportedHeaderModule();
+    return import->isClangHeaderImportModule();
   };
 
   clang::FileSystemOptions fileSystemOptions;

lib/Sema/TypeCheckNameLookup.cpp

@@ -809,7 +809,7 @@ class MissingImportFixItCache {
 static void diagnoseMissingImportForMember(const ValueDecl *decl,
                                            SourceFile *sf, SourceLoc loc) {
   auto &ctx = sf->getASTContext();
-  auto definingModule = decl->getModuleContext();
+  auto definingModule = decl->getModuleContextForNameLookup();
   ctx.Diags.diagnose(loc, diag::candidate_from_missing_import,
                      decl->getDescriptiveKind(), decl->getName(),
                      definingModule);
@@ -823,7 +823,7 @@ diagnoseAndFixMissingImportForMember(const ValueDecl *decl, SourceFile *sf,
   diagnoseMissingImportForMember(decl, sf, loc);
 
   auto &ctx = sf->getASTContext();
-  auto definingModule = decl->getModuleContext();
+  auto definingModule = decl->getModuleContextForNameLookup();
   SourceLoc bestLoc = ctx.Diags.getBestAddImportFixItLoc(decl, sf);
   if (!bestLoc.isValid())
     return;
@@ -874,9 +874,18 @@ diagnoseAndFixMissingImportForMember(const ValueDecl *decl, SourceFile *sf,
 bool swift::maybeDiagnoseMissingImportForMember(const ValueDecl *decl,
                                                 const DeclContext *dc,
                                                 SourceLoc loc) {
-  if (decl->findImport(dc))
+  if (dc->isDeclImported(decl))
     return false;
 
+  if (dc->getASTContext().LangOpts.EnableCXXInterop) {
+    // With Cxx interop enabled, there are some declarations that always belong
+    // to the Clang header import module which should always be implicitly
+    // visible. However, that module is not implicitly imported in source files
+    // so we need to special case it here and avoid diagnosing.
+    if (decl->getModuleContextForNameLookup()->isClangHeaderImportModule())
+      return false;
+  }
+
   auto sf = dc->getParentSourceFile();
   if (!sf)
     return false;

lib/Serialization/Serialization.cpp

@@ -744,11 +744,7 @@ IdentifierID Serializer::addContainingModuleRef(const DeclContext *DC,
     return CURRENT_MODULE_ID;
   if (M == this->M->getASTContext().TheBuiltinModule)
     return BUILTIN_MODULE_ID;
-
-  auto clangImporter =
-    static_cast<ClangImporter *>(
-      this->M->getASTContext().getClangModuleLoader());
-  if (M == clangImporter->getImportedHeaderModule())
+  if (M->isClangHeaderImportModule())
     return OBJC_HEADER_MODULE_ID;
 
   auto exportedModuleName = file->getExportedModuleName();

test/Interop/Cxx/namespace/Inputs/members-direct.h

@@ -0,0 +1,24 @@
+#ifndef TEST_INTEROP_CXX_CLASS_INPUTS_MEMBERS_DIRECT_H
+#define TEST_INTEROP_CXX_CLASS_INPUTS_MEMBERS_DIRECT_H
+
+#include <members-transitive.h>
+
+namespace DirectNamespace { }
+
+namespace CommonNamespace {
+
+struct DirectStruct {
+  int memberVar;
+};
+
+} // namespace CommonNamespace
+
+CommonNamespace::TransitiveStruct returnsTransitiveStruct() {
+  return CommonNamespace::TransitiveStruct{};
+}
+
+TopLevelTransitiveStruct returnsTopLevelTransitiveStruct() {
+  return TopLevelTransitiveStruct{};
+}
+
+#endif // TEST_INTEROP_CXX_CLASS_INPUTS_MEMBERS_DIRECT_H

test/Interop/Cxx/namespace/Inputs/members-transitive.h

@@ -0,0 +1,18 @@
+#ifndef TEST_INTEROP_CXX_CLASS_INPUTS_MEMBERS_TRANSITIVE_H
+#define TEST_INTEROP_CXX_CLASS_INPUTS_MEMBERS_TRANSITIVE_H
+
+namespace TransitiveNamespace { }
+
+namespace CommonNamespace {
+
+struct TransitiveStruct {
+  bool memberVar;
+};
+
+} // namespace CommonNamespace
+
+struct TopLevelTransitiveStruct {
+  bool memberVar;
+};
+
+#endif // TEST_INTEROP_CXX_CLASS_INPUTS_MEMBERS_TRANSITIVE_H

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

@@ -59,3 +59,13 @@ module Enums {
   header "enums.h"
   requires cplusplus
 }
+
+module MembersDirect {
+  header "members-direct.h"
+  requires cplusplus
+}
+
+module MembersTransitive {
+  header "members-transitive.h"
+  requires cplusplus
+}

test/Interop/Cxx/namespace/member-import-visibility.swift

@@ -0,0 +1,26 @@
+// RUN: %target-typecheck-verify-swift -I %S/Inputs -enable-experimental-cxx-interop
+// RUN: %target-typecheck-verify-swift -I %S/Inputs -enable-experimental-cxx-interop -enable-upcoming-feature MemberImportVisibility -verify-additional-prefix member-visibility-
+
+import MembersDirect
+
+// expected-member-visibility-note@-1 4 {{add import of module 'MembersTransitive'}}
+
+extension DirectNamespace { }
+extension TransitiveNamespace { } // expected-error {{cannot find type 'TransitiveNamespace' in scope}}
+
+extension CommonNamespace.DirectStruct {
+  func extensionMethod() -> Int32 {
+    return memberVar
+  }
+}
+
+extension CommonNamespace.TransitiveStruct { // expected-member-visibility-error {{struct 'TransitiveStruct' is not available due to missing import of defining module 'MembersTransitive'}}
+  func extensionMethod() -> Bool {
+    return memberVar // expected-member-visibility-error {{property 'memberVar' is not available due to missing import of defining module 'MembersTransitive'}}
+  }
+}
+
+func test() {
+  let _: Bool = returnsTransitiveStruct().memberVar // expected-member-visibility-error {{property 'memberVar' is not available due to missing import of defining module 'MembersTransitive'}}
+  let _: Bool = returnsTopLevelTransitiveStruct().memberVar // expected-member-visibility-error {{property 'memberVar' is not available due to missing import of defining module 'MembersTransitive'}}
+}

tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp

@@ -1043,8 +1043,6 @@ static void collectModuleDependencies(ModuleDecl *TopMod,
   if (!TopMod)
     return;
 
-  auto ClangModuleLoader = TopMod->getASTContext().getClangModuleLoader();
-
   ModuleDecl::ImportFilter ImportFilter = {
       ModuleDecl::ImportFilterKind::Exported,
       ModuleDecl::ImportFilterKind::Default};
@@ -1061,8 +1059,7 @@ static void collectModuleDependencies(ModuleDecl *TopMod,
     if (Mod->isSystemModule())
       continue;
     // FIXME: Setup dependencies on the included headers.
-    if (ClangModuleLoader &&
-        Mod == ClangModuleLoader->getImportedHeaderModule())
+    if (Mod->isClangHeaderImportModule())
       continue;
     bool NewVisit = Visited.insert(Mod).second;
     if (!NewVisit)