[clangd] Not collect include headers for dynamic index for now.

[ioeric]

Summary

The new behaviors introduced by this patch:
o When include collection is enabled, we always set IncludeHeader field in Symbol
even if it's the same as FileURI in decl.
o Disable include collection in FileIndex which is currently only used to build
dynamic index. We should revisit when we actually want to use FileIndex to global
index.
o Code-completion only uses IncludeHeader to insert headers but not FileURI in
CanonicalDeclaration. This ensures that inserted headers are always canonicalized.
Note that include insertion can still be triggered for symbols that are already
included if they are merged from dynamic index and static index, but we would
only use includes that are already canonicalized (e.g. from static index).

Reason for change:
Collecting header includes in dynamic index enables inserting includes for headers
that are not indexed but opened in the editor. Comparing to inserting includes for
symbols in global/static index, this is nice-to-have but would probably require
non-trivial amount of work to get right. For example:
o Currently it's not easy to fully support CanonicalIncludes in dynamic index, given the way
we run dynamic index.
o It's also harder to reason about the correctness of include canonicalization for dynamic index
(i.e. symbols in the current file/TU) than static index where symbols are collected
offline and sanity check is possible before shipping to production.
o We have less control/flexibility over symbol info in the dynamic index
(e.g. URIs, path normalization), which could be used to help make decision when inserting includes.

As header collection (especially canonicalization) is relatively new, and enabling
it for dynamic index would immediately affect current users with only dynamic
index support, I propose we disable it for dynamic index for now to avoid
compromising other hot features like code completion and only support it for
static index where include insertion would likely to bring more value.

[ilya-biryukov]

Is there a way to still enable include insertion but in a restricted manner, e.g. only for the current project?
File of canonical declaration is usually a pretty good guess and it would be nice to have it. Maybe we could allowing to disable the include insertion via a flag or config parameter rather than disabling it for dynamic index entirely.

It bugs me that internal STL headers are added all the time, but it also lets me test the feature without building the static index. I find the second more valuable, given that we're still actively working on it.

clangd/index/FileIndex.cpp

18

Should we also remove the mutex from CanonicalIncludes now?

clangd/index/SymbolCollector.cpp

172

Maybe simplify to return toURI(...)?
Or is there a reason why it doesn't work?

[ioeric]

In D43550#1014319, @ilya-biryukov wrote:

Is there a way to still enable include insertion but in a restricted manner, e.g. only for the current project?

I'm not sure if this is currently supported, as we don't have a good definition of a "project" yet. Could you provide a use case?

File of canonical declaration is usually a pretty good guess and it would be nice to have it. Maybe we could allowing to disable the include insertion via a flag or config parameter rather than disabling it for dynamic index entirely.

