[clangd] Implement end-definition-comment inlay hints

[daiyousei-qz]

Summary

This patch implements a new inlay hint feature proposed in https://github.com/clangd/clangd/issues/1634. It introduces a new inlay hint kind BlockEnd which shows a comment-like hint after a definition brace pair, including function/type/namespace. For example,

void foo() {
} ^

In the code shown above, a hint should be displayed at ^ labelling // foo. Such hint only shows when there's no trailing character after the position except whitespaces and optionally ';'.

Also, new configurations are introduced in the inlay hints block

InlayHints:
    BlockEnd: Yes # toggling the feature

[daiyousei-qz]

minor naming fix

[daiyousei-qz]

minor naming fix2 (last fix breaks builds)

[daiyousei-qz]

Requesting for some advice

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

1743

This is unwanted behavior from my understanding. Do you guys have any insight how we could fix this? What I can think of is tracking if we are currently inside a variable declaration and turning off the hint. However, this would have some side effects on

auto f = [] {
   struct S {};
};

[zyounan]

Sorry for chiming in. Left a few nit comments and I hope you don't mind. :)

clang-tools-extra/clangd/InlayHints.cpp

798

Question: Should we restrict the type here? What will happen if D was something unexpected like a TypedefDecl? Would it fall into the logic that makes Label result to <anonymous>?

803–807

Perhaps duplicate the logic from DeclPrinter::VisitEnumDecl? For enum struct, printing enum only might confuse users.

809–814

nit: How about using tagDecl::getKindName()?

[sammccall]

Thanks for doing this! Some high-level comments here, will review implementation next:

There seems to be a lot of "decoration" on the hint text.

If declaring a class "Foo" then we add /* class before and */ after Foo, for a total of 14 characters apart from the name itself. Even if we're mostly adding these hints to mostly-empty lines, I think we should try to be a bit more economical to reduce breaking up the flow of the code.

We could reduce this by some combination of:

• not using comment syntax, e.g. class Foo or (class Foo). We've used pseudo-C++ syntax for other hints, but largely because it was convenient and terse.
• using // rather than /* */ comment syntax where possible, e.g. // class Foo
• dropping the kind, e.g. /* Foo */
• dropping the name, e.g. /* class */

I think my favorite of these is probably // class Foo - this is 5 characters shorter than the current version, provides all the information, uses pseudo-C++ syntax, is consistent with clang-format's namespace comments, and would be suitable for a "insert closing comment" code action in future.

This implies putting the hints after the semicolon (I think it's fine to do this purely textually by examining characters), and choosing some behavior when there's a trailing comment or code (my suggestion would be just to drop the hint in this case).

WDYT?

Naming: EndDefinitionComment is a bit of a mouthful, I'd like something simpler.

"Comment" is the form of the hint, not the thing being hinted, so it seems superfluous. ("Designator" is similar, but I couldn't think of a good alternative there).
"Definition" is OK but a little vague - many things have definitions, so it's not clear what we'll hint. I think "Block" is clearer. It could describe e.g. the end of a long while loop, but I think that's actually OK - we could add that one day, and I think it would make sense to have it controlled by the same setting.

So I think my favorite name would be EndBlock - most critically in the configuration, but also consistently in the code.
(Renaming things is a lot of churn, feel free to defer changes until we've got a solid agreement on a name!)

Configurability: Does the min-lines really need a config knob now?

Fairly few people change the defaults, so:

• we'll need to find the best default in any case, and
• to justify adding an option it should be *very* useful to the people that use it

In particular, "the default is arbitrary" is not itself a strong reason to make it configurable.
The downsides to configuration are extra testing/plumbing, and we also lock ourselves into "number of lines" being the criterion - maybe we want a different heuristic in future and this will interfere.

If we ship without it, we'll get clear feedback on whether the default is good and if customization is needed, and it's easy to add an option later.
Unless I'm missing something, I'd prefer this to be a constant in the code for now.

I think the default of 2 you've got here is worth a try, maybe after some experience a higher number will be better.

[sammccall]

clang-tools-extra/clangd/InlayHints.cpp

345

Not sure why if need to handle this + EnumDecl, can we just handle VisitTagDecl to cover both?

627

this part of the comment is just echoing the code (too much detail). Maybe something like

Only add end-block hints if the rest of the line is trivial.

In particular, we avoid hinting:
 - if a user comment might provide the same information
 - when it might not be clear what construct the hint is for

(The latter is mostly important if we switch to line-comments: if there's }} on a line we don't want to hint the inner one!)

628

Not only is this rare, but it's not clear hinting is particularly undesirable here - I'd just drop this comment.

640

The +1 is suspicious. if (!Suffix.consume_front("{")) return false; would be clearer and hold up better in the presence of macros.

710

I don't really like this signature change.

I understand the goal to avoid duplicating the range computation but it seems unlikely to be critical.
Also, the caller could get the line numbers of the {} from the SourceManager and compare those, which should be cheaper than computing the range, so we wouldn't be duplicating all of the work.

786

I believe it makes more sense to target the } rather than the whole declaration here - we're really talking about what the } closes, rather than what the declaration itself is.

This is sort of conceptual - at the protocol level the target range doesn't really make a difference.

There is one practical difference though: this affects whether we allow hinting in scenarios where (part of) the declaration is expanded from macros.

It's also more consistent with the other hints, which (generally) use fairly small ranges. So if we use the range for something else in future it's likely to play better.

789

I'm not sure we're using the right range here. Consider:

const some::long<type, name>* function
   many params, therefore spanning, lines) {}

I don't think hinting } here is useful: the relevant distance is from { rather than the start of the declaration.

793

This is kind of true for all inlay hints (not just end-definition), and we haven't really decided to do so, so I don't think we need this comment.

797

why is printName OK here and not below?

798

Yeah, this case-enumeration feels a bit fuzzy to me, like we had more structured information earlier and are now trying to recover it.

I'd like the signature of this function to be a bit more concrete, e.g. taking parameters:

• SourceRange Braces
• StringRef Kind (e.g. "class")
• DeclarationName Name (we can overload/change the signature of getSimpleName)

This is also more similar to the other functions in this family.

800

if you're not going to say what the issues are then there's not much point in the comment!

803–807

I'm torn here: "enum class Foo" is wordy so "enum Foo" is tempting to me. Don't have a strong opinion.

regardless, printing the tag for class but not struct is definitely wrong :-)

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

1643

can you add testcases where:

• the function name is an operator
• the function is templated

1653

we're not adding the kind here as in "function foo".

Maybe that's OK, "function" is long, doesn't appear in the C++ syntax, and is mostly obvious.
But in that case I'd like to have some hint, maybe /* foo() */? (Not the actual param list of course, the parens would always be empty)

If you don't think we need any hint, then I'd question whether we need "class" etc for class hints!

1657

can you add a test with a lambda?
I think the hint should probably just be lambda or (lambda), this probably needs special handling.

It definitely shouldn't be operator() or (lambda at somefile.cc:47).

1664

can you add a template method?
(These are weird because they're functiondecls rather than methoddecls, I think)

1669

can you add an out-of-line definition for method2?

In particular we need to decide whether the hint will be Test::method2 or method2.
(This also serves as a test for definitions of functions via qualified names - they should work the same way)

1694

I'm not certain this is a good idea - we're printing types, these can be very long, have various levels of sugar.

Elsewhere where we print types we need to apply a length cutoff, we could try to do that here or just bail out for now on certain types of names.

1708

I think this should just be "namespace" for consistency with clang-format

This probably goes for all unnamed decls, it also matches the C++ syntax

1743

This behavior looks totally fine to me, I certainly wouldn't go out of my way to change it.

if s2 were on the same line, we'd suppress the class hint, and I think that's also fine (esp if we switch to line-comment syntax)

[daiyousei-qz]

• addressed many review comment

[daiyousei-qz]

@zyounan np! Feel free to leave your review comments!

@sammccall Thank you for you insightful and detailed review! I addressed many review comments in the updated patch and left things I'm not very sure.

Regarding to Naming, I agree that EndDefinitionComment sounds so tedious but I couldn't come up with a better name. However, I think EndBlock also feel a little strange to me as a public name. If I come across a setting key in InlayHints section named EndBlock, honestly I would be confused by what it would do ("end" could be a verb, and "block" usually make me to think of the local scope block). What about AfterBlock or AfterDefinition?

Regarding to Configuration, I suppose the minimum line number of definition is a very natural parameter for this feature. I doubt we could come up with some good heuristic to conditionally show this hint. Considering how straightforward this parameter its implementation is, I'd prefer to having this around if possible. But I'll let you to decide.

clang-tools-extra/clangd/InlayHints.cpp

789

I agree. What do you think is a good way get the range of the definition block? What I can think of is to walking backwards the expanded token buffer from the ending '}' and searching for the balanced '}'.

797

I'm using printName here to print names for operator overload. Do you have any advice?

803–807

Fixed enum struct. Honestly, just learnt there's such a thing, lol.

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

1643

Added template function. I don't quite understand what you mean by "the function name is an operator". I'm assuming you meant operator overload so closing this one as covered below.

1653

I think just // foo is good enough. Having no "qualifier" could be a privilege for functions since they are usually the majority of the source code. When reading through the source code, people will get that if no prefix is attached, then it's a function. (OFC they'll also see // namespace and .etc, but rare)

Honestly, IMO // foo() is worse than just the name. It could mislead me to think that this may be a complete signature while it is definitely not.

1657

I would argue we don't need hint for a lambda. This hint is only added to long definitions that may introduce a symbol. It is helpful to highlight the boundaries of different declarations. Because of the implementation assumes the hint is added to a NamedDecl, lambda should be safe.

1694

I agree we should avoid very long hints being displayed. Instead of a length cutoff on type, however, I suppose a limit on the entire hint would be much simpler to implement. Do you think that's okay?

[daiyousei-qz]

For a quick peek to the hint:

[zyounan]

Thank you for the update. A few questions:

• Can you add a test with macro expansion? Would we get two inlay hints right after the spelling location? e.g.,

#define Decl_Record \
  struct Widget {  \
    [very long definition that reach `EndDefinitionCommentMinLines`]. \
    struct SubWidget { \
      [another chunk of members.] \
    // not ending with a bracket here!

//  but appears here:
//          v                       v
Decl_Record } // struct SubWidget ; } // struct Widget (Hmm...) ;

• I was thinking if we should add some test cases for C. (I have learned that it is allowed to write a declaration in an expression while C++ doesn't.) For example,

int main(void) {
  _Alignof(struct S {
    [long definition]
  } // Is it suitable to hint here? What do you think?
  );
}

clang-tools-extra/clangd/InlayHints.cpp

338–339

nit: bail out early?

802

nit: assert(Label.empty() && "Label should be empty with FunctionDecl"). might be helpful for debugging.

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

1793

typo: becaus -> because?

[sammccall]

In D150635#4348702, @daiyousei-qz wrote:

Regarding to Naming, I agree that EndDefinitionComment sounds so tedious but I couldn't come up with a better name. However, I think EndBlock also feel a little strange to me as a public name. If I come across a setting key in InlayHints section named EndBlock, honestly I would be confused by what it would do ("end" could be a verb, and "block" usually make me to think of the local scope block). What about AfterBlock or AfterDefinition?

Agreed that "End" looks too much like a verb here. "After" isn't quite right, again because we're trying to describe the thing we're hinting, not where the hint is positioned.
I prefer "Block" over "Definition" exactly because this sort of hint should also be added to local scope blocks (e.g. if bodies) later.

Does BlockEnd address your concern? ("Block" can be a verb but I think there's no risk of confusion here)

Regarding to Configuration, I suppose the minimum line number of definition is a very natural parameter for this feature. I doubt we could come up with some good heuristic to conditionally show this hint. Considering how straightforward this parameter its implementation is, I'd prefer to having this around if possible. But I'll let you to decide.

Right: It's tempting to make it an option, because we don't know what the value should be, and an option is easy to implement.

But these are not good reasons to make it an option:

• an option doesn't substantially change the importance of picking a good default heuristic, because 95+% of people will use it anyway. If we can't find a good default, we shouldn't implement the feature.
• the vast majority of the cost of an option is maintenance and user-facing complexity, rather than implementation

clang-tools-extra/clangd/InlayHints.cpp

331

I think this is a wrong/incomplete check, e.g. Foo(const Foo&) = default is a definition but doesn't have a brace range.

Maybe it's less redundant just to try to get the brace range and then check if it's valid.

345

This is marked done but isn't done.
I'd suggest something like

VisitTagDecl(*D) {
  if (...) {
    string Kind = D->getKindName();
    if (auto *ED = dyn_cast<EnumDecl>(D); ED->isScoped())
     S += ED->isScopedUsingClassTag() ? " class" : " struct";
    addEndDefinitionCommentHint(*D, Kind);
  }
}

782

SkipSemicolon doesn't need to be optional, a trailing semicolon never adds any ambiguity about what the hint is for

788

This is too much arithmetic and fiddly coupling between this function and shouldHintEndDefinitionComment.

Among other things, it allows for confusion between unicode characters (UTF-32), clang bytes (UTF-8), and LSP characters (usually UTF-16). And we do have a bug here: shouldHintEndDefinition provides SkippedChars in clang bytes, but you add it to end.character below which is in LSP characters.

---

Let's redesign a little...

We have a } on some line. We want to compute a sensible part of that line to attach to.
A suitable range may not exist, in which case we're going to omit the hint.

• The line consists of text we don't care about , the }, and then some mix of whitespace, "trivial" punctuation, and "nontrivial" chars.
• the range should always start at the }, since that's what we're really hinting
• to get the hint in the right place, the range should end after the trivial chars, but before any trailing whitespace
• if there are any nontrivial chars, there's no suitable range

so something like:

optional<Range> findBraceTargetRange(SourceLocation CloseBrace) {
  auto [File, Offset] = SM.getDecomposedLoc(SM.getFileLoc(CloseBrace));
  if (File != MainFileID) return std::nullopt;
  StringRef RestOfLine = MainFileBuf.substr(Offset).split('\n').first.rtrim();
  if (!RestOfLine.consume_front("{")) return;
  if (StringRef::npos != Punctuation.find_first_of(" ,;")) return std::nullopt;
  return {offsetToPosition(MainFileBuf, Offset), offsetToPosition(MainFileBuf, Result.bytes_end() - MainFileBuf.bytes_end()};
}

and then call from addEndDefinitionComment, the result is LSPRange already.

789

What do you think is a good way get the range of the definition block?

For class etc: TagDecl::getBraceRange()
For function: getBody()->get{L,R}BracLoc().
For namespace, I don't see a way in the AST. I think it's fine to use the full range, as the namespace and { will almost always be on the same line.

These ranges can be extracted from the specific types in the Visit* methods and passed into addEndDefinitionCommentHint

797

only to be consistent: either print in the same way here vs below, or document why you're doing different things.

(From what's written in the code, it's not clear what the downside is to printName such that we don't use it everywhere)

797

I would print the DeclarationName only, probably
and try to avoid doing different things for different types of decl

798

this is not quite done: we're still passing in the NamedDecl which offers the opportunity to go digging back into the class hierarchy. I think it's better to pass the name and source range in directly to make it clear we're not doing that.

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

1643

oops, indeed it is - can you move one of the operator tests into "methods" and add a non-method one to "functions"?
(Operators aren't really different enough to be separated out, I think)

1653

I'm skeptical, but let's give this a try, we can always add it later.

1657

Up to you, I think a hint would be helpful but no hint is also fine.

In any case, please add a test showing the behavior.

This hint is only added to long definitions that may introduce a symbol

FWIW I don't think this is the greatest value from the hint - rather that *when the scope changes* we get some extra context that the local syntax doesn't provide. (This is why I think lambdas, for loops etc should eventually be hinted)

1689

I think we *probably* just want method2 here: showing qualifiers seems inconsistent with how we otherwise prefer brevity over disambiguation (e.g. not showing params etc)

[daiyousei-qz]

Address review comments

[daiyousei-qz]

Addressed most review comments.

I'm currently using the name "BlockEnd" as suggested. Though I'm proposing a name like "DeclBlockEnd" to make it clearer. What do you think? If we want to broaden the hint, maybe another one named "StmtBlockEnd" could be added.

clang-tools-extra/clangd/InlayHints.cpp

338–339

No longer needed as merged into VisitTagDecl

710

As per another comment below, this change is kept.

782

Yes, no ambiguity. But the following hint looks weird to me:

void foo() {
}; // foo

Since this isn't that complicated to filter them out, I'd prefer making it more precise.

786

Now targeting

1. "}" if this is the only character
2. "};" or "} ;" or alike for enum/class/struct

788

Done, also moved min-line and max-length logic to this function. Btw, I think we should avoid offsetToPosition as much as possible. It is a large overhead considering inlay hints are recomputed throughout the entire file for each edit. I frequently work with source code that's nearly 1MB large (Yes, I don't think we should have source file this large, but it is what it is).

798

Done

802

No longer needed as this assert is removed.

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

1643

Merged test cases

1657

I agree, but the threshold of showing such hint should probably be more conservative since the number could blow up really quickly. I definitely think a hint for a long long if/for/while statement could be very helpful to understand the code structure.

Added test case for lambda. Close this for now since it's not going to be resolved in this patch.

1689

I disagree with this and think the current behavior is consistent. The name we show is exactly the same with what is used to define the symbol. Also, I think the type name is an essential part of a method name.

1694

Added a max length limit of 50 for now.

1793

Good catch!

[sammccall]

Though I'm proposing a name like "DeclBlockEnd" to make it clearer

I prefer the current name. DeclBlockEnd is both too long and contains a cryptic abbreviation (should be DeclarationBlockEnd). And I don't think distinguishing between declaration blocks and flow-control blocks is likely to be helpful.

clang-tools-extra/clangd/InlayHints.cpp

632

can you move this function below the related addBlockEndHint?

635

We actually already have a configurable inlay hint length limit.
It has the wrong name (TypeNameLimit) , but I think we should use it anyway (and maybe try to fix the name later).

(I'm not sure making this configurable was a great idea, but given that it exists, people will expect it to effectively limit the length of hints)

647

nit: Trimed => Trimmed, or just shorten the name

scope of var can be reduced here:

if (StringRef TrailingText = ...; !TrailingText.empty() && ...)
  return std::nullopt

654

this should be getFileLoc(BraceRange.getBegin()), I think?

655

we can drop all the invalid checks. These are ~never going to fire, and aren't the best condition to check when they do fire.

For RBrace, we already know it's in the main file (can't be invalid)
For LBrace, something weird like token-pasting or an inline #include needs to happen for this to be invalid. But if you want to check this, a better test would be to decompose LBrace like you do RBrace and check that the FileID is right. Because if you somehow end up in a different file, the line number will be "valid" but the comparison is not meaningful.

662

motivating comment would be useful here:
// Don't show hints on trivial blocks like class X {};

673

please don't do this, call sourceLocToPosition unless you have benchmarks showing this is a bottleneck on real-world code.

lspLength and direct manipulation of line/character is unneccesarily subtle.
(If the performance matters, there's no need to be computing the LSP line of the lbrace at all - we never use it - this is one reason I think this function has the wrong signature)

710

Please revert this unless you can show a significant performance difference in a real-world setting.

788

also moved min-line and max-length logic to this function

I'd prefer you to move them back. As specified, that function is otherwise strictly about examining the textual source code around the closing brace. Now it's muddled: it also looks at the opening brace, and does lookups into line tables.

You seem to be aiming to optimize an operation that you think is expensive, but I don't see any evidence (measurements) that this implementation is actually faster or that the difference matters.

Btw, I think we should avoid offsetToPosition as much as possible

I understand the concern: computing inlay hints is quadratic. However things are *universally* done this way in clangd, and diverging here won't significantly improve performance but makes the code much harder to understand in context.

In practice we haven't found places where overheads that are length(file)*length(LSP response) are significant compared to parse times. If you have such examples, it would be great to gather & share profiles and propose a solution.
I suspect we could maintain some line lookup data structure that gets updated when source code updates come in over LSP, or something. But micro-optimizing individual codepaths isn't going to get us there: if two offsetToPosition() calls are bad, then one is still bad.

clang-tools-extra/clangd/Protocol.h

1681

nit: update the comment syntax

[daiyousei-qz]

clang-tools-extra/clangd/InlayHints.cpp

673

I argue performance does matter here. I eye-balled inlay hint compute time in clangd's output window from vscode. On "./clang/lib/Sema/SemaOpenMP.cpp" which is around 1MB, using sourceLocToPosition roughly takes around 1250~1350ms. However, using two offsetLocToPosition usually use 1400~1500ms. I think 100ms delta is already a noticeable difference.

[sammccall]

Thanks, I'll try to repro those results.
100ms is significant in absolute terms, but >1s seems unacceptably slow.
I believe VSCode always sends ranges along with latency-sensitive hint requests, I think we currently just post-filter. If we propagate the limits deeper we should be able to filter much earlier and never hit these codepaths at all for 99% of the file.

[sammccall]

clang-tools-extra/clangd/InlayHints.cpp

673

I'm unable to reproduce these results. After verifying EndBlock hints work in an editor:

$ bin/clangd --check=../clang/lib/Sema/SemaOpenMP.cpp
I[15:50:26.357] clangd version 17.0.0
I[15:50:26.358] Features: linux+debug-tidy
...
 I[15:50:26.358] Testing on source file /usr/local/google/home/sammccall/src/llvm-project/clang/lib/Sema/SemaOpenMP.cpp
...
I[15:50:26.428] Building preamble...
I[15:50:31.097] Indexing headers...
I[15:50:32.027] Built preamble of size 53468188 for file /usr/local/google/home/sammccall/src/llvm-project/clang/lib/Sema/SemaOpenMP.cpp version null in 5.60 seconds
I[15:50:32.032] Building AST...
I[15:50:38.857] Indexing AST...
I[15:50:38.975] Building inlay hints
I[15:50:38.993] Building semantic highlighting
I[15:50:39.050] Testing features at each token (may be slow in large files)
...

So that's 18ms for inlay hints after 5.6 seconds to parse the preamble and 6 seconds to parse the main file. (This is a debug build, opt must be faster).

This is on a fairly powerful machine, but surely it can't be 1000x faster than yours - something else must be going on.

[daiyousei-qz]

I'm not sure but 18ms is definitely wrong here, especially for a debug build. In your log, even semantic highlighting builds in 60ms, which looks incorrect. I tried again with TOT clangd and noticed the following pattern.

• If I scroll with vscode, inlay hints typically computes in 10~20ms
• If I edit the source code, inlay hints computes in around 600ms

I'm not sure why that's happening. Maybe @nridge could have a better answer. Somehow, this patch seems introduce a significant slowdown. I'll investigate further to see why that's happening.

[sammccall]

In D150635#4366730, @daiyousei-qz wrote:

I'm not sure but 18ms is definitely wrong here, especially for a debug build. In your log, even semantic highlighting builds in 60ms, which looks incorrect. I tried again with TOT clangd and noticed the following pattern.

• If I scroll with vscode, inlay hints typically computes in 10~20ms
• If I edit the source code, inlay hints computes in around 600ms

That sounds exactly like rebuilding the AST takes ~600ms and computing inlay hints takes <20ms.

After an edit, a request for inlay hints will wait while the AST rebuilds before computing inlay hints, so the request latency will show as ~620ms.

If you think you're seeing something else, please provide repro instructions and logs (with -log=verbose)

[daiyousei-qz]

Yeah, makes sense. Thank you for your insight! I missed the fact that this time include AST build time.

I manually injected a time region into inlayHints, here's part of logs printed for scrolling "offsetToPosition" build:

I[23:36:43.055] <-- textDocument/hover(21)
I[23:36:43.057] --> reply:textDocument/hover(21) 1 ms
I[23:36:43.057] --> textDocument/clangd.fileStatus
I[23:36:44.422] <-- textDocument/inlayHint(22)
E[23:36:44.612] ++++++ Inlay hints takes 189 ms to compute
I[23:36:44.612] --> reply:textDocument/inlayHint(22) 189 ms
I[23:36:44.612] --> textDocument/clangd.fileStatus
I[23:36:44.875] <-- textDocument/inlayHint(23)
E[23:36:45.053] ++++++ Inlay hints takes 178 ms to compute
I[23:36:45.054] --> reply:textDocument/inlayHint(23) 179 ms
I[23:36:45.054] --> textDocument/clangd.fileStatus
I[23:36:45.686] <-- textDocument/inlayHint(24)
E[23:36:45.865] ++++++ Inlay hints takes 178 ms to compute
I[23:36:45.865] --> reply:textDocument/inlayHint(24) 179 ms
I[23:36:45.865] --> textDocument/clangd.fileStatus

And here's logs scrolling "sourceLocToPosition" + "lspLength":

I[23:48:22.875] <-- textDocument/inlayHint(20)
E[23:48:22.890] ++++++ Inlay hints takes 14 ms to compute
I[23:48:22.890] --> reply:textDocument/inlayHint(20) 14 ms
I[23:48:22.890] --> textDocument/clangd.fileStatus
I[23:48:22.907] <-- textDocument/inlayHint(21)
E[23:48:22.922] ++++++ Inlay hints takes 14 ms to compute
I[23:48:22.923] --> reply:textDocument/inlayHint(21) 15 ms
I[23:48:22.923] --> textDocument/clangd.fileStatus
I[23:48:22.974] <-- textDocument/inlayHint(22)
E[23:48:22.988] ++++++ Inlay hints takes 13 ms to compute
I[23:48:22.989] --> reply:textDocument/inlayHint(22) 14 ms
I[23:48:22.989] --> textDocument/clangd.fileStatus
I[23:48:23.787] --> $/progress
I[23:48:23.941] <-- textDocument/inlayHint(23)
E[23:48:23.955] ++++++ Inlay hints takes 14 ms to compute
I[23:48:23.956] --> reply:textDocument/inlayHint(23) 14 ms
I[23:48:23.956] --> textDocument/clangd.fileStatus

(edited, sorry I forgot to restart server)

As this is more than 10x slower, I'm holding my position that we shouldn't use offsetToPosition for LSP features triggered per edit/scroll. 150ms is more than noticeable.

If you want to repro on your side, replace computation of HintStart/HintEnd with the following code

Position HintStart = offsetToPosition(MainFileBuf, RBraceOffset);
Position HintEnd =
    offsetToPosition(MainFileBuf, RBraceOffset + HintRangeText.size());

Btw, it's always fascinating to see how modern computer system is so fast, and in the meantime still so "underpowered" :)

[daiyousei-qz]

• Move computeBlockEndHintRange
• Correct label length limit logic
• Correct line number computation for '{'
• Address other review comments

[daiyousei-qz]

Addressed review comments except for those we don't have an agreement yet.

clang-tools-extra/clangd/InlayHints.cpp

635

TypeNameLimit was introduced to limit the length of "DeducedTypes" only. I don't think using that number for all hints is a good idea because different hints appear in different contexts. For deduced type hint, it is displayed in the middle of the actual code, so it must be concise to not overwhelm the actual code. However, this hint is usually displayed at the end of an almost empty line. A much larger number of characters should be allowed.

(I'm not arguing here, but IMO such options are definitely helpful. Not everybody would fork llvm-project and build their own version of clangd binary. Without options to configure length of hints, people would disable "DeducedTypes" entirely if they cannot tolerate the default length limit, while this feature is definitely a cool thing to turn on. Personally, I actually think clangd has too little option compared to say rust-analyzer. But it's just my understanding)

654

Thanks for pointing out! As per comments below, calling getLineNumber instead of getSpellingLineNumber in the latest version.

788

I'd prefer you to move them back. As specified, that function is otherwise strictly about examining the textual source code around the closing brace. Now it's muddled: it also looks at the opening brace, and does lookups into line tables.

Moved max-length logic back, but kept the block line number check there because the line number of RBrace is readily available in this function. I don't see why we need to duplicate code to obtain that line number.

[nridge]

clang-tools-extra/clangd/InlayHints.cpp

635

For deduced type hint, it is displayed in the middle of the actual code, so it must be concise to not overwhelm the actual code. However, this hint is usually displayed at the end of an almost empty line. A much larger number of characters should be allowed.

Another consideration besides the location of the hint that is worth keeping in mind, is what type of content is being printed.

Type names in C++ are composable in ways that identifiers are not (e.g. vector<basic_string<char, char_traits<char>, allocator<char>, allocator<basic_string<...>> etc.), such that there is a need to limit type hints that doesn't really apply to say, parameter hints.

So, a question I have here is: can template argument lists appear in an end-definition-comment hint?

For example, for:

template <typename T>
struct S {
  void foo();
};

template <typename T>
void S::foo() {
}  // <--- HERE

is the hint at the indicated location S::foo(), or S<T>::foo()? In the latter case, we can imagine the hint getting long due to the type parameters, and we may want to consider either limiting its length, or tweaking the code to not print the type parameters.

[daiyousei-qz]

clang-tools-extra/clangd/InlayHints.cpp

635

Yes, currently this hint is only shown if the total length of hint label is less than 60 characters. I believe what we display should be exactly what is used to define the symbol. In your example,

template <typename T>
struct S {
  void foo();
};

template <typename T>
void S<T>::foo() {
}  // S<T>::foo

Basically, "[A]" and "[B]" is the same text (OFC excluding whitespaces).

void [A]() {
} // [B]

The hint won't be displayed if "[B]" is more than 60 characters.

[sammccall]

150ms is more than noticeable

Fair enough. I also see we *are* avoiding the quadratic algorithm in other places in inlay hints, and that we can't easily reuse the getHintRange() function in any case without switching to handling tokens instead of strings. So overall I think the current version is fine. Thanks for digging into this!

clang-tools-extra/clangd/Config.h

147

this needs to be false initially (a month or so?) so we have a chance to try it out in practice, ensure it's not too spammy/has crashes etc, then we can flip on by default

clang-tools-extra/clangd/InlayHints.cpp

635

OK, I'm sold on wanting a higher limit here, and a hard-coded 60 as a limit on the hint seems simple and reasonable for now. We can iterate on this.

(There's another case with arbitrary types being printed: operator vector<int> or so)

710

OK, I'm convinced we need this. Not for performance, but because it's not natural to obtain a closed token range in this case where we're manipulating character by character.

846

I'd prefer sourceLocToPosition(SM, RBraceLoc.getLocationWithOffset(HintRangeText.size())) which would avoids such low-level conversion between coordinate systems, and seems to perform just fine (we're going to hit SourceManager's caches).

Will leave this up to you though.

clang-tools-extra/clangd/Protocol.cpp

1462

nit: move the comment to the return statement rather than repeating it

[daiyousei-qz]

• Address remaining comments

[daiyousei-qz]

Addressed the remaining comments. Thanks everyone for helping review and improve this patch!

clang-tools-extra/clangd/InlayHints.cpp

846

Yeah, this is definitely clearer. Updated.

[sammccall]

Oh no, I just saw this never landed - I suspect you were waiting for me to commit it, and I was expecting you to do it!

I've rebased and I'll commit it for you now.

clang-tools-extra/clangd/Config.h

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;
+    bool BlockEnd = false;
     // Limit the length of type names in inlay hints. (0 means no limit)
     uint32_t TypeNameLimit = 32;
   } InlayHints;
 
   struct {
     /// Controls highlighting kinds that are disabled.
     std::vector<std::string> DisabledKinds;
     /// Controls highlighting modifiers that are disabled.

clang-tools-extra/clangd/ConfigCompile.cpp

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.BlockEnd)
+      Out.Apply.push_back([Value(**F.BlockEnd)](const Params &, Config &C) {
+        C.InlayHints.BlockEnd = Value;
+      });
     if (F.TypeNameLimit)
       Out.Apply.push_back(
           [Value(**F.TypeNameLimit)](const Params &, Config &C) {
             C.InlayHints.TypeNameLimit = Value;
           });
   }
 
   void compile(Fragment::SemanticTokensBlock &&F) {

clang-tools-extra/clangd/ConfigFragment.h

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;
+    /// Show defined symbol names at the end of a definition block.
+    std::optional<Located<bool>> BlockEnd;
     /// Limit the length of type name hints. (0 means no limit)
     std::optional<Located<uint32_t>> TypeNameLimit;
   };
   InlayHintsBlock InlayHints;
 
   /// Configures semantic tokens that are produced by clangd.
   struct SemanticTokensBlock {
     /// Disables clangd to produce semantic tokens for the given kinds.

clang-tools-extra/clangd/ConfigYAML.cpp

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("BlockEnd", [&](Node &N) {
+      if (auto Value = boolValue(N, "BlockEnd"))
+        F.BlockEnd = *Value;
+    });
     Dict.handle("TypeNameLimit", [&](Node &N) {
       if (auto Value = uint32Value(N, "TypeNameLimit"))
         F.TypeNameLimit = *Value;
     });
     Dict.parse(N);
   }
 
   void parse(Fragment::SemanticTokensBlock &F, Node &N) {

clang-tools-extra/clangd/InlayHints.cpp

public:
   bool VisitFunctionDecl(FunctionDecl *D) {
     if (auto *FPT =
             llvm::dyn_cast<FunctionProtoType>(D->getType().getTypePtr())) {
       if (!FPT->hasTrailingReturn()) {
         if (auto FTL = D->getFunctionTypeLoc())
           addReturnTypeHint(D, FTL.getRParenLoc());
       }
     }
+    if (Cfg.InlayHints.BlockEnd && D->isThisDeclarationADefinition()) {
+      // We use `printName` here to properly print name of ctor/dtor/operator
+      // overload.
+      if (const Stmt *Body = D->getBody())
+        addBlockEndHint(Body->getSourceRange(), "", printName(AST, *D), "");
+    }
+    return true;
+  }
+
+  bool VisitTagDecl(TagDecl *D) {
+    if (Cfg.InlayHints.BlockEnd && D->isThisDeclarationADefinition()) {
+      std::string DeclPrefix = D->getKindName().str();
+      if (const auto *ED = dyn_cast<EnumDecl>(D)) {
+        if (ED->isScoped())
+          DeclPrefix += ED->isScopedUsingClassTag() ? " class" : " struct";
+      };
+      addBlockEndHint(D->getBraceRange(), DeclPrefix, getSimpleName(*D), ";");
+    }
+    return true;
+  }
+
+  bool VisitNamespaceDecl(NamespaceDecl *D) {
+    if (Cfg.InlayHints.BlockEnd) {
+      // For namespace, the range actually starts at the namespace keyword. But
+      // it should be fine since it's usually very short.
+      addBlockEndHint(D->getSourceRange(), "namespace", getSimpleName(*D), "");
+    }
     return true;
   }
 
   bool VisitLambdaExpr(LambdaExpr *E) {
     FunctionDecl *D = E->getCallOperator();
     if (!E->hasExplicitResultType())
       addReturnTypeHint(D, E->hasExplicitParameters()
                                ? D->getFunctionTypeLoc().getRParenLoc()
bool isPrecededByParamNameComment(const Expr *E, StringRef ParamName) {
     // Other than that, the comment must contain exactly ParamName.
     if (!SourcePrefix.consume_back(ParamName))
       return false;
     SourcePrefix = SourcePrefix.rtrim(IgnoreChars);
     return SourcePrefix.endswith("/*");
   }
 
   // If "E" spells a single unqualified identifier, return that name.
   // Otherwise, return an empty string.
   static StringRef getSpelledIdentifier(const Expr *E) {
     E = E->IgnoreUnlessSpelledInSource();
 
     if (auto *DRE = dyn_cast<DeclRefExpr>(E))
       if (!DRE->getQualifier())
         return getSimpleName(*DRE->getDecl());
 
     if (auto *ME = dyn_cast<MemberExpr>(E))
       if (!ME->getQualifier() && ME->isImplicitAccess())
         return getSimpleName(*ME->getMemberDecl());
 
     return {};
   }
 
   NameVec chooseParameterNames(SmallVector<const ParmVarDecl *> Parameters) {
     NameVec ParameterNames;
     for (const auto *P : Parameters) {
       if (isExpandedFromParameterPack(P)) {
         // If we haven't resolved a pack paramater (e.g. foo(Args... args)) to a
         // non-pack parameter, then hinting as foo(args: 1, args: 2, args: 3) is
         // unlikely to be useful.
         ParameterNames.emplace_back();
       } else {
         auto SimpleName = getSimpleName(*P);
         // If the parameter is unnamed in the declaration:
         // attempt to get its name from the definition
         if (SimpleName.empty()) {
           if (const auto *PD = getParamDefinition(P)) {
             SimpleName = getSimpleName(*PD);
           }
         }
         ParameterNames.emplace_back(SimpleName);
       }
     }
 
     // Standard library functions often have parameter names that start
     // with underscores, which makes the hints noisy, so strip them out.
     for (auto &Name : ParameterNames)
       stripLeadingUnderscores(Name);
 
     return ParameterNames;
   }
 
   // for a ParmVarDecl from a function declaration, returns the corresponding
   // ParmVarDecl from the definition if possible, nullptr otherwise.
   static const ParmVarDecl *getParamDefinition(const ParmVarDecl *P) {
     if (auto *Callee = dyn_cast<FunctionDecl>(P->getDeclContext())) {
       if (auto *Def = Callee->getDefinition()) {
         auto I = std::distance(Callee->param_begin(),
                                llvm::find(Callee->parameters(), P));
         if (I < Callee->getNumParams()) {
           return Def->getParamDecl(I);
         }
       }
static StringRef getSimpleName(const NamedDecl &D) {
     return StringRef();
   }
 
   // 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) {
+    auto LSPRange = getHintRange(R);
+    if (!LSPRange)
+      return;
+
+    addInlayHint(*LSPRange, Side, Kind, Prefix, Label, Suffix);
+  }
+
+  void addInlayHint(Range LSPRange, 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(Parameter, Parameters);
       CHECK_KIND(Type, DeducedTypes);
       CHECK_KIND(Designator, Designators);
+      CHECK_KIND(BlockEnd, BlockEnd);
 #undef CHECK_KIND
     }
 
-    auto LSPRange = getHintRange(R);
-    if (!LSPRange)
-      return;
-    Position LSPPos = Side == HintSide::Left ? LSPRange->start : LSPRange->end;
+    Position LSPPos = Side == HintSide::Left ? LSPRange.start : LSPRange.end;
     if (RestrictRange &&
         (LSPPos < RestrictRange->start || !(LSPPos < RestrictRange->end)))
       return;
     bool PadLeft = Prefix.consume_front(" ");
     bool PadRight = Suffix.consume_back(" ");
     Results.push_back(InlayHint{LSPPos, (Prefix + Label + Suffix).str(), Kind,
-                                PadLeft, PadRight, *LSPRange});
+                                PadLeft, PadRight, LSPRange});
   }
 
   // Get the range of the main file that *exactly* corresponds to R.
   std::optional<Range> getHintRange(SourceRange R) {
     const auto &SM = AST.getSourceManager();
     auto Spelled = Tokens.spelledForExpanded(Tokens.expandedTokens(R));
     // TokenBuffer will return null if e.g. R corresponds to only part of a
     // macro expansion.
#undef CHECK_KIND
   }
 
   void addDesignatorHint(SourceRange R, llvm::StringRef Text) {
     addInlayHint(R, HintSide::Left, InlayHintKind::Designator,
                  /*Prefix=*/"", Text, /*Suffix=*/"=");
   }
 
   bool shouldPrintTypeHint(llvm::StringRef TypeName) const noexcept {
     return Cfg.InlayHints.TypeNameLimit == 0 ||
            TypeName.size() < Cfg.InlayHints.TypeNameLimit;
   }
 
+  void addBlockEndHint(SourceRange BraceRange, StringRef DeclPrefix,
+                       StringRef Name, StringRef OptionalPunctuation) {
+    auto HintRange = computeBlockEndHintRange(BraceRange, OptionalPunctuation);
+    if (!HintRange)
+      return;
+
+    std::string Label = DeclPrefix.str();
+    if (!Label.empty() && !Name.empty())
+      Label += ' ';
+    Label += Name;
+
+    constexpr unsigned HintMaxLengthLimit = 60;
+    if (Label.length() > HintMaxLengthLimit)
+      return;
+
+    addInlayHint(*HintRange, HintSide::Right, InlayHintKind::BlockEnd, " // ",
+                 Label, "");
+  }
+
+  // Compute the LSP range to attach the block end hint to, if any allowed.
+  // 1. "}" is the last non-whitespace character on the line. The range of "}"
+  // is returned.
+  // 2. After "}", if the trimmed trailing text is exactly
+  // `OptionalPunctuation`, say ";". The range of "} ... ;" is returned.
+  // 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] =
+        SM.getDecomposedLoc(SM.getFileLoc(BraceRange.getBegin()));
+    auto RBraceLoc = SM.getFileLoc(BraceRange.getEnd());
+    auto [RBraceFileId, RBraceOffset] = SM.getDecomposedLoc(RBraceLoc);
+
+    // Because we need to check the block satisfies the minimum line limit, we
+    // require both source location to be in the main file. This prevents hint
+    // to be shown in weird cases like '{' is actually in a "#include", but it's
+    // rare anyway.
+    if (BlockBeginFileId != MainFileID || RBraceFileId != MainFileID)
+      return std::nullopt;
+
+    StringRef RestOfLine = MainFileBuf.substr(RBraceOffset).split('\n').first;
+    if (!RestOfLine.starts_with("}"))
+      return std::nullopt;
+
+    StringRef TrimmedTrailingText = RestOfLine.drop_front().trim();
+    if (!TrimmedTrailingText.empty() &&
+        TrimmedTrailingText != OptionalPunctuation)
+      return std::nullopt;
+
+    auto BlockBeginLine = SM.getLineNumber(BlockBeginFileId, BlockBeginOffset);
+    auto RBraceLine = SM.getLineNumber(RBraceFileId, RBraceOffset);
+
+    // Don't show hint on trivial blocks like `class X {};`
+    if (BlockBeginLine + HintMinLineLimit - 1 > RBraceLine)
+      return std::nullopt;
+
+    // This is what we attach the hint to, usually "}" or "};".
+    StringRef HintRangeText = RestOfLine.take_front(
+        TrimmedTrailingText.empty()
+            ? 1
+            : TrimmedTrailingText.bytes_end() - RestOfLine.bytes_begin());
+
+    Position HintStart = sourceLocToPosition(SM, RBraceLoc);
+    Position HintEnd = sourceLocToPosition(
+        SM, RBraceLoc.getLocWithOffset(HintRangeText.size()));
+    return Range{HintStart, HintEnd};
+  }
+
   std::vector<InlayHint> &Results;
   ASTContext &AST;
   const syntax::TokenBuffer &Tokens;
   const Config &Cfg;
   std::optional<Range> RestrictRange;
   FileID MainFileID;
   StringRef MainFileBuf;
   const HeuristicResolver *Resolver;

clang-tools-extra/clangd/Protocol.h

enum class InlayHintKind {
 
   /// A hint before an element of an aggregate braced initializer list,
   /// indicating what it is initializing.
   ///   Pair{^1, ^2};
   /// Uses designator syntax, e.g. `.first:`.
   /// This is a clangd extension.
   Designator = 3,
 
+  /// A hint after function, type or namespace definition, indicating the
+  /// defined symbol name of the definition.
+  ///
+  /// An example of a decl name hint in this position:
+  ///    void func() {
+  ///    } ^
+  /// Uses comment-like syntax like "// func".
+  /// This is a clangd extension.
+  BlockEnd = 4,
+
   /// Other ideas for hints that are not currently implemented:
   ///
   /// * Chaining hints, showing the types of intermediate expressions
   ///   in a chain of function calls.
   /// * Hints indicating implicit conversions or implicit constructor calls.
 };
 llvm::json::Value toJSON(const InlayHintKind &);
 

clang-tools-extra/clangd/Protocol.cpp

 }
 
 llvm::json::Value toJSON(const InlayHintKind &Kind) {
   switch (Kind) {
   case InlayHintKind::Type:
     return 1;
   case InlayHintKind::Parameter:
     return 2;
-  case InlayHintKind::Designator: // This is an extension, don't serialize.
+  case InlayHintKind::Designator:
+  case InlayHintKind::BlockEnd:
+    // This is an extension, don't serialize.
     return nullptr;
   }
   llvm_unreachable("Unknown clang.clangd.InlayHintKind");
 }
 
 llvm::json::Value toJSON(const InlayHint &H) {
   llvm::json::Object Result{{"position", H.position},
                             {"label", H.label},
llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, InlayHintKind Kind) {
   auto ToString = [](InlayHintKind K) {
     switch (K) {
     case InlayHintKind::Parameter:
       return "parameter";
     case InlayHintKind::Type:
       return "type";
     case InlayHintKind::Designator:
       return "designator";
+    case InlayHintKind::BlockEnd:
+      return "block-end";
     }
     llvm_unreachable("Unknown clang.clangd.InlayHintKind");
   };
   return OS << ToString(Kind);
 }
 
 static const char *toString(OffsetEncoding OE) {
   switch (OE) {

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

 
 MATCHER_P(labelIs, Label, "") { return arg.label == Label; }
 
 Config noHintsConfig() {
   Config C;
   C.InlayHints.Parameters = false;
   C.InlayHints.DeducedTypes = false;
   C.InlayHints.Designators = false;
+  C.InlayHints.BlockEnd = false;
   return C;
 }
 
 template <typename... ExpectedHints>
 void assertHints(InlayHintKind Kind, llvm::StringRef AnnotatedSource,
                  ExpectedHints... Expected) {
   Annotations Source(AnnotatedSource);
   TestTU TU = TestTU::withCode(Source.code());

 void assertDesignatorHints(llvm::StringRef AnnotatedSource,
                            ExpectedHints... Expected) {
   Config Cfg;
   Cfg.InlayHints.Designators = true;
   WithContextValue WithCfg(Config::Key, std::move(Cfg));
   assertHints(InlayHintKind::Designator, AnnotatedSource, 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...);
+}
+
 TEST(ParameterHints, Smoke) {
   assertParameterHints(R"cpp(
     void foo(int param);
     void bar() {
       foo($param[[42]]);
     }
   )cpp",
                        ExpectedHint{"param: ", "param"});
assertParameterHints(
     }
     void foo() {
       bar(1, 2); // FIXME: We could have `bar(a: 1, a: 2)` here.
     }
   )cpp",
       ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
       ExpectedHint{"c: ", "param3"});
 }
+
+TEST(BlockEndHints, Functions) {
+  assertBlockEndHints(R"cpp(
+    int foo() {
+      return 41;
+    $foo[[}]]
+
+    template<int X> 
+    int bar() { 
+      // No hint for lambda for now
+      auto f = []() { 
+        return X; 
+      };
+      return f(); 
+    $bar[[}]]
+
+    // No hint because this isn't a definition
+    int buz();
+
+    struct S{};
+    bool operator==(S, S) {
+      return true;
+    $opEqual[[}]]
+  )cpp",
+                      ExpectedHint{" // foo", "foo"},
+                      ExpectedHint{" // bar", "bar"},
+                      ExpectedHint{" // operator==", "opEqual"});
+}
+
+TEST(BlockEndHints, Methods) {
+  assertBlockEndHints(R"cpp(
+    struct Test {
+      // No hint because there's no function body
+      Test() = default;
+      
+      ~Test() {
+      $dtor[[}]]
+
+      void method1() {
+      $method1[[}]]
+
+      // No hint because this isn't a definition
+      void method2();
+
+      template <typename T>
+      void method3() {
+      $method3[[}]]
+
+      // No hint because this isn't a definition
+      template <typename T>
+      void method4();
+
+      Test operator+(int) const {
+        return *this;
+      $opIdentity[[}]]
+
+      operator bool() const {
+        return true;
+      $opBool[[}]]
+
+      // No hint because there's no function body
+      operator int() const = delete;
+    } x;
+
+    void Test::method2() {
+    $method2[[}]]
+
+    template <typename T>
+    void Test::method4() {
+    $method4[[}]]
+  )cpp",
+                      ExpectedHint{" // ~Test", "dtor"},
+                      ExpectedHint{" // method1", "method1"},
+                      ExpectedHint{" // method3", "method3"},
+                      ExpectedHint{" // operator+", "opIdentity"},
+                      ExpectedHint{" // operator bool", "opBool"},
+                      ExpectedHint{" // Test::method2", "method2"},
+                      ExpectedHint{" // Test::method4", "method4"});
+}
+
+TEST(BlockEndHints, Namespaces) {
+  assertBlockEndHints(
+      R"cpp(
+    namespace {
+      void foo();
+    $anon[[}]]
+
+    namespace ns {
+      void bar();
+    $ns[[}]]
+  )cpp",
+      ExpectedHint{" // namespace", "anon"},
+      ExpectedHint{" // namespace ns", "ns"});
+}
+
+TEST(BlockEndHints, Types) {
+  assertBlockEndHints(
+      R"cpp(
+    struct S {
+    $S[[};]]
+
+    class C {
+    $C[[};]]
+
+    union U {
+    $U[[};]]
+
+    enum E1 {
+    $E1[[};]]
+
+    enum class E2 {
+    $E2[[};]]
+  )cpp",
+      ExpectedHint{" // struct S", "S"}, ExpectedHint{" // class C", "C"},
+      ExpectedHint{" // union U", "U"}, ExpectedHint{" // enum E1", "E1"},
+      ExpectedHint{" // enum class E2", "E2"});
+}
+
+TEST(BlockEndHints, TrailingSemicolon) {
+  assertBlockEndHints(R"cpp(
+    // The hint is placed after the trailing ';'
+    struct S1 {
+    $S1[[}  ;]]   
+
+    // The hint is always placed in the same line with the closing '}'.
+    // So in this case where ';' is missing, it is attached to '}'.
+    struct S2 {
+    $S2[[}]]
+
+    ;
+
+    // No hint because only one trailing ';' is allowed
+    struct S3 {
+    };;
+
+    // No hint because trailing ';' is only allowed for class/struct/union/enum
+    void foo() {
+    };
+
+    // Rare case, but yes we'll have a hint here.
+    struct {
+      int x;
+    $anon[[}]]
+    
+    s2;
+  )cpp",
+                      ExpectedHint{" // struct S1", "S1"},
+                      ExpectedHint{" // struct S2", "S2"},
+                      ExpectedHint{" // struct", "anon"});
+}
+
+TEST(BlockEndHints, TrailingText) {
+  assertBlockEndHints(R"cpp(
+    struct S1 {
+    $S1[[}      ;]]
+
+    // No hint for S2 because of the trailing comment
+    struct S2 {
+    }; /* Put anything here */
+
+    struct S3 {
+      // No hint for S4 because of the trailing source code
+      struct S4 {
+      };$S3[[};]]
+
+    // No hint for ns because of the trailing comment
+    namespace ns {
+    } // namespace ns
+  )cpp",
+                      ExpectedHint{" // struct S1", "S1"},
+                      ExpectedHint{" // struct S3", "S3"});
+}
+
+TEST(BlockEndHints, Macro) {
+  assertBlockEndHints(R"cpp(
+    #define DECL_STRUCT(NAME) struct NAME {
+    #define RBRACE }
+
+    DECL_STRUCT(S1)
+    $S1[[};]]
+
+    // No hint because we require a '}'
+    DECL_STRUCT(S2)
+    RBRACE;
+  )cpp",
+                      ExpectedHint{" // struct S1", "S1"});
+}
+
 // FIXME: Low-hanging fruit where we could omit a type hint:
 //  - auto x = TypeName(...);
 //  - auto x = (TypeName) (...);
 //  - auto x = static_cast<TypeName>(...);  // and other built-in casts
 
 // Annoyances for which a heuristic is not obvious:
 //  - auto x = llvm::dyn_cast<LongTypeName>(y);  // and similar
 //  - stdlib algos return unwieldy __normal_iterator<X*, ...> type
 //    (For this one, perhaps we should omit type hints that start
 //     with a double underscore.)
 
 } // namespace
 } // namespace clangd
 } // namespace clang