Language configs are not merged between user-wide and project-specific files

[poliorcetics]

Summary

I have a .helix/languages.toml in a project. Since a few days ago, I my LSP config has been wonky. I found out looking at the logs it's because the language.config key is not merged when present in both .helix/languages.toml and ~/.config/helix/languages.toml, only the first is used.

Reproduction Steps

I tried this (using current master, 1619766):

1. Have rust-analyzer installed in a recent version, and rust too
2. mv ~/.config/helix/languages.toml ~/.config/helix/languages.toml.save || true
3. cd /tmp
4. cargo new repro-helix
5. cd new repro-helix
6. echo 'fn main() { println!("Hello, world"); }\n#[cfg(feature = "non-existent-feature")]\nfn feature_func() {}' > src/main.rs
7. echo '[[language]]\nname = "rust"\n[language.config]\ncheckOnSave.command = "clippy"\ndiagnostics.disabled = [ "inactive-code", "inactive_code" ]' > ~/.config/helix/languages.toml
8. hx src/main.rs
9. You should not see any diagnostic for the featured out code.
10. Quit helix :q
11. mkdir .helix
12. echo '[[language]]\nname = "rust"\n[language.config]' > .helix/languages.toml
13. hx src/main.rs
14. You will see diagnostics for the featured out code, you should not
15. Quit helix `:q'
16. mv ~/.config/helix/languages.toml.save ~/.config/helix/languages.toml || true

I expected this to happen:

The configs in ~/.config/helix/languages.toml and .helix/languages.toml are merged, with the second taking precedence over the user-wide config.

Instead, this happened:

The project-specific config completely erases the user-wide config.

Helix log

Config passed to LSP without .helix/languages.toml

2022-09-05T09:37:58.571 helix_lsp::client [INFO] Using custom LSP config: {"checkOnSave":{"command":"clippy"},"diagnostics":{"disabled":["inactive-code","inactive_code"]}}

Config passed to LSP with .helix/languages.toml

2022-09-05T09:39:56.591 helix_lsp::client [INFO] Using custom LSP config: {}

Platform

macOS

Terminal Emulator

Kitty 0.26.1

Helix Version

helix master (1619766)

[poliorcetics]

Temporary workaround for people affected: copy ~/.config/helix/languages.toml to .helix/languages/toml and make the wanted modifs

[poliorcetics]

Research:

This was most likely caused by 235237d. Sadly this commit is correct for the specific comment it added and wrong for people wanting anything else.

I can make a PR reverting it but then the example use case in the commit would break again. Adding a config option for the depth is possible but it exposes an obscure implementation detail to users that will be difficult to explain properly and will still be very imperfect since a paramater could be meant to be an overwrite at the same depth as an other meant to be a merge.

We could do it on a language-basis, either fully overwriting from level 3 onwards or merging (setting the merge level to something very high).

[MarijnS95]

It appears this is still an issue with the new [language-server] config (that I just migrated to, to validate that this problem is still an issue on the latest git version: hx --version returns helix 23.05 (68fce3e1)).

I have the following user config in ~/.config/helix/languages.toml:

[language-server.rust-analyzer.config]
check.command = "clippy"

And all projects show clippy lints. According to the logs:

22812:2023-10-06T11:55:54.303 helix_lsp::client [INFO] Using custom LSP config: {"check":{"command":"clippy"}}

However, in one specific project I want clippy to cross-compile via MSVC so that I can also work on cfg(windows) code (with everything rust-analyzer provides there), so I've added the following in a project-specific .helix/languages.toml file.

[language-server.rust-analyzer.config.cargo]
target = "x86_64-pc-windows-msvc"
extraArgs = ["--target-dir", "target/msvc"]

According to the logs:

19661:2023-10-06T11:55:17.699 helix_lsp::client [INFO] Using custom LSP config: {"cargo":{"extraArgs":["--target-dir","target/msvc"],"target":"x86_64-pc-windows-msvc"}}

Note that "check":{"command":"clippy"} has disappeared from the config object. Copy-pasting the aforementioned user config into the project-specific .helix/languages.toml file gets it back:

16373:2023-10-06T11:54:46.283 helix_lsp::client [INFO] Using custom LSP config: {"cargo":{"extraArgs":["--target-dir","target/msvc"],"target":"x86_64-pc-windows-msvc"},"check":{"command":"clippy"}}

As per the original issue report, these should be properly merged?

[pascalkuthe]

I actually think its working as intended. Languages Server config is a single configuration item that is always fully overwritten.

OItherwise it would not be possible to remove config options. That is ok for our normal config because there is always a sensible default but for LSPs we dont control the config and options may conflict.

For example if you had an array of enabled features in your global config [foo, bar] and wanted to overeirte them to [foo1, bar1] in your local config you wouldn't be able to do that anymore (you would endup with a merged array [foo, bar, foo1, bar1]). Similar concerns apply to dicts which may contain cnflicting options or could be intended as a mapping.

So I don't think we would want to actually change behaviour here

[MarijnS95]

@pascalkuthe but that is not what the current documentation states. It does not make an exception for LSP .config nor justification for this choice:

helix/book/src/languages.md

Lines 29 to 32 in 68fce3e

	3. In a `.helix` folder in your project. Language configuration may also be
	overridden local to a project by creating a `languages.toml` file in a
	`.helix` folder. Its settings will be merged with the language configuration
	in the configuration directory and the built-in configuration.

[poliorcetics]

I actually think its working as intended. Languages Server config is a single configuration item that is always fully overwritten.

That was not the case when LS config was introduced. Additionally, you are saying that overriding a single value in the config means having to fully copy the LSP config and then maintaining 2 ? If I have another project using the same LSP with a third config I'm supposed to maintain 3, then 4, 5, ... that's not scalable at all.

We could instead allow overriding by using the same thing as themes, with an "inherit from root" boolean that is false by default for example. How do other editors solve this ?

[the-mikedavis]

That looks like a bug in the docs rather than in the way we merge config options.

Ideally this would be solved by scriptable configs - you could decide whether to merge with- or ignore the global config in the local config. The local config could be a function of the global config. Without that, there's really no winning. With the current behavior you need to add the full configuration everywhere but with the merging behavior that you're proposing there is no way to remove config which is specified globally. For example if you set

[language-server.rust-analyzer.config]
check.command = "clippy"

in your global config, it's impossible to unset check.command in any local configurations. An option to choose whether to inherit or not would work too but I'd prefer to wait for scriptable configs - the TOML config will be removed eventually so it not being scalable is acceptable IMO.

That was not the case when LS config was introduced

When local language config merging was introduced it was quite buggy since it was recursive with no depth limit, so all arrays would be appended to each other instead of cleared (#1000). Since then it went through a few changes but it's now limited to a merge depth of 3 (#3080) so that we have a way to remove items from the config.

[pascalkuthe]

There isn't also really much prior art here. Vim/nvim font have per project configuration and their config are scripts so it's all Imperative (if you want to merge co figs you have to call a function to do that).

VSCode has all its language support as language specific plug-ins which provides custom lgoi. To handle all configuration and then forward the final merged config to the LSP. That is not feasible for us. We can't teat LSP config as anything but an opaque blob

[pascalkuthe]

closing in favor of #10389