This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Show used symbols on #include line hover.
ClosedPublic

Authored by VitaNuo on Mar 16 2023, 10:06 AM.

Diff Detail

Event Timeline

VitaNuo created this revision.Mar 16 2023, 10:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2023, 10:06 AM
VitaNuo requested review of this revision.Mar 16 2023, 10:06 AM
VitaNuo updated this revision to Diff 506141.Mar 17 2023, 11:00 AM

Finish the test.

hokein added inline comments.Mar 20 2023, 5:14 AM
clang-tools-extra/clangd/Hover.cpp
1143

we have a similar function symbolName in include-cleaner's FindHeaders.cpp, but it is not exposed. I think it would be nice to share the implementation (probably add a name() method in the include_cleaner::Symbol structure). No action required, can you add a FIXME here?

1147

we need to check whether the dyn_cast is a null

1160

just want to check the intention, we're using the set here because we want an alphabet order of UsedSymbolNames. If yes, then looks good (probably add a comment in the field UsedSymbolNames of HoverInfo).

1168

the indentation seems wrong here.

1173

since we expose convertIncludes in your previous patch, we can construct a include_cleaner::Includes struct from the parssing Inc, and use the match() method to match the header.

1459

if the UsedSymbolNames.size() > 5, we will show the last element rather than the fifth one, my reading of the code this is not intended, right?

1460

let's abstract a const variable e.g. SymbolNamesLimit for this magic number 5, and use it in all related places

clang-tools-extra/clangd/Hover.h
116

we can use just a vector, we can use .empty() to the unused-include case.

llvm::SmallVector is preferred, per https://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h.

clang-tools-extra/clangd/unittests/HoverTests.cpp
2989

the [[]] range doesn't seem to be used, remove?

2999

nit: please fix the code indentation.

VitaNuo updated this revision to Diff 507326.Mar 22 2023, 5:31 AM
VitaNuo marked 10 inline comments as done.

Address review comments.

Thanks for the comments!

clang-tools-extra/clangd/Hover.cpp
1160

Actually no, we're using set primarily to deduplicate symbol names. Otherwise, many symbols have multiple references each, and the resulting list of used symbols ends up having a lot of duplicates.

I didn't know a set would order used symbols names alphabetically, but if so, that's a nice side effect :)

1173

Yeah, good idea, this makes the function considerably shorter.

1459

Right, sorry. That was only covering the case of UsedSymbolNames.size() <= 5.

clang-tools-extra/clangd/Hover.h
116

Ok for std::optional removal, but I wouldn't want to use llvm::SmallVector. HoverInfo does not contain any LLVM types atm (most of the fields are either std::string or std::vector), so I am not sure we want to introduce a divergence for unclear benefit.

hokein added inline comments.Mar 23 2023, 2:23 AM
clang-tools-extra/clangd/Hover.cpp
1160

yeah, if duplication is the only purpose, then llvm::DenseSet (which is based on a hash-table, thus elements are not sorted) is preferred. The bonus point of using std::set (which is based on the red-black tree) is that the elements are sorted.

I think it would be nice to keep the result sorted (either by the ref range, or the symbol name), using symbol name seems fine to me, can you add a comment for UsedSymbolNames field to clarify that it is sorted by the symbol name?

1174

can you move it out of the callback? otherwise we will construct an include_cleaner::Includes every time when the callback is invoked, which is an unnecessary cost.

1178

nit: inline the Matches in the if body, if (!ConvertedIncludes.match(H).empty())

1461

I think it'd be better to move this into the above for loop (adding a special logic) rather than having a non-trivial handling for the last element.

What do you think about the something like below (it has less usages of .size() and SymbolNamesLimit-1)

auto Fronts = llvm::ArrayRef(UsedSymbolNames).take_front(std::min(UnusedSymbolNames.size(), SymbolNamesLimit));

for (const auto& Sym : Fronts) {
   P.appendCode(Sym);
   if (Sym != Fronts.back())
     P.appendText(", ");
}

if (UsedSymbolNames.size() > Fronts.size()) {
    P.appendText(" and ");
    P.appendText(std::to_string(UsedSymbolNames.size() - Fronts.size()));
    P.appendText(" more");
}
clang-tools-extra/clangd/unittests/HoverTests.cpp
24

the newly-introduced code doesn't seem to use the std::optional symbol

2988

nit: fix the indentation (align with the one using in this file), see my comment in your other patch.

VitaNuo updated this revision to Diff 507793.Mar 23 2023, 10:18 AM
VitaNuo marked 5 inline comments as done.

Address review comments.

VitaNuo marked an inline comment as done.Mar 23 2023, 10:22 AM

Thanks for the comments!

clang-tools-extra/clangd/Hover.cpp
1174

Oh right, sorry.

1461

Sure, thank you.

thanks, the implementation looks good. I left some comments around the test.

clang-tools-extra/clangd/unittests/HoverTests.cpp
2984

Can you add a test in the existing TEST(Hover, Present) to verify the present() method work as expected when the number provided symbols <= 5/ > 5?

2990

can you add a macro FOO to the test?

