This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add hover info for `this` expr
ClosedPublic

Authored by xndcn on Nov 24 2020, 9:07 AM.

Details

Summary

How about add hover information for this expr?
It seems useful to show related information about the class for this expr sometimes.

Diff Detail

Event Timeline

xndcn created this revision.Nov 24 2020, 9:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 24 2020, 9:07 AM
xndcn requested review of this revision.Nov 24 2020, 9:07 AM

Thanks, this sounds like a sensible idea. I got a few suggestions for the implementation though.

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

what about using the existing getHoverContents(QualType ..) overload instead ?

633–634

can you handle CXXThisExpr inside this function, instead of an extra branch in the getHover?

xndcn added inline comments.Nov 25 2020, 12:02 AM
clang-tools-extra/clangd/Hover.cpp
610

what about using the existing getHoverContents(QualType ..) overload instead ?

610

Thanks, I tried using getHoverContents(QualType ..) overload and the result looks more simplicity:

The origin patch HoverInfo looks like:

Both seems reasonable, not sure which one is better.

633–634

Thanks, here need additional parameter Index *, will update patch soon.

kadircet added inline comments.Nov 25 2020, 12:14 AM
clang-tools-extra/clangd/Hover.cpp
610

I am not sure if there's much value in repeating this and type definition. So I would go with using the QualType overload here.
But, rather than providing CTE->getType() directly, i believe we should display information for the PointeeType, as it will also display comments and such for TagDecls, rather than just providing the type-name.

lebedev.ri retitled this revision from Add hover info for `this` expr to [clangd] Add hover info for `this` expr.Nov 25 2020, 12:14 AM
xndcn added inline comments.Nov 25 2020, 12:48 AM
clang-tools-extra/clangd/Hover.cpp
610

Thanks! It looks more simplicity with PointeeType. Shall we keep the additional information like namespace and template parameters? So we can use getHoverInfo(const NamedDecl ..) overload?

xndcn updated this revision to Diff 307622.Nov 25 2020, 8:25 AM

Update the diff with getHoverContents(const NamedDecl ..) overload function.

kadircet added inline comments.Nov 25 2020, 10:45 AM
clang-tools-extra/clangd/Hover.cpp
610

i was thinking those would be just repeating what's already available but you seem to be right. especially namespace and template arguments might be useful.

631

let's update the comment too. "Generates hover info for this and evaluatable expressions."

656

can you put this above printExprValue, also the else is unnecessary as branch ends with return, i.e:

if(thisexpr) {
...
return ..;
}
if(printExprValue..) {
...
return ..;
}
657

s/getAsCXXRecordDecl/getAsTagDecl/

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

can you add two more test cases, one for a template class and one for a specialization:

template <typename T> 
struct Foo {
  Foo* bar() { return [[thi^s]]; }
};
template <> 
struct Foo<int> {
  Foo* bar() { return [[thi^s]]; }
};
xndcn updated this revision to Diff 307749.Nov 25 2020, 7:39 PM

Thanks! Update the commit as review comments.

kadircet accepted this revision.Nov 26 2020, 5:09 AM

thanks, LGTM!

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

nit: return getHoverContents(D, Index);

This revision is now accepted and ready to land.Nov 26 2020, 5:09 AM
xndcn updated this revision to Diff 307947.EditedNov 26 2020, 6:57 PM

Thanks. Update commit to fix the last nit.

It seems that I don't have the commit access...

Do you have commit access or should I commit this for you?

xndcn added a comment.Nov 27 2020, 7:20 PM

Do you have commit access or should I commit this for you?

I still don't have commit access, so please help me commit this, thank you!

can you give me an email address to associate the commit with?

One last point, is it worth including cv qualifications in the hover info?

xndcn added a comment.Dec 1 2020, 6:14 AM

can you give me an email address to associate the commit with?

xndchn@gmail.com, thank you.

kadircet requested changes to this revision.Dec 1 2020, 9:18 AM

One last point, is it worth including cv qualifications in the hover info?

Yes, I think we should (and I thought we already did!). Sorry for messing this up, I am not sure what made me change my mind in the middle but we should definitely be using QualType printing rather than NamedDecl printing, similar to how we generate hover for autos.

It produces an output like:

namespace ns {
template <typename T> struct Foo {};
tempate <> struct Foo<int> { void bar() { thi^s; } };
}

-> const ns::Foo<int>*

