[clangd] Omit parameter hint for setter functions

[nridge]

I chose to be conservative here and only omit the hint in the case where the setter's name (after the "set") matches the parameter name (ignoring case).

I gave void setTimeout(int timeoutMillis); as a motivating example for where a non-matching parameter name could contain extra information that could be useful to show in a hint. Let me know if you agree with this reasoning.

[njames93]

I'm not sure how related this is, but Hover has methods that try to detect setter functions in class to give them a trivial description, would that be of any help here?

clang-tools-extra/clangd/InlayHints.cpp

119

This heuristic is far from perfect. There's definitely diminishing returns for the more advanced you make it, but I'd argue it makes sense to strip any underscore from the Name, e.g,
void set_timeout(int timeout)

[nridge]

Handle snake_case setter functions

[nridge]

clang-tools-extra/clangd/InlayHints.cpp

119

Good call, snake_case is definitely worth supporting

[nridge]

In D100731#2697480, @njames93 wrote:

I'm not sure how related this is, but Hover has methods that try to detect setter functions in class to give them a trivial description, would that be of any help here?

It looks like that code introspects the body of the function to see if it's a setter based on what it does, regardless of its name. Not sure if that's a good match for cases where it makes sense to omit a parameter hint.

[sammccall]

clang-tools-extra/clangd/InlayHints.cpp

113

This is nice and conservative, but a bit style dependent.
e.g. setSema(Sema &S)

Seems fine for now, maybe we want to tweak at some point.

119

This doesn't handle multi-word variable names if params use snake case and functions don't, e.g.:

e.g. void setExceptionHandler(EHFunc exception_handler)

This is fine for now but probably worth a comment.
Ultimately replacing equals_lower with some sloppy_equals that ignores case and skips underscores would solve this neatly.

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

344

can you add setTimeoutMillis(int timeout_millis) just to show the current behavior?

[nridge]

Address review comments

clang-tools-extra/clangd/InlayHints.cpp

processCall(E->getRParenLoc(),
                 dyn_cast_or_null<FunctionDecl>(E->getCalleeDecl()),
                 {E->getArgs(), E->getNumArgs()});
     return true;
   }
 
   // FIXME: Handle RecoveryExpr to try to hint some invalid calls.
 
 private:
+  using NameVec = SmallVector<StringRef, 8>;
+
   // The purpose of Anchor is to deal with macros. It should be the call's
   // opening or closing parenthesis or brace. (Always using the opening would
   // make more sense but CallExpr only exposes the closing.) We heuristically
   // assume that if this location does not come from a macro definition, then
   // the entire argument list likely appears in the main file and can be hinted.
   void processCall(SourceLocation Anchor, const FunctionDecl *Callee,
                    llvm::ArrayRef<const Expr *const> Args) {
     if (Args.size() == 0 || !Callee)
       return;
 
     // If the anchor location comes from a macro defintion, there's nowhere to
     // put hints.
     if (!AST.getSourceManager().getTopMacroCallerLoc(Anchor).isFileID())
       return;
 
     // The parameter name of a move or copy constructor is not very interesting.
     if (auto *Ctor = dyn_cast<CXXConstructorDecl>(Callee))
       if (Ctor->isCopyOrMoveConstructor())
         return;
 
-    // FIXME: Exclude setters (i.e. functions with one argument whose name
-    // begins with "set"), as their parameter name is also not likely to be
-    // interesting.
-
     // Don't show hints for variadic parameters.
     size_t FixedParamCount = getFixedParamCount(Callee);
     size_t ArgCount = std::min(FixedParamCount, Args.size());
 
     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;
 
       addInlayHint(Args[I]->getSourceRange(), InlayHintKind::ParameterHint,
                    Name.str() + ": ");
     }
   }
 
+  static bool isSetter(const FunctionDecl *Callee, const NameVec &ParamNames) {
+    if (ParamNames.size() != 1)
+      return false;
+
+    StringRef Name = getSimpleName(*Callee);
+    if (!Name.startswith_lower("set"))
+      return false;
+
+    // In addition to checking that the function has one parameter and its
+    // name starts with "set", also check that the part after "set" matches
+    // the name of the parameter (ignoring case). The idea here is that if
+    // the parameter name differs, it may contain extra information that
+    // may be useful to show in a hint, as in:
+    //   void setTimeout(int timeoutMillis);
+    // This currently doesn't handle cases where params use snake_case
+    // and functions don't, e.g.
+    //   void setExceptionHandler(EHFunc exception_handler);
+    // We could improve this by replacing `equals_lower` with some
+    // `sloppy_equals` which ignores case and also skips underscores.
+    StringRef WhatItIsSetting = Name.substr(3).ltrim("_");
+    return WhatItIsSetting.equals_lower(ParamNames[0]);
+  }
+
   bool shouldHint(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.
     if (ParamName == getSpelledIdentifier(Arg))
       return false;
static StringRef getSpelledIdentifier(const Expr *E) {
 
     if (auto *ME = dyn_cast<MemberExpr>(E))
       if (!ME->getQualifier() && ME->isImplicitAccess())
         return getSimpleName(*ME->getMemberDecl());
 
     return {};
   }
 
-  using NameVec = SmallVector<StringRef, 8>;
-
   NameVec chooseParameterNames(const FunctionDecl *Callee, size_t ArgCount) {
     // The current strategy here is to use all the parameter names from the
     // canonical declaration, unless they're all empty, in which case we
     // use all the parameter names from the definition (in present in the
     // translation unit).
     // We could try a bit harder, e.g.:
     //   - try all re-declarations, not just canonical + definition
     //   - fall back arg-by-arg rather than wholesale

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

void bar() {
       foo(/*param*/42);
       foo( /* param = */ 42);
       foo(/* the answer */$param[[42]]);
     }
   )cpp",
                        ExpectedHint{"param: ", "param"});
 }
 
+TEST(ParameterHints, SetterFunctions) {
+  assertParameterHints(R"cpp(
+    struct S {
+      void setParent(S* parent);
+      void set_parent(S* parent);
+      void setTimeout(int timeoutMillis);
+      void setTimeoutMillis(int timeout_millis);
+    };
+    void bar() {
+      S s;
+      // Parameter name matches setter name - omit hint.
+      s.setParent(nullptr);
+      // Support snake_case
+      s.set_parent(nullptr);
+      // Parameter name may contain extra info - show hint.
+      s.setTimeout($timeoutMillis[[120]]);
+      // FIXME: Ideally we'd want to omit this.
+      s.setTimeoutMillis($timeout_millis[[120]]);
+    }
+  )cpp",
+                       ExpectedHint{"timeoutMillis: ", "timeoutMillis"},
+                       ExpectedHint{"timeout_millis: ", "timeout_millis"});
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang