[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

> 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!



================
Comment at: clang-tools-extra/clangd/Config.h:150
     bool Designators = true;
+    bool BlockEnd = true;
     // Limit the length of type names in inlay hints. (0 means no limit)
----------------
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


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:802
+    Position HintStart = sourceLocToPosition(SM, RBraceLoc);
+    Position HintEnd = {HintStart.line,
+                        HintStart.character +
----------------
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.


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:578
+    constexpr unsigned HintMinLineLimit = 2;
+    constexpr unsigned HintMaxLengthLimit = 50;
+
----------------
daiyousei-qz wrote:
> nridge wrote:
> > daiyousei-qz wrote:
> > > sammccall wrote:
> > > > 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)
> > > `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)
> > > 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.
> 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.
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)


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:671
+
+  void addInlayHint(Range LSPRange, HintSide Side, InlayHintKind Kind,
+                    llvm::StringRef Prefix, llvm::StringRef Label,
----------------
sammccall wrote:
> daiyousei-qz wrote:
> > sammccall wrote:
> > > 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.
> > As per another comment below, this change is kept.
> Please revert this unless you can show a significant performance difference in a real-world setting.
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.


================
Comment at: clang-tools-extra/clangd/Protocol.cpp:1438
   case InlayHintKind::Designator: // This is an extension, don't serialize.
+  case InlayHintKind::BlockEnd:   // This is an extension, don't
+                                  // serialize.
----------------
nit: move the comment to the return statement rather than repeating it


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150635/new/

https://reviews.llvm.org/D150635