Merge topic 'cxxmodules-private-between-targets'

d38779df2a ci: Enable RunCMake.CXXModules collation cases in clang jobs
69e4525241 Tests/CXXModules: add example for private modules between targets
18f87c87f8 cmCxxModuleMapper: track whether modules are private or not
56f7d6f827 cmCxxModuleMapper: add a structure to represent BMI locations
8207a3a266 cmDyndepCollation: add a query for visibility of an object's modules
e8efcbec8c iwyu: ignore `std::remove_reference` requirements

Acked-by: Kitware Robot <[email protected]>
Merge-request: !8476

.gitlab/ci/configure_linux_clang_cxx_modules_ninja.cmake

@@ -1,4 +1,4 @@
-set(CMake_TEST_MODULE_COMPILATION "named,partitions,internal_partitions,export_bmi,install_bmi,shared" CACHE STRING "")
+set(CMake_TEST_MODULE_COMPILATION "named,collation,partitions,internal_partitions,export_bmi,install_bmi,shared" CACHE STRING "")
 set(CMake_TEST_MODULE_COMPILATION_RULES "${CMAKE_CURRENT_LIST_DIR}/cxx_modules_rules_clang.cmake" CACHE STRING "")
 
 include("${CMAKE_CURRENT_LIST_DIR}/configure_external_test.cmake")

.gitlab/ci/configure_linux_clang_cxx_modules_ninja_multi.cmake

@@ -1,4 +1,4 @@
-set(CMake_TEST_MODULE_COMPILATION "named,partitions,internal_partitions,export_bmi,install_bmi,shared" CACHE STRING "")
+set(CMake_TEST_MODULE_COMPILATION "named,collation,partitions,internal_partitions,export_bmi,install_bmi,shared" CACHE STRING "")
 set(CMake_TEST_MODULE_COMPILATION_RULES "${CMAKE_CURRENT_LIST_DIR}/cxx_modules_rules_clang.cmake" CACHE STRING "")
 
 include("${CMAKE_CURRENT_LIST_DIR}/configure_external_test.cmake")

.gitlab/ci/configure_windows_clang_ninja.cmake

@@ -1,5 +1,5 @@
 if("$ENV{CMAKE_CI_BUILD_NAME}" MATCHES "(^|_)gnu(_|$)")
-  set(CMake_TEST_MODULE_COMPILATION "named,partitions,internal_partitions,export_bmi,install_bmi,shared" CACHE STRING "")
+  set(CMake_TEST_MODULE_COMPILATION "named,collation,partitions,internal_partitions,export_bmi,install_bmi,shared" CACHE STRING "")
   set(CMake_TEST_MODULE_COMPILATION_RULES "${CMAKE_CURRENT_LIST_DIR}/cxx_modules_rules_clang.cmake" CACHE STRING "")
 endif()
 include("${CMAKE_CURRENT_LIST_DIR}/configure_windows_clang_common.cmake")

Source/cmCMakeHostSystemInformationCommand.cxx

@@ -8,7 +8,6 @@
 #include <initializer_list>
 #include <map>
 #include <string>
-#include <type_traits>
 #include <utility>
 
 #include <cm/optional>

Source/cmCxxModuleMapper.cxx

@@ -17,13 +17,59 @@
 #include "cmStringAlgorithms.h"
 #include "cmSystemTools.h"
 
