Improve Inlay Hints Handling with [@merlin.hide]
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:
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.twith new test cases illustrating the improved inlay hints behavior. - Removed support for the
avoid-ghost-locationoption (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
Is it ok to change the protocol/library in a non-backward compatible way ?
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.
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 ?