[modules] PR20507: Avoid silent textual inclusion.

Summary:
If a module was unavailable (either a missing requirement on the module
being imported, or a missing file anywhere in the top-level module (and
not dominated by an unsatisfied `requires`)), we would silently treat
inclusions as textual. This would cause all manner of crazy and
confusing errors (and would also silently "work" sometimes, making the
problem difficult to track down).

I'm really not a fan of the `M->isAvailable(getLangOpts(), getTargetInfo(),
Requirement, MissingHeader)` function; it seems to do too many things at
once, but for now I've done things in a sort of awkward way.

The changes to test/Modules/Inputs/declare-use/module.map
were necessitated because the thing that was meant to be tested there
(introduced in r197805) was predicated on silently falling back to textual
inclusion, which we no longer do.

The changes to test/Modules/Inputs/macro-reexport/module.modulemap
are just an overlooked missing header that seems to have been missing since
this code was committed (r213922), which is now caught.

Reviewers: rsmith, benlangmuir, djasper

Subscribers: cfe-commits

Differential Revision: http://reviews.llvm.org/D10423

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@245228 91177308-0d34-0410-b5e6-96231b3b80d8

16 files changed, 127 insertions, 10 deletions

diff --git a/include/clang/Basic/DiagnosticCommonKinds.td b/include/clang/Basic/DiagnosticCommonKinds.td
index 605736f076..e5c924d886 100644
--- a/include/clang/Basic/DiagnosticCommonKinds.td
+++ b/include/clang/Basic/DiagnosticCommonKinds.td
@@ -84,6 +84,10 @@ def err_module_not_built : Error<"could not build module '%0'">, DefaultFatal;
 def err_module_build_disabled: Error<
   "module '%0' is needed but has not been provided, and implicit use of module "
   "files is disabled">, DefaultFatal;
+def err_module_unavailable : Error<
+  "module '%0' %select{is incompatible with|requires}1 feature '%2'">;
+def err_module_header_missing : Error<
+  "%select{|umbrella }0header '%1' not found">;
 def err_module_lock_failure : Error<
   "could not acquire lock file for module '%0'">, DefaultFatal;
 def err_module_lock_timeout : Error<
diff --git a/include/clang/Basic/DiagnosticFrontendKinds.td b/include/clang/Basic/DiagnosticFrontendKinds.td
index 66c17f104c..0d61d8bc02 100644
--- a/include/clang/Basic/DiagnosticFrontendKinds.td
+++ b/include/clang/Basic/DiagnosticFrontendKinds.td
@@ -183,10 +183,6 @@ def err_no_submodule_suggest : Error<
   "no submodule named %0 in module '%1'; did you mean '%2'?">;
 def warn_missing_submodule : Warning<"missing submodule '%0'">,
   InGroup<IncompleteUmbrella>;
-def err_module_unavailable : Error<
-  "module '%0' %select{is incompatible with|requires}1 feature '%2'">;
-def err_module_header_missing : Error<
-  "%select{|umbrella }0header '%1' not found">;
 def err_module_cannot_create_includes : Error<
   "cannot create includes file for module %0: %1">;
 def warn_module_config_macro_undef : Warning<
diff --git a/include/clang/Basic/DiagnosticLexKinds.td b/include/clang/Basic/DiagnosticLexKinds.td
index 4acaf9cc86..712ea63ae8 100644
--- a/include/clang/Basic/DiagnosticLexKinds.td
+++ b/include/clang/Basic/DiagnosticLexKinds.td
@@ -623,6 +623,8 @@ def warn_mmap_unknown_attribute : Warning<"unknown attribute '%0'">,
 def warn_auto_module_import : Warning<
   "treating #%select{include|import|include_next|__include_macros}0 as an "
   "import of module '%1'">, InGroup<AutoImport>, DefaultIgnore;
+def note_implicit_top_level_module_import_here : Note<
+  "submodule of top-level module '%0' implicitly imported here">;
 def warn_uncovered_module_header : Warning<
   "umbrella header for module '%0' does not include header '%1'">, 
   InGroup<IncompleteUmbrella>;
diff --git a/lib/Lex/ModuleMap.cpp b/lib/Lex/ModuleMap.cpp
index 3825ceb0a6..9554d3ba45 100644
--- a/lib/Lex/ModuleMap.cpp
+++ b/lib/Lex/ModuleMap.cpp
@@ -320,6 +320,10 @@ void ModuleMap::diagnoseHeaderInclusion(Module *RequestingModule,
 
 static bool isBetterKnownHeader(const ModuleMap::KnownHeader &New,
                                 const ModuleMap::KnownHeader &Old) {
+  // Prefer available modules.
+  if (New.getModule()->isAvailable() && !Old.getModule()->isAvailable())
+    return true;
+
   // Prefer a public header over a private header.
   if ((New.getRole() & ModuleMap::PrivateHeader) !=
       (Old.getRole() & ModuleMap::PrivateHeader))
@@ -349,9 +353,6 @@ ModuleMap::KnownHeader ModuleMap::findModuleForHeader(const FileEntry *File) {
       // Prefer a header from the current module over all others.
       if (H.getModule()->getTopLevelModule() == CompilingModule)
         return MakeResult(H);
-      // Cannot use a module if it is unavailable.
-      if (!H.getModule()->isAvailable())
-        continue;
       if (!Result || isBetterKnownHeader(H, Result))
         Result = H;
     }
diff --git a/lib/Lex/PPDirectives.cpp b/lib/Lex/PPDirectives.cpp
index ce64538de4..82f94d7da4 100644
--- a/lib/Lex/PPDirectives.cpp
+++ b/lib/Lex/PPDirectives.cpp
@@ -1674,6 +1674,29 @@ void Preprocessor::HandleIncludeDirective(SourceLocation HashLoc,
           getLangOpts().CurrentModule &&
       SuggestedModule.getModule()->getTopLevelModuleName() !=
           getLangOpts().ImplementationOfModule) {
+
+    // If this include corresponds to a module but that module is
+    // unavailable, diagnose the situation and bail out.
+    if (!SuggestedModule.getModule()->isAvailable()) {
+      clang::Module::Requirement Requirement;
+      clang::Module::UnresolvedHeaderDirective MissingHeader;
+      Module *M = SuggestedModule.getModule();
+      // Identify the cause.
+      (void)M->isAvailable(getLangOpts(), getTargetInfo(), Requirement,
+                           MissingHeader);
+      if (MissingHeader.FileNameLoc.isValid()) {
+        Diag(MissingHeader.FileNameLoc, diag::err_module_header_missing)
+            << MissingHeader.IsUmbrella << MissingHeader.FileName;
+      } else {
+        Diag(M->DefinitionLoc, diag::err_module_unavailable)
+            << M->getFullModuleName() << Requirement.second << Requirement.first;
+      }
+      Diag(FilenameTok.getLocation(),
+           diag::note_implicit_top_level_module_import_here)
+          << M->getTopLevelModuleName();
+      return;
+    }
+
     // Compute the module access path corresponding to this module.
     // FIXME: Should we have a second loadModule() overload to avoid this
     // extra lookup step?
diff --git a/test/Modules/Inputs/auto-import-unavailable/missing_header/not_missing.h b/test/Modules/Inputs/auto-import-unavailable/missing_header/not_missing.h
new file mode 100644
index 0000000000..5bab8334a4
--- /dev/null
+++ b/test/Modules/Inputs/auto-import-unavailable/missing_header/not_missing.h
@@ -0,0 +1 @@
+// missing_header/not_missing.h
diff --git a/test/Modules/Inputs/auto-import-unavailable/missing_requirement.h b/test/Modules/Inputs/auto-import-unavailable/missing_requirement.h
new file mode 100644
index 0000000000..7090978c9b
--- /dev/null
+++ b/test/Modules/Inputs/auto-import-unavailable/missing_requirement.h
@@ -0,0 +1 @@
+// missing_requirement.h
diff --git a/test/Modules/Inputs/auto-import-unavailable/module.modulemap b/test/Modules/Inputs/auto-import-unavailable/module.modulemap
new file mode 100644
index 0000000000..26196dcb37
--- /dev/null
+++ b/test/Modules/Inputs/auto-import-unavailable/module.modulemap
@@ -0,0 +1,19 @@
+module missing_header {
+  module missing { header "missing_header/missing.h" }
+  module error_importing_this { header "missing_header/not_missing.h" }
+}
+
+module nonrequired_missing_header {
+  module unsatisfied_requires {
+    requires nonexistent_feature
+    header "nonrequired_missing_header/missing.h"
+  }
+  module fine_to_import {
+    header "nonrequired_missing_header/not_missing.h"
+  }
+}
+
+module missing_requirement {
+  requires nonexistent_feature
+  header "missing_requirement.h"
+}
diff --git a/test/Modules/Inputs/auto-import-unavailable/nonrequired_missing_header/not_missing.h b/test/Modules/Inputs/auto-import-unavailable/nonrequired_missing_header/not_missing.h
new file mode 100644
index 0000000000..3ccfcb1b47
--- /dev/null
+++ b/test/Modules/Inputs/auto-import-unavailable/nonrequired_missing_header/not_missing.h
@@ -0,0 +1 @@
+// nonrequired_missing_header/not_missing.h
diff --git a/test/Modules/Inputs/auto-import-unavailable/nonrequired_missing_header/requires_feature_you_dont_have.h b/test/Modules/Inputs/auto-import-unavailable/nonrequired_missing_header/requires_feature_you_dont_have.h
new file mode 100644
index 0000000000..1bcb70d2af
--- /dev/null
+++ b/test/Modules/Inputs/auto-import-unavailable/nonrequired_missing_header/requires_feature_you_dont_have.h
@@ -0,0 +1 @@
+// nonrequired_missing_header/requires_feature_you_dont_have.h
diff --git a/test/Modules/Inputs/available-is-better/available-is-better.h b/test/Modules/Inputs/available-is-better/available-is-better.h
new file mode 100644
index 0000000000..8ed032081a
--- /dev/null
+++ b/test/Modules/Inputs/available-is-better/available-is-better.h
@@ -0,0 +1,2 @@
+#pragma once
+int available;
diff --git a/test/Modules/Inputs/available-is-better/module.modulemap b/test/Modules/Inputs/available-is-better/module.modulemap
new file mode 100644
index 0000000000..19ffabdffb
--- /dev/null
+++ b/test/Modules/Inputs/available-is-better/module.modulemap
@@ -0,0 +1,17 @@
+// There is some order-dependence to how clang chooses modules, so make
+// sure that both the first and last modules here are ones that would
+// cause a test failure if they were picked.
+
+module unavailable_before {
+  requires nonexistent_feature
+  header "available-is-better.h"
+}
+
+module available {
+  header "available-is-better.h"
+}
+
+module unavailable_after {
+  requires nonexistent_feature
+  header "available-is-better.h"
+}
diff --git a/test/Modules/Inputs/declare-use/module.map b/test/Modules/Inputs/declare-use/module.map
index 2dad0d061c..14551fde7e 100644
--- a/test/Modules/Inputs/declare-use/module.map
+++ b/test/Modules/Inputs/declare-use/module.map
@@ -20,14 +20,12 @@ module XD {
 
 module XE {
   header "e.h"
-  header "unavailable.h"
   use XA
   use XB
 }
 
 module XF {
   header "f.h"
-  header "unavailable.h"
   use XA
   use XB
 }
diff --git a/test/Modules/Inputs/macro-reexport/module.modulemap b/test/Modules/Inputs/macro-reexport/module.modulemap
index 896bda041c..f861ff8955 100644
--- a/test/Modules/Inputs/macro-reexport/module.modulemap
+++ b/test/Modules/Inputs/macro-reexport/module.modulemap
@@ -19,5 +19,4 @@ module e {
 }
 module f {
   module f1 { header "f1.h" export * }
-  module f2 { header "f2.h" export * }
 }
diff --git a/test/Modules/auto-import-unavailable.cpp b/test/Modules/auto-import-unavailable.cpp
new file mode 100644
index 0000000000..a35695d42a
--- /dev/null
+++ b/test/Modules/auto-import-unavailable.cpp
@@ -0,0 +1,47 @@
+// RUN: rm -rf %t
+// RUN: not %clang_cc1 -x c++ -Rmodule-build -DMISSING_HEADER -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/auto-import-unavailable %s 2>&1 | FileCheck %s --check-prefix=MISSING-HEADER
+// RUN: %clang_cc1 -x c++ -Rmodule-build -DNONREQUIRED_MISSING_HEADER -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/auto-import-unavailable %s 2>&1 | FileCheck %s --check-prefix=NONREQUIRED-MISSING-HEADER
+// RUN: not %clang_cc1 -x c++ -Rmodule-build -DMISSING_REQUIREMENT -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/auto-import-unavailable %s 2>&1 | FileCheck %s --check-prefix=MISSING-REQUIREMENT
+
+#ifdef MISSING_HEADER
+
+// Even if the header we ask for is not missing, if the top-level module
+// containing it has a missing header, then the whole top-level is
+// unavailable and we issue an error.
+
+// MISSING-HEADER: module.modulemap:2:27: error: header 'missing_header/missing.h' not found
+// MISSING-HEADER-DAG: auto-import-unavailable.cpp:[[@LINE+1]]:10: note: submodule of top-level module 'missing_header' implicitly imported here
+#include "missing_header/not_missing.h"
+
+// We should not attempt to build the module.
+// MISSING-HEADER-NOT: remark: building module
+
+#endif // #ifdef MISSING_HEADER
+
+
+#ifdef NONREQUIRED_MISSING_HEADER
+
+// However, if the missing header is dominated by an unsatisfied
+// `requires`, then that is acceptable.
+// This also tests that an unsatisfied `requires` elsewhere in the
+// top-level module doesn't affect an available module.
+
+// NONREQUIRED-MISSING-HEADER: auto-import-unavailable.cpp:[[@LINE+2]]:10: remark: building module 'nonrequired_missing_header'
+// NONREQUIRED-MISSING-HEADER: auto-import-unavailable.cpp:[[@LINE+1]]:10: remark: finished building module 'nonrequired_missing_header'
+#include "nonrequired_missing_header/not_missing.h"
+
+#endif // #ifdef NONREQUIRED_MISSING_HEADER
+
+
+#ifdef MISSING_REQUIREMENT
+
+// If the header is unavailable due to a missing requirement, an error
+// should be emitted if a user tries to include it.
+
+// MISSING-REQUIREMENT:module.modulemap:16:8: error: module 'missing_requirement' requires feature 'nonexistent_feature'
+// MISSING-REQUIREMENT: auto-import-unavailable.cpp:[[@LINE+1]]:10: note: submodule of top-level module 'missing_requirement' implicitly imported here
+#include "missing_requirement.h"
+
+// MISSING-REQUIREMENT-NOT: remark: building module
+
+#endif // #ifdef MISSING_REQUIREMENT
diff --git a/test/Modules/available-is-better.cpp b/test/Modules/available-is-better.cpp
new file mode 100644
index 0000000000..39d37c405e
--- /dev/null
+++ b/test/Modules/available-is-better.cpp
@@ -0,0 +1,5 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -x c++ -Rmodule-build -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/available-is-better %s 2>&1 | FileCheck %s
+
+#include "available-is-better.h"
+// CHECK: remark: building module