-cm::optional<std::string> CxxModuleLocations::BmiGeneratorPathForModule(
+CxxBmiLocation::CxxBmiLocation() = default;
+
+CxxBmiLocation::CxxBmiLocation(std::string path)
+  : BmiLocation(std::move(path))
+{
+}
+
+CxxBmiLocation CxxBmiLocation::Unknown()
+{
+  return {};
+}
+
+CxxBmiLocation CxxBmiLocation::Private()
+{
+  return { std::string{} };
+}
+
+CxxBmiLocation CxxBmiLocation::Known(std::string path)
+{
+  return { std::move(path) };
+}
+
+bool CxxBmiLocation::IsKnown() const
+{
+  return this->BmiLocation.has_value();
+}
+
+bool CxxBmiLocation::IsPrivate() const
+{
+  if (auto const& loc = this->BmiLocation) {
+    return loc->empty();
+  }
+  return false;
+}
+
+std::string const& CxxBmiLocation::Location() const
+{
+  if (auto const& loc = this->BmiLocation) {
+    return *loc;
+  }
+  static std::string empty;
+  return empty;
+}
+
+CxxBmiLocation CxxModuleLocations::BmiGeneratorPathForModule(
   std::string const& logical_name) const
 {
-  if (auto l = this->BmiLocationForModule(logical_name)) {
-    return this->PathForGenerator(std::move(*l));
+  auto bmi_loc = this->BmiLocationForModule(logical_name);
+  if (bmi_loc.IsKnown() && !bmi_loc.IsPrivate()) {
+    bmi_loc =
+      CxxBmiLocation::Known(this->PathForGenerator(bmi_loc.Location()));
   }
-  return {};
+  return bmi_loc;
 }
 
 namespace {
@@ -42,18 +88,21 @@ std::string CxxModuleMapContentClang(CxxModuleLocations const& loc,
   // A series of flags which tell the compiler where to look for modules.
 
   for (auto const& p : obj.Provides) {
-    if (auto bmi_loc = loc.BmiGeneratorPathForModule(p.LogicalName)) {
+    auto bmi_loc = loc.BmiGeneratorPathForModule(p.LogicalName);
+    if (bmi_loc.IsKnown()) {
       // Force the TU to be considered a C++ module source file regardless of
       // extension.
       mm << "-x c++-module\n";
 
-      mm << "-fmodule-output=" << *bmi_loc << '\n';
+      mm << "-fmodule-output=" << bmi_loc.Location() << '\n';
       break;
     }
   }
   for (auto const& r : obj.Requires) {
-    if (auto bmi_loc = loc.BmiGeneratorPathForModule(r.LogicalName)) {
-      mm << "-fmodule-file=" << r.LogicalName << "=" << *bmi_loc << '\n';
+    auto bmi_loc = loc.BmiGeneratorPathForModule(r.LogicalName);
+    if (bmi_loc.IsKnown()) {
+      mm << "-fmodule-file=" << r.LogicalName << "=" << bmi_loc.Location()
+         << '\n';
     }
   }
 
@@ -76,13 +125,15 @@ std::string CxxModuleMapContentGcc(CxxModuleLocations const& loc,
   mm << "$root " << loc.RootDirectory << "\n";
 
   for (auto const& p : obj.Provides) {
-    if (auto bmi_loc = loc.BmiGeneratorPathForModule(p.LogicalName)) {
-      mm << p.LogicalName << ' ' << *bmi_loc << '\n';
+    auto bmi_loc = loc.BmiGeneratorPathForModule(p.LogicalName);
+    if (bmi_loc.IsKnown()) {
+      mm << p.LogicalName << ' ' << bmi_loc.Location() << '\n';
     }
   }
   for (auto const& r : obj.Requires) {
-    if (auto bmi_loc = loc.BmiGeneratorPathForModule(r.LogicalName)) {
-      mm << r.LogicalName << ' ' << *bmi_loc << '\n';
+    auto bmi_loc = loc.BmiGeneratorPathForModule(r.LogicalName);
+    if (bmi_loc.IsKnown()) {
+      mm << r.LogicalName << ' ' << bmi_loc.Location() << '\n';
     }
   }
 
@@ -123,8 +174,9 @@ std::string CxxModuleMapContentMsvc(CxxModuleLocations const& loc,
       mm << "-internalPartition\n";
     }
 
-    if (auto bmi_loc = loc.BmiGeneratorPathForModule(p.LogicalName)) {
-      mm << "-ifcOutput " << *bmi_loc << '\n';
+    auto bmi_loc = loc.BmiGeneratorPathForModule(p.LogicalName);
+    if (bmi_loc.IsKnown()) {
+      mm << "-ifcOutput " << bmi_loc.Location() << '\n';
     }
   }
 
@@ -132,10 +184,11 @@ std::string CxxModuleMapContentMsvc(CxxModuleLocations const& loc,
   std::set<std::string> transitive_usage_names;
 
   for (auto const& r : obj.Requires) {
-    if (auto bmi_loc = loc.BmiGeneratorPathForModule(r.LogicalName)) {
+    auto bmi_loc = loc.BmiGeneratorPathForModule(r.LogicalName);
+    if (bmi_loc.IsKnown()) {
       auto flag = flag_for_method(r.Method);
 
-      mm << flag << ' ' << r.LogicalName << '=' << *bmi_loc << "\n";
+      mm << flag << ' ' << r.LogicalName << '=' << bmi_loc.Location() << "\n";
       transitive_usage_directs.insert(r.LogicalName);
 
       // Insert transitive usages.
@@ -237,18 +290,28 @@ std::set<std::string> CxxModuleUsageSeed(
   for (cmScanDepInfo const& object : objects) {
     // Add references for each of the provided modules.
     for (auto const& p : object.Provides) {
-      if (auto bmi_loc = loc.BmiGeneratorPathForModule(p.LogicalName)) {
+      auto bmi_loc = loc.BmiGeneratorPathForModule(p.LogicalName);
+      if (bmi_loc.IsKnown()) {
         // XXX(cxx-modules): How to support header units?
-        usages.AddReference(p.LogicalName, *bmi_loc, LookupMethod::ByName);
+        usages.AddReference(p.LogicalName, bmi_loc.Location(),
+                            LookupMethod::ByName);
       }
     }
 
     // For each requires, pull in what is required.
     for (auto const& r : object.Requires) {
-      // Find transitive usages.
-      auto transitive_usages = usages.Usage.find(r.LogicalName);
       // Find the required name in the current target.
       auto bmi_loc = loc.BmiGeneratorPathForModule(r.LogicalName);
+      if (bmi_loc.IsPrivate()) {
+        cmSystemTools::Error(
+          cmStrCat("Unable to use module '", r.LogicalName,
+                   "' as it is 'PRIVATE' and therefore not accessible outside "
+                   "of its owning target."));
+        continue;
+      }
+
+      // Find transitive usages.
+      auto transitive_usages = usages.Usage.find(r.LogicalName);
 
       for (auto const& p : object.Provides) {
         auto& this_usages = usages.Usage[p.LogicalName];
@@ -260,14 +323,14 @@ std::set<std::string> CxxModuleUsageSeed(
         if (transitive_usages != usages.Usage.end()) {
           this_usages.insert(transitive_usages->second.begin(),
                              transitive_usages->second.end());
-        } else if (bmi_loc) {
+        } else if (bmi_loc.IsKnown()) {
           // Mark that we need to update transitive usages later.
           internal_usages[p.LogicalName].insert(r.LogicalName);
         }
       }
 
-      if (bmi_loc) {
-        usages.AddReference(r.LogicalName, *bmi_loc, r.Method);
+      if (bmi_loc.IsKnown()) {
+        usages.AddReference(r.LogicalName, bmi_loc.Location(), r.Method);
       }
     }
   }

Source/cmCxxModuleMapper.h

@@ -22,6 +22,23 @@ enum class CxxModuleMapFormat
   Msvc,
 };
 
+struct CxxBmiLocation
+{
+  static CxxBmiLocation Unknown();
+  static CxxBmiLocation Private();
+  static CxxBmiLocation Known(std::string path);
+
+  bool IsKnown() const;
+  bool IsPrivate() const;
+  std::string const& Location() const;
+
+private:
+  CxxBmiLocation();
+  CxxBmiLocation(std::string path);
+
+  cm::optional<std::string> BmiLocation;
+};
+
 struct CxxModuleLocations
 {
   // The path from which all relative paths should be computed. If
@@ -33,12 +50,11 @@ struct CxxModuleLocations
   std::function<std::string(std::string)> PathForGenerator;
 
   // Lookup the BMI location of a logical module name.
-  std::function<cm::optional<std::string>(std::string const&)>
-    BmiLocationForModule;
+  std::function<CxxBmiLocation(std::string const&)> BmiLocationForModule;
 
   // Returns the generator path (if known) for the BMI given a
   // logical module name.
-  cm::optional<std::string> BmiGeneratorPathForModule(
+  CxxBmiLocation BmiGeneratorPathForModule(
     std::string const& logical_name) const;
 };
 

Source/cmDyndepCollation.cxx

@@ -623,3 +623,20 @@ bool cmDyndepCollation::WriteDyndepMetadata(
 
   return result;
 }
+
+bool cmDyndepCollation::IsObjectPrivate(
+  std::string const& object, cmCxxModuleExportInfo const& export_info)
+{
+#ifdef _WIN32
+  std::string output_path = object;
+  cmSystemTools::ConvertToUnixSlashes(output_path);
+#else
+  std::string const& output_path = object;
+#endif
+  auto fileset_info_itr = export_info.ObjectToFileSet.find(output_path);
+  if (fileset_info_itr == export_info.ObjectToFileSet.end()) {
+    return false;
+  }
+  auto const& file_set = fileset_info_itr->second;
+  return !cmFileSetVisibilityIsForInterface(file_set.Visibility);
+}

Source/cmDyndepCollation.h

@@ -49,4 +49,6 @@ struct cmDyndepCollation
                                   std::vector<cmScanDepInfo> const& objects,
                                   cmCxxModuleExportInfo const& export_info,
                                   cmDyndepMetadataCallbacks const& cb);
+  static bool IsObjectPrivate(std::string const& object,
+                              cmCxxModuleExportInfo const& export_info);
 };

Source/cmGlobalNinjaGenerator.cxx

@@ -2536,7 +2536,12 @@ bool cmGlobalNinjaGenerator::WriteDyndepFile(
   CxxModuleUsage usages;
 
   // Map from module name to module file path, if known.
-  std::map<std::string, std::string> mod_files;
+  struct AvailableModuleInfo
+  {
+    std::string BmiPath;
+    bool IsPrivate;
+  };
+  std::map<std::string, AvailableModuleInfo> mod_files;
 
   // Populate the module map with those provided by linked targets first.
   for (std::string const& linked_target_dir : linked_target_dirs) {
@@ -2560,7 +2565,15 @@ bool cmGlobalNinjaGenerator::WriteDyndepFile(
       Json::Value const& target_modules = ltm["modules"];
       if (target_modules.isObject()) {
         for (auto i = target_modules.begin(); i != target_modules.end(); ++i) {
-          mod_files[i.key().asString()] = i->asString();
+          Json::Value const& visible_module = *i;
+          if (visible_module.isObject()) {
+            Json::Value const& bmi_path = visible_module["bmi"];
+            Json::Value const& is_private = visible_module["is-private"];
+            mod_files[i.key().asString()] = AvailableModuleInfo{
+              bmi_path.asString(),
+              is_private.asBool(),
+            };
+          }
         }
       }
       Json::Value const& target_modules_references = ltm["references"];
@@ -2641,8 +2654,15 @@ bool cmGlobalNinjaGenerator::WriteDyndepFile(
         cmSystemTools::ReplaceString(safe_logical_name, ":", "-");
         mod = cmStrCat(module_dir, safe_logical_name, module_ext);
       }
-      mod_files[p.LogicalName] = mod;
-      target_modules[p.LogicalName] = mod;
+      mod_files[p.LogicalName] = AvailableModuleInfo{
+        mod,
+        false, // Always visible within our own target.
+      };
+      Json::Value& module_info = target_modules[p.LogicalName] =
+        Json::objectValue;
+      module_info["bmi"] = mod;
+      module_info["is-private"] =
+        cmDyndepCollation::IsObjectPrivate(object.PrimaryOutput, export_info);
     }
   }
 
@@ -2662,12 +2682,15 @@ bool cmGlobalNinjaGenerator::WriteDyndepFile(
       return path;
     };
     locs.BmiLocationForModule =
-      [&mod_files](std::string const& logical) -> cm::optional<std::string> {
+      [&mod_files](std::string const& logical) -> CxxBmiLocation {
       auto m = mod_files.find(logical);
       if (m != mod_files.end()) {
-        return m->second;
+        if (m->second.IsPrivate) {
+          return CxxBmiLocation::Private();
+        }
+        return CxxBmiLocation::Known(m->second.BmiPath);
       }
-      return {};
+      return CxxBmiLocation::Unknown();
     };
 
     // Insert information about the current target's modules.
@@ -2689,13 +2712,14 @@ bool cmGlobalNinjaGenerator::WriteDyndepFile(
       build.ImplicitOuts.clear();
       for (auto const& p : object.Provides) {
         build.ImplicitOuts.push_back(
-          this->ConvertToNinjaPath(mod_files[p.LogicalName]));
+          this->ConvertToNinjaPath(mod_files[p.LogicalName].BmiPath));
       }
       build.ImplicitDeps.clear();
       for (auto const& r : object.Requires) {
         auto mit = mod_files.find(r.LogicalName);
         if (mit != mod_files.end()) {
-          build.ImplicitDeps.push_back(this->ConvertToNinjaPath(mit->second));
+          build.ImplicitDeps.push_back(
+            this->ConvertToNinjaPath(mit->second.BmiPath));
         }
       }
       build.Variables.clear();
@@ -2761,7 +2785,7 @@ bool cmGlobalNinjaGenerator::WriteDyndepFile(
     [mod_files](std::string const& name) -> cm::optional<std::string> {
     auto m = mod_files.find(name);
     if (m != mod_files.end()) {
-      return m->second;
+      return m->second.BmiPath;
     }
     return {};
   };

Source/cmString.cxx

@@ -9,7 +9,6 @@
 #include <ostream>
 #include <stdexcept>
 #include <string>
-#include <type_traits>
 
 namespace cm {
 

Tests/CMakeLib/testStringAlgorithms.cxx

@@ -1,12 +1,11 @@
 /* Distributed under the OSI-approved BSD 3-Clause License.  See accompanying
    file Copyright.txt or https://cmake.org/licensing for details.  */
 
-#include <cmConfigure.h> // IWYU pragma: keep
+#include "cmConfigure.h" // IWYU pragma: keep
 
 #include <iostream>
 #include <sstream>
 #include <string>
-#include <type_traits>
 #include <utility>
 #include <vector>
 

Tests/RunCMake/CXXModules/RunCMakeTest.cmake

@@ -155,6 +155,9 @@ endif ()
 # Tests which require collation work.
 if ("collation" IN_LIST CMake_TEST_MODULE_COMPILATION)
   run_cxx_module_test(public-req-private)
+  set(RunCMake_CXXModules_NO_TEST 1)
+  run_cxx_module_test(req-private-other-target)
+  unset(RunCMake_CXXModules_NO_TEST)
 endif ()
 
 # Tests which use named modules in shared libraries.

Tests/RunCMake/CXXModules/examples/req-private-other-target-build-result.txt

@@ -0,0 +1 @@
+1

Tests/RunCMake/CXXModules/examples/req-private-other-target-build-stdout.txt

@@ -0,0 +1 @@
+((Ninja generators)?(Unable to use module 'lib.priv' as it is 'PRIVATE' and therefore not accessible outside of its owning target.))

Tests/RunCMake/CXXModules/examples/req-private-other-target-stderr.txt

@@ -0,0 +1,9 @@
+CMake Warning \(dev\) at CMakeLists.txt:7 \(target_sources\):
+  CMake's C\+\+ module support is experimental.  It is meant only for
+  experimentation and feedback to CMake developers.
+This warning is for project developers.  Use -Wno-dev to suppress it.
+
+CMake Warning \(dev\):
+  C\+\+20 modules support via CMAKE_EXPERIMENTAL_CXX_MODULE_DYNDEP is
+  experimental.  It is meant only for compiler developers to try.
+This warning is for project developers.  Use -Wno-dev to suppress it.

Tests/RunCMake/CXXModules/examples/req-private-other-target/CMakeLists.txt

@@ -0,0 +1,16 @@
+cmake_minimum_required(VERSION 3.26)
+project(req_private_other_target CXX)
+
+include("${CMAKE_SOURCE_DIR}/../cxx-modules-rules.cmake")
+
+add_library(lib)
+target_sources(lib
+  PUBLIC FILE_SET pub TYPE CXX_MODULES FILES lib.cxx
+  PRIVATE FILE_SET priv TYPE CXX_MODULES FILES priv.cxx
+)
+target_compile_features(lib PUBLIC cxx_std_20)
+
+add_executable(exe main.cxx)
+target_link_libraries(exe PRIVATE lib)
+
+add_test(NAME exe COMMAND exe)

Tests/RunCMake/CXXModules/examples/req-private-other-target/lib.cxx

@@ -0,0 +1 @@
+export module lib;

Tests/RunCMake/CXXModules/examples/req-private-other-target/main.cxx

@@ -0,0 +1,7 @@
+import lib;
+import lib.priv;
+
+int main(int argc, char const* argv[])
+{
+  return 0;
+}

Tests/RunCMake/CXXModules/examples/req-private-other-target/priv.cxx

@@ -0,0 +1 @@
+export module lib.priv;

Utilities/IWYU/mapping.imp

@@ -99,6 +99,7 @@
   { symbol: [ "std::enable_if<true, std::chrono::duration<long, std::ratio<60, 1> > >::type", private, "\"cmConfigure.h\"", public ] },
   { symbol: [ "std::enable_if<true, std::chrono::duration<long, std::ratio<1, 1000> > >::type", private, "\"cmConfigure.h\"", public ] },
   { symbol: [ "__gnu_cxx::__enable_if<true, bool>::__type", private, "\"cmConfigure.h\"", public ] },
+  { symbol: [ "std::remove_reference<std::basic_string<char, std::char_traits<char>, std::allocator<char> > &>::type", private, "\"cmConfigure.h\"", public ] },
   { symbol: [ "std::remove_reference<Defer &>::type", private, "\"cmConfigure.h\"", public ] },
 
   # Wrappers for 3rd-party libraries