[clangd] Don't insert absolute paths, give up instead.

[sammccall]

Summary

Also implement resolution of paths relative to mainfile without HeaderSearchInfo.

[kadircet]

clang-tools-extra/clangd/Headers.cpp

206 ↗

(On Diff #208284)

why not directly replace with ""?

clang-tools-extra/clangd/unittests/HeadersTests.cpp

100 ↗

(On Diff #208284)

can you also log the error ?

[sammccall]

clang-tools-extra/clangd/unittests/HeadersTests.cpp

100 ↗

(On Diff #208284)

I'm a bit confused about this - why do we need to log it in a unit test?

[sammccall]

clang-tools-extra/clangd/Headers.cpp

206 ↗

(On Diff #208284)

Because replace_path_prefix("/foo/bar.c", "/foo", "") yields "/bar.c" rather than "bar.c". Added a comment.

clang-tools-extra/trunk/clangd/CodeComplete.cpp

auto Inserted = [&](llvm::StringRef Header)
       if (!DeclaringURI)
         return DeclaringURI.takeError();
       auto ResolvedDeclaring = URI::resolve(*DeclaringURI, FileName);
       if (!ResolvedDeclaring)
         return ResolvedDeclaring.takeError();
       auto ResolvedInserted = toHeaderFile(Header, FileName);
       if (!ResolvedInserted)
         return ResolvedInserted.takeError();
+      auto Spelled = Includes.calculateIncludePath(*ResolvedInserted, FileName);
+      if (!Spelled)
+        return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                       "Header not on include path");
       return std::make_pair(
-          Includes.calculateIncludePath(*ResolvedInserted, FileName),
+          std::move(*Spelled),
           Includes.shouldInsertInclude(*ResolvedDeclaring, *ResolvedInserted));
     };
     bool ShouldInsert = C.headerToInsertIfAllowed(Opts).hasValue();
     // Calculate include paths and edits for all possible headers.
     for (const auto &Inc : C.RankedIncludeHeaders) {
       if (auto ToInclude = Inserted(Inc)) {
         CodeCompletion::IncludeCandidate Include;
         Include.Header = ToInclude->first;

clang-tools-extra/trunk/clangd/Headers.h

public:
   /// the same as DeclaringHeader but must be provided.
   bool shouldInsertInclude(PathRef DeclaringHeader,
                            const HeaderFile &InsertedHeader) const;
 
   /// Determines the preferred way to #include a file, taking into account the
   /// search path. Usually this will prefer a shorter representation like
   /// 'Foo/Bar.h' over a longer one like 'Baz/include/Foo/Bar.h'.
   ///
-  /// \param InsertedHeader The preferred header to be inserted. This could be
-  /// the same as DeclaringHeader but must be provided.
+  /// \param InsertedHeader The preferred header to be inserted.
   ///
   /// \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,
-                                   llvm::StringRef IncludingFile) const;
+  /// \return A quoted "path" or <path> to be included, or None if it couldn't
+  /// be shortened.
+  llvm::Optional<std::string>
+  calculateIncludePath(const HeaderFile &InsertedHeader,
 
   /// Calculates an edit that inserts \p VerbatimHeader into code. If the header
   /// is already included, this returns None.
   llvm::Optional<TextEdit> insert(llvm::StringRef VerbatimHeader) const;
 
 private:
   StringRef FileName;
   StringRef Code;

clang-tools-extra/trunk/clangd/Headers.cpp

bool IncludeInserter::shouldInsertInclude(
   if (FileName == DeclaringHeader || FileName == InsertedHeader.File)
     return false;
   auto Included = [&](llvm::StringRef Header) {
     return IncludedHeaders.find(Header) != IncludedHeaders.end();
   };
   return !Included(DeclaringHeader) && !Included(InsertedHeader.File);
 }
 
-std::string
+llvm::Optional<std::string>
 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(
+  std::string Suggested;
+  if (HeaderSearchInfo) {
+    Suggested = HeaderSearchInfo->suggestPathToFileForDiagnostics(
         InsertedHeader.File, BuildDir, IncludingFile, &IsSystem);
+  } else {
+    // Calculate include relative to including file only.
+    StringRef IncludingDir = llvm::sys::path::parent_path(IncludingFile);
+    SmallString<256> RelFile(InsertedHeader.File);
+    // Replacing with "" leaves "/RelFile" if IncludingDir doesn't end in "/".
+    llvm::sys::path::replace_path_prefix(RelFile, IncludingDir, "./");
+    Suggested = llvm::sys::path::convert_to_slash(
+        llvm::sys::path::remove_leading_dotslash(RelFile));
+  }
+  // FIXME: should we allow (some limited number of) "../header.h"?
+  if (llvm::sys::path::is_absolute(Suggested))
+    return None;
   if (IsSystem)
     Suggested = "<" + Suggested + ">";
   else
     Suggested = "\"" + Suggested + "\"";
   return Suggested;
 }
 
 llvm::Optional<TextEdit>

clang-tools-extra/trunk/clangd/IncludeFixer.cpp

auto Inserted = [&](const Symbol &Sym, llvm::StringRef Header)
     if (!DeclaringURI)
       return DeclaringURI.takeError();
     auto ResolvedDeclaring = URI::resolve(*DeclaringURI, File);
     if (!ResolvedDeclaring)
       return ResolvedDeclaring.takeError();
     auto ResolvedInserted = toHeaderFile(Header, File);
     if (!ResolvedInserted)
       return ResolvedInserted.takeError();
+    auto Spelled = Inserter->calculateIncludePath(*ResolvedInserted, File);
+    if (!Spelled)
+      return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                     "Header not on include path");
     return std::make_pair(
-        Inserter->calculateIncludePath(*ResolvedInserted, File),
+        std::move(*Spelled),
         Inserter->shouldInsertInclude(*ResolvedDeclaring, *ResolvedInserted));
   };
 
   std::vector<Fix> Fixes;
   // Deduplicate fixes by include headers. This doesn't distiguish symbols in
   // different scopes from the same header, but this case should be rare and is
   // thus ignored.
   llvm::StringSet<> InsertedHeaders;

clang-tools-extra/trunk/clangd/unittests/HeadersTests.cpp

std::string calculate(PathRef Original, PathRef Preferred = "",
     IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
                              CDB.getCompileCommand(MainFile)->Directory,
                              &Clang->getPreprocessor().getHeaderSearchInfo());
     for (const auto &Inc : Inclusions)
       Inserter.addExisting(Inc);
     auto Inserted = ToHeaderFile(Preferred);
     if (!Inserter.shouldInsertInclude(Original, Inserted))
       return "";
-    std::string Path = Inserter.calculateIncludePath(Inserted, MainFile);
+    auto Path = Inserter.calculateIncludePath(Inserted, MainFile);
     Action.EndSourceFile();
-    return Path;
+    return Path.getValueOr("");
   }
 
   llvm::Optional<TextEdit> insert(llvm::StringRef VerbatimHeader) {
     auto Clang = setupClang();
     PreprocessOnlyAction Action;
     EXPECT_TRUE(
         Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]));
 
