[clangd] Improve BlockEnd inlayhint presentation
#136106
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MythreyaK
force-pushed
the
mythreyak/improve_blockend_inlayhint_presentation
branch
2 times, most recently
from
01e830f to
1dceb5b
Compare
|
@llvm/pr-subscribers-clangd @llvm/pr-subscribers-clang-tools-extra Author: Mythreya (MythreyaK) ChangesPrevious iteration of this PR was here. I retained their first commit as-is, rebased it, and added my changes based on the comments in the thread here. I am working on adding tests, but wanted to get an initial review to make sure I am on the right track. Full diff: https://github.com/llvm/llvm-project/pull/136106.diff 3 Files Affected:
diff --git a/clang-tools-extra/clangd/InlayHints.cpp b/clang-tools-extra/clangd/InlayHints.cpp
index 40a824618f782..bdab2b8a9f377 100644
--- 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 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
// 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 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
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 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
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
diff --git a/clang-tools-extra/clangd/InlayHints.h b/clang-tools-extra/clangd/InlayHints.h
index 6a0236a0ab08a..544f3c8c2d03a 100644
--- a/clang-tools-extra/clangd/InlayHints.h
+++ b/clang-tools-extra/clangd/InlayHints.h
@@ -22,10 +22,16 @@ namespace clang {
namespace clangd {
class ParsedAST;
+struct InlayHintOptions {
+ // Minimum lines for BlockEnd inlay-hints to be shown
+ int HintMinLineLimit{2};
+};
+
/// 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
diff --git a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
index 77d78b8777fe3..bef60f0306f5f 100644
--- 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 DefaultInlayHintOpts{};
+
+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, DefaultInlayHintOpts,
+ Expected...);
}
template <typename... ExpectedHints>
void assertTypeHints(llvm::StringRef AnnotatedSource,
ExpectedHints... Expected) {
ignore(Expected.Side = Right...);
- assertHints(InlayHintKind::Type, AnnotatedSource, Expected...);
+ assertHints(InlayHintKind::Type, AnnotatedSource, DefaultInlayHintOpts,
+ 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, DefaultInlayHintOpts,
+ Expected...);
}
template <typename... ExpectedHints>
-void assertBlockEndHints(llvm::StringRef AnnotatedSource,
- ExpectedHints... Expected) {
+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, Expected...);
+ assertHints(InlayHintKind::BlockEnd, AnnotatedSource, Opts, Expected...);
+}
+
+template <typename... ExpectedHints>
+void assertBlockEndHints(llvm::StringRef AnnotatedSource,
+ ExpectedHints... Expected) {
+ assertBlockEndHintsWithOpts(AnnotatedSource, DefaultInlayHintOpts,
+ 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, DefaultInlayHintOpts).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, DefaultInlayHintOpts,
ExpectedHint{"A: 4", "default1", Left},
ExpectedHint{", B: 1, C: foo()", "default2", Left},
ExpectedHint{"5", "unnamed", Left});
- assertHints(InlayHintKind::Parameter, Code,
+ assertHints(InlayHintKind::Parameter, Code, DefaultInlayHintOpts,
ExpectedHint{"A: ", "explicit", Left});
}
@@ -1528,14 +1544,14 @@ TEST(DefaultArguments, WithoutParameterNames) {
}
)cpp";
- assertHints(InlayHintKind::DefaultArgument, Code,
+ assertHints(InlayHintKind::DefaultArgument, Code, DefaultInlayHintOpts,
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, DefaultInlayHintOpts);
}
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, DefaultInlayHintOpts),
+ IsEmpty());
}
TEST(TypeHints, CallingConvention) {
@@ -1589,7 +1606,8 @@ TEST(TypeHints, CallingConvention) {
TU.PredefineMacros = true; // for the __cdecl
auto AST = TU.build();
- EXPECT_THAT(hintsOfKind(AST, InlayHintKind::Type), IsEmpty());
+ EXPECT_THAT(hintsOfKind(AST, InlayHintKind::Type, DefaultInlayHintOpts),
+ IsEmpty());
}
TEST(TypeHints, Decltype) {
@@ -1671,7 +1689,7 @@ TEST(TypeHints, SubstTemplateParameterAliases) {
)cpp";
assertHintsWithHeader(
- InlayHintKind::Type, VectorIntPtr, Header,
+ InlayHintKind::Type, VectorIntPtr, Header, DefaultInlayHintOpts,
ExpectedHint{": int *", "no_modifier"},
ExpectedHint{": int **", "ptr_modifier"},
ExpectedHint{": int *&", "ref_modifier"},
@@ -1695,7 +1713,7 @@ TEST(TypeHints, SubstTemplateParameterAliases) {
)cpp";
assertHintsWithHeader(
- InlayHintKind::Type, VectorInt, Header,
+ InlayHintKind::Type, VectorInt, Header, DefaultInlayHintOpts,
ExpectedHint{": int", "no_modifier"},
ExpectedHint{": int *", "ptr_modifier"},
ExpectedHint{": int &", "ref_modifier"},
@@ -1722,6 +1740,7 @@ TEST(TypeHints, SubstTemplateParameterAliases) {
)cpp";
assertHintsWithHeader(InlayHintKind::Type, TypeAlias, Header,
+ DefaultInlayHintOpts,
ExpectedHint{": Short", "short_name"},
ExpectedHint{": static_vector<int>", "vector_name"});
}
@@ -2122,30 +2141,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) {
@@ -2305,7 +2335,45 @@ TEST(BlockEndHints, PointerToMemberFunction) {
$ptrmem[[}]]
} // suppress
)cpp",
- ExpectedHint{" // if", "ptrmem"});
+ ExpectedHint{" // if ()", "ptrmem"});
+}
+
+TEST(BlockEndHints, MinLineLimit) {
+ 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;
+ $struct[[}]];
+ $namespace[[}]]
+ void foo() {
+ int int_a {};
+ while (ns::Var) {
+ $var[[}]]
+
+ while (ns::func1()) {
+ $func1[[}]]
+
+ while (ns::func2(int_a, int_a)) {
+ $func2[[}]]
+
+ while (ns::S{}.Field) {
+ $field[[}]]
+
+ while (ns::S{}.method1()) {
+ $method1[[}]]
+
+ while (ns::S{}.method2(int_a, int_a)) {
+ $method2[[}]]
+ $foo[[}]]
+ )cpp",
+ InlayHintOptions{10}, ExpectedHint{" // namespace ns", "namespace"},
+ ExpectedHint{" // foo", "foo"});
}
// FIXME: Low-hanging fruit where we could omit a type hint:
|
HighCommander4
requested changes
Apr 21, 2025
| @@ -22,10 +22,16 @@ namespace clang { | |||
| namespace clangd { | |||
| class ParsedAST; | |||
|
|
|||
| struct InlayHintOptions { | |||
| // Minimum lines for BlockEnd inlay-hints to be shown | |||
There was a problem hiding this comment.
Let's make this comment a bit more specific:
// Minimum height of a code block in lines for a BlockEnd hint to be shown
// Includes the lines containing the braces
| @@ -22,10 +22,16 @@ namespace clang { | |||
| namespace clangd { | |||
| class ParsedAST; | |||
|
|
|||
| struct InlayHintOptions { | |||
| // Minimum lines for BlockEnd inlay-hints to be shown | |||
| int HintMinLineLimit{2}; | |||
There was a problem hiding this comment.
What we discussed in the original issue was that the default should remain 2 for tests, so that we don't have to change a lot of tests / artificially make the blocks long in test code, but that the limit used in production should be increased to 10.
That requires either:
- passing an options struct with the limit set to 10 in the production call site; or
- setting the default to 10 here and adjusting test code accordingly (i.e. explicitly passing
2toDefaultInlayHintOpts)
My preference is the latter, as that ensures that any new call sites use the production value by default (and it seems to be what we do for other options).
(Also, small code style nit: the convention in clangd code seems to be equals initialization for members.)
There was a problem hiding this comment.
What we discussed in the original issue was that the default should remain 2 for tests, so that we don't have to change a lot of tests / artificially make the blocks long in test code, but that the limit used in production should be increased to 10.
Yep I definitely mixed that up, sorry! Should be fixed now.
| @@ -36,9 +36,12 @@ namespace { | |||
| using ::testing::ElementsAre; | |||
| using ::testing::IsEmpty; | |||
|
|
|||
| std::vector<InlayHint> hintsOfKind(ParsedAST &AST, InlayHintKind Kind) { | |||
| constexpr InlayHintOptions DefaultInlayHintOpts{}; | |||
There was a problem hiding this comment.
nit: let's call this DefaultInlayHintOptsForTests (or just DefaultOptsForTests)
| int Field; | ||
| int method1() const; | ||
| int method2(int, int) const; | ||
| $struct[[}]]; |
There was a problem hiding this comment.
this and other unused annotations can be removed
There was a problem hiding this comment.
I left them in because I thought that this would ensure the test fails when these annotations (unexpectedly) generate hints. Do I remove them?
There was a problem hiding this comment.
It's not necessary.
The core assertion the tests make is this one:
EXPECT_THAT(hintsOfKind(AST, Kind),
ElementsAre(HintMatcher(Expected, Source)...));
hintsOfKind(AST, Kind) returns all inlay hints of the specified kind in the code; its result does not depend on the presence or absence of annotations in the code.
ElementsAre(...) then asserts that these returned hints match the provided ExpectedHint objects, including how many there are. If in the test case you have two ExpectedHint objects, and hintsOfKind() starts returning three hints, the test will start failing.
The only purpose of the annotations is so that you can name a range passed in to an ExpectedHint; if you never need to name the range, you don't need the annotation.
There was a problem hiding this comment.
Very helpful explanation, thank you for the clarification!
Will push an amended commit soon. Done.
| $method2[[}]] | ||
| $foo[[}]] | ||
| )cpp", | ||
| InlayHintOptions{10}, ExpectedHint{" // namespace ns", "namespace"}, |
There was a problem hiding this comment.
I'd rather we be a bit more explicit:
InlayhintOptions Opts;
Opts.HintMinLineLimit = 10;
...
assertBlockEndHintsWithOpts(..., Opts, ...);as there may be more options in the future and it's not obvious which is first.
| @@ -147,6 +149,9 @@ std::string summarizeExpr(const Expr *E) { | |||
| } | |||
|
|
|||
| // Literals are just printed | |||
| std::string VisitCXXNullPtrLiteralExpr(const CXXNullPtrLiteralExpr *E) { | |||
| return "nullptr"; | |||
There was a problem hiding this comment.
Could you add a small test case (or extend an existing one) that exercises this change? e.g. something like:
void foo(char *s) {
if (s != nullptr) {
} // if s != nullptr
}There was a problem hiding this comment.
Added it to TEST(BlockEndHints, If)
MythreyaK
force-pushed
the
mythreyak/improve_blockend_inlayhint_presentation
branch
from
21faecd to
fd82e33
Compare
1. Explicitly state a function call 2. Print literal nullptr 3. Escape for abbreviated string 4. Adjust min line limit to 10
Add a new test `MinLineLimit` that ensures hints are generated only when the line threshold is met. Limit is set through `InlayHintOptions.HintMinLineLimit`
MythreyaK
force-pushed
the
mythreyak/improve_blockend_inlayhint_presentation
branch
from
fd82e33 to
22bd7ea
Compare
HighCommander4
approved these changes
Apr 23, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previous iteration of this PR was here. I retained their first commit as-is, rebased it, and added my changes based on the comments in the thread here.
Related issue.
Added one extra test for min-line limit.