[clang] 6bc7502 - When making modules transitively visible, don't take into account

[Richard Smith via cfe-commits]

Author: Richard Smith
Date: 2020-04-17T22:49:58-07:00
New Revision: 6bc7502385cc2a06954082a7d0e6418e610d35f4

URL: https://github.com/llvm/llvm-project/commit/6bc7502385cc2a06954082a7d0e6418e610d35f4
DIFF: https://github.com/llvm/llvm-project/commit/6bc7502385cc2a06954082a7d0e6418e610d35f4.diff

LOG: When making modules transitively visible, don't take into account
whether they have missing header files.

Whether a module's headers happen to be present on the local file system
should make no difference to whether we make its contents visible when
importing another module that re-exports it. If we have an up-to-date
AST file that we can load, that's all that matters.

This fixes the ability to header syntax checking for modular headers in
C++20 mode (or in prior modes where -fmodules-local-submodule-visibility
is enabled but -fmodules is not).

Added: 
    clang/test/Modules/Inputs/missing-header-local-visibility/a.h
    clang/test/Modules/Inputs/missing-header-local-visibility/all.h
    clang/test/Modules/Inputs/missing-header-local-visibility/b.h
    clang/test/Modules/Inputs/missing-header-local-visibility/module.modulemap
    clang/test/Modules/Inputs/missing-header-local-visibility/x.h
    clang/test/Modules/missing-header-local-visibility.cpp

Modified: 
    clang/lib/Basic/Module.cpp
    clang/lib/Lex/ModuleMap.cpp
    clang/lib/Serialization/ASTReader.cpp
    clang/lib/Serialization/ASTWriter.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Basic/Module.cpp b/clang/lib/Basic/Module.cpp
index 22e1da4e1859..b3daaa3a4442 100644
--- a/clang/lib/Basic/Module.cpp
+++ b/clang/lib/Basic/Module.cpp
@@ -654,8 +654,8 @@ void VisibleModuleSet::setVisible(Module *M, SourceLocation Loc,
     SmallVector<Module *, 16> Exports;
     V.M->getExportedModules(Exports);
     for (Module *E : Exports) {
-      // Don't recurse to unavailable submodules.
-      if (E->isAvailable())
+      // Don't import non-importable modules.
+      if (!E->isUnimportable())
         VisitModule({E, &V});
     }
 

diff  --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index 2c79bb28d8d8..4f7d5ab137e6 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -544,6 +544,9 @@ void ModuleMap::diagnoseHeaderInclusion(Module *RequestingModule,
 static bool isBetterKnownHeader(const ModuleMap::KnownHeader &New,
                                 const ModuleMap::KnownHeader &Old) {
   // Prefer available modules.
+  // FIXME: Considering whether the module is available rather than merely
+  // importable is non-hermetic and can result in surprising behavior for
+  // prebuilt modules. Consider only checking for importability here.
   if (New.getModule()->isAvailable() && !Old.getModule()->isAvailable())
     return true;
 

diff  --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index e2f89f51693e..873115835e5c 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -4033,8 +4033,8 @@ void ASTReader::makeModuleVisible(Module *Mod,
       continue;
     }
 
-    if (!Mod->isAvailable()) {
-      // Modules that aren't available cannot be made visible.
+    if (Mod->isUnimportable()) {
+      // Modules that aren't importable cannot be made visible.
       continue;
     }
 

diff  --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 18a92aaadd52..31d004f6c946 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -1729,7 +1729,8 @@ void ASTWriter::WriteHeaderSearch(const HeaderSearch &HS) {
     llvm::SmallVector<Module *, 16> Worklist(1, WritingModule);
     while (!Worklist.empty()) {
       Module *M = Worklist.pop_back_val();
-      if (!M->isAvailable())
+      // We don't care about headers in unimportable submodules.
+      if (M->isUnimportable())
         continue;
 
       // Map to disk files where possible, to pick up any missing stat

diff  --git a/clang/test/Modules/Inputs/missing-header-local-visibility/a.h b/clang/test/Modules/Inputs/missing-header-local-visibility/a.h
new file mode 100644
index 000000000000..3ff0ea836a29
--- /dev/null
+++ b/clang/test/Modules/Inputs/missing-header-local-visibility/a.h
@@ -0,0 +1,2 @@
+#include "x.h"
+X a();

diff  --git a/clang/test/Modules/Inputs/missing-header-local-visibility/all.h b/clang/test/Modules/Inputs/missing-header-local-visibility/all.h
new file mode 100644
index 000000000000..600af314ec35
--- /dev/null
+++ b/clang/test/Modules/Inputs/missing-header-local-visibility/all.h
@@ -0,0 +1,2 @@
+#include "a.h"
+#include "b.h"

diff  --git a/clang/test/Modules/Inputs/missing-header-local-visibility/b.h b/clang/test/Modules/Inputs/missing-header-local-visibility/b.h
new file mode 100644
index 000000000000..0fdb1154eb39
--- /dev/null
+++ b/clang/test/Modules/Inputs/missing-header-local-visibility/b.h
@@ -0,0 +1,2 @@
+#include "a.h"
+X b();

diff  --git a/clang/test/Modules/Inputs/missing-header-local-visibility/module.modulemap b/clang/test/Modules/Inputs/missing-header-local-visibility/module.modulemap
new file mode 100644
index 000000000000..0f21ceeeb29e
--- /dev/null
+++ b/clang/test/Modules/Inputs/missing-header-local-visibility/module.modulemap
@@ -0,0 +1,6 @@
+module M {
+  module A { header "a.h" export * }
+  module B { header "b.h" export * }
+  module X { header "x.h" header "missing.h" export * }
+  module All { header "all.h" export * }
+}

diff  --git a/clang/test/Modules/Inputs/missing-header-local-visibility/x.h b/clang/test/Modules/Inputs/missing-header-local-visibility/x.h
new file mode 100644
index 000000000000..0498c5d2aabc
--- /dev/null
+++ b/clang/test/Modules/Inputs/missing-header-local-visibility/x.h
@@ -0,0 +1,4 @@
+#ifndef X_H
+#define X_H
+struct X {};
+#endif

diff  --git a/clang/test/Modules/missing-header-local-visibility.cpp b/clang/test/Modules/missing-header-local-visibility.cpp
new file mode 100644
index 000000000000..e282a1babed7
--- /dev/null
+++ b/clang/test/Modules/missing-header-local-visibility.cpp
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 -fmodules-local-submodule-visibility -fimplicit-module-maps -I%S/Inputs/missing-header-local-visibility %s
+// RUN: %clang_cc1 -fmodules-local-submodule-visibility -fimplicit-module-maps -I%S/Inputs/missing-header-local-visibility -x c++-header %S/Inputs/missing-header-local-visibility/all.h
+// RUN: %clang_cc1 -fmodule-name=M -std=c++2a -fimplicit-module-maps -I%S/Inputs/missing-header-local-visibility -x c++-header %s
+// RUN: %clang_cc1 -fmodule-name=M -std=c++2a -fimplicit-module-maps -I%S/Inputs/missing-header-local-visibility -x c++-header %S/Inputs/missing-header-local-visibility/all.h
+
+#include "a.h"
+#include "b.h"