[clangd] Show hover info for expressions

[kadircet]

Summary

This currently populates only the Name with the expression's type and
Value if expression is evaluatable.

Fixes https://github.com/clangd/clangd/issues/56

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

[sammccall]

Basing this on the hover for the type doesn't seem right. e.g. int should be the Type rather than the Name.

Rather than printing the value if evaluable, I think we should only show the hover if evaluable. There's a cost to showing it and the value of just the type doesn't seem clearly high enough.

I think we should avoid triggering for literals. Maybe some exceptions, but a hover saying that 0 is an int with value 0 seems silly.

[lh123]

In D72500#1813975, @sammccall wrote:

I think we should avoid triggering for literals. Maybe some exceptions, but a hover saying that 0 is an int with value 0 seems silly.

Hovering over IntegerLiteral/FloatingLiteral may be useless, but I think it's useful when hovering over StringLiteral/UserDefinedLiteral/CXXNullPtrLiteralExpr ....

• "hello" -> char [6].
• nullptr -> std::nullptr_t.
• 1i -> std::complex<double>.

[kadircet]

In D72500#1813975, @sammccall wrote:

Basing this on the hover for the type doesn't seem right. e.g. int should be the Type rather than the Name.

Rather than printing the value if evaluable, I think we should only show the hover if evaluable. There's a cost to showing it and the value of just the type doesn't seem clearly high enough.

I think we should avoid triggering for literals. Maybe some exceptions, but a hover saying that 0 is an int with value 0 seems silly.

agreed.

In D72500#1815780, @lh123 wrote:

In D72500#1813975, @sammccall wrote:

I think we should avoid triggering for literals. Maybe some exceptions, but a hover saying that 0 is an int with value 0 seems silly.

Hovering over IntegerLiteral/FloatingLiteral may be useless, but I think it's useful when hovering over StringLiteral/UserDefinedLiteral/CXXNullPtrLiteralExpr ....

• "hello" -> char [6].
• nullptr -> std::nullptr_t.
• 1i -> std::complex<double>.

I believe all but the first case seems redundant. So I am only keeping the StringLiterals and dropping the rest.

[kadircet]

• Ignore literals

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

[kadircet]

• Update presentation for expressions, which doesn't have Name field set.

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

[kadircet]

ping

[sammccall]

clang-tools-extra/clangd/Hover.cpp

421

I'd really like to avoid *any literals here in the initial patch.

While there may be value in hover to see string length, I'm not sure the generic expr display code (which happens to show a type, which happens to include length) is the best way to get at this - seems pretty clunky.

