treewide: do not overwrite lsp-client-settings changed by user

[Hi-Angel]

If a user has set a setting inside lsp-client-settings, changing its value will result in a silent bug in user configuration that looks like a regression from lsp-mode mode update. Let's avoid that by adding a new function lsp-register-new-settings that avoids overwritting whatever was set by user and make use of it treewide.

While at it, prohibit lsp-register-custom-settings from internal use for all the same reasons.

Fixes: https://github.com/emacs-lsp/lsp-mode/issues/4420

[Hi-Angel]

upd: added a CI check for this to avoid regressions in the future

[Hi-Angel]

Hmm, apparently github workflows doesn't work well with exclamation sign. Had to quote per advice over at the link, hopefully it will work as expected.

[Hi-Angel]

Yeah, it didn't fail with a "unknown command", so it is working.

[yyoncho]

Thanks for the contribution but I have some concerns. lsp-register-custom-settings is supposed to be replaced by lsp-defcustom which does not have the option to use the new version because it will deviate from vanilla defcustom. This I would rather recommend using something like...

(with-eval-after-load 'lsp-python
   (lsp-register-custom-settings ...

... in your config.

[Hi-Angel]

Thanks for the contribution but I have some concerns. lsp-register-custom-settings is supposed to be replaced by lsp-defcustom which does not have the option to use the new version because it will deviate from vanilla defcustom.

Sorry, I'm a bit confused here… Are you talking about the PR changes or about an alternative way a user may configure lsp-server settings? In the first case I'm unclear what you want me to do. In the latter case, the documentation for lsp-register-custom-settings does not mention lsp-defcusom. I might add a commit with docs on that if you want, but that is a bit orthogonal to the problem the PR solves, because lsp-register-custom-settings has existed for years for user level configuration, so changes like 782e1dc15a5 will be happening and regressing random users.

This I would rather recommend using something like...

(with-eval-after-load 'lsp-python
   (lsp-register-custom-settings ...

... in your config.

Thank you, nice to know there's a workaround.

[yyoncho]

so changes like 782e1dc will be happening and regressing random users.

The point is that when we migrate that file to lsp-defcustom you will still get the same issue unless you use the with-eval-after-load

[Hi-Angel]

Ah, so you want me to fix lsp-defcustom as well, right?

[yyoncho]

No. I mean that lsp-defcustom is supposed to work that way and you cannot fix it because it won't behave as expected. So the only option seems to be to fix your config otherwise we will introduce inconsistencies (lsp-defcustom wont work like defcustom).

[Hi-Angel]

I mean that lsp-defcustom is supposed to work that way

Why? You earlier mentioned that it's supposed to work similarly to defcustom, and the latter does not override whatever was set by user. You can test it yourself: evaluate in emacs (setq foo nil) and then (defcustom foo t "test"), and then check value of foo. It will be nil.

[yyoncho]

I mean that lsp-defcustom is supposed to work that way

Why? You earlier mentioned that it's supposed to work similarly to defcustom, and the latter does not override whatever was set by user. You can test it yourself: evaluate in emacs (setq foo nil) and then (defcustom foo t "test"), and then check value of foo. It will be nil.

If you go to a defcustom form and eval only it - it will override the value.

[Hi-Angel]

I mean that lsp-defcustom is supposed to work that way

Why? You earlier mentioned that it's supposed to work similarly to defcustom, and the latter does not override whatever was set by user. You can test it yourself: evaluate in emacs (setq foo nil) and then (defcustom foo t "test"), and then check value of foo. It will be nil.

If you go to a defcustom form and eval only it - it will override the value.

It doesn't, here's a testcase I just made with a separate file:

λ cat test.el
(setq foo nil)
(defcustom foo t "test")
λ emacs --batch -l test.el --eval "(print foo)"

nil

[Hi-Angel]

As a matter of fact, here's the relevant quote from the defcustom documentation that describes that behavior:

[the default value] evaluated once by ‘defcustom’, and the value is assigned to SYMBOL if the variable is unbound.

IOW, if variable is "bound" aka exists, it does not assign the value to it.

[Hi-Angel]

Any news here?

[Hi-Angel]

From what I read, questions raised seems resolved, so… I imagine it'd be useful to have some solution to the user regressions discussed. I kind of hoped to have this merged faster, before more users are gonna be hit by the Python commit, but another week passed, so the hope becomes lesser. However, we still can make the code future-proof.

[Hi-Angel]

Ping

[Hi-Angel]

Ping 2

[yyoncho]

I mean that lsp-defcustom is supposed to work that way

Why? You earlier mentioned that it's supposed to work similarly to defcustom, and the latter does not override whatever was set by user. You can test it yourself: evaluate in emacs (setq foo nil) and then (defcustom foo t "test"), and then check value of foo. It will be nil.

If you go to a defcustom form and eval only it - it will override the value.

It doesn't, here's a testcase I just made with a separate file:

λ cat test.el
(setq foo nil)
(defcustom foo t "test")
λ emacs --batch -l test.el --eval "(print foo)"

nil

It overrides it if you eval only the defcustom form interactively.

[Hi-Angel]

(setq foo nil)
(defcustom foo t "test")

What version are you testing with? I just did a emacs -Q test.el and manually evaluated first the setq then the defcustom, and foo afterwards is nil.

Also, I don't see how interactive behavior matters in this context.

[Hi-Angel]

Oh, actually wait, I'm not sure why I had different result but now I am re-testing and indeed after iteractive re-evaluation it changes.

But anyway, why does interactive behavior matter here? It is frequent that interactive behavior differ from non-interactive one, and the bug we are facing here is for non-interactive code.