[PATCH] D105083: [clangd] Ensure Ref::Container refers to an indexed symbol

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:159
+// indexed, we walk further up because Ref::Container should always be
+// an indexed symbol.
+const Decl *getRefContainer(const Decl *Enclosing,
----------------
can you also add some info about why we are not using `DeclContext`s ? something like:
```
Decls can nest under non-DeclContext nodes, in cases like initializer, where they might be attributed to VarDecl.
Preserving that level of granularity is especially useful for initializers at top-level, as otherwise the only context we
can attach these refs is TranslationUnitDecl, which we don't preserve in the index.
FIXME: Maybe we should have some symbols for representing file-scopes, that way we can show these refs as
being contained in the file-scope.
```

(Last fixme bit is optional, please add that if you also think the functionality would be more useful that way)


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:489
     DeclRefs[ND].push_back(
-        SymbolRef{SM.getFileLoc(Loc), Roles, ASTNode.Parent});
+        SymbolRef{SM.getFileLoc(Loc), Roles, getRefContainer(ASTNode.Parent)});
   // Don't continue indexing if this is a mere reference.
----------------
nridge wrote:
> kadircet wrote:
> > as mentioned above I think we should start the traversal from `ASTNode.ParentDC`. any downsides to doing that ? I don't see a case where this container we return is **not** a `DeclContext`.
> I assume you mean `ASTNode.ContainerDC`.
> 
> I tried this suggestion, but it looks like `ContainerDC` does not contain what we want in many cases.
> 
> For all of the following cases in `SymbolCollectorTest.RefContainers`:
> 
>  * ref2 (variable initializer)
>  * ref3 (function parameter default value)
>  * ref5 (template parameter default value)
>  * ref6 (type of variable)
>  * ref7a (return type of function)
>  * ref7b (parameter type of function)
> 
> `ASTNode.ContainerDC` is the `TranslationUnitDecl` (i.e. the `DeclContext` in which the function or variable is declared) rather than the function or variable itself.
> 
> In addition, for ref4 (member initializer), `ContainerDC` is the `RecordDecl` rather than the `FieldDecl`.
> 
> So, I don't think this suggestion will work without making the `Ref::Container` field significantly less specific (and thereby the call hierarchy feature less useful in some cases).
> 
> I assume you mean ASTNode.ContainerDC.

yes, sorry for the mess.

> So, I don't think this suggestion will work without making the Ref::Container field significantly less specific (and thereby the call hierarchy feature less useful in some cases).

I can't seem to remember why we went with it in the first place. But now:
```
int foo();
int x = foo();
void bar() {
  int y = foo();
}
```

calling call hierarchy on `foo` will yield `x` and `bar`, creating some sort of discrepancy. I am not really sure if telling first call to `foo` is contained in `x` is really useful compared to saying it was contained in `filename.cpp` (but ofc we can't do that today as container needs to be a symbol in the index, hence the ref will be lost instead). do you also think that would be more useful (i am not really a user of this feature so I would like to hear which is better from some users)?

also I can't say that I find the discrepancy between an initializer in parmvardecl/nontypetemplateparm vs vardecl useful (i know it already exists today, but not really sure why).
they are actually attached to the first declcontext containing them, not the first decl (which you say is more useful, let's leave the fact that they are not indexed out for a while).
moreover it is a behavior we rely on through libindex, as it just marks `ASTNode.Parent` with declcontext rather than making use of the vardecl. which makes me believe it is not really important to have the top-level symbol as the container. But it is the only way we can retain some container information. 
if that's the case, I believe we should make this more explicit, i.e. start a traversal from the node itself, remember the last indexed decl we've seen as a parent and return that if we failed to find any indexed declcontext's containing the decl (with a fixme saying that we could have some file symbols in the index for translationunitdecls).
That way we can say that the container is the first `DeclContext` containing the reference (with the exception of fixme mentioned).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105083/new/

https://reviews.llvm.org/D105083