Page MenuHomePhabricator

[clangd] Introduce a structured hover response
ClosedPublic

Authored by kadircet on May 3 2019, 6:13 AM.

Details

Summary

Change ClangdServer layer to output a structured response for Hover,
which can be rendered by client according to their needs.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ilya-biryukov added inline comments.May 7 2019, 7:17 AM
clang-tools-extra/clangd/XRefs.h
53 ↗(On Diff #198457)

NIT: maybe go with std::string Type at a use-site instead?
The scope of the name is large enough to make single-letter names a bit confusing

(I only got about halfway through the implementation - it's missing a lot of comments I think ;-))

clang-tools-extra/clangd/XRefs.cpp
539 ↗(On Diff #198457)

I think this is basically ASTContext::getTypeDeclType()?

549 ↗(On Diff #198457)

what does this have to do with decls?

563 ↗(On Diff #198457)

printDefinition?

574 ↗(On Diff #198457)

I don't think it's reasonable to define this private helper as an overload of operator<<.
Make it a function, or inline it? (I think used only once)

1207 ↗(On Diff #198457)

avoid emitting the space if T/name are empty?

1214 ↗(On Diff #198457)

This doesn't seem sufficiently self-documenting.

clang-tools-extra/clangd/XRefs.h
52 ↗(On Diff #198457)

This needs docs :-)

54 ↗(On Diff #198457)

one line doc for this struct?

58 ↗(On Diff #198457)

needs a real name
mention the no-type case (macros)

59 ↗(On Diff #198457)

mention the unnamed case

60 ↗(On Diff #198457)

mention the no-default case

64 ↗(On Diff #198457)

qualiffied -> qualified

65 ↗(On Diff #198457)

what's the difference between Scope and ParentScope?
Can we give them more obvious names, or a comment?
(The current comment doesn't really say anything)

69 ↗(On Diff #198457)

This sounds like it's a source location, (e.g. "file.cc:42:7")
But I think it's rather source code?

72 ↗(On Diff #198457)

I'm not sure this is significant or should even be true (e.g. if T is a function reference?)

Could we explain the relationship between the different fields here instead, maybe briefly with examples? (A function will have return type and parameters set, etc)

73 ↗(On Diff #198457)

T needs a real name.

73 ↗(On Diff #198295)

I get the curiosity, but this is too much text, and only covers template parameters which are not the most important case.

Can we just say something like The pretty-printed parameter type, e.g. "int", or "typename" (in TemplateParameters)?

We should do something sensible for template template parameters, but it's not important or non-obvious enough to document.

52 ↗(On Diff #197979)

(this is not done I think - LocatedSymbol is still here)

sammccall added inline comments.May 7 2019, 7:30 AM
clang-tools-extra/clangd/XRefs.h
53 ↗(On Diff #198457)

We had discussed making Type a struct, in case we want to add links etc to it later in a back-compatible way.

The typedef doesn't achieve that, I agree with just inlining std::string if we're not going to have the struct.

kadircet updated this revision to Diff 198630.May 8 2019, 5:27 AM
kadircet marked 23 inline comments as done.
  • Address comments
  • Make little modifications to existing tests
clang-tools-extra/clangd/XRefs.cpp
574 ↗(On Diff #198457)

it is also used within render, making it a function

clang-tools-extra/clangd/XRefs.h
65 ↗(On Diff #198457)

We've been using Name and Scope to represent two parts of qualified names across the codebase.
Comment was unfortunately misplaced :/ LMK if you have any other choices for the name

kadircet marked 6 inline comments as done.May 8 2019, 5:28 AM
kadircet added inline comments.
clang-tools-extra/clangd/XRefs.cpp
539 ↗(On Diff #198457)

No longer needed

549 ↗(On Diff #198457)

No longer needed

1214 ↗(On Diff #198457)

No longer needed

comments on interface again, will take another pass at implementation

clang-tools-extra/clangd/XRefs.h
65 ↗(On Diff #198457)

Yes, but here you've got three concepts: name/scope/parentscope (now called containedin?). And this covers lots of cases that our existing Name/Scope doesn't really (there are lots of symbols we don't index).

It wasn't a rhetorical question, I can't suggest better names because I don't know what they are.

My suggestion would be to split into namespace scope/local scope/name, such that Name has no ::, NamespaceScope has exactly the (non-inline) enclosing namespaces, and NamespaceScope + LocalScope + Name == QName.

So for a local in a class member, NamespaceScope == "clang::clangd"", LocalScope == "SymbolCollector::consume:: and Name = Symbols. Or just Name = Symbols and the others are blank, depending on how we choose to display locals.

53 ↗(On Diff #198630)

I'm having trouble following the "serialized" sentence. Maybe "It can be rendered as a hover panel, or embedding clients can use the structured information to provide their own UI"?

76 ↗(On Diff #198630)

This seems overlapping/non-orthogonal with scope. It doesn't let us render a member as ClassName::method for instance.
If we really need "In function x::Y" rather than "in x::Y", we could add a member "ImmediateScopeKind" or something. But I'm not sure we do.

89 ↗(On Diff #198630)

nit: just "templates"?

(e.g. if we hover over a call to std::swap, we might be showing the instantiation rather than the declaration?)

93 ↗(On Diff #198630)

Even if this struct ends up containing the range, it might be more obvious to have render() produce the MarkupContent only, leaving the caller to pull out the range themselves.

Converting the range isn't closely related to rendering, and I think for cleanest layering/testing you want to return FormattedString after rL360151 (which doesn't have a Hover equivalent).

Main comment is that I think the code is doing too much work to exactly reproduce the current output, and include as much information as possible.
Minimizing the diff is good all else equal, but one of the goals here is to have richer hovercards that are more consistent across the different codepaths that generate them.

clang-tools-extra/clangd/XRefs.cpp
456 ↗(On Diff #198630)

This has been pulled from DeclPrinter, so by definition duplicates info in the printed decl. We should only do this for info that's important, and I'm not sure most of this meets the bar.

  • that a function is virtual is certainly important (but isVirtual(), not isVirtualAsWritten())
  • that the declaration we found for a function is extern is certainly unimportant (it says something about the *decl*, not the function)
  • that a function is static means *something* important, but being a non-instance member is pretty different from being an internal helper function. Saying Kind=StaticMethod seems more useful for presentation than putting "static" next to the type again.

Aside from this, the current implementation puts these specifiers in the type, which they're not - functions don't return virtual ints or static strings.

I think we can probably drop these for now, and leave them in the printed decl only. We may want to revisit this as part of having Kind describe results more precisely.

726 ↗(On Diff #198630)

what are the storage class specifiers that are relevant to function parameters?

737 ↗(On Diff #198630)

Not totally sure this is the best way to model variadics. We could consider leaving it out for now.

1201 ↗(On Diff #198630)

This really seems like the wrong idea to me.
"Kind" is a category description suitable to be shown to the user, it's supposed to be useful.
In odd cases we may choose to do things like not show the type if the kind is function.

But HoverInfo is basically a box of facts to render, and using totally different rendering strategies for different kinds (and assuming strings mean macros) cuts against this.

1209 ↗(On Diff #198630)

why do we have this no-definition case?
Please avoid reinventing code to print C++ syntax if possible...

clang-tools-extra/clangd/unittests/XRefsTests.cpp
940 ↗(On Diff #198630)

hmm, this is a bit weird - this uses specialization syntax but there's no actual specialization here right?
I think the old output without template<> is probably better if possible.

kadircet marked 16 inline comments as done.May 13 2019, 5:55 AM

Thanks for also taking a look at the implementation, it is not complete yet. I am rather waiting for a green light on struct itself, so that I can write tests.

clang-tools-extra/clangd/XRefs.cpp
726 ↗(On Diff #198630)

i suppose only "register" is allowed in the context of a function parameter. but doesn't matter now as requested above, leaving specifiers out.

1209 ↗(On Diff #198630)

oops, this branch is a leftover :/

clang-tools-extra/clangd/XRefs.h
89 ↗(On Diff #198630)

Existing hover behavior is to show declaration, except auto, in that case we might show instantiations.

But you are right it is not necessarily only declarations.

93 ↗(On Diff #198630)

I don't think caller of this method would have enough context to deduce symbol's range so leaving the Range in the struct, but changing the output

clang-tools-extra/clangd/unittests/XRefsTests.cpp
940 ↗(On Diff #198630)

The old behavior was inconsistent in the case of auto. We print the decl in all cases, but print the type in the case of auto. For example, if you had
initializer_list<int> i = {1,2} instead of auto i = {1,2} you would get the new response I've provided in this test.

I agree this looks weird in the case of instantiations but I believe it is more important to give a consistent look.

kadircet updated this revision to Diff 199257.May 13 2019, 5:56 AM
kadircet marked 2 inline comments as done.
  • Address comments
kadircet updated this revision to Diff 200732.May 22 2019, 7:09 AM
kadircet marked 5 inline comments as done.
  • Added tests, and went over implementation.

I am looking for input on:

  • Behavior on lambdas, currently it just keeps the old behaviour. I believe they should look more like functions. Do you think it is necessary for first patch?
  • Template classes/functions, what we have in definition is not great but I don't think it is too much of a problem since semantic representation carries all the useful information.
  • Declared in #SOME_KIND# #SOME_PLACE#, currently we drop SOME_KIND and it needs additional info to be propogated since it is not the type of the symbol but rather it is immediate container. I don't think it is necessary for initial version.
  • Discrepencies between hovering over auto/decltype and explicitly spelled types, again this keeps the existing behaviour. You can see details in testcases.
kadircet added inline comments.May 22 2019, 7:10 AM
clang-tools-extra/clangd/unittests/XRefsTests.cpp
940 ↗(On Diff #198630)

as discussed offline, leaving this as it is in the initial version and adding tests with both auto and non-auto cases.

sammccall accepted this revision.May 27 2019, 2:28 AM

Thanks, I think you're right about all of this. Implementation looks OK too. Please add TODOs for the cases where we're punting to later vs deciding not to fix.

  • Behavior on lambdas, currently it just keeps the old behaviour. I believe they should look more like functions. Do you think it is necessary for first patch?

Agree. Not necessary in first patch IMO. Add a TODO though.

  • Template classes/functions, what we have in definition is not great but I don't think it is too much of a problem since semantic representation carries all the useful information.

These look OK to me, though examples are complicated and I might be missing something.

  • Declared in #SOME_KIND# #SOME_PLACE#, currently we drop SOME_KIND and it needs additional info to be propogated since it is not the type of the symbol but rather it is immediate container. I don't think it is necessary for initial version.

This seems OK to me. As noted you can keep it in the case that it's "namespace". If you want to propagate the container kind in this patch that's also fine.

  • Discrepencies between hovering over auto/decltype and explicitly spelled types, again this keeps the existing behaviour. You can see details in testcases.

Testcases look pretty good I think.

clang-tools-extra/clangd/unittests/XRefsTests.cpp
567 ↗(On Diff #200732)

namespace?!

might be a clang-format bug?

574 ↗(On Diff #200732)

using the constructor/aggregate init here has a couple of downsides:

  • vebosity: you need to list every field, even those that are not set for this case (TemplateParameters), or are misleading (SymRange is none)
  • brittleness: it makes it hard to change the struct at all
  • readability: the /*foo=*/ comments aren't bad, but not ideal either

Using field-by-field initialization would be better I think, though it's a bit awkward here.

but e.g. you could make the member std::function<HoverInfo> ExpectedBuilder, and write

[&](HoverInfo &Expected) {
  Expected.Name = "foo";
  Expected.Kind = SymbolKind::Function;
  ...
}
592 ↗(On Diff #200732)

I think this should be "ns1::ns2::", as we use scope internally.
This means we can simply concatenate parts to form a qname.

For the current rendering, it's easy to strip ::

599 ↗(On Diff #200732)

It would be nice to add void() or void (&)() or so if it's easy.
This is what :YcmCompleter GetType would show

678 ↗(On Diff #200732)

can we have a class template example where the instantiation isn't templated?
e.g.

template<typename T> class vector{};
[[vector]]<int> X;

(just because this is an example that we've discussed a lot, and these edge-case tests make it hard to see the common behavior)

782 ↗(On Diff #200732)

is it easy to omit the body?

951 ↗(On Diff #200732)

note that if you want to it's easy to keep "Declared in namespace" - the LocalScope is empty.
(Though we can't provide this info for non-namespace containers)

This revision is now accepted and ready to land.May 27 2019, 2:28 AM
kadircet updated this revision to Diff 201536.May 27 2019, 8:08 AM
kadircet marked 10 inline comments as done.
  • Address comments
kadircet marked 10 inline comments as done.May 27 2019, 8:08 AM
kadircet added inline comments.
clang-tools-extra/clangd/unittests/XRefsTests.cpp
567 ↗(On Diff #200732)

I must have messed the brackets at some point :D

574 ↗(On Diff #200732)

yeah that looks a lot better, thanks!

592 ↗(On Diff #200732)

should it also be "::" for global namespace ? which would also result in prefixing any symbol in global namespace with "::" when printing.

599 ↗(On Diff #200732)

just put the type without any parameter names, but I am not sure whether users would want that. I believe people find current hover behavior a lot more useful then just showing type(which is done by libclang)

782 ↗(On Diff #200732)

yes, sent out D62487

951 ↗(On Diff #200732)

I believe it doesn't make sense to add only one, so leaving it until we hear from users that they need the kind information

Still LG

I forgot to mention - it would be nice to clang-format the definition, what do you think?

clang-tools-extra/clangd/unittests/XRefsTests.cpp
592 ↗(On Diff #200732)

We tend to use empty string for global namespace, and explicitly add it where we need it.
Using :: for global causes as many problems as it solves (e.g. in C).

599 ↗(On Diff #200732)

I don't think we should include it in the actual hover, but we can handle that in rendering (if it's a Function, don't print the type).
It does seem odd not to include it in the structured API, but up to you.

I thought specifically the :GetType might still be useful, but maybe not.

kadircet updated this revision to Diff 201543.May 27 2019, 9:52 AM
kadircet marked 7 inline comments as done.
  • Address comments
kadircet updated this revision to Diff 201628.May 28 2019, 3:10 AM
  • clang-format Definition
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 28 2019, 3:28 AM