[clangd] Add hover info for `this` expr

[xndcn]

Summary

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

[kadircet]

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]

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]

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.

[xndcn]

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]

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

[kadircet]

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]

Thanks! Update the commit as review comments.

[kadircet]

thanks, LGTM!

clang-tools-extra/clangd/Hover.cpp

644

nit: return getHoverContents(D, Index);

[xndcn]

Thanks. Update commit to fix the last nit.

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

[kadircet]

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

[xndcn]

In D92041#2420251, @kadircet wrote:

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!

[kadircet]

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

[njames93]

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

[xndcn]

In D92041#2424869, @kadircet wrote:

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

[e-mail], thank you.

[kadircet]

In D92041#2424928, @njames93 wrote:

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.

[xndcn]

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

[xndcn]

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...

[sammccall]

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]

In D92041#2446366, @sammccall wrote:

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.

[sammccall]

In D92041#2448176, @xndcn wrote:

In D92041#2446366, @sammccall wrote:

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?

[sammccall]

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]

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.

[sammccall]

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

[xndcn]

In D92041#2454236, @sammccall wrote:

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

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

clang-tools-extra/clangd/Hover.cpp

if (!Invalid) {
             ("#define " + Buffer.substr(StartOffset, EndOffset - StartOffset))
                 .str();
     }
   }
   return HI;
 }
 
 bool isLiteral(const Expr *E) {
   // Unfortunately there's no common base Literal classes inherits from
   // (apart from Expr), therefore these exclusions.
   return llvm::isa<CharacterLiteral>(E) || llvm::isa<CompoundLiteralExpr>(E) ||
          llvm::isa<CXXBoolLiteralExpr>(E) ||
          llvm::isa<CXXNullPtrLiteralExpr>(E) ||
          llvm::isa<FixedPointLiteral>(E) || llvm::isa<FloatingLiteral>(E) ||
          llvm::isa<ImaginaryLiteral>(E) || llvm::isa<IntegerLiteral>(E) ||
          llvm::isa<StringLiteral>(E) || llvm::isa<UserDefinedLiteral>(E);
 }
 
 llvm::StringLiteral getNameForExpr(const Expr *E) {
   // FIXME: Come up with names for `special` expressions.
   //
   // It's an known issue for GCC5, https://godbolt.org/z/Z_tbgi. Work around
   // that by using explicit conversion constructor.
   //
   // TODO: Once GCC5 is fully retired and not the minimal requirement as stated
   // in `GettingStarted`, please remove the explicit conversion constructor.
   return llvm::StringLiteral("expression");
 }
 
-// Generates hover info for evaluatable expressions.
+// Generates hover info for `this` and evaluatable expressions.
 // FIXME: Support hover for literals (esp user-defined)
-llvm::Optional<HoverInfo> getHoverContents(const Expr *E, ParsedAST &AST) {
+llvm::Optional<HoverInfo> getHoverContents(const Expr *E, ParsedAST &AST,
+                                           const SymbolIndex *Index) {
   // There's not much value in hovering over "42" and getting a hover card
   // saying "42 is an int", similar for other literals.
   if (isLiteral(E))
     return llvm::None;
 
   HoverInfo HI;
+  // For `this` expr we currently generate hover with class declaration.
+  if (const CXXThisExpr *CTE = dyn_cast<CXXThisExpr>(E)) {
+    const NamedDecl *D = CTE->getType()->getPointeeType()->getAsTagDecl();
+    return getHoverContents(D, Index);
+  }
   // For expressions we currently print the type and the value, iff it is
   // evaluatable.
   if (auto Val = printExprValue(E, AST.getASTContext())) {
     auto Policy =
         printingPolicyForDecls(AST.getASTContext().getPrintingPolicy());
     Policy.SuppressTagKeyword = true;
     HI.Type = printType(E->getType(), Policy);
     HI.Value = *Val;
     HI.Name = std::string(getNameForExpr(E));
     return HI;
   }
   return llvm::None;
 }
 
 bool isParagraphBreak(llvm::StringRef Rest) {
   return Rest.ltrim(" \t").startswith("\n");
 }
 
 bool punctuationIndicatesLineBreak(llvm::StringRef Line) {
   constexpr llvm::StringLiteral Punctuation = R"txt(.:,;!?)txt";
 
   Line = Line.rtrim();
   return !Line.empty() && Punctuation.contains(Line.back());
 }
 
 bool isHardLineBreakIndicator(llvm::StringRef Rest) {
   // '-'/'*' md list, '@'/'\' documentation command, '>' md blockquote,
   // '#' headings, '`' code blocks
   constexpr llvm::StringLiteral LinebreakIndicators = R"txt(-*@\>#`)txt";
 
if (const SelectionTree::Node *N = ST.commonAncestor()) {
         // Layout info only shown when hovering on the field/class itself.
         if (Decls.front() == N->ASTNode.get<Decl>())
           addLayoutInfo(*Decls.front(), *HI);
         // Look for a close enclosing expression to show the value of.
         if (!HI->Value)
           HI->Value = printExprValue(N, AST.getASTContext());
         maybeAddCalleeArgInfo(N, *HI, AST.getASTContext().getPrintingPolicy());
       } else if (const Expr *E = N->ASTNode.get<Expr>()) {
-        HI = getHoverContents(E, AST);
+        HI = getHoverContents(E, AST, Index);
       }
       // FIXME: support hovers for other nodes?
       //  - built-in types
     }
   }
 
   if (!HI)
     return llvm::None;

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

const std::function<void(HoverInfo &)> ExpectedBuilder;
           @end
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "MYObject";
             HI.Kind = index::SymbolKind::Class;
             HI.NamespaceScope = "";
             HI.Definition = "@interface MYObject\n@end";
           }},
+      {
+          R"cpp(// this expr
+          namespace ns {
+            // comment
+            class Foo {
+              Foo* bar() {
+                return [[t^his]];
+              }
+            };
+          }
+          )cpp",
+          [](HoverInfo &HI) {
+            HI.Name = "Foo";
+            HI.Kind = index::SymbolKind::Class;
+            HI.NamespaceScope = "ns::";
+            HI.Definition = "class Foo {}";
+            HI.Documentation = "comment";
+          }},
+      {
+          R"cpp(// this expr for template class
+          namespace ns {
+            template <typename T>
+            class Foo {
+              Foo* bar() {
+                return [[t^his]];
+              }
+            };
+          }
+          )cpp",
+          [](HoverInfo &HI) {
+            HI.Name = "Foo";
+            HI.Kind = index::SymbolKind::Class;
+            HI.NamespaceScope = "ns::";
+            HI.Definition = "template <typename T> class Foo {}";
+            HI.TemplateParameters = {
+                {std::string("typename"), std::string("T"), llvm::None}};
+          }},
+      {
+          R"cpp(// this expr for specialization struct
+          namespace ns {
+            template <typename T> struct Foo {};
+            template <>
+            struct Foo<int> {
+              Foo* bar() {
+                return [[thi^s]];
+              }
+            };
+          }
+          )cpp",
+          [](HoverInfo &HI) {
+            HI.Name = "Foo<int>";
+            HI.Kind = index::SymbolKind::Struct;
+            HI.NamespaceScope = "ns::";
+            HI.Definition = "template <> struct Foo<int> {}";
+          }},
   };
 
   // Create a tiny index, so tests above can verify documentation is fetched.
   Symbol IndexSym = func("indexSymbol");
   IndexSym.Documentation = "comment from index";
   SymbolSlab::Builder Symbols;
   Symbols.insert(IndexSym);
   auto Index =