Page MenuHomePhabricator

[clangd] Fix hover on symbol introduced by using declaration
ClosedPublic

Authored by tom-anders on Sep 11 2022, 3:56 AM.

Details

Summary

This fixes https://github.com/clangd/clangd/issues/1284. The example
there was C++20's "using enum", but I noticed that we had the same issue
for other using-declarations.

The problem is that Hover always uses the first element of explicitReferenceTargets which might not always be the one we want.
E.g. in the example from the linked issue, the first element is a UsingEnumDecl and the second one a EnumConstantDecl.

Diff Detail

Event Timeline

tom-anders created this revision.Sep 11 2022, 3:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 11 2022, 3:56 AM
tom-anders requested review of this revision.Sep 11 2022, 3:56 AM
tom-anders edited the summary of this revision. (Show Details)Sep 11 2022, 3:58 AM
kadircet added inline comments.Sep 13 2022, 6:42 AM
clang-tools-extra/clangd/Hover.cpp
1121

we actually want to show the using decl in certain cases, e.g:

namespace ns {
void foo(int); void foo(char);
}
using ns::foo;
template <typename T> void bar() { fo^o(T{}); }

so rather than this, can we have some logic that looks like:

- If we have more than two candidates, and there's exactly one `BaseUsingDecl`, prefer that.
- If we have two candidates, prefer the non-`BaseUsingDecl` one.
- Prefer the first. (we hit this case when there's a single candidate or there are `multiple or no` using decls. i think multiple using decls is not possible, but at least we preserve the existing behaviour in such cases).

can you also export this logic to a helper function, probably called pickDeclToUse?

Move logic to pickDeclToUse

tom-anders marked an inline comment as done.Sep 13 2022, 1:37 PM
nridge added a subscriber: nridge.Sep 13 2022, 1:49 PM
kadircet added inline comments.Sep 14 2022, 4:49 AM
clang-tools-extra/clangd/Hover.cpp
1023

nit: you can change this to Candidates.size() <= 2 and get rid of the extra single candidate case above (after changing it to always return Candidate.front() when Candidate.back() is a using decl.)

1026

just drop this if check and always return front, when it isn't a using decl.

1031

can we have a test case for this?

1032

nit: ns::foo not fo

kadircet added inline comments.Sep 14 2022, 5:50 AM
clang-tools-extra/clangd/Hover.cpp
1011

you can just pass in ArrayRef instead.

tom-anders marked 5 inline comments as done.

Add additional test, tidy up logic in pickDeclToUse()

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

Ah of course, forgot about that nice class

kadircet accepted this revision.Sep 14 2022, 6:54 AM

thanks, lgtm! i think you have commit access now, but let me know if I should land this for you (preferably with an email address for commit attribution)

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

nit: drop the empty line

This revision is now accepted and ready to land.Sep 14 2022, 6:54 AM

Yeah I do have commit access now, so I'll land this by myself.

I don't think I have the permissions to close the corresponding issue on Github though, so someone else would need to do that.

Yeah I do have commit access now, so I'll land this by myself.

I don't think I have the permissions to close the corresponding issue on Github though, so someone else would need to do that.

It happens automatically if the commit message includes "fixes <link to issue>", which it looks like this one does.

Hmm, Github noticed that I referenced the issue with this commit, but didn't close it.
According to https://github.blog/2013-03-18-closing-issues-across-repositories/ closing issues across repos should work, but only if you have push permissions in the repo that has the issue

Hmm, Github noticed that I referenced the issue with this commit, but didn't close it.
According to https://github.blog/2013-03-18-closing-issues-across-repositories/ closing issues across repos should work, but only if you have push permissions in the repo that has the issue

thanks for the heads up, closed it.