This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Index unscoped enums inside classes for code completion
ClosedPublic

Authored by tom-anders on Oct 28 2022, 1:30 AM.

Details

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

Diff Detail

Event Timeline

tom-anders created this revision.Oct 28 2022, 1:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2022, 1:30 AM
tom-anders requested review of this revision.Oct 28 2022, 1:30 AM
tom-anders edited the summary of this revision. (Show Details)Oct 28 2022, 1:35 AM
nridge added inline comments.Oct 30 2022, 12:53 AM
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 added inline comments.Oct 30 2022, 2:27 AM
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 added inline comments.Oct 30 2022, 9:15 PM
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 marked 3 inline comments as done.

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

tom-anders retitled this revision from [clangd] Index scoped enums for code completion to [clangd] Index unscoped enums inside classes for code completion.Oct 31 2022, 12:41 PM
nridge added inline comments.Nov 1 2022, 2:21 AM
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 added inline comments.Nov 1 2022, 2:51 AM
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
2970

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

tom-anders updated this revision to Diff 472425.Nov 1 2022, 3:11 PM
tom-anders marked 4 inline comments as done.

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 updated this revision to Diff 472428.Nov 1 2022, 3:15 PM

Fix accidental line break in comment

nridge accepted this revision.Nov 2 2022, 1:35 AM

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.

This revision is now accepted and ready to land.Nov 2 2022, 1:35 AM
This revision was landed with ongoing or failed builds.Nov 2 2022, 3:51 AM
This revision was automatically updated to reflect the committed changes.
tom-anders added a comment.EditedNov 2 2022, 3:53 AM

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)