[clangd] Improve BlockEnd inlayhint presentation (#136106)

* Only show for blocks 10 lines or taller (including braces) * Add parens for function call: "// if foo" -> "// if foo()" or "// if foo(...)" * Print literal nullptr * Escaping for abbreviated strings Fixes https://github.com/clangd/clangd/issues/1807. Based on the original PR at https://github.com/llvm/llvm-project/pull/72345. Co-authored-by: daiyousei-qz <qyzheng2@outlook.com>
2025-04-22 23:36:48 -07:00
parent 30c4714726
commit 665914fea1
3 changed files with 139 additions and 45 deletions
--- a/clang-tools-extra/clangd/InlayHints.cpp
+++ b/clang-tools-extra/clangd/InlayHints.cpp
@@ -112,7 +112,9 @@ std::string summarizeExpr(const Expr *E) {
      return getSimpleName(*E->getFoundDecl()).str();
    }
    std::string VisitCallExpr(const CallExpr *E) {
-      return Visit(E->getCallee());
+      std::string Result = Visit(E->getCallee());
+      Result += E->getNumArgs() == 0 ? "()" : "(...)";
+      return Result;
    }
    std::string
    VisitCXXDependentScopeMemberExpr(const CXXDependentScopeMemberExpr *E) {
@@ -147,6 +149,9 @@ std::string summarizeExpr(const Expr *E) {
    }

    // Literals are just printed
+    std::string VisitCXXNullPtrLiteralExpr(const CXXNullPtrLiteralExpr *E) {
+      return "nullptr";
+    }
    std::string VisitCXXBoolLiteralExpr(const CXXBoolLiteralExpr *E) {
      return E->getValue() ? "true" : "false";
    }
@@ -165,12 +170,14 @@ std::string summarizeExpr(const Expr *E) {
      std::string Result = "\"";
      if (E->containsNonAscii()) {
        Result += "...";
-      } else if (E->getLength() > 10) {
-        Result += E->getString().take_front(7);
-        Result += "...";
      } else {
        llvm::raw_string_ostream OS(Result);
-        llvm::printEscapedString(E->getString(), OS);
+        if (E->getLength() > 10) {
+          llvm::printEscapedString(E->getString().take_front(7), OS);
+          Result += "...";
+        } else {
+          llvm::printEscapedString(E->getString(), OS);
+        }
      }
      Result.push_back('"');
      return Result;
@@ -408,12 +415,14 @@ struct Callee {
 class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
 public:
  InlayHintVisitor(std::vector<InlayHint> &Results, ParsedAST &AST,
-                   const Config &Cfg, std::optional<Range> RestrictRange)
+                   const Config &Cfg, std::optional<Range> RestrictRange,
+                   InlayHintOptions HintOptions)
      : Results(Results), AST(AST.getASTContext()), Tokens(AST.getTokens()),
        Cfg(Cfg), RestrictRange(std::move(RestrictRange)),
        MainFileID(AST.getSourceManager().getMainFileID()),
        Resolver(AST.getHeuristicResolver()),
-        TypeHintPolicy(this->AST.getPrintingPolicy()) {
+        TypeHintPolicy(this->AST.getPrintingPolicy()),
+        HintOptions(HintOptions) {
    bool Invalid = false;
    llvm::StringRef Buf =
        AST.getSourceManager().getBufferData(MainFileID, &Invalid);
@@ -1120,7 +1129,6 @@ private:
  // Otherwise, the hint shouldn't be shown.
  std::optional<Range> computeBlockEndHintRange(SourceRange BraceRange,
                                                StringRef OptionalPunctuation) {
-    constexpr unsigned HintMinLineLimit = 2;

    auto &SM = AST.getSourceManager();
    auto [BlockBeginFileId, BlockBeginOffset] =
@@ -1148,7 +1156,7 @@ private:
    auto RBraceLine = SM.getLineNumber(RBraceFileId, RBraceOffset);

    // Don't show hint on trivial blocks like `class X {};`
-    if (BlockBeginLine + HintMinLineLimit - 1 > RBraceLine)
+    if (BlockBeginLine + HintOptions.HintMinLineLimit - 1 > RBraceLine)
      return std::nullopt;

    // This is what we attach the hint to, usually "}" or "};".
@@ -1178,17 +1186,20 @@ private:
  StringRef MainFileBuf;
  const HeuristicResolver *Resolver;
  PrintingPolicy TypeHintPolicy;
+  InlayHintOptions HintOptions;
 };

 } // namespace

 std::vector<InlayHint> inlayHints(ParsedAST &AST,
-                                  std::optional<Range> RestrictRange) {
+                                  std::optional<Range> RestrictRange,
+                                  InlayHintOptions HintOptions) {
  std::vector<InlayHint> Results;
  const auto &Cfg = Config::current();
  if (!Cfg.InlayHints.Enabled)
    return Results;
-  InlayHintVisitor Visitor(Results, AST, Cfg, std::move(RestrictRange));
+  InlayHintVisitor Visitor(Results, AST, Cfg, std::move(RestrictRange),
+                           HintOptions);
  Visitor.TraverseAST(AST.getASTContext());

  // De-duplicate hints. Duplicates can sometimes occur due to e.g. explicit
--- a/clang-tools-extra/clangd/InlayHints.h
+++ b/clang-tools-extra/clangd/InlayHints.h
@@ -22,10 +22,17 @@ namespace clang {
 namespace clangd {
 class ParsedAST;

+struct InlayHintOptions {
+  // Minimum height of a code block in lines for a BlockEnd hint to be shown
+  // Includes the lines containing the braces
+  int HintMinLineLimit = 10;
+};
+
 /// Compute and return inlay hints for a file.
 /// If RestrictRange is set, return only hints whose location is in that range.
 std::vector<InlayHint> inlayHints(ParsedAST &AST,
-                                  std::optional<Range> RestrictRange);
+                                  std::optional<Range> RestrictRange,
+                                  InlayHintOptions HintOptions = {});

 } // namespace clangd
 } // namespace clang
--- a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -36,9 +36,12 @@ namespace {
 using ::testing::ElementsAre;
 using ::testing::IsEmpty;

-std::vector<InlayHint> hintsOfKind(ParsedAST &AST, InlayHintKind Kind) {
+constexpr InlayHintOptions DefaultOptsForTests{2};
+
+std::vector<InlayHint> hintsOfKind(ParsedAST &AST, InlayHintKind Kind,
+                                   InlayHintOptions Opts) {
  std::vector<InlayHint> Result;
-  for (auto &Hint : inlayHints(AST, /*RestrictRange=*/std::nullopt)) {
+  for (auto &Hint : inlayHints(AST, /*RestrictRange=*/std::nullopt, Opts)) {
    if (Hint.kind == Kind)
      Result.push_back(Hint);
  }
@@ -90,7 +93,7 @@ Config noHintsConfig() {

 template <typename... ExpectedHints>
 void assertHintsWithHeader(InlayHintKind Kind, llvm::StringRef AnnotatedSource,
-                           llvm::StringRef HeaderContent,
+                           llvm::StringRef HeaderContent, InlayHintOptions Opts,
                           ExpectedHints... Expected) {
  Annotations Source(AnnotatedSource);
  TestTU TU = TestTU::withCode(Source.code());
@@ -98,18 +101,18 @@ void assertHintsWithHeader(InlayHintKind Kind, llvm::StringRef AnnotatedSource,
  TU.HeaderCode = HeaderContent;
  auto AST = TU.build();

-  EXPECT_THAT(hintsOfKind(AST, Kind),
+  EXPECT_THAT(hintsOfKind(AST, Kind, Opts),
              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());
+  EXPECT_THAT(inlayHints(AST, std::nullopt, Opts), IsEmpty());
 }

 template <typename... ExpectedHints>
 void assertHints(InlayHintKind Kind, llvm::StringRef AnnotatedSource,
-                 ExpectedHints... Expected) {
-  return assertHintsWithHeader(Kind, AnnotatedSource, "",
+                 InlayHintOptions Opts, ExpectedHints... Expected) {
+  return assertHintsWithHeader(Kind, AnnotatedSource, "", Opts,
                               std::move(Expected)...);
 }

@@ -120,14 +123,16 @@ template <typename... ExpectedHints>
 void assertParameterHints(llvm::StringRef AnnotatedSource,
                          ExpectedHints... Expected) {
  ignore(Expected.Side = Left...);
-  assertHints(InlayHintKind::Parameter, AnnotatedSource, Expected...);
+  assertHints(InlayHintKind::Parameter, AnnotatedSource, DefaultOptsForTests,
+              Expected...);
 }

 template <typename... ExpectedHints>
 void assertTypeHints(llvm::StringRef AnnotatedSource,
                     ExpectedHints... Expected) {
  ignore(Expected.Side = Right...);
-  assertHints(InlayHintKind::Type, AnnotatedSource, Expected...);
+  assertHints(InlayHintKind::Type, AnnotatedSource, DefaultOptsForTests,
+              Expected...);
 }

 template <typename... ExpectedHints>
@@ -136,16 +141,25 @@ void assertDesignatorHints(llvm::StringRef AnnotatedSource,
  Config Cfg;
  Cfg.InlayHints.Designators = true;
  WithContextValue WithCfg(Config::Key, std::move(Cfg));
-  assertHints(InlayHintKind::Designator, AnnotatedSource, Expected...);
+  assertHints(InlayHintKind::Designator, AnnotatedSource, DefaultOptsForTests,
+              Expected...);
+}
+
+template <typename... ExpectedHints>
+void assertBlockEndHintsWithOpts(llvm::StringRef AnnotatedSource,
+                                 InlayHintOptions Opts,
+                                 ExpectedHints... Expected) {
+  Config Cfg;
+  Cfg.InlayHints.BlockEnd = true;
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+  assertHints(InlayHintKind::BlockEnd, AnnotatedSource, Opts, Expected...);
 }

 template <typename... ExpectedHints>
 void assertBlockEndHints(llvm::StringRef AnnotatedSource,
                         ExpectedHints... Expected) {
-  Config Cfg;
-  Cfg.InlayHints.BlockEnd = true;
-  WithContextValue WithCfg(Config::Key, std::move(Cfg));
-  assertHints(InlayHintKind::BlockEnd, AnnotatedSource, Expected...);
+  assertBlockEndHintsWithOpts(AnnotatedSource, DefaultOptsForTests,
+                              Expected...);
 }

 TEST(ParameterHints, Smoke) {
@@ -1226,7 +1240,9 @@ TEST(ParameterHints, IncludeAtNonGlobalScope) {
  ASSERT_TRUE(bool(AST));

  // Ensure the hint for the call in foo.inc is NOT materialized in foo.cc.
-  EXPECT_EQ(hintsOfKind(*AST, InlayHintKind::Parameter).size(), 0u);
+  EXPECT_EQ(
+      hintsOfKind(*AST, InlayHintKind::Parameter, DefaultOptsForTests).size(),
+      0u);
 }

 TEST(TypeHints, Smoke) {
@@ -1488,12 +1504,12 @@ TEST(DefaultArguments, Smoke) {
    void baz(int = 5) { if (false) baz($unnamed[[)]]; };
  )cpp";

-  assertHints(InlayHintKind::DefaultArgument, Code,
+  assertHints(InlayHintKind::DefaultArgument, Code, DefaultOptsForTests,
              ExpectedHint{"A: 4", "default1", Left},
              ExpectedHint{", B: 1, C: foo()", "default2", Left},
              ExpectedHint{"5", "unnamed", Left});

-  assertHints(InlayHintKind::Parameter, Code,
+  assertHints(InlayHintKind::Parameter, Code, DefaultOptsForTests,
              ExpectedHint{"A: ", "explicit", Left});
 }

@@ -1528,14 +1544,14 @@ TEST(DefaultArguments, WithoutParameterNames) {
    }
  )cpp";

-  assertHints(InlayHintKind::DefaultArgument, Code,
+  assertHints(InlayHintKind::DefaultArgument, Code, DefaultOptsForTests,
              ExpectedHint{"...", "abbreviated", Left},
              ExpectedHint{", Baz{}", "paren", Left},
              ExpectedHint{", Baz{}", "brace1", Left},
              ExpectedHint{", Baz{}", "brace2", Left},
              ExpectedHint{", Baz{}", "brace3", Left});

-  assertHints(InlayHintKind::Parameter, Code);
+  assertHints(InlayHintKind::Parameter, Code, DefaultOptsForTests);
 }

 TEST(TypeHints, Deduplication) {
@@ -1573,7 +1589,8 @@ TEST(TypeHints, Aliased) {
  TU.ExtraArgs.push_back("-xc");
  auto AST = TU.build();

-  EXPECT_THAT(hintsOfKind(AST, InlayHintKind::Type), IsEmpty());
+  EXPECT_THAT(hintsOfKind(AST, InlayHintKind::Type, DefaultOptsForTests),
+              IsEmpty());
 }

 TEST(TypeHints, CallingConvention) {
@@ -1590,7 +1607,7 @@ TEST(TypeHints, CallingConvention) {
  auto AST = TU.build();

  EXPECT_THAT(
-      hintsOfKind(AST, InlayHintKind::Type),
+      hintsOfKind(AST, InlayHintKind::Type, DefaultOptsForTests),
      ElementsAre(HintMatcher(ExpectedHint{"-> void", "lambda"}, Source)));
 }

@@ -1673,7 +1690,7 @@ TEST(TypeHints, SubstTemplateParameterAliases) {
  )cpp";

  assertHintsWithHeader(
-      InlayHintKind::Type, VectorIntPtr, Header,
+      InlayHintKind::Type, VectorIntPtr, Header, DefaultOptsForTests,
      ExpectedHint{": int *", "no_modifier"},
      ExpectedHint{": int **", "ptr_modifier"},
      ExpectedHint{": int *&", "ref_modifier"},
@@ -1697,7 +1714,7 @@ TEST(TypeHints, SubstTemplateParameterAliases) {
  )cpp";

  assertHintsWithHeader(
-      InlayHintKind::Type, VectorInt, Header,
+      InlayHintKind::Type, VectorInt, Header, DefaultOptsForTests,
      ExpectedHint{": int", "no_modifier"},
      ExpectedHint{": int *", "ptr_modifier"},
      ExpectedHint{": int &", "ref_modifier"},
@@ -1724,6 +1741,7 @@ TEST(TypeHints, SubstTemplateParameterAliases) {
  )cpp";

  assertHintsWithHeader(InlayHintKind::Type, TypeAlias, Header,
+                        DefaultOptsForTests,
                        ExpectedHint{": Short", "short_name"},
                        ExpectedHint{": static_vector<int>", "vector_name"});
 }
@@ -2016,6 +2034,7 @@ TEST(BlockEndHints, If) {
  assertBlockEndHints(
      R"cpp(
    void foo(bool cond) {
+       void* ptr;
       if (cond)
          ;

@@ -2041,13 +2060,17 @@ TEST(BlockEndHints, If) {

       if (int i = 0; i > 10) {
       $init_cond[[}]]
+
+       if (ptr != nullptr) {
+       $null_check[[}]]
    } // suppress
  )cpp",
      ExpectedHint{" // if cond", "simple"},
      ExpectedHint{" // if cond", "ifelse"}, ExpectedHint{" // if", "elseif"},
      ExpectedHint{" // if !cond", "inner"},
      ExpectedHint{" // if cond", "outer"}, ExpectedHint{" // if X", "init"},
-      ExpectedHint{" // if i > 10", "init_cond"});
+      ExpectedHint{" // if i > 10", "init_cond"},
+      ExpectedHint{" // if ptr != nullptr", "null_check"});
 }

 TEST(BlockEndHints, Loops) {
@@ -2124,30 +2147,41 @@ TEST(BlockEndHints, PrintRefs) {
      R"cpp(
    namespace ns {
      int Var;
-      int func();
+      int func1();
+      int func2(int, int);
      struct S {
        int Field;
-        int method() const;
+        int method1() const;
+        int method2(int, int) const;
      }; // suppress
    } // suppress
    void foo() {
+      int int_a {};
      while (ns::Var) {
      $var[[}]]

-      while (ns::func()) {
-      $func[[}]]
+      while (ns::func1()) {
+      $func1[[}]]
+
+      while (ns::func2(int_a, int_a)) {
+      $func2[[}]]

      while (ns::S{}.Field) {
      $field[[}]]

-      while (ns::S{}.method()) {
-      $method[[}]]
+      while (ns::S{}.method1()) {
+      $method1[[}]]
+      
+      while (ns::S{}.method2(int_a, int_a)) {
+      $method2[[}]]
    } // suppress
  )cpp",
      ExpectedHint{" // while Var", "var"},
-      ExpectedHint{" // while func", "func"},
+      ExpectedHint{" // while func1()", "func1"},
+      ExpectedHint{" // while func2(...)", "func2"},
      ExpectedHint{" // while Field", "field"},
-      ExpectedHint{" // while method", "method"});
+      ExpectedHint{" // while method1()", "method1"},
+      ExpectedHint{" // while method2(...)", "method2"});
 }

 TEST(BlockEndHints, PrintConversions) {
@@ -2307,7 +2341,49 @@ TEST(BlockEndHints, PointerToMemberFunction) {
      $ptrmem[[}]]
    } // suppress
  )cpp",
-                      ExpectedHint{" // if", "ptrmem"});
+                      ExpectedHint{" // if ()", "ptrmem"});
+}
+
+TEST(BlockEndHints, MinLineLimit) {
+  InlayHintOptions Opts;
+  Opts.HintMinLineLimit = 10;
+
+  // namespace ns below is exactly 10 lines
+  assertBlockEndHintsWithOpts(
+      R"cpp(
+    namespace ns {
+      int Var;
+      int func1();
+      int func2(int, int);
+      struct S {
+        int Field;
+        int method1() const;
+        int method2(int, int) const;
+      };
+    $namespace[[}]]
+    void foo() {
+      int int_a {};
+      while (ns::Var) {
+      }
+
+      while (ns::func1()) {
+      }
+
+      while (ns::func2(int_a, int_a)) {
+      }
+
+      while (ns::S{}.Field) {
+      }
+
+      while (ns::S{}.method1()) {
+      }
+      
+      while (ns::S{}.method2(int_a, int_a)) {
+      }
+    $foo[[}]]
+  )cpp",
+      Opts, ExpectedHint{" // namespace ns", "namespace"},
+      ExpectedHint{" // foo", "foo"});
 }

 // FIXME: Low-hanging fruit where we could omit a type hint: