[clangd] Add inlay hints for mutable reference parameters

[upsj]

Summary

Add a & prefix to all parameter inlay hints that refer to a non-const l-value reference. That makes it easier to identify them even if semantic highlighting is not used (where this is already available)

[nridge]

Thanks for working on this!

Could you add some test cases to InlayHintTests.cpp to illustrate the new hint please?

As in the other patch, uploading the patch with context would also be helpful for review.

[upsj]

add tests for reference inlay hints

[upsj]

clang-tools-extra/clangd/InlayHints.cpp

444–446

It probably doesn't make too much sense to highlight r-value references here, since it should be much clearer from context that they are either temporaries or explicitly std::move'd

[upsj]

don't add reference inlay hints for r-value refs

[nridge]

Apologies for the slow response here. This generally looks good to me, but I have a couple of suggestions regarding the formatting of the hints and handling type aliases.

clang-tools-extra/clangd/InlayHints.cpp

444

We should make sure we handle the case where a parameter is a typedef to a reference type, e.g.:

using IntRef = int&;
void foo(IntRef);
int main() {
  int arg;
  foo(arg);  // should show `&: ` hint
}

Let's add the testcase for this, and if necessary adjust the code to handle it (it's not immediately obvious to me whether it already handles it as written).

450

nit: shouldHintName would make more sense to me as a revised method name

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

217

I prefer for the hint to continue to end with a space, to create visual separation between the hint and the actual code.

Especially in a case like this where a hint ends in &, it may be difficult to visually distinguish this from the & occurring in the actual code.

To that end, I think the hint would make more sense as param&: or &param: .

395

Similarly, and for consistency with other parameter hints, &: would make sense to me here.

[upsj]

address review comments: Move reference hint to the prefix, test type aliases

[upsj]

Thanks, on second thought moving the hint to the prefix also makes more sense to me.

[nridge]

Thanks, the changes here look good.

Something is strange about the diff though, it's not letting me see the complete changes relative to the baseline, only the incremental changes relative to a previous version (I think this is also why Phabricator's build status says patch application failed)

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

202

For good measure, let's add an alias = const int& case (expecting no &) as well

[upsj]

right, arcanist doesn't entirely match my git mental model. Here's the (hopefully) complete patch

[upsj]

add test for const ref inlay hint via type alias

[nridge]

Thanks!

Final nit: please update the commit message as it's a bit out of date, and then I'll go ahead and merge

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

168

nit: the annotation can be removed

[upsj]

remove unnecessary annotation

clang-tools-extra/clangd/InlayHints.cpp

 //===--- InlayHints.cpp ------------------------------------------*- C++-*-===//
 //
 // 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 "InlayHints.h"
 #include "AST.h"
 #include "Config.h"
 #include "HeuristicResolver.h"
 #include "ParsedAST.h"
