Communicate READER to merlin for configured dialects by andreypopp · Pull Request #8567 · ocaml/dune

andreypopp · 2023-09-01T12:45:46Z

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 · 2023-09-01T13:02:56Z

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 · 2023-09-01T13:04:28Z

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

andreypopp · 2023-09-01T13:08:04Z

@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 · 2023-09-01T13:18:38Z

@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 · 2023-09-01T13:20:27Z

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.

src/dune_rules/dialect.ml

andreypopp · 2023-09-01T14:30:38Z

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

andreypopp · 2023-09-01T14:37:09Z

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?

src/dune_rules/dialect.ml

nojb · 2023-09-01T14:44:26Z

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.

src/dune_rules/merlin/merlin.ml

nojb

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

src/dune_rules/dialect.ml

src/dune_rules/merlin/merlin.ml

andreypopp · 2023-09-06T07:52:36Z

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

src/dune_rules/merlin/merlin.ml

voodoos · 2023-09-08T09:52:09Z

src/dune_rules/merlin/merlin.ml

-        Path.Build.Map.find per_module_config file_without_ext
+        match Path.Build.Map.find per_module_config file with
+        | Some _ as s -> s
+        | None -> Path.Build.Map.find per_module_config (strip_pp_extensions file)


Why do you need this fallbacking mechanism ?

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.

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 👍

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

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).

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

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

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 · 2023-09-11T16:12:41Z

@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 · 2023-09-11T16:35:17Z

@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 · 2023-09-11T16:42:59Z

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.

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).

and adding test for promoted source files sounds important too :-)
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 · 2024-03-22T11:37:53Z

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

Signed-off-by: Andrey Popp <8mayday@gmail.com>

Relying on `.ml` extension present is not something we can do, instead store same config with and without extension and on lookup do a fallback with no extension. Signed-off-by: Andrey Popp <8mayday@gmail.com>

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com> Signed-off-by: Andrey Popp <8mayday@gmail.com>

Signed-off-by: Andrey Popp <8mayday@gmail.com>

due to dune 3.16 Signed-off-by: Andrey Popp <8mayday@gmail.com>

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 · 2024-05-01T07:12:55Z

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?

Signed-off-by: Andrey Popp <8mayday@gmail.com>

rgrinberg · 2024-05-04T20:34:59Z

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 · 2024-05-05T00:45:50Z

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

@anmonteiro

CHANGES: ### Added - allow libraries with the same `(name ..)` in projects as long as they don't conflict during resolution (via `enabled_if`). (ocaml/dune#10307, @anmonteiro, @jchavarri) - `dune describe pp` now finds the exact module and the stanza it belongs to, instead of guessing the name of the preprocessed file. (ocaml/dune#10321, @anmonteiro) - Print the result of `dune describe pp` with the respective dialect printer. (ocaml/dune#10322, @anmonteiro) - Add new flag `--context` to `dune ocaml-merlin`, which allows to select a Dune context when requesting Merlin config. Add `dune describe contexts` subcommand. Introduce a field `generate_merlin_rules` for contexts declared in the workspace, that allows to optionally produce Merlin rules for other contexts besides the one selected for Merlin (ocaml/dune#10324, @jchavarri) - melange: add include paths for private library `.cmj` files during JS emission. (ocaml/dune#10416, @anmonteiro) - `dune ocaml-merlin`: communicate additional directives `SOURCE_ROOT`, `UNIT_NAME` (the actual name with wrapping) and `INDEX` with the paths to the index(es). (ocaml/dune#10422, @voodoos) - Add a new alias `@ocaml-index` that uses the `ocaml-index` binary to generate indexes that can be read by tools such as Merlin to provide project-wide references search. (ocaml/dune#10422, @voodoos) - merlin: add optional `(merlin_reader CMD)` construct to `(dialect)` stanza to configure a merlin reader (ocaml/dune#8567, @andreypopp) ### Changed - melange: treat private libraries with `(package ..)` as public libraries, fixing an issue where `import` paths were wrongly emitted. (ocaml/dune#10415, @anmonteiro) - install `.glob` files for Coq theories too (ocaml/dune#10602, @ejgallego) ### Fixed - Don't try to document non-existent libraries in doc-new target (ocaml/dune#10319, fixes ocaml/dune#10056, @jonludlam) - Make `dune-site`'s `load_all` function look for `META` files so that it doesn't fail on empty directories in the plugin directory (ocaml/dune#10458, fixes ocaml/dune#10457, @shym) - Fix incorrect warning for libraries defined inside non-existant directories using `(subdir ..)` and used by executables using `dune-build-info` (ocaml/dune#10525, @rgrinberg) - Don't try to take build lock when running `coq top --no-build` (ocaml/dune#10547, fixes ocaml/dune#7671, @lzy0505) - Make sure to truncate dune's lock file after locking and unlocking so that users cannot observe incorrect pid's (ocaml/dune#10575, @rgrinberg) - mdx: link mdx binary with `byte_complete`. This fixes `(libraries)` with foreign archives on Linux. (ocaml/dune#10586, fixes ocaml/dune#10582, @anmonteiro)

