Preliminary inlay hint support

[jfeser]

Here's what they look in Emacs:

Outstanding issues:

• Existing type annotations are duplicated. I don't think we can tell whether a type annotation exists from just the typedtree. We could check in the parsetree or do something hacky with the document text to avoid adding these double annotations.
• For function shorthand bindings, the hint sits immediately after the function name. We might want to hint each parameter, like a type annotation.

Do hints like this look useful? Where else might we want to add them?

Closes #685

[rgrinberg]

Existing type annotations are duplicated. I don't think we can tell whether a type annotation exists from just the typedtree. We could check in the parsetree or do something hacky with the document text to avoid adding these double annotations.

That makes a lot of sense.

For function shorthand bindings, the hint sits immediately after the function name. We might want to hint each parameter, like a type annotation.

That would be a big improvement IMO.

Do hints like this look useful?

With the improvements above, they would be very useful.

Where else might we want to add them?

Something that I've seen are annotations on let* bindings (and its cousins let+, and+ etc). When mixing many of these types of operators, it's not very clear the modules from which each individual binding originates. It would be add a hint with the module name.

[jfeser]

The issues with type annotations and functions should be addressed.

Updated screenshot:

[Khady]

This looks nice, thanks for your work on this feature. Is there some plan to also add the return type on functions?

[jfeser]

I'm not sure where to attach function return types. Putting them at the end of the parameter list could be confusing with the parameter hints.

[sidkshatriya]

Thank you for this PR !

This works well for me in my kakoune + kak-lsp editor setup.

Given that OCaml code has relatively fewer explicit type annotations, this feature will be very useful and I hope that this feature is merged to master soon :-) !

[faldor20]

This is handy but I think many users may find it somewhat annoying.
See: ionide/ionide-vscode-fsharp#1693 for a summary of this issue in f#

I believe it's worth considering implementing f# style "line lens" hints as a less intrusive but still very informative option.

They have a few advantages:

• They don't clutter the code you actually want to read
• They don't cause bouncing while typing.

I think that second point is very significant, type hints cause horrible shifting of the text while editing it as the hints come in and out of existence and types change.

Great PR though, it's great to see ocaml's tooling improve so much :)

[sidkshatriya]

Are "line lens" something specific to vscode ? inlay hints will work well across many LSP supporting editors.

Rust analyzer uses inlay hints a lot also BTW and while there are some annoyances it doesn't seem to bother me too much.

[faldor20]

@sidkshatriya They are possible in any editor that supports virtual text, I believe that includes vscode neovim and emacs. https://github.com/fsharp/FsAutoComplete just implements a custom message that returns the location the LineLens should be and its contents.
This is the request for this feature in rust: rust-lang/rust-analyzer#4318
Potentially you don't actually need a separate message and could just work with the inlay hint data but display them differently
Thinking about it further, the fsharp implementation of linelens is much older than inlay hints being a part of lsp, so a specific message is actually likely to be unnecessary.

[sidkshatriya]

@faldor20 Given that these are type annotations, I personally prefer them to appear right after the variable name rather than at the end of the line.

So

let test : int list = List.map f lst

(Here : int list is the inlay hint)

Will be better (in my opinion) than probably something like

let test = List.map f lst (* test : int list *)

(Here (* test : int list *) will be the "line lens" added. It will probably be a inlay hint also, but just at the end of the line)

The issue with this is that for long lines the type information will be far away from the point that you wish to see it. The context will be lost. It will also be more verbose because you need to repeat the variable name again e.g. (* test : int list *)

Alternatively if you / @jfeser wish to work on a proof of concept that is more in line with your suggestion I would love to test it out ! I personally am happy with the feature as-is, to be honest.

[jfeser]

Vscode uses a toggle for inlay hints, which addresses some of the concerns around verbosity. I use a similar toggle in emacs. I don't expect that users will have these hints enabled all the time.

The F# approach is interesting, particularly because it hints more than just identifiers. I do think the F# style makes it less clear which expression the hint refers to.

[sidkshatriya]

Vscode uses a toggle for inlay hints

FWIW kak-lsp (the plugin for kakoune that provides LSP functionality) also has the ability to toggle inlay-hints. You can temporarily toggle them off or disable them altogether.

[voodoos]

