Eglot "inlay hints" landed

Eglot "inlay hints" landed

[João Távora]

I've just landed Eglot "inlay hints" on lisp/progmodes/eglot.el in the
emacs-29 branch.  It's a feature that some users (including me) were
looking for for some time.

Inlay hints are small text annotations to specific parts of the whole
buffer, not unlike diagnostics, but designed to help readability instead
of indicating problems.  For example, a C++ LSP server can serve hints
about positional parameter names in function calls and a variable's
automatically deduced type.  Emacs can display these hints in many
little 0-length overlays with an 'before-string property, thus helping
the user remember those types and parameter names.

Since inlay hints are potentially a large amount of data to request from
the LSP server, the implementation strives to be as parsimonious as
possible with these requests.

So, by default, inlay hints are only requested for the visible portions
of the buffer across windows showing this buffer.  This is done by
leveraging the 'window-scroll-functions' variable, making for a
reasonably complex implementation involving per-window timers.  When
scrolling a window, it may take a short amount of time for inlay hints
to "pop in".  The new user variable 'eglot-lazy-inlay-hints' can be used
to exert some control over this.

Specifically, if the variable's value is set to 'nil', then inlay hints
are greedily fetched for the whole buffer every time a change occurs.
This is a much simpler mode of operation which may avoid problems, but
is also likely much slower in large buffers.

Also, because the inlay feature is probably visually suprising to some,
it is turned OFF by default, which is not the usual practice of Eglot
(at least not when the necessary infrastructure is present).  This
decision may be changed soon.  Here's a good one-liner for enabling it
by default in every Eglot-managed buffer:

(add-hook 'eglot-managed-mode-hook #'eglot-inlay-hints-mode)

I haven't tested inlay hints extensively across many LSP servers, so I
would appreciate any testing, both for functional edge cases and
regarding performance.  There are possibly more optimization
oportunities in the "lazy" mode of operation, like more aggressively
deleting buffer overlays that are not in visible parts of the buffer.

Though I ended up writing this one from scratch, I want to thank Dimitry
Bolopopsky <dimitri@belopopsky.com> and Chinmay Dala
<dalal.chinmay.0101@gmail.com> for suggestions and early patches.

João

Re: [SPAM UNSURE] Eglot "inlay hints" landed

[Stephen Leake]

João Távora <joaotavora@gmail.com> writes:

> So, by default, inlay hints are only requested for the visible portions
> of the buffer across windows showing this buffer.  This is done by
> leveraging the 'window-scroll-functions' variable, making for a
> reasonably complex implementation involving per-window timers.  When
> scrolling a window, it may take a short amount of time for inlay hints
> to "pop in".  The new user variable 'eglot-lazy-inlay-hints' can be used
> to exert some control over this.

Since font-lock has the same requirement to refresh the visible portion
of the display, requesting inlay hints in a function registered with
jit-lock-register would be a simpler design.


-- 
-- Stephe

Re: Eglot "inlay hints" landed

[Chinmay Dalal]

Thanks, this works well.

> Since inlay hints are potentially a large amount of data to request from
> the LSP server, the implementation strives to be as parsimonious as
> possible with these requests.
>
> So, by default, inlay hints are only requested for the visible portions
> of the buffer across windows showing this buffer.  This is done by
> leveraging the 'window-scroll-functions' variable, making for a
> reasonably complex implementation involving per-window timers.  When
> scrolling a window, it may take a short amount of time for inlay hints
> to "pop in".

Can it be instead done in such a way that initially (when loading a new
file) they are requested for the whole buffer, then on subsequent
changes they are only requested for the visible regions.

This has some edge cases like pasting a huge block into a selected
region (here, text will be changed beyond what is currently visible so
it will take some time when someone scrolls to that region) but IMHO it
seems strictly better than the present.


Chinmay

Re: Eglot "inlay hints" landed

[Eli Zaretskii]

> From: Chinmay Dalal <dalal.chinmay.0101@gmail.com>
> Cc: emacs-devel@gnu.org, Dimitri Belopopsky <dimitri@belopopsky.com>, Po Lu
>  <luangruo@yahoo.com>, Eli  Zaretskii <eliz@gnu.org>
> Date: Thu, 23 Feb 2023 10:59:17 +0530
> 
> > So, by default, inlay hints are only requested for the visible portions
> > of the buffer across windows showing this buffer.  This is done by
> > leveraging the 'window-scroll-functions' variable, making for a
> > reasonably complex implementation involving per-window timers.  When
> > scrolling a window, it may take a short amount of time for inlay hints
> > to "pop in".
> 
> Can it be instead done in such a way that initially (when loading a new
> file) they are requested for the whole buffer, then on subsequent
> changes they are only requested for the visible regions.

Why would we want that?

Re: Eglot "inlay hints" landed

[Chinmay Dalal]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Chinmay Dalal <dalal.chinmay.0101@gmail.com>
>> Cc: emacs-devel@gnu.org, Dimitri Belopopsky <dimitri@belopopsky.com>, Po Lu
>>  <luangruo@yahoo.com>, Eli  Zaretskii <eliz@gnu.org>
>> Date: Thu, 23 Feb 2023 10:59:17 +0530
>> 
>> > So, by default, inlay hints are only requested for the visible portions
>> > of the buffer across windows showing this buffer.  This is done by
>> > leveraging the 'window-scroll-functions' variable, making for a
>> > reasonably complex implementation involving per-window timers.  When
>> > scrolling a window, it may take a short amount of time for inlay hints
>> > to "pop in".
>> 
>> Can it be instead done in such a way that initially (when loading a new
>> file) they are requested for the whole buffer, then on subsequent
>> changes they are only requested for the visible regions.
>
> Why would we want that?

It will solve this problem:

>> > When
>> > scrolling a window, it may take a short amount of time for inlay hints
>> > to "pop in".

Re: Eglot "inlay hints" landed

[João Távora]

On Thu, Feb 23, 2023 at 9:56 AM Chinmay Dalal
<dalal.chinmay.0101@gmail.com> wrote:

> > Why would we want that?
>
>
> >> > When
> >> > scrolling a window, it may take a short amount of time for inlay hints
> >> > to "pop in".

> It will solve this problem:

Not significantly though. There are many operations that
invalidate the whole buffer's hints, like changing the name
of a function parameter, or doing some git operation
which changes the contents of the whole buffer.  That will
still cause pop-in.  So I don't think it's worth the added
complexity: if you want no scrolling pop-in, set
eglot-lazy-inlay-hints to nil.  Maybe it's not bad for small
buffers and fast servers.

João

Re: Eglot "inlay hints" landed

[Dimitri Belopopsky]

[-- Attachment #1: Type: text/plain, Size: 1492 bytes --]

Inlay hints are working well on my end!
The only issue I seem to be having is when the server doesn't support inlay
hints, and I have " (add-hook 'eglot-managed-mode-hook
#'eglot-inlay-hints-mode)" in my config.

It keeps erroring in this case and the connection to the lsp server seems
to terminate in some cases. In any case it causes interference with other
things, like completion which can stop working.

You can try it with pylsp, and add "(add-hook 'eglot-managed-mode-hook
#'eglot-inlay-hints-mode)" to your config. Trying to write a simple main
function will show issues.

For example this python code:

def main():
    pass



On Thu, 23 Feb 2023 at 11:04, João Távora <joaotavora@gmail.com> wrote:

> On Thu, Feb 23, 2023 at 9:56 AM Chinmay Dalal
> <dalal.chinmay.0101@gmail.com> wrote:
>
> > > Why would we want that?
> >
> >
> > >> > When
> > >> > scrolling a window, it may take a short amount of time for inlay
> hints
> > >> > to "pop in".
>
> > It will solve this problem:
>
> Not significantly though. There are many operations that
> invalidate the whole buffer's hints, like changing the name
> of a function parameter, or doing some git operation
> which changes the contents of the whole buffer.  That will
> still cause pop-in.  So I don't think it's worth the added
> complexity: if you want no scrolling pop-in, set
> eglot-lazy-inlay-hints to nil.  Maybe it's not bad for small
> buffers and fast servers.
>
> João
>

[-- Attachment #2: Type: text/html, Size: 2145 bytes --]

Re: Eglot "inlay hints" landed

[João Távora]

On Thu, Feb 23, 2023 at 10:55 AM Dimitri Belopopsky
<dimitri@belopopsky.com> wrote:
>
> Inlay hints are working well on my end!
> The only issue I seem to be having is when the server doesn't support inlay hints, and I have " (add-hook 'eglot-managed-mode-hook #'eglot-inlay-hints-mode)" in my config.

That's odd.  I distinctly remembered thinking about this
and making this issue a warning, not an error. Maybe it didn't make
it in the commit I pushed somehow.  I'll check.

João

Re: Eglot "inlay hints" landed

[João Távora]

On Thu, Feb 23, 2023 at 10:55 AM Dimitri Belopopsky
<dimitri@belopopsky.com> wrote:
>
> Inlay hints are working well on my end!
> The only issue I seem to be having is when the server doesn't support inlay hints, and I have " (add-hook 'eglot-managed-mode-hook #'eglot-inlay-hints-mode)" in my config.
>
> It keeps erroring in this case and the connection to the lsp server seems to terminate in some cases. In any case it causes interference with other things, like completion which can stop working.
>
> You can try it with pylsp, and add "(add-hook 'eglot-managed-mode-hook #'eglot-inlay-hints-mode)" to your config. Trying to write a simple main function will show issues.

I can't reproduce this with this server.  All I see is a warning:

Warning (eglot): No :inlayHintProvider support. Inlay hints will not work.

which is how it's supposed to work.

João

Re: Eglot "inlay hints" landed

[Dimitri Belopopsky]

[-- Attachment #1: Type: text/plain, Size: 1167 bytes --]

On Thu, 23 Feb 2023 at 13:05, João Távora <joaotavora@gmail.com> wrote:

> On Thu, Feb 23, 2023 at 10:55 AM Dimitri Belopopsky
> <dimitri@belopopsky.com> wrote:
> >
> > Inlay hints are working well on my end!
> > The only issue I seem to be having is when the server doesn't support
> inlay hints, and I have " (add-hook 'eglot-managed-mode-hook
> #'eglot-inlay-hints-mode)" in my config.
> >
> > It keeps erroring in this case and the connection to the lsp server
> seems to terminate in some cases. In any case it causes interference with
> other things, like completion which can stop working.
> >
> > You can try it with pylsp, and add "(add-hook 'eglot-managed-mode-hook
> #'eglot-inlay-hints-mode)" to your config. Trying to write a simple main
> function will show issues.
>
> I can't reproduce this with this server.  All I see is a warning:
>
> Warning (eglot): No :inlayHintProvider support. Inlay hints will not work.
>
> which is how it's supposed to work.
>
> João
>

My bad... I'm using straight.el, and somehow it wasn't picking things
correctly. Forcing it to use the built-in variant makes everything work
correctly.

[-- Attachment #2: Type: text/html, Size: 1656 bytes --]

Re: Eglot "inlay hints" landed

[Eli Zaretskii]

> From: Chinmay Dalal <dalal.chinmay.0101@gmail.com>
> Cc: joaotavora@gmail.com, emacs-devel@gnu.org, dimitri@belopopsky.com,
>  luangruo@yahoo.com
> Date: Thu, 23 Feb 2023 15:25:21 +0530
> 
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Can it be instead done in such a way that initially (when loading a new
> >> file) they are requested for the whole buffer, then on subsequent
> >> changes they are only requested for the visible regions.
> >
> > Why would we want that?
> 
> It will solve this problem:
> 
> >> > When
> >> > scrolling a window, it may take a short amount of time for inlay hints
> >> > to "pop in".

So would using jit-lock-register, which was proposed here.

Re: Eglot "inlay hints" landed

[João Távora]

On Thu, Feb 23, 2023 at 11:05 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Chinmay Dalal <dalal.chinmay.0101@gmail.com>
> > Cc: joaotavora@gmail.com, emacs-devel@gnu.org, dimitri@belopopsky.com,
> >  luangruo@yahoo.com
> > Date: Thu, 23 Feb 2023 15:25:21 +0530
> >
> >
> > Eli Zaretskii <eliz@gnu.org> writes:
> >
> > >> Can it be instead done in such a way that initially (when loading a new
> > >> file) they are requested for the whole buffer, then on subsequent
> > >> changes they are only requested for the visible regions.
> > >
> > > Why would we want that?
> >
> > It will solve this problem:
> >
> > >> > When
> > >> > scrolling a window, it may take a short amount of time for inlay hints
> > >> > to "pop in".
>
> So would using jit-lock-register, which was proposed here.

The "pop-in" delay is just a function of the intentional
bandwidth-conserving timer delay + the normal LSP interprocess
communication delay. Any jit/lazy Emacs-side solution
is going to have to deal at least with the second addend of
that sum.

jit-lock-register was unknown to me.  It seems to rely on some
heuristic to know what regions need to be "refontified".  I wonder
if the heuristic will be accurate for inlay hints, since changing
void foo(int bar){...}  to void foo(int baz){...} in one part of
the buffer doesn't usually change the fontification of the rest
of the buffer.  But it might very well invalidate the inlay hints
everywhere.

In fact the invalidation impact is not just in the same buffer,
but potentially all other buffers (all the ones where a call to
'foo' is found).  Eglot's inlay hints implementation doesn't
handle this edge case.  Though I don't think it would be
extremely hard to, it doesn't seem extremely relevant for what
is usually a "best effort" helper feature from the LSP side.

João

Re: Eglot "inlay hints" landed

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Thu, 23 Feb 2023 11:23:40 +0000
> Cc: Chinmay Dalal <dalal.chinmay.0101@gmail.com>, emacs-devel@gnu.org, 
> 	dimitri@belopopsky.com, luangruo@yahoo.com
> 
> On Thu, Feb 23, 2023 at 11:05 AM Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > > Eli Zaretskii <eliz@gnu.org> writes:
> > >
> > > >> Can it be instead done in such a way that initially (when loading a new
> > > >> file) they are requested for the whole buffer, then on subsequent
> > > >> changes they are only requested for the visible regions.
> > > >
> > > > Why would we want that?
> > >
> > > It will solve this problem:
> > >
> > > >> > When
> > > >> > scrolling a window, it may take a short amount of time for inlay hints
> > > >> > to "pop in".
> >
> > So would using jit-lock-register, which was proposed here.
> 
> The "pop-in" delay is just a function of the intentional
> bandwidth-conserving timer delay + the normal LSP interprocess
> communication delay. Any jit/lazy Emacs-side solution
> is going to have to deal at least with the second addend of
> that sum.

I don't understand: using jit-lock-register just means your code is
called via jit-lock's fontification-functions instead of
window-scroll-functions that you used.  Any problems with LSP delays
that you deal with in the latter should be possible in the former as
well, no?  Or what am I missing?

The advantages of using jit-lock are that (a) it is more accurate in
telling you which parts of the buffer are about to be displayed, and
(b) it is much more reliable, because window-scroll-functions are not
necessarily called when something changes on display.  For example, we
lately discovered that pixel-scroll-precision-mode doesn't call
window-scroll-functions.

> jit-lock-register was unknown to me.  It seems to rely on some
> heuristic to know what regions need to be "refontified".

It isn't a heuristic.  jit-lock is called from the display engine,
which always has a pretty good idea which parts of the buffer it needs
to show on the screen.

> I wonder if the heuristic will be accurate for inlay hints, since
> changing void foo(int bar){...}  to void foo(int baz){...} in one
> part of the buffer doesn't usually change the fontification of the
> rest of the buffer.

The display engine doesn't know which parts will be affected by the
change, it only knowes what's on display and what isn't.  The function
called via fontification-functions are supposed to know their job, and
look at the parts of the buffer according to their needs; jit-lock
just gives them a hint in the form of the region of the buffer it
wants to display.

> In fact the invalidation impact is not just in the same buffer,
> but potentially all other buffers (all the ones where a call to
> 'foo' is found).  Eglot's inlay hints implementation doesn't
> handle this edge case.  Though I don't think it would be
> extremely hard to, it doesn't seem extremely relevant for what
> is usually a "best effort" helper feature from the LSP side.

We don't need to make the overlays until the buffer is shown in some
window, right?

Re: Eglot "inlay hints" landed

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

> The advantages of using jit-lock are that (a) it is more accurate in
> telling you which parts of the buffer are about to be displayed, and
> (b)

Ah, if it works with "parts of the buffer about to be displayed", then it
should be good, yes.  But the docstring of jit-lock-register says "START
and END indicating the region that needs to be (re)fontified".  If that
is guaranteed to always match the "region of the buffer about to be
displayed" in the window, then we're good.

I wonder if it also removes the need for the "smoothing" timers I am
using.

But note though, that while this has promise for a simpler and more
robust implementation, it will _not_ solve the "pop-in" delay.  The
effects that Eglot's jit-lock-register FUN fontification function
produces in the buffer are guaranteed _not_ be finished by the time FUN
returns (unless these FUN is allowed to be slow and blocking, which I
really don't think is the point).  That's what I meant by "any
Emacs-side solution [...] deal with [the normal LSP interprocess
communication] delay".

> it is much more reliable, because window-scroll-functions are not
> necessarily called when something changes on display.  For example, we
> lately discovered that pixel-scroll-precision-mode doesn't call
> window-scroll-functions.

>> In fact the invalidation impact is not just in the same buffer,
>> but potentially all other buffers (all the ones where a call to
>> 'foo' is found).  Eglot's inlay hints implementation doesn't
>> handle this edge case.  Though I don't think it would be
>> extremely hard to, it doesn't seem extremely relevant for what
>> is usually a "best effort" helper feature from the LSP side.
>
> We don't need to make the overlays until the buffer is shown in some
> window, right?

Yes, but two buffers A and B might already be showing in some window.
If you do the change in buffer A and it affects B, then in the current
version, the parts of A being show in windows will be updated, but B the
parts of B being shown in some other windows will not.

João

PS: On an unrelated note, I pushed this to emacs-29.  If you wish me to
revert the inlay hints implementation and do all this work in master,
it's fine by me.  In practice, it won't really make that much of a
difference because Eglot (along with being a new emacs-29 feature) is
also a GNU ELPA :core package and emacs-29 users will have access to the
latest and greatest features and bugfixes relatively easily anyway.

Re: Eglot "inlay hints" landed

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: dalal.chinmay.0101@gmail.com,  emacs-devel@gnu.org,
>   dimitri@belopopsky.com,  luangruo@yahoo.com
> Date: Thu, 23 Feb 2023 12:57:26 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > We don't need to make the overlays until the buffer is shown in some
> > window, right?
> 
> Yes, but two buffers A and B might already be showing in some window.
> If you do the change in buffer A and it affects B, then in the current
> version, the parts of A being show in windows will be updated, but B the
> parts of B being shown in some other windows will not.

If there's a change in A that affects B, jit-lock will call
fontification-functions in both A and B, each one when it's about to
display the corresponding window.

> PS: On an unrelated note, I pushed this to emacs-29.  If you wish me to
> revert the inlay hints implementation and do all this work in master,
> it's fine by me.

emacs-29 is fine, thanks.

Re: Eglot "inlay hints" landed

[João Távora]

On Thu, Feb 23, 2023 at 2:49 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: João Távora <joaotavora@gmail.com>
> > Cc: dalal.chinmay.0101@gmail.com,  emacs-devel@gnu.org,
> >   dimitri@belopopsky.com,  luangruo@yahoo.com
> > Date: Thu, 23 Feb 2023 12:57:26 +0000
> >
> > Eli Zaretskii <eliz@gnu.org> writes:
> >
> > > We don't need to make the overlays until the buffer is shown in some
> > > window, right?
> >
> > Yes, but two buffers A and B might already be showing in some window.
> > If you do the change in buffer A and it affects B, then in the current
> > version, the parts of A being show in windows will be updated, but B the
> > parts of B being shown in some other windows will not.
>
> If there's a change in A that affects B, jit-lock will call
> fontification-functions in both A and B, each one when it's about to
> display the corresponding window.

Sure, but you're in charge of coding up the "affection" by asking
the LSP server.  In other words, only the LSP server knows that the
change in A affects B. You must assume that it does and ask it "Hey LSP
server, given that I've just changed document A, in your document B from
42 to 420 is are there any new or different inlay hints you'd like to
give me?" jit-lock cannot foresee that upfront, it will only act on B's
display if B's buffer is changed.

Regardless of that separate issue, I started experimenting with
jit-lock-register.  It's promising, but has problems. Here's a
small experiment.  Use this foo.cpp file somewhere and have say,
the clangd server handy.

void foo(int bar){}

int main() {
  foo(42);
  .... repeats about ~250 times ....
  foo(42);
}

If you start eglot and eglot-inlay-hints-mode in this buffer,
you should see

void foo(int bar){}

int main() {
  foo(bar: 42);  // 'bar: ' is the untangible hint overlay
  foo(bar: 42);
  foo(bar: 42);
  ...
  < end of window >

I also traced the current eglot--update-hints-1 file which is a
binary function of two buffer positions, just like the jit
function.

This is the function that contacts the LSP server via JSONRPC
and some time in the future, after it returns, the 0-length
overlays will be created.

Here's how it is called after enabling eglot and scrolling 4
screenfuls forward (C-v) and 4 back (M-v).

======================================================================
1 -> (eglot--update-hints-1 1 476)
1 <- eglot--update-hints-1: nil
======================================================================
1 -> (eglot--update-hints-1 443 927)
1 <- eglot--update-hints-1: nil
======================================================================
1 -> (eglot--update-hints-1 894 1378)
1 <- eglot--update-hints-1: nil
======================================================================
1 -> (eglot--update-hints-1 1345 1829)
1 <- eglot--update-hints-1: nil
======================================================================
1 -> (eglot--update-hints-1 1796 2280)
1 <- eglot--update-hints-1: nil
======================================================================
1 -> (eglot--update-hints-1 1345 1829)
1 <- eglot--update-hints-1: nil
======================================================================
1 -> (eglot--update-hints-1 894 1378)
1 <- eglot--update-hints-1: nil
======================================================================
1 -> (eglot--update-hints-1 443 927)
1 <- eglot--update-hints-1: nil
======================================================================
1 -> (eglot--update-hints-1 1 476)
1 <- eglot--update-hints-1: nil

As you can see, it only requests inlay hints for the regions actually
displayed in the window.  It re-requests some stuff when going
back, since it has no memory of what it already requested or if
things were invalidated.

Now if I put eglot--update-hints-1 in jit-lock-function and
disable my window-scroll-functions, then do the same:

======================================================================
1 -> (eglot--update-hints-1 1 1501)
1 <- eglot--update-hints-1: nil
======================================================================
1 -> (eglot--update-hints-1 1501 2678)
1 <- eglot--update-hints-1: nil

As you can see, it did much larger, heavier requests upfront, even before
it knew I was going to scroll forward.  But in general it worked and you
can argue that doing only two requests for larger chunks of inlay hints
is better than more requests for smaller chunks.

Now, if I change the line void foo(int bar){} to void foo(int baz){}
the point is to get updated hints in the window:

int main() {
  foo(baz: 42);  // 'bar: ' is the untangible hint overlay
  foo(baz: 42);
  foo(baz: 42);
  ...
  < end of window >

The jit-lock implementation will do this:

======================================================================
1 -> (eglot--update-hints-1 1 19)
1 <- eglot--update-hints-1: nil
======================================================================
1 -> (eglot--update-hints-1 1 20)
1 <- eglot--update-hints-1: nil
======================================================================
1 -> (eglot--update-hints-1 20 1520)
1 <- eglot--update-hints-1: nil

The first two calls are useless and so is most of the third one.
For the third one you can argue, as above, that it's a good
thing to predict that the user is going to scroll down.  But
the first two, which also caused two LSP requests, are definitely
useless.

OT1H, I think I can make this work, by finding some means to
coalesce those three calls into one.  Suggestions welcome.
Even after that complexity, it seems it will be a much neater
implementation.

OTOH it sort of negates my original intention of requesting
only as many inlay hints as strictly necessary.  THough that
might have been premature optimization anyway.

> > PS: On an unrelated note, I pushed this to emacs-29.  If you wish me to
> > revert the inlay hints implementation and do all this work in master,
> > it's fine by me.
>
> emacs-29 is fine, thanks.

OK!

João

Re: Eglot "inlay hints" landed

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Thu, 23 Feb 2023 16:09:24 +0000
> Cc: dalal.chinmay.0101@gmail.com, emacs-devel@gnu.org, dimitri@belopopsky.com, 
> 	luangruo@yahoo.com
> 
> > If there's a change in A that affects B, jit-lock will call
> > fontification-functions in both A and B, each one when it's about to
> > display the corresponding window.
> 
> Sure, but you're in charge of coding up the "affection" by asking
> the LSP server.  In other words, only the LSP server knows that the
> change in A affects B. You must assume that it does and ask it "Hey LSP
> server, given that I've just changed document A, in your document B from
> 42 to 420 is are there any new or different inlay hints you'd like to
> give me?" jit-lock cannot foresee that upfront, it will only act on B's
> display if B's buffer is changed.

That's no for jit-lock to do.  And I don't see how it could be
relevant to the issue we are discussing.  How do you do this now?

> Regardless of that separate issue, I started experimenting with
> jit-lock-register.  It's promising, but has problems.

That might be so, but one problem it does NOT have is missing the
cases when you MUST ask the LSP server, because something is going to
change on display.  window-scroll-functions cannot promise that, since
they are only called "when the window is scrolled", and there's more
to that condition than meets the eye, believe me.

Re: Eglot "inlay hints" landed

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 2764 bytes --]

On Thu, Feb 23, 2023 at 5:17 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: João Távora <joaotavora@gmail.com>
> > Date: Thu, 23 Feb 2023 16:09:24 +0000
> > Cc: dalal.chinmay.0101@gmail.com, emacs-devel@gnu.org, dimitri@belopopsky.com,
> >       luangruo@yahoo.com
> >
> > > If there's a change in A that affects B, jit-lock will call
> > > fontification-functions in both A and B, each one when it's about to
> > > display the corresponding window.
> >
> > Sure, but you're in charge of coding up the "affection" by asking
> > the LSP server.  In other words, only the LSP server knows that the
> > change in A affects B. You must assume that it does and ask it "Hey LSP
> > server, given that I've just changed document A, in your document B from
> > 42 to 420 is are there any new or different inlay hints you'd like to
> > give me?" jit-lock cannot foresee that upfront, it will only act on B's
> > display if B's buffer is changed.
>
> That's no for jit-lock to do.  And I don't see how it could be
> relevant to the issue we are discussing.  How do you do this now?

I don't.  I was just pointing out that jit-lock by itself doesn't
solve this A -> B dependency, which you seemed to suggest it does when
you wrote:

> > If there's a change in A that affects B, jit-lock will call
> > fontification-functions in both A and B, each one when it's about to
> > display the corresponding window.

So I agree with you shouldn't be continuing the discussion of this
topic: it is for later.

> That might be so, but one problem it does NOT have is missing the
> cases when you MUST ask the LSP server, because something is going to
> change on display.

That's true.  But then so does the other more naive implementation
which you get when you set eglot-lazy-inlay-hints to nil, and I'm
not sure which one is more performance.

> window-scroll-functions cannot promise that, since
> they are only called "when the window is scrolled", and there's more
> to that condition than meets the eye, believe me.

I believe you.  But as far as I can tell so far, it's the least
imperfect of the methods, and I haven't seen demonstrations of
problems so far, only your speculation of hypothetical problems.
Which again, I believe in, but I would like to measure the actual
problems quantitatively and qualitatively to make a good decision.

I'd love to switch over to the jit-lock implementation as it has
potential to be much neater.  But I can't seem to get it to not
over-request stuff. I attach the patch I've been trying, and it's
clearly got some thinkos when you test it.  It doesn't help that
`window-start` and `window-end` aren't -- apparently -- reliable
when called from a jit-lock function.

João

[-- Attachment #2: better-inlay-hints-maybe.patch --]
[-- Type: application/octet-stream, Size: 4989 bytes --]

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 7b4251a1242..76b56fa14e0 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -3507,14 +3507,27 @@ eglot-lazy-inlay-hints
 (defun eglot--inlay-hints-fully ()
   (eglot--widening (eglot--update-hints-1 (point-min) (point-max))))
 
-(cl-defun eglot--inlay-hints-lazily (&optional (buffer (current-buffer)))
-  (eglot--when-live-buffer buffer
-    (when eglot--managed-mode
-      (dolist (window (get-buffer-window-list nil nil 'visible))
-        (eglot--update-hints-1 (window-start window) (window-end window))))))
+(defvar-local eglot--inlay-hints-outstanding (cons nil nil)
+  "Largest (BEG . END) window subregion not yet updated for inlay hints.")
+
+(defun eglot--update-hints (from to)
+  "Jit-lock function for Eglot inlay hints."
+  (cl-symbol-macrolet ((x eglot--inlay-hints-outstanding))
+    (setq x (cons (min from (or (car x) most-positive-fixnum))
+                  (max to (or (cdr x) 0))))
+    (let ((w (get-buffer-window)))
+      (trace-values "window:      " w)
+      (trace-values "window-region:      " (cons (window-start w) (window-end w)))
+      (trace-values "outstanding: " eglot--inlay-hints-outstanding)
+      ;  FIXME: this is NOT the correct condition to decide when to
+      ;  request stuff from the server.
+      (when (or (< (car x) (window-start w)) (> (cdr x) (window-end w)))
+        (unwind-protect
+            (eglot--update-hints-1 (car x) (cdr x))
+          (setq x (cons nil nil)))))))
 
 (defun eglot--update-hints-1 (from to)
-  "Request LSP inlay hints and annotate current buffer from FROM to TO."
+  "Do actual work for `eglot--update-hints', including LSP request."
   (let* ((buf (current-buffer))
          (paint-hint
           (eglot--lambda ((InlayHint) position kind label paddingLeft paddingRight)
@@ -3545,33 +3558,6 @@ eglot--update-hints-1
                       (mapc paint-hint hints))))
      :deferred 'eglot--update-hints-1)))
 
-(defun eglot--inlay-hints-after-scroll (window display-start)
-  (cl-macrolet ((wsetq (sym val) `(set-window-parameter window ',sym ,val))
-                (wgetq (sym) `(window-parameter window ',sym)))
-    (let ((buf (window-buffer window))
-          (timer (wgetq eglot--inlay-hints-timer))
-          (last-display-start (wgetq eglot--last-inlay-hint-display-start)))
-      (when (and eglot-lazy-inlay-hints
-                 ;; FIXME: If `window' is _not_ the selected window,
-                 ;; then for some unknown reason probably related to
-                 ;; the overlays added later to the buffer, the scroll
-                 ;; function will be called indefinitely.  Not sure if
-                 ;; an Emacs bug, but prevent useless duplicate calls
-                 ;; by saving and examining `display-start' fixes it.
-                 (not (eql last-display-start display-start)))
-        (when timer (cancel-timer timer))
-        (wsetq eglot--last-inlay-hint-display-start
-               display-start)
-        (wsetq eglot--inlay-hints-timer
-               (run-at-time
-                eglot-lazy-inlay-hints
-                nil (lambda ()
-                      (eglot--when-live-buffer buf
-                        (when (eq buf (window-buffer window))
-                          (eglot--update-hints-1 (window-start window)
-                                                 (window-end window))
-                          (wsetq eglot--inlay-hints-timer nil))))))))))
-
 (define-minor-mode eglot-inlay-hints-mode
   "Minor mode for annotating buffers with LSP server's inlay hints."
   :global nil
@@ -3581,24 +3567,15 @@ eglot-inlay-hints-mode
            (eglot--warn
             "No :inlayHintProvider support. Inlay hints will not work."))
           (eglot-lazy-inlay-hints
-           (add-hook 'eglot--document-changed-hook
-                     #'eglot--inlay-hints-lazily t t)
-           (add-hook 'window-scroll-functions
-                     #'eglot--inlay-hints-after-scroll nil t)
-           ;; Maybe there isn't a window yet for current buffer,
-           ;; so `run-at-time' ensures this runs after redisplay.
-           (run-at-time 0 nil #'eglot--inlay-hints-lazily))
+           (jit-lock-register #'eglot--update-hints))
           (t
            (add-hook 'eglot--document-changed-hook
                      #'eglot--inlay-hints-fully nil t)
            (eglot--inlay-hints-fully))))
         (t
-         (remove-hook 'eglot--document-changed-hook
-                      #'eglot--inlay-hints-lazily t)
+         (jit-lock-unregister #'eglot--update-hints)
          (remove-hook 'eglot--document-changed-hook
                       #'eglot--inlay-hints-fully t)
-         (remove-hook 'window-scroll-functions
-                      #'eglot--inlay-hints-after-scroll t)
          (remove-overlays nil nil 'eglot--inlay-hint t))))
 
 \f

Re: Eglot "inlay hints" landed

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Thu, 23 Feb 2023 17:46:08 +0000
> Cc: dalal.chinmay.0101@gmail.com, emacs-devel@gnu.org, dimitri@belopopsky.com, 
> 	luangruo@yahoo.com
> 
> > That's no for jit-lock to do.  And I don't see how it could be
> > relevant to the issue we are discussing.  How do you do this now?
> 
> I don't.  I was just pointing out that jit-lock by itself doesn't
> solve this A -> B dependency, which you seemed to suggest it does when
> you wrote:
> 
> > > If there's a change in A that affects B, jit-lock will call
> > > fontification-functions in both A and B, each one when it's about to
> > > display the corresponding window.

That was written under the assumption that the overlays in B are
already updated.  Then redisplay will know it must redraw B.

> > window-scroll-functions cannot promise that, since
> > they are only called "when the window is scrolled", and there's more
> > to that condition than meets the eye, believe me.
> 
> I believe you.  But as far as I can tell so far, it's the least
> imperfect of the methods, and I haven't seen demonstrations of
> problems so far, only your speculation of hypothetical problems.

I actually gave you a recipe for demonstrating the problems I have in
mind: scroll the window under pixel-scroll-precision-mode.  AFAIK, we
don't call window-scroll-functions in that case.

Another situation where we don't call window-scroll-functions is when
the user types into the buffer.

Yet another situation is when you type "C-x 1" to delete all the other
windows on the frame, leaving the current window that now shows more
stuff than before.

> I'd love to switch over to the jit-lock implementation as it has
> potential to be much neater.  But I can't seem to get it to not
> over-request stuff. I attach the patch I've been trying, and it's
> clearly got some thinkos when you test it.  It doesn't help that
> `window-start` and `window-end` aren't -- apparently -- reliable
> when called from a jit-lock function.

Sorry, I don't have time for that ATM, but maybe Stefan will want to
comment.

Re: Eglot "inlay hints" landed

[João Távora]

On Thu, Feb 23, 2023 at 6:01 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: João Távora <joaotavora@gmail.com>
> > Date: Thu, 23 Feb 2023 17:46:08 +0000
> > Cc: dalal.chinmay.0101@gmail.com, emacs-devel@gnu.org, dimitri@belopopsky.com,
> >       luangruo@yahoo.com
> >
> > > That's no for jit-lock to do.  And I don't see how it could be
> > > relevant to the issue we are discussing.  How do you do this now?
> >
> > I don't.  I was just pointing out that jit-lock by itself doesn't
> > solve this A -> B dependency, which you seemed to suggest it does when
> > you wrote:
> >
> > > > If there's a change in A that affects B, jit-lock will call
> > > > fontification-functions in both A and B, each one when it's about to
> > > > display the corresponding window.
>
> That was written under the assumption that the overlays in B are
> already updated.  Then redisplay will know it must redraw B.
>
> > > window-scroll-functions cannot promise that, since
> > > they are only called "when the window is scrolled", and there's more
> > > to that condition than meets the eye, believe me.
> >
> > I believe you.  But as far as I can tell so far, it's the least
> > imperfect of the methods, and I haven't seen demonstrations of
> > problems so far, only your speculation of hypothetical problems.
>
> I actually gave you a recipe for demonstrating the problems I have in
> mind: scroll the window under pixel-scroll-precision-mode.  AFAIK, we
> don't call window-scroll-functions in that case.

I don't have a pixel-scroll-enabled Emacs to test.  Can you scroll
large portions of the window like that and w-s-functions will never
get called?  I'd say that's a bug we should fix.

> Another situation where we don't call window-scroll-functions is when
> the user types into the buffer.

That is handled by Eglot's after-change-functions already so it
isn't a problem.

> Yet another situation is when you type "C-x 1" to delete all the other
> windows on the frame, leaving the current window that now shows more
> stuff than before.

Reproduced.  Easy enough to fix with window-configuration-change-functions.
I pushed a fix.

> > I'd love to switch over to the jit-lock implementation as it has
> > potential to be much neater.  But I can't seem to get it to not
> > over-request stuff. I attach the patch I've been trying, and it's
> > clearly got some thinkos when you test it.  It doesn't help that
> > `window-start` and `window-end` aren't -- apparently -- reliable
> > when called from a jit-lock function.
>
> Sorry, I don't have time for that ATM, but maybe Stefan will want to
> comment.

I'll keep trying. I think what I'd want is for the "contextual" call
with the big chunk about to be jit-refontified remember the smaller
adjacent chunks and make a request for the big chunk + the smaller
chunks.  Here's the latest version of the patch using a timer to
achieve that.  It's pretty gross, but I hope it expresses the idea.

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index eea8be6d1aa..251f6f11090 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -3507,14 +3507,47 @@ eglot-lazy-inlay-hints
 (defun eglot--inlay-hints-fully ()
   (eglot--widening (eglot--update-hints-1 (point-min) (point-max))))

-(cl-defun eglot--inlay-hints-lazily (&optional (buffer (current-buffer)))
-  (eglot--when-live-buffer buffer
-    (when eglot--managed-mode
-      (dolist (window (get-buffer-window-list nil nil 'visible))
-        (eglot--update-hints-1 (window-start window) (window-end window))))))
+(defvar-local eglot--inlay-hints-outstanding nil
+  "List of regions")
+
+(defvar-local eglot--inlay-hints-outstanding-timer nil
+  "Timer")
+
+(defun eglot--update-hints (from to)
+  "Jit-lock function for Eglot inlay hints."
+  (if eglot--inlay-hints-outstanding-timer
+      (cancel-timer eglot--inlay-hints-outstanding-timer))
+  (push (cons from to) eglot--inlay-hints-outstanding)
+  (setq eglot--inlay-hints-outstanding-timer
+        (run-with-timer
+         ;; FIXME: Gross
+         (+ 0.1 (max eglot-lazy-inlay-hints jit-lock-context-time))
+         nil (lambda ()
+               (trace-values eglot--inlay-hints-outstanding)
+               (cl-loop with region< = (lambda (r1 r2)
+                                         (if (= (car r1) (car r2))
+                                             (< (cdr r1) (cdr r2))
+                                           (< (car r1) (car r2))))
+                        with sorted = (cl-sort
eglot--inlay-hints-outstanding region<)
+                        with coalesced = (cl-reduce (lambda (acc x)
+                                                      (cond ((and acc
(>= (cdr (car acc))
+
    (car x)))
+                                                             (setcdr
(car acc) (cdr x))
+                                                             acc)
+                                                            (t
+                                                             (push x acc)
+                                                             acc)))
+                                                    sorted
+                                                    :initial-value nil)
+                        for r in coalesced
+                        do (eglot--update-hints-1 (max (car r) (point-min))
+                                                  (min (cdr r) (point-max)))
+                        finally
+                        (setq eglot--inlay-hints-outstanding nil
+                              eglot--inlay-hints-outstanding-timer nil))))))

 (defun eglot--update-hints-1 (from to)
-  "Request LSP inlay hints and annotate current buffer from FROM to TO."
+  "Do actual work for `eglot--update-hints', including LSP request."
   (let* ((buf (current-buffer))
          (paint-hint
           (eglot--lambda ((InlayHint) position kind label paddingLeft
paddingRight)
@@ -3545,36 +3578,6 @@ eglot--update-hints-1
                       (mapc paint-hint hints))))
      :deferred 'eglot--update-hints-1)))

-(defun eglot--inlay-hints-after-scroll (window display-start)
-  (cl-macrolet ((wsetq (sym val) `(set-window-parameter window ',sym ,val))
-                (wgetq (sym) `(window-parameter window ',sym)))
-    (let ((buf (window-buffer window))
-          (timer (wgetq eglot--inlay-hints-timer))
-          (last-display-start (wgetq eglot--last-inlay-hint-display-start)))
-      (when (and eglot-lazy-inlay-hints
-                 ;; FIXME: If `window' is _not_ the selected window,
-                 ;; then for some unknown reason probably related to
-                 ;; the overlays added later to the buffer, the scroll
-                 ;; function will be called indefinitely.  Not sure if
-                 ;; an Emacs bug, but prevent useless duplicate calls
-                 ;; by saving and examining `display-start' fixes it.
-                 (not (eql last-display-start display-start)))
-        (when timer (cancel-timer timer))
-        (wsetq eglot--last-inlay-hint-display-start
-               display-start)
-        (wsetq eglot--inlay-hints-timer
-               (run-at-time
-                eglot-lazy-inlay-hints
-                nil (lambda ()
-                      (eglot--when-live-buffer buf
-                        (when (eq buf (window-buffer window))
-                          (eglot--update-hints-1 (window-start window)
-                                                 (window-end window))
-                          (wsetq eglot--inlay-hints-timer nil))))))))))
-
-(defun eglot--inlay-hints-after-window-config-change ()
-  (eglot--update-hints-1 (window-start) (window-end)))
-
 (define-minor-mode eglot-inlay-hints-mode
   "Minor mode for annotating buffers with LSP server's inlay hints."
   :global nil
@@ -3584,28 +3587,15 @@ eglot-inlay-hints-mode
            (eglot--warn
             "No :inlayHintProvider support. Inlay hints will not work."))
           (eglot-lazy-inlay-hints
-           (add-hook 'eglot--document-changed-hook
-                     #'eglot--inlay-hints-lazily t t)
-           (add-hook 'window-scroll-functions
-                     #'eglot--inlay-hints-after-scroll nil t)
-           (add-hook 'window-configuration-change-hook
-                     #'eglot--inlay-hints-after-window-config-change nil t)
-           ;; Maybe there isn't a window yet for current buffer,
-           ;; so `run-at-time' ensures this runs after redisplay.
-           (run-at-time 0 nil #'eglot--inlay-hints-lazily))
+           (jit-lock-register #'eglot--update-hints))
           (t
            (add-hook 'eglot--document-changed-hook
                      #'eglot--inlay-hints-fully nil t)
            (eglot--inlay-hints-fully))))
         (t
-         (remove-hook 'window-configuration-change-hook
-                      #'eglot--inlay-hints-after-window-config-change)
-         (remove-hook 'eglot--document-changed-hook
-                      #'eglot--inlay-hints-lazily t)
+         (jit-lock-unregister #'eglot--update-hints)
          (remove-hook 'eglot--document-changed-hook
                       #'eglot--inlay-hints-fully t)
-         (remove-hook 'window-scroll-functions
-                      #'eglot--inlay-hints-after-scroll t)
          (remove-overlays nil nil 'eglot--inlay-hint t))))

Re: Eglot "inlay hints" landed

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Thu, 23 Feb 2023 19:26:10 +0000
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, dalal.chinmay.0101@gmail.com, 
> 	emacs-devel@gnu.org, dimitri@belopopsky.com, luangruo@yahoo.com
> 
> > I actually gave you a recipe for demonstrating the problems I have in
> > mind: scroll the window under pixel-scroll-precision-mode.  AFAIK, we
> > don't call window-scroll-functions in that case.
> 
> I don't have a pixel-scroll-enabled Emacs to test.  Can you scroll
> large portions of the window like that and w-s-functions will never
> get called?  I'd say that's a bug we should fix.
> 
> > Another situation where we don't call window-scroll-functions is when
> > the user types into the buffer.
> 
> That is handled by Eglot's after-change-functions already so it
> isn't a problem.
> 
> > Yet another situation is when you type "C-x 1" to delete all the other
> > windows on the frame, leaving the current window that now shows more
> > stuff than before.
> 
> Reproduced.  Easy enough to fix with window-configuration-change-functions.
> I pushed a fix.

My point is that using jit-lock machinery, you will never miss an
update, because redisplay is very good at knowing when something needs
to be redrawn.  It has to.  By contrast, all those hooks are less
reliable, and also make Emacs sluggish, because the hooks are many
times triggered when there's nothing to do wrt display.

Re: Eglot "inlay hints" landed

[João Távora]

On Thu, Feb 23, 2023 at 7:54 PM Eli Zaretskii <eliz@gnu.org> wrote:

> > Reproduced.  Easy enough to fix with window-configuration-change-functions.
> > I pushed a fix.
>
> My point is that using jit-lock machinery, you will never miss an
> update, because redisplay is very good at knowing when something needs
> to be redrawn.  It has to.

Sure, I agree there.

> By contrast, all those hooks are less
> reliable, and also make Emacs sluggish, because the hooks are many
> times triggered when there's nothing to do wrt display.

It remains to be seen whether this is more or less sluggish than the jit-lock
version, because at least the naive jit-lock version is making lots of
requests over to the process.  In TRAMP scenarios or networking
scenarios, this may be slow.  So I'm am focusing on finding the best solution
but it's not immediately obvious that jit is always best, because of the fixed
overhead (in this case LSP comms), which doesn't exist in the use cases
jit-lock was designed for.

João

Re: Eglot "inlay hints" landed

[Stefan Monnier]

> I'd love to switch over to the jit-lock implementation as it has
> potential to be much neater.  But I can't seem to get it to not
> over-request stuff. I attach the patch I've been trying, and it's
> clearly got some thinkos when you test it.  It doesn't help that
> `window-start` and `window-end` aren't -- apparently -- reliable
> when called from a jit-lock function.

Sorry, I have not followed the discussion, so I don't know what are
"inlay hints" nor how to test them.  What problem do you see what you
try your code?

> +(defvar-local eglot--inlay-hints-outstanding (cons nil nil)
> +  "Largest (BEG . END) window subregion not yet updated for inlay hints.")
> +
> +(defun eglot--update-hints (from to)
> +  "Jit-lock function for Eglot inlay hints."
> +  (cl-symbol-macrolet ((x eglot--inlay-hints-outstanding))
> +    (setq x (cons (min from (or (car x) most-positive-fixnum))
> +                  (max to (or (cdr x) 0))))
> +    (let ((w (get-buffer-window)))
> +      (trace-values "window:      " w)
> +      (trace-values "window-region:      " (cons (window-start w) (window-end w)))
> +      (trace-values "outstanding: " eglot--inlay-hints-outstanding)
> +      ;  FIXME: this is NOT the correct condition to decide when to
> +      ;  request stuff from the server.
> +      (when (or (< (car x) (window-start w)) (> (cdr x) (window-end w)))
> +        (unwind-protect
> +            (eglot--update-hints-1 (car x) (cdr x))
> +          (setq x (cons nil nil)))))))

Why do you compare to `window-start/end`?

As you say, they're not reliably available during jit-lock, and that's
for fundamental reasons: jit-lock effects can change `window-start/end`.

But I can't see a good reason why you'd need to do such comparison:
the fact that jit-lock calls you is supposed to say "we need this for
redisplay", so you shouldn't need to double-check against window bounds.

Unless maybe you have `jit-lock-stealth-time` set to a non-nil value, in
which case indeed jit-lock will be called even on non-displayed areas of
the buffer, but it's arguably what the user (you in this case) asked for.

IOW, you should be able to skip `eglot--update-hints` altogether and use
`eglot--update-hints-1` directly (modulo catching errors and such maybe).


        Stefan

Re: Eglot "inlay hints" landed

[João Távora]

On Thu, Feb 23, 2023 at 7:28 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> > I'd love to switch over to the jit-lock implementation as it has
> > potential to be much neater.  But I can't seem to get it to not
> > over-request stuff. I attach the patch I've been trying, and it's
> > clearly got some thinkos when you test it.  It doesn't help that
> > `window-start` and `window-end` aren't -- apparently -- reliable
> > when called from a jit-lock function.
>
> Sorry, I have not followed the discussion, so I don't know what are
> "inlay hints" nor how to test them.  What problem do you see what you
> try your code?

Hi Stefan.  Inlay hints are described summarily in the very first
message to this thread and in the commit implementing them.  They're
very many 0-length overlays, almost like tooltips, annotating
type deductions and positional parameter names.

The goal here is to request them at the correct time and balance
multiple objectives:

1. minimize the number of requests
2. ensure the request covers all visible portions of the buffer
across any windows showing it.
3. don't request for too large a region, else the amount of data can
be prohibitive.

The latest message I sent to Eli has my best shot at it.  If you have
a clangd executable somewhere, you can test this rather easily with
the eglot.el in core and a cpp file such as

void foo(int bar) {}

int main() {
   foo(42);
   foo(42);
  ... very many lines of this ...
   foo(42);
   foo(42);
   foo(42);
}

You should see some intangible text appear in the buffer so that
it will look like:


void foo(int bar) {}

int main() {
   foo( bar: 42);
   foo( bar: 42);
  ... very many lines of this ...
   foo( bar: 42);
   foo( bar: 42);
   foo( bar: 42);
}

Then scroll around in the buffer and check the traces of
eglot--update-hints-1.  Try to change that 'int bar' to
'int quux' and see if the system reacts correctly to the change
updating the ' bar: ' hint to ' quux: '.

See also my last message to Eli, which contains a horrible
patch that nevertheless does more or less achieve those
objectives.

João

Re: Eglot "inlay hints" landed

[Stefan Monnier]

> See also my last message to Eli, which contains a horrible
> patch that nevertheless does more or less achieve those
> objectives.

Hmm... that patch looks reasonably sane to me (while writing the
previous message I hesitated to suggest to delay the actual work to
after redisplay so you can coalesce several jit chunks).

Which part do you find horrible?


        Stefan

Re: Eglot "inlay hints" landed

[João Távora]

On Thu, Feb 23, 2023 at 7:53 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> > See also my last message to Eli, which contains a horrible
> > patch that nevertheless does more or less achieve those
> > objectives.
>
> Hmm... that patch looks reasonably sane to me (while writing the
> previous message I hesitated to suggest to delay the actual work to
> after redisplay so you can coalesce several jit chunks).
>
> Which part do you find horrible?

Mostly the fact that it's operating on a separate timer, but one
that is directly correlated to jit-lock-context-time.  So
the bookkeeping and the coalescing of the small+large
jit chunks should be provided by the jit infrastructure instead. And
then no extra timer or logic would be needed.  Can we make jit-lock.el
a :core ELPA package?

João

Re: Eglot "inlay hints" landed

[Stefan Monnier]

> Mostly the fact that it's operating on a separate timer, but one
> that is directly correlated to jit-lock-context-time.

But that's because you're willing to wait for the context refresh to do
your own.

> So the bookkeeping and the coalescing of the small+large jit chunks
> should be provided by the jit infrastructure instead.

So far, you're the first to need such a thing.  In my experience the
needs for "jit display refresh" can be fairly subtly different, so it's
not clear how generally useful your approach would be.  Maybe we could
provide some shared infrastructure to maintain a "coalescing set of
buffer regions", but if so, I suspect that it wouldn't need to be tied
to `jit-lock.el`.

Also, I'm not sure it gives exactly the info you need/want:
I suspect that in some languages you can have:

   foo (x)
   ...
   function foo (bar : Int)

so that changing the `foo` definition will need to update the inlay on
the call to `foo` that is earlier in the buffer, hence jit-lock-context
refresh won't be sufficient and you'll need to force your own refresh.
[ I think jit-lock would benefit from being able to flush a particular
  backend's without forcing all of the backends at the sane time.  ]

> And then no extra timer or logic would be needed.

You mean you'd integrate it into `jit-lock-context`?
Maybe that could be done.

> Can we make jit-lock.el a :core ELPA package?

I have no opinion on that.


        Stefan

Re: Eglot "inlay hints" landed

[João Távora]

On Thu, Feb 23, 2023 at 10:19 PM Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>
> > Mostly the fact that it's operating on a separate timer, but one
> > that is directly correlated to jit-lock-context-time.
>
> But that's because you're willing to wait for the context refresh to do
> your own.
>
> > So the bookkeeping and the coalescing of the small+large jit chunks
> > should be provided by the jit infrastructure instead.
>
> So far, you're the first to need such a thing.  In my experience the
> needs for "jit display refresh" can be fairly subtly different, so it's
> not clear how generally useful your approach would be.  Maybe we could
> provide some shared infrastructure to maintain a "coalescing set of
> buffer regions", but if so, I suspect that it wouldn't need to be tied
> to `jit-lock.el`.
>
> Also, I'm not sure it gives exactly the info you need/want:
> I suspect that in some languages you can have:
>
>    foo (x)
>    ...
>    function foo (bar : Int)

Yeah, in that case we're frobbed.  But isn't that a problem already
those "some languages" for regular "contextual" fontification?

> > And then no extra timer or logic would be needed.
>
> You mean you'd integrate it into `jit-lock-context`?
> Maybe that could be done.
> > Can we make jit-lock.el a :core ELPA package?
> I have no opinion on that.

That's the only reasonable way that Eglot can ever be changed
to make use of new jit-lock infrastructure.

Anyway, have a look at this patch:

This implementation is much simpler than the one based on
windows-scroll-functions.  It's also conceptually safer, since
jit-lock is guaranteed to refontify the regions that may need
redisplaying.

It's not trivially simple though, as simply adding
'eglot--update-hints-1' to jit-lock-functions, while possible, is
going to request inlay hints from the LSP server for every small
fraction changed by the user.  So we do some bookkeeping on regions
and coalesce them at a suitable time for a larger request.

To do this, we use a jit-lock implementation detail,
jit-lock-context-unfontify-pos, which tells us that the contextual
fontification has just finished.  Not sure how brittle it is, but it
seems to work reasonably.

* doc/misc/eglot.texi (Eglot Variables): Remove mention
to deleted eglot-lazy-inlay-hints.

* lisp/progmodes/eglot.el (eglot-lazy-inlay-hints)
(eglot--inlay-hints-after-scroll)
(eglot--inlay-hints-fully)
(eglot--inlay-hints-lazily): Remove.
(eglot--update-hints): Add function.
(eglot-inlay-hints-mode): Simplify.
---
diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index eea8be6d1aa..9955b04574f 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -3489,32 +3489,38 @@ eglot-type-hint-face
 (defface eglot-parameter-hint-face '((t (:inherit eglot-inlay-hint-face)))
   "Face used for parameter inlay hint overlays.")

-(defcustom eglot-lazy-inlay-hints 0.3
-  "If non-nil, restrict LSP inlay hints to visible portion of the buffer.
-
-Value is a number specifying how many seconds to wait after a
-window has been (re)scrolled before requesting new inlay hints
-for the now-visible portion of the buffer shown in the window.
-
-If nil, then inlay hints are requested for the entire buffer.
-This could be slow.
-
-This value is only meaningful if the minor mode
-`eglot-inlay-hints-mode' is turned on in a buffer."
-  :type 'number
-  :version "29.1")
-
-(defun eglot--inlay-hints-fully ()
-  (eglot--widening (eglot--update-hints-1 (point-min) (point-max))))
-
-(cl-defun eglot--inlay-hints-lazily (&optional (buffer (current-buffer)))
-  (eglot--when-live-buffer buffer
-    (when eglot--managed-mode
-      (dolist (window (get-buffer-window-list nil nil 'visible))
-        (eglot--update-hints-1 (window-start window) (window-end window))))))
+(defvar-local eglot--outstanding-inlay-regions nil
+  "List of regions that weren't yet \"contextually\" fontified.")
+
+(defun eglot--update-hints (from to)
+  "Jit-lock function for Eglot inlay hints."
+  ;; HACK: Comparing `jit-lock-context-unfontify-pos' to `point-max'
+  ;; is our way of telling whether this call to `jit-lock-functions'
+  ;; happens after `jit-lock-context-timer' has just run.  This is the
+  ;; best time to coalesce the regions in
+  ;; `eglot--outstanding-inlay-regions' and contact the LSP server.  Any
+  ;; other call to this function just adds another region to that list.
+  (if (= jit-lock-context-unfontify-pos (point-max))
+      (let* ((sorted (sort (cons (cons from to)
+                                 eglot--outstanding-inlay-regions)
+                           (pcase-lambda (`(,a1 . ,a2) `(,b1 . ,b2))
+                             (if (= a1 b1) (< a2 b2) (< a1 b1)))))
+             (coalesced (cl-reduce (lambda (acc x)
+                                     (cond ((and acc (>= (cdr (car acc))
+                                                         (car x)))
+                                            (setcdr (car acc) (cdr x))
+                                            acc)
+                                           (t (push x acc) acc)))
+                                   sorted
+                                   :initial-value nil)))
+        (dolist (r coalesced)
+          (eglot--update-hints-1 (max (car r) (point-min))
+                                 (min (cdr r) (point-max))))
+        (setq eglot--outstanding-inlay-regions nil))
+    (push (cons from to) eglot--outstanding-inlay-regions)))

 (defun eglot--update-hints-1 (from to)
-  "Request LSP inlay hints and annotate current buffer from FROM to TO."
+  "Do most work for `eglot--update-hints', including LSP request."
   (let* ((buf (current-buffer))
          (paint-hint
           (eglot--lambda ((InlayHint) position kind label paddingLeft
paddingRight)
@@ -3545,67 +3551,16 @@ eglot--update-hints-1
                       (mapc paint-hint hints))))
      :deferred 'eglot--update-hints-1)))

-(defun eglot--inlay-hints-after-scroll (window display-start)
-  (cl-macrolet ((wsetq (sym val) `(set-window-parameter window ',sym ,val))
-                (wgetq (sym) `(window-parameter window ',sym)))
-    (let ((buf (window-buffer window))
-          (timer (wgetq eglot--inlay-hints-timer))
-          (last-display-start (wgetq eglot--last-inlay-hint-display-start)))
-      (when (and eglot-lazy-inlay-hints
-                 ;; FIXME: If `window' is _not_ the selected window,
-                 ;; then for some unknown reason probably related to
-                 ;; the overlays added later to the buffer, the scroll
-                 ;; function will be called indefinitely.  Not sure if
-                 ;; an Emacs bug, but prevent useless duplicate calls
-                 ;; by saving and examining `display-start' fixes it.
-                 (not (eql last-display-start display-start)))
-        (when timer (cancel-timer timer))
-        (wsetq eglot--last-inlay-hint-display-start
-               display-start)
-        (wsetq eglot--inlay-hints-timer
-               (run-at-time
-                eglot-lazy-inlay-hints
-                nil (lambda ()
-                      (eglot--when-live-buffer buf
-                        (when (eq buf (window-buffer window))
-                          (eglot--update-hints-1 (window-start window)
-                                                 (window-end window))
-                          (wsetq eglot--inlay-hints-timer nil))))))))))
-
-(defun eglot--inlay-hints-after-window-config-change ()
-  (eglot--update-hints-1 (window-start) (window-end)))
-
 (define-minor-mode eglot-inlay-hints-mode
   "Minor mode for annotating buffers with LSP server's inlay hints."
   :global nil
   (cond (eglot-inlay-hints-mode
-         (cond
-          ((not (eglot--server-capable :inlayHintProvider))
+         (if (eglot--server-capable :inlayHintProvider)
+             (jit-lock-register #'eglot--update-hints 'contextual)
            (eglot--warn
-            "No :inlayHintProvider support. Inlay hints will not work."))
-          (eglot-lazy-inlay-hints
-           (add-hook 'eglot--document-changed-hook
-                     #'eglot--inlay-hints-lazily t t)
-           (add-hook 'window-scroll-functions
-                     #'eglot--inlay-hints-after-scroll nil t)
-           (add-hook 'window-configuration-change-hook
-                     #'eglot--inlay-hints-after-window-config-change nil t)
-           ;; Maybe there isn't a window yet for current buffer,
-           ;; so `run-at-time' ensures this runs after redisplay.
-           (run-at-time 0 nil #'eglot--inlay-hints-lazily))
-          (t
-           (add-hook 'eglot--document-changed-hook
-                     #'eglot--inlay-hints-fully nil t)
-           (eglot--inlay-hints-fully))))
+            "No :inlayHintProvider support. Inlay hints will not work.")))
         (t
-         (remove-hook 'window-configuration-change-hook
-                      #'eglot--inlay-hints-after-window-config-change)
-         (remove-hook 'eglot--document-changed-hook
-                      #'eglot--inlay-hints-lazily t)
-         (remove-hook 'eglot--document-changed-hook
-                      #'eglot--inlay-hints-fully t)
-         (remove-hook 'window-scroll-functions
-                      #'eglot--inlay-hints-after-scroll t)
+         (jit-lock-unregister #'eglot--update-hints)
          (remove-overlays nil nil 'eglot--inlay-hint t))))

Re: Eglot "inlay hints" landed

[Stefan Monnier]

>> Also, I'm not sure it gives exactly the info you need/want:
>> I suspect that in some languages you can have:
>>
>>    foo (x)
>>    ...
>>    function foo (bar : Int)
>
> Yeah, in that case we're frobbed.  But isn't that a problem already
> those "some languages" for regular "contextual" fontification?

Yes and no: yes, there are already cases where the jit-lock-context
heuristic of refreshing everything *after* a modification is not
sufficient, but as a general rule programming languages are designed for
"forward-only parsing" so it's unusual for a change at POS to affect the
parsing before POS (and most uses of jit-lock limit themselves to
syntactic information, so there are rather few cases where this is
a problem).  In those few cases we use things like the
`jit-lock-defer-multiline` property which the major mode's font-lock
rules can place manually in an ad-hoc manner.

In your case, I suspect for the affected language servers there's not much
more we can do but to say that the whole buffer's inlays need to be
recomputed after a change.

> To do this, we use a jit-lock implementation detail,
> jit-lock-context-unfontify-pos, which tells us that the contextual
> fontification has just finished.  Not sure how brittle it is, but it
> seems to work reasonably.

I think it won't work reliably in the case where the "immediate"
jit-lock refresh happens to cover everything until window-end (e.g. we
just inserted chunk of text that ends after window-end).  In that case
the jit-lock-context thingy will just mark the rest of the buffer as
"not fresh" but won't call your fontification function at all.
[ And of course, there are also those few rare modes that don't use
  jit-lock-context at all.  ]

I think using your own timer is probably the better option.  I'd
make it wait for `eglot-lazy-inlay-hints` rather than (+
eglot-lazy-inlay-hints jit-lock-context-time), but I'd give it as
default value a value computed from `jit-lock-context-time`.


        Stefan

Re: Eglot "inlay hints" landed

[João Távora]

On Fri, Feb 24, 2023 at 1:08 AM Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> In your case, I suspect for the affected language servers there's not much
> more we can do but to say that the whole buffer's inlays need to be
> recomputed after a change.

I'd really like to avoid that.  If it comes to that, and the problem
really is annoying enough (it might not be, and inlay hints are usually
a best-effort thing by the servers themselves) then I think going back
to the window-scroll-functions might be a better option.  It doesn't have this
problem.

> > To do this, we use a jit-lock implementation detail,
> > jit-lock-context-unfontify-pos, which tells us that the contextual
> > fontification has just finished.  Not sure how brittle it is, but it
> > seems to work reasonably.
>
> I think it won't work reliably in the case where the "immediate"
> jit-lock refresh happens to cover everything until window-end (e.g. we
> just inserted chunk of text that ends after window-end).  In that case
> the jit-lock-context thingy will just mark the rest of the buffer as
> "not fresh" but won't call your fontification function at all.

Yes, I noticed something like that: many immediate small jit lock calls
where the condition was true.

> [ And of course, there are also those few rare modes that don't use
>   jit-lock-context at all.  ]
>
> I think using your own timer is probably the better option.  I'd
> make it wait for `eglot-lazy-inlay-hints` rather than (+
> eglot-lazy-inlay-hints jit-lock-context-time), but I'd give it as
> default value a value computed from `jit-lock-context-time`.

Using a timer is necessary, but I've found that a 0-second timer is
sufficient to coalesce "well".  But we only fire the timer if
the "unfontify-pos == point-max" condition is met.  Then we don't
need to compete with jit-lock-context-timer, which is really the part I found
very bad.

I've pushed this last version to emacs-29.  Have a look there,
please.  And thanks for the insight!

João

Re: Eglot "inlay hints" landed

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  dalal.chinmay.0101@gmail.com,
>   emacs-devel@gnu.org,  dimitri@belopopsky.com,  luangruo@yahoo.com
> Date: Thu, 23 Feb 2023 20:08:41 -0500
> 
> Yes and no: yes, there are already cases where the jit-lock-context
> heuristic of refreshing everything *after* a modification is not
> sufficient

If a jit-lock client is smart enough, it should itself install an
after-change function to remove the 'fontified property from the
portions of buffer text affected by a change that are before the first
changed position (or maybe even in other buffers).

> but as a general rule programming languages are designed for
> "forward-only parsing" so it's unusual for a change at POS to affect the
> parsing before POS

Strictly speaking, this is inaccurate: a change in the middle of a
line could very well affect parsing of that entire line, so it in
effect affects everything from the beginning of that line (perhaps
even more, if that line is a continued line, in the language syntax).
But jit-lock is supposed to already account for that, and call the
fontification-functions with START set to the beginning of the line,
unless I'm misremembering.  (You already know all that, but maybe João
doesn't.)

> > To do this, we use a jit-lock implementation detail,
> > jit-lock-context-unfontify-pos, which tells us that the contextual
> > fontification has just finished.  Not sure how brittle it is, but it
> > seems to work reasonably.
> 
> I think it won't work reliably in the case where the "immediate"
> jit-lock refresh happens to cover everything until window-end (e.g. we
> just inserted chunk of text that ends after window-end).  In that case
> the jit-lock-context thingy will just mark the rest of the buffer as
> "not fresh" but won't call your fontification function at all.
> [ And of course, there are also those few rare modes that don't use
>   jit-lock-context at all.  ]

jit-lock doesn't use window-end.  It can't, really: the results of
fontification could affect where the window ends, which is why
window-end is only updated when redisplay of a window successfully
finishes, which is way after jit-lock did its job.

That is why jit-lock calls fontification-functions with END that is
arbitrarily computed from START; this is controlled by
jit-lock-chunk-size.  And a function called via
fontification-functions could legitimately decide it fontifies more
than just the chunk for which it was called.

(Again, you already know all that.  I'm writing this to avoid
potential misunderstandings and confusion.)

Re: Eglot "inlay hints" landed

[João Távora]

On Fri, Feb 24, 2023 at 7:35 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Stefan Monnier <monnier@iro.umontreal.ca>
> > Cc: Eli Zaretskii <eliz@gnu.org>,  dalal.chinmay.0101@gmail.com,
> >   emacs-devel@gnu.org,  dimitri@belopopsky.com,  luangruo@yahoo.com
> > Date: Thu, 23 Feb 2023 20:08:41 -0500
> >
> > Yes and no: yes, there are already cases where the jit-lock-context
> > heuristic of refreshing everything *after* a modification is not
> > sufficient
>
> If a jit-lock client is smart enough, it should itself install an
> after-change function to remove the 'fontified property from the
> portions of buffer text affected by a change that are before the first
> changed position (or maybe even in other buffers).

Thanks, I was going to ask about that. I suppose this is how one
marks sections of a buffer "dirty" or invalidatest them so that
jit-lock knows it must run over them again when it sees fit.

If it works with other buffers as well, it might provide an
elegant solution to the "A affects B" scenario we discussed
earlier.

> > but as a general rule programming languages are designed for
> > "forward-only parsing" so it's unusual for a change at POS to affect the
> > parsing before POS
>
> Strictly speaking, this is inaccurate: a change in the middle of a
> line could very well affect parsing of that entire line, so it in
> effect affects everything from the beginning of that line (perhaps
> even more, if that line is a continued line, in the language syntax).
> But jit-lock is supposed to already account for that, and call the
> fontification-functions with START set to the beginning of the line,
> unless I'm misremembering.  (You already know all that, but maybe João
> doesn't.)

Thanks, I think I can confirm this.  I've observed this behaviour.
Jit lock starts in the beginning of the line to end of the line
in a small chunk containing the small changed region, only then
proceeds to larger chunks (after some jit-lock-contextual-time,
apparently).

> > > To do this, we use a jit-lock implementation detail,
> > > jit-lock-context-unfontify-pos, which tells us that the contextual
> > > fontification has just finished.  Not sure how brittle it is, but it
> > > seems to work reasonably.
> >
> > I think it won't work reliably in the case where the "immediate"
> > jit-lock refresh happens to cover everything until window-end (e.g. we
> > just inserted chunk of text that ends after window-end).  In that case
> > the jit-lock-context thingy will just mark the rest of the buffer as
> > "not fresh" but won't call your fontification function at all.
> > [ And of course, there are also those few rare modes that don't use
> >   jit-lock-context at all.  ]
>
> jit-lock doesn't use window-end.  It can't, really: the results of
> fontification could affect where the window ends, which is why
> window-end is only updated when redisplay of a window successfully
> finishes, which is way after jit-lock did its job.
>
> That is why jit-lock calls fontification-functions with END that is
> arbitrarily computed from START; this is controlled by
> jit-lock-chunk-size.  And a function called via
> fontification-functions could legitimately decide it fontifies more
> than just the chunk for which it was called.

And Eglot's eglot--update-hints is indeed one such function.  The
coalescing of small jit-lock chunks is very "generous" and it will
request inlay hints for things in the middle that jit-lock didn't
strictly think are necessary.  This works out well in my testing
with the latest version of the feature installed in emacs-29.

> (Again, you already know all that.  I'm writing this to avoid
> potential misunderstandings and confusion.)

Thanks for the clarifications.

João

Re: Eglot "inlay hints" landed

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Fri, 24 Feb 2023 10:42:26 +0000
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, dalal.chinmay.0101@gmail.com, 
> 	emacs-devel@gnu.org, dimitri@belopopsky.com, luangruo@yahoo.com
> 
> On Fri, Feb 24, 2023 at 7:35 AM Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > If a jit-lock client is smart enough, it should itself install an
> > after-change function to remove the 'fontified property from the
> > portions of buffer text affected by a change that are before the first
> > changed position (or maybe even in other buffers).
> 
> Thanks, I was going to ask about that. I suppose this is how one
> marks sections of a buffer "dirty" or invalidatest them so that
> jit-lock knows it must run over them again when it sees fit.
> 
> If it works with other buffers as well, it might provide an
> elegant solution to the "A affects B" scenario we discussed
> earlier.

I don't see why it wouldn't work.  jit-lock relies on the 'fontified'
property to know which part(s) of buffer text need (re-)fontification
when they are about to be shown on display.  So any portion of buffer
text where this property is nil will cause jit-lock to call
fontification-functions on that part.

Re: Eglot "inlay hints" landed

[João Távora]

On Fri, Feb 24, 2023 at 11:33 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: João Távora <joaotavora@gmail.com>
> > Date: Fri, 24 Feb 2023 10:42:26 +0000
> > Cc: Stefan Monnier <monnier@iro.umontreal.ca>, dalal.chinmay.0101@gmail.com,
> >       emacs-devel@gnu.org, dimitri@belopopsky.com, luangruo@yahoo.com
> >
> > On Fri, Feb 24, 2023 at 7:35 AM Eli Zaretskii <eliz@gnu.org> wrote:
> > >
> > > If a jit-lock client is smart enough, it should itself install an
> > > after-change function to remove the 'fontified property from the
> > > portions of buffer text affected by a change that are before the first
> > > changed position (or maybe even in other buffers).
> >
> > Thanks, I was going to ask about that. I suppose this is how one
> > marks sections of a buffer "dirty" or invalidatest them so that
> > jit-lock knows it must run over them again when it sees fit.
> >
> > If it works with other buffers as well, it might provide an
> > elegant solution to the "A affects B" scenario we discussed
> > earlier.
>
> I don't see why it wouldn't work.

Yes.  I never said it wouldn't :-) (and you never said I said,
I'm just clarifying :-).

Anyway, there is also this to this topic.

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_inlayHint_refresh

an "inlay hint refresh request" from the server.  A separate
capability apparently designed to cover this case.  I don't
know how it fits into the equation but I would think it was
designed for editors with none of our jit-capabilities.  Maybe
it's mostly useless to us now, especially given its poor
interface (it's just a bare request with no parameters).

It does illustrate the typical LSP line of reasoning: "the server
knows best, because the server knows the language".  IOWs the LSP
server knowledge stands for Emacs  major-mode knowledge.  The
difference here is that the interfaces to extract that knowledge
from the server are very limited or poorly designed.

João

Re: Eglot "inlay hints" landed

[Tassilo Horn]

João Távora <joaotavora@gmail.com> writes:

Hi João,

> I've just landed Eglot "inlay hints" on lisp/progmodes/eglot.el in the
> emacs-29 branch.  It's a feature that some users (including me) were
> looking for for some time.

I've just tried it out within a rust project of mine and it's fabulous!
Thanks a lot.

Bye,
Tassilo

Re: Eglot "inlay hints" landed

[Chinmay Dalal]

I've come to realize that the pop-in is not a real problem - if one
is to read the information provided by the hints they have to spend time
doing that anyway

Chinmay

Re: Eglot "inlay hints" landed

[Nikola Pajkovsky]

João Távora <joaotavora@gmail.com> writes:

> I've just landed Eglot "inlay hints" on lisp/progmodes/eglot.el in the
> emacs-29 branch.  It's a feature that some users (including me) were
> looking for for some time.

Nice work João!

I tried rust inlay hints, and it does not work for me. I bet it's because
eglot uses label only if it's string. The same problem was in lsp-mode.

  https://github.com/emacs-lsp/lsp-mode/commit/6b01d49757994c09c90623bf67f072d02f00f8e9

--
Nikola

Re: Eglot "inlay hints" landed

[João Távora]

On Thu, Feb 23, 2023 at 7:50 PM Nikola Pajkovsky <n.pajkovsky@gmail.com> wrote:
>
> João Távora <joaotavora@gmail.com> writes:
>
> > I've just landed Eglot "inlay hints" on lisp/progmodes/eglot.el in the
> > emacs-29 branch.  It's a feature that some users (including me) were
> > looking for for some time.
>
> Nice work João!
>
> I tried rust inlay hints, and it does not work for me. I bet it's because
> eglot uses label only if it's string. The same problem was in lsp-mode.

Actually, I think I have code for that.  So please show a reproduction
recipe.  I think someone reported here that hints are working
correctly with rust-analyzer, and I've tried it briefly myself with good
results. So it must be something else.

João

Re: Eglot "inlay hints" landed

[Nikola Pajkovsky]

João Távora <joaotavora@gmail.com> writes:

> On Thu, Feb 23, 2023 at 7:50 PM Nikola Pajkovsky <n.pajkovsky@gmail.com> wrote:
>>
>> João Távora <joaotavora@gmail.com> writes:
>>
>> > I've just landed Eglot "inlay hints" on lisp/progmodes/eglot.el in the
>> > emacs-29 branch.  It's a feature that some users (including me) were
>> > looking for for some time.
>>
>> Nice work João!
>>
>> I tried rust inlay hints, and it does not work for me. I bet it's because
>> eglot uses label only if it's string. The same problem was in lsp-mode.
>
> Actually, I think I have code for that.  So please show a reproduction
> recipe.  I think someone reported here that hints are working
> correctly with rust-analyzer, and I've tried it briefly myself with good
> results. So it must be something else.

Interesting.

```
-> % rust-analyzer --version
rust-analyzer 0.3.1402-standalone
```

For instance, I don't see the inlay hints for the code like this:

```
struct Foo {
    b: u32,
}

fn main() {
    let s = Foo { b: 10 };
    let f = std::fs::File::create("t");
}
```

Re: Eglot "inlay hints" landed

[Chinmay Dalal]

[-- Attachment #1: Type: text/plain, Size: 1830 bytes --]


João Távora <joaotavora@gmail.com> writes:

> On Thu, Feb 23, 2023 at 7:50 PM Nikola Pajkovsky <n.pajkovsky@gmail.com> wrote:
>>
>> João Távora <joaotavora@gmail.com> writes:
>>
>> > I've just landed Eglot "inlay hints" on lisp/progmodes/eglot.el in the
>> > emacs-29 branch.  It's a feature that some users (including me) were
>> > looking for for some time.
>>
>> Nice work João!
>>
>> I tried rust inlay hints, and it does not work for me. I bet it's because
>> eglot uses label only if it's string. The same problem was in lsp-mode.
>
> Actually, I think I have code for that.  So please show a reproduction
> recipe.  I think someone reported here that hints are working
> correctly with rust-analyzer, and I've tried it briefly myself with good
> results. So it must be something else.
>
> João


You can look at the end of any long rust function with hl-line-mode on:
there's an empty overlay but it's supposed to have a link to the
beginning of the function. Screenshot attached.

I think the bug is in eglot--update-hints-1:

(if (stringp label) label (plist-get label :value))

It should instead be the value of :value from the _first element_ of
label, as the spec says a label is a string or a _list of_ `InlayHintLabelPart`s:

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#inlayHint

The response from the server is of this form:

(:position (:line 95 :character 1)
 :label [(:value "fn keyboard_interrupt_handler"
          :location (:uri "file:///home/chinmay/stuff/rust/blog_os/src/interrupts.rs"
                     :range (:start (:line 67 :character 26)
                             :end (:line 67 :character 52))))]
 :tooltip "fn keyboard_interrupt_handler"
 :paddingLeft t
 :paddingRight
 :json-false)



[-- Attachment #2: empty overlay --]
[-- Type: image/png, Size: 1424 bytes --]

Re: Eglot "inlay hints" landed

[Chinmay Dalal]

Chinmay Dalal <dalal.chinmay.0101@gmail.com> writes:

> João Távora <joaotavora@gmail.com> writes:
>
>> On Thu, Feb 23, 2023 at 7:50 PM Nikola Pajkovsky <n.pajkovsky@gmail.com> wrote:
>>>
>>> João Távora <joaotavora@gmail.com> writes:
>>>
>>> > I've just landed Eglot "inlay hints" on lisp/progmodes/eglot.el in the
>>> > emacs-29 branch.  It's a feature that some users (including me) were
>>> > looking for for some time.
>>>
>>> Nice work João!
>>>
>>> I tried rust inlay hints, and it does not work for me. I bet it's because
>>> eglot uses label only if it's string. The same problem was in lsp-mode.
>>
>> Actually, I think I have code for that.  So please show a reproduction
>> recipe.  I think someone reported here that hints are working
>> correctly with rust-analyzer, and I've tried it briefly myself with good
>> results. So it must be something else.
>>
>> João
>
>
> You can look at the end of any long rust function with hl-line-mode on:
> there's an empty overlay but it's supposed to have a link to the
> beginning of the function. Screenshot attached.
>
> I think the bug is in eglot--update-hints-1:
>
> (if (stringp label) label (plist-get label :value))
>
> It should instead be the value of :value from the _first element_ of
> label, as the spec says a label is a string or a _list of_ `InlayHintLabelPart`s:
>
> https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#inlayHint
>
> The response from the server is of this form:
>
> (:position (:line 95 :character 1)
>  :label [(:value "fn keyboard_interrupt_handler"
>           :location (:uri "file:///home/chinmay/stuff/rust/blog_os/src/interrupts.rs"
>                      :range (:start (:line 67 :character 26)
>                              :end (:line 67 :character 52))))]
>  :tooltip "fn keyboard_interrupt_handler"
>  :paddingLeft t
>  :paddingRight
>  :json-false)
>
>
> [2. empty overlay --- image/png; swappy-20230224-100015.png]...


Of course, this is not ideal when there are multiple elements in the
label list (but not strictly wrong either). I raised this point when I
opened bug#61412:
https://mail.gnu.org/archive/html/bug-gnu-emacs/2023-02/msg00760.html

Also, can the clickable link be actually implemented using the `keymap`
property of overlays:
https://www.gnu.org/software/emacs/manual/html_node/elisp/Special-Properties.html#index-keymap-of-character
instead of just displaying the text?

Chinmay

Re: Eglot "inlay hints" landed

[João Távora]

On Fri, Feb 24, 2023 at 4:30 AM Chinmay Dalal
<dalal.chinmay.0101@gmail.com> wrote:

> You can look at the end of any long rust function with hl-line-mode on:
> there's an empty overlay but it's supposed to have a link to the
> beginning of the function. Screenshot attached.
>
> I think the bug is in eglot--update-hints-1:
>
> (if (stringp label) label (plist-get label :value))
>
> It should instead be the value of :value from the _first element_ of
> label, as the spec says a label is a string or a _list of_ `InlayHintLabelPart`s:

Nice catch!  Indeed I missed that.

> Of course, this is not ideal when there are multiple elements in the
> label list (but not strictly wrong either).

Yeah, for now let's stick to the first.  I don't know how to interpret
"multiple hints" for the same "language element", in Emacs at least.
What do other editors do?

> Also, can the clickable link be actually implemented using the `keymap`
> property of overlays:
> https://www.gnu.org/software/emacs/manual/html_node/elisp/Special-Properties.html#index-keymap-of-character
> instead of just displaying the text?

Probably yes.

João

Re: Eglot "inlay hints" landed

[Nikola Pajkovsky]

João Távora <joaotavora@gmail.com> writes:

> On Fri, Feb 24, 2023 at 4:30 AM Chinmay Dalal
> <dalal.chinmay.0101@gmail.com> wrote:
>
>> You can look at the end of any long rust function with hl-line-mode on:
>> there's an empty overlay but it's supposed to have a link to the
>> beginning of the function. Screenshot attached.
>>
>> I think the bug is in eglot--update-hints-1:
>>
>> (if (stringp label) label (plist-get label :value))
>>
>> It should instead be the value of :value from the _first element_ of
>> label, as the spec says a label is a string or a _list of_ `InlayHintLabelPart`s:
>
> Nice catch!  Indeed I missed that.
>
>> Of course, this is not ideal when there are multiple elements in the
>> label list (but not strictly wrong either).
>
> Yeah, for now let's stick to the first.  I don't know how to interpret
> "multiple hints" for the same "language element", in Emacs at least.
> What do other editors do?

What I have done for lsp-mode is that I have joined all labels from
`InlayHintsLabelPart`, and printed out the result.

That works like a charm but probably not the best solution.

>> Also, can the clickable link be actually implemented using the `keymap`
>> property of overlays:
>> https://www.gnu.org/software/emacs/manual/html_node/elisp/Special-Properties.html#index-keymap-of-character
>> instead of just displaying the text?
>
> Probably yes.
>
> João

Re: Eglot "inlay hints" landed

[Johann Klähn]

Works great, thanks! AFAICT ‘(and paddingLeft …)’ in
eglot--update-hints-1 should check for :json-false, though.