[clang][HeaderSearch] Shorten paths for includes in mainfile's directory
authorKadir Cetinkaya <kadircet@google.com>
Wed, 3 Jul 2019 07:47:19 +0000 (07:47 +0000)
committerKadir Cetinkaya <kadircet@google.com>
Wed, 3 Jul 2019 07:47:19 +0000 (07:47 +0000)
Summary:
Currently HeaderSearch only looks at SearchDir's passed into it, but in
addition to those paths headers can be relative to including file's directory.

This patch makes sure that is taken into account.

Reviewers: gribozavr

Subscribers: jkorous, arphaman, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D63295

llvm-svn: 365005

clang-tools-extra/clang-include-fixer/IncludeFixer.cpp
clang-tools-extra/clangd/CodeComplete.cpp
clang-tools-extra/clangd/Headers.cpp
clang-tools-extra/clangd/Headers.h
clang-tools-extra/clangd/IncludeFixer.cpp
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
clang-tools-extra/clangd/unittests/HeadersTests.cpp
clang/include/clang/Lex/HeaderSearch.h
clang/lib/Lex/HeaderSearch.cpp
clang/lib/Sema/SemaLookup.cpp
clang/unittests/Lex/HeaderSearchTest.cpp

index d364021..c417342 100644 (file)
@@ -314,9 +314,9 @@ std::string IncludeFixerSemaSource::minimizeInclude(
   if (!Entry)
     return Include;
 
-  bool IsSystem;
+  bool IsSystem = false;
   std::string Suggestion =
-      HeaderSearch.suggestPathToFileForDiagnostics(Entry, &IsSystem);
+      HeaderSearch.suggestPathToFileForDiagnostics(Entry, "", &IsSystem);
 
   return IsSystem ? '<' + Suggestion + '>' : '"' + Suggestion + '"';
 }
index 5bb2494..3d895b0 100644 (file)
@@ -328,7 +328,7 @@ struct CodeCompletionBuilder {
       if (!ResolvedInserted)
         return ResolvedInserted.takeError();
       return std::make_pair(
-          Includes.calculateIncludePath(*ResolvedInserted),
+          Includes.calculateIncludePath(*ResolvedInserted, FileName),
           Includes.shouldInsertInclude(*ResolvedDeclaring, *ResolvedInserted));
     };
     bool ShouldInsert = C.headerToInsertIfAllowed(Opts).hasValue();
