[clang][modules] Umbrella with missing submodule: unify implicit & explicit

[jansvoboda11]

Summary

Submodules not covered by existing umbrella header are being imported as textual with implicit modules. Explicit modules error out, since they still expect to be provided a module file that covers such header.

Unify the behavior of explicit modules with implicit modules by allowing such cases and treating them as textual as well.

Depends on D113761.

[jansvoboda11]

Re-run CI

[vsapsai]

clang/test/Modules/missing-submodule.m

3

Haven't checked the implementation but does the test cover any new behavior? Based on description it should test explicit modules. But the added line is testing -fimplicit-module-maps, just using the system-wide shared modules cache.

[jansvoboda11]

clang/test/Modules/missing-submodule.m

3

The system-wide shared modules cache flag is being generated by the driver. Here I'm invoking -cc1 without -fmodules-cache-path=, which I think effectively disables implicit modules. I'll add -fno-implicit-modules to make this more obvious. So yes, this test does cover the new behavior.

However, I spent some time investigating why we're using -fimplicit-module-maps in the command lines for explicit build produced by the dependency scanner and came to the conclusion this is due to inferred modules. I tried to fix the underlying issue in D113880. If we don't pass -fimplicit-module-maps to Clang, we don't hit the case this patch solves. I'm not sure whether I should abandon this patch in favor of D113880, or commit it anyway, since it still might be useful for some.

[jansvoboda11]

Add -fno-implicit-modules to test case to make it more obvious.

[jansvoboda11]

Abandoning, since we have a better solution: disable implicit module maps in explicit modules builds.

clang/include/clang/Frontend/CompilerInstance.h

private:
   /// For prebuilt modules, the Module is not expected to exist in
   /// HeaderSearch's ModuleMap.  If a ModuleFile by that name is in the
   /// ModuleManager, then it will be loaded and looked up.
   ///
   /// For implicit modules, the Module is expected to already be in the
   /// ModuleMap.  First attempt to load it from the given path on disk.  If that
   /// fails, defer to compileModuleAndReadAST, which will first build and then
   /// load it.
