[clangd] Fixes for #include insertion.

[ioeric]

Summary

o Avoid inserting a header include into the header itself.
o Avoid inserting non-header files.
o Canonicalize include paths for symbols in dynamic index.

[ilya-biryukov]

clangd/Headers.cpp

45

There is another list of header extensions in ClangdServer::switchSourceHeader, let's reuse the list of extensions used here and there.

48

Maybe use llvm::sys::path::extension and search it in HeaderExtensions using StringRef::equals_lower?
Will also handle upper- and mixed-case.

60

Maybe replace !hasHeaderExtension(Header) to hasSourceExtension(Header)?
Knowing about all header extensions in advance is hard, knowing a subset of source extensions seems totally true.

clangd/index/FileIndex.cpp

18

Maybe store a static inside the function body (to properly free memory) and return a const reference (to avoid mutating shared data)?

const CanonicalIncludes &CanonicalIncludesForSystemHeaders() {
  static CanonicalInclude Includes = []() -> CanonicalHeader {
    CanonicalInclude Includes;
    addSystemHeadersMapping(Includes);
    return Includes;
  };
  return Includes;
}

Also Sam mentioned that CanonicalIncludes is not thread-safe, as the Regex's match() mutates.

unittests/clangd/FileIndexTests.cpp

173

Why not on Windows?

[sammccall]

clangd/Headers.cpp

42–43

As discussed offline, this seems dubious.

