[PATCH] D50958: [clangd] WIP: xrefs for dynamic index.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Mostly just reviewed the `references()` implementation.

For the infrastructure I have some high level questions:

- why are we combining SymbolSlab and occurrences?
- what's the motivation for the separate preamble/main-file indexes
- why put more functionality into SymbolCollector?

Let's discuss offline a bit if that works?



================
Comment at: clangd/TUScheduler.h:54
 
+class ParsingCallbacks {
+public:
----------------
(we're replacing a std::function, why not just a struct with two std::functions? saves boilerplate in a bunch of places...)


================
Comment at: clangd/XRefs.cpp:666
+std::vector<Location> references(ParsedAST &AST, Position Pos,
+                                 bool IncludeDeclaration,
+                                 const SymbolIndex *Index) {
----------------
I'm not sure unpacking the reference options into individual bools is going to scale well. I'd suggest passing the whole struct instead.


================
Comment at: clangd/XRefs.cpp:675
+  llvm::DenseSet<SymbolID> IDs;
+  llvm::DenseSet<SymbolID> NonLocalIDs;
+  for (const auto *D : Symbols.Decls) {
----------------
"local" is ambiguous here (this function cares both about file-local references and function-local symbols).
Consider `ExternallyVisibleIDs`


================
Comment at: clangd/XRefs.cpp:681
+                                                       : "Non-local\n");
+      if (!clang::index::isFunctionLocalSymbol(D))
+        NonLocalIDs.insert(*ID);
----------------
(This saves us hitting the index to look up references for one symbol, not sure if it's worth it at all).

I wouldn't particularly trust the helpers in clang::index::. Indeed the implementation looks wrong here (e.g. it would treat lambda captures as global, I think?)

I'd prefer D->getParentFunctionOrMethod() here.




================
Comment at: clangd/XRefs.cpp:688
+      SymbolOccurrenceKind::Reference | SymbolOccurrenceKind::Definition;
+  if (IncludeDeclaration)
+    Filter |= SymbolOccurrenceKind::Declaration;
----------------
I'm not sure this is the best interpretation of LSP `includeDeclaration`.

The LSP spec is very vague here, and we can usually assume it's written with typescript in mind :-) where the declaration/definition distinction doesn't really exist.
It appears the distinction they're trying to draw is declaration vs "only" reference, rather than definition vs "only" declaration. So I think if we're going to respect this flag, we should *exclude* definitions when the flag is false.

Alternatively we could punt on this and just ignore the flag for now, and add it in a followup.


================
Comment at: clangd/XRefs.cpp:694
+
+  SymbolCollector Collector({nullptr, &Opts}, {});
+  index::IndexingOptions IndexOpts;
----------------
Reusing symbolcollector here seems odd.

It forces us to go through SymbolID rather than just using the Decl*. This is certainly reliable for global symbols, but I've got no idea how robust USRs are for local symbols. It also ends up building the Symbol objects, which we then throw away.

How much of SymbolCollector are we really reusing, vs a custom IndexDataConsumer? How is this different from document-highlights? Maybe we can reuse the consumer.


================
Comment at: clangd/XRefs.cpp:699
+  IndexOpts.IndexFunctionLocals = true;
+  // Only find references for the current main file.
+  indexTopLevelDecls(AST.getASTContext(), AST.getLocalTopLevelDecls(),
----------------
This comment is surprising as it stands. Maybe 
```// Look in the AST for references from the current file.```
(and hoist the commentto the top of the code that deals with AST, currently line 686)


================
Comment at: clangd/XRefs.cpp:713
+
+  // Query index for non-local symbols.
+  if (Index && !NonLocalIDs.empty()) {
----------------
This is also a bit confusing - the symbols are non-local (in the function-scope sense) and the references are non-local (in the file sense).
Consider:
```
// Consult the index for references in other files.
// We only need to consider symbols visible to other files.
```


================
Comment at: clangd/XRefs.cpp:722
+      if (auto LSPLoc = toLSPLocation(O.Location, "")) {
+        if (!llvm::is_contained(SeenURIs, O.Location.FileURI))
+          Results.push_back(*LSPLoc);
----------------
in the usual case, SeenURIs is the current file (if there were any local usages), or empty (if there weren't).

This means if there are refs from the current file in the index but not locally (they were deleted), you'll return them when it would be better not to.

Why not just obtain the URI for the main file directly and compare that, instead of extracting it from the local references found?


================
Comment at: clangd/index/Index.h:368
   std::vector<Symbol> Symbols;  // Sorted by SymbolID to allow lookup.
-};
-
-// Describes the kind of a symbol occurrence.
-//
-// This is a bitfield which can be combined from different kinds.
-enum class SymbolOccurrenceKind : uint8_t {
-  Unknown = 0,
-  Declaration = static_cast<uint8_t>(index::SymbolRole::Declaration),
-  Definition = static_cast<uint8_t>(index::SymbolRole::Definition),
-  Reference = static_cast<uint8_t>(index::SymbolRole::Reference),
-};
-inline SymbolOccurrenceKind operator|(SymbolOccurrenceKind L,
-                                      SymbolOccurrenceKind R) {
-  return static_cast<SymbolOccurrenceKind>(static_cast<uint8_t>(L) |
-                                           static_cast<uint8_t>(R));
-}
-inline SymbolOccurrenceKind &operator|=(SymbolOccurrenceKind &L,
-                                        SymbolOccurrenceKind R) {
-  return L = L | R;
-}
-inline SymbolOccurrenceKind operator&(SymbolOccurrenceKind A,
-                                      SymbolOccurrenceKind B) {
-  return static_cast<SymbolOccurrenceKind>(static_cast<uint8_t>(A) &
-                                           static_cast<uint8_t>(B));
-}
-
-// Represents a symbol occurrence in the source file. It could be a
-// declaration/definition/reference occurrence.
-//
-// WARNING: Location does not own the underlying data - Copies are shallow.
-struct SymbolOccurrence {
-  // The location of the occurrence.
-  SymbolLocation Location;
-  SymbolOccurrenceKind Kind = SymbolOccurrenceKind::Unknown;
+  llvm::DenseMap<SymbolID, std::vector<SymbolOccurrence>> SymbolOccurrences;
 };
----------------
why are we moving occurrences into the symbol slab?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50958