[clangd] Support configuration of inlay hints.

[sammccall]

Summary

The idea is that the feature will always be advertised at the LSP level, but
depending on config we'll return partial or no responses.

We try to avoid doing the analysis for hints we're not going to return.

Examples of syntax:

# No hints
InlayHints:
  Enabled: No
---
# Turn off a default category
InlayHints:
  ParameterNames: No
---
# Turn on some categories
InlayHints:
  ParameterNames: Yes
  DeducedTypes: Yes

[hokein]

clang-tools-extra/clangd/InlayHints.cpp

54

this should be !Cfg.InlayHints.ParameterNames.

What do you think the idea of moving guards deeper (processCall and addTypeHint)? The code seems clearer and we don't have to add them in all Visit* implementation, this means that we pay cost of running some necessary code, but I think it is trivial and probably worthy.

69

!Cfg.InlayHints.ParameterNames

[sammccall]

Simplify & fix category checks

[sammccall]

clang-tools-extra/clangd/InlayHints.cpp

54

I agree where to put the checks is an annoying question (and worth minimizing, since I've managed to get two wrong already).
I do think there needs to be a pattern so we don't accidentally skip checks.

• Easest is to postfilter (check in addInlayHint). That *does* hurt performance. Debug build on my laptop is ~170ms for all hints, ~160ms for postfilter with all categories disabled, ~5ms for current impl (prefilter) with all categories disabled. (On SemaCodeComplete.cpp, just eyeballing traces)
• Checking in VisitXXX methods (as done here) is a very regular pattern. Downside is you need many checks, and you can forget/break one
• Checks in helpers so that one is always hit (as you propose) touches fewer places but it's ad-hoc. I'm afraid of getting it wrong as the impl is modified (missing checks, doing too much work first, etc).
• Splitting RAV per category is an interesting option. Checking is very elegant, nicer code structure, can trace per-category latency, disabled categories can't crash... The downside is extra overhead, but this must be <5ms in the performance above. We can still choose to bundle simple categories together...

I think I'll do as you propose for now, improve the tests, and refactor to try to make it feel less ad-hoc later.
Also I should work out what we're spending 170ms on... EDIT: it turns out it's just assembling JSON objects :-\

[hokein]

clang-tools-extra/clangd/InlayHints.cpp

54

Thanks for digging into this.

~5ms for current impl (prefilter) with all categories disabled. (On SemaCodeComplete.cpp, just eyeballing traces)

Yeah, we should have this prefilter implementation when all categories are disabled.

Debug build on my laptop is ~170ms for all hints, ~160ms for postfilter with all categories disabled,

The data doesn't seem reasonable, I think the total cost comes from two main sources (AST traversal cost + assembling JSON object cost),
I would expect the allhints and postfilter have the similar AST traversal cost, but postfilter should have a near zero cost of assembling JSON objectcs (as it returns an empty vector), so the AST traversal cost is ~160ms.

Also I should work out what we're spending 170ms on... EDIT: it turns out it's just assembling JSON objects :-\

And based on this, it turns out the cost of all-hints approach should be larger than 170ms, which should be ~300ms (160ms for AST traversal + 170ms for assembling JSON objects).

Anyway, the current implementation looks good.

[jackoalan]

Assuming origin/maain is a branch typo and is no longer being tracked, would you please delete the upstream one in https://github.com/llvm/llvm-project/branches?

clang-tools-extra/clangd/Config.h

struct {
     bool AllScopes = true;
   } Completion;
 
   /// Configures hover feature.
   struct {
     /// Whether hover show a.k.a type.
     bool ShowAKA = false;
   } Hover;
+
+  struct {
+    /// If false, inlay hints are completely disabled.
+    bool Enabled = true;
+
+    // Whether specific categories of hints are enabled.
+    bool Parameters = true;
+    bool DeducedTypes = true;
+  } InlayHints;
 };
 
 } // namespace clangd
 } // namespace clang
 
 namespace llvm {
 template <> struct DenseMapInfo<clang::clangd::Config::ExternalIndexSpec> {
   using ExternalIndexSpec = clang::clangd::Config::ExternalIndexSpec;

clang-tools-extra/clangd/ConfigCompile.cpp

if (!F.Source.Directory.empty()) {
         FragmentDirectory += '/';
     }
     compile(std::move(F.If));
     compile(std::move(F.CompileFlags));
     compile(std::move(F.Index));
     compile(std::move(F.Diagnostics));
     compile(std::move(F.Completion));
     compile(std::move(F.Hover));
+    compile(std::move(F.InlayHints));
   }
 
   void compile(Fragment::IfBlock &&F) {
     if (F.HasUnrecognizedCondition)
       Out.Conditions.push_back([&](const Params &) { return false; });
 
 #ifdef CLANGD_PATH_CASE_INSENSITIVE
     llvm::Regex::RegexFlags Flags = llvm::Regex::IgnoreCase;
#endif
   void compile(Fragment::HoverBlock &&F) {
     if (F.ShowAKA) {
       Out.Apply.push_back([ShowAKA(**F.ShowAKA)](const Params &, Config &C) {
         C.Hover.ShowAKA = ShowAKA;
       });
     }
   }
 
+  void compile(Fragment::InlayHintsBlock &&F) {
+    if (F.Enabled)
+      Out.Apply.push_back([Value(**F.Enabled)](const Params &, Config &C) {
+        C.InlayHints.Enabled = Value;
+      });
+    if (F.ParameterNames)
+      Out.Apply.push_back(
+          [Value(**F.ParameterNames)](const Params &, Config &C) {
+            C.InlayHints.Parameters = Value;
+          });
+    if (F.DeducedTypes)
+      Out.Apply.push_back([Value(**F.DeducedTypes)](const Params &, Config &C) {
+        C.InlayHints.DeducedTypes = Value;
+      });
+  }
+
   constexpr static llvm::SourceMgr::DiagKind Error = llvm::SourceMgr::DK_Error;
   constexpr static llvm::SourceMgr::DiagKind Warning =
       llvm::SourceMgr::DK_Warning;
   void diag(llvm::SourceMgr::DiagKind Kind, llvm::StringRef Message,
             llvm::SMRange Range) {
     if (Range.isValid() && SourceMgr != nullptr)
       Diagnostic(SourceMgr->GetMessage(Range.Start, Kind, Message, Range));
     else

clang-tools-extra/clangd/ConfigFragment.h

struct Fragment {
   CompletionBlock Completion;
 
   /// Describes hover preferences.
   struct HoverBlock {
     /// Whether hover show a.k.a type.
     llvm::Optional<Located<bool>> ShowAKA;
   };
   HoverBlock Hover;
+
+  /// Configures labels shown inline with the code.
+  struct InlayHintsBlock {
+    /// Enables/disables the inlay-hints feature.
+    llvm::Optional<Located<bool>> Enabled;
+
+    /// Show parameter names before function arguments.
+    llvm::Optional<Located<bool>> ParameterNames;
+    /// Show deduced types for `auto`.
+    llvm::Optional<Located<bool>> DeducedTypes;
+  };
+  InlayHintsBlock InlayHints;
 };
 
 } // namespace config
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/clangd/ConfigYAML.cpp

bool parse(Fragment &F, Node &N) {
     DictParser Dict("Config", this);
     Dict.handle("If", [&](Node &N) { parse(F.If, N); });
     Dict.handle("CompileFlags", [&](Node &N) { parse(F.CompileFlags, N); });
     Dict.handle("Index", [&](Node &N) { parse(F.Index, N); });
     Dict.handle("Style", [&](Node &N) { parse(F.Style, N); });
     Dict.handle("Diagnostics", [&](Node &N) { parse(F.Diagnostics, N); });
     Dict.handle("Completion", [&](Node &N) { parse(F.Completion, N); });
     Dict.handle("Hover", [&](Node &N) { parse(F.Hover, N); });
+    Dict.handle("InlayHints", [&](Node &N) { parse(F.InlayHints, N); });
     Dict.parse(N);
     return !(N.failed() || HadError);
   }
 
 private:
   void parse(Fragment::IfBlock &F, Node &N) {
     DictParser Dict("If", this);
     Dict.unrecognized([&](Located<std::string>, Node &) {
void parse(Fragment::IndexBlock::ExternalBlock &F, Node &N) {
     Dict.handle("MountPoint",
                 [&](Node &N) { F.MountPoint = scalarValue(N, "MountPoint"); });
     Dict.parse(N);
   }
 
   void parse(Fragment::CompletionBlock &F, Node &N) {
     DictParser Dict("Completion", this);
     Dict.handle("AllScopes", [&](Node &N) {
-      if (auto Value = scalarValue(N, "AllScopes")) {
-        if (auto AllScopes = llvm::yaml::parseBool(**Value))
-          F.AllScopes = *AllScopes;
-        else
-          warning("AllScopes should be a boolean", N);
-      }
+      if (auto AllScopes = boolValue(N, "AllScopes"))
     });
     Dict.parse(N);
   }
 
   void parse(Fragment::HoverBlock &F, Node &N) {
     DictParser Dict("Hover", this);
     Dict.handle("ShowAKA", [&](Node &N) {
-      if (auto Value = scalarValue(N, "ShowAKA")) {
-        if (auto ShowAKA = llvm::yaml::parseBool(**Value))
-          F.ShowAKA = *ShowAKA;
-        else
-          warning("ShowAKA should be a boolean", N);
-      }
+      if (auto ShowAKA = boolValue(N, "ShowAKA"))
+    });
+    Dict.parse(N);
+
+  void parse(Fragment::InlayHintsBlock &F, Node &N) {
+    DictParser Dict("InlayHints", this);
+    Dict.handle("Enabled", [&](Node &N) {
+      if (auto Value = boolValue(N, "Enabled"))
+        F.Enabled = *Value;
+    });
+    Dict.handle("ParameterNames", [&](Node &N) {
+      if (auto Value = boolValue(N, "ParameterNames"))
+        F.ParameterNames = *Value;
+    });
+    Dict.handle("DeducedTypes", [&](Node &N) {
+      if (auto Value = boolValue(N, "DeducedTypes"))
+        F.DeducedTypes = *Value;
     });
     Dict.parse(N);
   }
 
   // Helper for parsing mapping nodes (dictionaries).
   // We don't use YamlIO as we want to control over unknown keys.
   class DictParser {
     llvm::StringRef Description;
llvm::Optional<Located<std::string>> scalarValue(Node &N,
     if (auto *S = llvm::dyn_cast<ScalarNode>(&N))
       return Located<std::string>(S->getValue(Buf).str(), N.getSourceRange());
     if (auto *BS = llvm::dyn_cast<BlockScalarNode>(&N))
       return Located<std::string>(BS->getValue().str(), N.getSourceRange());
     warning(Desc + " should be scalar", N);
     return llvm::None;
   }
 
+  llvm::Optional<Located<bool>> boolValue(Node &N, llvm::StringRef Desc) {
+    if (auto Scalar = scalarValue(N, Desc)) {
+      if (auto Bool = llvm::yaml::parseBool(**Scalar))
+        return Located<bool>(*Bool, Scalar->Range);
+      else
+        warning(Desc + " should be a boolean", N);
+    }
+    return llvm::None;
+  }
+
   // Try to parse a list of single scalar values, or just a single value.
   llvm::Optional<std::vector<Located<std::string>>> scalarValues(Node &N) {
     std::vector<Located<std::string>> Result;
     if (auto *S = llvm::dyn_cast<ScalarNode>(&N)) {
       llvm::SmallString<256> Buf;
       Result.emplace_back(S->getValue(Buf).str(), N.getSourceRange());
     } else if (auto *S = llvm::dyn_cast<BlockScalarNode>(&N)) {
       Result.emplace_back(S->getValue().str(), N.getSourceRange());

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 "Config.h"
 #include "HeuristicResolver.h"
 #include "ParsedAST.h"
 #include "clang/AST/DeclarationName.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceManager.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
 // For now, inlay hints are always anchored at the left or right of their range.
 enum class HintSide { Left, Right };
 
 class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
 public:
   InlayHintVisitor(std::vector<InlayHint> &Results, ParsedAST &AST,
-                   llvm::Optional<Range> RestrictRange)
-      : Results(Results), AST(AST.getASTContext()),
+                   const Config &Cfg, llvm::Optional<Range> RestrictRange)
+      : Results(Results), AST(AST.getASTContext()), Cfg(Cfg),
         RestrictRange(std::move(RestrictRange)),
         MainFileID(AST.getSourceManager().getMainFileID()),
         Resolver(AST.getHeuristicResolver()),
         TypeHintPolicy(this->AST.getPrintingPolicy()),
         StructuredBindingPolicy(this->AST.getPrintingPolicy()) {
     bool Invalid = false;
     llvm::StringRef Buf =
         AST.getSourceManager().getBufferData(MainFileID, &Invalid);
InlayHintVisitor(std::vector<InlayHint> &Results, ParsedAST &AST,
     // For "auto", we often prefer sugared types.
     // Not setting PrintCanonicalTypes for "auto" allows
     // SuppressDefaultTemplateArgs (set by default) to have an effect.
     StructuredBindingPolicy = TypeHintPolicy;
     StructuredBindingPolicy.PrintCanonicalTypes = true;
   }
 
   bool VisitCXXConstructExpr(CXXConstructExpr *E) {
     // Weed out constructor calls that don't look like a function call with
     // an argument list, by checking the validity of getParenOrBraceRange().
     // Also weed out std::initializer_list constructors as there are no names
     // for the individual arguments.
     if (!E->getParenOrBraceRange().isValid() ||
         E->isStdInitListInitialization()) {
       return true;
     }
 
     processCall(E->getParenOrBraceRange().getBegin(), E->getConstructor(),
                 {E->getArgs(), E->getNumArgs()});
     return true;
   }
 
   bool VisitCallExpr(CallExpr *E) {
+    if (!Cfg.InlayHints.Parameters)
+      return true;
+
     // Do not show parameter hints for operator calls written using operator
     // syntax or user-defined literals. (Among other reasons, the resulting
     // hints can look awkard, e.g. the expression can itself be a function
     // argument and then we'd get two hints side by side).
     if (isa<CXXOperatorCallExpr>(E) || isa<UserDefinedLiteral>(E))
       return true;
 
     auto CalleeDecls = Resolver->resolveCalleeOfCallExpr(E);
private:
 
   // 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)
+    if (!Cfg.InlayHints.Parameters || 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.
NameVec getParameterNamesForDecl(const FunctionDecl *Function,
     return Result;
   }
 
   // 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!");
+    switch (Kind) {
+#define CHECK_KIND(Enumerator, ConfigProperty)                                 \
+  case InlayHintKind::Enumerator:                                              \
+    assert(Cfg.InlayHints.ConfigProperty &&                                    \
+           "Shouldn't get here if kind is disabled!");                         \
+    if (!Cfg.InlayHints.ConfigProperty)                                        \
+      return;                                                                  \
+    break
+      CHECK_KIND(ParameterHint, Parameters);
+      CHECK_KIND(TypeHint, DeducedTypes);
+#undef CHECK_KIND
+    }
+
     auto FileRange =
         toHalfOpenFileRange(AST.getSourceManager(), AST.getLangOpts(), R);
     if (!FileRange)
       return;
     Range LSPRange{
         sourceLocToPosition(AST.getSourceManager(), FileRange->getBegin()),
         sourceLocToPosition(AST.getSourceManager(), FileRange->getEnd())};
     Position LSPPos = Side == HintSide::Left ? LSPRange.start : LSPRange.end;
#undef CHECK_KIND
   }
 
   void addTypeHint(SourceRange R, QualType T, llvm::StringRef Prefix) {
     addTypeHint(R, T, Prefix, TypeHintPolicy);
   }
 
   void addTypeHint(SourceRange R, QualType T, llvm::StringRef Prefix,
                    const PrintingPolicy &Policy) {
-    // Do not print useless "NULL TYPE" hint.
-    if (!T.getTypePtrOrNull())
+    if (!Cfg.InlayHints.DeducedTypes || T.isNull())
       return;
 
     std::string TypeName = T.getAsString(Policy);
     if (TypeName.length() < TypeNameLimit)
       addInlayHint(R, HintSide::Right, InlayHintKind::TypeHint, Prefix,
                    TypeName, /*Suffix=*/"");
   }
 
   std::vector<InlayHint> &Results;
   ASTContext &AST;
+  const Config &Cfg;
   llvm::Optional<Range> RestrictRange;
   FileID MainFileID;
   StringRef MainFileBuf;
   const HeuristicResolver *Resolver;
   // We want to suppress default template arguments, but otherwise print
   // canonical types. Unfortunately, they're conflicting policies so we can't
   // have both. For regular types, suppressing template arguments is more
   // important, whereas printing canonical types is crucial for structured
   // bindings, so we use two separate policies. (See the constructor where
   // the policies are initialized for more details.)
   PrintingPolicy TypeHintPolicy;
   PrintingPolicy StructuredBindingPolicy;
 
   static const size_t TypeNameLimit = 32;
 };
 
 } // namespace
 
 std::vector<InlayHint> inlayHints(ParsedAST &AST,
                                   llvm::Optional<Range> RestrictRange) {
   std::vector<InlayHint> Results;
-  InlayHintVisitor Visitor(Results, AST, std::move(RestrictRange));
+  const auto &Cfg = Config::current();
+  if (!Cfg.InlayHints.Enabled)
+    return Results;
+  InlayHintVisitor Visitor(Results, AST, Cfg, std::move(RestrictRange));
   Visitor.TraverseAST(AST.getASTContext());
 
   // De-duplicate hints. Duplicates can sometimes occur due to e.g. explicit
   // template instantiations.
   llvm::sort(Results);
   Results.erase(std::unique(Results.begin(), Results.end()), Results.end());
 
   return Results;
 }
 
 } // namespace clangd
 } // namespace clang

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

Hover:
   ShowAKA: True
   )yaml");
   auto Results =
       Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
   ASSERT_THAT(Diags.Diagnostics, IsEmpty());
   ASSERT_EQ(Results.size(), 1u);
   EXPECT_THAT(Results[0].Hover.ShowAKA, llvm::ValueIs(Val(true)));
 }
+
+TEST(ParseYAML, InlayHints) {
+  CapturedDiags Diags;
+  Annotations YAML(R"yaml(
+InlayHints:
+  Enabled: No
+  ParameterNames: Yes
+  )yaml");
+  auto Results =
+      Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_EQ(Results.size(), 1u);
+  EXPECT_THAT(Results[0].InlayHints.Enabled, llvm::ValueIs(Val(false)));
+  EXPECT_THAT(Results[0].InlayHints.ParameterNames, llvm::ValueIs(Val(true)));
+  EXPECT_EQ(Results[0].InlayHints.DeducedTypes, llvm::None);
+}
+
 } // namespace
 } // namespace config
 } // namespace clangd
 } // namespace clang

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

 //===-- InlayHintTests.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 "Annotations.h"