which contains both the namespaces and template parameters, including cv-qualifier and is pretty concise. The only downside to Decl printing is we will lose Documentation support, I am not sure how important it is but we can easily augment the output to contain that if it turns out to be important.

This revision now requires changes to proceed.Dec 1 2020, 9:18 AM
xndcn added a comment.Dec 1 2020, 11:57 PM

Thanks, it look more clear really. I'm trying to make the hover looks like auto

xndcn updated this revision to Diff 308994.Dec 2 2020, 10:00 AM

getHoverInfo(CXXThisExpr->getType()->getPointeeType(), ...) does not output namespace scope and template parameters without specialization:

while getHoverInfo(CXXThisExpr->getType(), ...) looks weird with partial specialization:

So I did some mix here...

xndcn marked 3 inline comments as done.Dec 2 2020, 10:02 AM

Sorry for the delay here. Kadir is out on vacation.

Yikes - it's a shame reusing our existing type printing doesn't do the right thing, but injected-classname and partial specializations are indeed weird.
I'm tempted to say just to live with the "type-parameter-0-0" nonsense rather than implement the workaround, but it's up to you.

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

this needs a comment explaining why we can't reuse existing logic... as far as that can be explained.

The body here is complicated, so should probably be factored out into another function.

644

are we missing CV qualifiers?

647

getNamespaceScope isn't going to do the right thing for classes nested in classes.

However I think we *don't* want to print the scope here anyway.

Generally we just put the name in the hover. I get the argument for following auto, but auto is basically a weird historical exception (it was something like the first hover supported).
And it's clear that you need namespace qualifiers for auto (it could be anything!) but less clear you need it here as we're guaranteed to be inside the type. I'd suggest just dropping it.

667

why are we including default template param values? these are not part of the type.

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

And a test for const?

xndcn added a comment.Dec 11 2020, 2:18 AM

Sorry for the delay here. Kadir is out on vacation.

Yikes - it's a shame reusing our existing type printing doesn't do the right thing, but injected-classname and partial specializations are indeed weird.
I'm tempted to say just to live with the "type-parameter-0-0" nonsense rather than implement the workaround, but it's up to you.

Got it, thanks.
The weird string only happens in injected-classname and partial specialization, maybe we can add a PrintPolicy so that TypePrinter can handle this case? I have tried to extract template arguments by casting the injected-class to ClassTemplatePartialSpecializationDecl, and it seems work well.

Sorry for the delay here. Kadir is out on vacation.

Yikes - it's a shame reusing our existing type printing doesn't do the right thing, but injected-classname and partial specializations are indeed weird.
I'm tempted to say just to live with the "type-parameter-0-0" nonsense rather than implement the workaround, but it's up to you.

Got it, thanks.
The weird string only happens in injected-classname and partial specialization, maybe we can add a PrintPolicy so that TypePrinter can handle this case? I have tried to extract template arguments by casting the injected-class to ClassTemplatePartialSpecializationDecl, and it seems work well.

Oh, I forgot... we added declaredType() in AST.h - you pass in a TypeDecl and get out a QualType... but it should handle CTPSDecl correctly.
So something like:

Pointee = Thisexpr->getpointeetype
ClassDecl = Pointee->getAsTagDecl
ClassType = declaredType(ClassDecl)
PrettyThisType = ASTContext.getPointerType(QualType(ClassType.getTypePtr(), Pointee.getCVQualifiers()));
then render PrettyThisType?

Sorry, I didn't really address your question. I think the difficulty in making this a printingpolicy is that going from type-parameter-0-0 to the spelled parameter is going from a canonical type to its alias, and isn't really possible in the general case, just in a bunch of special cases. So it's hard to add to the API, because it's a hard promise to live up to.

xndcn updated this revision to Diff 311256.Dec 11 2020, 9:31 AM

Thank you, it works like a charm!
For class withou template, getHoverInfo(QualType ...) will add namespace scope by default, so I have to add SuppressScope printpolicy here.

xndcn marked 7 inline comments as done.Dec 11 2020, 9:33 AM
sammccall accepted this revision.Dec 15 2020, 12:44 AM

Very nice, thanks!
I'll land this for you now.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 15 2020, 12:56 AM
This revision was automatically updated to reflect the committed changes.
xndcn added a comment.Dec 15 2020, 9:50 PM

Very nice, thanks!
I'll land this for you now.

Thank you very much!
Learned a lot about clangd and clang AST from this!