[PATCH] D44954: [clangd] Add "member" symbols to the index

Thu May 17 13:42:10 PDT 2018

malaperle added a comment.

In https://reviews.llvm.org/D44954#1102807, @ilya-biryukov wrote:

> A few questions regarding class members. To pinpoint some interesting cases and agree on how we want those to behave in the long run.
>
> How do we handle template specializations? What will the qualified names of those instantiations be?
>  I.e. how do I query for `push_back` inside a vector?  Which of the following queries should produce a result?
>
> - `vector::push_back`. Should it match both `vector<T>::push_back` and `vector<bool>::push_back` or only the first one?
> - `vector<bool>::push_back`
> - `vector<int>::push_back`

It's probably better to consider this in a future patch. Maybe something like the first suggestion: vector::push_back and match both. Otherwise, I would think it might be a bit too verbose to have to spell out all of the specialization. Maybe we could allow it too. So... all of the above? :)

> What scopes will non-scoped enum members have?
>  E.g. if I have `enum En { A,B,C}`, which of the following queries will and won't find enumerators?
> 
> - `En::A`
> - `::A`
> - `A`

Hmm. I think all of them, since you can refer them like that in code too. Case #1 doesn't work but that was the case before this patch so it can probably be addressed separately. I'll add some tests though!

In https://reviews.llvm.org/D44954#1103026, @ioeric wrote:

> In https://reviews.llvm.org/D44954#1101922, @malaperle wrote:
>
> > @ioeric You mentioned in https://reviews.llvm.org/D46751 that it would make sense to add a flag to disable indexing members. Could you comment on that? What kind of granularity were you thinking? Would a "member" flag cover both class members (member vars and functions) and enum class enumerators for example? I think that would be reasonable. But I will also add symbols in main files too, so another flag for that? Hmmm.
>
>
> Sam convinced me that members would still be interesting for our internal index service (e.g. locations of members would be useful for go-to-def). I'll investigate how much impact that would be by running the indexer with your change patched in, but I don't want to block you on that, so I'm fine with checking this in without any filter. We could revisit the filter design when needed.
>
> We actually had an option for indexing symbols in main files :) But it was removed as it turned out to be unused for the features clangd had at that point. I think it would be reasonable to add it back if we start supporting collecting main file symbols again. Maybe out of the scope of this patch, but I am interested in the use cases you have for symbols in main files, besides in `workspaceSymbols`?

It's also for textDocument/documentSymbol. For this, we technically don't need them in the static index since we could collect symbols when the document is opened, but we also want them for workspaceSymbols so we might as well use the same symbol collector, etc. There should be more things that would also use symbol in main files, like Type Hierarchy.

================
Comment at: clangd/CodeComplete.cpp:932
     Req.Query = Filter->pattern();
+    Req.DeclContexts = {Decl::Kind::Namespace, Decl::Kind::TranslationUnit,
+                        Decl::Kind::LinkageSpec, Decl::Kind::Enum};
----------------
ilya-biryukov wrote:
> malaperle wrote:
> > I want to add a comment here, but I want to make sure I understand why in the first place we were not indexing symbols outside these contexts for the purpose of code completion. Is it because those will be available by Sema code completion anyway?
> C++ lookup rules inside classes are way more complicated than in namespaces and we can't possibly hope to give a decent approximation for those.
> Moreover, completion inside classes does not require any non-local information, so there does not seem to be a win when using the index anyway.
> So we'd rather rely on clang to do completion there, it will give way more useful results than any index implementation.
Makes sense. Thanks! I'll be able to document this better now with the full picture.

================
Comment at: clangd/index/Index.h:160
+  /// The Decl::Kind for the context of the symbol, i.e. what contains it.
+  Decl::Kind DeclContextKind;
+  /// Whether or not this is an enumerator inside a scoped enum (C++11).
----------------
ioeric wrote:
> ilya-biryukov wrote:
> > How do we use `DeclContextKind`?
> > Why did we decide to not go with a `bool ForCompletion` instead? (I'm probably missing a conversation in the workspaceSymbol review, could you point me to those instead?)
> > 
> > I'm asking because clang enums are very detailed and designed for use in the compiler, using them in the index seems to complicate things.
> > It feels we don't need this level of detail in the symbols. Similar to how we don't store the whole structural type, but rely on string representation of completion label instead.
> +1
> 
> `ForCompletion` sounds reasonable as the current design of index-based code completion relies on assumptions about contexts. 
My thinking was that the "ForCompletion" boolean was too specific and tailored for one client use. I thought the Symbol information should not have that much hard-coded knowledge on how it would be used. It would be odd to have "ForWorkspaceSymbol", "ForDocumentSymbol", etc. That is why I was going for a slightly more detailed symbol information that opened the door for more arbitrary queries for symbol clients. But it complicates things a bit more and I'd be happy to bring back the "ForCompletion" if it makes more sense for now.

================
Comment at: clangd/index/SymbolCollector.cpp:113

-  // We only want:
-  //   * symbols in namespaces or translation unit scopes (e.g. no class
-  //     members)
-  //   * enum constants in unscoped enum decl (e.g. "red" in "enum {red};")
-  auto InTopLevelScope = hasDeclContext(
-      anyOf(namespaceDecl(), translationUnitDecl(), linkageSpecDecl()));
-  // Don't index template specializations.
+  // Don't index template specializations and expansions in main files.
   auto IsSpecialization =
----------------
ioeric wrote:
> We should be careful when removing the `InTopLevelScope` filter. According to clang/AST/DeclBase.h, `DeclContext` can be any of the following. For example, indexing symbols in functions seems to be out of the scope of this patch. I think we should keep a whitelist instead of turning them all at once. It's unfortunately we don't have tests to catch those...
> ```
> ///   TranslationUnitDecl
> ///   NamespaceDecl
> ///   FunctionDecl
> ///   TagDecl
> ///   ObjCMethodDecl
> ///   ObjCContainerDecl
> ///   LinkageSpecDecl
> ///   ExportDecl
> ///   BlockDecl
> ///   OMPDeclareReductionDecl
> ```
> 
> For class members, I would be good to add more tests for symbol collector e.g. more realistic class layouts with constructor, friends etc.
For symbol in functions, they are not collected because "IndexOpts.IndexFunctionLocals = false" for the symbol collector. I'll investigate the other ones and add more tests.

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44954