+#include "Config.h"
 #include "InlayHints.h"
 #include "Protocol.h"
 #include "TestTU.h"
 #include "TestWorkspace.h"
 #include "XRefs.h"
+#include "support/Context.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 
 std::ostream &operator<<(std::ostream &Stream, const InlayHint &Hint) {
   return Stream << Hint.label;
 }
 
 namespace {
 
 using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::UnorderedElementsAre;
 
 std::vector<InlayHint> hintsOfKind(ParsedAST &AST, InlayHintKind Kind) {
   std::vector<InlayHint> Result;
   for (auto &Hint : inlayHints(AST, /*RestrictRange=*/llvm::None)) {
     if (Hint.kind == Kind)
       Result.push_back(Hint);
   }
   return Result;
MATCHER_P2(HintMatcher, Expected, Code, "") {
   return arg.label == Expected.Label &&
          arg.range == Code.range(Expected.RangeName) &&
          arg.position ==
              ((Expected.Side == Left) ? arg.range.start : arg.range.end);
 }
 
 MATCHER_P(labelIs, Label, "") { return arg.label == Label; }
 
+Config noHintsConfig() {
+  Config C;
+  C.InlayHints.Parameters = false;
+  C.InlayHints.DeducedTypes = false;
+  return C;
+}
+
 template <typename... ExpectedHints>
 void assertHints(InlayHintKind Kind, llvm::StringRef AnnotatedSource,
                  ExpectedHints... Expected) {
   Annotations Source(AnnotatedSource);
   TestTU TU = TestTU::withCode(Source.code());
   TU.ExtraArgs.push_back("-std=c++14");
   auto AST = TU.build();
 
   EXPECT_THAT(hintsOfKind(AST, Kind),
               ElementsAre(HintMatcher(Expected, Source)...));
+  // Sneak in a cross-cutting check that hints are disabled by config.
+  // We'll hit an assertion failure if addInlayHint still gets called.
+  WithContextValue WithCfg(Config::Key, noHintsConfig());
+  EXPECT_THAT(inlayHints(AST, llvm::None), IsEmpty());
 }
 
 // Hack to allow expression-statements operating on parameter packs in C++14.
 template <typename... T> void ignore(T &&...) {}
 
 template <typename... ExpectedHints>
 void assertParameterHints(llvm::StringRef AnnotatedSource,
                           ExpectedHints... Expected) {