Page MenuHomePhabricator

[clangd] Treat lambdas as functions when preparing hover response
ClosedPublic

Authored by kadircet on Jun 3 2019, 9:26 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

kadircet created this revision.Jun 3 2019, 9:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2019, 9:26 AM
kadircet updated this revision to Diff 202739.Jun 3 2019, 9:29 AM
  • Get rid of wrong check

Do we care about pointers or references to lambda types?

clang-tools-extra/clangd/XRefs.cpp
662 ↗(On Diff #202739)

Variable types can be null (for C++17 deconstruction syntax), use getTypePtrOrNull.

664 ↗(On Diff #202739)

NIT: add extra braces to the inner if for more readable code

669 ↗(On Diff #202739)

NIT: add extra braces to the inner if for more readable code

kadircet updated this revision to Diff 202870.Jun 4 2019, 1:09 AM
kadircet marked 3 inline comments as done.
  • Address comments
  • Handle pointer/reference types to lambdas

Nice catch! I think it makes sense to show signature in those cases as well.
Updating according to that.

ilya-biryukov accepted this revision.EditedJun 4 2019, 1:24 AM

Nice catch! I think it makes sense to show signature in those cases as well.
Updating according to that.

Those cases are pretty rare, though, so I wasn't even sure they were worth it. They are pretty cheap to handle, though, so LG.

LGTM with a few last second NITs

clang-tools-extra/clangd/XRefs.cpp
664 ↗(On Diff #202739)

Uh, I meant the outer if, sorry for the confusion. My idea is that only one level of nesting is allowed to omit braces.

if (condition1) {
  if (condition2) 
    return true;
}

Not a big deal, though, don't want to nit-pick on minor code style issues. Feel free to tailor to your liking

clang-tools-extra/clangd/unittests/XRefsTests.cpp
831 ↗(On Diff #202870)

Could you add another test with even weirder types where we fail to show the signature? To make sure we don't break when reaching the limitations of the chosen approach and document what those limitations are.

Something like:

auto a = [](int a) { return 10; };
auto *b = &a;
auto *c = &b;

We would fail to show the signature here, but it's totally ok to ignore it.

This revision is now accepted and ready to land.Jun 4 2019, 1:24 AM
sammccall accepted this revision.Jun 4 2019, 1:30 AM
sammccall added inline comments.
clang-tools-extra/clangd/XRefs.cpp
657 ↗(On Diff #202870)

can this be a separate function rather than a local lambda?

I think the name could be clearer, e.g. getUnderlyingFunction

clang-tools-extra/clangd/unittests/XRefsTests.cpp
819 ↗(On Diff #202870)

I'm slightly nervous about the fact that "lambda" doesn't appear anywhere here.

e.g. maybe the Type should be "<lambda> bool(int, bool)" or so?
Many lambdas are not interchangeable with "plain" functions references.

kadircet marked 4 inline comments as done.Jun 4 2019, 2:55 AM
kadircet added inline comments.
clang-tools-extra/clangd/unittests/XRefsTests.cpp
819 ↗(On Diff #202870)

I've added the textual info to type. I don't think it is useful enough to put it as semantic info into the struct.

I believe it is rather a visual cue to the user, which seems pretty available in the "Type" field.

831 ↗(On Diff #202870)

added cases, and changed code(a lot simpler now) to generate signatures for those cases as well.

kadircet updated this revision to Diff 202888.Jun 4 2019, 2:55 AM
  • Address comments
ilya-biryukov added inline comments.Jun 4 2019, 3:32 AM
clang-tools-extra/clangd/XRefs.cpp
628 ↗(On Diff #202888)

We need a check for !QT.isNull here

clang-tools-extra/clangd/unittests/XRefsTests.cpp
831 ↗(On Diff #202870)

Here's an example when the new approach falls short too:

auto a = [](int) { return 10; }
std::function<void(decltype(a) x)> b;

In general, are we ok with loosing all the information about the type that we drop?
One level of references and pointers seemed ok, dropping more is a bit more cheesy..

At the same time, either case is so rare that we probably don't care.

ilya-biryukov requested changes to this revision.Jun 4 2019, 3:33 AM

Please add a check for the type of variable being not null before landing. The other comment is not very important

This revision now requires changes to proceed.Jun 4 2019, 3:33 AM
kadircet updated this revision to Diff 202933.Jun 4 2019, 7:11 AM
kadircet marked an inline comment as done.
  • Address comments
kadircet added inline comments.Jun 4 2019, 7:11 AM
clang-tools-extra/clangd/unittests/XRefsTests.cpp
831 ↗(On Diff #202870)

are you talking about hovering over x ? I don't think AST contains information regarding that one.

for a code like this:

auto foo = []() { return 5; };

template <class T>
class Cls {};

Cls<void(decltype(foo) bar)> X;

This is the AST dump for variable X:

`-VarDecl 0x2b0e808 <line:6:1, col:30> col:30 X 'Cls<void (decltype(foo))>':'Cls<void ((lambda at a.cc:1:12))>' callinit
  `-CXXConstructExpr 0x2b12e80 <col:30> 'Cls<void (decltype(foo))>':'Cls<void ((lambda at a.cc:1:12))>' 'void () noexcept'
ilya-biryukov added inline comments.Jun 4 2019, 8:18 AM
clang-tools-extra/clangd/unittests/XRefsTests.cpp
831 ↗(On Diff #202870)

I'm talking about hovering over b and, as Sam mentioned, there's a good chance you don't have this information in the type and we need to look at TypeLocs instead.

Also agree with Sam, we don't want any complexity for that case. Just wanted to make sure we added a test like this just to make sure we have some idea of what's produced there and it does not crash.

kadircet updated this revision to Diff 203533.Jun 7 2019, 4:14 AM
kadircet marked 3 inline comments as done.
  • Address comments
clang-tools-extra/clangd/unittests/XRefsTests.cpp
831 ↗(On Diff #202870)

I see, but then I don't think this case has anything to do with current patch, right?

It becomes a matter of decomposing a type with sugared components(which I believe should be visited but not in this patch) rather than expanding a lambda to a function like.

ilya-biryukov marked an inline comment as done.Jun 7 2019, 9:34 AM
ilya-biryukov added inline comments.
clang-tools-extra/clangd/unittests/XRefsTests.cpp
831 ↗(On Diff #202870)

Sorry, I missed that we still have the Type field set which will contain all pointers/references.
We don't seem to loose any information in that case, it's really up to the presentation layer to figure it out. LG

This revision was not accepted when it landed; it landed in state Needs Review.Jun 13 2019, 1:49 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2019, 1:49 AM