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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

If we're extending the API, we should be careful to do it in an orthogonal and reasonable general way. Apple may not need USR in other places now, but there are users other than Apple and times other than today :-)

One of the reasons this looks odd is the method is "find definitions" rather than "get symbol info". So e.g. it's odd to return the symbol USR here but not in "find references", and doing a lookup in the index is unnecessary if what you want want is the USR. So I do think a new "get symbol info" method is better, if that's what you want.

This is doable as an LSP extension, though it does give me pause to reconsider the design:

  - the C++ API is for this much easier to design: get a SymbolID & Decl* while the AST is alive, with the option of looking up in the index. With LSP, it's not obvious what to include/exclude.
  - an LSP call can't return any persistent handle to the symbol (i.e Decl*) as the AST has no lifetime guarantees. So any follow-up operation on the symbol needs to repeat work and handle errors (symbol gone?). This puts further pressure on orthogonality.
- 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"?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54529