Update support for rust-analyzer's inlay hints #3404

rjmac · 2022-03-15T00:34:57Z

I believe that this is no longer actually rust-analyzer-specific and
instead will work with any LSP backend that supports the draft inlay
hint feature, but I have no other backend to try with.

Fixes #3403

I believe that this is no longer actually rust-analyzer-specific and instead will work with any LSP backend that supports the draft inlay hint feature, but I have no other backend to try with. Fixes emacs-lsp#3403

rjmac · 2022-03-15T00:35:28Z

clients/lsp-rust.el

-                   (overlay (make-overlay start end nil 'front-advance 'end-advance)))
+           (-let* (((&rust-analyzer:InlayHint :position :label :kind) hint)
+                   (pos (lsp--position-to-point position))
+                   (overlay (make-overlay pos (+ pos 1) nil 'front-advance 'end-advance)))


I'm not actually sure what that second parameter should be; pos doesn't work, but the position we get doesn't really have an end per se.

My guess would be that pos doesn't work because we set evaporate, which makes Emacs remove the hints when they have length 0. I don't know if that's necessary, since we also remove the overlays manually when we update them. Maybe @yyoncho knows more here

rjmac · 2022-03-15T00:35:52Z

lsp-protocol.el

-(defconst lsp/rust-analyzer-inlay-hint-kind-chaining-hint "ChainingHint")
+(defconst lsp/rust-analyzer-inlay-hint-kind-type-hint 1)
+(defconst lsp/rust-analyzer-inlay-hint-kind-param-hint 2)
+(defconst lsp/rust-analyzer-inlay-hint-kind-chaining-hint nil)


This nil seems like it's a bug in rust-analyzer? It feels like it should be 3 but it definitely isn't on my machine.

Ah, so not a bug but an extension to the draft spec.

Fixed in rust-lang/rust-analyzer#11720

So remove the formatting configuration options for them and the codepath that handled rendering them separately. Also, ask rust-analyzer not to pre-format the hints on its end so that the formatting options on the elisp side are actually useful.

rjmac · 2022-03-21T15:26:17Z

This week's r-a added a bunch of new inlay hint options. I'll take a look at adding support for turning them on to this PR sometime tonight.

* Lifetime elision hints * Reborrow hints * Closure return type hints The way rust-analyzer is evolving, the emacs-side formatting looks like it will become less and less useful, as these new hint types (and indeed the already-existing chaining hint type) are not distinguished in a way that the mode can determine; right now, I've left the support in, but I've disabled it by default in preference to letting the server format them. In that case, the server provides a small amount of formatting info (in particular, whether the inlay should be formatted with padding on the left and/or right).

rjmac · 2022-03-22T04:40:37Z

As I mentioned in that latest commit message, the emacs-side formatting of hints looks like it'll become less useful over time. The new inlay hint protocol doesn't appear to be expressive enough to tell us what these hints are (being restricted to "parameter" and "type") and indeed the new hints once again provide a null kind. What it does do is tell us about what padding should appear.

So anyway, I've undone the change where I asked rust-analyzer to never format the hints itself and made that configurable (defaulting to on). When it is on, the formatting and spacing stuff is all ignored - the code assumes that the label comes back pre-formatted and uses the paddingLeft and paddingRight fields in the response instead of a spacing configuration.

I'm not super happy with it, to be honest, but I thought I'd put it out there and see what other people have to say. My current inclination is to remove the elisp formatting code altogether, always have rust-analyzer format it, and use the kind field only to determine the exact font-face to display the inlay with, but I honestly have no extremely strong opinion about what it should do, and I'm really only a drive-by contributor, so I'll happily take input from anybody (especially regular maintainers!)

rjmac · 2022-03-22T04:45:44Z

But all that said, if you're curious, here are the new lifetime elision, closure return type, and reborrowing hints showing up in Emacs:

yyoncho · 2022-03-22T08:59:25Z

@flodiebold @brotzeit willing to take a look?

lnicola · 2022-03-22T09:02:11Z

The new inlay hint protocol doesn't appear to be expressive enough to tell us what these hints are

Oof, @Veykril can we use parameter/parameter/type for those three? Lifetimes are parameters too.

Veykril · 2022-03-22T10:26:18Z

That sounds wrong to me, those kinds are solely meant for styling, with null falling back to the default styling the user chooses. I'm not sure what the problem here is though, null is a valid kind, the one that applies the default styling?

It's a classic case of the LSP being limited to only a few known things and not allowing custom kinds though which I believe is a serious flaw in the spec :/

flodiebold

LGTM mostly. I agree that letting rust-analyzer format the hints is probably the way forward.

flodiebold · 2022-03-22T12:44:02Z

clients/lsp-rust.el

@@ -342,6 +342,13 @@ PARAMS progress report notification data."
  :group 'lsp-rust-analyzer
  :package-version '(lsp-mode . "6.2"))

+(defcustom lsp-rust-analyzer-server-format-inlay-hints t


in general I'd prefer if the defcustoms mirror the naming of the options in rust-analyzer, but I recognize it's too late here :/

Left that on the wrong line, it was referring to lsp-rust-analyzer-display-lifetime-elision-hints etc.

Maybe we should add defvaralias for those options and update the names to reflect the rust-analyzer options. It would certainly make things less confusing for users.

flodiebold · 2022-03-22T12:44:53Z

clients/lsp-rust.el

                 :chainingHints ,(lsp-json-bool lsp-rust-analyzer-display-chaining-hints)
+                 :closureReturnTypeHints ,(lsp-json-bool lsp-rust-analyzer-display-closure-return-type-hints)
+                 :lifetimeElisionHints ,lsp-rust-analyzer-display-lifetime-elision-hints


I think the structure of this setting might change again soon (right, @Veykril ?), so maybe it's best to just leave it out

I believe you're right, that's rust-lang/rust-analyzer#11778. Hasn't actually happened yet, but I'm aware it almost certainly will.

Yes that will change, I'll do that later today and let you know here again.

I went ahead and changed one of the configs here https://github.com/rust-analyzer/rust-analyzer/pull/11789/files

Note that r-a might have to change a bunch of configs in the future for consistency ( we dug ourselves a bad hole here rust-lang/rust-analyzer#11790 )

flodiebold · 2022-03-22T12:46:12Z

clients/lsp-rust.el

-                   (overlay (make-overlay start end nil 'front-advance 'end-advance)))
+           (-let* (((&rust-analyzer:InlayHint :position :label :kind) hint)
+                   (pos (lsp--position-to-point position))
+                   (overlay (make-overlay pos (+ pos 1) nil 'front-advance 'end-advance)))


My guess would be that pos doesn't work because we set evaporate, which makes Emacs remove the hints when they have length 0. I don't know if that's necessary, since we also remove the overlays manually when we update them. Maybe @yyoncho knows more here

rjmac · 2022-03-22T13:54:27Z

Ah ok, that makes more sense. Looking at it in that light, I think I can make it a fair bit less scattered-feeling today, then I'll consider the PR "done".

Now we always rely on rust-analyzer for spacing info, but allow the formatting within the label proper to be overridden emacs-side.

rjmac · 2022-03-23T06:28:28Z

Ok, I'm pretty happy with the PR as it is now. I incorporated most of the feedback above, specifically:

The new-as-of-today lifetime hint configuration is used
The overlay creation code has been substantially simplified so there's really only one majro path through it. Spacing is now controlled purely by the padding values in the inlay information rust-analyzer returns rather than emacs-side config. I've left in emacs-side config for formatting "type" and "parameter" hints (default disabled, just letting rust-analyzer handle it) but always use the kind to determine which face to lay out the overlays in.
I didn't do the the defvaralias thing mainly because it wasn't obvious to me how it interacted with the customization system.

brotzeit · 2022-03-24T10:23:59Z

I didn't do the the defvaralias thing mainly because it wasn't obvious to me how it interacted with the customization system.

AFAIK it shouldn't make a difference if we don't change :type for the new defcustom. But you don't have to apply it, maybe I'll update the names. I merge this now, thanks for your work.

rksm · 2022-03-24T14:01:14Z

Thank you @rjmac, very much appreciated!

Update support for rust-analyzer's inlay hints

034553a

I believe that this is no longer actually rust-analyzer-specific and instead will work with any LSP backend that supports the draft inlay hint feature, but I have no other backend to try with. Fixes emacs-lsp#3403

github-actions bot added client One or more of lsp-mode language clients documentation rust labels Mar 15, 2022

rjmac commented Mar 15, 2022

View reviewed changes

flodiebold mentioned this pull request Mar 15, 2022

Upstream inlay hints rust-lang/rust-analyzer#11445

Merged

flodiebold approved these changes Mar 22, 2022

View reviewed changes

Robert Macomber added 3 commits March 22, 2022 22:34

Simplify inlay hint formatting

7e80dea

Now we always rely on rust-analyzer for spacing info, but allow the formatting within the label proper to be overridden emacs-side.

Support new configuration for lifetime elision inlay hints

f95fb23

Remove "evaporate" on inlay hints and use pos for the end position

44d1f3e

brotzeit approved these changes Mar 24, 2022

View reviewed changes

brotzeit merged commit 704f9d6 into emacs-lsp:master Mar 24, 2022

This was referenced Apr 8, 2022

Add support for inlay hints from TypeScript language server #3455

Merged

Inlay hints added to wrong buffer when original buffer is not active (lsp-javascript, lsp-rust) #3459

Closed

balloneij mentioned this pull request Apr 24, 2022

Rust: Lost Inlay Hints after Upgrade doomemacs/doomemacs#6329

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update support for rust-analyzer's inlay hints #3404

Update support for rust-analyzer's inlay hints #3404

rjmac commented Mar 15, 2022

rjmac Mar 15, 2022

flodiebold Mar 22, 2022

rjmac Mar 15, 2022

flodiebold Mar 15, 2022

rjmac Mar 15, 2022

lnicola Mar 16, 2022 •

edited

Loading

rjmac commented Mar 21, 2022 •

edited

Loading

rjmac commented Mar 22, 2022

rjmac commented Mar 22, 2022

yyoncho commented Mar 22, 2022

lnicola commented Mar 22, 2022

Veykril commented Mar 22, 2022

flodiebold left a comment

flodiebold Mar 22, 2022

flodiebold Mar 22, 2022

brotzeit Mar 22, 2022

flodiebold Mar 22, 2022

rjmac Mar 22, 2022

Veykril Mar 22, 2022

Veykril Mar 22, 2022

flodiebold Mar 22, 2022

rjmac commented Mar 22, 2022

rjmac commented Mar 23, 2022

brotzeit commented Mar 24, 2022

rksm commented Mar 24, 2022

Update support for rust-analyzer's inlay hints #3404

Update support for rust-analyzer's inlay hints #3404

Conversation

rjmac commented Mar 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lnicola Mar 16, 2022 • edited Loading

Choose a reason for hiding this comment

rjmac commented Mar 21, 2022 • edited Loading

rjmac commented Mar 22, 2022

rjmac commented Mar 22, 2022

yyoncho commented Mar 22, 2022

lnicola commented Mar 22, 2022

Veykril commented Mar 22, 2022

flodiebold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmac commented Mar 22, 2022

rjmac commented Mar 23, 2022

brotzeit commented Mar 24, 2022

rksm commented Mar 24, 2022

lnicola Mar 16, 2022 •

edited

Loading

rjmac commented Mar 21, 2022 •

edited

Loading