[modules] When diagnosing a missing module import, suggest adding a #include if

the current language doesn't have an import syntax and we can figure out a
suitable file to include.

llvm-svn: 267802

26 files changed, 237 insertions, 15 deletions

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 4e2a0349645..afa741077c3 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -8262,6 +8262,10 @@ def err_module_private_local_class : Error<
 def err_module_unimported_use : Error<
   "%select{declaration|definition|default argument}0 of %1 must be imported "
   "from module '%2' before it is required">;
+def err_module_unimported_use_header : Error<
+  "missing '#include %3'; "
+  "%select{declaration|definition|default argument}0 of %1 must be imported "
+  "from module '%2' before it is required">;
 def err_module_unimported_use_multiple : Error<
   "%select{declaration|definition|default argument}0 of %1 must be imported "
   "from one of the following modules before it is required:%2">;
diff --git a/clang/include/clang/Lex/HeaderSearch.h b/clang/include/clang/Lex/HeaderSearch.h
index 6d592e19c06..458f07e5e44 100644
--- a/clang/include/clang/Lex/HeaderSearch.h
+++ b/clang/include/clang/Lex/HeaderSearch.h
@@ -634,13 +634,18 @@ public:
   /// \brief Retrieve a uniqued framework name.
   StringRef getUniqueFrameworkName(StringRef Framework);
   
+  /// \brief Suggest a path by which the specified file could be found, for
+  /// use in diagnostics to suggest a #include.
+  ///
+  /// \param IsSystem If non-null, filled in to indicate whether the suggested
+  ///        path is relative to a system header directory.
+  std::string suggestPathToFileForDiagnostics(const FileEntry *File,
+                                              bool *IsSystem = nullptr);
+
   void PrintStats();
   
   size_t getTotalMemory() const;
 
-  static std::string NormalizeDashIncludePath(StringRef File,
-                                              FileManager &FileMgr);
-
 private:
   /// \brief Describes what happened when we tried to load a module map file.
   enum LoadModuleMapResult {
diff --git a/clang/include/clang/Lex/ModuleMap.h b/clang/include/clang/Lex/ModuleMap.h
index d8d374b622b..b8cfb8faa8d 100644
--- a/clang/include/clang/Lex/ModuleMap.h
+++ b/clang/include/clang/Lex/ModuleMap.h
@@ -130,6 +130,12 @@ public:
       return getModule()->isAvailable();
     }
 
+    /// \brief Whether this header is accessible from the specified module.
+    bool isAccessibleFrom(Module *M) const {
+      return !(getRole() & PrivateHeader) ||
+             (M && M->getTopLevelModule() == getModule()->getTopLevelModule());
+    }
+
     // \brief Whether this known header is valid (i.e., it has an
     // associated module).
     explicit operator bool() const {
diff --git a/clang/include/clang/Lex/Preprocessor.h b/clang/include/clang/Lex/Preprocessor.h
index 817c09fcdbc..30cc37f6f88 100644
--- a/clang/include/clang/Lex/Preprocessor.h
+++ b/clang/include/clang/Lex/Preprocessor.h
@@ -1891,6 +1891,19 @@ public:
   /// directly or indirectly.
   Module *getModuleContainingLocation(SourceLocation Loc);
 
+  /// \brief We want to produce a diagnostic at location IncLoc concerning a
+  /// missing module import.
+  ///
+  /// \param IncLoc The location at which the missing import was detected.
+  /// \param MLoc A location within the desired module at which some desired
+  ///        effect occurred (eg, where a desired entity was declared).
+  ///
+  /// \return A file that can be #included to import a module containing MLoc.
+  ///         Null if no such file could be determined or if a #include is not
+  ///         appropriate.
+  const FileEntry *getModuleHeaderToIncludeForDiagnostics(SourceLocation IncLoc,
+                                                          SourceLocation MLoc);
+
 private:
   // Macro handling.
   void HandleDefineDirective(Token &Tok, bool ImmediatelyAfterTopLevelIfndef);
diff --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp
index f663caeff81..4e88248607b 100644
--- a/clang/lib/Lex/HeaderSearch.cpp
+++ b/clang/lib/Lex/HeaderSearch.cpp
@@ -1419,3 +1419,54 @@ void HeaderSearch::loadSubdirectoryModuleMaps(DirectoryLookup &SearchDir) {
 
   SearchDir.setSearchedAllModuleMaps(true);
 }
+
+std::string HeaderSearch::suggestPathToFileForDiagnostics(const FileEntry *File,
+                                                          bool *IsSystem) {
+  // FIXME: We assume that the path name currently cached in the FileEntry is
+  // the most appropriate one for this analysis (and that it's spelled the same
+  // way as the corresponding header search path).
+  const char *Name = File->getName();
+
+  unsigned BestPrefixLength = 0;
+  unsigned BestSearchDir;
+
+  for (unsigned I = 0; I != SearchDirs.size(); ++I) {
+    // FIXME: Support this search within frameworks and header maps.
+    if (!SearchDirs[I].isNormalDir())
+      continue;
+
+    const char *Dir = SearchDirs[I].getDir()->getName();
+    for (auto NI = llvm::sys::path::begin(Name),
+              NE = llvm::sys::path::end(Name),
+              DI = llvm::sys::path::begin(Dir),
+              DE = llvm::sys::path::end(Dir);
+         /*termination condition in loop*/; ++NI, ++DI) {
+      // '.' components in Name are ignored.
+      while (NI != NE && *NI == ".")
+        ++NI;
+      if (NI == NE)
+        break;
+
+      // '.' components in Dir are ignored.
+      while (DI != DE && *DI == ".")
+        ++DI;
+      if (DI == DE) {
+        // Dir is a prefix of Name, up to '.' components and choice of path
+        // separators.
+        unsigned PrefixLength = NI - llvm::sys::path::begin(Name);
+        if (PrefixLength > BestPrefixLength) {
+          BestPrefixLength = PrefixLength;
+          BestSearchDir = I;
+        }
+        break;
+      }
+
+      if (*NI != *DI)
+        break;
+    }
+  }
+
+  if (IsSystem)
+    *IsSystem = BestPrefixLength ? BestSearchDir >= SystemDirIdx : false;
+  return Name + BestPrefixLength;
+}
diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index 4b782a3e8ec..467ae7ec518 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -209,29 +209,25 @@ ModuleMap::findHeaderInUmbrellaDirs(const FileEntry *File,
 
 static bool violatesPrivateInclude(Module *RequestingModule,
                                    const FileEntry *IncFileEnt,
-                                   ModuleMap::ModuleHeaderRole Role,
-                                   Module *RequestedModule) {
-  bool IsPrivateRole = Role & ModuleMap::PrivateHeader;
+                                   ModuleMap::KnownHeader Header) {
 #ifndef NDEBUG
-  if (IsPrivateRole) {
+  if (Header.getRole() & ModuleMap::PrivateHeader) {
     // Check for consistency between the module header role
     // as obtained from the lookup and as obtained from the module.
     // This check is not cheap, so enable it only for debugging.
     bool IsPrivate = false;
     SmallVectorImpl<Module::Header> *HeaderList[] = {
-        &RequestedModule->Headers[Module::HK_Private],
-        &RequestedModule->Headers[Module::HK_PrivateTextual]};
+        &Header.getModule()->Headers[Module::HK_Private],
+        &Header.getModule()->Headers[Module::HK_PrivateTextual]};
     for (auto *Hs : HeaderList)
       IsPrivate |=
           std::find_if(Hs->begin(), Hs->end(), [&](const Module::Header &H) {
             return H.Entry == IncFileEnt;
           }) != Hs->end();
-    assert((!IsPrivateRole || IsPrivate) && "inconsistent headers and roles");
+    assert(IsPrivate && "inconsistent headers and roles");
   }
 #endif
-  return IsPrivateRole && (!RequestingModule ||
-                           RequestedModule->getTopLevelModule() !=
-                               RequestingModule->getTopLevelModule());
+  return !Header.isAccessibleFrom(RequestingModule);
 }
 
 static Module *getTopLevelOrNull(Module *M) {
@@ -259,8 +255,7 @@ void ModuleMap::diagnoseHeaderInclusion(Module *RequestingModule,
   if (Known != Headers.end()) {
     for (const KnownHeader &Header : Known->second) {
       // Remember private headers for later printing of a diagnostic.
-      if (violatesPrivateInclude(RequestingModule, File, Header.getRole(),
-                                 Header.getModule())) {
+      if (violatesPrivateInclude(RequestingModule, File, Header)) {
         Private = Header.getModule();
         continue;
       }
diff --git a/clang/lib/Lex/PPDirectives.cpp b/clang/lib/Lex/PPDirectives.cpp
index 59ea27c5868..c33f9aba267 100644
--- a/clang/lib/Lex/PPDirectives.cpp
+++ b/clang/lib/Lex/PPDirectives.cpp
@@ -597,6 +597,62 @@ Module *Preprocessor::getModuleContainingLocation(SourceLocation Loc) {
       FullSourceLoc(Loc, SourceMgr));
 }
 
+const FileEntry *
+Preprocessor::getModuleHeaderToIncludeForDiagnostics(SourceLocation IncLoc,
+                                                     SourceLocation Loc) {
+  // If we have a module import syntax, we shouldn't include a header to
+  // make a particular module visible.
+  if (getLangOpts().ObjC2)
+    return nullptr;
+
+  // Figure out which module we'd want to import.
+  Module *M = getModuleContainingLocation(Loc);
+  if (!M)
+    return nullptr;
+
+  Module *TopM = M->getTopLevelModule();
+  Module *IncM = getModuleForLocation(IncLoc);
+
+  // Walk up through the include stack, looking through textual headers of M
+  // until we hit a non-textual header that we can #include. (We assume textual
+  // headers of a module with non-textual headers aren't meant to be used to
+  // import entities from the module.)
+  auto &SM = getSourceManager();
+  while (!Loc.isInvalid() && !SM.isInMainFile(Loc)) {
+    auto ID = SM.getFileID(SM.getExpansionLoc(Loc));
+    auto *FE = SM.getFileEntryForID(ID);
+
+    bool InTextualHeader = false;
+    for (auto Header : HeaderInfo.getModuleMap().findAllModulesForHeader(FE)) {
+      if (!Header.getModule()->isSubModuleOf(TopM))
+        continue;
+
+      if (!(Header.getRole() & ModuleMap::TextualHeader)) {
+        // If this is an accessible, non-textual header of M's top-level module
+        // that transitively includes the given location and makes the
+        // corresponding module visible, this is the thing to #include.
+        if (Header.isAccessibleFrom(IncM))
+          return FE;
+
+        // It's in a private header; we can't #include it.
+        // FIXME: If there's a public header in some module that re-exports it,
+        // then we could suggest including that, but it's not clear that's the
+        // expected way to make this entity visible.
+        continue;
+      }
+
+      InTextualHeader = true;
+    }
+
+    if (!InTextualHeader)
+      break;
+
+    Loc = SM.getIncludeLoc(ID);
+  }
+
+  return nullptr;
+}
+
 const FileEntry *Preprocessor::LookupFile(
     SourceLocation FilenameLoc,
     StringRef Filename,
diff --git a/clang/lib/Sema/SemaLookup.cpp b/clang/lib/Sema/SemaLookup.cpp
index 08133d68899..66e3601f047 100644
--- a/clang/lib/Sema/SemaLookup.cpp
+++ b/clang/lib/Sema/SemaLookup.cpp
@@ -4929,6 +4929,16 @@ void Sema::diagnoseMissingImport(SourceLocation Loc, NamedDecl *Decl,
                         Recover);
 }
 
+/// \brief Get a "quoted.h" or <angled.h> include path to use in a diagnostic
+/// suggesting the addition of a #include of the specified file.
+static std::string getIncludeStringForHeader(Preprocessor &PP,
+                                             const FileEntry *E) {
+  bool IsSystem;
+  auto Path =
+      PP.getHeaderSearchInfo().suggestPathToFileForDiagnostics(E, &IsSystem);
+  return (IsSystem ? '<' : '"') + Path + (IsSystem ? '>' : '"');
+}
+
 void Sema::diagnoseMissingImport(SourceLocation UseLoc, NamedDecl *Decl,
                                  SourceLocation DeclLoc,
                                  ArrayRef<Module *> Modules,
@@ -4949,6 +4959,16 @@ void Sema::diagnoseMissingImport(SourceLocation UseLoc, NamedDecl *Decl,
 
     Diag(UseLoc, diag::err_module_unimported_use_multiple)
       << (int)MIK << Decl << ModuleList;
+  } else if (const FileEntry *E =
+                 PP.getModuleHeaderToIncludeForDiagnostics(UseLoc, DeclLoc)) {
+    // The right way to make the declaration visible is to include a header;
+    // suggest doing so.
+    //
+    // FIXME: Find a smart place to suggest inserting a #include, and add
+    // a FixItHint there.
+    Diag(UseLoc, diag::err_module_unimported_use_header)
+      << (int)MIK << Decl << Modules[0]->getFullModuleName()
+      << getIncludeStringForHeader(PP, E);
   } else {
     Diag(UseLoc, diag::err_module_unimported_use)
       << (int)MIK << Decl << Modules[0]->getFullModuleName();
diff --git a/clang/test/Modules/Inputs/suggest-include/empty.h b/clang/test/Modules/Inputs/suggest-include/empty.h
new file mode 100644
index 00000000000..e69de29bb2d
--- /dev/null
+++ b/clang/test/Modules/Inputs/suggest-include/empty.h
diff --git a/clang/test/Modules/Inputs/suggest-include/module.modulemap b/clang/test/Modules/Inputs/suggest-include/module.modulemap
new file mode 100644
index 00000000000..46afd7b2c21
--- /dev/null
+++ b/clang/test/Modules/Inputs/suggest-include/module.modulemap
@@ -0,0 +1,22 @@
+module X {
+  module Empty { header "empty.h" }
+
+  exclude header "textual1.h"
+  textual header "textual2.h"
+  textual header "textual3.h"
+
+  module A { header "usetextual1.h" }
+  module B { header "usetextual2.h" }
+  module C { header "usetextual3.h" }
+  module D { header "usetextual4.h" }
+  module E { header "usetextual5.h" }
+
+  module P { private header "private1.h" }
+  module Q { private header "private2.h" }
+  module R { private header "private3.h" }
+  module S { header "useprivate1.h" export * }
+  module T { header "useprivate3.h" }
+}
+
+module Other { textual header "textual4.h" }
+
diff --git a/clang/test/Modules/Inputs/suggest-include/private1.h b/clang/test/Modules/Inputs/suggest-include/private1.h
new file mode 100644
index 00000000000..afc7ac71bb4
--- /dev/null
+++ b/clang/test/Modules/Inputs/suggest-include/private1.h
@@ -0,0 +1 @@
+extern int private1;
diff --git a/clang/test/Modules/Inputs/suggest-include/private2.h b/clang/test/Modules/Inputs/suggest-include/private2.h
new file mode 100644
index 00000000000..24a1893d31e
--- /dev/null
+++ b/clang/test/Modules/Inputs/suggest-include/private2.h
@@ -0,0 +1 @@
+extern int private2;
diff --git a/clang/test/Modules/Inputs/suggest-include/private3.h b/clang/test/Modules/Inputs/suggest-include/private3.h
new file mode 100644
index 00000000000..26852af2a6a
--- /dev/null
+++ b/clang/test/Modules/Inputs/suggest-include/private3.h
@@ -0,0 +1 @@
+extern int private3;
diff --git a/clang/test/Modules/Inputs/suggest-include/textual1.h b/clang/test/Modules/Inputs/suggest-include/textual1.h
new file mode 100644
index 00000000000..5b18bfb3685
--- /dev/null
+++ b/clang/test/Modules/Inputs/suggest-include/textual1.h
@@ -0,0 +1 @@
+#define FOO(X) X
diff --git a/clang/test/Modules/Inputs/suggest-include/textual2.h b/clang/test/Modules/Inputs/suggest-include/textual2.h
new file mode 100644
index 00000000000..0c06d4ea454
--- /dev/null
+++ b/clang/test/Modules/Inputs/suggest-include/textual2.h
@@ -0,0 +1 @@
+EXPAND_MACRO
diff --git a/clang/test/Modules/Inputs/suggest-include/textual3.h b/clang/test/Modules/Inputs/suggest-include/textual3.h
new file mode 100644
index 00000000000..1e525211610
--- /dev/null
+++ b/clang/test/Modules/Inputs/suggest-include/textual3.h
@@ -0,0 +1 @@
+extern int textual3;
diff --git a/clang/test/Modules/Inputs/suggest-include/textual4.h b/clang/test/Modules/Inputs/suggest-include/textual4.h
new file mode 100644
index 00000000000..091e0c0ac19
--- /dev/null
+++ b/clang/test/Modules/Inputs/suggest-include/textual4.h
@@ -0,0 +1 @@
+extern int textual4;
diff --git a/clang/test/Modules/Inputs/suggest-include/textual5.h b/clang/test/Modules/Inputs/suggest-include/textual5.h
new file mode 100644
index 00000000000..d808617d500
--- /dev/null
+++ b/clang/test/Modules/Inputs/suggest-include/textual5.h
@@ -0,0 +1 @@
+extern int textual5;
diff --git a/clang/test/Modules/Inputs/suggest-include/useprivate1.h b/clang/test/Modules/Inputs/suggest-include/useprivate1.h
new file mode 100644
index 00000000000..817b900eccc
--- /dev/null
+++ b/clang/test/Modules/Inputs/suggest-include/useprivate1.h
@@ -0,0 +1 @@
+#include "private1.h"
diff --git a/clang/test/Modules/Inputs/suggest-include/useprivate3.h b/clang/test/Modules/Inputs/suggest-include/useprivate3.h
new file mode 100644
index 00000000000..5d5d221b87b
--- /dev/null
+++ b/clang/test/Modules/Inputs/suggest-include/useprivate3.h
@@ -0,0 +1 @@
+#include "private3.h"
diff --git a/clang/test/Modules/Inputs/suggest-include/usetextual1.h b/clang/test/Modules/Inputs/suggest-include/usetextual1.h
new file mode 100644
index 00000000000..34ab1c76bcc
--- /dev/null
+++ b/clang/test/Modules/Inputs/suggest-include/usetextual1.h
@@ -0,0 +1,2 @@
+#include "textual1.h"
+FOO(extern int usetextual1;)
diff --git a/clang/test/Modules/Inputs/suggest-include/usetextual2.h b/clang/test/Modules/Inputs/suggest-include/usetextual2.h
new file mode 100644
index 00000000000..95b24457363
--- /dev/null
+++ b/clang/test/Modules/Inputs/suggest-include/usetextual2.h
@@ -0,0 +1,2 @@
+#define EXPAND_MACRO extern int usetextual2;
+#include "textual2.h"
diff --git a/clang/test/Modules/Inputs/suggest-include/usetextual3.h b/clang/test/Modules/Inputs/suggest-include/usetextual3.h
new file mode 100644
index 00000000000..15a75cc839f
--- /dev/null
+++ b/clang/test/Modules/Inputs/suggest-include/usetextual3.h
@@ -0,0 +1 @@
+#include "textual3.h"
diff --git a/clang/test/Modules/Inputs/suggest-include/usetextual4.h b/clang/test/Modules/Inputs/suggest-include/usetextual4.h
new file mode 100644
index 00000000000..395bb6fd65b
--- /dev/null
+++ b/clang/test/Modules/Inputs/suggest-include/usetextual4.h
@@ -0,0 +1 @@
+#include "textual4.h"
diff --git a/clang/test/Modules/Inputs/suggest-include/usetextual5.h b/clang/test/Modules/Inputs/suggest-include/usetextual5.h
new file mode 100644
index 00000000000..a7335d37aeb
--- /dev/null
+++ b/clang/test/Modules/Inputs/suggest-include/usetextual5.h
@@ -0,0 +1 @@
+#include "textual5.h"
diff --git a/clang/test/Modules/suggest-include.cpp b/clang/test/Modules/suggest-include.cpp
new file mode 100644
index 00000000000..e10c3f38aba
--- /dev/null
+++ b/clang/test/Modules/suggest-include.cpp
@@ -0,0 +1,33 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -fimplicit-module-maps -I%S/Inputs/suggest-include %s -verify
+
+#include "empty.h" // import the module file
+
+// expected-note@usetextual1.h:2 {{previous}}
+// expected-note@textual2.h:1 {{previous}}
+// expected-note@textual3.h:1 {{previous}}
+// expected-note@textual4.h:1 {{previous}}
+// expected-note@textual5.h:1 {{previous}}
+// expected-note@private1.h:1 {{previous}}
+// expected-note@private2.h:1 {{previous}}
+// expected-note@private3.h:1 {{previous}}
+
+void f() {
+  (void)::usetextual1; // expected-error {{missing '#include "usetextual1.h"'}}
+  (void)::usetextual2; // expected-error {{missing '#include "usetextual2.h"'}}
+  (void)::textual3; // expected-error-re {{{{^}}missing '#include "usetextual3.h"'}}
+  // Don't suggest a #include that includes the entity via a path that leaves
+  // the module. In that case we can't be sure that we've picked the right header.
+  (void)::textual4; // expected-error-re {{{{^}}declaration of 'textual4'}}
+  (void)::textual5; // expected-error-re {{{{^}}declaration of 'textual5'}}
+
+  // Don't suggest #including a private header.
+  // FIXME: We could suggest including "useprivate1.h" here, as it's the only
+  // public way to get at this declaration.
+  (void)::private1; // expected-error-re {{{{^}}declaration of 'private1'}}
+  // FIXME: Should we be suggesting an import at all here? Should declarations
+  // in private headers be visible when the surrounding module is imported?
+  (void)::private2; // expected-error-re {{{{^}}declaration of 'private2'}}
+  // Even if we suggest an include for private1, we should not do so here.
+  (void)::private3; // expected-error-re {{{{^}}declaration of 'private3'}}
+}