Thanks for the nice feature @jfeser !

I made some comments on the iterator which I think could be simplified.

I am also wondering if we should restrict hints to let-bindings and function parameters (right now every pattern variable gets annotated and that adds a lot of noise).

More importantly, since the switch to merlin-lib and the recent release supporting multiple versions of OCaml, we are trying to reduce direct Typedtree usage in the codebase because it introduces tight coupling with a very unstable API of the compiler.

Do you think you could move it upstream to Merlin instead ? There is a bit more boilerplate but I can guide you through it if you want. The main additional task is to create a new command with a serializer that will allow testing the PR in Merlin's testsuite. It's totally understandable if you don't have the time to give it a try, and we can think of solutions with @rgrinberg to give early access to that feature until one of us do the upstreaming.

Like others I am also concerned with the verbosity and would prefer the hints to be disabled by default. A toggle to show the hints only when pressed would be nice. My personal opinion is that they are often redundant, break code formatting (especially if you have fixed column numbers) and generally make the code less readable. I think the OCaml pattern of "relying on type-inference and asking users to carefully name values and annotate the types when it makes sense" is very valuable for readability. Here are some examples in ocaml-lsp's codebase:

Maybe we could introduce ellipsis ... when types are too long ? (But this is probably the responsibility of the plugin, not the server.)

[jfeser]

Thanks for the review!

I am also wondering if we should restrict hints to let-bindings and function parameters (right now every pattern variable gets annotated and that adds a lot of noise).

I had configuration for this at one point. I suspect there isn't a one-size-fits-all choice here. We should look at what other servers do.

Like others I am also concerned with the verbosity and would prefer the hints to be disabled by default. A toggle to show the hints only when pressed would be nice.

I think vscode does exactly this. Maybe someone who uses vscode can confirm.

Maybe we could introduce ellipsis ... when types are too long ?

Maybe there's a way to have the hints expand when hovered over? I'll have to check the spec.

Do you think you could move it upstream to Merlin instead ? There is a bit more boilerplate but I can guide you through it if you want. The main additional task is to create a new command with a serializer that will allow testing the PR in Merlin's testsuite.

I'm not sure I see the value here unless there is a desire to use this feature from non-lsp merlin clients (and in that case I think they should just become lsp clients). I think the testing style we use in e.g. https://github.com/ocaml/ocaml-lsp/blob/019f835a2334a0cd00bd005e3161cf8d85537092/ocaml-lsp-server/test/e2e-new/action_inline.ml is a better fit for this kind of ui-heavy code than merlin's cram testing.

More importantly, since the switch to merlin-lib and the recent release supporting multiple versions of OCaml, we are trying to reduce direct Typedtree usage in the codebase because it introduces tight coupling with a very unstable API of the compiler.

That's fair. Is Browse_tree the merlin feature that corresponds to Typedtree iterators?

[voodoos]

More importantly, since the switch to merlin-lib and the recent release supporting multiple versions of OCaml, we are trying to reduce direct Typedtree usage in the codebase because it introduces tight coupling with a very unstable API of the compiler.

That's fair. Is Browse_tree the merlin feature that corresponds to Typedtree iterators?

Do you think you could move it upstream to Merlin instead ?

I'm not sure I see the value here unless.

Using Browse_raw won't really solve the issue, you still have to manipulate Typedtree nodes at some point. We don't have any usable abstraction for the Typedtree (to be useful it would be almost as unstable), so this issue is the main reason why we would like these to be part of the merlin-lib package (note the -lib part).

I think the testing style we use in e.g. is a better fit for this kind of ui-heavy code than merlin's cram testing.

As far as I understand inlay-hints are not modifying the document, so the natural test method would rather be to print the server's answer and check that it is the one expected as in this test. I am not saying that the tests cannot be written in ocaml-lsp but it certainly also fits merlin-lib's cram workflow.

This is the first PR since ocaml-lsp started using merlin-lib without vendoring it (and have a master branch compatible with 3 different versions of OCaml) so we are still experimenting. Also this PR doesn't break that compatibility so we can merge it before upstreaming it if we want to move faster (wdyt @rgrinberg ?).