TEST_F(HeadersTest, InsertInclude) {
   EXPECT_EQ(calculate(Path), "\"bar.h\"");
 }
 
 TEST_F(HeadersTest, DoNotInsertIfInSameFile) {
   MainFile = testPath("main.h");
   EXPECT_EQ(calculate(MainFile), "");
 }
 
+TEST_F(HeadersTest, DoNotInsertOffIncludePath) {
+  MainFile = testPath("sub/main.cpp");
+  EXPECT_EQ(calculate(testPath("sub2/main.cpp")), "");
+}
+
 TEST_F(HeadersTest, ShortenIncludesInSearchPath) {
   std::string BarHeader = testPath("sub/bar.h");
   EXPECT_EQ(calculate(BarHeader), "\"bar.h\"");
 
   SearchDirArg = (llvm::Twine("-I") + Subdir + "/..").str();
   CDB.ExtraClangFlags = {SearchDirArg.c_str()};
   BarHeader = testPath("sub/bar.h");
   EXPECT_EQ(calculate(BarHeader), "\"sub/bar.h\"");
TEST(Headers, NoHeaderSearchInfo) {
   std::string MainFile = testPath("main.cpp");
   IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
                            /*BuildDir=*/"", /*HeaderSearchInfo=*/nullptr);
 
   auto HeaderPath = testPath("sub/bar.h");
   auto Inserting = HeaderFile{HeaderPath, /*Verbatim=*/false};
   auto Verbatim = HeaderFile{"<x>", /*Verbatim=*/true};
 
-  // FIXME(kadircet): This should result in "sub/bar.h" instead of full path.
   EXPECT_EQ(Inserter.calculateIncludePath(Inserting, MainFile),
-            '"' + HeaderPath + '"');
+            std::string("\"sub/bar.h\""));
   EXPECT_EQ(Inserter.shouldInsertInclude(HeaderPath, Inserting), false);
 
-  EXPECT_EQ(Inserter.calculateIncludePath(Verbatim, MainFile), "<x>");
+  EXPECT_EQ(Inserter.calculateIncludePath(Verbatim, MainFile),
+            std::string("<x>"));
   EXPECT_EQ(Inserter.shouldInsertInclude(HeaderPath, Verbatim), true);
+
+  EXPECT_EQ(Inserter.calculateIncludePath(Inserting, "sub2/main2.cpp"),
+            llvm::None);
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang