Search by type feature, a kind of sherlodoc in Merlin

[xvw]

This patch introduces a kind of Sherlodoc in Merlin!

Feature added

Adds to merlin's protocol the search-by-type command, which takes a query (ie: 'a list list -> ’a list) and returns a list of possible candidates in the current environment. The structure of the result is as follows:

{
  "name": "string (the path of the candidate function)",
  "type": "string (the type of value)",
  "cost": "int (the computed cost of the result, for additional sorting on client side)",
  "doc": "string (the docstring of the value)",
  ...
}

The result also include the location (using with_location) of the value (useful to, from a search list, quickly jump to definition or look for usages using project-wide-occurences).

Comparison with polarity-search

For the moment, I find that 100% of experiments give more convincing results with type search (which supports type parameters and functions, unlike polarity search) and in many case, I think that the query is more natural to write. For example:

desired value	Polarity query	By Type query	Comment
int_of_string	-string +int	string -> int	Almost the same query (and almost the same result)
int_of_string_opt	-string +option	string -> int option	Search by type is more precise
List.flatten	-list +list	'a list list -> 'a list	Search by type is more precise

There is also a test-suite that make a comparison for the same expectation between by type and by polarity.

Here is some direct examples:

Search by polarity

Search by type

Difference with Sherlodoc

At present, Sherlodoc is much more intelligent when it comes to indexing, which is not at all within the scope of this PR, which uses the same environment as that used for polarity search. In addition, Sherlodoc is better at comparing names and this PR distinguishes polymorphic variables (but both approach deal relatively properly with equivalence modulo isomorphism). In practice, this distinction is expressed as follows: 'foo list -> ('foo -> 'bar) -> 'bar list is treated by sherlodoc where every variables are Poly and the following PR use indexes (so we can distinguish fst and snd for example).

Client support

Currently, only the suppport for emacs has been added and uses the same output as the polarity search result. The emacs client introduces 2 new commands:

• merlin-search-by-polarity (that replace merlin-search)
• merlin-search-by-type (that perform a searchby type)

And merlin-search was changed to switch between search-by-type or search-by-polarity (depending on the query, so if the query starts by - or +, it arbitrary decide that this is a polarity query).

About the code base

The core of the feature is isolated inside a dedicated sub library : sherlodoc and has no dependencies and just deal with "computing distances between queries and values". The PR also introduce unit-test (using Alcotest) and ocamlformat, currently only available for src/sherldoc and tests/test-units.

Further work

Currently, I think that the PR is an huge improvement, because it leads (IMHO) in every case to a better result set than the polarity search but we can have improvement:

• Support for first-class modules
• dealing with row-polymorphism (object and polymorphic variant)
• dealing with labels
• better client support (that should be part of the implementation of a decent support of OCaml LSP from Emacs and Vim)

But I think we can improve progressively the feature according to user reviews.

More visual examples

Here we can see that 'foo list -> ('foo -> 'bar) -> 'bar list is valid for map: ('a -> 'b) -> 'a list -> 'b list!

Here, we can see that fst and snd are properly ordered!

[xvw]

cc @art-w

[xvw]

The behaviour of the feature is very strange on Windows :/

[xvw]

@voodoos :

• d7094d6 generated the value with holes (to be constructed)
• 5d4125c make the command search-by-type relay on polarity query if the query looks like a polarity-one
• 8deb0fc use a better buffer to display results (allowing search inside the result and generating the expression with holes)

[art-w]

In case it helps others review this PR, the general algorithm implemented is to:

• Traverse the environment to score every value against the query, and filter out values that don't match
• Sort the matching values by their score to output the best results first

The scoring uses an algorithm similar to sherlodoc to compare types, with enhancements by @xvw that I'm going to steal.

• The type ASTs of the query and the tested value are broken into a set of paths from root to leaves: for example int list -> string * 'a becomes the three paths { [Left_arrow; list; int], [Right_arrow; Product (0,2); string], [Right_arrow; Product (1,2); Poly 0 }.
• Then each of the N value type-paths is scored against each of the M query type-paths with levenshtein distance, using rules specific to types to e.g. check that terms have the same polarity to avoid matching an argument with an output. We get an NxM integer matrix out with some cells containing 0 for exact matches, while others may have max_distance to signal strong incompatibility (e.g. if one path is [string] and the other [char]).
• A stable marriage algorithm is then applied to the matrix to select exactly one cell per column and row such that the sum of selected cells is minimal, which correspond to finding how to make the value and query types most similar up to reordering or extra things in the value type (but not in the query type, as that would indicate the value type is missing stuff requested by the user).

While this doesn't attempt to solve the much harder type isomorphism problem, it does rank types by similarity by preferring types that requires the less modifications to match the query, while tolerating user mistakes. (If the previous explanations are incomprehensible, there's an even worst description in French with drawings at around 21:00-23:00 in this presentation)

@xvw added string levenshtein distance to compare type names while sherlodoc only checks for substring match. His normalization of the type ASTs is a really smart and simple way to handle polymorphic variables! He's also much more principled in his scoring, while sherlodoc had to resort to magic constants to produce results in a nice order... I have yet to test this PR to evaluate if the ordering feels right, but tuning the heuristic constants is highly subjective anyway :)

The type scoring algorithm can be very expensive. If you run into performance issues on large environments, sherlodoc has an additional trick that might be useful in a follow-up PR: The values are pre-filtered to remove obviously bad candidates using the simpler polarity_search matching, and pre-sorted with a heuristic ~= name length + type length, as visually short results are preferable. The costly type scoring is then performed on those best candidates, with a cut-off once enough have been scored such that we can assume the remaining untested candidates will have a terrible score and won't make it into the top results shown to users.

