[Clangd] Make the type hint length limit configurable

[zhangyi1357]

Summary

[Clangd] Make the type hint length limit configurable

This commit is about clangd's type name hint length limit. The past behavior was 32 characters fixed limit. It is now configurable.

Projects can now add the following config fragment to their .clangd:

InlayHints:
  TypeNameLimit: 34

Ref: Make the type hint length limit configurable

[zhangyi1357]

Format code modified using clang format.

[hokein]

Thanks for the contribution!

clang-tools-extra/clangd/Config.h

151

I would extend it a bit more -- 0 means no limit.

Can you also add a unittest in TEST(TypeHints, LongTypeName) in InlayHintTests.cpp?

clang-tools-extra/clangd/ConfigFragment.h

326

if we add uint32Value method (see my other comment), then the type is std::optional<Located<uint32_t>>.

clang-tools-extra/clangd/ConfigYAML.cpp

258

Similar to the boolValue, I think it would be better to have a uint32Value method (this is the first time that we have an integer in the config), something like below

 std::optional<Located<uint32_t>> uint32Value(Node &N, llvm::StringRef Desc) {
   if (auto Scalar = scalarValue(N, Desc)) {
        unsigned long long n;
        if (getAsUnsignedInteger(Scalar, 0, n)) {
           warning(Desc + "invalid number", N);
           return;
        }
        return Located<uint32_t>(n, Scalar->Range);
    }
    return std::nullopt;
}

[zhangyi1357]

Add uint32Value() for uint32_t type parsing in config file.
For TypeNameLimit config, support no limit with value 0.

[zhangyi1357]

This comment was removed by zhangyi1357.

clang-tools-extra/clangd/Config.h

151

0 means no limit.

This is quite a good idea. I've done it.

For unittest, there is already TEST(TypeHints, LongTypeName) in InlayHintTests.cpp. Do you mean add more tests in the same TEST or another TEST with TypeNameLimit configured?

[zhangyi1357]

Try to fix the build problem.

[zhangyi1357]

In D147395#4240227, @hokein wrote:

Thanks for the contribution!

It's quite interesting for me. Thanks for your time reviewing and your great advice!

[hokein]

clang-tools-extra/clangd/Config.h

151

I mean adding one more test in the same TEST(TypeHints, LongTypeName).

This test verifies the the long type name is shown when the limit is set to 0, something like

TEST(TypeHints, LongTypeName) {
  assertTypeHints(R"cpp(
    template <typename, typename, typename>
    struct A {};
    struct MultipleWords {};
    A<MultipleWords, MultipleWords, MultipleWords> foo();
    // Omit type hint past a certain length (currently 32)
    auto var = foo();
  )cpp");

    Config cfg; 
     ... // set the limit to 0

   assertTypeHints(R"cpp(
    template <typename, typename, typename>
    struct A {};
    struct MultipleWords {};
    A<MultipleWords, MultipleWords, MultipleWords> foo();
    // Omit type hint past a certain length (currently 32)
    auto var = foo();
  )cpp", ExpectedHint...);
}

[zhangyi1357]

• [Clangd] Add unittest for TypeNameLimit config

[hokein]

Thanks, looks good with a nit.

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

80 ↗

(On Diff #510795)

why do we need this change? I think it should be fine without it.

[zhangyi1357]

• [Clangd] Remove unneccessary modification

[zhangyi1357]

Combine mutiple commits into single one

[zhangyi1357]

I dont have commit access. Could you help committing the change for me? @hokein

clang-tools-extra/clangd/Config.h

151

Thanks for your example! I've added that.

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

80 ↗

(On Diff #510795)

Yes, without this line the tests will not fail. But I am a little confused about the code below without 'C.InlayHints.TypeNameLimit = 1;'.

Config noHintsConfig() {
  Config C;
  C.InlayHints.Parameters = false;
  C.InlayHints.DeducedTypes = false;
  C.InlayHints.Designators = false;
  // C.InlayHints.TypeNameLimit = 1;
  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++20");
  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, std::nullopt), IsEmpty());  // Why does this succeed with TypeNameLimit = 32 ?
}

TEST(TypeHints, Smoke) {
  assertTypeHints(R"cpp(
    auto $waldo[[waldo]] = 42;
  )cpp",
                  ExpectedHint{": int", "waldo"});
}

The dault TypeNameLimit is 32, why does the second EXPECT_THAT succeed with TypeNameLimit = 32.

[hokein]

In D147395#4253753, @zhangyi1357 wrote:

