merlin icon indicating copy to clipboard operation
merlin copied to clipboard

Improve Inlay Hints Handling with [@merlin.hide]

Open mlasson opened this issue 1 year ago • 3 comments

Description

Previously, inlay hints logic relied on ghost locations to avoid annotating generated code. But it lead to some inconsistencies because ghost locations are used in non-generated code, for instance:

let f = fun x -> x ^ ""
let g _ = fun x -> x ^ ""

In this case, an inlay hint appears for x in f, but no hint is shown for g.

In VS Code, this results in the following display:
image

This is because

let f _ = ...

is syntactic sugar for:

let f = fun _ -> ...

where fun _ -> introduces a ghost location (the whole fun node).

This is why you needed to disable the check on ghost location in your samples.t test.

Proposed Fix

This PR replaces ghost location handling with the [@merlin.hide] attribute. This allows for more precise control over which parts of the AST should be ignored for inlay hints.

To make this approach work correctly, I refactored the main AST iterator to use Ast_iterators.iter_only_visible. The iterator now ensures attributes are not skipped by recursing on non-immediate sub-terms.

Also, this restructuring simplifies the code and makes inlay hints more consistent across different contexts (e.g., top-level let bindings, class-level let bindings, and expression let bindings).

Additional Changes

  • Updated samples.t with new test cases illustrating the improved inlay hints behavior.
  • Removed support for the avoid-ghost-location option (both in the CLI and protocol).

Related

  • https://github.com/ocaml/merlin/pull/1892 (an early version of this PR)
  • https://github.com/ocaml/ocaml-lsp/pull/1290 : the introduction of the ghost location check
  • https://github.com/ocaml/ocaml-lsp/pull/1159

Screenshot

image

mlasson avatar Feb 05 '25 17:02 mlasson

Is it ok to change the protocol/library in a non-backward compatible way ?

mlasson avatar Feb 05 '25 17:02 mlasson

It took me a while, but I finally understood why top-level let bindings were not annotated even with "hint_let_binding"—this is because code lenses already handle them.

In a future pull request, we could make this behavior configurable with a new option, allowing users to choose between inlay hints and code lenses.

In the meantime, I’ve reverted my change to the structure item iterator in commit 9b69539. I still need to rewrite it slightly to ensure we don’t skip a [@merlin.hide] attribute on value bindings.

mlasson avatar Feb 06 '25 09:02 mlasson

Hi @mlasson, sorry for not having answered earlier, I must have missed it.

Looking at the tests this seems really interesting, thanks for that well though of and tested PR.

Are you happy with it's current state ? Do you think you could rebase it so that we can do a proper review ?

voodoos avatar Oct 03 '25 12:10 voodoos