@anmonteiro

CHANGES: ### Added - allow libraries with the same `(name ..)` in projects as long as they don't conflict during resolution (via `enabled_if`). (ocaml/dune#10307, @anmonteiro, @jchavarri) - `dune describe pp` now finds the exact module and the stanza it belongs to, instead of guessing the name of the preprocessed file. (ocaml/dune#10321, @anmonteiro) - Print the result of `dune describe pp` with the respective dialect printer. (ocaml/dune#10322, @anmonteiro) - Add new flag `--context` to `dune ocaml-merlin`, which allows to select a Dune context when requesting Merlin config. Add `dune describe contexts` subcommand. Introduce a field `generate_merlin_rules` for contexts declared in the workspace, that allows to optionally produce Merlin rules for other contexts besides the one selected for Merlin (ocaml/dune#10324, @jchavarri) - melange: add include paths for private library `.cmj` files during JS emission. (ocaml/dune#10416, @anmonteiro) - `dune ocaml-merlin`: communicate additional directives `SOURCE_ROOT`, `UNIT_NAME` (the actual name with wrapping) and `INDEX` with the paths to the index(es). (ocaml/dune#10422, @voodoos) - Add a new alias `@ocaml-index` that uses the `ocaml-index` binary to generate indexes that can be read by tools such as Merlin to provide project-wide references search. (ocaml/dune#10422, @voodoos) - merlin: add optional `(merlin_reader CMD)` construct to `(dialect)` stanza to configure a merlin reader (ocaml/dune#8567, @andreypopp) ### Changed - melange: treat private libraries with `(package ..)` as public libraries, fixing an issue where `import` paths were wrongly emitted. (ocaml/dune#10415, @anmonteiro) - install `.glob` files for Coq theories too (ocaml/dune#10602, @ejgallego) ### Fixed - Don't try to document non-existent libraries in doc-new target (ocaml/dune#10319, fixes ocaml/dune#10056, @jonludlam) - Make `dune-site`'s `load_all` function look for `META` files so that it doesn't fail on empty directories in the plugin directory (ocaml/dune#10458, fixes ocaml/dune#10457, @shym) - Fix incorrect warning for libraries defined inside non-existant directories using `(subdir ..)` and used by executables using `dune-build-info` (ocaml/dune#10525, @rgrinberg) - Don't try to take build lock when running `coq top --no-build` (ocaml/dune#10547, fixes ocaml/dune#7671, @lzy0505) - Make sure to truncate dune's lock file after locking and unlocking so that users cannot observe incorrect pid's (ocaml/dune#10575, @rgrinberg) - mdx: link mdx binary with `byte_complete`. This fixes `(libraries)` with foreign archives on Linux. (ocaml/dune#10586, fixes ocaml/dune#10582, @anmonteiro) - virtual libraries: fix an issue where linking an executable involving several virtual libries would cause an error. (ocaml/dune#10581, fixes ocaml/dune#10460, @rgrinberg)

@anmonteiro