+#include "clang/AST/Decl.h"
 #include "clang/AST/DeclarationName.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceManager.h"
 #include "llvm/ADT/ScopeExit.h"
 
 namespace clang {
 namespace clangd {
void processCall(SourceLocation Anchor, const FunctionDecl *Callee,
     // The parameter name of a move or copy constructor is not very interesting.
     if (auto *Ctor = dyn_cast<CXXConstructorDecl>(Callee))
       if (Ctor->isCopyOrMoveConstructor())
         return;
 
     // Don't show hints for variadic parameters.
     size_t FixedParamCount = getFixedParamCount(Callee);
     size_t ArgCount = std::min(FixedParamCount, Args.size());
+    auto Params = Callee->parameters();
 
     NameVec ParameterNames = chooseParameterNames(Callee, ArgCount);
 
     // Exclude setters (i.e. functions with one argument whose name begins with
     // "set"), as their parameter name is also not likely to be interesting.
     if (isSetter(Callee, ParameterNames))
       return;
 
     for (size_t I = 0; I < ArgCount; ++I) {
       StringRef Name = ParameterNames[I];
-      if (!shouldHint(Args[I], Name))
-        continue;
+      bool NameHint = shouldHintName(Args[I], Name);
+      bool ReferenceHint = shouldHintReference(Params[I]);
 
+      if (NameHint || ReferenceHint) {
         addInlayHint(Args[I]->getSourceRange(), HintSide::Left,
-                   InlayHintKind::ParameterHint, /*Prefix=*/"", Name,
-                   /*Suffix=*/": ");
+                     InlayHintKind::ParameterHint, ReferenceHint ? "&" : "",
+                     NameHint ? Name : "", ": ");
+      }
     }
   }
 
   static bool isSetter(const FunctionDecl *Callee, const NameVec &ParamNames) {
     if (ParamNames.size() != 1)
       return false;
 
     StringRef Name = getSimpleName(*Callee);
static bool isSetter(const FunctionDecl *Callee, const NameVec &ParamNames) {
     // and functions don't, e.g.
     //   void setExceptionHandler(EHFunc exception_handler);
     // We could improve this by replacing `equals_insensitive` with some
     // `sloppy_equals` which ignores case and also skips underscores.
     StringRef WhatItIsSetting = Name.substr(3).ltrim("_");
     return WhatItIsSetting.equals_insensitive(ParamNames[0]);
   }
 
-  bool shouldHint(const Expr *Arg, StringRef ParamName) {
+  bool shouldHintName(const Expr *Arg, StringRef ParamName) {
     if (ParamName.empty())
       return false;
 
     // If the argument expression is a single name and it matches the
-    // parameter name exactly, omit the hint.
+    // parameter name exactly, omit the name hint.
     if (ParamName == getSpelledIdentifier(Arg))
       return false;
 
     // Exclude argument expressions preceded by a /*paramName*/.
     if (isPrecededByParamNameComment(Arg, ParamName))
       return false;
 
     return true;
   }
 
+  bool shouldHintReference(const ParmVarDecl *Param) {
+    // If the parameter is a non-const reference type, print an inlay hint
+    auto Type = Param->getType();
+    return Type->isLValueReferenceType() &&
+           !Type.getNonReferenceType().isConstQualified();
+  }
+
   // Checks if "E" is spelled in the main file and preceded by a C-style comment
   // whose contents match ParamName (allowing for whitespace and an optional "="
   // at the end.
   bool isPrecededByParamNameComment(const Expr *E, StringRef ParamName) {
     auto &SM = AST.getSourceManager();
     auto ExprStartLoc = SM.getTopMacroCallerLoc(E->getBeginLoc());
     auto Decomposed = SM.getDecomposedLoc(ExprStartLoc);
     if (Decomposed.first != MainFileID)
NameVec getParameterNamesForDecl(const FunctionDecl *Function,
     for (size_t I = 0; I < ArgCount; ++I) {
       const ParmVarDecl *Parm = Function->getParamDecl(I);
       assert(Parm);
       Result.emplace_back(getSimpleName(*Parm));
     }
     return Result;
   }
 
-  // We pass HintSide rather than SourceLocation because we want to ensure 
+  // We pass HintSide rather than SourceLocation because we want to ensure
   // it is in the same file as the common file range.
   void addInlayHint(SourceRange R, HintSide Side, InlayHintKind Kind,
                     llvm::StringRef Prefix, llvm::StringRef Label,
                     llvm::StringRef Suffix) {
     // We shouldn't get as far as adding a hint if the category is disabled.
     // We'd like to disable as much of the analysis as possible above instead.
     // Assert in debug mode but add a dynamic check in production.
     assert(Cfg.InlayHints.Enabled && "Shouldn't get here if disabled!");

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

TEST(ParameterHints, NoName) {
   assertParameterHints(R"cpp(
     void foo(int);
     void bar() {
       foo(42);
     }
   )cpp");
 }
 
+TEST(ParameterHints, NoNameConstReference) {
+  // No hint for anonymous const l-value ref parameter.
+  assertParameterHints(R"cpp(
+    void foo(const int&);
+    void bar() {
+      foo(42);
+    }
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameReference) {
+  // Reference hint for anonymous l-value ref parameter.
+  assertParameterHints(R"cpp(
+    void foo(int&);
+    void bar() {
+      int i;
+      foo($param[[i]]);
+    }
+  )cpp",
+                       ExpectedHint{"&: ", "param"});
+}
+
+TEST(ParameterHints, NoNameRValueReference) {
+  // No reference hint for anonymous r-value ref parameter.
+  assertParameterHints(R"cpp(
+    void foo(int&&);
+    void bar() {
+      foo($param[[42]]);
+    }
+  )cpp");
+}
+
 TEST(ParameterHints, NameInDefinition) {
   // Parameter name picked up from definition if necessary.
   assertParameterHints(R"cpp(
     void foo(int);
     void bar() {
       foo($param[[42]]);
     }
     void foo(int param) {};
   )cpp",
                        ExpectedHint{"param: ", "param"});
 }
 
 TEST(ParameterHints, NameMismatch) {
   // Prefer name from declaration.
   assertParameterHints(R"cpp(
     void foo(int good);
     void bar() {
       foo($good[[42]]);
     }
     void foo(int bad) {};
   )cpp",
                        ExpectedHint{"good: ", "good"});
 }
 
+TEST(ParameterHints, NameConstReference) {
+  // Only name hint for const l-value ref parameter.
+  assertParameterHints(R"cpp(
+    void foo(const int& param);
+    void bar() {
+      foo($param[[42]]);
+    }
+  )cpp",
+                       ExpectedHint{"param: ", "param"});
+}
+
+TEST(ParameterHints, NameReference) {
+  // Reference and name hint for l-value ref parameter.
+  assertParameterHints(R"cpp(
+    void foo(int& param);
+    void bar() {
+      int i;
+      foo($param[[i]]);
+    }
+  )cpp",
+                       ExpectedHint{"&param: ", "param"});
+}
+
+TEST(ParameterHints, NameTypeAliasReference) {
+  // Reference and name hint for l-value ref parameter via type alias.
+  assertParameterHints(R"cpp(
+    using alias = int&;
+    void foo(alias param);
+    void bar() {
+      int i;
+      foo($param[[i]]);
+    }
+  )cpp",
+                       ExpectedHint{"&param: ", "param"});
+}
+
+TEST(ParameterHints, NameRValueReference) {
+  // Only name hint for r-value ref parameter.
+  assertParameterHints(R"cpp(
+    void foo(int&& param);
+    void bar() {
+      foo($param[[42]]);
+    }
+  )cpp",
+                       ExpectedHint{"param: ", "param"});
+}
+
 TEST(ParameterHints, Operator) {
   // No hint for operator call with operator syntax.
   assertParameterHints(R"cpp(
     struct S {};
     void operator+(S lhs, S rhs);
     void bar() {
       S a, b;
       a + b;
struct A {
         // Do not show "param: param" for member-expr.
         foo(param);
       }
     };
   )cpp",
                        ExpectedHint{"param: ", "param"});
 }
 
+TEST(ParameterHints, ArgMatchesParamReference) {
+  assertParameterHints(R"cpp(
+    void foo(int& param);
+    void foo2(const int& param);
+    void bar() {
+      int param;
+      // show reference hint on mutable reference
+      foo($param[[param]]);
+      // but not on const reference
+      foo2(param);
+    }
+  )cpp",
+                       ExpectedHint{"&: ", "param"});
+}
+
 TEST(ParameterHints, LeadingUnderscore) {
   assertParameterHints(R"cpp(
     void foo(int p1, int _p2, int __p3);
     void bar() {
       foo($p1[[41]], $p2[[42]], $p3[[43]]);
     }
   )cpp",
                        ExpectedHint{"p1: ", "p1"}, ExpectedHint{"p2: ", "p2"},