From: Richard Smith Date: Wed, 27 Apr 2016 21:57:05 +0000 (+0000) Subject: [modules] When diagnosing a missing module import, suggest adding a #include if X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=d8ee96875e9a7d44f9d245763263738d2d210193;p=clang [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. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@267802 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 4e2a034964..afa741077c 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/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/include/clang/Lex/HeaderSearch.h b/include/clang/Lex/HeaderSearch.h index 6d592e19c0..458f07e5e4 100644 --- a/include/clang/Lex/HeaderSearch.h +++ b/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/include/clang/Lex/ModuleMap.h b/include/clang/Lex/ModuleMap.h index d8d374b622..b8cfb8faa8 100644 --- a/include/clang/Lex/ModuleMap.h +++ b/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/include/clang/Lex/Preprocessor.h b/include/clang/Lex/Preprocessor.h index 817c09fcdb..30cc37f6f8 100644 --- a/include/clang/Lex/Preprocessor.h +++ b/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/lib/Lex/HeaderSearch.cpp b/lib/Lex/HeaderSearch.cpp index f663caeff8..4e88248607 100644 --- a/lib/Lex/HeaderSearch.cpp +++ b/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/lib/Lex/ModuleMap.cpp b/lib/Lex/ModuleMap.cpp index 4b782a3e8e..467ae7ec51 100644 --- a/lib/Lex/ModuleMap.cpp +++ b/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 *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/lib/Lex/PPDirectives.cpp b/lib/Lex/PPDirectives.cpp index 59ea27c586..c33f9aba26 100644 --- a/lib/Lex/PPDirectives.cpp +++ b/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/lib/Sema/SemaLookup.cpp b/lib/Sema/SemaLookup.cpp index 08133d6889..66e3601f04 100644 --- a/lib/Sema/SemaLookup.cpp +++ b/lib/Sema/SemaLookup.cpp @@ -4929,6 +4929,16 @@ void Sema::diagnoseMissingImport(SourceLocation Loc, NamedDecl *Decl, Recover); } +/// \brief Get a "quoted.h" or 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 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/test/Modules/Inputs/suggest-include/empty.h b/test/Modules/Inputs/suggest-include/empty.h new file mode 100644 index 0000000000..e69de29bb2 diff --git a/test/Modules/Inputs/suggest-include/module.modulemap b/test/Modules/Inputs/suggest-include/module.modulemap new file mode 100644 index 0000000000..46afd7b2c2 --- /dev/null +++ b/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/test/Modules/Inputs/suggest-include/private1.h b/test/Modules/Inputs/suggest-include/private1.h new file mode 100644 index 0000000000..afc7ac71bb --- /dev/null +++ b/test/Modules/Inputs/suggest-include/private1.h @@ -0,0 +1 @@ +extern int private1; diff --git a/test/Modules/Inputs/suggest-include/private2.h b/test/Modules/Inputs/suggest-include/private2.h new file mode 100644 index 0000000000..24a1893d31 --- /dev/null +++ b/test/Modules/Inputs/suggest-include/private2.h @@ -0,0 +1 @@ +extern int private2; diff --git a/test/Modules/Inputs/suggest-include/private3.h b/test/Modules/Inputs/suggest-include/private3.h new file mode 100644 index 0000000000..26852af2a6 --- /dev/null +++ b/test/Modules/Inputs/suggest-include/private3.h @@ -0,0 +1 @@ +extern int private3; diff --git a/test/Modules/Inputs/suggest-include/textual1.h b/test/Modules/Inputs/suggest-include/textual1.h new file mode 100644 index 0000000000..5b18bfb368 --- /dev/null +++ b/test/Modules/Inputs/suggest-include/textual1.h @@ -0,0 +1 @@ +#define FOO(X) X diff --git a/test/Modules/Inputs/suggest-include/textual2.h b/test/Modules/Inputs/suggest-include/textual2.h new file mode 100644 index 0000000000..0c06d4ea45 --- /dev/null +++ b/test/Modules/Inputs/suggest-include/textual2.h @@ -0,0 +1 @@ +EXPAND_MACRO diff --git a/test/Modules/Inputs/suggest-include/textual3.h b/test/Modules/Inputs/suggest-include/textual3.h new file mode 100644 index 0000000000..1e52521161 --- /dev/null +++ b/test/Modules/Inputs/suggest-include/textual3.h @@ -0,0 +1 @@ +extern int textual3; diff --git a/test/Modules/Inputs/suggest-include/textual4.h b/test/Modules/Inputs/suggest-include/textual4.h new file mode 100644 index 0000000000..091e0c0ac1 --- /dev/null +++ b/test/Modules/Inputs/suggest-include/textual4.h @@ -0,0 +1 @@ +extern int textual4; diff --git a/test/Modules/Inputs/suggest-include/textual5.h b/test/Modules/Inputs/suggest-include/textual5.h new file mode 100644 index 0000000000..d808617d50 --- /dev/null +++ b/test/Modules/Inputs/suggest-include/textual5.h @@ -0,0 +1 @@ +extern int textual5; diff --git a/test/Modules/Inputs/suggest-include/useprivate1.h b/test/Modules/Inputs/suggest-include/useprivate1.h new file mode 100644 index 0000000000..817b900ecc --- /dev/null +++ b/test/Modules/Inputs/suggest-include/useprivate1.h @@ -0,0 +1 @@ +#include "private1.h" diff --git a/test/Modules/Inputs/suggest-include/useprivate3.h b/test/Modules/Inputs/suggest-include/useprivate3.h new file mode 100644 index 0000000000..5d5d221b87 --- /dev/null +++ b/test/Modules/Inputs/suggest-include/useprivate3.h @@ -0,0 +1 @@ +#include "private3.h" diff --git a/test/Modules/Inputs/suggest-include/usetextual1.h b/test/Modules/Inputs/suggest-include/usetextual1.h new file mode 100644 index 0000000000..34ab1c76bc --- /dev/null +++ b/test/Modules/Inputs/suggest-include/usetextual1.h @@ -0,0 +1,2 @@ +#include "textual1.h" +FOO(extern int usetextual1;) diff --git a/test/Modules/Inputs/suggest-include/usetextual2.h b/test/Modules/Inputs/suggest-include/usetextual2.h new file mode 100644 index 0000000000..95b2445736 --- /dev/null +++ b/test/Modules/Inputs/suggest-include/usetextual2.h @@ -0,0 +1,2 @@ +#define EXPAND_MACRO extern int usetextual2; +#include "textual2.h" diff --git a/test/Modules/Inputs/suggest-include/usetextual3.h b/test/Modules/Inputs/suggest-include/usetextual3.h new file mode 100644 index 0000000000..15a75cc839 --- /dev/null +++ b/test/Modules/Inputs/suggest-include/usetextual3.h @@ -0,0 +1 @@ +#include "textual3.h" diff --git a/test/Modules/Inputs/suggest-include/usetextual4.h b/test/Modules/Inputs/suggest-include/usetextual4.h new file mode 100644 index 0000000000..395bb6fd65 --- /dev/null +++ b/test/Modules/Inputs/suggest-include/usetextual4.h @@ -0,0 +1 @@ +#include "textual4.h" diff --git a/test/Modules/Inputs/suggest-include/usetextual5.h b/test/Modules/Inputs/suggest-include/usetextual5.h new file mode 100644 index 0000000000..a7335d37ae --- /dev/null +++ b/test/Modules/Inputs/suggest-include/usetextual5.h @@ -0,0 +1 @@ +#include "textual5.h" diff --git a/test/Modules/suggest-include.cpp b/test/Modules/suggest-include.cpp new file mode 100644 index 0000000000..e10c3f38ab --- /dev/null +++ b/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'}} +}