[clangd] Index unscoped enums inside classes for code completion

[tom-anders]

Summary

As discussed in https://github.com/clangd/clangd/issues/1082:

"Without all-scopes-completion, such nested symbols would indeed be unnecessary in the
index, because the only way you could get them as a completion result is if you're
invoking the completion in the nested scope (e.g. Foo::^), in which case result can be
obtained from the AST. All-scopes-completion breaks this assumption, so the decision needs to be reevaluated."

I tested this on the LLVM codebase and there was no significant increase in index size
(less than 1MB). I think this makes sense if we look at the additional data that is stored
in SymbolCollector::addDeclaration when IndexedForCodeCompletion is set:

• Signature: Empty string for enum constants
• SnippetSuffix: Empty
• Documentation: Empty most of the time
• ReturnType and Type: Same string for all enum constants within a enum, so only two additional strings per indexed enum

[nridge]

clang-tools-extra/clangd/CodeComplete.cpp

2142

Just to make sure I understand:

By also removing the !isScoped() condition, in addition to changing the behaviour for the scenario described in https://github.com/clangd/clangd/issues/1082 (enum declared at class scope), you are also changing the behaviour for scenarios like this:

enum class Foo { Bar };  // at any scope, including global

Completing Bar will now offer Foo::Bar when previously it didn't.

Is this your intention?

[tom-anders]

clang-tools-extra/clangd/CodeComplete.cpp

2142

Ah sorry, that is indeed not what I described in the issue - but yeah this change is intended here, IMO it's more consistent from a user perspective: Why should all-scopes-completion ignore scoped enums?

[nridge]

clang-tools-extra/clangd/CodeComplete.cpp

2142

I have no objection to this additional change personally, but to play devil's advocate a bit: one could argue that for scoped enums, because the enumeration name needs to be mentioned at every use, the enumerator names are more likely to be shorter/common words, such that they may appear in multiple enumerations across a project, making the completion results more noisy.

Perhaps, it would make sense to split this out into a separate patch, so that in case it turns out to be controversial, it can be reverted without affecting the other part of the fix?

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

1334

I think it would be nice to add a test case to CodeCompleteTests.cpp as well.

(For example, it was not obvious to me that in the scoped enum case, completing the enumerator name would correctly insert the enumeration name as part of the scope, until I actually tried it in the editor.)

[tom-anders]

Add test to CodeCompletionTests, only consider unscoped enums in this patch (move scoped enums to separate patch)

[nridge]

clang-tools-extra/clangd/index/Serialization.cpp

458 ↗

(On Diff #472101)

I don't think indexing new symbols is a breaking change to the index format, in the sense that an older index cannot be read by a newer clangd.

As such, I don't think it's necessary to increment this version.

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

2970

Hmm, I don't think this type of test actually exercises the isIndexedForCodeCompletion() codepath (since we're just mocking out the index contents, instead of running the actual indexer).

Could we use this type instead?

(Sorry, this is partly my fault for not looking more carefully at what sorts of tests are in CodeCompleteTests.cpp before my earlier comment.)

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

1333

nit: place after ns / ns::Black to match the order in the code

[nridge]

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

2970

(Indeed, this test passes even without the change to CodeComplete.cpp.)

[tom-anders]

Add additional test, don't bump index version

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

2970

Ah right, no problem, I didn't see that there also tests in that file that don't mock out the index, I added a test similar to the one you mentioned.

[tom-anders]

Fix accidental line break in comment

[nridge]

Thanks!

I don't think we need this much detail in the commit message; I think after the first line it would be sufficient to say:

Fixes https://github.com/clangd/clangd/issues/1082
See the issue for disk/memory usage measurements

(Note also that "Fixes <issue>" triggers the github issue to be closed when the commit is merged.)

I forget whether you have commit access -- let me know if you need me to merge the patch for you.

[tom-anders]

In D136925#3901509, @nridge wrote:

Thanks!

I don't think we need this much detail in the commit message; I think after the first line it would be sufficient to say:

Fixes https://github.com/clangd/clangd/issues/1082
See the issue for disk/memory usage measurements

