-
Notifications
You must be signed in to change notification settings - Fork 410
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
Conversation
2b74737
to
7df18fb
Compare
6fd6463
to
bbc7ac8
Compare
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 Did you consider adding this feature to the RPC instead ? It feels like it would be a more correct way to proceed. |
@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 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 dune/bin/ocaml/ocaml_merlin.ml Line 215 in 7a5395e
which is stored as a dune/bin/ocaml/ocaml_merlin.ml Line 181 in 7a5395e
This is handy because this value can also be used by dune/bin/ocaml/ocaml_merlin.ml Line 186 in 7a5395e
As can be seen, this state handling happens only a few lines apart and doesn't spread to other modules outside If the new commands were moved to Dune's RPC, then we would need to find a way to sync between the two processes (
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 |
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'.)) |
There was a problem hiding this comment.
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.
Ah yes, you are right, unless we move everything to the RPC we cannot easily share the state here. |
@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:
Maybe that would make easier to transition to RPC in the future? We would just need to move the |
ac96149
to
8bb8ca4
Compare
@voodoos I have updated the PR to follow our recent decision to use a The work on ocaml-lsp is still ongoing, but any feedback you might have on this one is already much appreciated. Thanks! |
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. |
@rgrinberg @voodoos Apologies for pinging, I wonder if you have any thoughts on this PR? Thanks. |
There was a problem hiding this 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 justprint-conf
?
bin/ocaml/ocaml_merlin.ml
Outdated
match ctx_name with | ||
| None -> Default | ||
| Some ctx_name -> | ||
(match Context_name.of_string_opt ctx_name with |
There was a problem hiding this comment.
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
bin/ocaml/ocaml_merlin.ml
Outdated
| 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 ] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
@anmonteiro thanks for the review!
I thought the answer to these 2 questions is yes, so just included them in the PR, pls lmk if I'm missing anything :) |
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]>
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]>
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]>
Signed-off-by: Javier Chávarri <[email protected]>
Signed-off-by: Antonio Nuno Monteiro <[email protected]>
1ee8fe2
to
9ddb4c3
Compare
nevermind, I just messed up the rebase. |
…in dump Signed-off-by: Antonio Nuno Monteiro <[email protected]>
I also just pushed a commit that:
|
Thank you, let's do it |
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)
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)
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)
* merlin: add rules regardless of (merlin) Signed-off-by: Javier Chávarri <[email protected]> * merlin: update tests Signed-off-by: Javier Chávarri <[email protected]> * merlin: add GetContexts command Signed-off-by: Javier Chávarri <[email protected]> * merlin: add SetContexts (wip) Signed-off-by: Javier Chávarri <[email protected]> * better tests Signed-off-by: Javier Chávarri <[email protected]> * merlin: simpler get-set-contexts test Signed-off-by: Javier Chávarri <[email protected]> * Revert "merlin: add rules regardless of (merlin)" This reverts commit 3d3c97f. Signed-off-by: Javier Chávarri <[email protected]> * merlin: introduce generate_merlin_rules Signed-off-by: Javier Chávarri <[email protected]> * add docs and changelog Signed-off-by: Javier Chávarri <[email protected]> * merlin: roll back changes in default-based-context test Signed-off-by: Javier Chávarri <[email protected]> * merlin: update get-set-contexts test Signed-off-by: Javier Chávarri <[email protected]> * update changes Signed-off-by: Javier Chávarri <[email protected]> * rename Standard to Default Signed-off-by: Javier Chávarri <[email protected]> * merlin: rename Nothing to Not_selected Signed-off-by: Javier Chávarri <[email protected]> * merlin: fix tests Signed-off-by: Javier Chávarri <[email protected]> * describe: add contexts subcommand Signed-off-by: Javier Chávarri <[email protected]> * merlin: replace get/set context with flag Signed-off-by: Javier Chávarri <[email protected]> * merlin: cleanup Signed-off-by: Javier Chávarri <[email protected]> * cleanup Signed-off-by: Javier Chávarri <[email protected]> * merlin: remove context check Signed-off-by: Javier Chávarri <[email protected]> * apply suggestions from code review Co-authored-by: Antonio Nuno Monteiro <[email protected]> Signed-off-by: Javier Chávarri <[email protected]> * gate generate_merlin_rules to 3.16 Signed-off-by: Javier Chávarri <[email protected]> * merlin: add --context to dump_dot_merlin Signed-off-by: Javier Chávarri <[email protected]> * apply suggestions from code review Co-authored-by: Antonio Nuno Monteiro <[email protected]> Signed-off-by: Javier Chávarri <[email protected]> * merlin: add Select_context.conv Signed-off-by: Javier Chávarri <[email protected]> * fix: promote tests after rebase Signed-off-by: Antonio Nuno Monteiro <[email protected]> * refactor: remove `Selected_context.t`, reuse the context arg, use it in dump Signed-off-by: Antonio Nuno Monteiro <[email protected]> --------- Signed-off-by: Javier Chávarri <[email protected]> Signed-off-by: Antonio Nuno Monteiro <[email protected]> Co-authored-by: Antonio Nuno Monteiro <[email protected]>
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:
--context
todune ocaml-merlin
: if passed, this flag will be used to return data to subsequent requests to theocaml-merlin
serverdune describe contexts
subcommands, which returns the list of Dune contexts available (1 per line)generate_merlin_rules
to the contexts declared in the workspace (originally implemented in merlin: introducegenerate_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 theSetContext
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 fromIt 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 theSetContext
command mentioned above.