Skip to content

Commit

Permalink
treewide: do not overwrite lsp-client-settings changed by user
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Hi-Angel committed Apr 8, 2024
1 parent 5b01984 commit 42fdebe
Show file tree
Hide file tree
Showing 36 changed files with 68 additions and 39 deletions.
3 changes: 3 additions & 0 deletions .github/workflows/lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ jobs:

- name: Run Indentation Lint
run: |
# `lsp-register-custom-settings` should not be used for defining client
# settings, see its description for details
! git grep lsp-register-custom-settings -- clients
git diff-tree --no-commit-id --name-only -r ${{ github.sha }} \
| grep \.el | grep -v \.dir-locals\.el \
| xargs -t -I^ eask emacs -q --batch ^ --eval "\
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-ansible.el
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ Python virtual environment."
'(:npm :package "@ansible/ansible-language-server"
:path "ansible-language-server"))

(lsp-register-custom-settings
(lsp-register-new-settings
'(("ansible.ansible.path" lsp-ansible-ansible-path)
("ansible.ansible.useFullyQualifiedCollectionNames" lsp-ansible-use-fully-qualified-collection-names t)
("ansible.validation.enabled" lsp-ansible-validation-enabled t)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-css.el
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ server."
(const "messages")
(const "verbose")))

(lsp-register-custom-settings
(lsp-register-new-settings
'(("css.trace.server" lsp-css-trace-server)
("css.lint.unknownAtRules" lsp-css-lint-unknown-at-rules)
("css.lint.idSelector" lsp-css-lint-id-selector)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-elixir.el
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ be available here: https://github.com/elixir-lsp/elixir-ls/releases/"
:binary-path lsp-elixir-server-command
:set-executable? t))

(lsp-register-custom-settings
(lsp-register-new-settings
'(("elixirLS.dialyzerEnabled" lsp-elixir-dialyzer-enabled t)
("elixirLS.dialyzerWarnOpts" lsp-elixir-dialyzer-warn-opts)
("elixirLS.dialyzerFormat" lsp-elixir-dialyzer-format)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-fsharp.el
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ available, else the globally installed tool."
(push '(:AutomaticWorkspaceInit . t) opts)
opts)))

(lsp-register-custom-settings
(lsp-register-new-settings
`(("FSharp.KeywordsAutocomplete" lsp-fsharp-keywords-autocomplete t)
("FSharp.ExternalAutocomplete" lsp-fsharp-external-autocomplete t)
("FSharp.Linter" lsp-fsharp-linter t)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-go.el
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ $GOPATH/pkg/mod along with the value of
:risky t
:package-version '(lsp-mode "8.0.0"))

(lsp-register-custom-settings
(lsp-register-new-settings
'(("gopls.usePlaceholders" lsp-go-use-placeholders t)
("gopls.hoverKind" lsp-go-hover-kind)
("gopls.buildFlags" lsp-go-build-flags)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-groovy.el
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
:risky t
:type 'lsp-string-vector)

(lsp-register-custom-settings
(lsp-register-new-settings
'(("groovy.classpath" lsp-groovy-classpath)))

(lsp-register-client
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-haxe.el
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@
:type 'string
:group 'lsp-haxe)

(lsp-register-custom-settings
(lsp-register-new-settings
'(("haxe.hxml" lsp-haxe-hxml)
("haxe.postfixCompletion" lsp-haxe-postfix-completion)
("haxe.exclude" lsp-haxe-exclude)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-html.el
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ styles."
:group 'lsp-html
:package-version '(lsp-mode . "6.1"))

(lsp-register-custom-settings
(lsp-register-new-settings
'(("html.trace.server" lsp-html-trace-server)
("html.autoClosingTags" lsp-html-auto-closing-tags t)
("html.validate.styles" lsp-html-validate-styles t)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-idris.el
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
:type '(choice (:tag "off" "messages" "verbose"))
:package-version '(lsp-mode . "9.0.0"))

(lsp-register-custom-settings
(lsp-register-new-settings
'(("idris2-lsp.trace.server" lsp-idris2-lsp-trace-server)
("idris2-lsp.path" lsp-idris2-lsp-path)))

Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-javascript.el
Original file line number Diff line number Diff line change
Expand Up @@ -648,7 +648,7 @@ name (e.g. `data' variable passed as `data' parameter)."
:type 'boolean
:package-version '(lsp-mode . "9.0.0"))

(lsp-register-custom-settings
(lsp-register-new-settings
'(("javascript.autoClosingTags" lsp-javascript-auto-closing-tags t)
("javascript.implicitProjectConfig.checkJs" lsp-javascript-implicit-project-config-check-js t)
("javascript.implicitProjectConfig.experimentalDecorators" lsp-javascript-implicit-project-config-experimental-decorators t)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-json.el
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ here, https://github.com/emacs-lsp/lsp-mode/issues/3368#issuecomment-1049635155.
:group 'lsp-json
:package-version '(lsp-mode . "6.3"))

(lsp-register-custom-settings
(lsp-register-new-settings
'(("json.schemas" lsp-json-schemas)
("http.proxy" lsp-http-proxy)
("http.proxyStrictSSL" lsp-http-proxyStrictSSL)))
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-kotlin.el
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ Requires lsp-inlay-hints-mode."
:type 'boolean
:group 'lsp-kotlin)

(lsp-register-custom-settings
(lsp-register-new-settings
'(("kotlin.externalSources.autoConvertToKotlin" lsp-kotlin-external-sources-auto-convert-to-kotlin t)
("kotlin.externalSources.useKlsScheme" lsp-kotlin-external-sources-use-kls-scheme t)
("kotlin.debugAdapter.path" lsp-kotlin-debug-adapter-path)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-lua.el
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,7 @@ and `../lib` ,exclude `../lib/temp`.
:package-version '(lsp-mode . "8.0.0")
:group 'lsp-lua-language-server)

(lsp-register-custom-settings
(lsp-register-new-settings
'(("files.associations" lsp-lua-files-associations t)
("files.exclude" lsp-lua-files-exclude t)
("Lua.workspace.useGitIgnore" lsp-lua-workspace-use-git-ignore t)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-magik.el
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@
(lsp--set-configuration (lsp-configuration-section "magik"))))
:server-id 'magik))

(lsp-register-custom-settings
(lsp-register-new-settings
`(("magik.javaHome" lsp-magik-java-home)
("magik.smallworldGis" lsp-magik-smallworld-gis)
("magik.typing.typeDatabasePaths" lsp-magik-typing-type-database-paths)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-markdown.el
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ For a complete list of plugins, check:
'(:npm :package "unified-language-server"
:path "unified-language-server"))

(lsp-register-custom-settings
(lsp-register-new-settings
`(("unified-language-server.remark-parse.plugins" lsp-markdown-remark-plugins)
("unified-language-server.remark-parse.checkTextWith.setting" lsp-markdown-remark-check-text-with-setting)
("unified-language-server.remark-parse.checkTextWith.mutator" lsp-markdown-remark-check-text-with-mutator)))
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-openscad.el
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@
:type 'string
:group 'lsp-openscad)

(lsp-register-custom-settings
(lsp-register-new-settings
'(("openscad.search_paths" lsp-openscad-search-paths)
("openscad.fmt_exe" lsp-openscad-format-exe)
("openscad.fmt_style" lsp-openscad-format-style)))
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-perl.el
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ Defaults to 0."
:group 'lsp-perl
:package-version '(lsp-mode . "8.0.0"))

(lsp-register-custom-settings
(lsp-register-new-settings
'(("perl.perlCmd" lsp-perl-perl-cmd)
("perl.perlInc" lsp-perl-perl-inc)
("perl.fileFilter" lsp-perl-file-filter)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-perlnavigator.el
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ default to ~/.perlcriticrc. (no aliases, .bat files or ~/)."
:group 'lsp-perlnavigator
:package-version '(lsp-mode . "9.0.0"))

(lsp-register-custom-settings
(lsp-register-new-settings
'(("perlnavigator.trace.server" lsp-perlnavigator-trace-server)
("perlnavigator.logging" lsp-perlnavigator-logging t)
("perlnavigator.includePaths" lsp-perlnavigator-include-paths)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-pls.el
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ changes behavior depending on the contents of @ARGV."
:group 'lsp-pls
:package-version '(lsp-mode . "9.0.0"))

(lsp-register-custom-settings
(lsp-register-new-settings
'(("pls.cmd" lsp-pls-executable)
("pls.args" lsp-pls-arguments)
("pls.cwd" lsp-pls-working-dir)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-pwsh.el
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ extension."
:group 'lsp-pwsh
:package-version '(lsp-mode . "6.2"))

(lsp-register-custom-settings
(lsp-register-new-settings
'(("powershell.developer.featureFlags" lsp-pwsh-developer-feature-flags)
("powershell.developer.editorServicesWaitForDebugger" lsp-pwsh-developer-editor-services-wait-for-debugger t)
("powershell.codeFormatting.useCorrectCasing" lsp-pwsh-code-formatting-use-correct-casing t)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-pyls.el
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,7 @@ So it will rename only references it can find."
(setenv "PYENV_VERSION" pyenv-version)
python-env))))))

(lsp-register-custom-settings
(lsp-register-new-settings
'(("pyls.rope.ropeFolder" lsp-pyls-rope-rope-folder)
("pyls.rope.extensionModules" lsp-pyls-rope-extension-modules)
("pyls.plugins.rope_rename.enabled" (lambda () (eq lsp-pyls-rename-backend 'rope)) t)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-pylsp.el
Original file line number Diff line number Diff line change
Expand Up @@ -573,7 +573,7 @@ So it will rename only references it can find."
(setenv "PYENV_VERSION" pyenv-version)
python-env))))))

(lsp-register-custom-settings
(lsp-register-new-settings
'(("pylsp.rope.ropeFolder" lsp-pylsp-rope-rope-folder)
("pylsp.rope.extensionModules" lsp-pylsp-rope-extension-modules)
("pylsp.plugins.rope_rename.enabled" (lambda () (eq lsp-pylsp-rename-backend 'rope)) t)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-rf.el
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ Glob patterns as strings are accepted (eg. *bad.robot between double quotes)"
x))
seq)))

(lsp-register-custom-settings
(lsp-register-new-settings
'(
("rfLanguageServer.trace.server" lsp-rf-language-server-trace-server)
("rfLanguageServer.logLevel" lsp-rf-language-server-log-level)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-rust.el
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ is often the type local variable declaration."
:group 'lsp-rust-rls
:package-version '(lsp-mode . "6.1"))

(lsp-register-custom-settings
(lsp-register-new-settings
'(("rust.show_hover_context" lsp-rust-show-hover-context t)
("rust.full_docs" lsp-rust-full-docs t)
("rust.build_command" lsp-rust-build-command)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-sml.el
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ more information."
:type 'string
:group 'lsp-sml)

(lsp-register-custom-settings
(lsp-register-new-settings
'(("millet.format.engine" lsp-sml-millet-format-engine)
("millet.server.diagnostics.filter" lsp-sml-millet-server-diagnostics-filter)
("millet.server.diagnostics.moreInfoHint.enable" lsp-sml-millet-server-diagnostics-moreInfoHint-enable)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-solargraph.el
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@
(append '("bundle" "exec") lsp-solargraph-server-command)
lsp-solargraph-server-command))

(lsp-register-custom-settings
(lsp-register-new-settings
'(("solargraph.logLevel" lsp-solargraph-log-level)
("solargraph.folding" lsp-solargraph-folding t)
("solargraph.references" lsp-solargraph-references t)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-svelte.el
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ Example: `((css-unused-selector . ignore) (unused-export-let . error))"
:type 'boolean
:package-version '(lsp-mode . "8.0.0"))

(lsp-register-custom-settings
(lsp-register-new-settings
'(("svelte.plugin.svelte.rename.enable" lsp-svelte-plugin-svelte-rename-enable t)
("svelte.plugin.svelte.selectionRange.enable" lsp-svelte-plugin-svelte-selection-range-enable t)
("svelte.plugin.svelte.codeActions.enable" lsp-svelte-plugin-svelte-code-actions-enable t)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-verilog.el
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@
:library-folders-fn 'lsp-clients-svlangserver-get-workspace-additional-dirs
:server-id 'svlangserver))

(lsp-register-custom-settings '(("systemverilog.includeIndexing" lsp-clients-svlangserver-includeIndexing)
(lsp-register-new-settings '(("systemverilog.includeIndexing" lsp-clients-svlangserver-includeIndexing)
("systemverilog.excludeIndexing" lsp-clients-svlangserver-excludeIndexing)
("systemverilog.defines" lsp-clients-svlangserver-defines)
("systemverilog.launchConfiguration" lsp-clients-svlangserver-launchConfiguration)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-vetur.el
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ Set a source to \"\" to disable it.
:group 'lsp-vetur
:package-version '(lsp-mode . "6.1"))

(lsp-register-custom-settings
(lsp-register-new-settings
'(("vetur.trace.server" lsp-vetur-trace-server)
("vetur.ignoreProjectWarning" lsp-vetur-ignore-project-warning t)
("vetur.format.scriptInitialIndent" lsp-vetur-format-script-initial-indent t)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-volar.el
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@
'(:system "vue-language-server")
'(:npm :package "@vue/language-server" :path "vue-language-server"))

(lsp-register-custom-settings
(lsp-register-new-settings
'(("typescript.tsdk"
(lambda ()
(if-let ((project-root (lsp-workspace-root))
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-xml.el
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ The value for `enabled' can be always, never or onValidSchema."
:group 'lsp-xml
:package-version '(lsp-mode . "6.1"))

(lsp-register-custom-settings '
(lsp-register-new-settings '
(("xml.validation.schema" lsp-xml-validation-schema)
("xml.validation.resolveExternalEntities" lsp-xml-validation-resolve-external-entities)
("xml.validation.enabled" lsp-xml-validation-enabled t)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-yaml.el
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ Limited for performance reasons."
(defvar lsp-yaml--schema-store-schemas-alist nil
"A list of schemas fetched from schema stores.")

(lsp-register-custom-settings
(lsp-register-new-settings
'(("yaml.format.enable" lsp-yaml-format-enable t)
("yaml.format.singleQuote" lsp-yaml-single-quote t)
("yaml.format.bracketSpacing" lsp-yaml-bracket-spacing)
Expand Down
2 changes: 1 addition & 1 deletion clients/lsp-zig.el
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ and not the global storage."
;;
;;; Core

(lsp-register-custom-settings
(lsp-register-new-settings
'(("zls.enable_snippets" lsp-zls-enable-snippets t)
("zls.enable_argument_placeholders" lsp-zig-enable-argument-placeholders t)
("zls.enable_build_on_save" lsp-zig-enable-build-on-save t)
Expand Down
30 changes: 28 additions & 2 deletions lsp-mode.el
Original file line number Diff line number Diff line change
Expand Up @@ -8539,12 +8539,38 @@ optional flag that should be non-nil for boolean settings, when it is nil the
property will be ignored if the VALUE is nil.

Example: `(lsp-register-custom-settings `((\"foo.bar.buzz.enabled\" t t)))'
\(note the double parentheses)"
\(note the double parentheses)

Internal note: this function should not be used in by `lsp' interanlly
due to potentially overriding user settings. Use
`lsp-register-new-settings' instead."
(mapc
(-lambda ((path . rest))
(puthash path rest lsp-client-settings))
props))

(defun lsp-register-new-settings (props)
"Register PROPS that did not exist yet.

This function avoids overwriting a setting that has already existed. For
overwriting purposes use `lsp-register-custom-settings'.

PROPS is list of triple (path value boolean?) where PATH is the path to the
property; VALUE can be a literal value, symbol to be evaluated, or either a
function or lambda function to be called without arguments; BOOLEAN? is an
optional flag that should be non-nil for boolean settings, when it is nil the
property will be ignored if the VALUE is nil.

Example: `(lsp-register-new-settings `((\"foo.bar.buzz.enabled\" t t)))'
\(note the double parentheses)

"
(mapc
(-lambda ((path . rest))
(unless (gethash path lsp-client-settings)
(puthash path rest lsp-client-settings)))
props))

(defun lsp-region-text (region)
"Get the text for REGION in current buffer."
(-let (((start . end) (lsp--range-to-region region)))
Expand Down Expand Up @@ -8572,7 +8598,7 @@ TBL - a hash table, PATHS is the path to the nested VALUE."
(let ((path (plist-get args :lsp-path)))
(cl-remf args :lsp-path)
`(progn
(lsp-register-custom-settings
(lsp-register-new-settings
(quote ((,path ,symbol ,(equal ''boolean (plist-get args :type))))))

(defcustom ,symbol ,standard ,doc
Expand Down
8 changes: 4 additions & 4 deletions test/lsp-common-test.el
Original file line number Diff line number Diff line change
Expand Up @@ -169,8 +169,8 @@
:risky t
:type 'string)

(lsp-register-custom-settings '(("section2.nested.prop1" lsp-nested-prop1)))
(lsp-register-custom-settings '(("section2.nested.prop2" lsp-nested-prop2)))
(lsp-register-new-settings '(("section2.nested.prop1" lsp-nested-prop1)))
(lsp-register-new-settings '(("section2.nested.prop2" lsp-nested-prop2)))

(ert-deftest lsp--custom-settings-test-2 ()
(let ((actual (lsp-ht->alist (lsp-configuration-section "section2"))))
Expand All @@ -192,7 +192,7 @@
:risky t
:type 'string)

(lsp-register-custom-settings '(("section3.prop1" lsp-prop3 t)))
(lsp-register-new-settings '(("section3.prop1" lsp-prop3 t)))

(ert-deftest lsp--boolean-property ()
(cl-assert (equal (lsp-ht->alist (lsp-configuration-section "section3"))
Expand All @@ -203,7 +203,7 @@
(cl-assert (equal (aref (lsp--build-workspace-configuration-response request) 0)
:json-false))))

(lsp-register-custom-settings '(("section4.prop1" "value")))
(lsp-register-new-settings '(("section4.prop1" "value")))

(ert-deftest lsp--non-boolean-property ()
(cl-assert (equal (lsp-ht->alist (lsp-configuration-section "section4"))
Expand Down

0 comments on commit 42fdebe

Please sign in to comment.