In the end we should aim at improving merlin-lib and freeing other projects from the burden of compiler-libs' compatibility by adding new features that require language knowledge there. I see that this adds a lot of friction for PRs that are already targeting ocaml-lsp like this one, but for future features it should not be much more complex if the development is made on merlin-lib first.

[sidkshatriya]

@voodoos wrote:

Do you think you could move it upstream to Merlin instead ?

Inlay hints were introduced in the LSP specification 3.17 ( See https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_inlayHint ).

Strictly speaking, inlay hints are a part of LSP and OCaml's implementation of LSP lives in ocaml-lsp (this project), so I think this is the right part of the stack to implement this feature.

(Of course, this could be implemented in merlin -- but we have this feature built here and now and by the time it gets into merlin a lot more time may pass)

[sidkshatriya]

P.S. As far as enabling or disabling inlay hints, your LSP client should provide a way to disable/or enable them.

My LSP client (kakoune+kak-lsp) allows me to disable/enable inlay hints at any time I want while using the editor, start the editor with them disabled or start the editor with them enabled. In other words, this particular PR should not need to worry about this aspect, I think. (I enjoy inlay hints a lot with rust-analyzer)

[voodoos]

Strictly speaking, inlay hints are a part of LSP and OCaml's implementation of LSP lives in ocaml-lsp (this project), so I think this is the right part of the stack to implement this feature.

Right now, a majority of the features that are "part of LSP" are actually implemented in merlin-lib. The choice is not based on the fact that a feature is "more or less LSP-specific" but on the fact that it manipulates the Typedtree. We decided moving up things relying on the Typedtree to merlin-lib whenever that's possible.

by the time it gets into merlin-lib a lot more time may pass

That doesn't have to be the case if we work together 🙂. Especially for future PRs if they target directly merlin-lib.

[rgrinberg]

Regarding the default behavior:

How about we only annotate the structure items by default so that we match the behavior of the old code lens. That one was just as intrusive and people liked it. For anything more, we can allow it through configuration.

Regarding the LSP/Merlin split:

It appears that we have two separate concerns and we need to be careful to manage them without conflict. On one hand, we want to maintain some control to iterate on this feature - especially since it's not really clear in the short term what users are going to like. On the other hand, we don't want the typed tree to leak into our implementation. So I propose the following:

• We finish the implementation on the LSP side to get a better idea on the behavior that we need from merlin.

• Once this is done, the merlin team can work out an API that we can use to implement this feature without touching the typed tree. As @voodoos has noted, all this feature really needs is an iterator over the patterns in the typed tree and their types that span a location.

• While the merlin team is working out the 2nd point, we want them to take their time and work out a good API. But there's no particular rush as this this feature isn't in anyone's critical path. However, we want to make life easy for users that want to upgrade their merlin fork (with their custom typed tree) and keep using ocamllsp as well. So if we are to merge this feature before merlin releases an API, we need to move it to a separate library and allow ocamllsp to easily be built without it. It's a little more work, but it will be good to do this with all existing features that touch the typed tree anyway.

Regarding tests:

I see two issues with writing most of the tests on the merlin side. The first one is that we'll still need the test on the lsp side to make sure we're speaking the protocol correctly, so if we're not careful, we'll just double the work without really testing anything in addition.

The second issue is that testing this feature by printing json payloads isn't going to cut it - whether it's done through ppx_expect or cram. This feature is complex enough that I would expect the tests to display the document with the inline hints rendered in some way inside the tests. For example, output such as:

let f x $: int $ y $: int $ = ..

Where the $ .. $ denotes the inline hints provided by the server. See our semantic highlighting tests (semantic_hl_tests.ml) or action extract (action_extract.ml) tests for such infrastructure on the lsp side.

If merlin provides us with the typed tree iterator that we need, I imagine that the only tests we'll really need on the merlin side are going to be unit tests of this iterator. I don't know how hard it is to write those, but I imagine it should be less work than full blown cram tests. In addition, I expect less redundancy in the tests this way.

@voodoos @jfeser is this plan acceptable?

[jfeser]

Regarding the default behavior:

How about we only annotate the structure items by default so that we match the behavior of the old code lens. That one was just as intrusive and people liked it. For anything more, we can allow it through configuration.

I think we should provide the option to additionally annotate locals and pattern variables. It might also be worth adding some documentation on server configuration. Many users weren't able to figure out how to reenable the code lenses.