index 8d20971..551dde1 100644 (file)
@@ -14,6 +14,7 @@
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Lex/HeaderSearch.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Path.h"
 
 namespace clang {
@@ -186,15 +187,18 @@ bool IncludeInserter::shouldInsertInclude(
 }
 
 std::string
-IncludeInserter::calculateIncludePath(const HeaderFile &InsertedHeader) const {
+IncludeInserter::calculateIncludePath(const HeaderFile &InsertedHeader,
+                                      llvm::StringRef IncludingFile) const {
   assert(InsertedHeader.valid());
   if (InsertedHeader.Verbatim)
     return InsertedHeader.File;
   bool IsSystem = false;
+  // FIXME(kadircet): Handle same directory includes even if there is no
+  // HeaderSearchInfo.
   if (!HeaderSearchInfo)
     return "\"" + InsertedHeader.File + "\"";
   std::string Suggested = HeaderSearchInfo->suggestPathToFileForDiagnostics(
-      InsertedHeader.File, BuildDir, &IsSystem);
+      InsertedHeader.File, BuildDir, IncludingFile, &IsSystem);
   if (IsSystem)
     Suggested = "<" + Suggested + ">";
   else
index f2eaf08..5a20ea2 100644 (file)
@@ -151,8 +151,12 @@ public:
   /// \param InsertedHeader The preferred header to be inserted. This could be
   /// the same as DeclaringHeader but must be provided.
   ///
+  /// \param IncludingFile is the absolute path of the file that InsertedHeader
+  /// will be inserted.
+  ///
   /// \return A quoted "path" or <path> to be included.
-  std::string calculateIncludePath(const HeaderFile &InsertedHeader) const;
+  std::string calculateIncludePath(const HeaderFile &InsertedHeader,
+                                   llvm::StringRef IncludingFile) const;
 
   /// Calculates an edit that inserts \p VerbatimHeader into code. If the header
   /// is already included, this returns None.
index f58ddba..5671ad2 100644 (file)
@@ -152,7 +152,7 @@ std::vector<Fix> IncludeFixer::fixesForSymbols(const SymbolSlab &Syms) const {
     if (!ResolvedInserted)
       return ResolvedInserted.takeError();
     return std::make_pair(
-        Inserter->calculateIncludePath(*ResolvedInserted),
+        Inserter->calculateIncludePath(*ResolvedInserted, File),
         Inserter->shouldInsertInclude(*ResolvedDeclaring, *ResolvedInserted));
   };
 
@@ -397,7 +397,6 @@ std::vector<Fix> IncludeFixer::fixUnresolvedName() const {
   return {};
 }
 
-
 llvm::Optional<const SymbolSlab *>
 IncludeFixer::fuzzyFindCached(const FuzzyFindRequest &Req) const {
   auto ReqStr = llvm::formatv("{0}", toJSON(Req)).str();
index 23fa052..9bccc8f 100644 (file)
@@ -762,11 +762,7 @@ TEST(CompletionTest, DynamicIndexIncludeInsertion) {
   // Wait for the dynamic index being built.
   ASSERT_TRUE(Server.blockUntilIdleForTest());
   EXPECT_THAT(completions(Server, "Foo^ foo;").Completions,
-              ElementsAre(AllOf(Named("Foo"),
-                                HasInclude('"' +
-                                           llvm::sys::path::convert_to_slash(
-                                               testPath("foo_header.h")) +
-                                           '"'),
+              ElementsAre(AllOf(Named("Foo"), HasInclude("\"foo_header.h\""),
                                 InsertInclude())));
 }
 
index a014d78..f42b235 100644 (file)
@@ -97,7 +97,7 @@ protected:
     auto Inserted = ToHeaderFile(Preferred);
     if (!Inserter.shouldInsertInclude(Original, Inserted))
       return "";
-    std::string Path = Inserter.calculateIncludePath(Inserted);
+    std::string Path = Inserter.calculateIncludePath(Inserted, MainFile);
     Action.EndSourceFile();
     return Path;
   }
@@ -180,13 +180,14 @@ TEST_F(HeadersTest, PreambleIncludesPresentOnce) {
   // includes. (We'd test more directly, but it's pretty well encapsulated!)
   auto TU = TestTU::withCode(R"cpp(
     #include "a.h"
+
     #include "a.h"
     void foo();
     #include "a.h"
   )cpp");
   TU.HeaderFilename = "a.h"; // suppress "not found".
   EXPECT_THAT(TU.build().getIncludeStructure().MainFileIncludes,
-              ElementsAre(IncludeLine(1), IncludeLine(2), IncludeLine(4)));
+              ElementsAre(IncludeLine(1), IncludeLine(3), IncludeLine(5)));
 }
 
 TEST_F(HeadersTest, UnResolvedInclusion) {
@@ -211,7 +212,7 @@ TEST_F(HeadersTest, DoNotInsertIfInSameFile) {
   EXPECT_EQ(calculate(MainFile), "");
 }
 
-TEST_F(HeadersTest, ShortenedInclude) {
+TEST_F(HeadersTest, ShortenIncludesInSearchPath) {
   std::string BarHeader = testPath("sub/bar.h");
   EXPECT_EQ(calculate(BarHeader), "\"bar.h\"");
 
@@ -221,10 +222,10 @@ TEST_F(HeadersTest, ShortenedInclude) {
   EXPECT_EQ(calculate(BarHeader), "\"sub/bar.h\"");
 }
 
-TEST_F(HeadersTest, NotShortenedInclude) {
+TEST_F(HeadersTest, ShortenedIncludeNotInSearchPath) {
   std::string BarHeader =
       llvm::sys::path::convert_to_slash(testPath("sub-2/bar.h"));
-  EXPECT_EQ(calculate(BarHeader, ""), "\"" + BarHeader + "\"");
+  EXPECT_EQ(calculate(BarHeader, ""), "\"sub-2/bar.h\"");
 }
 
 TEST_F(HeadersTest, PreferredHeader) {
@@ -267,10 +268,12 @@ TEST(Headers, NoHeaderSearchInfo) {
   auto Inserting = HeaderFile{HeaderPath, /*Verbatim=*/false};
   auto Verbatim = HeaderFile{"<x>", /*Verbatim=*/true};
 
-  EXPECT_EQ(Inserter.calculateIncludePath(Inserting), "\"" + HeaderPath + "\"");
+  // FIXME(kadircet): This should result in "sub/bar.h" instead of full path.
+  EXPECT_EQ(Inserter.calculateIncludePath(Inserting, MainFile),
+            '"' + HeaderPath + '"');
   EXPECT_EQ(Inserter.shouldInsertInclude(HeaderPath, Inserting), false);
 
-  EXPECT_EQ(Inserter.calculateIncludePath(Verbatim), "<x>");
+  EXPECT_EQ(Inserter.calculateIncludePath(Verbatim, MainFile), "<x>");
   EXPECT_EQ(Inserter.shouldInsertInclude(HeaderPath, Verbatim), true);
 }
 
index d660236..c5e6624 100644 (file)
@@ -709,21 +709,29 @@ public:
 
   /// Suggest a path by which the specified file could be found, for use in
   /// diagnostics to suggest a #include. Returned path will only contain forward
-  /// slashes as separators.
+  /// slashes as separators. MainFile is the absolute path of the file that we
+  /// are generating the diagnostics for. It will try to shorten the path using
+  /// MainFile location, if none of the include search directories were prefix
+  /// of File.
   ///
   /// \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,
+                                              llvm::StringRef MainFile,
                                               bool *IsSystem = nullptr);
 
   /// Suggest a path by which the specified file could be found, for use in
   /// diagnostics to suggest a #include. Returned path will only contain forward
-  /// slashes as separators.
+  /// slashes as separators. MainFile is the absolute path of the file that we
+  /// are generating the diagnostics for. It will try to shorten the path using
+  /// MainFile location, if none of the include search directories were prefix
+  /// of File.
   ///
   /// \param WorkingDir If non-empty, this will be prepended to search directory
   /// paths that are relative.
   std::string suggestPathToFileForDiagnostics(llvm::StringRef File,
                                               llvm::StringRef WorkingDir,
+                                              llvm::StringRef MainFile,
                                               bool *IsSystem = nullptr);
 
   void PrintStats();
index 3ad0e1e..ca94883 100644 (file)
@@ -1665,28 +1665,25 @@ void HeaderSearch::loadSubdirectoryModuleMaps(DirectoryLookup &SearchDir) {
   SearchDir.setSearchedAllModuleMaps(true);
 }
 
-std::string HeaderSearch::suggestPathToFileForDiagnostics(const FileEntry *File,
-                                                          bool *IsSystem) {
+std::string HeaderSearch::suggestPathToFileForDiagnostics(
+    const FileEntry *File, llvm::StringRef MainFile, 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).
-  return suggestPathToFileForDiagnostics(File->getName(), /*BuildDir=*/"",
-                                         IsSystem);
+  return suggestPathToFileForDiagnostics(File->getName(), /*WorkingDir=*/"",
+                                         MainFile, IsSystem);
 }
 
 std::string HeaderSearch::suggestPathToFileForDiagnostics(
-    llvm::StringRef File, llvm::StringRef WorkingDir, bool *IsSystem) {
+    llvm::StringRef File, llvm::StringRef WorkingDir, llvm::StringRef MainFile,
+    bool *IsSystem) {
   using namespace llvm::sys;
 
   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;
-
-    StringRef Dir = SearchDirs[I].getDir()->getName();
+  // Checks whether Dir and File shares a common prefix, if they do and that's
+  // the longest prefix we've seen so for it returns true and updates the
+  // BestPrefixLength accordingly.
+  auto CheckDir = [&](llvm::StringRef Dir) -> bool {
     llvm::SmallString<32> DirPath(Dir.begin(), Dir.end());
     if (!WorkingDir.empty() && !path::is_absolute(Dir))
       fs::make_absolute(WorkingDir, DirPath);
@@ -1710,7 +1707,7 @@ std::string HeaderSearch::suggestPathToFileForDiagnostics(
         unsigned PrefixLength = NI - path::begin(File);
         if (PrefixLength > BestPrefixLength) {
           BestPrefixLength = PrefixLength;
-          BestSearchDir = I;
+          return true;
         }
         break;
       }
@@ -1723,9 +1720,24 @@ std::string HeaderSearch::suggestPathToFileForDiagnostics(
       if (*NI != *DI)
         break;
     }
+    return false;
+  };
+
+  for (unsigned I = 0; I != SearchDirs.size(); ++I) {
+    // FIXME: Support this search within frameworks and header maps.
+    if (!SearchDirs[I].isNormalDir())
+      continue;
+
+    StringRef Dir = SearchDirs[I].getDir()->getName();
+    if (CheckDir(Dir) && IsSystem)
+      *IsSystem = BestPrefixLength ? I >= SystemDirIdx : false;
   }
 
-  if (IsSystem)
-    *IsSystem = BestPrefixLength ? BestSearchDir >= SystemDirIdx : false;
+  // Try to shorten include path using TUs directory, if we couldn't find any
+  // suitable prefix in include search paths.
+  if (!BestPrefixLength && CheckDir(path::parent_path(MainFile)) && IsSystem)
+    *IsSystem = false;
+
+
   return path::convert_to_slash(File.drop_front(BestPrefixLength));
 }
index 086f90f..c50592c 100644 (file)
@@ -21,6 +21,7 @@
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/Basic/Builtins.h"
+#include "clang/Basic/FileManager.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/ModuleLoader.h"
@@ -5195,10 +5196,11 @@ void Sema::diagnoseMissingImport(SourceLocation Loc, NamedDecl *Decl,
 /// 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);
+                                             const FileEntry *E,
+                                             llvm::StringRef IncludingFile) {
+  bool IsSystem = false;
+  auto Path = PP.getHeaderSearchInfo().suggestPathToFileForDiagnostics(
+      E, IncludingFile, &IsSystem);
   return (IsSystem ? '<' : '"') + Path + (IsSystem ? '>' : '"');
 }
 
@@ -5240,6 +5242,11 @@ void Sema::diagnoseMissingImport(SourceLocation UseLoc, NamedDecl *Decl,
       UniqueModules.push_back(M);
   }
 
+  llvm::StringRef IncludingFile;
+  if (const FileEntry *FE =
+          SourceMgr.getFileEntryForID(SourceMgr.getFileID(UseLoc)))
+    IncludingFile = FE->tryGetRealPathName();
+
   if (UniqueModules.empty()) {
     // All candidates were global module fragments. Try to suggest a #include.
     const FileEntry *E =
@@ -5248,7 +5255,7 @@ void Sema::diagnoseMissingImport(SourceLocation UseLoc, NamedDecl *Decl,
     // a FixItHint there.
     Diag(UseLoc, diag::err_module_unimported_use_global_module_fragment)
         << (int)MIK << Decl << !!E
-        << (E ? getIncludeStringForHeader(PP, E) : "");
+        << (E ? getIncludeStringForHeader(PP, E, IncludingFile) : "");
     // Produce a "previous" note if it will point to a header rather than some
     // random global module fragment.
     // FIXME: Suppress the note backtrace even under
@@ -5284,8 +5291,8 @@ void Sema::diagnoseMissingImport(SourceLocation UseLoc, NamedDecl *Decl,
     // 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);
+        << (int)MIK << Decl << Modules[0]->getFullModuleName()
+        << getIncludeStringForHeader(PP, E, IncludingFile);
   } else {
     // FIXME: Add a FixItHint that imports the corresponding module.
     Diag(UseLoc, diag::err_module_unimported_use)
index 499acec..626cfad 100644 (file)
@@ -59,35 +59,41 @@ protected:
 
 TEST_F(HeaderSearchTest, NoSearchDir) {
   EXPECT_EQ(Search.search_dir_size(), 0u);
-  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/x/y/z", /*WorkingDir=*/""),
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/x/y/z", /*WorkingDir=*/"",
+                                                   /*MainFile=*/""),
             "/x/y/z");
 }
 
 TEST_F(HeaderSearchTest, SimpleShorten) {
   addSearchDir("/x");
   addSearchDir("/x/y");
-  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/x/y/z", /*WorkingDir=*/""),
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/x/y/z", /*WorkingDir=*/"",
+                                                   /*MainFile=*/""),
             "z");
   addSearchDir("/a/b/");
-  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/a/b/c", /*WorkingDir=*/""),
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/a/b/c", /*WorkingDir=*/"",
+                                                   /*MainFile=*/""),
             "c");
 }
 
 TEST_F(HeaderSearchTest, ShortenWithWorkingDir) {
   addSearchDir("x/y");
   EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/a/b/c/x/y/z",
-                                                   /*WorkingDir=*/"/a/b/c"),
+                                                   /*WorkingDir=*/"/a/b/c",
+                                                   /*MainFile=*/""),
             "z");
 }
 
 TEST_F(HeaderSearchTest, Dots) {
   addSearchDir("/x/./y/");
   EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/x/y/./z",
-                                                   /*WorkingDir=*/""),
+                                                   /*WorkingDir=*/"",
+                                                   /*MainFile=*/""),
             "z");
   addSearchDir("a/.././c/");
   EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/m/n/./c/z",
