Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

merlin: add new contexts commands #10324

Merged
merged 27 commits into from
May 6, 2024

Conversation

jchavarri
Copy link
Collaborator

@jchavarri jchavarri commented Mar 28, 2024

Fixes #10222.

This work is part of the multi-context support for vscode (see ocamllabs/vscode-ocaml-platform#1432).

Current version

This PR prepares Dune to support multi-context config retrieval from Merlin by doing the following:

  • Add an optional flag --context to dune ocaml-merlin: if passed, this flag will be used to return data to subsequent requests to the ocaml-merlin server
  • Add dune describe contexts subcommands, which returns the list of Dune contexts available (1 per line)
  • It also adds a new field generate_merlin_rules to the contexts declared in the workspace (originally implemented in merlin: introduce generate_merlin_rules #10328). This new field allows to generate merlin rules in any case, even if the context is not selected using (merlin). This addition is useful to dynamically change the context for Merlin via the SetContext command mentioned above.

Original version (now obsolete)

This PR adds two new commands to the ocaml-merlin API:

  • GetContexts: returns the list of contexts available in the workspace (only ctxt names)
  • SetContext(name): sets the current context to get merlin data from

It also adds a new field generate_merlin_rules to the contexts declared in the workspace (originally implemented in #10328). This new field allows to generate merlin rules in any case, even if the context is not selected using (merlin). This addition is useful to dynamically change the context for Merlin via the SetContext command mentioned above.

@voodoos
Copy link
Collaborator

voodoos commented Apr 3, 2024

I am wondering whether adding more feature to the "configuration server" is the correct way to proceed. This ad-hoc server was implemented before the landing of the more general Dune's RPC feature, and one future goal at some point was to move the ad-hoc configuration server logic to the RPC. Adding new features to dune ocaml-merlin would only make a transition to the RPC more complex.

Did you consider adding this feature to the RPC instead ? It feels like it would be a more correct way to proceed.
There would be one drawback however: only lsp-based editor plugins will be able to rely on it since, as of today, the old Merlin server does not speak with Dune's RPC. Since we might switch every plugins to using lsp in the future, and you seem to mostly target vscode, that might not be an issue ?

@jchavarri
Copy link
Collaborator Author

@voodoos Thanks for the feedback. I wasn't aware of how ocaml-lsp and Dune communicated through Dune's RPC feature, I understand you are referring to this Dune module?

I am not sure I understand how the suggested approach would work in terms of storing the information about the selected context. In the current implementation, the SetContext command will change the value of the current context:

| true -> selected_context := Custom ctxt_name)

which is stored as a ref that is initialized as soon as dune ocaml-merlin starts:

let selected_context = ref Commands.Default in

This is handy because this value can also be used by dune ocaml-merlin where the editor information is requested:

let* () = print_merlin_conf ~selected_context:!selected_context path in

As can be seen, this state handling happens only a few lines apart and doesn't spread to other modules outside Ocaml_merlin.

If the new commands were moved to Dune's RPC, then we would need to find a way to sync between the two processes (rpc and ocaml-merlin), because we would be writing the state through Dune's RPC, but we would still have to read it from the ocaml-merlin process. Or is there an alternative to avoid that?

Adding new features to dune ocaml-merlin would only make a transition to the RPC more complex.

Considering the above, I think this addition doesn't change much the current state of things, and moving all the Merlin configuration handling and state at once to the RPC will be easier than spreading state across processes. I would be happy to eventually work on transitioning everything to the RPC if/when time permits, but that sounds like a much larger task and scope than the current goal: to allow users to select the context used by print_merlin_conf.

@jchavarri
Copy link
Collaborator Author

Since we might switch every plugins to using lsp in the future, and you seem to mostly target vscode, that might not be an issue ?

Unfortunately, I would need to support users that don't use vscode / ocaml-lsp and query merlin directly via vim or emacs.

Request config for file in alt context before calling SetContext

$ printf '(4:File%d:%s)' ${#FILE2} $FILE2 | dune ocaml-merlin | grep -i "$lib2" | sed 's/^[^:]*:[^:]*://'
No config found for file bar.ml. Try calling 'dune build'.))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should improve the error messages now that the user might be using the wrong context since dune build won't solve that issue.

@voodoos
Copy link
Collaborator

voodoos commented Apr 4, 2024

I am not sure I understand how the suggested approach would work in terms of storing the information about the selected context.

Ah yes, you are right, unless we move everything to the RPC we cannot easily share the state here.

@jchavarri
Copy link
Collaborator Author

@voodoos Thanks for the review. I wonder what your thoughts are about the suggestion in ocamllabs/vscode-ocaml-platform#1432 (comment)? In particular, this part:

add --context CONTEXT option dune ocaml ocaml-merlin

Maybe that would make easier to transition to RPC in the future? We would just need to move the --context flag from the dune ocaml-merlin executable to the RPC one. Besides that, I find interesting about the suggestion that it removes the need to patch dot-merlin-reader, which I was preparing in ocaml/merlin#1743.

@jchavarri
Copy link
Collaborator Author

@voodoos I have updated the PR to follow our recent decision to use a --context flag rather than a new protocol command to set the context. I also added a new subcommand dune describe contexts to list the contexts, which I will use from ocaml-lsp later on to offer clients the list of contexts available (to show in e.g. vscode a menu to choose from).

The work on ocaml-lsp is still ongoing, but any feedback you might have on this one is already much appreciated. Thanks!

@jchavarri jchavarri requested a review from voodoos April 10, 2024 13:11
bin/ocaml/ocaml_merlin.ml Outdated Show resolved Hide resolved
@jchavarri
Copy link
Collaborator Author

The downstream PRs that depend on this feature are ready for review:

The latter includes a short screen capture that helps visualize what the features added in this PR enable, from the user perspective.

@jchavarri
Copy link
Collaborator Author

@rgrinberg @voodoos Apologies for pinging, I wonder if you have any thoughts on this PR? Thanks.

Copy link
Collaborator

@anmonteiro anmonteiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good overall and my suggestions are mostly minor. The important ones are:

  • should we gate this on lang dune 3.16?
  • should we make the other commands in dune ocaml-merlin take --context too, not just print-conf?

bin/describe/describe_contexts.ml Outdated Show resolved Hide resolved
match ctx_name with
| None -> Default
| Some ctx_name ->
(match Context_name.of_string_opt ctx_name with
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering what the valid names for contexts were and incidentally found a crash: #10472

| None -> Default
| Some ctx_name ->
(match Context_name.of_string_opt ctx_name with
| None -> User_error.raise [ Pp.textf "Invalid context name %S" ctx_name ]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could provide a hint (maybe it could even live in context_name.ml) about what's a valid context name here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i agree. maybe this can be done in a separate PR (worth including a fix for your repro in #10472 as well).

bin/ocaml/ocaml_merlin.ml Outdated Show resolved Hide resolved
doc/changes/10324.md Outdated Show resolved Hide resolved
doc/reference/dune-workspace/context.rst Outdated Show resolved Hide resolved
src/dune_rules/workspace.ml Outdated Show resolved Hide resolved
@jchavarri
Copy link
Collaborator Author

jchavarri commented May 2, 2024

@anmonteiro thanks for the review!

should we gate this on lang dune 3.16?
should we make the other commands in dune ocaml-merlin take --context too, not just print-conf?

I thought the answer to these 2 questions is yes, so just included them in the PR, pls lmk if I'm missing anything :)

jchavarri and others added 18 commits May 5, 2024 17:05
Signed-off-by: Javier Chávarri <[email protected]>
Signed-off-by: Javier Chávarri <[email protected]>
Signed-off-by: Javier Chávarri <[email protected]>
Signed-off-by: Javier Chávarri <[email protected]>
Signed-off-by: Javier Chávarri <[email protected]>
Signed-off-by: Javier Chávarri <[email protected]>
Signed-off-by: Javier Chávarri <[email protected]>
Signed-off-by: Javier Chávarri <[email protected]>
Co-authored-by: Antonio Nuno Monteiro <[email protected]>
Signed-off-by: Javier Chávarri <[email protected]>
Co-authored-by: Antonio Nuno Monteiro <[email protected]>
Signed-off-by: Javier Chávarri <[email protected]>
Signed-off-by: Javier Chávarri <[email protected]>
Signed-off-by: Antonio Nuno Monteiro <[email protected]>
@anmonteiro
Copy link
Collaborator

nevermind, I just messed up the rebase.

@anmonteiro
Copy link
Collaborator

I also just pushed a commit that:

  • removes Selected_context.t (Context_name.t and associated functions have all we need)
  • add the --context arg to dump too
  • reuse the context arg as Selected_context.arg

@anmonteiro anmonteiro merged commit 0471f45 into ocaml:main May 6, 2024
26 of 27 checks passed
@anmonteiro
Copy link
Collaborator

Thank you, let's do it

@jchavarri jchavarri deleted the merlin-add-contxts-cmds branch May 6, 2024 05:50
gridbugs added a commit to gridbugs/opam-repository that referenced this pull request Jun 5, 2024
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)
emillon added a commit to emillon/opam-repository that referenced this pull request Jun 17, 2024
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)
avsm pushed a commit to avsm/opam-repository that referenced this pull request Sep 5, 2024
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support "multi-context" libraries
3 participants