[PATCH] D144976: [clangd] Add provider info on symbol hover.

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo marked an inline comment as done.
VitaNuo added a comment.

Thanks for the comments!



================
Comment at: clang-tools-extra/clangd/Hover.cpp:1099
+  trace::Span Tracer("Hover::maybeAddSymbolProviders");
+  include_cleaner::walkUsed(
+      AST.getLocalTopLevelDecls(), MacroReferences, AST.getPragmaIncludes(),
----------------
hokein wrote:
> 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:
> 
> - on hover, we have the selected symbol (either a regular declaration or a macro)
> - it is easy to construct a `include_cleaner::Symbol` from the selected symbol
> - choose the first result from `headersForSymbol`
> 
> To do that we need to expose `headersForSymbol` from the internal `AnalysisInternal.h`.
Thank you! I am using `headersForSymbols` now.

Ref satisfaction is not entirely out of scope.
If the provider is included, we would like to show this provider in the hover card, irrespective of the ranking.

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.
Because of that, the implementation is actually not that much shorter than the version with `walkUsed`.

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.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1140
+        // Check that the user is hovering over the current symbol reference.
+        if (CurLoc < Ref.RefLocation || CurLoc > RefEndLocation) {
+          return;
----------------
VitaNuo wrote:
> hokein wrote:
> > 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
> > ```
> > 
> > - clangd's hover uses the underlying decl (std::string_view), see the `pickDeclToUse` function for details;
> > - include-cleaner lib reports the using decl (asbl::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).
> > 
> 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.
> 
> 
> 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
> ```
> 
> - clangd's hover uses the underlying decl (std::string_view), see the `pickDeclToUse` function for details;
> - include-cleaner lib reports the using decl (asbl::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).
> 




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144976/new/

https://reviews.llvm.org/D144976