[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:543
+      return CB(InpAST.takeError());
+    CB(clangd::getTypeHierarchy(InpAST->AST, Item.range.start, Resolve,
+                                Direction));
----------------
nridge wrote:
> sammccall wrote:
> > nridge wrote:
> > > sammccall wrote:
> > > > relying on the item range to resolve the actual symbol isn't reliable:
> > > >  - the source code may have changed
> > > >  - the range may not be within the TU, and might be e.g. in an indexed header we don't have a compile command for.
> > > Good point. I appreciate a bit better now why you suggested trying to avoid `resolve` for now :)
> > > 
> > > What do you think of the following implementation approach:
> > > 
> > >  * Use the `data` field of `TypeHierarchyItem` (which the client will send back to the server for `resolve` queries) to send the `SymbolID` of the item to the client.
> > >  * When the client sends back the `SymbolID` in the `resolve` request, look up the symbol in the index, and use the source range from the symbol.
> > > 
> > > (Later, when we start storing base <--> derived relationships in the index for subtype support, we could just answer the entire `resolve` query using the index.)
> > Thanks for exploring all the options here!
> > 
> > Generally we've tried to avoid relying on the index unless it's needed, using the AST where possible. There are failure modes here:
> >  - the base type is in the current file, which is actively edited. The ranges in the index may be off due to staleness.
> >  - the base type is not indexed, because it is e.g. a member class inside a class template
> >  - there's no index (`-index=0`, though I'm not sure why we still support this) or the index is stale and the type is missing (we're working on making index updates more async)
> >  - the base type is not encodable.
> > 
> > There are just more moving pieces here, I think. Is there a clear reason to support resolve for parents?
> > Is there a clear reason to support resolve for parents?
> 
> Just what I said earlier about a hypothetical client that relies on it.
> 
> However, given the complications involved in implementing it, I'm happy with only being concerned with actual clients, not hypothetical ones. The only client implementation I currently know of is Theia, and I checked that it works fine without `resolve`, so I'm happy with deferring work on `resolve` for now.
> 
> If another client comes along that relies on `resolve`, we can revisit this.
> Just what I said earlier about a hypothetical client that relies on it.

I'll try to get the spec clarified such that eager resolution is always fine.
But if we ever do need to deal with such clients, @ioeric had a neat idea: we can just stash the rest of the response in the `data` field, and then "resolution" can just pull it out. This should be easy to bolt on.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:842
+
+  StringRef Filename = SM.getFilename(BeginLoc);
+  std::string FileURI = toURI(SM, Filename, {});
----------------
This is more conversions than necessary.
I think we just need:

```auto FilePath = getCanonicalPath(SM.getFileEntryForID(SM.getFileID(BeginLoc)), SM);
auto TUPath = getCanonicalPath(SM.getFileEntryForID(SM.getMainFileID()));
if (!FilePath || !TUPath)
  return;
THI.uri = URIForFile::Canonicalize(*FilePath, *TUPath);
```

(This is still more lines than I'd like, maybe we should have some helper for going from (File ID, SourceManager) --> URI)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370