-  ModuleLoadResult findOrCompileModuleAndReadAST(StringRef ModuleName,
-                                                 SourceLocation ImportLoc,
-                                                 SourceLocation ModuleNameLoc,
-                                                 bool IsInclusionDirective);
+  ModuleLoadResult
+  findOrCompileModuleAndReadAST(StringRef ModuleName, SourceLocation ImportLoc,
+                                SourceLocation ModuleNameLoc, ModuleIdPath Path,
+                                Module::NameVisibilityKind Visibility,
 
   // Get the submodule corresponding to the module path.
   Module *getSubmoduleCorrespondingToPath(
     bool &MapPrivateSubModToTopLevel, Module *Module, SourceLocation ImportLoc,
     ModuleIdPath Path, Module::NameVisibilityKind Visibility,
     bool IsInclusionDirective);
 
 public:

clang/lib/Frontend/CompilerInstance.cpp

static ModuleSource selectModuleSource(
   if (M) {
     ModuleFilename = HS.getCachedModuleFileName(M);
     return MS_ModuleCache;
   }
 
   return MS_ModuleNotFound;
 }
 
+static ModuleLoadResult handleMissingSubmodule(DiagnosticsEngine &Diags,
+                                               Module *SubM,
+                                               SourceLocation ImportLoc,
+                                               ModuleIdPath Path) {
+  // We have an umbrella header or directory that doesn't actually include all
+  // the headers within the directory it covers. Complain about this missing
+  // submodule and recover by forgetting that we ever saw this submodule.
+  Diags.Report(ImportLoc, diag::warn_missing_submodule)
+      << SubM->getFullModuleName()
+      << SourceRange(Path.front().second, Path.back().second);
+
+  return ModuleLoadResult::MissingExpected;
+}
+
 ModuleLoadResult CompilerInstance::findOrCompileModuleAndReadAST(
     StringRef ModuleName, SourceLocation ImportLoc,
-    SourceLocation ModuleNameLoc, bool IsInclusionDirective) {
+    SourceLocation ModuleNameLoc, ModuleIdPath Path,
+    Module::NameVisibilityKind Visibility, bool IsInclusionDirective) {
   // Search for a module with the given name.
   HeaderSearch &HS = PP->getHeaderSearchInfo();
   Module *M =
       HS.lookupModule(ModuleName, ImportLoc, true, !IsInclusionDirective);
 
   // Select the source and filename for loading the named module.
   std::string ModuleFilename;
   ModuleSource Source =
       selectModuleSource(M, ModuleName, ModuleFilename, BuiltModules, HS);
   if (Source == MS_ModuleNotFound) {
     // We can't find a module, error out here.
     getDiagnostics().Report(ModuleNameLoc, diag::err_module_not_found)
         << ModuleName << SourceRange(ImportLoc, ModuleNameLoc);
     return nullptr;
   }
   if (ModuleFilename.empty()) {
     if (M && M->HasIncompatibleModuleFile) {
       // We tried and failed to load a module file for this module. Fall
       // back to textual inclusion for its headers.
       return ModuleLoadResult::ConfigMismatch;
     }
 
+    bool MapPrivateSubModToTopLevel = false;
+    // FIXME: Should we detect this at module load time? It seems fairly
+    // expensive (and rare).
+    Module *SubM = getSubmoduleCorrespondingToPath(
+        MapPrivateSubModToTopLevel, M, ImportLoc, Path, Visibility,
+        IsInclusionDirective);
+
+    if (SubM != M && !SubM->IsFromModuleFile && !MapPrivateSubModToTopLevel)
+      return handleMissingSubmodule(getDiagnostics(), SubM, ImportLoc, Path);
+
     getDiagnostics().Report(ModuleNameLoc, diag::err_module_build_disabled)
         << ModuleName;
     return nullptr;
   }
 
   // Create an ASTReader on demand.
   if (!getASTReader())
     createASTReader();
if (auto MaybeModule = MM.getCachedModuleLoad(*Path[0].first)) {
     //if (Module == nullptr) {
     //  getDiagnostics().Report(ModuleNameLoc, diag::err_module_not_found)
     //    << ModuleName;
     //  DisableGeneratingGlobalModuleIndex = true;
     //  return ModuleLoadResult();
     //}
     MM.cacheModuleLoad(*Path[0].first, Module);
   } else {
-    ModuleLoadResult Result = findOrCompileModuleAndReadAST(
-        ModuleName, ImportLoc, ModuleNameLoc, IsInclusionDirective);
+    ModuleLoadResult Result =
+        findOrCompileModuleAndReadAST(ModuleName, ImportLoc, ModuleNameLoc,
+                                      Path, Visibility, IsInclusionDirective);
     if (!Result.isNormal())
       return Result;
     if (!Result)
       DisableGeneratingGlobalModuleIndex = true;
     Module = Result;
     MM.cacheModuleLoad(*Path[0].first, Module);
   }
 
   // If we never found the module, fail.  Otherwise, verify the module and link
   // it up.
   if (!Module)
     return ModuleLoadResult();
 
   bool MapPrivateSubModToTopLevel = false;
+  class Module *TopLevelModule = Module;
+  // FIXME: Should we detect this at module load time? It seems fairly
+  // expensive (and rare).
   Module = getSubmoduleCorrespondingToPath(MapPrivateSubModToTopLevel, Module,
                                            ImportLoc, Path, Visibility,
                                            IsInclusionDirective);
 
   // Make the named module visible, if it's not already part of the module
   // we are parsing.
   if (ModuleName != getLangOpts().CurrentModule) {
-    if (!Module->IsFromModuleFile && !MapPrivateSubModToTopLevel) {
-      // We have an umbrella header or directory that doesn't actually include
-      // all of the headers within the directory it covers. Complain about
-      // this missing submodule and recover by forgetting that we ever saw
-      // this submodule.
-      // FIXME: Should we detect this at module load time? It seems fairly
-      // expensive (and rare).
-      getDiagnostics().Report(ImportLoc, diag::warn_missing_submodule)
-        << Module->getFullModuleName()
-        << SourceRange(Path.front().second, Path.back().second);
-
-      return ModuleLoadResult::MissingExpected;
-    }
+    if (Module != TopLevelModule && !Module->IsFromModuleFile &&
+        !MapPrivateSubModToTopLevel)
+      return handleMissingSubmodule(getDiagnostics(), Module, ImportLoc, Path);
 
     // Check whether this module is available.
     if (Preprocessor::checkModuleIsAvailable(getLangOpts(), getTarget(),
                                              getDiagnostics(), Module)) {
       getDiagnostics().Report(ImportLoc, diag::note_module_import_here)
         << SourceRange(Path.front().second, Path.back().second);
       LastModuleImportLoc = ImportLoc;
       LastModuleImportResult = ModuleLoadResult();

clang/test/Modules/missing-submodule.m

 // RUN: rm -rf %t
 // RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs %s -verify
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fno-implicit-modules   -F %S/Inputs %s -verify
 #include <Module/NotInModule.h> // expected-warning{{missing submodule 'Module.NotInModule'}}
 
 int getNotInModule() {
   return not_in_module;
 }