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

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo added a comment.

thanks for the review!



================
Comment at: clang-tools-extra/clangd/Hover.cpp:1094
 
+void maybeAddSymbolProviders(ParsedAST &AST, HoverInfo &HI,
+                             std::optional<const NamedDecl *> UsedDecl,
----------------
hokein wrote:
> 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).
> 
> 
thanks, that's right.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1138
+    if (H.kind() == include_cleaner::Header::Physical &&
+        H.physical() == SM.getFileEntryForID(SM.getMainFileID()))
+      continue;
----------------
hokein wrote:
> MainFile provider is a special case (I don't recall the details).
> 
> IIUC, the model is:
> 
> 1) a symbol usage that is satisfied (e.g. any of its providers that are directly included in the main file), we show the one with highest rank of these included providers
> 2) a symbol usage that is not satisfied (we choose the highest rank of all providers)
> 3) If the provider is the main-file, we don't show it in the hover card. 
> 
> 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
> -- on L1123 `ConvertedIncludes.match(H)` is always false  if H is a main-file, and we will choose a lower-rank provider if the main-file is the first element of `Headers`
> -- the logic here doesn't seem to work, we should do a `break` on L1139 rather than `continue`, which means we always use the `Headers[0]` element.
> 
> 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).
> we should do a break on L1139 rather than continue

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?),
but you're right that we shouldn't show the lower-ranked provider for this case.

> one alternative is to show the information for main-file provider as well

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 :)
So of the two options, I'd rather opt for not showing anything at all.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1099
+  trace::Span Tracer("Hover::maybeAddSymbolProviders");
+  include_cleaner::walkUsed(
+      AST.getLocalTopLevelDecls(), MacroReferences, AST.getPragmaIncludes(),
----------------
hokein wrote:
> VitaNuo wrote:
> > 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.
> > 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.
> 
> Ah, right, I missed this point.
> 
> > Because of that, the implementation is actually not that much shorter than the version with walkUsed.
> 
> 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.
> 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.

Totally, I agree.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144976