Regarding the LSP/Merlin split:

It appears that we have two separate concerns and we need to be careful to manage them without conflict. On one hand, we want to maintain some control to iterate on this feature - especially since it's not really clear in the short term what users are going to like. On the other hand, we don't want the typed tree to leak into our implementation. So I propose the following:

• We finish the implementation on the LSP side to get a better idea on the behavior that we need from merlin.

• Once this is done, the merlin team can work out an API that we can use to implement this feature without touching the typed tree. As @voodoos has noted, all this feature really needs is an iterator over the patterns in the typed tree and their types that span a location.

There's a few other places where we currently use the typedtree. It would be worth looking at them to see if there are some common patterns that we can extract.

I see two issues with writing most of the tests on the merlin side. The first one is that we'll still need the test on the lsp side to make sure we're speaking the protocol correctly...

The second issue is that testing this feature by printing json payloads isn't going to cut it - whether it's done through ppx_expect or cram. This feature is complex enough that I would expect the tests to display the document with the inline hints rendered in some way inside the tests.

+1 to both.

@voodoos @jfeser is this plan acceptable?

Sure.

[voodoos]

How about we only annotate the structure items by default so that we match the behavior of the old code lens. That one was just as intrusive and people liked it. For anything more, we can allow it through configuration.

I think this would be a bit too restrictive. (As you said, the code-lens does the same and has the advantage of not messing with indentation).

The default option appeared to be on in vscode:

But the onUnlessPressed one looks useful (if it shows the types of enough elements), might be something we can configure as default in the plugin ?

@voodoos @jfeser is this plan acceptable?

Yes it does.

[benjamin-thomas]

Just chiming in from here: https://discuss.ocaml.org/t/ocaml-platform-newsletter-june-2023/12640/2

I agree 100% with @faldor20 here. I remember turning this feature right off when I was learning a bit of rust a few months ago (very annoying/distracting).

But the F# guys got it (the info is still very useful to have)

Here's a couple of screenshots to illustrate:

VScode + Ionide extension:

Rider:

I'm not sure if that's an lsp concern, but please consider pushing that info on the right if you're working on this (or make the position configurable if possible)

[faldor20]

I did some research into how ionide handles these linelens annotations and ended up doing a bit of refactoring on the codebase. We could likely just copy what ionide is doing directly:
ionide/ionide-vscode-fsharp#1895
Or extract the basics out into a little npm library for use in other language client plugins too

[voodoos]

We should also add cases for pattern matching and inline function definition, with the on/off options.

[jfeser]

Yep, planning to.

[rgrinberg]

@jfeser do you think this will be ready soon? I'd like release 1.17.0. If it's not ready, we can just push it back until 1.18.0

[jfeser]

Let me see if I can finish it up by this weekend. If it doesn't happen by then, we can just push it back. Does that work for you?

[EduardoRFS]

Can the inlay hint be placed above bindings instead of the side? I personally disable inlay hints in Rust exactly because they're in the horizontal.

[faldor20]

@EduardoRFS rendering of inlay hints is up to the editor really. However there is codelens which allows some info such as function signatures to be displayed above a line.

If someone was smart they could probably use whitespace inside codelens to make type signatures appear over their definitions, but It'd probably be kind of a mess when types are longer than names

[rgrinberg]

@jfeser do you still plan to finish this up?

We could just merge it as is and disable it until it's ready. At least it will save you some rebasing when you do decide to polish it up.

[jfeser]

@rgrinberg I think this is ready to go.

[coveralls]

Pull Request Test Coverage Report for Build 4103

Details

• -37 of 115 (67.83%) changed or added relevant lines in 4 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+0.6%) to 21.029%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
ocaml-lsp-server/src/range.ml	2	4	50.0%
ocaml-lsp-server/src/inlay_hints.ml	52	65	80.0%
ocaml-lsp-server/src/config_data.ml	22	44	50.0%

Files with Coverage Reduction	New Missed Lines	%
ocaml-lsp-server/src/config_data.ml	1	39.26%

Totals
Change from base Build 4100:	0.6%
Covered Lines:	5029
Relevant Lines:	23915

---

💛 - Coveralls

[rgrinberg]

LGTM