I dont have commit access. Could you help committing the change for me? @hokein

sure, I will land it for you (sorry for the late response).

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

80 ↗

(On Diff #510795)

The second EXPECT_THAT(inlayHints(AST, std::nullopt), IsEmpty()); is expected, because we have disabled the inlay-hint by the previous statement WithContextValue WithCfg(Config::Key, noHintsConfig());, so the expected result is empty.

clang-tools-extra/clangd/Config.h

 //===--- Config.h - User configuration of clangd behavior --------*- 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
 //
 //===----------------------------------------------------------------------===//
 //
struct Config {
   struct {
     /// If false, inlay hints are completely disabled.
     bool Enabled = true;
 
     // Whether specific categories of hints are enabled.
     bool Parameters = true;
     bool DeducedTypes = true;
     bool Designators = true;
+    // Limit the length of type names in inlay hints.
+    size_t TypeNameLimit = 32;
   } InlayHints;
 };
 
 } // namespace clangd
 } // namespace clang
 
 namespace llvm {
 template <> struct DenseMapInfo<clang::clangd::Config::ExternalIndexSpec> {

clang-tools-extra/clangd/ConfigCompile.cpp

 //===--- ConfigCompile.cpp - Translating Fragments into Config ------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 //
void compile(Fragment::InlayHintsBlock &&F) {
     if (F.DeducedTypes)
       Out.Apply.push_back([Value(**F.DeducedTypes)](const Params &, Config &C) {
         C.InlayHints.DeducedTypes = Value;
       });
     if (F.Designators)
       Out.Apply.push_back([Value(**F.Designators)](const Params &, Config &C) {
         C.InlayHints.Designators = Value;
       });
+    if (F.TypeNameLimit)
+      Out.Apply.push_back(
+          [Value(**F.TypeNameLimit)](const Params &, Config &C) {
+            C.InlayHints.TypeNameLimit = stoul(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)

clang-tools-extra/clangd/ConfigFragment.h

 //===--- ConfigFragment.h - Unit of user-specified configuration -*- 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
 //
 //===----------------------------------------------------------------------===//
 //
struct InlayHintsBlock {
     std::optional<Located<bool>> Enabled;
 
     /// Show parameter names before function arguments.
     std::optional<Located<bool>> ParameterNames;
     /// Show deduced types for `auto`.
     std::optional<Located<bool>> DeducedTypes;
     /// Show designators in aggregate initialization.
     std::optional<Located<bool>> Designators;
+    /// Limit the length of type name hints.
+    std::optional<Located<std::string>> TypeNameLimit;
   };
   InlayHintsBlock InlayHints;
 };
 
 } // namespace config
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/clangd/ConfigYAML.cpp

 //===--- ConfigYAML.cpp - Loading configuration fragments from YAML files -===//
 //
 // 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 "ConfigFragment.h"
void parse(Fragment::InlayHintsBlock &F, Node &N) {
     Dict.handle("DeducedTypes", [&](Node &N) {
       if (auto Value = boolValue(N, "DeducedTypes"))
         F.DeducedTypes = *Value;
     });
     Dict.handle("Designators", [&](Node &N) {
       if (auto Value = boolValue(N, "Designators"))
         F.Designators = *Value;
     });
+    Dict.handle("TypeNameLimit", [&](Node &N) {
+      if (auto Value = scalarValue(N, "TypeNameLimit"))
+        F.TypeNameLimit = *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;
     std::vector<std::pair<llvm::StringRef, std::function<void(Node &)>>> Keys;

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"
#undef CHECK_KIND
   }
 
   void addTypeHint(SourceRange R, QualType T, llvm::StringRef Prefix,
                    const PrintingPolicy &Policy) {
     if (!Cfg.InlayHints.DeducedTypes || T.isNull())
       return;
 
     std::string TypeName = T.getAsString(Policy);
-    if (TypeName.length() < TypeNameLimit)
+    if (TypeName.length() < Cfg.InlayHints.TypeNameLimit)
       addInlayHint(R, HintSide::Right, InlayHintKind::Type, Prefix, TypeName,
                    /*Suffix=*/"");
   }
 
   void addDesignatorHint(SourceRange R, llvm::StringRef Text) {
     addInlayHint(R, HintSide::Left, InlayHintKind::Designator,
                  /*Prefix=*/"", Text, /*Suffix=*/"=");
   }
#undef CHECK_KIND
   // 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,
                                   std::optional<Range> RestrictRange) {
   std::vector<InlayHint> Results;
   const auto &Cfg = Config::current();