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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Hi-Angel
Copy link
Contributor

@Hi-Angel Hi-Angel commented Apr 8, 2024

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: #4420

@github-actions github-actions bot added rust client One or more of lsp-mode language clients labels Apr 8, 2024
@Hi-Angel Hi-Angel force-pushed the dont-overwrite-user-settings branch 2 times, most recently from 42b0e44 to 9c0905c Compare April 8, 2024 20:47
@Hi-Angel
Copy link
Contributor Author

Hi-Angel commented Apr 8, 2024

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

@Hi-Angel Hi-Angel force-pushed the dont-overwrite-user-settings branch from 42fdebe to 3df350a Compare April 8, 2024 20:57
@github-actions github-actions bot added the CI label Apr 8, 2024
@Hi-Angel Hi-Angel force-pushed the dont-overwrite-user-settings branch 2 times, most recently from 3df350a to c048a7f Compare April 8, 2024 21:00
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: emacs-lsp#4420
@Hi-Angel Hi-Angel force-pushed the dont-overwrite-user-settings branch from c048a7f to 03d89dd Compare April 8, 2024 21:03
@Hi-Angel
Copy link
Contributor Author

Hi-Angel commented Apr 8, 2024

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
Copy link
Contributor Author

Hi-Angel commented Apr 8, 2024

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

@yyoncho
Copy link
Member

yyoncho commented Apr 11, 2024

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
Copy link
Contributor Author

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 782e1dc 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
Copy link
Member

yyoncho commented Apr 11, 2024

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
Copy link
Contributor Author

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

@yyoncho
Copy link
Member

yyoncho commented Apr 11, 2024

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
Copy link
Contributor Author

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
Copy link
Member

yyoncho commented Apr 11, 2024

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
Copy link
Contributor Author

Hi-Angel commented Apr 11, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Any news here?

@Hi-Angel
Copy link
Contributor Author

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
Copy link
Contributor Author

Hi-Angel commented Jun 6, 2024

Ping

@Hi-Angel
Copy link
Contributor Author

Hi-Angel commented Jun 9, 2024

Ping 2

@yyoncho
Copy link
Member

yyoncho commented Jun 9, 2024

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
Copy link
Contributor Author

Hi-Angel commented Jun 9, 2024

(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
Copy link
Contributor Author

Hi-Angel commented Jun 9, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI client One or more of lsp-mode language clients rust
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Random regressions due to lsp overriding user settings via lsp-register-custom-settings
2 participants