2993

nit: remove the unused [[]].

2996

IIUC, this test is to test multiple symbols provided by a header, I would suggest removing the foo.h, and just keep bar.h (to reduce the noise from the test).

3000

It is not quite obvious to readers that foobar, and X is from the bar.h, maybe use some more obvious name (e.g. defining classes named Bar1, Bar2 etc).

3007

This test is to test the system header, I'd simulate it more further to reflect the reality. Let's use <vector>, and a system/vector file with the dummy content namespace std { template<typename> class vector {}; }

(pardon the interruption, some drive-by comments :))

clang-tools-extra/clangd/Hover.cpp
1175

note that we don't care about each reference of a target, so speed things up (rather than going through each reference inside the main file), you can check if you've seen Ref.Target before and skip processing providers in that case.

1176

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.

1203

can you introduce a trace::metric here for tracking hover distributions on certain symbol types? you can see an example in https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/XRefs.cpp#L352

i believe the cases we care about are:

  • include
  • macro
  • keyword
  • expr
  • decl
  • attribute

(so that we have some idea about how often we provide this information)

VitaNuo updated this revision to Diff 508661.Mar 27 2023, 7:53 AM
VitaNuo marked 10 inline comments as done.

Address review comments.

Thanks for the comments!

clang-tools-extra/clangd/Hover.cpp
1176

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.

1203

Ok. This goes a bit out of scope, but I hope we won't get too bogged down in this. Have a look, I hope I understood the API correctly.

clang-tools-extra/clangd/unittests/HoverTests.cpp
2996

Ok, I will join it with the foo.h testcase then, because they've become the same thing essentially.

VitaNuo updated this revision to Diff 508675.Mar 27 2023, 8:36 AM

Add a couple more macro test cases.

VitaNuo updated this revision to Diff 508708.Mar 27 2023, 9:47 AM

Remove extra check (already done in walkUsed).

kadircet added inline comments.Mar 28 2023, 1:06 AM
clang-tools-extra/clangd/Hover.cpp
1174

creating these symbol names for every reference is still a lot of waste, you can directly cache Ref.Target pointers, which are a lot cheaper. you can also store them in an llvm::DenseSet<const Decl*> (std::set has O(logN) operation times). afterwards you can call getRefName only once, on the decls that we care about, and llvm::sort the result.

also because you're using just the declname, we might get erroneous de-duplication (symbols might have same name under different qualifiers) and all of a sudden ns1::Foo might suppress the analysis of ns2::Foo because logic here will think we've already seen a symbol named Foo

1179

you can't actually keep a reference here, return is a value type. just auto MatchingIncludes = ..;

1179

nit: this might read easier with:

auto MatchingIncludes = ConvertedMainFileIncludes.match(H);
// No match for this provider in the main file.
if (MatchingIncludes.empty())
  continue;
// Check if our include matched this provider.
for (auto *MatchingInc : MatchingIncludes) {
  if (MatchingInc->Line == Inc.HashLine + 1)
    UsedSymbolNames.insert(getRefName(Ref));
// Don't look for rest of the providers once we've found a match in the main file.
break;
}
VitaNuo updated this revision to Diff 508948.Mar 28 2023, 3:37 AM
VitaNuo marked 3 inline comments as done.

Address review comments.

Thanks for the comments, see the updated version.

clang-tools-extra/clangd/Hover.cpp
1174

Ok, I am storing include_cleaner::Symbols now (not the Decls, since those would require a switch on the symbol kind, etc.) and llvm::sort'ing the result then.

1179
for (auto *MatchingInc : MatchingIncludes) {
  if (MatchingInc->Line == Inc.HashLine + 1)
    UsedSymbolNames.insert(getRefName(Ref));
}

I'm not sure I agree that this snippet is better. It's more verbose than the current version, and IMO matching the hovered include against the provider is quite readable already.

I'm using your suggestions for the comments now, thanks.

hokein accepted this revision.Mar 28 2023, 6:16 AM

thanks, looks good from my side.

clang-tools-extra/clangd/Hover.cpp
62

nit: this is unused now.

1176

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.

1187

nit: my personal preference would be using break here.

clang-tools-extra/clangd/unittests/HoverTests.cpp
3011

it would be nice to have a comment explaining why we show nothing here (because we have a higher provider bar.h)

This revision is now accepted and ready to land.Mar 28 2023, 6:16 AM
kadircet accepted this revision.Mar 28 2023, 6:50 AM

thanks the pieces i looked at LG too

VitaNuo updated this revision to Diff 508994.Mar 28 2023, 6:55 AM
VitaNuo marked 4 inline comments as done.

Address review comments.

clang-tools-extra/clangd/Hover.cpp
1176

No solution here, just a comment: this seems to go along the lines of our discussion in the sync today, i.e., that we might want to make the unused include analysis stricter eventually. So it seems like the general tendency might be towards making the unused include analysis stricter, rather than showing more provided symbols on hover.

This revision was landed with ongoing or failed builds.Mar 28 2023, 7:06 AM
This revision was automatically updated to reflect the committed changes.