[clangd] fix wrong CalleeArgInfo in the hover

[v1nh1shungry]

Summary

void foobar(int);
int main() {
  foobar(1 + 2);
           ^
}

Currently the CalleeArgInfo will be "Passed by reference", which should
be "Passed by value".

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

[nridge]

Thanks for the patch!

I had a look at the code history, and it looks like the original reason for not using the param decl's type to determine the passing mode was (based on this comment) to handle cases like this correctly:

struct C { C(int&); };
void foo(const C&);
int main() {
  int y;
  foo(y);  // want PassMode::Ref
}

However, your patch keeps this case working by keeping the check for a CXXConstructExpr and using the constructor's parameter decl instead (and this scenario has test coverage in the test Hover.CallPassType), so this seems fine to me, and indeed a nice simplification.

That said, I will cc @adamcz and @kadircet (as author and reviewer of the original patch) to see if they have any concerns about this refactoring breaking any other cases.

clang-tools-extra/clangd/Hover.cpp

955

nit: name the parameter ParmType, so it's clear whose type it's expecting

1030

Please explicily check getNumParams() before calling getParamDecl(0). (Even though the constructpr should have at least one parameter, we don't want to risk crashing in the case of invalid code or some other unexpected situation.)

[v1nh1shungry]

land comment's suggestions

[v1nh1shungry]

Thanks for the review! @nridge

[nridge]

clang-tools-extra/clangd/Hover.cpp

1030

This check should be >= 1, since the constructor could have additional parameters that have default arguments and still be used for implicit conversion

[v1nh1shungry]

== to >=

[v1nh1shungry]

Thanks!

[tom-anders]

clang-tools-extra/clangd/Hover.cpp

1035

This branch is now empty, do we still need it?

[v1nh1shungry]

clang-tools-extra/clangd/Hover.cpp

1035

I keep this branch because the original implementation doesn't want to mark Converted in this branch. Yeah I can modify the following else to else if (const ...; !MTE) (or something cleaner), but I think the two are the same ugly :(

[tom-anders]

clang-tools-extra/clangd/Hover.cpp

1035

Ah I see, what about just adding PassType.Converted = false here (even though it has no effect, but just to make this more clear)?

[v1nh1shungry]

clang-tools-extra/clangd/Hover.cpp

1035

Then how about just adding some comments? I don't have a strong opinion here though.

[v1nh1shungry]

land review's suggestions

[v1nh1shungry]

Thanks for the review! @tom-anders

clang-tools-extra/clangd/Hover.cpp

1035

Done. Sorry for the confusion here! Hope it is clear now.

[kadircet]

clang-tools-extra/clangd/Hover.cpp

994

sorry for showing up late to the party but instead of changing rest of the code, can we apply this logic only when there are no implicit casts/conversions between the arg and callexpr (i.e N == &OuterNode)?

[v1nh1shungry]

clang-tools-extra/clangd/Hover.cpp

994

To make sure I understand it correctly, do you mean I should give up any other code changes I made but keep this logic, and put this logic into the N == &OuterNode branch?

Sorry if I misunderstood anything! Shame for not being a good reader :(

[v1nh1shungry]

I just came up with a case where the original implementation and this patch behave differently.

void foobar(const float &);
int main() {
  foobar(0);
         ^
}

Used to Passed by value, now it is Passed by const reference. I think the former one is apparently better.

This case can be fixed by getting the PassType.PassBy = HoverInfo::PassType::Value; back to the ImplicitCastExpr case, but hmm...It does make me hesitate to insist on the current code change. Maybe we should switch to @kadircet's idea if I understand him correctly. WDYT, @nridge, @tom-anders and @adamcz?

But the isLiteral() and ImplicitCastExpr cases in the original implementation bring enough mystery and risks to me. For me this is a hard decision.

[kadircet]

clang-tools-extra/clangd/Hover.cpp

994

To make sure I understand it correctly, do you mean I should give up any other code changes I made but keep this logic, and put this logic into the N == &OuterNode branch?

Somewhat.

Basically this code had the assumption that if we don't see any casts/conversions between the expression creating the argument and the expression getting passed to the callexpr, it must be passed by reference, and this was wrong. Even before the patch that added handling for literals.

The rest of the handling for casts/conversions/constructors in between have been working so far and was constructed based on what each particular cast does, not for specific cases hence they're easier (for the lack of a better word) to reason about. Hence I'd rather keep them as is, especially the changes in handling MaterializeTemporaryExpr don't sound right. I can see the example you've at hand, but we shouldn't be producing "converted" results for it anyway (the AST should have a NoOp implicit cast to const int and then a MaterializeTemporaryExpr, which shouldn't generated any converted signals with the existing code already).

Hence the my proposal is getting rid of the assumption around "if we don't see any casts/conversions between the expression creating the argument and the expression getting passed to the callexpr, it must be passed by reference", and use the type information in ParmVarDecl directly when we don't have any implicit nodes in between to infer PassBy.
This should imply also getting rid of the special case for literals (if (isLiteral(E) && N->Parent == OuterNode.Parent)).

Does that make sense?

[nridge]

In D142014#4081922, @v1nh1shungry wrote:

I just came up with a case where the original implementation and this patch behave differently.

void foobar(const float &);
int main() {
  foobar(0);
         ^
}

Used to Passed by value, now it is Passed by const reference. I think the former one is apparently better.

Why is "passed by value" better?

My mental model here is that the value does not get copied; for a built-in type that may not be much of a distinction, but if you imagine generalizing to a class type constructed using a user-defined literal, it could be a noncopyable (and in C++17, even non-moveable) type that can't be passed around by value.

[v1nh1shungry]

In D142014#4084263, @nridge wrote:

In D142014#4081922, @v1nh1shungry wrote:

I just came up with a case where the original implementation and this patch behave differently.

void foobar(const float &);
int main() {
  foobar(0);
         ^
}

Used to Passed by value, now it is Passed by const reference. I think the former one is apparently better.

Why is "passed by value" better?

My mental model here is that the value does not get copied; for a built-in type that may not be much of a distinction, but if you imagine generalizing to a class type constructed using a user-defined literal, it could be a noncopyable (and in C++17, even non-moveable) type that can't be passed around by value.

Ah, I think you're right. I was confused. This case seems good to me. I guess I was thinking about

void foobar(const float &);
int main() {
  int a;
  foobar(a);
         ^
}

Used to be Passed by value, now it is Passed by const reference. I think it's kind of confusing because the local variable a isn't actually referenced, right?

clang-tools-extra/clangd/Hover.cpp

994

Thanks for the detailed explanation! But before we go ahead here, what do you think about the new test case I'm talking about above? Do you agree with my conclusion?

[kadircet]

clang-tools-extra/clangd/Hover.cpp

994

i suppose you mean:

void foobar(const float &);
int main() {
  foobar(0);
              ^
}

first of all the version of the patch that i propose doesn't involve any changes in behaviour here (as we actually have an implicit cast in between, and i am suggesting finding out passby based on type of the parmvardecl only when there are no casts in between).

i can see @nridge 's reasoning about indicating copies by saying pass by value vs ref, which unfortunately doesn't align with C++ semantics directly (as what we have here is a prvalue, and it is indeed passed by value, without any copies to the callee).

it isn't very obvious anywhere but the main functionality we wanted to provide to the developer was help them figure out if a function call can mutate a parameter they were passing in, therefore it didn't prioritise literals at all. we probably should've made better wording choices in the UI and talked about "immutability". hence from functionality point of view calling this pass by value vs const ref doesn't make a huge difference (but that's probably only in my mind and people are already using it to infer other things like whether we're going to trigger copies).

so i'd actually leave this case as-is, and think about what we're actually trying to provide by showing arg info on literals. as it's currently trying to overload the meaning of passby and causing confusions. since the initial intent was to just convey "immutability" one suggestion would be to just hide the passby information for literals.
otherwise from value categories point of view, these are always passed by value, but this is going to create confusion for people that are using it to infer "copies" and getting that right, while preserving the semantics around "is this mutable" just complicates things.

best thing moving forward would probably be to just have two separate fields, one indicating mutability and another indicating copies and not talking about pass by type at all.

---

sorry for the lengthy answer, LMK if it needs clarification.

[v1nh1shungry]

clang-tools-extra/clangd/Hover.cpp

994

hence from functionality point of view calling this pass by value vs const ref doesn't make a huge difference

Agree. But since we now choose to provide information in such a way, we should keep it as correct as we can. But for this case, I'm good with both two (indeed I now prefer value because of the prvalue, I was confused again :/ )

However, when I try to follow your proposal, I find the current code is somehow complicated for me to understand. You said we can get rid of if (isLiteral(E) && N->Parent == OuterNode.Parent), that's right, but how about the rest?

if (const auto *E = N->ASTNode.get<Expr>()) {
  if (E->getType().isConstQualified())
    PassType.PassBy = HoverInfo::PassType::ConstRef;
}

Whether it exists the tests won't break. If we remove it, the CK_NoOp cases seem to rely on the default value of HoverInfo::PassType when there's an implicit cast. If we keep this, why we're saying if the expression under the cursor is const-qualified then the PassType.PassBy will be ConstRef? What about const int?

So I think if we want to distinguish the cast case and the non-cast case, it needs a better starting point for the cast case. Or we get the PassType::PassBy through the parameter's type, which seems to be more intuitive, and then do the correction according to the cast kind if we'd like. I think it's kind of the same as the original implementation. They both are based on the cast kinds.

especially the changes in handling MaterializeTemporaryExpr don't sound right. I can see the example you've at hand, but we shouldn't be producing "converted" results for it anyway

Remove the code inside the branch and it'll behave as the same as the original implementation.

best thing moving forward would probably be to just have two separate fields, one indicating mutability and another indicating copies and not talking about pass by type at all.

+1

[kadircet]

clang-tools-extra/clangd/Hover.cpp

994

Whether it exists the tests won't break. If we remove it, the CK_NoOp cases seem to rely on the default value of HoverInfo::PassType when there's an implicit cast. If we keep this, why we're saying if the expression under the cursor is const-qualified then the PassType.PassBy will be ConstRef? What about const int?

It's unfortunate that we don't have an existing test case, but I guess something like the following should demonstrate it's need:

struct Foo {
  Foo(const int &);
};
void foo(Foo);
void bar() {
  const int x = 0;
  foo(^x);
}

In the absence of that logic, we'd say passed by reference and not by const reference.

So I think if we want to distinguish the cast case and the non-cast case, it needs a better starting point for the cast case. Or we get the PassType::PassBy through the parameter's type, which seems to be more intuitive, and then do the correction according to the cast kind if we'd like. I think it's kind of the same as the original implementation. They both are based on the cast kinds.

I think this logic is setting up the right starting point for the case when we have implicit nodes in between. We should just make sure we bail out early when we don't have any implicit nodes in between and don't execute this logic. I agree that it just complicates things for that case.

[v1nh1shungry]

clang-tools-extra/clangd/Hover.cpp

994

Thanks for the test case! Since @kadircet supposes to keep it as-is, I'm happy to follow the proposal. I'm going to restore the code change.

[v1nh1shungry]

• restore code changes
• add test cases

[kadircet]

thanks!

[v1nh1shungry]

Thanks for the review! @kadircet

Would you mind having a look at if there're any concerns about the current code change, @nridge, @tom-anders, and @adamcz? Thanks a lot!

[tom-anders]

In D142014#4106088, @v1nh1shungry wrote:

Thanks for the review! @kadircet

Would you mind having a look at if there're any concerns about the current code change, @nridge, @tom-anders, and @adamcz? Thanks a lot!

Nothing more from my side!

clang-tools-extra/clangd/Hover.cpp

if (Record && !Record->isInvalidDecl() && !Record->isDependentType() &&
       // Offset in a union is always zero, so not really useful to report.
       if (Record->isUnion())
         HI.Offset.reset();
     }
     return;
   }
 }
 