• this is probably the generalization of '.inc' handling, and so probably wants the include stack and belongs at indexing time (we're determining "what's the public header for this symbol, or is the symbol private")?
  • as discussed offline, it seems like we can sidestep this whole problem by not indexing symbols only visible in main files, for now

(later we'll want a flag on the symbol for whether it's public, so we can support find-symbol-by-name for mainfile-declared symbols)

[ioeric]

• Stop indexing main files in dynamic index; addressed review comments.

clangd/Headers.cpp

45

Acked. (The change is not relevant anymore.)

clangd/index/FileIndex.cpp

18

As discussed offline, we want to avoid destructing static objects because ...

Made CanonicalIncludes thread-safe by adding a mutex around matches.

unittests/clangd/FileIndexTests.cpp

173

because our system header map doesn't really support windows...

[sammccall]

clangd/Headers.h

26–27

File also needs to be absolute.
(May want to add asserts for this at the start of the impl - up to you)

clangd/index/FileIndex.cpp

35

if this is always true now, we could remove the option?

36

This is not going to handle IWYU right?
It seems like at this point the right thing to do is totally hide the CanonicalIncludes inside SymbolCollector, which would init it (to system headers), write IWYU mappings using the PP, and consume the mappings (when emitting symbols).
Churn :(

[ilya-biryukov]

clangd/Headers.cpp

68

Not related to this patch, but just noticed that we don't call FS->setCurrentWorkingDirectory(CompileCommand.Directory) here.
This might break if CompileCommand has non-absolute paths.

clangd/index/CanonicalIncludes.cpp

21 ↗

(On Diff #134932)

Technically, if one thread initializes CanonicalIncludes and the other thread reads from it, this is a data race.
Maybe we're ok with our current usage, but I don't have enough confidence in that (i.e. don't know C++'s memory model good enough) and would advice to take the lock in other functions of CanonicalIncludes too.

clangd/index/FileIndex.cpp

18

NIT: lowerCase the function name.

37

NIT: remove local var, replace with CollectorOpts.Includes = CanonicalIncludesForSystemHeaders()?

unittests/clangd/FileIndexTests.cpp

173

Thanks for clarification.
I thought it's standard-library specific, not OS-specific. I would have expected it to work on Windows with libstdc++. (which we're "mocking" here).
Anyway, this not related to this patch.

[ilya-biryukov]

clangd/index/CanonicalIncludes.cpp

21 ↗

(On Diff #134932)

As Sam pointed out, it's fine the way it is right now. We don't want any thread-safety guarantees from the non-const methods of the class.
But could you please add a comment to the class that non-const methods of it are not thread-safe even though it has a mutex inside?

[ioeric]

clangd/Headers.cpp

68

This was done in ClangdServer, but we should probably do it here as you suggested.

clangd/index/FileIndex.cpp

35

Will do this in a followup patch. Added a FIXME

36

You are right :(

I'll prepare a followup patch to handle this. Added a FIXME.

[ioeric]

• addressed comments.

[ioeric]

• added a comment about thread safety

[ioeric]

• assert in the very beginning!

[ilya-biryukov]

LGTM

clangd/ClangdServer.cpp

if (!Resolved)
       return Resolved.takeError();
 
     auto FS = FSProvider.getTaggedFileSystem(File).Value;
     tooling::CompileCommand CompileCommand =
         CompileArgs.getCompileCommand(File);
     FS->setCurrentWorkingDirectory(CompileCommand.Directory);
 
     auto Include =
-        shortenIncludePath(File, Code, *Resolved, CompileCommand, FS);
+        calculateIncludePath(File, Code, *Resolved, CompileCommand, FS);
     if (!Include)
       return Include.takeError();
     if (Include->empty())
       return tooling::Replacements();
     ToInclude = std::move(*Include);
   }
 
   auto Style = format::getStyle("file", File, "llvm");

clangd/Headers.h

 #include "Path.h"
 #include "clang/Basic/VirtualFileSystem.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 
 namespace clang {
 namespace clangd {
+
 /// 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 Header is an absolute file path.
-/// \return A quoted "path" or <path>. If \p Header is already (directly)
-/// included in the file (including those included via different paths), this
-/// returns an empty string.
+/// \return A quoted "path" or <path>. This returns an empty string if:
+///   - \p Header is already (directly) included in the file (including those
+///   included via different paths).
+///   - \p Header is the same as \p File.
+///   - \p Header does not ends with a proper extenstion for a header file e.g.
+///   ".h".
 llvm::Expected<std::string>
-shortenIncludePath(PathRef File, llvm::StringRef Code, llvm::StringRef Header,
+calculateIncludePath(PathRef File, llvm::StringRef Code, llvm::StringRef Header,
                      const tooling::CompileCommand &CompileCommand,
                      IntrusiveRefCntPtr<vfs::FileSystem> FS);
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_HEADERS_H

clangd/Headers.cpp

void InclusionDirective(SourceLocation /*HashLoc*/,
                           const Module * /*Imported*/) override {
     if (File != nullptr && !File->tryGetRealPathName().empty())
       Headers.insert(File->tryGetRealPathName());
   }
 
 private:
   std::set<std::string> &Headers;
 };
 
+/// Returns true if \p Path has header extensions like .h and .hpp etc.
+bool hasHeaderExtension(PathRef Path) {
+  constexpr static const char *HeaderExtensions[] = {".h", ".hpp", ".hh",
+                                                     ".hxx"};
+  return std::any_of(std::begin(HeaderExtensions), std::end(HeaderExtensions),
+                     [&Path](const char *Ext) { return Path.endswith(Ext); });
+}
+
 } // namespace
 
 /// FIXME(ioeric): we might not want to insert an absolute include path if the
 /// path is not shortened.
 llvm::Expected<std::string>
-shortenIncludePath(llvm::StringRef File, llvm::StringRef Code,
+calculateIncludePath(llvm::StringRef File, llvm::StringRef Code,
                      llvm::StringRef Header,
                      const tooling::CompileCommand &CompileCommand,
                      IntrusiveRefCntPtr<vfs::FileSystem> FS) {
+  if (File == Header || !hasHeaderExtension(Header))
+    return "";
   // Set up a CompilerInstance and create a preprocessor to collect existing
   // #include headers in \p Code. Preprocesor also provides HeaderSearch with
   // which we can calculate the shortest include path for \p Header.
   std::vector<const char *> Argv;
   for (const auto &S : CompileCommand.CommandLine)
     Argv.push_back(S.c_str());
   IgnoringDiagConsumer IgnoreDiags;
   auto CI = clang::createInvocationFromCommandLine(
       Argv,
       CompilerInstance::createDiagnostics(new DiagnosticOptions(), &IgnoreDiags,
                                           false),
       FS);
   if (!CI)
     return llvm::make_error<llvm::StringError>(
         "Failed to create a compiler instance for " + File,

clangd/index/FileIndex.cpp

 #include "FileIndex.h"
 #include "SymbolCollector.h"
 #include "clang/Index/IndexingAction.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
+CanonicalIncludes *CanonicalIncludesForSystemHeaders() {
+  auto *Includes = new CanonicalIncludes();
+  addSystemHeadersMapping(Includes);
+  return Includes;
+}
+
 /// Retrieves namespace and class level symbols in \p Decls.
 std::unique_ptr<SymbolSlab> indexAST(ASTContext &Ctx,
                                      std::shared_ptr<Preprocessor> PP,
                                      llvm::ArrayRef<const Decl *> Decls) {
   SymbolCollector::Options CollectorOpts;
   // Code completion gets main-file results from Sema.
   // But we leave this option on because features like go-to-definition want it.
   CollectorOpts.IndexMainFiles = true;
+  CollectorOpts.CollectIncludePath = true;
+  static auto *Includes = CanonicalIncludesForSystemHeaders();
+  CollectorOpts.Includes = Includes;
+
   auto Collector = std::make_shared<SymbolCollector>(std::move(CollectorOpts));
   Collector->setPreprocessor(std::move(PP));
   index::IndexingOptions IndexOpts;
   IndexOpts.SystemSymbolFilter =
       index::IndexingOptions::SystemSymbolFilterKind::All;
   IndexOpts.IndexFunctionLocals = false;
 
   index::indexTopLevelDecls(Ctx, Decls, Collector, IndexOpts);
   auto Symbols = llvm::make_unique<SymbolSlab>();
   *Symbols = Collector->takeSymbols();

unittests/clangd/FileIndexTests.cpp

 //===-- FileIndexTests.cpp  ---------------------------*- C++ -*-----------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
+#include "TestFS.h"
 #include "index/FileIndex.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
 #include "clang/Frontend/Utils.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 using testing::UnorderedElementsAre;
M.update("f1",
            build("f1", "class X { static int m1; int m2; static void f(); };")
                .getPointer());
 
   FuzzyFindRequest Req;
   Req.Query = "";
   EXPECT_THAT(match(M, Req), UnorderedElementsAre("X"));
 }
 
+#ifndef LLVM_ON_WIN32
+TEST(FileIndexTest, CanonicalizeSystemHeader) {
+  FileIndex M;
+  std::string File = testPath("bits/basic_string.h");
+  M.update(File, build(File, "class string {};").getPointer());
+
+  FuzzyFindRequest Req;
+  Req.Query = "";
+  M.fuzzyFind(Req, [&](const Symbol &Sym) {
+    EXPECT_EQ(Sym.Detail->IncludeHeader, "<string>");
+  });
+}
+#endif
+
 } // namespace
 } // namespace clangd
 } // namespace clang

unittests/clangd/HeadersTests.cpp

 class HeadersTest : public ::testing::Test {
 public:
   HeadersTest() {
     CDB.ExtraClangFlags = {SearchDirArg.c_str()};
     FS.Files[MainFile] = "";
   }
 
 protected:
-  // Adds \p File with content \p Code to the sub directory and returns the
-  // absolute path.
-//  std::string addToSubdir(PathRef File, llvm::StringRef Code = "") {
-//    assert(llvm::sys::path::is_relative(File) && "FileName should be relative");
-//    llvm::SmallString<32> Path;
-//    llvm::sys::path::append(Path, Subdir, File);
-//    FS.Files[Path] = Code;
-//    return Path.str();
-//  };
-
-  // Shortens the header and returns "" on error.
-  std::string shorten(PathRef Header) {
+  // Calculates the include path for \p Header, or returns "" on error.
+  std::string calculate(PathRef Header) {
     auto VFS = FS.getTaggedFileSystem(MainFile).Value;
     auto Cmd = CDB.getCompileCommand(MainFile);
     assert(static_cast<bool>(Cmd));
     VFS->setCurrentWorkingDirectory(Cmd->Directory);
     auto Path =
-        shortenIncludePath(MainFile, FS.Files[MainFile], Header, *Cmd, VFS);
+        calculateIncludePath(MainFile, FS.Files[MainFile], Header, *Cmd, VFS);
     if (!Path) {
       llvm::consumeError(Path.takeError());
       return std::string();
     }
     return std::move(*Path);
   }
   MockFSProvider FS;
   MockCompilationDatabase CDB;
   std::string MainFile = testPath("main.cpp");
   std::string Subdir = testPath("sub");
   std::string SearchDirArg = (llvm::Twine("-I") + Subdir).str();
 };
 
 TEST_F(HeadersTest, InsertInclude) {
   std::string Path = testPath("sub/bar.h");
   FS.Files[Path] = "";
-  EXPECT_EQ(shorten(Path), "\"bar.h\"");
+  EXPECT_EQ(calculate(Path), "\"bar.h\"");
 }
 
 TEST_F(HeadersTest, DontInsertDuplicateSameName) {
   FS.Files[MainFile] = R"cpp(
 #include "bar.h"
 )cpp";
   std::string BarHeader = testPath("sub/bar.h");
   FS.Files[BarHeader] = "";
-  EXPECT_EQ(shorten(BarHeader), "");
+  EXPECT_EQ(calculate(BarHeader), "");
 }
 
 TEST_F(HeadersTest, DontInsertDuplicateDifferentName) {
   std::string BarHeader = testPath("sub/bar.h");
   FS.Files[BarHeader] = "";
   FS.Files[MainFile] = R"cpp(
 #include "sub/bar.h"  // not shortest
 )cpp";
-  EXPECT_EQ(shorten(BarHeader), "");
+  EXPECT_EQ(calculate(BarHeader), "");
 }
 
 TEST_F(HeadersTest, StillInsertIfTrasitivelyIncluded) {
   std::string BazHeader = testPath("sub/baz.h");
   FS.Files[BazHeader] = "";
   std::string BarHeader = testPath("sub/bar.h");
   FS.Files[BarHeader] = R"cpp(
 #include "baz.h"
 )cpp";
   FS.Files[MainFile] = R"cpp(
 #include "bar.h"
 )cpp";
-  EXPECT_EQ(shorten(BazHeader), "\"baz.h\"");
+  EXPECT_EQ(calculate(BazHeader), "\"baz.h\"");
+}
+
+TEST_F(HeadersTest, DoNotInsertIfInSameFile) {
+  MainFile = testPath("main.h");
+  FS.Files[MainFile] = "";
+  EXPECT_EQ(calculate(MainFile), "");
+}
+
+TEST_F(HeadersTest, DoNotInsertCppFile) {
+  std::string CppFile = testPath("sub/x.cpp");
+  FS.Files[CppFile] = "";
+  EXPECT_EQ(calculate(CppFile), "");
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang