[PATCH] D54799: [clangd][WIP] textDocument/CursorInfo method
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Nov 21 09:51:07 PST 2018
sammccall added a comment.
Thanks for sending this! Broadly looks good, a few details to work out. I think the biggest one is multiple symbols which you've flagged.
> I'd like to ask for early feedback - what's still missing is relevant client capability.
I'm actually not 100% sure that's necessary for custom LSP methods.
- a server capability would suffice at least, unless we're worried clients are going to call it "by mistake"?
- even that doesn't seem much better than just allowing the call where other servers would return an error. Keep it simple?
I'd suggest leaving it out unless others feel strongly.
> Couple things that I'd love to hear opinions about are:
>
> - conditional return in `getCursorInfo` - Should we return for example data with empty `USR`?
Please return a symbol unless it has no SymbolID (we don't treat those as symbols in clangd).
In practice this amounts to the same thing for now (SymbolID is derived from USR). May change in the future though.
> - `containerName` of local variables - It's currently empty even if semantic parent has a name (say it's a function). (Please search for local.cpp in the test.) Is that what we want?
good question, no idea.
> - For now I used `getSymbolAtPosition()` as it's simpler and fits the context better. However I assume I could use this optimization from `tooling::getNamedDeclAt()` (in a separate patch): https://github.com/llvm-mirror/clang/blob/master/lib/Tooling/Refactoring/Rename/#L82
Yes, though at least for C++ this optimization won't usually buy anything, as the top-level decl will just be a namespace decl I think.
We could plumb this deeper if we want to win more here.
- One thing I am wondering about is whether we could use (and whether it's a significant improvement) some early return in `indexTopLevelDecls` (using `DeclarationAndMacrosFinder`) also for hover and definition use-cases. Is it correct to assume that at a given `Location` there can be maximum of one declaration and one definition?
There can be multiple and we should return them all. (Not sure what we do for hover, but defs handles this correctly).
I can't remember which are the important cases, but one we handle like this is implicit constructor calls:
class Foo { Foo(const char*); }
void g(Foo);
const char* str = "abc";
g(str);
completion on the `s` in the last line finds both the Foo constructor and the definition of `str`.
Maybe it's worth revisiting this if we can't find any really important examples. That would involve digging up the historical context (I don't remember it right now).
> P. S. Alex and Ben have thanksgiving break now so they'll probably add any feedback next week.
Hope they're having a good break!
================
Comment at: ClangdLSPServer.cpp:728
MsgHandler->bind("workspace/didChangeConfiguration", &ClangdLSPServer::onChangeConfiguration);
+ MsgHandler->bind("textDocument/cursorInfo", &ClangdLSPServer::onCursorInfo);
// clang-format on
----------------
I'd suggest `textDocument/symbolInfo` for consistency with LSP (and similar with internal names).
The term "symbol" is used throughout LSP to describe roughly what we're using it to mean here.
Conversely I guess "cursor" refers to the editor cursor, but LSP seems to prefer just talking about positions.
================
Comment at: ClangdServer.h:205
+ /// Get cursor info for given position.
+ void getCursorInfo(PathRef File, Position Pos,
+ Callback<llvm::Optional<IdentifierData>> CB);
----------------
nit: just `cursorInfo` (or `symbolInfo` etc) for consistency with other where there isn't a specific verb.
================
Comment at: Protocol.cpp:436
+ const IdentifierData &SUCI) {
+ O << SUCI.containerName << "::" << SUCI.name << " - " << toJSON(SUCI);
+ return O;
----------------
(you only want containername if non-empty, right?)
================
Comment at: Protocol.h:676
+/// Represents information about identifier.
+struct IdentifierData {
----------------
Unless I'm misunderstanding, this is about a symbol, not an identifier. (e.g. overloads of C++ functions are distinct symbols). Maybe `SymbolDetails` would be a useful name.
There's a question of scope here: `getCursorInfo` seems to return a single instance of these, but there can be 0, 1, or many symbols at a given location. These could be `vector<SymbolDetails>` or so, or if this struct is intended to encapsulate them all, it could be named `SymbolsAtLocation` or so.
================
Comment at: Protocol.h:676
+/// Represents information about identifier.
+struct IdentifierData {
----------------
sammccall wrote:
> Unless I'm misunderstanding, this is about a symbol, not an identifier. (e.g. overloads of C++ functions are distinct symbols). Maybe `SymbolDetails` would be a useful name.
>
> There's a question of scope here: `getCursorInfo` seems to return a single instance of these, but there can be 0, 1, or many symbols at a given location. These could be `vector<SymbolDetails>` or so, or if this struct is intended to encapsulate them all, it could be named `SymbolsAtLocation` or so.
/// This is returned from textDocument/cursorInfo, which is a clangd extension.
================
Comment at: Protocol.h:678
+struct IdentifierData {
+ /// The name of this symbol.
+ std::string name;
----------------
This comment doesn't really say anything. It can be omitted, or we could add additional information.
(Some of the existing comments are equally content-free, because we've copied the comments from LSP verbatim, but that's not the case here)
================
Comment at: Protocol.h:682
+ /// The name of the symbol containing this symbol.
+ std::string containerName;
+
----------------
If we're not going to merge, I'd suggest making this "scope" and allowing it to end with ::.
This makes it easier to compose (qname = scope + name), and makes it clearer what "contains" means. (LSP got this wrong IMO, at least for C++).
================
Comment at: Protocol.h:682
+ /// The name of the symbol containing this symbol.
+ std::string containerName;
+
----------------
sammccall wrote:
> If we're not going to merge, I'd suggest making this "scope" and allowing it to end with ::.
> This makes it easier to compose (qname = scope + name), and makes it clearer what "contains" means. (LSP got this wrong IMO, at least for C++).
what do you want this to do for objc methods and fields?
It'd be worth having a test if it's important, I'm guilty of not knowing/checking ObjC behavior.
================
Comment at: Protocol.h:684
+
+ /// The USR of this symbol.
+ llvm::SmallString<128> USR;
----------------
USR could use some definition/references.
================
Comment at: Protocol.h:685
+ /// The USR of this symbol.
+ llvm::SmallString<128> USR;
+
----------------
please don't micro-optimize with smallstring etc in this file.
================
Comment at: Protocol.h:689
+ /// Size is dependent on clang::clangd::SymbolID::RawValue.
+ llvm::SmallString<32> ID;
+};
----------------
can this just be SymbolID?
================
Comment at: Protocol.h:690
+ llvm::SmallString<32> ID;
+};
+llvm::json::Value toJSON(const IdentifierData &);
----------------
I think SymbolKind would be useful (in particular distinguishing macros might be important?)
This does point towards maybe adding to SymbolInformation rather than a new struct, but I'm not sure which is best.
================
Comment at: XRefs.cpp:95
Preprocessor &PP;
+ const bool StopOnFirstDeclFound;
----------------
Please don't do this - it's inconsistent with the other XRefs features.
(If for some reason you *really* want just one result, then this is easy to do on the client side if we get the API right)
================
Comment at: XRefs.cpp:770
+
+ if (!Symbols.Decls.empty()) {
+ if (const NamedDecl *ND = dyn_cast<NamedDecl>(Symbols.Decls.front().D)) {
----------------
(as alluded to elsewhere, we should return all decls/macros, not just one)
================
Comment at: clangd/cursor-info.test:1
+# RUN: clangd -lit-test < %s | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
----------------
It's great to have a smoke test for features like this (probably just a trivial call), but we test the details in unit tests for easier maintenance.
XRefsTests has a bunch of tests for similar features. TestTU + Annotations should make these tests fairly easy to write.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54799
More information about the cfe-commits
mailing list