Rls: Support kebab-case in configuration deserialization, report unused configs

[alexheretic]

In ide-rust i let users set their configuration in a rls.toml file. This means it's quite easy to mistype a config, ie not in snake case. This is particularly confusing when .cargo/config uses kebab-case ie target-dir. Rubbing salt in the wound; rls does not log any warnings when unknown/mistyped configs are ignored.

To improve this I propose we:

• [X] Log a warning showing all discarded settings.rust.* config keys.
• [X] Accept/convert kebab-case config keys. _done in #1298_
• [X] Bonus: Accept/convert camelCase config keys. _done in #1298_

Implementation

• For log-warning ignored fields see https://github.com/dtolnay/serde-ignored. Warn with warn!.

Old/done

* Add test(s) duplicating `test_init_with_configuration()` using kebab-case settings _(like `clippy-preference`)_. Also test the _didChangeConfiguration_ path to change config. https://github.com/rust-lang/rls/blob/816017b91b7bb36343f50cbf9d803b8d7970f43c/src/test/mod.rs#L1114-L1115 * See serde renaming https://serde.rs/container-attrs.html#rename_all. Note you can't have two of these so maybe forget about the camel case bonus points. * Simple serde tweaking can be done at `Config` def site https://github.com/rust-lang/rls/blob/816017b91b7bb36343f50cbf9d803b8d7970f43c/src/config.rs#L117-L129 * Maybe not needed, but initial config is actually deserialized at https://github.com/rust-lang/rls/blob/816017b91b7bb36343f50cbf9d803b8d7970f43c/src/server/mod.rs#L98-L108 * _didChangeConfiguration_ at https://github.com/rust-lang/rls/blob/816017b91b7bb36343f50cbf9d803b8d7970f43c/src/actions/notifications.rs#L159

[alexheretic]

I promise not to implement this in 30 seconds time (#1026). @Xanewok @nrc good first issue?

[Xanewok]

I'd say it makes sense. Thank you for writing this up!

[jens1o]

What is kebab-case?

[alexheretic]

What is kebab-case?

It's exactly what you wrote; separating lower case words with -.

[bspeice]

I'm taking a look at this, but #[serde(rename_all)] causes issues because it's not able to accept both formats. It's not quite clear to me how one would go about unifying the keys in a clean format, aside from a much more involved parser for Config. With mixing snake_ and kebab-case, it also doesn't make sense to build an IntoConfig I think.

[alexheretic]

@bspeice thanks for looking at this. So I guess serde won't do this for us.

We could try deserializing first to a dynamic Value (it might already do this as part of languageserver-types), processing through all the config object keys and normalizing case to snake_case, then using from_value to complete the deserialization to Config.

heck could handle the case conversions to snake_case.

[bspeice]

Sounds reasonable. Last question from my side would be whether duplicate keys with different casing should trigger an error? Given how (in)frequently this comes up, I'd guess yes.

[alexheretic]

Good point, I think duplicate different case same keys should log an error but not panic, it'd be annoying to have to restart rls because i added a dodgy line in my rls.toml.

Handling conflicts by ignoring all conflicting keys seems an ok approach to me.

[bspeice]

I lied, that wasn't the last question. The json Value seems to be parsed as part of serde_json::from_value in InitializeRequest. I'm not sure there's a good way to let serde handle most things, and take over for config parsing. Alternately, writing a bit by hand to parse most of the config options, and then delegating to a Config::from_value might make sense, but I'm not sure that impl Deserialize for Config works well.

So, the question: is there actually a good way to approach this?

[alexheretic]

I'd use dynamic style processing the Value directly to normalise the case, then pass on to the struct deserializer.

Here is an example of handling target-dir.

impl BlockingNotificationAction for DidChangeConfiguration {
    fn handle<O: Output>(
        mut params: DidChangeConfigurationParams,
        ctx: &mut InitActionContext,
        out: O,
    ) -> Result<(), ()> {
        trace!("config change: {:?}", params.settings);

        // Handle different configuration cases permissively
        if let Some(config) = params.settings["rust"].as_object_mut() {
            if let Some(kebab_target_dir) = config.remove("target-dir") {
                if config.insert("target_dir".into(), kebab_target_dir).is_some() {
                    log::error!("Multiple different case uses of `target_dir` config");
                    config.remove("target_dir");
                }
            }
        }

So you can work with the config as a &mut Map<String, Value>. All config keys _should_ be snake_case so I think this can be generalised to handle all config keys with a similar amount of code.

[bspeice]

While continuing to work on this, it should be noted that my schedule is super weird with the holidays. I'll update here as I can, but if someone is available to finish it off, please do!

[bspeice]

For future reference, I'm not able to return to this. Apologies for coming on and back off, things got crazy over here.

[alexheretic]

Ok np thanks for the update

[lijinpei]

Hi, alexheretic. Maybe I can work on this problem. I have implemented something like this https://github.com/lijinpei/rls/blob/e7da8ba0c7aa3c62a33feeb3255f4aa3a782b851/src/lsp_data.rs#L399 , but I am a bit confused because I think RLS only accept a few initialization options deserialized in camelCase https://github.com/lijinpei/rls/blob/e7da8ba0c7aa3c62a33feeb3255f4aa3a782b851/src/lsp_data.rs#L260 , any suggestion on further work(besides code formatting and more test cases) ?

[alexheretic]

@lijinpei Yep please do. That looks on the right lines. You just need to finish it off with initial config conversion & tests (I'd just clone test_init_with_configuration and create a single test for case converted config).

The stuff outside of Config is less important, as we don't expect end-users to have to configure them, only client developers.

Raise a pr when you're ready and I can help review the code from there.

[lijinpei]

As for "Log a warning showing all discarded settings.rust.* config keys.", would it be preferred to log a message on the server side, or to send a notification back to the client?

[alexheretic]

I think either would be an improvement. Some clients can view RLS stderr in a console. But it might be nicer to send a warning via LSP so this is more visible for everyone.

[alexheretic]

Closed by #1300 thanks @lijinpei

[lijinpei]

@alexheretic Thank you very much for reviewing my commits, do you have any suggestion on some other work that is needed for rls?

[alexheretic]

do you have any suggestion on some other work that is needed for rls?

I'm not sure I do right now. @Xanewok may have some ideas, we could definitely do with more _good first issues_.