Communicate READER to merlin for configured dialects

[andreypopp]

Communicate READER to merlin for configured dialects.

The user visible change is the (merlin_reader ...) is now allowed in dune-project in (dialect ...) configuration:

(dialect
 (name mlx)
 (implementation
  (extension mlx)
  (merlin_reader mlx)
  (preprocess
   (run ./mlx/pp.exe %{input-file}))))

[rgrinberg]

cc @nojb who is the main author of the dialects feature.

Could you add some tests please? See our merlin tests for how we test the communication with merlin.

[nojb]

Thanks for the ping @rgrinberg. @andreypopp: I am not familiar with the READER feature. Do you mind explaining what it is about? Thanks!

[andreypopp]

@nojb in merlin there's a way to plug a custom "reader" which allows to customise parsing (+ a few other features). The usual way to implement a reader is to use https://github.com/let-def/merlin-extend SDK.

Merlin supports configuring readers through READER directive (which is apparently undocumented) and also have some readers hard coded.

[nojb]

@nojb in merlin there's a way to plug a custom "reader" which allows to customise parsing (a few other features). The usually way to implement reader is to use https://github.com/let-def/merlin-extend SDK.

Thanks. Do I understand correctly that a reader is an executable? In any case, it seems like a coherent addition to the (dialect) stanza.

[andreypopp]

Thanks. Do I understand correctly that a reader is an executable?

The reader itself is an executable, yes, the configuration though specifies just a reader's NAME, then merlin seaches for ocamlmerlin-NAME in $PATH.

[andreypopp]

Ok, added a test case, fixed ocaml merlin dump-config subcommand and added versioning.

[andreypopp]

Actually I'm wondering if we should allow to define a reader per dialect or per dialect * ml_kind?

I've made it per dialect * ml_kind as merlin maps extensions to readers but, in practice I think, both implementations and interfaces will be handled by a single executable so maybe we should move this stanza up to be per dialect? What do you think?

[nojb]

What do you think?

I think the current approach (dialect * ml_kind) gives a bit more flexibility and stays closer to the interface exposed by Merlin, so I would leave it as it is.

[nojb]

I didn't looks in full detail, but generally speaking this LGTM

[andreypopp]

Made the tests pass. Let me know if there's any more I need to fix/tweak before this can be merged.

[voodoos]

Why do you need this fallbacking mechanism ?

[andreypopp]

Modules with pp extensions module_name.xxx.xx.ext can be also valid modules residing in source directories (e.g. extra extension were not added by a preprocessor but present in source files). In this case it makes sense to try to resolve using non normalised filename first, I think.

[voodoos]

Do you have a test illustrating that ? In any case, I would like to take some time next week to pull your changes and understand better the relationship between source file / module / merlin config. It's a set of conventions that would benefit a lot from being correctly defined and documented.

The PR looks good otherwise 👍

[andreypopp]

No, don't have a test case...

1. I'm actually wondering if we should remove any normalisation now that we have per filename config (rather per module) and such config is built by looking at source modules (so we see "real" source files).

2. Another thing to check (add test) is we still support promoted files.

For reference the behaviour of stripping the extension was added in c36df0b

[voodoos]

Modules with pp extensions module_name.xxx.xx.ext can be also valid modules residing in source directories (e.g. extra extension were not added by a preprocessor but present in source files). In this case it makes sense to try to resolve using non normalised filename first, I think.

Added a test for that in my commit (at the end of the file), and it doesn't work right now.

Another thing to check (add test) is we still support promoted files.
That's a good idea, can you do it ? (you can reuse the merlin_config.sh script in my new test to easily query the configuration.

[rgrinberg]

@voodoos are you happy with this PR being merged to be available in 3.11? Or do you want to take another look and have it for 3.12?

[voodoos]

@voodoos are you happy with this PR being merged to be available in 3.11? Or do you want to take another look and have it for 3.12?

It's probably all right but I would like to do a last pass and add a few test cases.
What is the schedule for 3.11 ? (I can do that tomorrow)

[rgrinberg]

I think @emillon wants it out by end of week.

[voodoos]

I pushed a commit with a new test that focuses on Merlin's config's levels of granularity.

Most standard use cases looks good. There is one change of behavior compared to before: source files with a custom extension that changed after preprocessing are not handled anymore. For example:

Given a file, and a preprocessing action:
modname.something.cml --pp--> modname.ml

One could expect to get the configuration for the module modname when running merlin on the modname.something.cml.

Before that PR the granularity was at the module level and a simple heuristic was used to match a source-file with a module: split the filename by ., the first segment must be the module name.

Now the granularity is finer, and there is no way, given the source file modname.something.cml to guess that the appropriate config is stored under the key modname.ml. The new heuristic is: split the filename by ., the first segment must be the module name, the last segment must be the extension.

1. I agree that the correct thing to do, as you suggested, @andreypopp, is to use real source-file names as keys:

I'm actually wondering if we should remove any normalization now that we have per filename config (rather per module) and such config is built by looking at source modules (so we see "real" source files).

2. and adding test for promoted source files sounds important too :-)

3. Additionally, the currently provided suffixes for melange are mlx mlx. Isn't there a different suffix for signatures ? If mli are to be used, then I think the list should be mlx mli for Merlin jump to declaration to work as expected.

[dmmulroy]

Are there any updates on this merging? I would love to start seriously utilizing, advocating, and teaching mlx

outdated review, items have been addressed

[anmonteiro]

This looks good to me now.

@andreypopp feel free to tag me on the next PR about mlx/mlx (merlin suffix) too.

[andreypopp]

Thanks @anmonteiro!

I've added another test case for promoted modules.

@rgrinberg @voodoos please let me know if anything else should be done with this PR or we can merge it?

[rgrinberg]

No issues from me - though I haven't looked at it closely. @anmonteiro feel free to merge if you're satisfied and if the CI failures aren't related.

[anmonteiro]

Let's do this. Thanks for your continued work on this, @andreypopp