[PATCH] D142014: [clangd] fix wrong CalleeArgInfo in the hover

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added inline comments.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:994
       HI.CalleeArgInfo.emplace(toHoverInfoParam(PVD, PP));
+      PassType.PassBy = getPassMode(PVD->getType());
+    }
----------------
kadircet wrote:
> v1nh1shungry wrote:
> > kadircet wrote:
> > > v1nh1shungry wrote:
> > > > kadircet wrote:
> > > > > v1nh1shungry wrote:
> > > > > > kadircet wrote:
> > > > > > > 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`)?
> > > > > > 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 :(
> > > > > > 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?
> > > > 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?
> > > 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.
> > > 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
> > 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142014/new/

https://reviews.llvm.org/D142014