Skip to content

lsp: Exclude dynamic settings from LanguageServerSeed identity#47376

Merged
osiewicz merged 1 commit intozed-industries:mainfrom
aviatesk:fix-workspace-configuration
Jan 28, 2026
Merged

lsp: Exclude dynamic settings from LanguageServerSeed identity#47376
osiewicz merged 1 commit intozed-industries:mainfrom
aviatesk:fix-workspace-configuration

Conversation

@aviatesk
Copy link
Contributor

@aviatesk aviatesk commented Jan 22, 2026

LanguageServerSeed is used as a key to identify language servers. Previously, it included the entire LspSettings, which meant that changing lsp.<server>.settings (dynamic configuration) would cause the server to restart unnecessarily.

Dynamic settings can be updated via LSP's workspace/didChangeConfiguration notification without requiring a server restart. Only binary and initialization_options should be part of the server identity, as changes to these genuinely require restarting the server.

This is a follow-up fix to #35270 which introduced LanguageServerSeed but inadvertently included dynamic settings in the server identity (although I remember that this dynamic settings reflection stopped working pretty recently, so there might be other commits besides #35270 that changed the behavior of LanguageServerSeed)

Closes #ISSUE

Release Notes:

  • Fixed language servers unnecessarily restarting when changing lsp.<server>.settings configuration. Dynamic settings are now properly updated via workspace/didChangeConfiguration without requiring a server restart.

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Jan 22, 2026
osiewicz
osiewicz reviewed Jan 22, 2026
View reviewed changes
osiewicz
osiewicz requested changes Jan 22, 2026
View reviewed changes
Copy link
Member

@osiewicz osiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if instead of jumping hoops around comparison operators, we could instead create a newtype that would contain settings.binary and settings.initialization_options. I think that'd be less error-prone. What do you think?

@osiewicz osiewicz self-assigned this Jan 22, 2026
@aviatesk
Copy link
Contributor Author

aviatesk commented Jan 22, 2026

I wonder if instead of jumping hoops around comparison operators, we could instead create a newtype that would contain settings.binary and settings.initialization_options. I think that'd be less error-prone. What do you think?

I considered this, but the newtype approach would touch 3 files instead of 2, and we'd still need to remember to update the From impl when LspSettings changes, which would be same maintenance burden with more indirection?
Happy to reconsider if you feel strongly about it though.

@aviatesk aviatesk force-pushed the fix-workspace-configuration branch from 051475a to d5e7da1 Compare January 22, 2026 12:55
@osiewicz
Copy link
Member

osiewicz commented Jan 22, 2026

Yeah, I feel somewhat strongly about it; for one, you don't need to provide From impl because it'd be the responsibility of whoever is using that newtype to figure out what they want to do with the settings part.

Right now settings.settings has no meaning whatsoever when it is extracted from the tree, because we quietly ignore it's contents. I'd rather not have settings.settings within the tree at all and force the user to figure it out. Your PR is a great contribution because it solves a real issue, but in face of that I don't think we should be using LspSettings anymore - we just want to know what settings the language was spawned with.

@aviatesk aviatesk force-pushed the fix-workspace-configuration branch from d5e7da1 to 766a10e Compare January 22, 2026 13:53
@aviatesk
Copy link
Contributor Author

aviatesk commented Jan 22, 2026

Makes sense. I've introduced LanguageServerSeedSettings newtype for LanguageServerSeed without From impl and with callers constructing it explicitly.
LaunchDisposition.settings still holds Arc<LspSettings> since start_language_server needs initialization_options from it. Functionally this is fine, but for consistency we could also change it to exclude dynamic settings. Worth doing in this PR, or separate concern?

@aviatesk
Copy link
Contributor Author

aviatesk commented Jan 26, 2026

Friendly ping. This issue is central to the language server UX in Zed (might just to me though?) , so I'd be happy if we could move forward with it.

osiewicz
osiewicz approved these changes Jan 26, 2026
View reviewed changes
Copy link
Member

@osiewicz osiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Let's address your concern on start_language_server in a follow-up PR. Also, the CI does not pass currently.

@aviatesk
lsp: Exclude dynamic settings from LanguageServerSeed identity
eb50972 
`LanguageServerSeed` is used as a key to identify language servers.
Previously, it included the entire `LspSettings`, which meant that
changing `lsp.<server>.settings` (dynamic configuration) would cause
the server to restart unnecessarily.

Dynamic settings can be updated via LSP's `workspace/didChangeConfiguration`
notification without requiring a server restart. Only `binary` and
`initialization_options` should be part of the server identity, as
changes to these genuinely require restarting the server.

This is a follow-up fix to zed-industries#35270 which introduced `LanguageServerSeed`
but inadvertently included dynamic settings in the server identity.
@aviatesk aviatesk force-pushed the fix-workspace-configuration branch from 766a10e to eb50972 Compare January 26, 2026 21:28
@aviatesk
Copy link
Contributor Author

aviatesk commented Jan 26, 2026

Thanks. Fixed the formatting issue.

@osiewicz osiewicz merged commit 2dd4897 into zed-industries:main Jan 28, 2026
27 checks passed
@osiewicz
Copy link
Member

osiewicz commented Jan 28, 2026

Thank you!

@aviatesk aviatesk deleted the fix-workspace-configuration branch January 28, 2026 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@osiewicz osiewicz osiewicz approved these changes

Assignees

@osiewicz osiewicz

Labels

cla-signed The user has signed the Contributor License Agreement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@aviatesk @osiewicz