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

522

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

519

nit: this is just "expression"

[kadircet]

clang-tools-extra/clangd/Hover.cpp

519

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

 //===--- Hover.cpp - Information about code at the cursor location --------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Hover.h"
 
 #include "AST.h"
 #include "CodeCompletionStrings.h"
 #include "FindTarget.h"
 #include "FormattedString.h"
 #include "Logger.h"
+#include "ParsedAST.h"
 #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/ExprCXX.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/Type.h"
 #include "clang/Index/IndexSymbol.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/raw_ostream.h"
 #include <string>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 PrintingPolicy printingPolicyForDecls(PrintingPolicy Base) {
if (!Invalid) {
       if (EndOffset <= Buffer.size() && StartOffset < EndOffset)
         HI.Definition =
             ("#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 this is a nasty blacklist.
+  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.
+  return "expression";
+}
+
+// Generates hover info for evaluatable expressions.
+// FIXME: Support hover for literals (esp user-defined)
+llvm::Optional<HoverInfo> getHoverContents(const Expr *E, ParsedAST &AST) {
+  // 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 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 = E->getType().getAsString(Policy);
+    HI.Value = *Val;
+    HI.Name = getNameForExpr(E);
+    return HI;
+  }
+  return llvm::None;
+}
 } // 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(
if (auto Deduced = getDeducedType(AST.getASTContext(), SourceLocationBeg)) {
     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>()) {
+        HI = getHoverContents(E, AST);
       }
       // FIXME: support hovers for other nodes?
-      //  - certain expressions (sizeof etc)
       //  - built-in types
-      //  - literals (esp user-defined)
     }
   }
 
   if (!HI)
     return llvm::None;
 
   auto Replacements = format::reformat(
       Style, HI->Definition, tooling::Range(0, HI->Definition.size()));

 markup::Document HoverInfo::present() const {
   markup::Document Output;
   // Header contains a text of the form:
   // variable `var` : `int`
   //
   // class `X`
   //
   // function `foo` → `int`
+  //
+  // expression : `int`
   // Note that we are making use of a level-3 heading because VSCode renders
   // level 1 and 2 headers in a huge font, see
   // https://github.com/microsoft/vscode/issues/88417 for details.
   markup::Paragraph &Header = Output.addHeading(3);
   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) {

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

llvm::StringRef Tests[] = {
             }
             void foo() {
                func<1>();
             }
           )cpp",
       R"cpp(// non-named decls don't get hover. Don't crash!
             ^static_assert(1, "");
           )cpp",
+      R"cpp(// non-evaluatable expr
+          template <typename T> void foo() {
+            (void)[[size^of]](T);
+          })cpp",
+      // literals
+      "auto x = t^rue;",
+      "auto x = '^A';",
+      "auto x = ^(int){42};",
+      "auto x = ^42.;",
+      "auto x = ^42.0i;",
+      "auto x = ^42;",
+      "auto x = ^nullptr;",
+      "auto x = ^\"asdf\";",
   };
 
   for (const auto &Test : Tests) {
     SCOPED_TRACE(Test);
 
     Annotations T(Test);
     TestTU TU = TestTU::withCode(T.code());
     TU.ExtraArgs.push_back("-std=c++17");
const std::function<void(HoverInfo &)> ExpectedBuilder;
           )cpp",
           [](HoverInfo &HI) {
             HI.Definition = "template <> struct cls<cls<cls<int>>> {}";
             HI.Kind = index::SymbolKind::Struct;
             HI.NamespaceScope = "";
             HI.Name = "cls<cls<cls<int> > >";
             HI.Documentation = "type of nested templates.";
           }},
+      {
+          R"cpp(// sizeof expr
+          void foo() {
+            (void)[[size^of]](char);
+          })cpp",
+          [](HoverInfo &HI) {
+            HI.Name = "expression";
+            HI.Type = "unsigned long";
+            HI.Value = "1";
+          }},
+      {
+          R"cpp(// alignof expr
+          void foo() {
+            (void)[[align^of]](char);
+          })cpp",
+          [](HoverInfo &HI) {
+            HI.Name = "expression";
+            HI.Type = "unsigned long";
+            HI.Value = "1";
+          }},
   };
 
   // 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 =