[clangd] Code completion uses Sema for NS-level things in the current file.

[sammccall]

Summary

To stay fast, it avoids deserializing anything outside the current file, by
disabling the LoadExternal code completion option added in r3223771, when the
index is enabled.

[hokein]

nice, LGTM.

By r322377, I think you mean r322371.

We also need to set CollectorOpts.IndexMainFiles to false in FileIndex.cpp::indexAST -- we don't need dynamic index to catch the symbols in current main file since sema does that for us now :)

[sammccall]

In D41996#974751, @hokein wrote:

nice, LGTM.

By r322377, I think you mean r322371.

Done.

We also need to set CollectorOpts.IndexMainFiles to false in FileIndex.cpp::indexAST -- we don't need dynamic index to catch the symbols in current main file since sema does that for us now :)

This would fix the problem. But it will break index-based go-to-definition for modified files, so I'd rather leave this at least for now, and pursue deduplicating symbols instead.

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

 
 clang::CodeCompleteOptions CodeCompleteOptions::getClangCompleteOpts() const {
   clang::CodeCompleteOptions Result;
   Result.IncludeCodePatterns = EnableSnippets && IncludeCodePatterns;
   Result.IncludeMacros = IncludeMacros;
   Result.IncludeGlobals = IncludeGlobals;
   Result.IncludeBriefComments = IncludeBriefComments;
 
-  // Enable index-based code completion when Index is provided.
-  Result.IncludeNamespaceLevelDecls = !Index;
+  // When an is used, Sema is responsible for completing the main file,
+  // the index can provide results from the preamble.
+  // Tell Sema not to deserialize the preamble to look for results.
+  Result.LoadExternal = !Index;
 
   return Result;
 }
 
 CompletionList codeComplete(const Context &Ctx, PathRef FileName,
                             const tooling::CompileCommand &Command,
                             PrecompiledPreamble const *Preamble,
                             StringRef Contents, Position Pos,

clang-tools-extra/trunk/clangd/index/FileIndex.cpp

 namespace clangd {
 namespace {
 
 /// 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;
   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;
 

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

 
 namespace {
 using namespace llvm;
 using ::testing::AllOf;
 using ::testing::Contains;
 using ::testing::Each;
 using ::testing::ElementsAre;
 using ::testing::Not;
+using ::testing::UnorderedElementsAre;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
   void
   onDiagnosticsReady(const Context &Ctx, PathRef File,
                      Tagged<std::vector<DiagWithFixIts>> Diagnostics) override {
   }
 };
 
CompletionList completions(StringRef Text,
                            clangd::CodeCompleteOptions Opts = {}) {
   MockFSProvider FS;
   MockCompilationDatabase CDB;
   IgnoreDiagnostics DiagConsumer;
   ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
                       /*StorePreamblesInMemory=*/true);
   auto File = getVirtualTestFilePath("foo.cpp");
   Annotations Test(Text);
-  Server.addDocument(Context::empty(), File, Test.code());
+  Server.addDocument(Context::empty(), File, Test.code()).wait();
   auto CompletionList =
       Server.codeComplete(Context::empty(), File, Test.point(), Opts)
           .get()
           .second.Value;
   // Sanity-check that filterText is valid.
   EXPECT_THAT(CompletionList.items, Each(FilterContainsName()));
   return CompletionList;
 }
std::unique_ptr<SymbolIndex> simpleIndexFromSymbols(
   return MemIndex::build(std::move(Slab).build());
 }
 
 TEST(CompletionTest, NoIndex) {
   clangd::CodeCompleteOptions Opts;
   Opts.Index = nullptr;
 
   auto Results = completions(R"cpp(
-      namespace ns { class No {}; }
+      namespace ns { class Local {}; }
       void f() { ns::^ }
   )cpp",
                              Opts);
-  EXPECT_THAT(Results.items, Has("No"));
+  EXPECT_THAT(Results.items, Has("Local"));
 }
 
 TEST(CompletionTest, StaticAndDynamicIndex) {
   clangd::CodeCompleteOptions Opts;
   auto StaticIdx =
       simpleIndexFromSymbols({{"ns::XYZ", index::SymbolKind::Class}});
   Opts.StaticIndex = StaticIdx.get();
   auto DynamicIdx =

 TEST(CompletionTest, SimpleIndexBased) {
   clangd::CodeCompleteOptions Opts;
   auto I = simpleIndexFromSymbols({{"ns::XYZ", index::SymbolKind::Class},
                                    {"nx::XYZ", index::SymbolKind::Class},
                                    {"ns::foo", index::SymbolKind::Function}});
   Opts.Index = I.get();
 
   auto Results = completions(R"cpp(
-      namespace ns { class No {}; }
+      namespace ns { int local; }
       void f() { ns::^ }
   )cpp",
                              Opts);
   EXPECT_THAT(Results.items, Has("XYZ", CompletionItemKind::Class));
   EXPECT_THAT(Results.items, Has("foo", CompletionItemKind::Function));
-  EXPECT_THAT(Results.items, Not(Has("No")));
+  EXPECT_THAT(Results.items, Has("local"));
 }
 
 TEST(CompletionTest, IndexBasedWithFilter) {
   clangd::CodeCompleteOptions Opts;
   auto I = simpleIndexFromSymbols({{"ns::XYZ", index::SymbolKind::Class},
                                    {"ns::foo", index::SymbolKind::Function}});
   Opts.Index = I.get();
 
TEST(CompletionTest, FullyQualifiedScope) {
 
   auto Results = completions(R"cpp(
       void f() { ::ns::^ }
   )cpp",
                              Opts);
   EXPECT_THAT(Results.items, Has("XYZ", CompletionItemKind::Class));
 }
 
+TEST(CompletionTest, IndexSuppressesPreambleCompletions) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+                      /*StorePreamblesInMemory=*/true);
+
+  FS.Files[getVirtualTestFilePath("bar.h")] =
+      R"cpp(namespace ns { int preamble; })cpp";
+  auto File = getVirtualTestFilePath("foo.cpp");
+  Annotations Test(R"cpp(
+      #include "bar.h"
+      namespace ns { int local; }
+      void f() { ns::^ }
+  )cpp");
+  Server.addDocument(Context::empty(), File, Test.code()).wait();
+  clangd::CodeCompleteOptions Opts = {};
+
+  auto WithoutIndex =
+      Server.codeComplete(Context::empty(), File, Test.point(), Opts)
+          .get()
+          .second.Value;
+  EXPECT_THAT(WithoutIndex.items,
+              UnorderedElementsAre(Named("local"), Named("preamble")));
+
+  auto I = simpleIndexFromSymbols({{"ns::index", index::SymbolKind::Variable}});
+  Opts.Index = I.get();
+  auto WithIndex =
+      Server.codeComplete(Context::empty(), File, Test.point(), Opts)
+          .get()
+          .second.Value;
+  EXPECT_THAT(WithIndex.items,
+              UnorderedElementsAre(Named("local"), Named("index")));
+}
+
 TEST(CompletionTest, ASTIndexMultiFile) {
   MockFSProvider FS;
   MockCompilationDatabase CDB;
   IgnoreDiagnostics DiagConsumer;
   ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
                       /*StorePreamblesInMemory=*/true,
                       /*BuildDynamicSymbolIndex=*/true);