(Note also that "Fixes <issue>" triggers the github issue to be closed when the commit is merged.)

I forget whether you have commit access -- let me know if you need me to merge the patch for you.

Yep, I do have access. I adjusted the message and pushed this, but the corresponding Github issue is not closed automatically because I don't have the necessary permisions (I think we already had that problem in another patch)

clang-tools-extra/clangd/CodeComplete.cpp

auto InTopLevelScope = [](const NamedDecl &ND) {
     case Decl::Namespace:
     case Decl::LinkageSpec:
       return true;
     default:
       break;
     };
     return false;
   };
+  auto InClassScope = [](const NamedDecl &ND) {
+    return ND.getDeclContext()->getDeclKind() == Decl::CXXRecord;
+  };
   // We only complete symbol's name, which is the same as the name of the
   // *primary* template in case of template specializations.
   if (isExplicitTemplateSpecialization(&ND))
     return false;
 
   // Category decls are not useful on their own outside the interface or
   // implementation blocks. Moreover, sema already provides completion for
   // these, even if it requires preamble deserialization. So by excluding them
   // from the index, we reduce the noise in all the other completion scopes.
   if (llvm::isa<ObjCCategoryDecl>(&ND) || llvm::isa<ObjCCategoryImplDecl>(&ND))
     return false;
 
   if (InTopLevelScope(ND))
     return true;
 
+  // Always index enum constants, even if they're not in the top level scope:
+  // when
+  // --all-scopes-completion is set, we'll want to complete those as well.
   if (const auto *EnumDecl = dyn_cast<clang::EnumDecl>(ND.getDeclContext()))
