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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks, left a few comments on the implementation.



================
Comment at: clang-tools-extra/clangd/Hover.cpp:1091
 
+// FIXME(bakalova): Remove after merging https://reviews.llvm.org/D143496
+std::vector<include_cleaner::SymbolReference>
----------------
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`.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1093
+std::vector<include_cleaner::SymbolReference>
+collectMacroReferences(ParsedAST &AST) {
+  const auto &SM = AST.getSourceManager();
----------------
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.
so we can simply the implementation:

1) For macro case, we already have the located macro and Tok in the call site (on line1231), we can just construct a single `include_cleaner::SymbolReference` from them, and pass a single-element `Macros` and empty `ASTRoots` to the `walkUsed` API.

2) For non-macro case, we can just pass an empty macro references to `walkUsed` as we already know the symbol under the hover is not a macro. 


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1123
+        if (!Ref.RefLocation.isFileID() ||
+            !SM.isWrittenInMainFile(SM.getSpellingLoc(Ref.RefLocation)))
+          // Ignore locations within macro expansions or not in the main file
----------------
no need to do the isWrittenInMainFile check in the callback, the walkUsed implementation already does it.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1128
+        if (Ref.RT != include_cleaner::RefType::Explicit)
+          // Ignore non-explicit references (e.g. variable names).
+          return;
----------------
I don't get it, why variable names are non-explicit references?


================
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;
----------------
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).



================
Comment at: clang-tools-extra/clangd/Hover.cpp:1153
+                SM.getFileEntryForID(SM.getMainFileID()) ==
+                    Provider.physical()) {
+              // Do not report main file as provider.
----------------
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.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1163
+            case include_cleaner::Header::Physical:
+              if (Provider.physical()->tryGetRealPathName() == Inc.Resolved)
+                DirectlyIncludedProviders.push_back(WrittenRef.str());
----------------
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`)


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1172
+              if (Provider.verbatim() == Inc.Written)
+                DirectlyIncludedProviders.push_back(WrittenRef.str());
+            }
----------------
nit: I'd suggest adding a `break` even though it is the last case. 


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1181
+        HI.ProviderInfo = std::optional<std::vector<std::string>>{
+            std::move(DirectlyIncludedProviders)};
+      });
----------------
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. 


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1232
         HI = getHoverContents(*M, Tok, AST);
+        if (HI) 
+          maybeAddSymbolProviders(AST, *HI, *CurLoc);
----------------
nit: no need to check `HI`, it is always non-null as it is set by the above line.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1268
         // Look for a close enclosing expression to show the value of.
-        if (!HI->Value)
+        if (HI && !HI->Value)
           HI->Value = printExprValue(N, AST.getASTContext());
----------------
any reason add a null check here, the HI should always be non-null becase we have set it on Line1263.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1335
 
+  if (ProviderInfo) {
+    Output.addRuler();
----------------
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.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1340
+    P.appendSpace();
+    for (unsigned I = 0; I < ProviderInfo->size() - 1; I++) {
+      P.appendCode((*ProviderInfo)[I]);
----------------
just use `llvm::join(*ProviderInfo, ", ");`


================
Comment at: clang-tools-extra/clangd/Hover.h:71
+  /// Headers providing the symbol (directly included in the main file).
+  std::optional<std::vector<std::string>> ProviderInfo;
   std::optional<Range> SymRange;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144976