[clangd] Hover: show CalleeArgInfo for literals

[tom-anders]

Summary

This is very useful when inlay hints are disabled.

[tom-anders]

clang-tools-extra/clangd/Hover.cpp

812

(I also modified this function, so I added this forward declaration for easier review.

If this patch gets accepted, I'd move the definition up to here before landing)

831

HoverInfo::present has an assertion that the Name has to be non-empty. I'm open for other name suggestions here (Or we could of course adjust HoverInfo::present instead)

[nridge]

clang-tools-extra/clangd/Hover.cpp

782

nit: "contents" seems a bit strange now that this is no longer necessarily the entirety of the hover contents (nor is it the string literal's contents)

maybe name it addStringLiteralInfo?

827

nit: for consistency with non-literal expressions, maybe the CalleeArgInfo should come after the other contents of the hover?

831

It at least seems no worse than "expression" for other expressions.

I think the expression's value would be more useful (and despite the "not much value" comment above, I think there can be value in showing this if e.g. the expression is written as hex and the hover shows you the decimal value), but that can be left for a later change.

848

Any reason not to also add CalleeArgInfo in this branch?

I know this branch is not for literals so I'm suggesting to expand the scope of the patch, feel free to leave this for later, but conceptually I don't see why the CalleeArgInfo would be any less useful for non-literal expressions that take this branch.

[tom-anders]

Refactor getHoverContents to add CalleeArgInfo for all kinds of expression

[tom-anders]

clang-tools-extra/clangd/Hover.cpp

782

Changed back to the previous signature in current patch version

831

Added a FIXME for that

848

Good point, I refactored the logic here a bit, now CalleeArgInfo is added for this branch, and also the two branches above.

[nridge]

Do you want to add a simple test case for a non-literal expression? Something like hovering over the + in 2 + 3 should work.

Also, this is pre-existing, but I noticed that in a case like this:

void bar(int);
void func() {
  int x = 42;
  bar(x);
}

the hover for x includes a line that just says Passed -- is that useful in any way? Should we just omit the CalleeArgInfo a case like that?

[tom-anders]

In D140775#4021634, @nridge wrote:

Do you want to add a simple test case for a non-literal expression? Something like hovering over the + in 2 + 3 should work.

Will do!

Also, this is pre-existing, but I noticed that in a case like this:

void bar(int);
void func() {
  int x = 42;
  bar(x);
}

the hover for x includes a line that just says Passed -- is that useful in any way? Should we just omit the CalleeArgInfo a case like that?

Hmm, the thing is that if the function signature is void bar(int&), then "Passed by reference" would be useful again, so we can't just add a if (!CalleeArgInfo->Name.empty()) around the OS << "Passed "; Maybe we could instead display "Passed by value" if and only if the name is empty?

[nridge]

In D140775#4021967, @tom-anders wrote:

In D140775#4021634, @nridge wrote:

Also, this is pre-existing, but I noticed that in a case like this:

void bar(int);
void func() {
  int x = 42;
  bar(x);
}

the hover for x includes a line that just says Passed -- is that useful in any way? Should we just omit the CalleeArgInfo a case like that?

Hmm, the thing is that if the function signature is void bar(int&), then "Passed by reference" would be useful again, so we can't just add a if (!CalleeArgInfo->Name.empty()) around the OS << "Passed "; Maybe we could instead display "Passed by value" if and only if the name is empty?

That sounds fine to me.

[tom-anders]

Add test for expression, improve presentation for signature with unnamed parameter

[nridge]

Thanks!

clang-tools-extra/clangd/Hover.cpp

HoverInfo getDeducedTypeHoverContents(QualType QT, const syntax::Token &Tok,
   }
 
   return HI;
 }
 
 HoverInfo getStringLiteralContents(const StringLiteral *SL,
                                    const PrintingPolicy &PP) {
   HoverInfo HI;
 
   HI.Name = "string-literal";
   HI.Size = (SL->getLength() + 1) * SL->getCharByteWidth();
   HI.Type = SL->getType().getAsString(PP).c_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<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<UserDefinedLiteral>(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");
 }
 
+void maybeAddCalleeArgInfo(const SelectionTree::Node *N, HoverInfo &HI,
+                           const PrintingPolicy &PP);
+
 // Generates hover info for `this` and evaluatable expressions.
 // FIXME: Support hover for literals (esp user-defined)
-std::optional<HoverInfo> getHoverContents(const Expr *E, ParsedAST &AST,
+std::optional<HoverInfo> getHoverContents(const SelectionTree::Node *N,
+                                          const Expr *E, ParsedAST &AST,
                                           const PrintingPolicy &PP,
                                           const SymbolIndex *Index) {
+  std::optional<HoverInfo> HI;
+
+  if (const StringLiteral *SL = dyn_cast<StringLiteral>(E)) {
+    // Print the type and the size for string literals
+    HI = getStringLiteralContents(SL, PP);
+  } else if (isLiteral(E)) {
     // 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))
+    // saying "42 is an int", similar for most other literals.
+    // However, if we have CalleeArgInfo, it's still useful to show it.
+    maybeAddCalleeArgInfo(N, HI.emplace(), PP);
+    if (HI->CalleeArgInfo) {
+      // FIXME Might want to show the expression's value here instead?
+      // E.g. if the literal is in hex it might be useful to show the decimal
+      // value here.
+      HI->Name = "literal";
+      return HI;
+    }
     return std::nullopt;
+  }
 
-  HoverInfo HI;
-  // Print the type and the size for string literals
-  if (const StringLiteral *SL = dyn_cast<StringLiteral>(E))
-    return getStringLiteralContents(SL, PP);
   // For `this` expr we currently generate hover with pointee type.
   if (const CXXThisExpr *CTE = dyn_cast<CXXThisExpr>(E))
-    return getThisExprHoverContents(CTE, AST.getASTContext(), PP);
+    HI = getThisExprHoverContents(CTE, AST.getASTContext(), PP);
   if (const PredefinedExpr *PE = dyn_cast<PredefinedExpr>(E))
-    return getPredefinedExprHoverContents(*PE, AST.getASTContext(), PP);
+    HI = getPredefinedExprHoverContents(*PE, AST.getASTContext(), PP);
   // For expressions we currently print the type and the value, iff it is
   // evaluatable.
   if (auto Val = printExprValue(E, AST.getASTContext())) {
-    HI.Type = printType(E->getType(), AST.getASTContext(), PP);
-    HI.Value = *Val;
-    HI.Name = std::string(getNameForExpr(E));
-    return HI;
+    HI.emplace();
+    HI->Type = printType(E->getType(), AST.getASTContext(), PP);
+    HI->Value = *Val;
+    HI->Name = std::string(getNameForExpr(E));
   }
-  return std::nullopt;
+
+  if (HI)
+    maybeAddCalleeArgInfo(N, *HI, PP);
+
+  return HI;
 }
 
 // Generates hover info for attributes.
 std::optional<HoverInfo> getHoverContents(const Attr *A, ParsedAST &AST) {
   HoverInfo HI;
   HI.Name = A->getSpelling();
   if (A->hasScope())
     HI.LocalScope = A->getScopeName()->getName().str();
void maybeAddCalleeArgInfo(const SelectionTree::Node *N, HoverInfo &HI,
 
   // If we found a matching argument, also figure out if it's a
   // [const-]reference. For this we need to walk up the AST from the arg itself
   // to CallExpr and check all implicit casts, constructor calls, etc.
   HoverInfo::PassType PassType;
   if (const auto *E = N->ASTNode.get<Expr>()) {
     if (E->getType().isConstQualified())
       PassType.PassBy = HoverInfo::PassType::ConstRef;
+
+    // No implicit node, literal passed by value
+    if (isLiteral(E) && N->Parent == OuterNode.Parent)
+      PassType.PassBy = HoverInfo::PassType::Value;
   }
 
   for (auto *CastNode = N->Parent;
        CastNode != OuterNode.Parent && !PassType.Converted;
        CastNode = CastNode->Parent) {
     if (const auto *ImplicitCast = CastNode->ASTNode.get<ImplicitCastExpr>()) {
       switch (ImplicitCast->getCastKind()) {
       case CK_NoOp:
for (auto *CastNode = N->Parent;
     } else if (const auto *CtorCall =
                    CastNode->ASTNode.get<CXXConstructExpr>()) {
       // We want to be smart about copy constructors. They should not show up as
       // type conversion, but instead as passing by value.
       if (CtorCall->getConstructor()->isCopyConstructor())
         PassType.PassBy = HoverInfo::PassType::Value;
       else
         PassType.Converted = true;
+    } else if (const auto *MTE =
+                   CastNode->ASTNode.get<MaterializeTemporaryExpr>()) {
+      // Can't bind a non-const-ref to a temporary, so has to be const-ref
+      PassType.PassBy = HoverInfo::PassType::ConstRef;
     } else { // Unknown implicit node, assume type conversion.
       PassType.PassBy = HoverInfo::PassType::Value;
       PassType.Converted = true;
     }
   }
 
   HI.CallPassType.emplace(PassType);
 }
if (const SelectionTree::Node *N = ST.commonAncestor()) {
         // Layout info only shown when hovering on the field/class itself.
         if (DeclToUse == N->ASTNode.get<Decl>())
           addLayoutInfo(*DeclToUse, *HI);
         // Look for a close enclosing expression to show the value of.
         if (!HI->Value)
           HI->Value = printExprValue(N, AST.getASTContext());
         maybeAddCalleeArgInfo(N, *HI, PP);
       } else if (const Expr *E = N->ASTNode.get<Expr>()) {
-        HI = getHoverContents(E, AST, PP, Index);
+        HI = getHoverContents(N, E, AST, PP, Index);
       } else if (const Attr *A = N->ASTNode.get<Attr>()) {
         HI = getHoverContents(A, AST);
       }
       // FIXME: support hovers for other nodes?
       //  - built-in types
     }
   }
 
if (CalleeArgInfo) {
     if (CallPassType->PassBy != HoverInfo::PassType::Value) {
       OS << "by ";
       if (CallPassType->PassBy == HoverInfo::PassType::ConstRef)
         OS << "const ";
       OS << "reference ";
     }
     if (CalleeArgInfo->Name)
       OS << "as " << CalleeArgInfo->Name;
+    else if (CallPassType->PassBy == HoverInfo::PassType::Value)
+      OS << "by value";
     if (CallPassType->Converted && CalleeArgInfo->Type)
       OS << " (converted to " << CalleeArgInfo->Type->Type << ")";
     Output.addParagraph().appendText(OS.str());
   }
 
   if (!Documentation.empty())
     parseDocumentation(Documentation, Output);
 

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

class Foo final {})cpp";
          HI.LocalScope = "code::";
          HI.Value = "2";
          HI.Type = "int";
          HI.CalleeArgInfo.emplace();
          HI.CalleeArgInfo->Name = "arg_b";
          HI.CalleeArgInfo->Type = "int &";
          HI.CallPassType = HoverInfo::PassType{PassMode::Ref, false};
        }},
+      {// Literal passed to function call
+       R"cpp(
+          void fun(int arg_a, const int &arg_b) {};
+          void code() {
+            int a = 1;
+            fun(a, [[^2]]);
+          }
+          )cpp",
+       [](HoverInfo &HI) {
+         HI.Name = "literal";
+         HI.Kind = index::SymbolKind::Unknown;
+         HI.CalleeArgInfo.emplace();
+         HI.CalleeArgInfo->Name = "arg_b";
+         HI.CalleeArgInfo->Type = "const int &";
+         HI.CallPassType = HoverInfo::PassType{PassMode::ConstRef, false};
+       }},
+      {// Expression passed to function call
+       R"cpp(
+          void fun(int arg_a, const int &arg_b) {};
+          void code() {
+            int a = 1;
+            fun(a, 1 [[^+]] 2);
+          }
+          )cpp",
+       [](HoverInfo &HI) {
+         HI.Name = "expression";
+         HI.Kind = index::SymbolKind::Unknown;
+         HI.Type = "int";
+         HI.Value = "3";
+         HI.CalleeArgInfo.emplace();
+         HI.CalleeArgInfo->Name = "arg_b";
+         HI.CalleeArgInfo->Type = "const int &";
+         HI.CallPassType = HoverInfo::PassType{PassMode::ConstRef, false};
+       }},
       {// Extra info for method call.
        R"cpp(
           class C {
            public:
             void fun(int arg_a = 3, int arg_b = 4) {}
           };
           void code() {
             int a = 1, b = 2;
)cpp";
 
   struct {
     const char *const Code;
     HoverInfo::PassType::PassMode PassBy;
     bool Converted;
   } Tests[] = {
       // Integer tests
       {"int_by_value([[^int_x]]);", PassMode::Value, false},
+      {"int_by_value([[^123]]);", PassMode::Value, false},
       {"int_by_ref([[^int_x]]);", PassMode::Ref, false},
       {"int_by_const_ref([[^int_x]]);", PassMode::ConstRef, false},
+      {"int_by_const_ref([[^123]]);", PassMode::ConstRef, false},
       {"int_by_value([[^int_ref]]);", PassMode::Value, false},
       {"int_by_const_ref([[^int_ref]]);", PassMode::ConstRef, false},
       {"int_by_const_ref([[^int_ref]]);", PassMode::ConstRef, false},
       {"int_by_const_ref([[^int_const_ref]]);", PassMode::ConstRef, false},
       // Custom class tests
       {"base_by_ref([[^base]]);", PassMode::Ref, false},
       {"base_by_const_ref([[^base]]);", PassMode::ConstRef, false},
       {"base_by_const_ref([[^base_const_ref]]);", PassMode::ConstRef, false},
       {"base_by_value([[^base]]);", PassMode::Value, false},
       {"base_by_value([[^base_const_ref]]);", PassMode::Value, false},
       {"base_by_ref([[^derived]]);", PassMode::Ref, false},
       {"base_by_const_ref([[^derived]]);", PassMode::ConstRef, false},
       {"base_by_value([[^derived]]);", PassMode::Value, false},
       // Converted tests
       {"float_by_value([[^int_x]]);", PassMode::Value, true},
       {"float_by_value([[^int_ref]]);", PassMode::Value, true},
       {"float_by_value([[^int_const_ref]]);", PassMode::Value, true},
+      {"float_by_value([[^123.0f]]);", PassMode::Value, false},
+      {"float_by_value([[^123]]);", PassMode::Value, true},
       {"custom_by_value([[^int_x]]);", PassMode::Ref, true},
       {"custom_by_value([[^float_x]]);", PassMode::Value, true},
       {"custom_by_value([[^base]]);", PassMode::ConstRef, true},
   };
   for (const auto &Test : Tests) {
     SCOPED_TRACE(Test.Code);
 
     const auto Code = (CodePrefix + Test.Code + CodeSuffix).str();

 
 // In test::Bar
 int foo = 3)",
       },
       {
           [](HoverInfo &HI) {
             HI.Kind = index::SymbolKind::Variable;
             HI.Name = "foo";
+            HI.CalleeArgInfo.emplace();
+            HI.CalleeArgInfo->Type = "int";
+            HI.CallPassType = HoverInfo::PassType{PassMode::Value, false};
+          },
+          R"(variable foo
+
+Passed by value)",
+      },
+      {
+          [](HoverInfo &HI) {
+            HI.Kind = index::SymbolKind::Variable;
+            HI.Name = "foo";
             HI.Definition = "int foo = 3";
             HI.LocalScope = "test::Bar::";
             HI.Value = "3";
             HI.Type = "int";
             HI.CalleeArgInfo.emplace();
             HI.CalleeArgInfo->Name = "arg_a";
             HI.CalleeArgInfo->Type = "int";
             HI.CalleeArgInfo->Default = "7";