[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

2133

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

2133

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

2133

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

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

2966 ↗

(On Diff #472101)

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

2966 ↗

(On Diff #472101)

(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

2966 ↗

(On Diff #472101)

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.h

 SignatureHelp signatureHelp(PathRef FileName, Position Pos,
                             const PreambleData &Preamble,
                             const ParseInputs &ParseInput,
                             MarkupKind DocumentationFormat);
 
 // For index-based completion, we only consider:
 //   * symbols in namespaces or translation unit scopes (e.g. no class
 //     members, no locals)
-//   * enum constants in unscoped enum decl (e.g. "red" in "enum {red};")
+//   * enum constants (both scoped and unscoped)
 //   * primary templates (no specializations)
 // For the other cases, we let Clang do the completion because it does not
 // need any non-local information and it will be much better at following
 // lookup rules. Other symbols still appear in the index for other purposes,
 // like workspace/symbols or textDocument/definition, but are not used for code
 // completion.
 bool isIndexedForCodeCompletion(const NamedDecl &ND, ASTContext &ASTCtx);
 

clang-tools-extra/clangd/CodeComplete.cpp

bool isIndexedForCodeCompletion(const NamedDecl &ND, ASTContext &ASTCtx) {
   // 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 true;
 
   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/index/Serialization.cpp

 //   - srcs: information related to include graph
 //   - stri: string table
 //   - symb: symbols
 //   - refs: references to symbols
 
 // The current versioning scheme is simple - non-current versions are rejected.
 // If you make a breaking change, bump this version number to invalidate stored
 // data. Later we may want to support some backward compatibility.
-constexpr static uint32_t Version = 17;
+constexpr static uint32_t Version = 18;
 
 llvm::Expected<IndexFileIn> readRIFF(llvm::StringRef Data,
                                      SymbolOrigin Origin) {
   auto RIFF = riff::readFile(Data);
   if (!RIFF)
     return RIFF.takeError();
   if (RIFF->Type != riff::fourCC("CdIx"))
     return error("wrong RIFF filetype: {0}", riff::fourCCStr(RIFF->Type));

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("Color2::Yellow"), forCodeCompletion(true)),
+                  AllOf(qName("Color3"), forCodeCompletion(true)),
+                  AllOf(qName("Color3::Blue"), forCodeCompletion(true)),
                   AllOf(qName("ns"), forCodeCompletion(true)),
                   AllOf(qName("ns::Black"), forCodeCompletion(true))));
 }
 
 TEST_F(SymbolCollectorTest, NamelessSymbols) {
   const std::string Header = R"(
     struct {
       int a;