Skip to content

Conversation

@liam923
Copy link
Contributor

@liam923 liam923 commented Feb 7, 2025

This PR backports upstream pr ocaml/merlin#1864, which fixes some behavior in the type-enclosing query. By default, type-enclosing returns a list of results, corresponding to nodes enclosing the given position with expanding ranges. For example, if you run type-enclosing on Some (x + 1) with the cursor on x, you will get results corresponding to the expressions x, x + 1, and Some (x + 1).

Printing types is expensive, so Merlin offers a parameter -index to type-enclosing that allows you to only request the nth element of that list. Thus, editors can start by requesting only the innermost result and then progressively increase the range as a user requests to.

But the current implementation has a problem. Merlin has some logic to deduplicate results, but the logic is partially based on the types that are printed. Since Merlin will only print types for the requested index(es), the deduplication is based on what index is requested. This means that the list changes out from under you as you attempt to increase the range. This has caused bad behavior in emacs.

The upstream PR that this backports makes deduplication stable by not relying on printing. As a consequence, results are sometimes duplicated, and it is left to the client (which can maintain state between queries, unlike Merlin) to deduplicate them.

But after this upstream PR, Merlin still deduplicates results based on the ranges of the corresponding nodes. Nodes may sometimes have the same range, and I argue that if two nodes have the same range:

  • If they give the same type, we've already delegated deduplication to clients, so we should make them deduplicate this case too.
  • If they have different types, this is interesting information and we should display it. Based on Merlin's test suite, it seems that the only case where this happens is when there is a type error. In this case, one of the nodes has the actual type of the expression, and the other has the expected type.

So this PR also makes the change such that we never deduplicate results. The short-term consequence of this (until we update clients to perform deduplication) will be that users sometimes see duplicate results. But this seems better than the current state of the world, which is that progressively expanding the range will frequently fail.

I've discussed this with JS lsp folks and they are on board with this change.

Reviewing

  • The first four commits backport PR 1864.
  • The next commit (4a5aed1) promotes tests that changed as a result of backporting that PR.
  • The last commit (e22f3e0) stops deduplication on the basis of location.

voodoos and others added 6 commits February 7, 2025 12:52
Deduplication can only be done if the type have been printed. We perform that printing only at the junction between the reconstructed identifier enclosings and the ones from the tree nodes because we often want to keep both. All other duplicated ranges are removed.
Since deduplciation has been introduced a while ago the results of type-enclosing (the number of enclosings) have been unstable. This cannot be solved easily on the server-side without printing more types which can lead to performance issues when large modules are involed. We now leave the responsability of deduplication to the clients.
@liam923 liam923 requested a review from goldfirere February 7, 2025 19:23
@liam923 liam923 merged commit 350bc14 into main Jun 27, 2025
2 checks passed
@liam923 liam923 deleted the backport-1864 branch June 27, 2025 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants