SE-0444: Fix interactions with Cxx interop.

With the upcoming `MemberImportVisibility` feature enabled, code built with Cxx
interop also enabled could be rejected by the compiler with cryptic errors
about the `__ObjC` module not being imported. This is the result of a
surprising implementation detail of Cxx interop. When importing C++ namespaces
and their members, the Clang importer puts these declarations in the Clang
header import module (a.k.a. the bridging header module, `__ObjC`). C++
namespaces don't have a logical modular home in the Swift AST because they can
span multiple modules, so it's understandable why this implementation was
chosen. However, the concrete members of namespaces also get placed in the
`__ObjC` module too, and this really confuses things.

To work around this idiosyncrasy of Cxx interop, I've introduced
`Decl::getModuleContextForNameLookup()` which returns the module that a
declaration would ideally belong to if Cxx interop didn't have this behavior.
This alternative to `Decl::getModuleContext()` is now used everywhere that
`MemberImportVisibility` rules are enforced to provide consistency.

Additionally, I found that I also had to further special-case the header import
module for Cxx interop because it turns out that there are some additional
declarations, beyond imported namespaces, that also live there and need to be
implicitly visible in every source file. The `__ObjC` module is not implicitly
imported in source files when Cxx interop is enabled, so these declarations are
not deemed visible under normal name lookup rules. When I tried to add an
implicit import of `__ObjC` when Cxx interop is enabled, it broke a bunch
tests. So for now, when a decl really belongs to the `__ObjC` module in Cxx
interop mode, we just always allow it to be referenced.

This Cxx interop behavior really needs a re-think in my opinion, but that will
require larger discussions.

Resolves rdar://136600598.

Author: tshortli

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 {

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;
@@ -3941,7 +3977,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/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/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;

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'}}
+}