-    return InTopLevelScope(*EnumDecl) && !EnumDecl->isScoped();
+    return (InTopLevelScope(*EnumDecl) || InClassScope(*EnumDecl)) && !EnumDecl->isScoped();
 
   return false;
 }
 
 CompletionItem CodeCompletion::render(const CodeCompleteOptions &Opts) const {
   CompletionItem LSP;
   const auto *InsertInclude = Includes.empty() ? nullptr : &Includes[0];
   LSP.label = ((InsertInclude && InsertInclude->Insertion)

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

TEST(CompletionTest, AllScopesCompletion) {
 
   auto Results = completions(
       R"cpp(
     namespace na {
     void f() { Clangd^ }
     }
   )cpp",
       {cls("nx::Clangd1"), cls("ny::Clangd2"), cls("Clangd3"),
-       cls("na::nb::Clangd4")},
+       cls("na::nb::Clangd4"), enmConstant("na::C::Clangd5")},
       Opts);
   EXPECT_THAT(
       Results.Completions,
-      UnorderedElementsAre(AllOf(qualifier("nx::"), named("Clangd1")),
-                           AllOf(qualifier("ny::"), named("Clangd2")),
-                           AllOf(qualifier(""), scope(""), named("Clangd3")),
-                           AllOf(qualifier("nb::"), named("Clangd4"))));
+      UnorderedElementsAre(AllOf(qualifier("nx::"), named("Clangd1"),
+                                 kind(CompletionItemKind::Class)),
+                           AllOf(qualifier("ny::"), named("Clangd2"),
+                                 kind(CompletionItemKind::Class)),
+                           AllOf(qualifier(""), scope(""), named("Clangd3"),
+                                 kind(CompletionItemKind::Class)),
+                           AllOf(qualifier("nb::"), named("Clangd4"),
+                                 kind(CompletionItemKind::Class)),
+                           AllOf(qualifier("C::"), named("Clangd5"),
+                                 kind(CompletionItemKind::EnumMember))));
 }
 
 TEST(CompletionTest, NoQualifierIfShadowed) {
   clangd::CodeCompleteOptions Opts = {};
   Opts.AllScopes = true;
 
   auto Results = completions(R"cpp(
     namespace nx { class Clangd1 {}; }
TEST(CompletionTest, UsingDecl) {
   Opts.Index = Index.get();
   Opts.AllScopes = true;
   auto R = completions(Source, {}, Opts);
   EXPECT_THAT(R.Completions,
               ElementsAre(AllOf(scope("std::"), named("foo"),
                                 kind(CompletionItemKind::Reference))));
 }
 
+TEST(CompletionTest, Enums) {
+  const char *Header(R"cpp(
+    namespace ns {
+      enum Unscoped { Clangd1 };
+      class C {
+      enum Unscoped { Clangd2 };
+      };
+      enum class Scoped { Clangd3 };
+    })cpp");
+  const char *Source(R"cpp(
+    void bar() {
+      Clangd^
+    })cpp");
+  auto Index = TestTU::withHeaderCode(Header).index();
+  clangd::CodeCompleteOptions Opts;
+  Opts.Index = Index.get();
+  Opts.AllScopes = true;
+  auto R = completions(Source, {}, Opts);
+  EXPECT_THAT(R.Completions,
+              ElementsAre(AllOf(scope("ns::"), named("Clangd1"),
+                                kind(CompletionItemKind::EnumMember)),
+                          AllOf(scope("ns::C::"), named("Clangd2"),
+                                kind(CompletionItemKind::EnumMember))));
+}
+
 TEST(CompletionTest, ScopeIsUnresolved) {
   clangd::CodeCompleteOptions Opts = {};
   Opts.AllScopes = true;
 
   auto Results = completions(R"cpp(
     namespace a {
     void f() { b::X^ }
     }

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

const std::string Header = R"(
     enum class Color2 {
       Yellow
     };
     namespace ns {
     enum {
       Black
     };
     }
+    class Color3 {
+      enum {
+        Blue
+      };
+    };
   )";
   runSymbolCollector(Header, /*Main=*/"");
   EXPECT_THAT(Symbols,
               UnorderedElementsAre(
                   AllOf(qName("Red"), forCodeCompletion(true)),
                   AllOf(qName("Color"), forCodeCompletion(true)),
                   AllOf(qName("Green"), forCodeCompletion(true)),
                   AllOf(qName("Color2"), forCodeCompletion(true)),
                   AllOf(qName("Color2::Yellow"), forCodeCompletion(false)),
                   AllOf(qName("ns"), forCodeCompletion(true)),
-                  AllOf(qName("ns::Black"), forCodeCompletion(true))));
+                  AllOf(qName("ns::Black"), forCodeCompletion(true)),
+                  AllOf(qName("Color3"), forCodeCompletion(true)),
+                  AllOf(qName("Color3::Blue"), forCodeCompletion(true))));
 }
 
 TEST_F(SymbolCollectorTest, NamelessSymbols) {
   const std::string Header = R"(
     struct {
       int a;
     } Foo;
   )";

clang-tools-extra/clangd/unittests/TestIndex.h

 Symbol sym(llvm::StringRef QName, index::SymbolKind Kind,
            llvm::StringRef USRFormat);
 // Creats a function symbol assuming no function arg.
 Symbol func(llvm::StringRef Name);
 // Creates a class symbol.
 Symbol cls(llvm::StringRef Name);
 // Creates an enum symbol.
 Symbol enm(llvm::StringRef Name);
+// Creates an enum constant symbol.
+Symbol enmConstant(llvm::StringRef Name);
 // Creates a variable symbol.
 Symbol var(llvm::StringRef Name);
 // Creates a namespace symbol.
 Symbol ns(llvm::StringRef Name);
 // Create a C++20 concept symbol.
 Symbol conceptSym(llvm::StringRef Name);
 
 // Create an Objective-C symbol.

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

 Symbol cls(llvm::StringRef Name) {
   return sym(Name, index::SymbolKind::Class, "@S@\\0");
 }
 
 Symbol enm(llvm::StringRef Name) {
   return sym(Name, index::SymbolKind::Enum, "@E@\\0");
 }
 
+Symbol enmConstant(llvm::StringRef Name) {
+  return sym(Name, index::SymbolKind::EnumConstant, "@\\0");
+}
+
 Symbol var(llvm::StringRef Name) {
   return sym(Name, index::SymbolKind::Variable, "@\\0");
 }
 
 Symbol ns(llvm::StringRef Name) {
   return sym(Name, index::SymbolKind::Namespace, "@N@\\0");
 }