Separate parsing out Swift and Clang modules from the explicit module map

Since #63178 added support for Clang modules in the explicit module map, it is possible for there to be multiple modules with the same name: a Swift module and a Clang module. The current parsing logic just overwrites the corresponding entry module in a hashmap so we always only preserved the module that comes last, with the same name.

This change separates the parsing of the modulemap JSON file to produce a separate Swift module map and Clang module map. The Swift one is used by the 'ExplicitSwiftModuleLoader', as before, and the Clang one is only used to populate the ClangArgs with the requried -fmodule-... flags.

Author: artemcm

include/swift/Frontend/ModuleInterfaceLoader.h

@@ -176,24 +176,49 @@ class ExplicitSwiftModuleLoader: public SerializedModuleLoaderBase {
   ~ExplicitSwiftModuleLoader();
 };
 
-/// Information about explicitly specified Swift and Clang module files.
-struct ExplicitModuleInfo {
-  // Path of the .swiftmodule file. Empty for pure Clang modules.
+
+// Explicitly-specified Swift module inputs
+struct ExplicitSwiftModuleInputInfo {
+  ExplicitSwiftModuleInputInfo(std::string modulePath,
+                               llvm::Optional<std::string> moduleDocPath,
+                               llvm::Optional<std::string> moduleSourceInfoPath,
+                               bool isFramework = false,
+                               bool isSystem = false)
+    : modulePath(modulePath),
+      moduleDocPath(moduleDocPath),
+      moduleSourceInfoPath(moduleSourceInfoPath),
+      isFramework(isFramework),
+      isSystem(isSystem) {}
+  // Path of the .swiftmodule file.
+  std::string modulePath;
+  // Path of the .swiftmoduledoc file.
+  llvm::Optional<std::string> moduleDocPath;
+  // Path of the .swiftsourceinfo file.
+  llvm::Optional<std::string> moduleSourceInfoPath;
+  // A flag that indicates whether this module is a framework
+  bool isFramework = false;
+  // A flag that indicates whether this module is a system module
+  bool isSystem = false;
+};
+
+// Explicitly-specified Clang module inputs
+struct ExplicitClangModuleInputInfo {
+  ExplicitClangModuleInputInfo(std::string moduleMapPath,
+                               std::string modulePath,
+                               bool isFramework = false,
+                               bool isSystem = false)
+    : moduleMapPath(moduleMapPath),
+      modulePath(modulePath),
+      isFramework(isFramework),
+      isSystem(isSystem) {}
+  // Path of the Clang module map file.
+  std::string moduleMapPath;
+  // Path of a compiled Clang explicit module file (pcm).
   std::string modulePath;
-  // Path of the .swiftmoduledoc file. Empty for pure Clang modules.
-  std::string moduleDocPath;
-  // Path of the .swiftsourceinfo file. Empty for pure Clang modules.
-  std::string moduleSourceInfoPath;
   // A flag that indicates whether this module is a framework
   bool isFramework = false;
   // A flag that indicates whether this module is a system module
-  // Set the default to be false.
   bool isSystem = false;
-  // Path of the Clang module map file. Empty for pure Swift modules.
-  std::string clangModuleMapPath;
-  // Path of a compiled Clang explicit module file. Empty for pure Swift
-  // modules.
-  std::string clangModulePath;
 };
 
 /// Parser of explicit module maps passed into the compiler.
@@ -223,7 +248,8 @@ class ExplicitModuleMapParser {
 
   std::error_code
   parseSwiftExplicitModuleMap(llvm::StringRef fileName,
-                              llvm::StringMap<ExplicitModuleInfo> &moduleMap) {
+                              llvm::StringMap<ExplicitSwiftModuleInputInfo> &swiftModuleMap,
+                              llvm::StringMap<ExplicitClangModuleInputInfo> &clangModuleMap) {
     using namespace llvm::yaml;
     // Load the input file.
     llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> fileBufOrErr =
@@ -240,7 +266,7 @@ class ExplicitModuleMapParser {
       assert(DI != Stream.end() && "Failed to read a document");
       if (auto *MN = dyn_cast_or_null<SequenceNode>(DI->getRoot())) {
         for (auto &entry : *MN) {
-          if (parseSingleModuleEntry(entry, moduleMap)) {
+          if (parseSingleModuleEntry(entry, swiftModuleMap, clangModuleMap)) {
             return std::make_error_code(std::errc::invalid_argument);
           }
         }
@@ -269,40 +295,65 @@ class ExplicitModuleMapParser {
   }
 
   bool parseSingleModuleEntry(llvm::yaml::Node &node,
-                              llvm::StringMap<ExplicitModuleInfo> &moduleMap) {
+                              llvm::StringMap<ExplicitSwiftModuleInputInfo> &swiftModuleMap,
+                              llvm::StringMap<ExplicitClangModuleInputInfo> &clangModuleMap) {
     using namespace llvm::yaml;
     auto *mapNode = dyn_cast<MappingNode>(&node);
     if (!mapNode)
       return true;
     StringRef moduleName;
-    ExplicitModuleInfo result;
+    llvm::Optional<std::string> swiftModulePath, swiftModuleDocPath,
+                                swiftModuleSourceInfoPath;
+    std::string clangModuleMapPath = "", clangModulePath = "";
+    bool isFramework = false, isSystem = false;
     for (auto &entry : *mapNode) {
       auto key = getScalaNodeText(entry.getKey());
       auto val = getScalaNodeText(entry.getValue());
       if (key == "moduleName") {
         moduleName = val;
       } else if (key == "modulePath") {
-        result.modulePath = val.str();
+        swiftModulePath = val.str();
       } else if (key == "docPath") {
-        result.moduleDocPath = val.str();
+        swiftModuleDocPath = val.str();
       } else if (key == "sourceInfoPath") {
-        result.moduleSourceInfoPath = val.str();
+        swiftModuleSourceInfoPath = val.str();
       } else if (key == "isFramework") {
-        result.isFramework = parseBoolValue(val);
+        isFramework = parseBoolValue(val);
       } else if (key == "isSystem") {
-        result.isSystem = parseBoolValue(val);
+        isSystem = parseBoolValue(val);
       } else if (key == "clangModuleMapPath") {
-        result.clangModuleMapPath = val.str();
+        clangModuleMapPath = val.str();
       } else if (key == "clangModulePath") {
-        result.clangModulePath = val.str();
+        clangModulePath = val.str();
       } else {
         // Being forgiving for future fields.
         continue;
       }
     }
     if (moduleName.empty())
       return true;
-    moduleMap[moduleName] = std::move(result);
+
+    if (swiftModulePath.has_value()) {
+      assert((clangModuleMapPath.empty() &&
+              clangModulePath.empty()) &&
+             "Unexpected Clang dependency details for Swift module");
+      ExplicitSwiftModuleInputInfo entry(swiftModulePath.value(),
+                                         swiftModuleDocPath,
+                                         swiftModuleSourceInfoPath,
+                                         isFramework,
+                                         isSystem);
+      swiftModuleMap.try_emplace(moduleName, std::move(entry));
+    } else {
+      assert((!clangModuleMapPath.empty() ||
+              !clangModulePath.empty()) &&
+             "Expected Clang dependency module");
+      ExplicitClangModuleInputInfo entry(clangModuleMapPath,
+                                         clangModulePath,
+                                         isFramework,
+                                         isSystem);
+      clangModuleMap.try_emplace(moduleName, std::move(entry));
+    }
+
     return false;
   }
 

include/swift/Serialization/ModuleDependencyScanner.h

@@ -84,8 +84,10 @@ namespace swift {
       /// Scan the given placeholder module map
       void parsePlaceholderModuleMap(StringRef fileName) {
         ExplicitModuleMapParser parser(Allocator);
+        llvm::StringMap<ExplicitClangModuleInputInfo> ClangDependencyModuleMap;
         auto result =
-          parser.parseSwiftExplicitModuleMap(fileName, PlaceholderDependencyModuleMap);
+          parser.parseSwiftExplicitModuleMap(fileName, PlaceholderDependencyModuleMap,
+                                             ClangDependencyModuleMap);
         if (result == std::errc::invalid_argument) {
           Ctx.Diags.diagnose(SourceLoc(),
                              diag::placeholder_dependency_module_map_corrupted,
@@ -98,7 +100,7 @@ namespace swift {
         }
       }
 
-      llvm::StringMap<ExplicitModuleInfo> PlaceholderDependencyModuleMap;
+      llvm::StringMap<ExplicitSwiftModuleInputInfo> PlaceholderDependencyModuleMap;
       llvm::BumpPtrAllocator Allocator;
 
     public:

lib/Frontend/ModuleInterfaceLoader.cpp

@@ -1859,13 +1859,15 @@ InterfaceSubContextDelegateImpl::runInSubCompilerInstance(StringRef moduleName,
 struct ExplicitSwiftModuleLoader::Implementation {
   ASTContext &Ctx;
   llvm::BumpPtrAllocator Allocator;
-  llvm::StringMap<ExplicitModuleInfo> ExplicitModuleMap;
+  llvm::StringMap<ExplicitSwiftModuleInputInfo> ExplicitModuleMap;
   Implementation(ASTContext &Ctx) : Ctx(Ctx) {}
 
   void parseSwiftExplicitModuleMap(StringRef fileName) {
     ExplicitModuleMapParser parser(Allocator);
+    llvm::StringMap<ExplicitClangModuleInputInfo> ExplicitClangModuleMap;
     auto result =
-        parser.parseSwiftExplicitModuleMap(fileName, ExplicitModuleMap);
+        parser.parseSwiftExplicitModuleMap(fileName, ExplicitModuleMap,
+                                           ExplicitClangModuleMap);
     if (result == std::errc::invalid_argument)
       Ctx.Diags.diagnose(SourceLoc(), diag::explicit_swift_module_map_corrupted,
                          fileName);
@@ -1877,16 +1879,16 @@ struct ExplicitSwiftModuleLoader::Implementation {
     // we've seen so that we don't generate duplicate flags.
     std::set<std::string> moduleMapsSeen;
     std::vector<std::string> &extraClangArgs = Ctx.ClangImporterOpts.ExtraArgs;
-    for (auto &entry : ExplicitModuleMap) {
-      const auto &moduleMapPath = entry.getValue().clangModuleMapPath;
+    for (auto &entry : ExplicitClangModuleMap) {
+      const auto &moduleMapPath = entry.getValue().moduleMapPath;
       if (!moduleMapPath.empty() &&
           moduleMapsSeen.find(moduleMapPath) == moduleMapsSeen.end()) {
         moduleMapsSeen.insert(moduleMapPath);
         extraClangArgs.push_back(
             (Twine("-fmodule-map-file=") + moduleMapPath).str());
       }
 
-      const auto &modulePath = entry.getValue().clangModulePath;
+      const auto &modulePath = entry.getValue().modulePath;
       if (!modulePath.empty()) {
         extraClangArgs.push_back(
             (Twine("-fmodule-file=") + entry.getKey() + "=" + modulePath)
@@ -1899,8 +1901,7 @@ struct ExplicitSwiftModuleLoader::Implementation {
       const std::vector<std::pair<std::string, std::string>>
           &commandLineExplicitInputs) {
     for (const auto &moduleInput : commandLineExplicitInputs) {
-      ExplicitModuleInfo entry;
-      entry.modulePath = moduleInput.second;
+      ExplicitSwiftModuleInputInfo entry(moduleInput.second, {}, {});
       ExplicitModuleMap.try_emplace(moduleInput.first, std::move(entry));
     }
   }
@@ -1939,16 +1940,6 @@ bool ExplicitSwiftModuleLoader::findModule(
   }
   auto &moduleInfo = it->getValue();
 
-  // If this is only a Clang module with no paired Swift module, return false
-  // now so that we don't emit diagnostics about it being missing. This gives
-  // ClangImporter an opportunity to import it.
-  bool hasClangModule = !moduleInfo.clangModuleMapPath.empty() ||
-                        !moduleInfo.clangModulePath.empty();
-  bool hasSwiftModule = !moduleInfo.modulePath.empty();
-  if (hasClangModule && !hasSwiftModule) {
-    return false;
-  }
-
   // Set IsFramework bit according to the moduleInfo
   IsFramework = moduleInfo.isFramework;
   IsSystemModule = moduleInfo.isSystem;
@@ -1990,14 +1981,14 @@ bool ExplicitSwiftModuleLoader::findModule(
   *ModuleBuffer = std::move(moduleBuf.get());
 
   // Open .swiftdoc file
-  if (!moduleInfo.moduleDocPath.empty()) {
-    auto moduleDocBuf = fs.getBufferForFile(moduleInfo.moduleDocPath);
+  if (moduleInfo.moduleDocPath.has_value()) {
+    auto moduleDocBuf = fs.getBufferForFile(moduleInfo.moduleDocPath.value());
     if (moduleBuf)
       *ModuleDocBuffer = std::move(moduleDocBuf.get());
   }
   // Open .swiftsourceinfo file
-  if (!moduleInfo.moduleSourceInfoPath.empty()) {
-    auto moduleSourceInfoBuf = fs.getBufferForFile(moduleInfo.moduleSourceInfoPath);
+  if (moduleInfo.moduleSourceInfoPath.has_value()) {
+    auto moduleSourceInfoBuf = fs.getBufferForFile(moduleInfo.moduleSourceInfoPath.value());
     if (moduleSourceInfoBuf)
       *ModuleSourceInfoBuffer = std::move(moduleSourceInfoBuf.get());
   }

lib/Serialization/ModuleDependencyScanner.cpp

@@ -83,8 +83,11 @@ bool PlaceholderSwiftModuleScanner::findModule(
   }
   auto &moduleInfo = it->getValue();
   auto dependencies = ModuleDependencyInfo::forPlaceholderSwiftModuleStub(
-      moduleInfo.modulePath, moduleInfo.moduleDocPath,
-      moduleInfo.moduleSourceInfoPath);
+      moduleInfo.modulePath,
+      moduleInfo.moduleDocPath.has_value() ?
+            moduleInfo.moduleDocPath.value() : "",
+      moduleInfo.moduleSourceInfoPath.has_value() ?
+            moduleInfo.moduleSourceInfoPath.value() : "");
   this->dependencies = std::move(dependencies);
   return true;
 }

test/ScanDependencies/explicit-module-map-clang-and-swift.swift

@@ -3,13 +3,18 @@
 // RUN: mkdir -p %t/inputs
 // RUN: echo "public func anotherFuncA() {}" > %t/A.swift
 // RUN: %target-swift-frontend -emit-module -emit-module-path %t/inputs/A.swiftmodule -emit-module-doc-path %t/inputs/A.swiftdoc -emit-module-source-info -emit-module-source-info-path %t/inputs/A.swiftsourceinfo -import-underlying-module -I%S/Inputs/CHeaders -module-cache-path %t.module-cache %t/A.swift -module-name A
+// RUN: %target-swift-emit-pcm -module-name A -o %t/inputs/A.pcm %S/Inputs/CHeaders/module.modulemap
 
 // RUN: echo "[{" > %/t/inputs/map.json
 // RUN: echo "\"moduleName\": \"A\"," >> %/t/inputs/map.json
 // RUN: echo "\"modulePath\": \"%/t/inputs/A.swiftmodule\"," >> %/t/inputs/map.json
 // RUN: echo "\"docPath\": \"%/t/inputs/A.swiftdoc\"," >> %/t/inputs/map.json
 // RUN: echo "\"sourceInfoPath\": \"%/t/inputs/A.swiftsourceinfo\"," >> %/t/inputs/map.json
 // RUN: echo "\"isFramework\": false," >> %/t/inputs/map.json
+// RUN: echo "}," >> %/t/inputs/map.json
+// RUN: echo "{" >> %/t/inputs/map.json
+// RUN: echo "\"moduleName\": \"A\"," >> %/t/inputs/map.json
+// RUN: echo "\"clangModulePath\": \"%/t/inputs/A.pcm\"," >> %/t/inputs/map.json
 // RUN: echo "\"clangModuleMapPath\": \"%/S/Inputs/CHeaders/module.modulemap\"" >> %/t/inputs/map.json
 // RUN: echo "}," >> %/t/inputs/map.json
 // RUN: echo "{" >> %/t/inputs/map.json
@@ -33,7 +38,9 @@
 // RUN: echo "\"isFramework\": false" >> %/t/inputs/map.json
 // RUN: echo "}]" >> %/t/inputs/map.json
 
-// RUN: %target-swift-frontend -emit-module -emit-module-path %t/Foo.swiftmodule -module-cache-path %t.module-cache -explicit-swift-module-map-file %t/inputs/map.json %s
+// RUN: %target-swift-frontend -emit-module -emit-module-path %t/Foo.swiftmodule -disable-implicit-swift-modules -module-cache-path %t.module-cache -explicit-swift-module-map-file %t/inputs/map.json -Rmodule-loading -Xcc -Rmodule-import %s 2>&1 | %FileCheck %s
+
+// CHECK: <unknown>:0: remark: importing module 'A' from {{.*}}{{/|\\}}explicit-module-map-clang-and-swift.swift.tmp{{/|\\}}inputs{{/|\\}}A.pcm'
 
 import A