-                                                   /*WorkingDir=*/"/m/n/"),
+                                                   /*WorkingDir=*/"/m/n/",
+                                                   /*MainFile=*/""),
             "z");
 }
 
@@ -95,14 +101,16 @@ TEST_F(HeaderSearchTest, Dots) {
 TEST_F(HeaderSearchTest, BackSlash) {
   addSearchDir("C:\\x\\y\\");
   EXPECT_EQ(Search.suggestPathToFileForDiagnostics("C:\\x\\y\\z\\t",
-                                                   /*WorkingDir=*/""),
+                                                   /*WorkingDir=*/"",
+                                                   /*MainFile=*/""),
             "z/t");
 }
 
 TEST_F(HeaderSearchTest, BackSlashWithDotDot) {
   addSearchDir("..\\y");
   EXPECT_EQ(Search.suggestPathToFileForDiagnostics("C:\\x\\y\\z\\t",
-                                                   /*WorkingDir=*/"C:/x/y/"),
+                                                   /*WorkingDir=*/"C:/x/y/",
+                                                   /*MainFile=*/""),
             "z/t");
 }
 #endif
@@ -110,9 +118,23 @@ TEST_F(HeaderSearchTest, BackSlashWithDotDot) {
 TEST_F(HeaderSearchTest, DotDotsWithAbsPath) {
   addSearchDir("/x/../y/");
   EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/y/z",
-                                                   /*WorkingDir=*/""),
+                                                   /*WorkingDir=*/"",
+                                                   /*MainFile=*/""),
             "z");
 }
 
+TEST_F(HeaderSearchTest, IncludeFromSameDirectory) {
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/y/z/t.h",
+                                                   /*WorkingDir=*/"",
+                                                   /*MainFile=*/"/y/a.cc"),
+            "z/t.h");
+
+  addSearchDir("/");
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/y/z/t.h",
+                                                   /*WorkingDir=*/"",
+                                                   /*MainFile=*/"/y/a.cc"),
+            "y/z/t.h");
+}
+
 } // namespace
 } // namespace clang