json_schema_store: Include available LSP adapters in settings schema#46766
Conversation
5f810dc to
d191727
Compare
d191727 to
148ea05
Compare
Fix "Property `ty` is not allowed" warning in settings.json for LSP adapters registered via `register_available_lsp_adapter()`. Previously, only adapters registered via `register_lsp_adapter()` were included in the settings schema. This caused validation warnings for adapters like `ty`, `pylsp`, `eslint`, and `tailwindcss-language-server`. Changes: - Add `available_lsp_adapter_names()` method to LanguageRegistry - Include available adapter names in settings schema generation - Support initialization_options schema lookup for available adapters Fixes zed-industries#46556
148ea05 to
54ff93b
Compare
|
We require contributors to sign our Contributor License Agreement, and we don't have @baeseokjae on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
|
We require contributors to sign our Contributor License Agreement, and we don't have @baeseokjae on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
|
I made some fixes for the PR. Sorry for the delay. Please follow the cla instructions above, and I'll merge this! |
| @@ -224,6 +225,9 @@ async fn resolve_dynamic_schema( | |||
| .all_lsp_adapters() | |||
| .into_iter() | |||
| .find(|adapter| adapter.name().as_ref() as &str == lsp_name) | |||
| .or_else(|| { | |||
| languages.load_available_lsp_adapter(&LanguageServerName::from(lsp_name)) | |||
There was a problem hiding this comment.
I'm concerned about calling load_available_lsp_adapter here. It is not a pure "getter" for information; it has significant side effects and directly mutates the state of the language server module.
In my view, the settings schema resolution should be decoupled from the LSP lifecycle. We should only be querying the existing state rather than triggering a "load" action, which could lead to unintended consequences.
There was a problem hiding this comment.
@lingyaochu thanks for the review.
Just to clarify, load_available_lsp_adapter here doesn’t really act as a pure accessor, but it also doesn’t mutate shared state. It only takes a read lock and creates a temporary instance for schema lookup, then drops it.
For available adapters like ty, eslint, or pylsp, this is basically the only way to access their schemas today since they’re lazy-loaded and not part of all_lsp_adapters.
I agree that having schema resolution go through a “load” path isn’t ideal. That feels like something better handled as a separate issue later on. For this PR though, sticking with the current approach seems reasonable to me and I don’t see this causing issues in practice.
There was a problem hiding this comment.
Thanks for the clarification! I misunderstood how load_available_lsp_adapter worked under the hood. Given that it doesn't mutate shared state and is necessary for lazy-loaded adapters, this approach makes sense for now. I'm happy with the fix!
|
@cla-bot check |
|
We require contributors to sign our Contributor License Agreement, and we don't have @baeseokjae on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
@probably-neb I’ve signed the CLA before, but I changed username recently and the bot doesn’t seem to recognize it. Could you take a quick look when you get a chance? Thanks! |
|
@baeseokjae Can I ask you to re-sign the CLA? We track this only based on the current username, so would be nice if you could just sign it again. Thanks! |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
Auto-applied queued documentation suggestions from: - PR #48908 - PR #48909 - PR #48910 - PR #48912 - PR #48930 - PR #44794 - PR #48763 - PR #45073 - PR #48495 - PR #49374 - PR #49139 - PR #48780 - PR #48619 - PR #48978 - PR #48962 - PR #48988 - PR #47860 - PR #49015 - PR #47095 - PR #47475 - PR #48542 - PR #46766 - PR #47754 - PR #48807 - PR #44506 - PR #49051 - PR #49069 - PR #48842 - PR #48851 - PR #48736 - PR #47673 - PR #49094 - PR #49098 - PR #49622 Generated with script/docs-suggest-publish for human review in draft PR.
Auto-applied queued documentation suggestions from: - PR #48908 - PR #48909 - PR #48910 - PR #48912 - PR #48930 - PR #44794 - PR #48763 - PR #45073 - PR #48495 - PR #49374 - PR #49139 - PR #48780 - PR #48619 - PR #48978 - PR #48962 - PR #48988 - PR #47860 - PR #49015 - PR #47095 - PR #47475 - PR #48542 - PR #46766 - PR #47754 - PR #48807 - PR #44506 - PR #49051 - PR #49069 - PR #48842 - PR #48851 - PR #48736 - PR #47673 - PR #49094 - PR #49098 - PR #49622 - PR #49554 - PR #49710 - PR #49716 - PR #49732 - PR #49788 - PR #49876 - PR #49902 - PR #49910 - PR #49390 - PR #50027 Generated with script/docs-suggest-publish for human review in draft PR.
Auto-applied documentation from: - PR #48542: Bedrock extended context window - PR #46766: LSP adapters in settings schema - PR #47754: VSCode tasks.json label generation Skipped (no target file exists): - PR #49069: panel zoom state persistence Already documented from prior batches: - PR #48807, PR #44506, PR #49051, PR #48842, PR #48851, PR #48736
Auto-applied queued documentation suggestions from: - PR #48908 - PR #48909 - PR #48910 - PR #48912 - PR #48930 - PR #44794 - PR #48763 - PR #45073 - PR #48495 - PR #49374 - PR #49139 - PR #48780 - PR #48619 - PR #48978 - PR #48962 - PR #48988 - PR #47860 - PR #49015 - PR #47095 - PR #47475 - PR #48542 - PR #46766 - PR #47754 - PR #48807 - PR #44506 - PR #49051 - PR #49069 - PR #48842 - PR #48851 - PR #48736 - PR #47673 - PR #49094 - PR #49098 - PR #49622 - PR #49554 - PR #49710 - PR #49716 - PR #49732 - PR #49788 - PR #49876 - PR #49902 - PR #49910 - PR #49390 - PR #50027 Generated with script/docs-suggest-publish for human review in draft PR.
Auto-applied documentation from: - PR #48542: Bedrock extended context window - PR #46766: LSP adapters in settings schema - PR #47754: VSCode tasks.json label generation Skipped (no target file exists): - PR #49069: panel zoom state persistence Already documented from prior batches: - PR #48807, PR #44506, PR #49051, PR #48842, PR #48851, PR #48736
Closes #46556
Summary
tyis not allowed" warning insettings.jsonfor LSP adapters registered viaregister_available_lsp_adapter()available_lsp_adapter_names()method to include these adapters in schema generationinitialization_optionsschema lookup for available adaptersProblem
LSP adapters registered via
register_available_lsp_adapter()were not included in the settings JSON schema. This caused validation warnings like:Property ty is not allowed
Even though
tyis a built-in Python language server that works correctly.Affected adapters:
ty,py,python-lsp-servereslint,vtsls,typescript-language-servertailwindcss-language-server,tailwindcss-intellisense-cssSolution
Schema generation now queries both:
all_lsp_adapters()- adapters bound to specific languagesavailable_lsp_adapter_names()- adapters enabled via settings (new)Related: #43104, #45928
Release Notes:
settings.json