Details
- Reviewers
hokein - Commits
- rG2bececb8bed1: [clangd] Add provider info on symbol hover.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Thanks, left a few comments on the implementation.
clang-tools-extra/clangd/Hover.cpp | ||
---|---|---|
1094 | I'm not sure how we can do it. https://reviews.llvm.org/D143496 doesn't expose collectMacroReferences function, so we still duplicate this hacky implementations in two places, which is probably fine at the moment. I think this will be fixed when we unify them with clangd's CollectMainFileMacros. | |
1096 | On a second thought, I think we might not really need it. We handle the macro vs non-macro cases separately, and we only care about the symbol reference under the cursor.
| |
1126 | no need to do the isWrittenInMainFile check in the callback, the walkUsed implementation already does it. | |
1131 | I don't get it, why variable names are non-explicit references? | |
1143 | We're using the location as a heuristic to join the symbol (Decl) from two difference sources (clangd's hover impl, and include-cleaner lib impl) This heuristics works most of time, but it will fail in the case where the symbol under the hover is a UsingDecl // absl_string_view.h namespace absl { using std::string_view; } // main.cpp #includle "absl_string_view.h" absl::str^ing_view abc; // hover on string_view
As a result, we will show the #include of the using decl for the underlying decl in the hover card. To solve the issue, I think we need to check the equality of Decl from hover and Decl from SymbolReference (comparing both getCanonicalDecl should be enough). | |
1156 | We probably don't need to handle this main-file case specially, in practice, main-file is hardly included in the MainFileIncludes, so the following logic will filter the main-file as well. | |
1166 | checking the spelled string can cause subtle behavior & bugs. A robust way is to use the include_cleaner::Includes.match API to determine a header is used or not (this is similar to what you did in https://reviews.llvm.org/D143496, we need to expose the transformation function convertIncludes) | |
1175 | nit: I'd suggest adding a break even though it is the last case. | |
1176 | nit: no need to check HI, it is always non-null as it is set by the above line. | |
1184 | I wonder whether we need to deduplicate the DirectlyIncludedProviders, reading the code, it might be possible that we add duplicated headers, but in practice, we should only have a single Ref reported, so the deduplication is probably not needed. | |
1216 | any reason add a null check here, the HI should always be non-null becase we have set it on Line1263. | |
1291 | Is the code position intended? It looks like we're inserting the Provided by XXX text in the middle of the function return type and the function parameters, which seems weird to me. I think the function return type and parameters should group together. Depending on the final UI, I think moving it before the ReturnType (right after the header) seems better than the current one. | |
1296 | just use llvm::join(*ProviderInfo, ", "); | |
clang-tools-extra/clangd/Hover.h | ||
72 | I think we can get rid of the std::optional we can distinguish by checking the ProviderInfo.empty(). Since we have 1 header for most cases, let use llvm::SmallVector. We can store the Inclusion* as the container element, and convert to string during the present() call. |
Implement the most recent version from the design document: always show a single provider, whether directly included or not (as long as there is a provider). Address review comments.
Thank you for the comments! I've implemented the new version as per the design doc, as well as did my best to address the review comments.
clang-tools-extra/clangd/Hover.cpp | ||
---|---|---|
1096 | This sounds like a great idea. Doing this now, thank you. | |
1131 | I made a mistake in the wording, sorry. I meant something like this: std::string Hello = ◊"Hello"; and wanted to prevent that hover over the string literal "Hello" shows provided by <string>. In the current implementation, however, this is not the case even without the explicit check for reference type. | |
1143 | Interesting case. I've tried it out, you're right indeed. And it seems like there's no way to pick the standard library header as provider, since include-cleaner doesn't report it (the absl header is the only candidate). I guess we'll skip providing the header info in this case then. | |
1156 | I believe we do need it. Otherwise, if you declare/define a new type in the main file (let's say main.cpp) and later use this type, you'd get provided by main.cpp in the hover, which seems pointless IMO. | |
1166 | Using the convertIncludes function now for to find the best included provider. | |
1175 | This is gone now. | |
1184 | Since we have decided that we always report one header, no de-duplication necessary anymore :) | |
1216 | sorry, probably some debugging artefact. | |
1291 | No, was not intended. We seem to have agreed to put it in the header section, so this should be the case now. | |
1296 | No need to join anything anymore, since we're showing a single header :) | |
clang-tools-extra/clangd/Hover.h | ||
72 | Ok, now since we have 1 header in all cases, there's definitely no need for std::optional. Let's just store a string and check that it's not empty before rendering. |
clang-tools-extra/clangd/Hover.cpp | ||
---|---|---|
1099 | It seems that the walkUsed API might be not the best fit. walkUsed API has some special logic on handling different AST nodes, e.g. refs of operators are ignored, so if we hover on an operator ref, we will not show the providing header (which we should). Our goal is to provide the information (header) where the symbol under the hover comes from (ref satisfaction is out of the scope). I think include_cleaner::headersForSymbol is a better fit for our purpose, and the implementation is simpler:
To do that we need to expose headersForSymbol from the internal AnalysisInternal.h. |
Thanks for the comments!
clang-tools-extra/clangd/Hover.cpp | ||
---|---|---|
1099 | Thank you! I am using headersForSymbols now. Ref satisfaction is not entirely out of scope. If the provider is not included, we show the best provider from the whole list of possible providers. The behavior is different, based on ref satisfaction. However, you're right that it solves the issue with operators (wasn't aware of that, thanks!). I've added a test case for the hover on operators. As an additional bonus, it also solves the issue with using decls as discussed in a different comment thread below. We can now say Provided by <string_view> in the hover card that pops up for the absl::string_view references because we are doing the analysis on the std::string_view decl. | |
1143 |
|
clang-tools-extra/clangd/Hover.cpp | ||
---|---|---|
1094 | we can simplify the signature like (ParsedAST&, include_cleaner::Symbol&, HoverInfo&), constructing a Symbol in call site is trivial, and it can help simplify the implementation (no sanity check for two std::optional etc). | |
1099 |
If the provider is included, we would like to show this provider in the hover card, irrespective of the ranking. Ah, right, I missed this point.
I think the implementation is still simpler, we don't need to non-trivial thing like comparing ref range with the selected range, and ref symbol declaration vs the selected symbol etc. | |
1138 | MainFile provider is a special case (I don't recall the details). IIUC, the model is:
Based on 1), if the main-file provider is the highest, we will not show it in the hover based on 3). However, the current implementation doesn't match this behavior
Not sure we have discussed 3), one alternative is to show the information for main-file provider as well, it seems fine to me that the hover shows provided by the current file text (not the full path). |
thanks for the review!
clang-tools-extra/clangd/Hover.cpp | ||
---|---|---|
1094 | thanks, that's right. | |
1099 |
Totally, I agree. | |
1138 |
Ok. I'm not sure if this is of great practical importance (what are the chances that the main file is the first provider, and there are more providers for the same symbol?),
Yeah, this is possible ofc, but I'm not sure what the use would be. The general intention of this feature (or feature set, even) is to help users eliminate unnecessary dependencies, and they can hardly get rid of the main file :) |
Thanks, left some comments on the unittest, I think we can make it simpler.
clang-tools-extra/clangd/Hover.cpp | ||
---|---|---|
1107 | The position of this comment is a bit confusing:
The comment here reads like the Matches is sorted! I think it is better to move it to the outer for-loop. The first element of Headers which satisfies !Matches.empty() is the best-ranked provider. | |
1118 | now the for-range loop doesn't seem necessary, we could always use the Headers.front(), right? | |
1138 |
I think the code should match our mental model, otherwise it is hard to reason about whether the actual behavior is expected or a bug.
I think the pattern is not rare (especially for headers), think of the case below. #include "other.h" // other.h transitively includes the `foo.h` class Foo; const Foo& createFoo(); although the main-file provider is not the top1 given the current ranking implementation, we have a plan to address it, see the FIXME in https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/include-cleaner/lib/FindHeaders.cpp#L219. After addressing the FIXME, the main-file could be the top1.
This comment doesn't seem to be addressed, now it is L1105. Given the following case, if the returned providers is [main-file, foo.h],
-> we should not show any information in hover. #include "foo.h" class Foo; Foo* b; // hover on `Foo`. | |
1219 | I wonder how well it works for the NamespaceDecl. NamespaceDecl is usually declared in many files, we will basically show a random provider in hover. We're not interested in NamesapceDecl, we probably need to ignore it. No action required here, but this is something we should bear in mind. | |
clang-tools-extra/clangd/IncludeCleaner.cpp | ||
258 | let's keep it unchanged, we don't need any change for this function in this patch. | |
clang-tools-extra/clangd/unittests/HoverTests.cpp | ||
2897 | I have some trouble following the test here:
I'd suggest to find way to simplify it, some ideas
// foo.h // foo_fwd.h // all.h | |
2901 | we can git rid of the int foo() content in foo.h since this test only tests symbol refs that is satisfied by the main file. | |
2914 | I guest this test is testing we chose a right ranking header, but it is hard to see why foo.h is better than bar.h, since both foo.h and bar.h content are identical, it is becase foo.h is better matching the symbol name (to make it more obvious, we could make foo.h contains a definition, or add a comment explaining it). | |
2922 | unclear to me the intention of this testcase. | |
2948 | this case seems to be duplicated with the first one, can be removed, I think. | |
3008 | This will have macro-redefined warnings, foo.h is better than bar.h because the MACRO in main-file refers to the one defined in foo.h. Not sure the value of this testcase, I would suggest dropping it for simplicity. The same for the below one. | |
3023 | this seems to be duplicated with the first testcase, nothing related to macro, can be dropped as well. | |
3052 | I'd just drop the IWYU-related test, since this patch has nothing to do with IWYU (we have the IWYU-specific tests in include-cleaner unittest). | |
3123 | What's the difference between HIFoo and HIFooBar? Looks like they are the same. I guess you want to check the <> and "" cases? | |
3139 | I think we can simplify this test and merge it to the above TEST(Hover, Providers). Just verifying the hover symbol respects the using decl is sufficient, something like #include "foo.h" namespace ns { using ::Foo; } ns::F^oo d; // verify that provider in hover is the main-file. |
Address review comments, simplify test.
Take care of main file as provider handling:
- provider list [main-file,foo.h] and foo.h \#include'd - not show any providers on hover.
- provider list [foo.h,main-file] and foo.h \#include'd - "provided by foo.h".
- provider list [main-file, bar.h] and bar.h not \#include'd - not show any providers on hover.
- provider list [bar.h,main-file] and bar.h not \#include'd - "provided by bar.h".
Thanks for the comments and the discussions!
clang-tools-extra/clangd/Hover.cpp | ||
---|---|---|
1118 | Ok. It seems like I need to put an empty check on Headers, however, because contrary to the loop, front() might actually crash AFAIU. | |
1138 |
Makes sense, I didn't think about headers at all.
Oops. See the current version please. | |
1219 | Thank you. | |
clang-tools-extra/clangd/IncludeCleaner.cpp | ||
258 | oh it's not changed, just got moved because of the other two. But no worries, I can put it exactly in the previous position. | |
clang-tools-extra/clangd/unittests/HoverTests.cpp | ||
2897 | Ok, I have simplified the test considerably. Please take a look. | |
2922 | Testing that foo.h is ranked higher since it's providing the symbol directly. But maybe this makes no sense, I will remove it then. | |
2948 | yeah this is also provided by the main file | |
3008 | ok. | |
3023 | ok. | |
3123 | yes, thank you. | |
3139 | Ok, I will translate this to a "foobar" example. But I think the correct answer in this example is actually foo.h since we want the provider of the underlying decl, not the using decl. |
Thanks, this looks great, some more comments (most are nits)
clang-tools-extra/clangd/Hover.cpp | ||
---|---|---|
1099 | maybe name it SortedProviders, it contains the main-file (calling Headers is a bit confusing). | |
1111 | if we perform an early return inside the loop, the code can be simpler. // Pickup the highest provider that satisfied the symbol usage, if available. // If the main-file is the chosen provider, we deliberately do not display it (as it may cause too much noise, e.g. for local variables). for (const auto & H : Headers) { if (isMainFile(H)) { return; } if (!Matches.empty()) { HI.Provider = Matches[0]->quote(); return; } } HI.Provider = spellHeader(AST...); | |
clang-tools-extra/clangd/Hover.h | ||
71 | nit: mention this header is with ""<>. | |
clang-tools-extra/clangd/IncludeCleaner.h | ||
71 | nit: can you add some document comment on these two functions? | |
clang-tools-extra/clangd/unittests/HoverTests.cpp | ||
2896 | looks like [[]] is not used in the test, we only use the ^ point. Remove them? | |
2897 | I think making the indentation like below is cleaer: {Annotations(R"cpp( struct Foo {}; int F = [[FO^O]]{}; )cpp"), [](HoverInfo &HI) { HI.Provider = ""; }}, | |
2911 | The indentation of the code block doesn't seem to align with the above cases. Unfortunately, clang-format doesn't format the codeblock inside the cpp()cpp string literal, we need to do it manually. | |
2938 | I think this should be "", the using declaration is defined in main-file, main-file should be the only one provider, and we don't display main-file provider. | |
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h | ||
16 | we have been using forward declaration for the SourceLocation, we can add a forward declaration class SourceManager on Line 26, and get rid of the #include here. | |
84 | since we move it from AnalysisInternal.h, can you remove the one in AnalysisInternal.h? I think we need to add #include "Analysis.h" to AnalysisInternal.h. |
Thanks for the review!
clang-tools-extra/clangd/Hover.cpp | ||
---|---|---|
1099 | I think RankedProviders is better, they are not exactly sorted. | |
1111 | Not really, this is not what we'd agreed upon. Consider the provider list [foo.h, main-file], foo.h _not_ being #include'd. | |
clang-tools-extra/clangd/unittests/HoverTests.cpp | ||
2938 | This is the same situation as absl::string_view vs. std::string_view, I believe. |
Thanks, this looks great! Please fix the code style of the unit-test (see my other comment) before landing it.
clang-tools-extra/clangd/Hover.cpp | ||
---|---|---|
1111 | Yeah, you're right. I don't remember what I thought on writing the comment. | |
clang-tools-extra/clangd/unittests/HoverTests.cpp | ||
2894 | Can you follow the the indentation-style of R"cpp()cpp" from the above TEST(Hover, NoHover) example? It would be nice to be consistent. We can save the explicit Annotations word here by changing the type of Code from Annotations to const char* const, and constructing the Annotations object inside the for-loop. | |
2938 | oh, right. I missed that fact that the target on Hover here is the underlying decl of the using decl, can you add a comment clarifying the hover here is showing the underlying decl? |
nit: mention this header is with ""<>.