[PATCH] D54529: [clangd] Add USR to textDocument/definition response

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

I don't think we should be using `textDocument/definition` here, and I agree with Sam that a new method would be better. We don't actually need the semantic guarantees/constrains imposed by LSP's description of  `textDocument/definition`, as we want to find any declaration that corresponds to the reference at the location, so we don't need to guarantee that we return some definition. This would also avoid any confusion about USRs returned in `textDocument/definition`/`textDocument/references`.

@sammccall

> Good to know, this does seem like a reasonable direction, and exposing USR there seems totally fine. The only high-level question I can see here is should such a request hit the index or rely on AST only

It's up to all of us to come up with the semantics of this new request. For our use-case we need the AST information only, and we don't want to pay the index hit here. I would propose to define the semantics similar to `returning information about the symbol at the location from the view point of just the textDocument in which the query is located`. This might not be useful to high level clients that want all queries to be backed by the Index, but it could be useful to other clients that are interested in the symbol as its viewed by that text document. We don't have to specify whether it's a declaration or a definition as well.

> it's early days, are there more? The more custom actions there are that don't fit LSP, the less coherent ClangdLSPServer becomes (it's meant to model LSP quite closely, where ClangdServer matches "reality")

So are we still sure "mostly unmodified clangd over a different transport with a few extensions" is a better fit for the xcode XPC service than "AST management from ClangdServer, some features from ClangdServer, some custom features built on TUScheduler, exposed over an XPC protocol inspired by LSP"?

We are committed to relying on LSP with necessary extension / capability points. We don't really have any other option at this point due to scheduling concerns. As far as the need for different customizations, this "cursor-info" request is probably the most low level extension we need, and the majority of our future work will be relevant to high level, pure LSP clients (e.g. semantic highlighting, refactoring).

@jkorous

We don't need to rely on finding the definition or using the index for the new method. What we really need is to map from a location to a `Decl *` which we can represent as a symbol (USR, etc.) in the response. A declaration in this case is perfectly fine.  You can leverage existing code like `clang::tooling::getNamedDeclAt` that will return the declaration for a location, and that's used by things like clang-rename.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54529