(Similarly I think there's a case for UDLs, maybe showing numbers in different bases etc - but that's not the central point of this patch, so let's not try to work out exactly where the line is)

428

you could extract a isLiteral(StmtClass) function here and dispense with the second half of this comment, if you like.

485

I don't think it's a good idea to try to infer here what's being hovered on from which fields happen to be set, or to bail out assuming there's no documentation etc. (e.g it would be pretty reasonable for a UDL to return the doc of the operator as doc).

I'm also not sure abandoning the standard layout of properties is such a good idea, vs e.g.

---

Binary expression

Type: int
Value = 4

---

which would be consistent with the others (and simpler here).
We could always use "Expression" or "BinaryOperator" (from StmtClass) for now. Or maybe

---

Expression: int

Value = 4

---

Which still requires some special-casing, but less.

[kadircet]

• Populate Name for expressions
• Don't special case StringLiterals

[merge_guards_bot]

Unit tests: pass. 61794 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[sammccall]

LG!

clang-tools-extra/clangd/Hover.cpp

484

nit: this is just "expression"

[kadircet]

clang-tools-extra/clangd/Hover.cpp

484

not yet, it will be after D72623 lands.

[thakis]

This breaks tests on Windows: http://45.33.8.238/win/5837/step_9.txt

clang-tools-extra/clangd/Hover.cpp

 #include "Selection.h"
 #include "SourceCode.h"
 #include "index/SymbolCollector.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/Expr.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/Type.h"
 #include "clang/Index/IndexSymbol.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/raw_ostream.h"
if (!Invalid) {
         HI.Definition =
             ("#define " + Buffer.substr(StartOffset, EndOffset - StartOffset))
                 .str();
     }
   }
   return HI;
 }
 } // namespace
 
 llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
                                    format::FormatStyle Style,
                                    const SymbolIndex *Index) {
   const SourceManager &SM = AST.getSourceManager();
   llvm::Optional<HoverInfo> HI;
   SourceLocation SourceLocationBeg = SM.getMacroArgExpandedLocation(
       getBeginningOfIdentifier(Pos, SM, AST.getLangOpts()));
 
   if (auto Deduced = getDeducedType(AST.getASTContext(), SourceLocationBeg)) {
     HI = getHoverContents(*Deduced, AST.getASTContext(), Index);
   } else if (auto M = locateMacroAt(SourceLocationBeg, AST.getPreprocessor())) {
     HI = getHoverContents(*M, AST);
   } else {
     auto Offset = positionToOffset(SM.getBufferData(SM.getMainFileID()), Pos);
     if (!Offset) {
       llvm::consumeError(Offset.takeError());
       return llvm::None;
     }
     SelectionTree Selection(AST.getASTContext(), AST.getTokens(), *Offset);
     std::vector<const Decl *> Result;
     if (const SelectionTree::Node *N = Selection.commonAncestor()) {
       auto Decls = explicitReferenceTargets(N->ASTNode, DeclRelation::Alias);
       if (!Decls.empty()) {
         HI = getHoverContents(Decls.front(), Index);
         // Look for a close enclosing expression to show the value of.
         if (!HI->Value)
           HI->Value = printExprValue(N, AST.getASTContext());
+      } else if (const Expr *E = N->ASTNode.get<Expr>()) {
+        // For expressions we currently print the type and the value, if they
+        // are evaluatable.
+        HI = getHoverContents(E->getType(), AST.getASTContext(), Index);
+        HI->Value = printExprValue(E, AST.getASTContext());
       }
       // FIXME: support hovers for other nodes?
-      //  - certain expressions (sizeof etc)
       //  - built-in types
       //  - literals (esp user-defined)
     }
   }
 
   if (!HI)
     return llvm::None;
 
markup::Document HoverInfo::present() const {
   markup::Document Output;
   // Header contains a text of the form:
   // variable `var` : `int`
   //
   // class `X`
   //
   // function `foo` → `int`
   markup::Paragraph &Header = Output.addParagraph();
   Header.appendText(index::getSymbolKindString(Kind));
   assert(!Name.empty() && "hover triggered on a nameless symbol");
   Header.appendCode(Name);
   if (ReturnType) {
     Header.appendText("→");
     Header.appendCode(*ReturnType);
   } else if (Type) {
     Header.appendText(":");
     Header.appendCode(*Type);
   }

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

const std::function<void(HoverInfo &)> ExpectedBuilder;
           decltype(a) [[b^]] = a;)cpp",
           [](HoverInfo &HI) {
             HI.Definition = "decltype(a) b = a";
             HI.Kind = index::SymbolKind::Variable;
             HI.NamespaceScope = "";
             HI.Name = "b";
             HI.Type = "int";
           }},
+      {
+          R"cpp(// sizeof expr
+          void foo() {
+            (void)[[size^of]](char);
+          })cpp",
+          [](HoverInfo &HI) {
+            HI.Name = "unsigned long";
+            HI.Value = "1";
+          }},
+      {
+          R"cpp(// alignof expr
+          void foo() {
+            (void)[[align^of]](char);
+          })cpp",
+          [](HoverInfo &HI) {
+            HI.Name = "unsigned long";
+            HI.Value = "1";
+          }},
+      {
+          R"cpp(// non-evaluatable expr
+          template <typename T> void foo() {
+            (void)[[size^of]](T);
+          })cpp",
+          [](HoverInfo &HI) { HI.Name = "unsigned long"; }},
   };
 
   // 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 =