From 54cac3348ad57029ef9511cfdfb4e5ba8ab79d2e Mon Sep 17 00:00:00 2001 From: Volodymyr Sapsai Date: Tue, 5 Feb 2019 22:34:55 +0000 Subject: [PATCH] [Preprocessor] Add a note with framework location for "file not found" error. When a framework with the same name is available at multiple framework search paths, we use the first matching location. If a framework at this location doesn't have all the headers, it can be confusing for developers because they see only an error `'Foo/Foo.h' file not found`, can find the complete framework with required header, and don't know the incomplete framework was used instead. Add a note explaining a framework without required header was found. Also mention framework directory path to make it easier to find the incomplete framework. rdar://problem/39246514 Reviewers: arphaman, erik.pilkington, jkorous Reviewed By: jkorous Subscribers: jkorous, dexonsmith, cfe-commits Differential Revision: https://reviews.llvm.org/D56561 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@353231 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Basic/DiagnosticLexKinds.td | 2 + include/clang/Lex/DirectoryLookup.h | 7 +++- include/clang/Lex/HeaderSearch.h | 29 +++++++++------ include/clang/Lex/Preprocessor.h | 3 +- lib/Frontend/FrontendActions.cpp | 2 +- lib/Frontend/Rewrite/InclusionRewriter.cpp | 2 +- lib/Frontend/VerifyDiagnosticConsumer.cpp | 2 +- lib/Lex/HeaderSearch.cpp | 31 ++++++++++------ lib/Lex/PPDirectives.cpp | 37 ++++++++++++++----- lib/Lex/PPMacroExpansion.cpp | 2 +- lib/Lex/Pragma.cpp | 2 +- lib/Lex/Preprocessor.cpp | 3 +- .../include-header-missing-in-framework.c | 18 +++++++++ 13 files changed, 100 insertions(+), 40 deletions(-) create mode 100644 test/Preprocessor/include-header-missing-in-framework.c diff --git a/include/clang/Basic/DiagnosticLexKinds.td b/include/clang/Basic/DiagnosticLexKinds.td index 684251678e..bb85bed94d 100644 --- a/include/clang/Basic/DiagnosticLexKinds.td +++ b/include/clang/Basic/DiagnosticLexKinds.td @@ -418,6 +418,8 @@ def err_pp_file_not_found_angled_include_not_fatal : Error< "'%0' file not found with include; use \"quotes\" instead">; def err_pp_file_not_found_typo_not_fatal : Error<"'%0' file not found, did you mean '%1'?">; +def note_pp_framework_without_header : Note< + "did not find header '%0' in framework '%1' (loaded from '%2')">; def err_pp_error_opening_file : Error< "error opening file '%0': %1">, DefaultFatal; def err_pp_empty_filename : Error<"empty filename">; diff --git a/include/clang/Lex/DirectoryLookup.h b/include/clang/Lex/DirectoryLookup.h index 6b22f8fb3f..7c556ac351 100644 --- a/include/clang/Lex/DirectoryLookup.h +++ b/include/clang/Lex/DirectoryLookup.h @@ -170,6 +170,9 @@ public: /// set to true if the file is located in a framework that has been /// user-specified to be treated as a system framework. /// + /// \param [out] IsFrameworkFound For a framework directory set to true if + /// specified '.framework' directory is found. + /// /// \param [out] MappedName if this is a headermap which maps the filename to /// a framework include ("Foo.h" -> "Foo/Foo.h"), set the new name to this /// vector and point Filename to it. @@ -180,6 +183,7 @@ public: Module *RequestingModule, ModuleMap::KnownHeader *SuggestedModule, bool &InUserSpecifiedSystemFramework, + bool &IsFrameworkFound, bool &HasBeenMapped, SmallVectorImpl &MappedName) const; @@ -190,7 +194,8 @@ private: SmallVectorImpl *RelativePath, Module *RequestingModule, ModuleMap::KnownHeader *SuggestedModule, - bool &InUserSpecifiedSystemFramework) const; + bool &InUserSpecifiedSystemFramework, + bool &IsFrameworkFound) const; }; diff --git a/include/clang/Lex/HeaderSearch.h b/include/clang/Lex/HeaderSearch.h index 53821672ef..879d24d0c6 100644 --- a/include/clang/Lex/HeaderSearch.h +++ b/include/clang/Lex/HeaderSearch.h @@ -142,22 +142,22 @@ public: virtual HeaderFileInfo GetHeaderFileInfo(const FileEntry *FE) = 0; }; +/// This structure is used to record entries in our framework cache. +struct FrameworkCacheEntry { + /// The directory entry which should be used for the cached framework. + const DirectoryEntry *Directory; + + /// Whether this framework has been "user-specified" to be treated as if it + /// were a system framework (even if it was found outside a system framework + /// directory). + bool IsUserSpecifiedSystemFramework; +}; + /// Encapsulates the information needed to find the file referenced /// by a \#include or \#include_next, (sub-)framework lookup, etc. class HeaderSearch { friend class DirectoryLookup; - /// This structure is used to record entries in our framework cache. - struct FrameworkCacheEntry { - /// The directory entry which should be used for the cached framework. - const DirectoryEntry *Directory; - - /// Whether this framework has been "user-specified" to be treated as if it - /// were a system framework (even if it was found outside a system framework - /// directory). - bool IsUserSpecifiedSystemFramework; - }; - /// Header-search options used to initialize this header search. std::shared_ptr HSOpts; @@ -390,13 +390,18 @@ public: /// /// \param IsMapped If non-null, and the search involved header maps, set to /// true. + /// + /// \param IsFrameworkFound If non-null, will be set to true if a framework is + /// found in any of searched SearchDirs. Doesn't guarantee the requested file + /// is found. const FileEntry *LookupFile( StringRef Filename, SourceLocation IncludeLoc, bool isAngled, const DirectoryLookup *FromDir, const DirectoryLookup *&CurDir, ArrayRef> Includers, SmallVectorImpl *SearchPath, SmallVectorImpl *RelativePath, Module *RequestingModule, ModuleMap::KnownHeader *SuggestedModule, - bool *IsMapped, bool SkipCache = false, bool BuildSystemModule = false); + bool *IsMapped, bool *IsFrameworkFound, bool SkipCache = false, + bool BuildSystemModule = false); /// Look up a subframework for the specified \#include file. /// diff --git a/include/clang/Lex/Preprocessor.h b/include/clang/Lex/Preprocessor.h index dc99753ae9..e701f14695 100644 --- a/include/clang/Lex/Preprocessor.h +++ b/include/clang/Lex/Preprocessor.h @@ -1854,7 +1854,8 @@ public: SmallVectorImpl *SearchPath, SmallVectorImpl *RelativePath, ModuleMap::KnownHeader *SuggestedModule, - bool *IsMapped, bool SkipCache = false); + bool *IsMapped, bool *IsFrameworkFound, + bool SkipCache = false); /// Get the DirectoryLookup structure used to find the current /// FileEntry, if CurLexer is non-null and if applicable. diff --git a/lib/Frontend/FrontendActions.cpp b/lib/Frontend/FrontendActions.cpp index 9b9051212c..a71d74ec79 100644 --- a/lib/Frontend/FrontendActions.cpp +++ b/lib/Frontend/FrontendActions.cpp @@ -286,7 +286,7 @@ bool GenerateHeaderModuleAction::BeginSourceFileAction( const DirectoryLookup *CurDir = nullptr; const FileEntry *FE = HS.LookupFile( Name, SourceLocation(), /*Angled*/ false, nullptr, CurDir, - None, nullptr, nullptr, nullptr, nullptr, nullptr); + None, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr); if (!FE) { CI.getDiagnostics().Report(diag::err_module_header_file_not_found) << Name; diff --git a/lib/Frontend/Rewrite/InclusionRewriter.cpp b/lib/Frontend/Rewrite/InclusionRewriter.cpp index 4797a64991..cb4e773aca 100644 --- a/lib/Frontend/Rewrite/InclusionRewriter.cpp +++ b/lib/Frontend/Rewrite/InclusionRewriter.cpp @@ -414,7 +414,7 @@ bool InclusionRewriter::HandleHasInclude( // FIXME: Why don't we call PP.LookupFile here? const FileEntry *File = PP.getHeaderSearchInfo().LookupFile( Filename, SourceLocation(), isAngled, Lookup, CurDir, Includers, nullptr, - nullptr, nullptr, nullptr, nullptr); + nullptr, nullptr, nullptr, nullptr, nullptr); FileExists = File != nullptr; return true; diff --git a/lib/Frontend/VerifyDiagnosticConsumer.cpp b/lib/Frontend/VerifyDiagnosticConsumer.cpp index 33cb148982..95f847cfa4 100644 --- a/lib/Frontend/VerifyDiagnosticConsumer.cpp +++ b/lib/Frontend/VerifyDiagnosticConsumer.cpp @@ -480,7 +480,7 @@ static bool ParseDirective(StringRef S, ExpectedData *ED, SourceManager &SM, const DirectoryLookup *CurDir; const FileEntry *FE = PP->LookupFile(Pos, Filename, false, nullptr, nullptr, CurDir, - nullptr, nullptr, nullptr, nullptr); + nullptr, nullptr, nullptr, nullptr, nullptr); if (!FE) { Diags.Report(Pos.getLocWithOffset(PH.C-PH.Begin), diag::err_verify_missing_file) << Filename << KindStr; diff --git a/lib/Lex/HeaderSearch.cpp b/lib/Lex/HeaderSearch.cpp index 32393d1227..9c92f772c2 100644 --- a/lib/Lex/HeaderSearch.cpp +++ b/lib/Lex/HeaderSearch.cpp @@ -334,6 +334,7 @@ const FileEntry *DirectoryLookup::LookupFile( Module *RequestingModule, ModuleMap::KnownHeader *SuggestedModule, bool &InUserSpecifiedSystemFramework, + bool &IsFrameworkFound, bool &HasBeenMapped, SmallVectorImpl &MappedName) const { InUserSpecifiedSystemFramework = false; @@ -362,7 +363,7 @@ const FileEntry *DirectoryLookup::LookupFile( if (isFramework()) return DoFrameworkLookup(Filename, HS, SearchPath, RelativePath, RequestingModule, SuggestedModule, - InUserSpecifiedSystemFramework); + InUserSpecifiedSystemFramework, IsFrameworkFound); assert(isHeaderMap() && "Unknown directory lookup"); const HeaderMap *HM = getHeaderMap(); @@ -462,7 +463,7 @@ const FileEntry *DirectoryLookup::DoFrameworkLookup( StringRef Filename, HeaderSearch &HS, SmallVectorImpl *SearchPath, SmallVectorImpl *RelativePath, Module *RequestingModule, ModuleMap::KnownHeader *SuggestedModule, - bool &InUserSpecifiedSystemFramework) const { + bool &InUserSpecifiedSystemFramework, bool &IsFrameworkFound) const { FileManager &FileMgr = HS.getFileMgr(); // Framework names must have a '/' in the filename. @@ -471,7 +472,7 @@ const FileEntry *DirectoryLookup::DoFrameworkLookup( // Find out if this is the home for the specified framework, by checking // HeaderSearch. Possible answers are yes/no and unknown. - HeaderSearch::FrameworkCacheEntry &CacheEntry = + FrameworkCacheEntry &CacheEntry = HS.LookupFrameworkCache(Filename.substr(0, SlashPos)); // If it is known and in some other directory, fail. @@ -516,8 +517,9 @@ const FileEntry *DirectoryLookup::DoFrameworkLookup( } } - // Set the 'user-specified system framework' flag. + // Set out flags. InUserSpecifiedSystemFramework = CacheEntry.IsUserSpecifiedSystemFramework; + IsFrameworkFound = CacheEntry.Directory; if (RelativePath) { RelativePath->clear(); @@ -696,10 +698,14 @@ const FileEntry *HeaderSearch::LookupFile( ArrayRef> Includers, SmallVectorImpl *SearchPath, SmallVectorImpl *RelativePath, Module *RequestingModule, ModuleMap::KnownHeader *SuggestedModule, - bool *IsMapped, bool SkipCache, bool BuildSystemModule) { + bool *IsMapped, bool *IsFrameworkFound, bool SkipCache, + bool BuildSystemModule) { if (IsMapped) *IsMapped = false; + if (IsFrameworkFound) + *IsFrameworkFound = false; + if (SuggestedModule) *SuggestedModule = ModuleMap::KnownHeader(); @@ -851,16 +857,19 @@ const FileEntry *HeaderSearch::LookupFile( for (; i != SearchDirs.size(); ++i) { bool InUserSpecifiedSystemFramework = false; bool HasBeenMapped = false; + bool IsFrameworkFoundInDir = false; const FileEntry *FE = SearchDirs[i].LookupFile( Filename, *this, IncludeLoc, SearchPath, RelativePath, RequestingModule, - SuggestedModule, InUserSpecifiedSystemFramework, HasBeenMapped, - MappedName); + SuggestedModule, InUserSpecifiedSystemFramework, IsFrameworkFoundInDir, + HasBeenMapped, MappedName); if (HasBeenMapped) { CacheLookup.MappedName = copyString(Filename, LookupFileCache.getAllocator()); if (IsMapped) *IsMapped = true; } + if (IsFrameworkFound) + *IsFrameworkFound |= IsFrameworkFoundInDir; if (!FE) continue; CurDir = &SearchDirs[i]; @@ -926,10 +935,10 @@ const FileEntry *HeaderSearch::LookupFile( ScratchFilename += '/'; ScratchFilename += Filename; - const FileEntry *FE = - LookupFile(ScratchFilename, IncludeLoc, /*isAngled=*/true, FromDir, - CurDir, Includers.front(), SearchPath, RelativePath, - RequestingModule, SuggestedModule, IsMapped); + const FileEntry *FE = LookupFile( + ScratchFilename, IncludeLoc, /*isAngled=*/true, FromDir, CurDir, + Includers.front(), SearchPath, RelativePath, RequestingModule, + SuggestedModule, IsMapped, /*IsFrameworkFound=*/nullptr); if (checkMSVCHeaderSearch(Diags, MSFE, FE, IncludeLoc)) { if (SuggestedModule) diff --git a/lib/Lex/PPDirectives.cpp b/lib/Lex/PPDirectives.cpp index d4fad6a348..5e97d9fb45 100644 --- a/lib/Lex/PPDirectives.cpp +++ b/lib/Lex/PPDirectives.cpp @@ -665,7 +665,8 @@ const FileEntry *Preprocessor::LookupFile( const DirectoryLookup *FromDir, const FileEntry *FromFile, const DirectoryLookup *&CurDir, SmallVectorImpl *SearchPath, SmallVectorImpl *RelativePath, - ModuleMap::KnownHeader *SuggestedModule, bool *IsMapped, bool SkipCache) { + ModuleMap::KnownHeader *SuggestedModule, bool *IsMapped, + bool *IsFrameworkFound, bool SkipCache) { Module *RequestingModule = getModuleForLocation(FilenameLoc); bool RequestingModuleIsModuleInterface = !SourceMgr.isInMainFile(FilenameLoc); @@ -723,7 +724,8 @@ const FileEntry *Preprocessor::LookupFile( while (const FileEntry *FE = HeaderInfo.LookupFile( Filename, FilenameLoc, isAngled, TmpFromDir, TmpCurDir, Includers, SearchPath, RelativePath, RequestingModule, - SuggestedModule, /*IsMapped=*/nullptr, SkipCache)) { + SuggestedModule, /*IsMapped=*/nullptr, + /*IsFrameworkFound=*/nullptr, SkipCache)) { // Keep looking as if this file did a #include_next. TmpFromDir = TmpCurDir; ++TmpFromDir; @@ -739,8 +741,8 @@ const FileEntry *Preprocessor::LookupFile( // Do a standard file entry lookup. const FileEntry *FE = HeaderInfo.LookupFile( Filename, FilenameLoc, isAngled, FromDir, CurDir, Includers, SearchPath, - RelativePath, RequestingModule, SuggestedModule, IsMapped, SkipCache, - BuildSystemModule); + RelativePath, RequestingModule, SuggestedModule, IsMapped, + IsFrameworkFound, SkipCache, BuildSystemModule); if (FE) { if (SuggestedModule && !LangOpts.AsmPreprocessor) HeaderInfo.getModuleMap().diagnoseHeaderInclusion( @@ -1756,6 +1758,7 @@ void Preprocessor::HandleIncludeDirective(SourceLocation HashLoc, // Search include directories. bool IsMapped = false; + bool IsFrameworkFound = false; const DirectoryLookup *CurDir; SmallString<1024> SearchPath; SmallString<1024> RelativePath; @@ -1774,7 +1777,7 @@ void Preprocessor::HandleIncludeDirective(SourceLocation HashLoc, FilenameLoc, LangOpts.MSVCCompat ? NormalizedPath.c_str() : Filename, isAngled, LookupFrom, LookupFromFile, CurDir, Callbacks ? &SearchPath : nullptr, Callbacks ? &RelativePath : nullptr, - &SuggestedModule, &IsMapped); + &SuggestedModule, &IsMapped, &IsFrameworkFound); if (!File) { if (Callbacks) { @@ -1791,7 +1794,8 @@ void Preprocessor::HandleIncludeDirective(SourceLocation HashLoc, FilenameLoc, LangOpts.MSVCCompat ? NormalizedPath.c_str() : Filename, isAngled, LookupFrom, LookupFromFile, CurDir, nullptr, nullptr, - &SuggestedModule, &IsMapped, /*SkipCache*/ true); + &SuggestedModule, &IsMapped, /*IsFrameworkFound=*/nullptr, + /*SkipCache*/ true); } } } @@ -1806,7 +1810,8 @@ void Preprocessor::HandleIncludeDirective(SourceLocation HashLoc, LangOpts.MSVCCompat ? NormalizedPath.c_str() : Filename, false, LookupFrom, LookupFromFile, CurDir, Callbacks ? &SearchPath : nullptr, - Callbacks ? &RelativePath : nullptr, &SuggestedModule, &IsMapped); + Callbacks ? &RelativePath : nullptr, &SuggestedModule, &IsMapped, + /*IsFrameworkFound=*/nullptr); if (File) { SourceRange Range(FilenameTok.getLocation(), CharEnd); Diag(FilenameTok, diag::err_pp_file_not_found_angled_include_not_fatal) << @@ -1842,7 +1847,8 @@ void Preprocessor::HandleIncludeDirective(SourceLocation HashLoc, : TypoCorrectionName, isAngled, LookupFrom, LookupFromFile, CurDir, Callbacks ? &SearchPath : nullptr, - Callbacks ? &RelativePath : nullptr, &SuggestedModule, &IsMapped); + Callbacks ? &RelativePath : nullptr, &SuggestedModule, &IsMapped, + /*IsFrameworkFound=*/nullptr); if (File) { SourceRange Range(FilenameTok.getLocation(), CharEnd); auto Hint = isAngled @@ -1859,9 +1865,22 @@ void Preprocessor::HandleIncludeDirective(SourceLocation HashLoc, } // If the file is still not found, just go with the vanilla diagnostic - if (!File) + if (!File) { Diag(FilenameTok, diag::err_pp_file_not_found) << OriginalFilename << FilenameRange; + if (IsFrameworkFound) { + size_t SlashPos = OriginalFilename.find('/'); + assert(SlashPos != StringRef::npos && + "Include with framework name should have '/' in the filename"); + StringRef FrameworkName = OriginalFilename.substr(0, SlashPos); + FrameworkCacheEntry &CacheEntry = + HeaderInfo.LookupFrameworkCache(FrameworkName); + assert(CacheEntry.Directory && "Found framework should be in cache"); + Diag(FilenameTok, diag::note_pp_framework_without_header) + << OriginalFilename.substr(SlashPos + 1) << FrameworkName + << CacheEntry.Directory->getName(); + } + } } } diff --git a/lib/Lex/PPMacroExpansion.cpp b/lib/Lex/PPMacroExpansion.cpp index 00bc7aa888..2e9c686b2a 100644 --- a/lib/Lex/PPMacroExpansion.cpp +++ b/lib/Lex/PPMacroExpansion.cpp @@ -1235,7 +1235,7 @@ static bool EvaluateHasIncludeCommon(Token &Tok, const DirectoryLookup *CurDir; const FileEntry *File = PP.LookupFile(FilenameLoc, Filename, isAngled, LookupFrom, LookupFromFile, - CurDir, nullptr, nullptr, nullptr, nullptr); + CurDir, nullptr, nullptr, nullptr, nullptr, nullptr); if (PPCallbacks *Callbacks = PP.getPPCallbacks()) { SrcMgr::CharacteristicKind FileType = SrcMgr::C_User; diff --git a/lib/Lex/Pragma.cpp b/lib/Lex/Pragma.cpp index 41d170e0d7..c82b531737 100644 --- a/lib/Lex/Pragma.cpp +++ b/lib/Lex/Pragma.cpp @@ -506,7 +506,7 @@ void Preprocessor::HandlePragmaDependency(Token &DependencyTok) { const DirectoryLookup *CurDir; const FileEntry *File = LookupFile(FilenameTok.getLocation(), Filename, isAngled, nullptr, - nullptr, CurDir, nullptr, nullptr, nullptr, nullptr); + nullptr, CurDir, nullptr, nullptr, nullptr, nullptr, nullptr); if (!File) { if (!SuppressIncludeNotFoundError) Diag(FilenameTok, diag::err_pp_file_not_found) << Filename; diff --git a/lib/Lex/Preprocessor.cpp b/lib/Lex/Preprocessor.cpp index 2952c6408d..95193147b1 100644 --- a/lib/Lex/Preprocessor.cpp +++ b/lib/Lex/Preprocessor.cpp @@ -566,7 +566,8 @@ void Preprocessor::EnterMainSourceFile() { SourceLocation(), PPOpts->PCHThroughHeader, /*isAngled=*/false, /*FromDir=*/nullptr, /*FromFile=*/nullptr, CurDir, /*SearchPath=*/nullptr, /*RelativePath=*/nullptr, - /*SuggestedModule=*/nullptr, /*IsMapped=*/nullptr); + /*SuggestedModule=*/nullptr, /*IsMapped=*/nullptr, + /*IsFrameworkFound=*/nullptr); if (!File) { Diag(SourceLocation(), diag::err_pp_through_header_not_found) << PPOpts->PCHThroughHeader; diff --git a/test/Preprocessor/include-header-missing-in-framework.c b/test/Preprocessor/include-header-missing-in-framework.c new file mode 100644 index 0000000000..cb09326a42 --- /dev/null +++ b/test/Preprocessor/include-header-missing-in-framework.c @@ -0,0 +1,18 @@ +// RUN: %clang_cc1 -fsyntax-only -F %S/Inputs -verify %s +// RUN: %clang_cc1 -fsyntax-only -F %S/Inputs -DTYPO_CORRECTION -verify %s + +// After finding a requested framework, we don't look for the same framework in +// a different location even if requested header is not found in the framework. +// It can be confusing when there is a framework with required header later in +// header search paths. Mention in diagnostics where the header lookup stopped. + +#ifndef TYPO_CORRECTION +#include +// expected-error@-1 {{'TestFramework/NotExistingHeader.h' file not found}} +// expected-note@-2 {{did not find header 'NotExistingHeader.h' in framework 'TestFramework' (loaded from}} + +#else +// Don't emit extra note for unsuccessfully typo-corrected include. +#include <#TestFramework/NotExistingHeader.h> +// expected-error@-1 {{'#TestFramework/NotExistingHeader.h' file not found}} +#endif // TYPO_CORRECTION -- 2.40.0