Page MenuHomePhabricator

[clangd] Implement textDocument/declaration from LSP 3.14
ClosedPublic

Authored by sammccall on Jan 29 2019, 7:38 AM.

Details

Summary

LSP now reflects the declaration/definition distinction.

Language server changes:

  • textDocument/definition now returns a definition if one is found, otherwise the declaration. It no longer returns declaration + definition if they are distinct.
  • textDocument/declaration returns the best declaration we can find.
  • For macros, the active macro definition is returned for both methods.
  • For include directive, the top of the target file is returned for both.

There doesn't appear to be a discovery mechanism (we can't return everything to
clients that only know about definition), so this changes existing behavior.
In practice, it should greatly reduce the fraction of the time we need to show
the user a menu of options.

C++ API changes:

  • findDefinitions is replaced by locateSymbolAt, which returns a vector<LocatedSymbol> - one for each symbol under the cursor.
  • this contains the preferred declaration, the definition (if found), and the symbol name

This API enables some potentially-neat extensions, like swapping between decl
and def, and exposing the symbol name to the UI in the case of multiple symbols.

Diff Detail

Event Timeline

sammccall created this revision.Jan 29 2019, 7:38 AM
sammccall updated this revision to Diff 184096.Jan 29 2019, 8:15 AM

Add server capability declarationProvider.
Technically an extension, but that's probably just a protocol bug.

Looks good overall, most are nits.

textDocument/definition now returns a definition if one is found, otherwise the declaration. It no longer returns declaration + definition if they are distinct.
textDocument/declaration returns the best declaration we can find.
For macros, the active macro definition is returned for both methods.
For include directive, the top of the target file is returned for both.

Just realized these at the last minutes, I think these are well-documented (and very helpful for future-code readers), I'd suggest putting them to the code comments as well.

clangd/ClangdLSPServer.cpp
357
clangd/ClangdLSPServer.h
84

nit: maybe put it before onGoToDefinition, it seems to me more natural that declaration comes first.

clangd/ClangdServer.h
170–171

nit: this comment seems out-of-date?

clangd/XRefs.cpp
40

is this a case that we were missing before? can we add a unittest for it (I didn't find a related change in the unittest).

303–304

nit: this is not used anymore.

clangd/XRefs.h
29

nit: maybe also mention marco? it is unclear to me how the declaration and definition would be like for macro without reading the implementation.

unittests/clangd/XRefsTests.cpp
339

I think this should be WantDef?

sammccall updated this revision to Diff 184678.Jan 31 2019, 9:24 PM
sammccall marked 7 inline comments as done.

Address review comments

Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2019, 9:24 PM
sammccall added inline comments.Jan 31 2019, 9:24 PM
clangd/ClangdLSPServer.cpp
357

Yeah, agree.

clangd/XRefs.cpp
40

Previously, this function only had to be correct for things that can be declared and defined separately.
For some decls that are always definitions (e.g. member variables) we would return nullptr here, and treat them as decl-only ... but that was OK, because the return value was just "a list of locations" and it didn't matter whether we treated them as decls or defs when there's just one.

However now the return type is "here's the decl, and here's the def". Without this change, we have Definition == llvm::None for e.g. member variables. While the LSP method falls back from def->decl so you probably can't observe this problem, API users can.

This is in fact covered by the tests, though it's kind of indirect (this is a private helper function, after all).

hokein accepted this revision.Feb 1 2019, 12:12 AM
hokein added inline comments.
clangd/XRefs.cpp
40

Thanks for the explanation. Maybe add more in the comment?

unittests/clangd/XRefsTests.cpp
339

what about this comment?

This revision is now accepted and ready to land.Feb 1 2019, 12:12 AM
sammccall marked 2 inline comments as done.Feb 1 2019, 2:50 AM
sammccall added inline comments.
clangd/XRefs.cpp
40

Added a little bit - there's not really so much to say, because the new behavior is kind of the obvious one.
The old behavior ("just return nullptr if it doesn't matter") maybe deserved a longer comment, but we didn't notice.

sammccall updated this revision to Diff 184710.Feb 1 2019, 2:51 AM

address review comments

This revision was automatically updated to reflect the committed changes.