Yes, it is a nice-to-have, when it works :) Without proper canonicalization, this could easily go wrong (e.g. we don't want to insert gtest/gmock internal headers guarded by IWYU pragma).

Do you think it's beneficial to collect headers in dynamic index even when it doesn't work correctly? Again, I'm not arguing that collecting includes in dynamic index is not worth doing; I just think it's a rabbit hole that we don't need to chase down at this point :)

It bugs me that internal STL headers are added all the time, but it also lets me test the feature without building the static index. I find the second more valuable, given that we're still actively working on it.

I think we can test the feature in a safer approach (e.g. build a small static index offline) while not bugging normal users with the wrong includes, which IMO would hurt code completion experience very badly... In general, I think user experience is much more valuable than us being able to test features.

[ilya-biryukov]

As discussed offline, properly supporting IWYU pragma in the dynamic index seems like too much design work for now and it's not gonna get fixed soon.
So opting for disabling the feature seems like the right approach.

Therefore, LGTM. (There are a few nits in inline comments from my previous reply that you might want to look at before landing this)

[ioeric]

Properly use toURI.

clangd/index/FileIndex.cpp

18

I am inclined to keeping it as it brings thread-safety to CanonicalIncludes (which we wouldn't need to worry about in the future), and the current users are not in performance critical code path anyway.

clangd/index/SymbolCollector.cpp

172

No reason... #this is what happened when copy-paste code...

clangd/CodeComplete.cpp

if (IndexResult) {
                            ? IndexResult->CompletionSnippetInsertText
                            : IndexResult->CompletionPlainInsertText;
 
       if (auto *D = IndexResult->Detail) {
         if (I.documentation.empty())
           I.documentation = D->Documentation;
         if (I.detail.empty())
           I.detail = D->CompletionDetail;
-        // We only insert #include for items with details, since we can't tell
-        // whether the file URI of the canonical declaration would be the
-        // canonical #include without checking IncludeHeader in the detail.
         // FIXME: delay creating include insertion command to
         // "completionItem/resolve", when it is supported
-        if (!D->IncludeHeader.empty() ||
-            !IndexResult->CanonicalDeclaration.FileURI.empty()) {
+        if (!D->IncludeHeader.empty()) {
           // LSP favors additionalTextEdits over command. But we are still using
           // command here because it would be expensive to calculate #include
           // insertion edits for all candidates, and the include insertion edit
           // is unlikely to conflict with the code completion edits.
           Command Cmd;
           // Command title is not added since this is not a user-facing command.
           Cmd.command = ExecuteCommandParams::CLANGD_INSERT_HEADER_INCLUDE;
           IncludeInsertion Insertion;
-          Insertion.header = D->IncludeHeader.empty()
-                                 ? IndexResult->CanonicalDeclaration.FileURI
-                                 : D->IncludeHeader;
+          Insertion.header = D->IncludeHeader;
           Insertion.textDocument.uri = URIForFile(FileName);
           Cmd.includeInsertion = std::move(Insertion);
           I.command = std::move(Cmd);
         }
       }
     }
     I.scoreInfo = Scores;
     I.sortText = sortText(Scores.finalScore, Name);

clangd/index/FileIndex.cpp

 #include "FileIndex.h"
 #include "SymbolCollector.h"
 #include "clang/Index/IndexingAction.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
-const CanonicalIncludes *canonicalIncludesForSystemHeaders() {
-  static const auto *Includes = [] {
-    auto *I = new CanonicalIncludes();
-    addSystemHeadersMapping(I);
-    return I;
-  }();
-  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;
   // Although we do not index symbols in main files (e.g. cpp file), information
   // in main files like definition locations of class declarations will still be
   // collected; thus, the index works for go-to-definition.
-  // FIXME(ioeric): handle IWYU pragma for dynamic index. We might want to make
-  // SymbolCollector always provides include canonicalization (e.g. IWYU, STL).
   // FIXME(ioeric): get rid of `IndexMainFiles` as this is always set to false.
   CollectorOpts.IndexMainFiles = false;
-  CollectorOpts.CollectIncludePath = true;
-  CollectorOpts.Includes = canonicalIncludesForSystemHeaders();
+  // FIXME(ioeric): we might also want to collect include headers. We would need
+  // to make sure all includes are canonicalized (with CanonicalIncludes), which
+  // is not trivial given the current way of collecting symbols: we only have
+  // AST at this point, but we also need preprocessor callbacks (e.g.
+  // CommentHandler for IWYU pragma) to canonicalize includes.
+  CollectorOpts.CollectIncludePath = false;
 
   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;
 

clangd/index/Index.h

struct Details {
     /// This is what goes into the LSP detail field in a completion item. For
     /// example, the result type of a function.
     llvm::StringRef CompletionDetail;
     /// This can be either a URI of the header to be #include'd for this symbol,
     /// or a literal header quoted with <> or "" that is suitable to be included
     /// directly. When this is a URI, the exact #include path needs to be
     /// calculated according to the URI scheme.
     ///
-    /// If empty, FileURI in CanonicalDeclaration should be used to calculate
-    /// the #include path.
+    /// This is a canonical include for the symbol and can be different from
+    /// FileURI in the CanonicalDeclaration.
     llvm::StringRef IncludeHeader;
   };
 
   // Optional details of the symbol.
   const Details *Detail = nullptr;
 
   // FIXME: add all occurrences support.
   // FIXME: add extra fields for index scoring signals.

clangd/index/SymbolCollector.cpp

bool shouldCollectIncludePath(index::SymbolKind Kind) {
   case SK::Variable:
   case SK::EnumConstant:
     return true;
   default:
     return false;
   }
 }
 
-/// Gets a canonical include (<header>  or "header") for header of \p Loc.
-/// Returns None if the header has no canonical include.
+/// Gets a canonical include (URI of the header or <header>  or "header") for
+/// header of \p Loc.
+/// Returns None if fails to get include header for \p Loc.
 /// FIXME: we should handle .inc files whose symbols are expected be exported by
 /// their containing headers.
 llvm::Optional<std::string>
 getIncludeHeader(const SourceManager &SM, SourceLocation Loc,
                  const SymbolCollector::Options &Opts) {
   llvm::StringRef FilePath = SM.getFilename(Loc);
   if (FilePath.empty())
     return llvm::None;
   if (Opts.Includes) {
     llvm::StringRef Mapped = Opts.Includes->mapHeader(FilePath);
     if (Mapped != FilePath)
       return (Mapped.startswith("<") || Mapped.startswith("\""))
                  ? Mapped.str()
                  : ("\"" + Mapped + "\"").str();
   }
-  // If the header path is the same as the file path of the declaration, we skip
-  // storing the #include path; users can use the URI in declaration location to
-  // calculate the #include path.
+
+  auto U = toURI(SM, SM.getFilename(Loc), Opts);
+  if (!U)
     return llvm::None;
+  return std::move(*U);
 }
 
 // Return the symbol location of the given declaration `D`.
 //
 // For symbols defined inside macros:
 //   * use expansion location, if the symbol is formed via macro concatenation.
 //   * use spelling location, otherwise.
 llvm::Optional<SymbolLocation>

unittests/clangd/FileIndexTests.cpp

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) {
+TEST(FileIndexTest, NoIncludeCollected) {
   FileIndex M;
-  std::string File = testPath("bits/basic_string");
-  M.update(File, build(File, "class string {};").getPointer());
+  M.update("f", build("f", "class string {};").getPointer());
 
   FuzzyFindRequest Req;
   Req.Query = "";
+  bool SeenSymbol = false;
   M.fuzzyFind(Req, [&](const Symbol &Sym) {
-    EXPECT_EQ(Sym.Detail->IncludeHeader, "<string>");
+    EXPECT_TRUE(Sym.Detail->IncludeHeader.empty());
+    SeenSymbol = true;
   });
+  EXPECT_TRUE(SeenSymbol);
 }
-#endif
 
 } // namespace
 } // namespace clangd
 } // namespace clang

unittests/clangd/SymbolCollectorTests.cpp

EXPECT_THAT(ConcatenatedSymbols,
                                    QName("clang::Foo2")));
 }
 
 TEST_F(SymbolCollectorTest, IncludeHeaderSameAsFileURI) {
   CollectorOpts.CollectIncludePath = true;
   runSymbolCollector("class Foo {};", /*Main=*/"");
   EXPECT_THAT(Symbols,
               UnorderedElementsAre(AllOf(QName("Foo"), DeclURI(TestHeaderURI),
-                                         IncludeHeader(""))));
+                                         IncludeHeader(TestHeaderURI))));
 }
 
 #ifndef LLVM_ON_WIN32
 TEST_F(SymbolCollectorTest, CanonicalSTLHeader) {
   CollectorOpts.CollectIncludePath = true;
   CanonicalIncludes Includes;
   addSystemHeadersMapping(&Includes);
   CollectorOpts.Includes = &Includes;