[PATCH] D146244: [clangd] Show used symbols on #include line hover.

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

thanks, looks good from my side.



================
Comment at: clang-tools-extra/clangd/Hover.cpp:1175
+
+        for (const include_cleaner::Header &H : Providers) {
+          if (!ConvertedIncludes.match(H).empty()) {
----------------
VitaNuo wrote:
> kadircet wrote:
> > note that this will attribute a symbol to it's non-preferred providers too, e.g. if you have:
> > h1.h:
> > ```
> > struct Foo; // defined in foo.h
> > struct Bar; // defined in bar.h
> > struct Baz; // defined in baz.h
> > struct H1 {};
> > ```
> > 
> > I believe we want the following:
> > main.cc:
> > ```
> > #include foo.h // Provides Foo
> > #include bar.h // Provides Bar
> > #include h1.h // Provides Baz, H1
> > 
> > Foo *x;
> > Bar *y;
> > Baz *z;
> > H1 *h;
> > ```
> > 
> > and not saying that h1.h provides all Foo, Bar, Baz and H1 (otherwise an LLVM header will always provide dozens of symbols, e.g. https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/Type.h#L109)
> > 
> > Basically in addition to checking that the particular header we're interested in being a provider, we should also be checking there were no other providers that are mentioned in the main file with a higher rank.
> Yeah I know that and I've actually assumed until now that it's the correct behavior :) I.e., that we would like to report all the possible providers as providing the symbol, not only the highest ranked one. 
> 
> But what you're saying makes sense too. See the updated version. Also added a smaller version of the above as a test case.
thanks for raising it (I didn't think too much about this case). 

> and not saying that h1.h provides all Foo, Bar, Baz and H1 (otherwise an LLVM header will always provide dozens of symbols, e.g. https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/Type.h#L109)

Agree that for this case, we should prevent providing dozens of forward-declared symbols.

But the combination behavior of the unused diagnotics and hover can be confusing for case where `h1.h` provides all forward declarations and it is not the highest provider in any symbol refs of main.cc, so we will:
1) `h1.h` is considered as used -> no unused-include diagnostic 
2) `hover` on `h1.h` doesn't show any provided symbols

My mental model of 2) is that it indicates `h1.h` is not used, which seems to conflict with 1).  Maybe we can think it as a feature, for this case, it is safe to remove this header.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:62
 #include <optional>
+#include <set>
 #include <string>
----------------
nit: this is unused now.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1188
+          // in the main file.
+          return;
+        }
----------------
nit: my personal preference would be using `break` here. 


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3011
+                  #include "bar.h"
+                  #include "for^ward.h"
+                  Bar *x;
----------------
it would be nice to have a comment explaining why we show nothing here (because we have a higher provider `bar.h`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146244