Anyway, I'm super hyped by this PR, it will be excellent to not leave my editor while also getting better results specific to my codebase!

[xvw]

Thank you very much for your feedback and for helping us with future reviews!

The scoring uses an algorithm similar to sherlodoc to compare types, with enhancements by @xvw that I'm going to steal.

Of course! When I've got a bit more time, I'll try to make a small pass at at least integrating the polymorphic variable distinction (and potentially try to implement what I've got in mind for unsupported forms of type).

@xvw added string levenshtein distance to compare type names while sherlodoc only checks for substring match. His normalization of the type ASTs is a really smart and simple way to handle polymorphic variables! He's also much more principled in his scoring, while sherlodoc had to resort to magic constants to produce results in a nice order... I have yet to test this PR to evaluate if the ordering feels right, but tuning the heuristic constants is highly subjective anyway :)

I don't know what the best approach is yet, but if I ever get any user feedback complaining about the search by term, I'll try out your approach.

The type scoring algorithm can be very expensive. If you run into performance issues on large environments, sherlodoc has an additional trick that might be useful in a follow-up PR

As this patch is not based on indexing and does not allow a suffix tree to be expressed, as is the case with Sherlodoc, no pre-optimisation phase has been carried out for the time being. If users ever complain about performance, I'll do some experiments with filters, in the environment, by polarity! (Unlike Sherlodoc, which looks at all OPAM packets, here the lookup is only run on the environment, so the number of values is much smaller.)

---

On the whole, I agree with the points you raise! And I'll keep all that in mind when we give our user feedback. For the moment, I think we need to move forward and collect user feedback to refine the heuristics and optimise (avoiding premature optimisations).

[voodoos]

I guess the Ci faisl on Windows because the standard libraries are slightly different.
You could try to make the tests compatible, but don't worry too much about it, we can also simply disable them.

[xvw]

I guess the Ci faisl on Windows because the standard libraries are slightly different. You could try to make the tests compatible, but don't worry too much about it, we can also simply disable them.

Is there a straightforward way to do it (make tests compatible)?

[voodoos]

I guess the Ci faisl on Windows because the standard libraries are slightly different. You could try to make the tests compatible, but don't worry too much about it, we can also simply disable them.

Is there a straightforward way to do it (make tests compatible)?

No. It depends on the test :-) Here I would imagine that you could filter out results that are not in both sides ? But maybe not :-)

[voodoos]

Also, could you update https://github.com/ocaml/merlin/blob/main/doc/dev/PROTOCOL.md and maybe describe the query language here (or in a dedicated wiki page) ?

[xvw]

Also, could you update https://github.com/ocaml/merlin/blob/main/doc/dev/PROTOCOL.md and maybe describe the query language here (or in a dedicated wiki page) ?

Done in e47a488

[voodoos]

I still have to look at the distance calculations but this looks good so far 👍

[voodoos]

I really wonder why lazy tries where used for polarity search...

[xvw]

@voodoos
Comments are adressed here 7cf0b85

Thanks for the first round of the review!

[voodoos]

This looks very good. My only worries are related to performances, I think we should make documentation fetching optional.

[voodoos]

I am wondering about the cost of fetching the doc of all the returned items. It's not a trivial operation. Maybe we should provide an option to the command so that clients can opt-out from that. (I put the comment here but of course it is also valable for the new type-search results

[xvw]

What do you think about the approach proposed here a8688aa

[voodoos]

La CI macos a aussi un problème:

# File "tests/test-dirs/search-by-type-comparison-to-polarity-search.t", line 1, characters 0-0:
# /opt/homebrew/bin/git --no-pager diff --no-index --color=always -u _build/.sandbox/1535e15bb85fec87f6431da9c6ded6e4/default/tests/test-dirs/search-by-type-comparison-to-polarity-search.t _build/.sandbox/1535e15bb85fec87f6431da9c6ded6e4/default/tests/test-dirs/search-by-type-comparison-to-polarity-search.t.corrected
# diff --git a/_build/.sandbox/1535e15bb85fec87f6431da9c6ded6e4/default/tests/test-dirs/search-by-type-comparison-to-polarity-search.t b/_build/.sandbox/1535e15bb85fec87f6431da9c6ded6e4/default/tests/test-dirs/search-by-type-comparison-to-polarity-search.t.corrected
# index dc5aa5f..0ac9c7d 100644
# --- a/_build/.sandbox/1535e15bb85fec87f6431da9c6ded6e4/default/tests/test-dirs/search-by-type-comparison-to-polarity-search.t
# +++ b/_build/.sandbox/1535e15bb85fec87f6431da9c6ded6e4/default/tests/test-dirs/search-by-type-comparison-to-polarity-search.t.corrected
# @@ -37,11 +37,11 @@ potential failures, so lifting the result in an int option).
#      "type": "string -> bool option"
#    }
#    {
# -    "name": "Float.of_string_opt",
# +    "name": "float_of_string_opt",
#      "type": "string -> float option"
#    }
#    {
# -    "name": "float_of_string_opt",
# +    "name": "Float.of_string_opt",
#      "type": "string -> float option"
#    }

[xvw]

Rebased and formatted

[voodoos]

Great work @xvw !

As we discussed, what this feature needs now is user feedback. Let's merge it 🙂