Change ClangdServer layer to output a structured response for Hover,
which can be rendered by client according to their needs.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 31511 Build 31510: arc lint + arc unit
Event Timeline
This is still WIP, just looking for feedbacks on HoverInfo struct and overall layering
Layering and such looks good. This should compose well with D58547
clang-tools-extra/clangd/XRefs.h | ||
---|---|---|
53 | I'm not sure about reuse of LocatedSymbol - do we want to commit to returning decl/def locations? | |
55 | This comes from LSP, and within the scope of C++ I think we might want stronger semantics here. | |
56 | SymbolInfo is a bit of a mess. Maybe we just want SymbolInfo::Kind? (LSP SymbolKind is tempting but loses a lot of info for C++). I do think we'll need to extend SymbolInfo::Kind or eventually use our own enum, e.g. lambdas may need their own kind (they're variables, but don't have a printable type and need to be displayed more like functions) | |
58 | I think we probably want a struct to represent types, so we can annotate it with links later on if needed. (because type can be e.g. mytemplate<mytype>). This wouldn't matter except we should probably reuse it... | |
58 | we may want ReturnType too (unless you want to overload Type for that - I'd suggest against it because of e.g. lambdas) | |
60 | maybe vector<param> params, where param is struct { string name; Type type }? We might render as: Parameters:
| |
64 | TemplateArgs might want to follow the same structure as params? |
clang-tools-extra/clangd/XRefs.h | ||
---|---|---|
53 | It might be nice to provide editors with enough info to jump to definition(it was brought up during last meeting). But happy to reduce it to just name. | |
56 | As you mentioned we might need to extend LSP's enum, but switching to it anyway. Currently it doesn't support "macro" kind exactly. |
Was passing by, just a small clarifying question...
clang-tools-extra/clangd/XRefs.h | ||
---|---|---|
73 | What does Type mean for non-type and template template parameters? |
clang-tools-extra/clangd/XRefs.h | ||
---|---|---|
73 | added comment for template template parameters. For non-type template params, isn't it clear that this will hold the type of that param? e.g template <C<int>... X> -> C<int>... |
clang-tools-extra/clangd/XRefs.h | ||
---|---|---|
73 | Ah, sure, sorry, I meant the type parameters :-) |
clang-tools-extra/clangd/XRefs.h | ||
---|---|---|
57 | Same about type template parameters. |
I will start adding test cases(and most likely change the way I populate the
struct at some places) if everyone is happy with current representation for
HoverInfo.
clang-tools-extra/clangd/XRefs.h | ||
---|---|---|
57 | Exactly, adding comments. |
clang-tools-extra/clangd/XRefs.h | ||
---|---|---|
53 | NIT: maybe go with std::string Type at a use-site instead? |
(I only got about halfway through the implementation - it's missing a lot of comments I think ;-))
clang-tools-extra/clangd/XRefs.cpp | ||
---|---|---|
539–547 | I think this is basically ASTContext::getTypeDeclType()? | |
549–561 | what does this have to do with decls? | |
563–569 | printDefinition? | |
574–582 | I don't think it's reasonable to define this private helper as an overload of operator<<. | |
1207 | avoid emitting the space if T/name are empty? | |
1214 | This doesn't seem sufficiently self-documenting. | |
clang-tools-extra/clangd/XRefs.h | ||
52 | This needs docs :-) | |
53 | (this is not done I think - LocatedSymbol is still here) | |
54 | one line doc for this struct? | |
58 | needs a real name | |
59 | mention the unnamed case | |
60 | mention the no-default case | |
64 | qualiffied -> qualified | |
65 | what's the difference between Scope and ParentScope? | |
69 | This sounds like it's a source location, (e.g. "file.cc:42:7") | |
72 | 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 | 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. | |
73 | T needs a real name. |
clang-tools-extra/clangd/XRefs.h | ||
---|---|---|
53 | 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. |
- Address comments
- Make little modifications to existing tests
clang-tools-extra/clangd/XRefs.cpp | ||
---|---|---|
574–582 | it is also used within render, making it a function | |
clang-tools-extra/clangd/XRefs.h | ||
65 | We've been using Name and Scope to represent two parts of qualified names across the codebase. |
comments on interface again, will take another pass at implementation
clang-tools-extra/clangd/XRefs.h | ||
---|---|---|
53 | 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"? | |
65 | 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. | |
76 | This seems overlapping/non-orthogonal with scope. It doesn't let us render a member as ClassName::method for instance. | |
89 | 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 | 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 | ||
---|---|---|
455 | 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.
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. | |
672 | what are the storage class specifiers that are relevant to function parameters? | |
683 | Not totally sure this is the best way to model variadics. We could consider leaving it out for now. | |
1169 | This really seems like the wrong idea to me. 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. | |
1177 | why do we have this no-definition case? | |
clang-tools-extra/clangd/unittests/XRefsTests.cpp | ||
940 | hmm, this is a bit weird - this uses specialization syntax but there's no actual specialization here right? |
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 | ||
---|---|---|
672 | i suppose only "register" is allowed in the context of a function parameter. but doesn't matter now as requested above, leaving specifiers out. | |
1177 | oops, this branch is a leftover :/ | |
clang-tools-extra/clangd/XRefs.h | ||
89 | 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 | 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 | 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 I agree this looks weird in the case of instantiations but I believe it is more important to give a consistent look. |
- 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.
clang-tools-extra/clangd/unittests/XRefsTests.cpp | ||
---|---|---|
940 | as discussed offline, leaving this as it is in the initial version and adding tests with both auto and non-auto cases. |
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.
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 | ||
---|---|---|
565 | namespace?! might be a clang-format bug? | |
572 | using the constructor/aggregate init here has a couple of downsides:
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; ... } | |
590 | I think this should be "ns1::ns2::", as we use scope internally. For the current rendering, it's easy to strip :: | |
597 | It would be nice to add void() or void (&)() or so if it's easy. | |
605 | note that if you want to it's easy to keep "Declared in namespace" - the LocalScope is empty. | |
676 | can we have a class template example where the instantiation isn't templated? 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) | |
780 | is it easy to omit the body? |
clang-tools-extra/clangd/unittests/XRefsTests.cpp | ||
---|---|---|
565 | I must have messed the brackets at some point :D | |
572 | yeah that looks a lot better, thanks! | |
590 | should it also be "::" for global namespace ? which would also result in prefixing any symbol in global namespace with "::" when printing. | |
597 | 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) | |
605 | 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 | |
780 | yes, sent out D62487 |
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 | ||
---|---|---|
590 | We tend to use empty string for global namespace, and explicitly add it where we need it. | |
597 | 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). I thought specifically the :GetType might still be useful, but maybe not. |
This needs docs :-)