+HoverInfo::PassType::PassMode getPassMode(QualType ParmType) {
+  if (ParmType->isReferenceType()) {
+    if (ParmType->getPointeeType().isConstQualified())
+      return HoverInfo::PassType::ConstRef;
+    return HoverInfo::PassType::Ref;
+  }
+  return HoverInfo::PassType::Value;
+}
+
 // If N is passed as argument to a function, fill HI.CalleeArgInfo with
 // information about that argument.
 void maybeAddCalleeArgInfo(const SelectionTree::Node *N, HoverInfo &HI,
                            const PrintingPolicy &PP) {
   const auto &OuterNode = N->outerImplicit();
   if (!OuterNode.Parent)
     return;
   const auto *CE = OuterNode.Parent->ASTNode.get<CallExpr>();
   if (!CE)
     return;
   const FunctionDecl *FD = CE->getDirectCallee();
   // For non-function-call-like operatators (e.g. operator+, operator<<) it's
   // not immediattely obvious what the "passed as" would refer to and, given
   // fixed function signature, the value would be very low anyway, so we choose
   // to not support that.
   // Both variadic functions and operator() (especially relevant for lambdas)
   // should be supported in the future.
   if (!FD || FD->isOverloadedOperator() || FD->isVariadic())
     return;
 
+  HoverInfo::PassType PassType;
+
   // Find argument index for N.
   for (unsigned I = 0; I < CE->getNumArgs() && I < FD->getNumParams(); ++I) {
     if (CE->getArg(I) != OuterNode.ASTNode.get<Expr>())
       continue;
 
     // Extract matching argument from function declaration.
-    if (const ParmVarDecl *PVD = FD->getParamDecl(I))
+    if (const ParmVarDecl *PVD = FD->getParamDecl(I)) {
       HI.CalleeArgInfo.emplace(toHoverInfoParam(PVD, PP));
+      PassType.PassBy = getPassMode(PVD->getType());
+    }
     break;
   }
   if (!HI.CalleeArgInfo)
     return;
 
   // 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:
       case CK_DerivedToBase:
       case CK_UncheckedDerivedToBase:
-        // If it was a reference before, it's still a reference.
-        if (PassType.PassBy != HoverInfo::PassType::Value)
-          PassType.PassBy = ImplicitCast->getType().isConstQualified()
-                                ? HoverInfo::PassType::ConstRef
-                                : HoverInfo::PassType::Ref;
-        break;
       case CK_LValueToRValue:
       case CK_ArrayToPointerDecay:
       case CK_FunctionToPointerDecay:
       case CK_NullToPointer:
       case CK_NullToMemberPointer:
-        // No longer a reference, but we do not show this as type conversion.
-        PassType.PassBy = HoverInfo::PassType::Value;
         break;
       default:
-        PassType.PassBy = HoverInfo::PassType::Value;
         PassType.Converted = true;
         break;
       }
     } 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())