CHANGES: ### Added - allow libraries with the same `(name ..)` in projects as long as they don't conflict during resolution (via `enabled_if`). (ocaml/dune#10307, @anmonteiro, @jchavarri) - `dune describe pp` now finds the exact module and the stanza it belongs to, instead of guessing the name of the preprocessed file. (ocaml/dune#10321, @anmonteiro) - Print the result of `dune describe pp` with the respective dialect printer. (ocaml/dune#10322, @anmonteiro) - Add new flag `--context` to `dune ocaml-merlin`, which allows to select a Dune context when requesting Merlin config. Add `dune describe contexts` subcommand. Introduce a field `generate_merlin_rules` for contexts declared in the workspace, that allows to optionally produce Merlin rules for other contexts besides the one selected for Merlin (ocaml/dune#10324, @jchavarri) - melange: add include paths for private library `.cmj` files during JS emission. (ocaml/dune#10416, @anmonteiro) - `dune ocaml-merlin`: communicate additional directives `SOURCE_ROOT`, `UNIT_NAME` (the actual name with wrapping) and `INDEX` with the paths to the index(es). (ocaml/dune#10422, @voodoos) - Add a new alias `@ocaml-index` that uses the `ocaml-index` binary to generate indexes that can be read by tools such as Merlin to provide project-wide references search. (ocaml/dune#10422, @voodoos) - merlin: add optional `(merlin_reader CMD)` construct to `(dialect)` stanza to configure a merlin reader (ocaml/dune#8567, @andreypopp) ### Changed - melange: treat private libraries with `(package ..)` as public libraries, fixing an issue where `import` paths were wrongly emitted. (ocaml/dune#10415, @anmonteiro) - install `.glob` files for Coq theories too (ocaml/dune#10602, @ejgallego) ### Fixed - Don't try to document non-existent libraries in doc-new target (ocaml/dune#10319, fixes ocaml/dune#10056, @jonludlam) - Make `dune-site`'s `load_all` function look for `META` files so that it doesn't fail on empty directories in the plugin directory (ocaml/dune#10458, fixes ocaml/dune#10457, @shym) - Fix incorrect warning for libraries defined inside non-existant directories using `(subdir ..)` and used by executables using `dune-build-info` (ocaml/dune#10525, @rgrinberg) - Don't try to take build lock when running `coq top --no-build` (ocaml/dune#10547, fixes ocaml/dune#7671, @lzy0505) - Make sure to truncate dune's lock file after locking and unlocking so that users cannot observe incorrect pid's (ocaml/dune#10575, @rgrinberg) - mdx: link mdx binary with `byte_complete`. This fixes `(libraries)` with foreign archives on Linux. (ocaml/dune#10586, fixes ocaml/dune#10582, @anmonteiro) - virtual libraries: fix an issue where linking an executable involving several virtual libries would cause an error. (ocaml/dune#10581, fixes ocaml/dune#10460, @rgrinberg)

* refactor: best effort to track orig module sources This is going to be useful for more granular merlin configs. Signed-off-by: Andrey Popp <8mayday@gmail.com> * refactor: per file merlin configs, communicate merlin reader Signed-off-by: Andrey Popp <8mayday@gmail.com> * test: promote merlin tests Signed-off-by: Andrey Popp <8mayday@gmail.com> * test: add merlin/dialect.t tests Signed-off-by: Andrey Popp <8mayday@gmail.com> * test: add a test focusing on merlin's configuration various granularity levels Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com> Signed-off-by: Andrey Popp <8mayday@gmail.com> Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com> * test: tweak the prev commit (to be squashed) Signed-off-by: Andrey Popp <8mayday@gmail.com> * wip: Module.File.t not to encode orig_path Signed-off-by: Andrey Popp <8mayday@gmail.com> * merlin: lookup fallback with extensionless filename Relying on `.ml` extension present is not something we can do, instead store same config with and without extension and on lookup do a fallback with no extension. Signed-off-by: Andrey Popp <8mayday@gmail.com> * Apply suggestions from code review Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com> Signed-off-by: Andrey Popp <8mayday@gmail.com> * Extract `handle_ml_kind` to the outside scope. Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com> Signed-off-by: Andrey Popp <8mayday@gmail.com> * Use Filename.Extension.t for file extensions Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com> Signed-off-by: Andrey Popp <8mayday@gmail.com> * tests: update merlin/dialect tests Signed-off-by: Andrey Popp <8mayday@gmail.com> * More descriptive naming in Module Signed-off-by: Andrey Popp <8mayday@gmail.com> * promote tests due to dune 3.16 Signed-off-by: Andrey Popp <8mayday@gmail.com> * merlin: test merlin config for promoted modules Signed-off-by: Andrey Popp <8mayday@gmail.com> * doc: describe changes Signed-off-by: Andrey Popp <8mayday@gmail.com> --------- Signed-off-by: Andrey Popp <8mayday@gmail.com> Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com> Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com> Co-authored-by: Ulysse Gérard <thevoodoos@gmail.com> Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

nojb reviewed Sep 1, 2023

View reviewed changes

src/dune_rules/dialect.ml Outdated Show resolved Hide resolved

nojb reviewed Sep 1, 2023

View reviewed changes

src/dune_rules/dialect.ml Outdated Show resolved Hide resolved

src/dune_rules/dialect.ml Outdated Show resolved Hide resolved

nojb reviewed Sep 1, 2023

View reviewed changes