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 rules always, regardless the configuration of (merlin) field #10325

Closed
wants to merge 3 commits into from

Conversation

jchavarri
Copy link
Collaborator

@jchavarri jchavarri commented Mar 28, 2024

This change is related to the introduction of selected context for merlin controlled via ocaml-lsp (see related issue ocamllabs/vscode-ocaml-platform#1432, and ongoing dune work in #10324).

The change consists on always adding merlin rules, regardless the user choice of context through the (merlin) field in dune-workspace.

The reason for this change is that it unlocks dynamically switching the context used for merlin without the need to rebuild again, nor changing the (merlin) value in dune-workspace (which kind of defeats the goal of dynamically switching context).

In theory, the change should not introduce any regressions, besides the expected extra time to create and execute the new rules added, because each context is independent from the others.

It shouldn't change any external behavior from the Dune user point of view either, because Ocaml_merlin isn't reading the information from Context but from the workspace value directly:

(match workspace.merlin_context with

@jchavarri jchavarri force-pushed the merlin/always-add-rules branch 3 times, most recently from b174a61 to 0b4f069 Compare March 28, 2024 10:51
@jchavarri jchavarri marked this pull request as ready for review March 28, 2024 10:52
Signed-off-by: Javier Chávarri <[email protected]>
@rgrinberg
Copy link
Member

It's not great that everyone will have to pay for this performance penalty for this relatively niche feature. What about just adding a setting for enabling/disabling the merlin per context?

@jchavarri
Copy link
Collaborator Author

jchavarri commented Mar 28, 2024

for this performance penalty

It is worth noting that the penalty for users with a single Dune context (which I am guessing should be the majority) will be non existent.

for this relatively niche feature

The feature doesn't exist yet, so it definitely has very few users 😄

What about just adding a setting for enabling/disabling the merlin per context?

You mean some new field in context like generate_merlin_rules? What would happen if user configures some context like:

(context
 (default
  (name foo)
  (generate_merlin_rules false)
  (merlin)))

?

To avoid such inconsistencies, maybe we can set some field in Dune config that applies to all contexts? Similar to sandboxing_preference:

(generate_merlin_rules <setting>)

where <setting> can be one of:
- current: Only generates Merlin rules for the current context selected. This is the default value.
- all: Generates Merlin rules for all contexts.

Edit: I realized later that nothing would happen if (generate_merlin_rules false) is used, it'd be just like if the field wasn't there.

@jchavarri
Copy link
Collaborator Author

Some random metrics to gather usage of context from GitHub open source repos:

  • 263 dune-workspace files found (link)
  • of those, 106 files use context stanza (link)
  • ~26 of those have more than one context defined

@jchavarri
Copy link
Collaborator Author

I also searched for dune-project and there are ~9500 files found. So at least based on this metric from open source OCaml projects hosted on GitHub, less than 0.3% of projects would be affected by the additional work.

@jchavarri
Copy link
Collaborator Author

jchavarri commented Mar 28, 2024

What about just adding a setting for enabling/disabling the merlin per context?

I implemented an interpretation of this suggestion in #10328.

@jchavarri
Copy link
Collaborator Author

jchavarri commented Apr 1, 2024

@rgrinberg Any thoughts on the above comments or the alt approach suggested that was implemented in #10328? Considering how niche the usage of multi-context is, I tend to think introducing this extra work will not have a lot of impact of users (as opposed to adding yet another configuration flag to Dune). cc @anmonteiro

(Apologies for pinging, but I am blocked on either this PR or #10328 in order to continue the work for new commands being done in #10324).

@rgrinberg
Copy link
Member

If we had a way to make cross compilation work without contexts, maybe this would be acceptable. Your usage search omits all the uses of opam-cross-{windows,android}, which use environment variables to setup a cross compilation build environment. Essentially all OCaml cross compilation is done through these repos, so it's pretty important. #10328 is the right way to go.

@rgrinberg rgrinberg closed this Apr 1, 2024
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.

2 participants