[cxx-interop] Use formal C++ interop mode to fix name lookup in module interfaces (#79984)

It is possible for a module interface (e.g., ModuleA) to be generated
with C++ interop disabled, and then rebuilt with C++ interop enabled
(e.g., because ModuleB, which imports ModuleA, has C++ interop enabled).

This circumstance can lead to various issues when name lookup behaves
differently depending on whether C++ interop is enabled, e.g., when
a module name is shadowed by a namespace of the same name---this only
happens in C++ because namespaces do not exist in C. Unfortunately,
naming namespaces the same as a module is a common C++ convention,
leading to many textual interfaces whose fully-qualified identifiers
(e.g., c_module.c_member) cannot be correctly resolved when C++ interop
is enabled (because c_module is shadowed by a namespace of the same
name).

This patch does two things. First, it introduces a new frontend flag,
-formal-cxx-interoperability-mode, which records the C++ interop mode
a module interface was originally compiled with. Doing so allows
subsequent consumers of that interface to interpret it according to the
formal C++ interop mode. Note that the actual "versioning" used by this
flag is very crude: "off" means disabled, and "swift-6" means enabled.
This is done to be compatible with C++ interop compat versioning scheme,
which seems to produce some invalid (but unused) version numbers. The
versioning scheme for both the formal and actual C++ interop modes
should be clarified and fixed in a subsequent patch.

The second thing this patch does is fix the module/namespace collision
issue in module interface files. It uses the formal C++ interop mode to
determine whether it should resolve C++-only decls during name lookup.
For now, the fix is very minimal and conservative: it only filters out
C++ namespaces during unqualified name lookup in an interface that was
originally generated without C++ interop. Doing so should fix the issue
while minimizing the chance for collateral breakge. More cases other
than C++ namespaces should be added in subsequent patches, with
sufficient testing and careful consideration.

rdar://144566922

Author: j-hui

include/swift/AST/DiagnosticsFrontend.def

@@ -573,7 +573,7 @@ ERROR(dont_enable_interop_and_compat,none,
       "'-cxx-interoperability-mode'; remove '-enable-experimental-cxx-interop'", ())
 
 NOTE(valid_cxx_interop_modes,none,
-      "valid arguments to '-cxx-interoperability-mode=' are %0", (StringRef))
+      "valid arguments to '%0' are %1", (StringRef, StringRef))
 NOTE(swift_will_maintain_compat,none,
      "Swift will maintain source compatibility for imported APIs based on the "
      "selected compatibility mode, so updating the Swift compiler will not "

include/swift/Basic/LangOptions.h

@@ -329,6 +329,10 @@ namespace swift {
     /// to the Swift language version.
     version::Version cxxInteropCompatVersion;
 
+    /// What version of C++ interoperability a textual interface was originally
+    /// generated with (if at all).
+    std::optional<version::Version> FormalCxxInteropMode;
+
     void setCxxInteropFromArgs(llvm::opt::ArgList &Args,
                                swift::DiagnosticEngine &Diags);
 

include/swift/ClangImporter/ClangImporter.h

@@ -757,6 +757,15 @@ AccessLevel convertClangAccess(clang::AccessSpecifier access);
 /// and should be parsed using swift::SourceFile::FileIDStr::parse().
 SmallVector<std::pair<StringRef, clang::SourceLocation>, 1>
 getPrivateFileIDAttrs(const clang::Decl *decl);
+
+/// Use some heuristics to determine whether the clang::Decl associated with
+/// \a decl would not exist without C++ interop.
+///
+/// For instance, a namespace is C++-only, but a plain struct is valid in both
+/// C and C++.
+///
+/// Returns false if \a decl was not imported by ClangImporter.
+bool declIsCxxOnly(const Decl *decl);
 } // namespace importer
 
 struct ClangInvocationFileMapping {

include/swift/Option/FrontendOptions.td

@@ -315,6 +315,10 @@ let Flags = [FrontendOption, NoDriverOption, HelpHidden, ModuleInterfaceOptionIg
     Joined<["-"], "enable-destroy-hoisting=">,
     HelpText<"Whether to enable destroy hoisting">,
     MetaVarName<"true|false">;
+  def formal_cxx_interoperability_mode :
+    Joined<["-"], "formal-cxx-interoperability-mode=">,
+    HelpText<"What version of C++ interoperability a textual interface was originally generated with">,
+    MetaVarName<"<cxx-interop-version>|off">;
 }
 
 // Flags that are saved into module interfaces

lib/AST/UnqualifiedLookup.cpp

@@ -487,6 +487,18 @@ void UnqualifiedLookupFactory::addImportedResults(const DeclContext *const dc) {
                  NLKind::UnqualifiedLookup, resolutionKind, dc,
                  Loc, nlOptions);
 
+  if (dc->isInSwiftinterface() &&
+      !dc->getASTContext().LangOpts.FormalCxxInteropMode) {
+    // It's possible that the textual interface was originally compiled without
+    // C++ interop enabled, but is now being imported in another compilation
+    // instance with C++ interop enabled. In that case, we filter out any decls
+    // that only exist due to C++ interop, e.g., namespace.
+    CurModuleResults.erase(std::remove_if(CurModuleResults.begin(),
+                                          CurModuleResults.end(),
+                                          importer::declIsCxxOnly),
+                           CurModuleResults.end());
+  }
+
   // Always perform name shadowing for type lookup.
   if (options.contains(Flags::TypeLookup)) {
     removeShadowedDecls(CurModuleResults, dc);

lib/ClangImporter/ClangImporter.cpp

@@ -94,6 +94,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/TypeSwitch.h"
 #include "llvm/CAS/CASReference.h"
 #include "llvm/CAS/ObjectStore.h"
 #include "llvm/Support/Casting.h"
@@ -8670,3 +8671,22 @@ importer::getPrivateFileIDAttrs(const clang::Decl *decl) {
 
   return files;
 }
+
+bool importer::declIsCxxOnly(const Decl *decl) {
+  if (auto *clangDecl = decl->getClangDecl()) {
+    return llvm::TypeSwitch<const clang::Decl *, bool>(clangDecl)
+        .template Case<const clang::NamespaceAliasDecl>(
+            [](auto) { return true; })
+        .template Case<const clang::NamespaceDecl>([](auto) { return true; })
+        // For the issues this filter function was trying to resolve at its
+        // time of writing, it suffices to only filter out namespaces. But
+        // there are many other kinds of clang::Decls that only appear in C++.
+        // This is obvious for some decls, e.g., templates, using directives,
+        // non-trivial structs, and scoped enums; but it is not obvious for
+        // other kinds of decls, e.g., an enum member or some variable.
+        //
+        // TODO: enumerate those kinds in a more precise and robust way
+        .Default([](auto) { return false; });
+  }
+  return false;
+}

lib/Frontend/CompilerInvocation.cpp

@@ -644,7 +644,8 @@ static void diagnoseCxxInteropCompatMode(Arg *verArg, ArgList &Args,
   auto validVers = {llvm::StringRef("off"), llvm::StringRef("default"),
                     llvm::StringRef("swift-6"), llvm::StringRef("swift-5.9")};
   auto versStr = "'" + llvm::join(validVers, "', '") + "'";
-  diags.diagnose(SourceLoc(), diag::valid_cxx_interop_modes, versStr);
+  diags.diagnose(SourceLoc(), diag::valid_cxx_interop_modes,
+                 verArg->getSpelling(), versStr);
 }
 
 void LangOptions::setCxxInteropFromArgs(ArgList &Args,
@@ -679,6 +680,54 @@ void LangOptions::setCxxInteropFromArgs(ArgList &Args,
       cxxInteropCompatVersion =
           validateCxxInteropCompatibilityMode("swift-5.9").second;
   }
+
+  if (Arg *A = Args.getLastArg(options::OPT_formal_cxx_interoperability_mode)) {
+    // Take formal version from explicitly specified formal version flag
+    StringRef version = A->getValue();
+
+    // FIXME: the only valid modes are 'off' and 'swift-6'; see below.
+    if (version == "off") {
+      FormalCxxInteropMode = std::nullopt;
+    } else if (version == "swift-6") {
+      FormalCxxInteropMode = {6};
+    } else {
+      Diags.diagnose(SourceLoc(), diag::error_invalid_arg_value,
+                     A->getAsString(Args), A->getValue());
+      Diags.diagnose(SourceLoc(), diag::valid_cxx_interop_modes,
+                     A->getSpelling(), "'off', 'swift-6'");
+    }
+  } else {
+    // In the absence of a formal mode flag, we capture it from the current
+    // C++ compat version (if C++ interop is enabled).
+    //
+    // FIXME: cxxInteropCompatVersion is computed based on the Swift language
+    // version, and is either 4, 5, 6, or 7 (even though only 5.9 and 6.* make
+    // any sense). For now, we don't actually care about the version, so we'll
+    // just use version 6 (i.e., 'swift-6') to mean that C++ interop mode is on.
+    if (EnableCXXInterop)
+      FormalCxxInteropMode = {6};
+    else
+      FormalCxxInteropMode = std::nullopt;
+  }
+}
+
+static std::string printFormalCxxInteropVersion(const LangOptions &Opts) {
+  std::string str;
+  llvm::raw_string_ostream OS(str);
+
+  OS << "-formal-cxx-interoperability-mode=";
+
+  // We must print a 'stable' C++ interop version here, which cannot be
+  // 'default' and 'upcoming-swift' (since those are relative to the current
+  // version, which may change in the future).
+  if (!Opts.FormalCxxInteropMode) {
+    OS << "off";
+  } else {
+    // FIXME: FormalCxxInteropMode will always be 6 (or nullopt); see above
+    OS << "swift-6";
+  }
+
+  return str;
 }
 
 static std::optional<swift::StrictConcurrency>
@@ -925,6 +974,7 @@ static bool ParseEnabledFeatureArgs(LangOptions &Opts, ArgList &Args,
 
 static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
                           DiagnosticEngine &Diags,
+                          ModuleInterfaceOptions &ModuleInterfaceOpts,
                           const FrontendOptions &FrontendOpts) {
   using namespace options;
   bool buildingFromInterface =
@@ -1478,6 +1528,9 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
     Opts.ClangTargetVariant = llvm::Triple(A->getValue());
 
   Opts.setCxxInteropFromArgs(Args, Diags);
+  if (!Args.hasArg(options::OPT_formal_cxx_interoperability_mode))
+    ModuleInterfaceOpts.PublicFlags.IgnorableFlags +=
+        " " + printFormalCxxInteropVersion(Opts);
 
   Opts.EnableObjCInterop =
       Args.hasFlag(OPT_enable_objc_interop, OPT_disable_objc_interop,
@@ -3893,7 +3946,8 @@ bool CompilerInvocation::parseArgs(
     return true;
   }
 
-  if (ParseLangArgs(LangOpts, ParsedArgs, Diags, FrontendOpts)) {
+  if (ParseLangArgs(LangOpts, ParsedArgs, Diags, ModuleInterfaceOpts,
+                    FrontendOpts)) {
     return true;
   }
 

test/Interop/Cxx/modules/emit-module-interface.swift

@@ -1,20 +1,19 @@
 // RUN: %empty-directory(%t)
 
-// Check if fragile Swift interface with struct
-// extensions can be reparsed:
-// RUN: %target-swift-frontend -swift-version 5 -typecheck -emit-module-interface-path %t/UsesCxxStruct.swiftinterface %s -I %S/Inputs -swift-version 5 -enable-experimental-cxx-interop %S/Inputs/namespace-extension-lib.swift
-// RUN: %target-swift-frontend -swift-version 5 -typecheck-module-from-interface  %t/UsesCxxStruct.swiftinterface -I %S/Inputs -enable-experimental-cxx-interop
+// Check if fragile Swift interface with struct extensions can be reparsed:
+// RUN: %target-swift-frontend -swift-version 5 -typecheck -emit-module-interface-path %t/UsesCxxStruct.swiftinterface %s -I %S/Inputs -swift-version 5 -cxx-interoperability-mode=default %S/Inputs/namespace-extension-lib.swift
+// RUN: %target-swift-frontend -swift-version 5 -typecheck-module-from-interface  %t/UsesCxxStruct.swiftinterface -I %S/Inputs -cxx-interoperability-mode=default
 // RUN: %FileCheck --input-file=%t/UsesCxxStruct.swiftinterface %s
 
-// The textual module interface should not contain the C++ interop flag.
+// The textual module interface should not contain the C++ interop flag, but it
+// should record the C++ interop version it was built with (the formal version):
 // CHECK-NOT: -enable-experimental-cxx-interop
 // CHECK-NOT: -cxx-interoperability-mode
+// CHECK:     -formal-cxx-interoperability-mode=
 
-
-// Check if resilient Swift interface with builtin
-// type extensions can be reparsed:
-// RUN: %target-swift-emit-module-interface(%t/ResilientStruct.swiftinterface) %s -I %S/Inputs -enable-library-evolution -swift-version 5 -enable-experimental-cxx-interop %S/Inputs/namespace-extension-lib.swift -DRESILIENT
-// RUN: %target-swift-typecheck-module-from-interface(%t/ResilientStruct.swiftinterface) -I %S/Inputs -DRESILIENT -enable-experimental-cxx-interop
+// Check if resilient Swift interface with builtin type extensions can be reparsed:
+// RUN: %target-swift-emit-module-interface(%t/ResilientStruct.swiftinterface) %s -I %S/Inputs -enable-library-evolution -swift-version 5 -cxx-interoperability-mode=default %S/Inputs/namespace-extension-lib.swift -DRESILIENT
+// RUN: %target-swift-typecheck-module-from-interface(%t/ResilientStruct.swiftinterface) -I %S/Inputs -DRESILIENT -cxx-interoperability-mode=default
 // RUN: %FileCheck --input-file=%t/ResilientStruct.swiftinterface %s
 
 import Namespaces

test/Interop/Cxx/modules/prevent-module-namespace-shadowed-by-namespace.swift

@@ -0,0 +1,63 @@
+// XFAIL: *
+// This test currently fails because there's no way to explicitly refer to
+// a module that has been shadowed by another declaration, e.g., a namespace.
+// Unlike in the prevent-module-shadowed-by-namespace.swift test, the textual
+// interface is generated with C++ interop enabled, which means namespaces are
+// not filtered out during name lookup when the interface is recompiled later.
+
+// RUN: %empty-directory(%t)
+// RUN: %empty-directory(%t/include)
+// RUN: split-file %s %t
+
+// Compile shim.swift with C++ interop, check that its interface is usable
+// (despite using a mix of C/C++ decls):
+//
+// RUN: %empty-directory(%t/lib)
+// RUN: %target-swift-emit-module-interface(%t/lib/shim.swiftinterface) %t/shim.swift -cxx-interoperability-mode=default -module-name shim -I %t/include
+// RUN: %FileCheck %t/shim.swift < %t/lib/shim.swiftinterface
+// RUN: %target-swift-frontend %t/program.swift -typecheck -verify -cxx-interoperability-mode=default -I %t/include -I %t/lib
+
+//--- include/module.modulemap
+// A Clang module which will first be compiled in C mode, and then later compiled in C++ mode
+module c2cxx {
+  header "c2cxx.h"
+  export *
+}
+
+//--- include/c2cxx.h
+// A header file that defines a namespace with the same name as the module,
+// a common C++ idiom. We want to make sure that it does not shadow the module
+// in textual interfaces generated with C++ interop disabled, but later compiled
+// with C++ interop enabled.
+#ifndef __C2CXX_NAMESPACE_H
+#define __C2CXX_NAMESPACE_H
+typedef int c2cxx_number;                         // always available and resilient
+#ifdef __cplusplus
+namespace c2cxx { typedef c2cxx_number number; }; // only available in C++
+#endif // __cplusplus
+#endif // __C2CXX_NAMESPACE_H
+
+//--- shim.swift
+// A shim around c2cxx that refers to a mixture of namespaced (C++) and
+// top-level (C) decls; requires cxx-interoperability-mode
+import c2cxx
+public func shimId(_ n: c2cxx.number) -> c2cxx_number { return n }
+//                      ^^^^^`- refers to the namespace
+// CHECK: public func shimId(_ n: c2cxx.c2cxx.number) -> c2cxx.c2cxx_number
+//                                ^^^^^\^^^^^`-namespace ^^^^^`-module
+//                                      `-module
+
+@inlinable public func shimIdInline(_ n: c2cxx_number) -> c2cxx.number {
+// CHECK:  public func shimIdInline(_ n: c2cxx.c2cxx_number) -> c2cxx.c2cxx.number
+//                                       ^^^^^`-module          ^^^^^\^^^^^`-namespace
+//                                                                    `-module
+          let m: c2cxx.number = n
+// CHECK: let m: c2cxx.number = n
+//               ^^^^^`-namespace
+          return m
+}
+
+//--- program.swift
+// Uses the shim and causes it to be (re)built from its interface
+import shim
+func numberwang() { let _ = shimId(42) + shimIdInline(24) }

test/Interop/Cxx/modules/prevent-module-shadowed-by-namespace.swift

@@ -0,0 +1,57 @@
+// RUN: %empty-directory(%t)
+// RUN: %empty-directory(%t/include)
+// RUN: split-file %s %t
+
+// Generate textual interface for shim.swift without C++ interop enabled, then
+// re-compile it with C++ interop enabled (because it is used by program.swift,
+// which is compiled with C++ interop enabled).
+//
+// RUN: %empty-directory(%t/lib)
+// RUN: %target-swift-emit-module-interface(%t/lib/shim.swiftinterface) %t/shim.swift -module-name shim -I %t/include
+// RUN: %FileCheck %t/shim.swift < %t/lib/shim.swiftinterface
+// RUN: %target-swift-frontend %t/program.swift -typecheck -verify -cxx-interoperability-mode=default -I %t/include -I %t/lib
+
+//--- include/module.modulemap
+// A Clang module which will first be compiled in C mode, and then later compiled in C++ mode
+module c2cxx {
+  header "c2cxx.h"
+  export *
+}
+
+//--- include/c2cxx.h
+// A header file that defines a namespace with the same name as the module,
+// a common C++ idiom. We want to make sure that it does not shadow the module
+// in textual interfaces generated with C++ interop disabled, but later compiled
+// with C++ interop enabled.
+#ifndef __C2CXX_NAMESPACE_H
+#define __C2CXX_NAMESPACE_H
+typedef int c2cxx_number;                         // always available and resilient
+#ifdef __cplusplus
+namespace c2cxx { typedef c2cxx_number number; }; // only available in C++
+#endif // __cplusplus
+#endif // __C2CXX_NAMESPACE_H
+
+//--- shim.swift
+// A shim around c2cxx that exposes a c2cxx decl in its module interface
+import c2cxx
+
+// Exposes a (fully-qualified) c2cxx decl in its module interface.
+public func shimId(_ n: c2cxx_number) -> c2cxx_number { return n }
+// CHECK: public func shimId(_ n: c2cxx.c2cxx_number) -> c2cxx.c2cxx_number
+//                                ^^^^^`- refers to the module
+
+// @inlinable functions have their bodies splatted in the module interface file;
+// those verbatim bodies may contain unqualified names.
+@inlinable public func shimIdInline(_ n: c2cxx_number) -> c2cxx_number {
+// CHECK:  public func shimIdInline(_ n: c2cxx.c2cxx_number) -> c2cxx.c2cxx_number
+//                                       ^^^^^`- refers to the module
+          let m: c2cxx_number = n
+// CHECK: let m: c2cxx_number = n
+//               ^^^^^^^^^^^^`- verbatim (unqualified)
+          return m
+}
+
+//--- program.swift
+// Uses the shim and causes it to be (re)built from its interface
+import shim
+func numberwang() { let _ = shimId(42) + shimIdInline(24) }