+      const CXXConstructorDecl *CD = CtorCall->getConstructor();
+      if (CD->isCopyConstructor()) {
         PassType.PassBy = HoverInfo::PassType::Value;
-      else
+      } else {
+        if (CD->getNumParams() >= 1)
+          PassType.PassBy = getPassMode(CD->getParamDecl(0)->getType());
         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);
 }
 
 const NamedDecl *pickDeclToUse(llvm::ArrayRef<const NamedDecl *> Candidates) {
const NamedDecl *pickDeclToUse(llvm::ArrayRef<const NamedDecl *> Candidates) {
   }
 
   // For something like
   //     namespace ns { void foo(int); void foo(char); }
   //     using ns::foo;
   //     template <typename T> void bar() { fo^o(T{}); }
   // we actually want to show the using declaration,
   // it's not clear which declaration to pick otherwise.
-  auto BaseDecls = llvm::make_filter_range(Candidates, [](const NamedDecl *D) {
-    return llvm::isa<UsingDecl>(D);
-  });
+  auto BaseDecls = llvm::make_filter_range(
+      Candidates, [](const NamedDecl *D) { return llvm::isa<UsingDecl>(D); });
   if (std::distance(BaseDecls.begin(), BaseDecls.end()) == 1)
     return *BaseDecls.begin();
 
   return Candidates.front();
 }
 
 } // namespace
 

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

class Foo final {})cpp";
          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};
        }},
+      {
+          R"cpp(
+        int add(int lhs, int rhs);
+        int main() {
+          add(1 [[^+]] 2, 3);
+        }
+        )cpp",
+          [](HoverInfo &HI) {
+            HI.Name = "expression";
+            HI.Kind = index::SymbolKind::Unknown;
+            HI.Type = "int";
+            HI.Value = "3";
+            HI.CalleeArgInfo.emplace();
+            HI.CalleeArgInfo->Name = "lhs";
+            HI.CalleeArgInfo->Type = "int";
+            HI.CallPassType = HoverInfo::PassType{PassMode::Value, 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;
const std::function<void(HoverInfo &)> ExpectedBuilder;
             HI.Name = "foo";
             HI.Kind = index::SymbolKind::Variable;
             HI.NamespaceScope = "ns::";
             HI.Type = "int";
             HI.Definition = "int foo";
           }},
       {
           R"cpp(// Function definition via using declaration
-            namespace ns { 
-              void foo(); 
+            namespace ns {
+              void foo();
             }
             int main() {
               using ns::foo;
               ^[[foo]]();
             }
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "foo";