Add locate_type_multi query - #1951
Conversation
| (library | ||
| (name query_protocol_kernel) | ||
| (public_name merlin-lib.query_protocol_kernel) | ||
| (libraries yojson) | ||
| (preprocess (pps ppx_yojson_conv))) |
There was a problem hiding this comment.
I created this new directory to put useful definitions that are compatible with js_of_ocaml. This is useful for us internally so that a VSCode extension (which is compiled with js_of_ocaml) can depend on the definitions in this directory. Unfortunately query_protocol.ml itself can't be compiled with js_of_ocaml naively due to some of its dependencies.
|
Oh! That's a great feature!
|
|
Wow thanks for the quick resposne!
Sold, I'll change it.
I don't think this is true. |
I generally agree with the analysis, but:
However, my goal is absolutely not to categorically block a (useful) patch, but I think that the cost involved in using the |
|
Regarding the first point, adding Regarding the second, this is something that I hadn't considered so thanks for bringing it up. If relying on a ppx indeed has that implication, I agree this isn't worth it. But I'm mildly surprised that this is an issue. I don't have much context about how OCaml version upgrades are handled, but I would expect the Merlin for the new version to be able to be compiled with the old version, (for example, OxCaml Merlin is currently compiled with 5.2.0) in which case bumping the version used to compile Merlin could simply wait until |
| module Type_tree = struct | ||
| type node_data = | ||
| | Arrow | ||
| | Tuple | ||
| | Object | ||
| | Type_ref of { path : Path.t; ty : Types.type_expr } | ||
|
|
||
| type t = { data : node_data; children : t list } | ||
| end |
There was a problem hiding this comment.
Maybe it is related to VSCode but I am not see the point of splitting repr into two types? (Maybe because I do not see cases when we have children in Type_ref?
There was a problem hiding this comment.
Oh I see, in case of type parameters, in (t, k) r:
ris the typetandkare children?
|
I should also mention that I intend to open a follow-up PR that adds |
|
Thanks for working on locating multiple (related) type definitions. This is hugely needed in ocamllsp. Before discussing the approach for OCaml I want to compare with Let's say you have a type in Rust: let response : Sender<EditorResponse> = ...Let's say in your editor you try to locate the type definition of The response of
I have checked and the standard LSP request [
{
"uri": "file:///home/abczy/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/crossbeam-channel-0.5.14/src/channel.rs",
"range": {
"start": {
"line": 354,
"character": 11
},
"end": {
"line": 354,
"character": 17
}
}
},
{
"uri": "file:///home/abcxyz/kakoune-lsp/src/types.rs",
"range": {
"start": {
"line": 326,
"character": 11
},
"end": {
"line": 326,
"character": 25
}
}
}
]i.e. basically the location of the types Question: Why not follow this behavior of rust-analyzer ? Pardon me if my understanding the discussion on this ticket is wrong but if we follow my suggestion:
This also means that the reply will be understood by editors other than VSCode (e.g. neovim, kakoune etc.) So coming back to OCaml. If you try to navigate to the type of a variable that has the type |
|
Thanks for the thoughts @sidkshatriya! I agree that a nice end-goal would be for the standard
|
|
@liam923 Thanks for your thoughts !
It would be cool if you can allow the current |
|
@sidkshatriya that seems like a good idea. I think it's outside the scope of this PR though as it would require making changes in https://github.com/ocaml/ocaml-lsp. I also have limited familiarity with the LSP, so I'm not sure I'm the best person to make that change. |
We can do it, at Tarides. |
This would be very welcome. A linearized version of this tree output via |
|
I expect a lot of Jane Street users will also be excited about using this toggle. |
|
Sorry for the late answer!
A agree. Merlin is installed in almost any opam switch, so we should keep the package restrictions it adds to a switch to a minimum. Depending on a PPX would be the opposite. |
|
It's still not clear to me that this will actually affect users much in practice. It will only affect users who a) don't use the lsp and b) don't already have this ppx as a dependency. (a) negates any vscode users, and for (b) Also if I'm wrong about the above and this does impact a large amount of switches, I still think this is acceptable. I don't see why we should treat this much different from the LSP's dependencies, as the LSP has the same use case as Merlin. Maybe I'm wrong, but I'd be surprised if there are many people choosing Merlin over the LSP because Merlin is lighter-weight and not some other reason (ease of setup, etc). |
|
Merlin for new version of the compiler is built with the new version of the compiler. Adding a dependency on ppxlib is already not ideal, and I would argue it is not worth the increased depth of the ecosystem bootstrap just to avoid writing a handful of small functions. Moreover, adding a dependency on a |
|
I'm taking one more stab at using PPXes because I had an idea that I think will work well. We can make use of For example, if you write: Dune will correct this to: This allows us to add The only compilation dependency this change now adds is to |
| ; [@@deriving_inline] gets expanded by the `@lint` dune alias. This makes `@lint` get run | ||
| ; when we run the `@runtest` alias, which aides development. | ||
| (alias | ||
| (name runtest) |
There was a problem hiding this comment.
It would be convenient for this to get run as part of the default alias. But there's two things that give me pause:
- I'm not sure if that will make opam try to run the ppx during compilation.
- Maybe this is bad practice since
ppx_yojson_convis awith-testdependency, and this would make that a bit of a lie.
| ("position", mk_position pos) | ||
| ] | ||
| | Locate_type pos -> mk "locate-type" [ ("position", mk_position pos) ] | ||
| | Locate_type_multi pos -> mk "locate-types" [ ("position", mk_position pos) ] |
There was a problem hiding this comment.
WDYT about using Locate_types instead of Locate_type_multi?
|
Hi! |
See my above comment about using |
bd55709 to
6e265b3
Compare
xvw
left a comment
There was a problem hiding this comment.
I just have a small nitpicking but I approve the PR.
Thanks a lot for the feature (and for being understanding about PPXs)
voodoos
left a comment
There was a problem hiding this comment.
Thanks a lot @liam923, that does sounds like a useful feature. Additionally, I suspect the client work for this will also be reusable for when we will finally propose "locate declarationS".
Do you already have such work done on your LSP server / client ?
| | `Builtin (_, s) -> `Builtin s | ||
| | `Not_in_env _ as s -> s | ||
| | `Not_found _ as s -> s | ||
| | `Found { file; location; _ } -> | ||
| `Found (Some file, location.loc_start) | ||
| | `File_not_found result -> `File_not_found result.file |
There was a problem hiding this comment.
Do we need to perform that additional mapping here ? It could be done in one pass when Query_json serializes it, as long as we pass along enough information.
There was a problem hiding this comment.
The reason for this was to make Locate_types_result.t a type that can round trip back from json easily. That way, clients of Merlin can use Locate_types_result.t to represent the parsed result. (We make use of this fact in our internal client implementation.)
| module Locate_types_result = struct | ||
| type node_data = | ||
| | Arrow | ||
| | Tuple | ||
| | Object | ||
| | Poly_variant | ||
| | Type_ref of | ||
| { type_ : string; | ||
| result : | ||
| [ `Found of string option * Lexing.position | ||
| | `Builtin of string | ||
| | `Not_in_env of string | ||
| | `File_not_found of string | ||
| | `Not_found of string * string option ] | ||
| } | ||
|
|
||
| type type_tree = { data : node_data; children : type_tree list } | ||
|
|
||
| type t = Success of type_tree | Invalid_context | ||
| end |
There was a problem hiding this comment.
At first I was a bit sad to see this type repeated in Locate_type, and thought it would be better to parametrize the tree. But after making the change in I am not sure if it's actually "better". WDYT ? (and @xvw ?)
My attempt: voodoos@d7cb643
There was a problem hiding this comment.
Even if it's just a minor detail, I also prefer @voodoos's approach (parametric).
There was a problem hiding this comment.
Thanks, I pushed a change to make this parametric.
| | #Msource.position as pos -> | ||
| run buffer (Query_protocol.Locate_type pos) | ||
| end; | ||
| command "locate-types" |
There was a problem hiding this comment.
This new command should be documented in https://github.com/ocaml/merlin/blob/main/doc/dev/PROTOCOL.md
We have, but we don't think that it's work that we can easily contribute. We didn't make any lsp changes to support this feature. Rather, we made our internal VSCode extension use the call-compatible lsp request to directly call the Merlin query. My understanding is that our extension is substantially different from the OCaml Platform extension, so unfortunately I don't think there's a staightforward way to share those changes. To give some more details of the user experience:
but using an older version of this command that doesn't support polymorphic variants. |
07d1066 to
1f0fc7d
Compare
|
Thanks Liam, I just rebased and moved the changelog entry to the unreleased section |
|
Great, thanks! Is this ready to merge then, or are there other changes I should make? (I don't have the power to click merge.) |
|
We're all good, I was just sleeping for the CI ! |
CHANGES:
Tue Jun 24 17:10:42 CEST 2025
+ merlin binary
- Add `locate-types` command (ocaml/merlin#1951)
+ merlin library
- Fix `merlin_reader` for OpenBSD (ocaml/merlin#1956)
- Improve recovery of mutually recursive definitions (ocaml/merlin#1962, ocaml/merlin#1963, fixes ocaml/merlin#1953)
+ vim plugin
- Fix error when `:MerlinOccurrencesProjectWide` fails to gather code previews (ocaml/merlin#1970)
- Add more short-paths tests cases (ocaml/merlin#1904)
CHANGES:
Sat Oct 04 15:10:42 CEST 2025
+ merlin binary
- Add `locate-types` command (ocaml/merlin#1951)
+ merlin library
- Fix `merlin_reader` for OpenBSD (ocaml/merlin#1956)
- Improve recovery of mutually recursive definitions (ocaml/merlin#1962, ocaml/merlin#1963, fixes ocaml/merlin#1953)
- Support for OCaml 5.4 (ocaml/merlin#1974)
+ vim plugin
- Fix error when `:MerlinOccurrencesProjectWide` fails to gather code previews (ocaml/merlin#1970)
- Add more short-paths tests cases (ocaml/merlin#1904)
CHANGES:
Thu Apr 09 09:59:38 WAT 2026
+ merlin library
- Implement new refactor-extract-region command for extracting region to a fresh let binding (warning: this feature is still experimental) (ocaml/merlin#1948)
- Add "Other" variant to locate-types result (ocaml/merlin#2025)
- Don't include `option` in locate-types result for optional parameters (ocaml/merlin#2027)
- Fix record field autocompletion (ocaml/merlin#2028)
- Signature help should not loop over the parameters once it is finished (ocaml/merlin#2023)
- Fix bugs on signature help about labelled and optional parameters (ocaml/merlin#2032)
- Add `-end-position` parameter for `enclosing` (ocaml/merlin#2029)
- Signature help should appear even if the 'in' is not written (ocaml/merlin#2036)
- Improve type enclosing behavior on various class and object related items
(ocaml/merlin#2053)
+ merlin binary
- Define PATH_MAX to 4096 if undefined (eg. hurd) (ocaml/merlin#2039)
+ test suite
- Add a reproduction case for ocaml/merlin#1214, the issue has been resolved before (ocaml/merlin#2022)
- Add reproduction case for ocaml/merlin#1763 but it is not failing anymore (ocaml/merlin#2021)
- Add a test to reproduce [Locate command fails on multi-line type definitions](ocaml/merlin#1987) (ocaml/merlin#2020)
- Add a regression test for issue ocaml/merlin#2019 (ocaml/merlin#2030)
merlin 5.6.1
Sat Dec 20 11:15:42 CET 2025
+ merlin binary
- Fix a plethora of minor issues with the C code (ocaml/merlin#1998)
+ merlin library
- Signature help should not appear on the function name (ocaml/merlin#1997)
- Fix completion not working for inlined records labels (ocaml/merlin#1978, fixes ocaml/merlin#1977)
- Perform buffer indexing only if the query requires it (ocaml/merlin#1990 and ocaml/merlin#1991)
- Stop unnecessarily forcing substitutions when initializing short-paths graph (ocaml/merlin#1988)
- Fix Mocaml.with_printer didn't update replacement_printer_doc (ocaml/merlin#2010)
+ test suite
- Add a test to ensure the behavior showed in issue ocaml/merlin#1517 is no longer relevant (ocaml/merlin#1995)
- Add a test to ensure the code fragment exhibited in issue ocaml/merlin#1118 no longer makes Merlin crash (ocaml/merlin#1996)
- Add a test case illustrating how a snippet produces two unrelated errors in issue ocaml/merlin#2000. (ocaml/merlin#2003)
- Add a test reproducing issue ocaml/merlin#1983 where `document` command which sometime concatenates consecutive variants and labels (ocaml/merlin#2005)
- Signature-help should trigger on unfinished `let ... in` bindings (ocaml/merlin#2009)
merlin 5.6
Sat Oct 04 15:10:42 CEST 2025
+ merlin binary
- Add `locate-types` command (ocaml/merlin#1951)
+ merlin library
- Implement new refactor-extract-region command for extracting region to a fresh let binding (ocaml/merlin#1948)
- Fix `merlin_reader` for OpenBSD (ocaml/merlin#1956)
- Improve recovery of mutually recursive definitions (ocaml/merlin#1962, ocaml/merlin#1963, fixes ocaml/merlin#1953)
- Support for OCaml 5.4 (ocaml/merlin#1974)
+ vim plugin
- Fix error when `:MerlinOccurrencesProjectWide` fails to gather code previews (ocaml/merlin#1970)
+ test suite
- Add more short-paths tests cases (ocaml/merlin#1904)


This PR adds a new command, locate-type-multi, that we plan to use at Jane Street for some new VSCode functionality.
A useful current command in VSCode is "Go to Type Definition", which uses the
locate-typequery to go to the definition of the type of the identifier under the cursor. However, this command is rather limited - it only works onTconstrs, and when theTconstrcarries arguments, it just ignores the args. In other words, the command doesn't provide the ability to go to a's defintion if the type is any of the following:a t, (a * _), a -> _, _ -> a.locate-type-multi, instead of always just returning the location of a single identifier, returns a tree structure that represents all the identifiers within a type. So for example, if the type isa -> (b * c), it returns the locations of all ofa,b, andc, and this is in a tree-like data structure that makes it easy for VSCode to present this to the user.