[clangd] Don't query index when completing inside classes

[ilya-biryukov]

Summary

We used to query the index when completing after class qualifiers,
e.g. 'ClassName::^'. We should not do that for the same reasons we
don't query the index for member access expressions.

[sammccall]

clangd/CodeComplete.cpp

810

we could use a high level comment here explaining what the rest of the function is about. e.g.

// We also avoid ClassName::bar (but allow namespace::bar).

or just take the "we only query the index" comment, hoist it a bit, and generalize it (to not assume we're in name:: context)

826

(nit: this seems like undue weight, and could just be a trailing line comment // Unresolved inside a template)

unittests/clangd/CodeCompleteTests.cpp

829

Can we avoid disabling clang-format here? I do find it useful, and it adds noise.
IIRC moving the .items into the EXPECT_THAT results in sensible formatting.

837

I think you should also include "Foo::SomeNameInTheIndex" in the index. At first glance this doesn't seem to actually test all the failure modes (though I think it tested the one we actually had)

845

I think one of these would be enough, but up to you

[ilya-biryukov]

• Address review comments

unittests/clangd/CodeCompleteTests.cpp

829

Thanks! I struggled to rewrite code in a way that makes clang-format happy.

845

Those test different kinds of NestedNameSpecifiers (Type, TypeSpecWithTemplate and Identifier), so I'd keep all of them to make sure we have good coverage on that front.

clangd/CodeComplete.cpp

if (!Action.Execute()) {
     log("Execute() failed when running codeComplete for " + Input.FileName);
     return false;
   }
   Action.EndSourceFile();
 
   return true;
 }
 
-// Should we perform index-based completion in this context?
+// Should we perform index-based completion in a context of the specified kind?
 // FIXME: consider allowing completion, but restricting the result types.
-bool allowIndex(enum CodeCompletionContext::Kind K) {
+bool contextAllowsIndex(enum CodeCompletionContext::Kind K) {
   switch (K) {
   case CodeCompletionContext::CCC_TopLevel:
   case CodeCompletionContext::CCC_ObjCInterface:
   case CodeCompletionContext::CCC_ObjCImplementation:
   case CodeCompletionContext::CCC_ObjCIvarList:
   case CodeCompletionContext::CCC_ClassStructUnion:
   case CodeCompletionContext::CCC_Statement:
   case CodeCompletionContext::CCC_Expression:
bool contextAllowsIndex(enum CodeCompletionContext::Kind K) {
   case CodeCompletionContext::CCC_ObjCInstanceMessage:
   case CodeCompletionContext::CCC_ObjCClassMessage:
   case CodeCompletionContext::CCC_Recovery:
     return false;
   }
   llvm_unreachable("unknown code completion context");
 }
 
+// Should we allow index completions in the specified context?
+bool allowIndex(CodeCompletionContext &CC) {
+  if (!contextAllowsIndex(CC.getKind()))
+    return false;
+  // We also avoid ClassName::bar (but allow namespace::bar).
+  auto Scope = CC.getCXXScopeSpecifier();
+  if (!Scope)
+    return true;
+  NestedNameSpecifier *NameSpec = (*Scope)->getScopeRep();
+  if (!NameSpec)
+    return true;
+  // We only query the index when qualifier is a namespace.
+  // If it's a class, we rely solely on sema completions.
+  switch (NameSpec->getKind()) {
+  case NestedNameSpecifier::Global:
+  case NestedNameSpecifier::Namespace:
+  case NestedNameSpecifier::NamespaceAlias:
+    return true;
+  case NestedNameSpecifier::Super:
+  case NestedNameSpecifier::TypeSpec:
+  case NestedNameSpecifier::TypeSpecWithTemplate:
+  // Unresolved inside a template.
+  case NestedNameSpecifier::Identifier:
+    return false;
+  }
+}
+
 } // namespace
 
 clang::CodeCompleteOptions CodeCompleteOptions::getClangCompleteOpts() const {
   clang::CodeCompleteOptions Result;
   Result.IncludeCodePatterns = EnableSnippets && IncludeCodePatterns;
   Result.IncludeMacros = IncludeMacros;
   Result.IncludeGlobals = true;
   Result.IncludeBriefComments = IncludeBriefComments;
CompletionList runWithSema() {
     CompletionList Output;
     for (auto &C : Top)
       Output.items.push_back(toCompletionItem(C.first, C.second));
     Output.isIncomplete = Incomplete;
     return Output;
   }
 
   SymbolSlab queryIndex() {
-    if (!Opts.Index || !allowIndex(Recorder->CCContext.getKind()))
+    if (!Opts.Index || !allowIndex(Recorder->CCContext))
       return SymbolSlab();
     trace::Span Tracer("Query index");
     SPAN_ATTACH(Tracer, "limit", Opts.Limit);
 
     SymbolSlab::Builder ResultsBuilder;
     // Build the query.
     FuzzyFindRequest Req;
     if (Opts.Limit)

unittests/clangd/CodeCompleteTests.cpp

auto Requests = captureIndexRequests(R"cpp(
       }
       } // namespace ns
   )cpp");
 
   EXPECT_THAT(Requests, ElementsAre(Field(&FuzzyFindRequest::Scopes,
                                           UnorderedElementsAre(""))));
 }
 
+TEST(CompletionTest, NoIndexCompletionsInsideClasses) {
+  auto Completions = completions(
+      R"cpp(
+    struct Foo {
+      int SomeNameOfField;
+      typedef int SomeNameOfTypedefField;
+    };
+
+    Foo::^)cpp",
+      {func("::SomeNameInTheIndex"), func("::Foo::SomeNameInTheIndex")});
+
+  EXPECT_THAT(Completions.items,
+              AllOf(Contains(Labeled("SomeNameOfField")),
+                    Contains(Labeled("SomeNameOfTypedefField")),
+                    Not(Contains(Labeled("SomeNameInTheIndex")))));
+}
+
+TEST(CompletionTest, NoIndexCompletionsInsideDependentCode) {
+  {
+    auto Completions = completions(
+        R"cpp(
+      template <class T>
+      void foo() {
+        T::^
+      }
+      )cpp",
+        {func("::SomeNameInTheIndex")});
+
+    EXPECT_THAT(Completions.items,
+                Not(Contains(Labeled("SomeNameInTheIndex"))));
+  }
+
+  {
+    auto Completions = completions(
+        R"cpp(
+      template <class T>
+      void foo() {
+        T::template Y<int>::^
+      }
+      )cpp",
+        {func("::SomeNameInTheIndex")});
+
+    EXPECT_THAT(Completions.items,
+                Not(Contains(Labeled("SomeNameInTheIndex"))));
+  }
+
+  {
+    auto Completions = completions(
+        R"cpp(
+      template <class T>
+      void foo() {
+        T::foo::^
+      }
+      )cpp",
+        {func("::SomeNameInTheIndex")});
+
+    EXPECT_THAT(Completions.items,
+                Not(Contains(Labeled("SomeNameInTheIndex"))));
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang