-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Support LSP client settings #19614
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
Conversation
59963e8
to
43e3f39
Compare
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
f4f95fc
to
0cd42eb
Compare
) -> crate::server::Result<Option<GotoDefinitionResponse>> { | ||
if snapshot.client_settings().is_language_services_disabled() { | ||
if snapshot | ||
.workspace_settings() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the new terminology used here
I like where this is going. It clarifies a lot of the differences!
This makes a lot of sense to me and seems more intuitive. Especially given that most editors don't support workspaces.
I don't fully understand this part. I also left an inline comment related to this but I think the structure we want to deserialize into in the workspace configuration handler is one that has two fields: |
} | ||
} | ||
|
||
/// Creates the default [`DiagnosticOptions`] for the server. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a bit out of place. The function doesn't contain any code related to capabilities
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does, the DiagnosticOptions
are the server capability for document diagnostic.
Notes for the previous iteration: Details
This PR adds support for client settings via the Currently, the settings can only be passed in via the initialization options which are limited because any change to the settings require a server restart to take effect. This is why Micha added the infrastructure to request configuration for a specific workspace via the This PR implements the point 1, 2, 3 from astral-sh/ty#82 (comment) i.e.,
This split is recommended in microsoft/language-server-protocol#567 and is also followed by Dart where initialization options are separate from workspace options. But, there seems to be one issue which is that the way For VS Code: {
"ty.logLevel": "debug",
"ty.diagnosticMode": "workspace"
} For Neovim: {
settings = {
diagnosticMode = "workspace",
},
init_options = {
logLevel = "debug",
}
} For Zed: {
"lsp": {
"ty": {
"settings": {
"ty": {
"diagnosticMode": "openFilesOnly"
}
},
"initialization_options": {
"logLevel": "debug"
}
}
}
} For Helix: [language-server.ty.config]
logLevel = "debug"
ty = { diagnosticMode = "workspace" } QuestionBased on all of the changes that I've made so far and what I'm observing when using this PR to define the settings in an editor is that this model still seems a bit confusing from a user perspective. One way to improve would be to accept all the settings in initialization options and consider them global settings. Then, if the client supports |
// Check if language services are disabled | ||
if snapshot | ||
.index() | ||
.global_settings() | ||
.is_language_services_disabled() | ||
{ | ||
return Ok(None); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't query this setting because it's scope is limited to a specific workspace and these endpoints are for all the workspaces managed by the server i.e., this setting is present under WorkspaceSettings
but the SessionSnapshot
does not know which workspace is this endpoint for.
I think this is fine because all the workspace related endpoints except for workspace diagnostics are triggered by user explicitly. But, if we do want to enforce this, we could use the settings value from each workspace and act on it accordingly. For example, in workspace symbols, we would skip the workspace if language services are disabled for that workspace:
// Iterate through all projects in the session
for db in snapshot.projects() {
// Find the workspace which this project belongs to
// Check if language services are enabled or not
if language_services_disabled {
continue;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome. Thank you.
The only change I'd like to see before landing this PR is to deduplicate Combine
. We already have two implementations of it (one in ty and one in ruff). I'd rather avoid a third.
serde_json::from_value(value).unwrap_or_else(|err| { | ||
tracing::warn!( | ||
"Failed to deserialize workspace options for {url}: {err}. \ | ||
Using default options" | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's unrelated to your changes, so I'm okay deferring this. But should we maybe show a pop up if this happens? E.g. could this happen if a user configures zed or neovim incorrectly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think that could happen and it might go unnoticed, I think it'd be useful to add a popup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be better as a follow-up instead
...ts/e2e/snapshots/e2e__initialize__open_files_diagnostic_registration_via_initialization.snap
Outdated
Show resolved
Hide resolved
let workspace_configurations = workspaces | ||
.into_iter() | ||
.map(|(folder, options)| (folder.uri, options)) | ||
.filter_map(|(folder, options)| Some((folder.uri, options?))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it required that we have one option for each url
? What's the reason for allowing None
for the configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a requirement for the storage part on the client side but it's a requirement when responding to the server which is handled by the handle_workspace_configuration_request
method.
(I've addressed most of the feedback, I need to address a couple more but I need to go out for sometime, will do it once I'm back.) |
|
# Summary Related PR for ty server: astral-sh/ruff#19614 This PR simplifies the settings handling in the extension by: - Replace `IInitializationOptions` with `InitializationOptions` that only contains `logLevel` and `logFile` - Let server ask for workspace specific options via the `workspace/configuration` request - Rename `ISettings` to `ExtensionSettings` as now that only contains settings specific to VS Code - Rename `python.ty.disableLanguageServices` to `ty.disableLanguageServices` ## Test Plan Refer to the test plan in astral-sh/ruff#19614 specific to VS Code.
## Summary This PR updates the documentation to reflect the changes made in astral-sh/ruff#19614 and astral-sh/ty-vscode#106. ## Test Plan <details><summary>Screenshot of the new editor settings reference page:</summary> <p> <img width="4992" height="3702" alt="image" src="https://github.com/user-attachments/assets/e3afd5a3-8b5b-43c3-addd-e1d6636ab1b7" /> </p> </details>
Summary
This PR implements support for providing LSP client settings.
The complementary PR in the ty VS Code extension: astral-sh/ty-vscode#106.
Notes for the previous iteration of this PR is in #19614 (comment) (click on "Details").
Specifically, this PR splits the client settings into 3 distinct groups. Keep in mind that these groups are not visible to the user, they're merely an implementation detail. The groups are:
GlobalOptions
- these are the options that are global to the language server and will be the same for all the workspaces that are handled by the serverWorkspaceOptions
- these are the options that are specific to a workspace and will be applied only when running any logic for that workspaceInitializationOptions
- these are the options that can be specified during initializationThe initialization options are a superset that contains both the global and workspace options flattened into a 1-dimensional structure. This means that the user can specify any and all fields present in
GlobalOptions
andWorkspaceOptions
in the initialization options in addition to the fields that are specific to initialization options.From the current set of available settings, following are only available during initialization because they are required at that time, are static during the runtime of the server and changing their values require a restart to take effect:
logLevel
logFile
And, following are available under
GlobalOptions
:diagnosticMode
And, following under
WorkspaceOptions
:disableLanguageServices
pythonExtension
(Python environment information that is populated by the ty VS Code extension)workspace/configuration
This request allows server to ask the client for configuration to a specific workspace. But, this is only supported by the client that has the
workspace.configuration
client capability set totrue
. What to do for clients that don't support pulling configurations?In that case, the settings needs to be provided in the initialization options and updating the values of those settings can only be done by restarting the server. With the way this is implemented, this means that if the client does not support pulling workspace configuration then there's no way to specify settings specific to a workspace. Earlier, this would've been possible by providing an array of client options with an additional field which specifies which workspace the options belong to but that adds complexity and clients that actually do not support
workspace/configuration
would usually not support multiple workspaces either.Now, for the clients that do support this, the server will initiate the request to get the configuration for all the workspaces at the start of the server. Once the server receives these options, it will resolve them for each workspace as follows:
The global options are resolved into the global settings and are available on the
Session
which is initialized with the default global settings. The workspace options are resolved into the workspace settings and are available on the respectiveWorkspace
.The
SessionSnapshot
contains the global settings while the document snapshot contains the workspace settings. We could add the global settings to the document snapshot but that's currently not needed.Document diagnostic dynamic registration
Currently, the document diagnostic server capability is created based on the
diagnosticMode
sent during initialization. But, that wouldn't provide us with the complete picture. This means the server needs to defer registering the document diagnostic capability at a later point once the settings have been resolved.This is done using dynamic registration for clients that support it. For clients that do not support dynamic registration for document diagnostic capability, the server advertises itself as always supporting workspace diagnostics and work done progress token.
This dynamic registration now allows us to change the server capability for workspace diagnostics based on the resolved
diagnosticMode
value. In the future, onceworkspace/didChangeConfiguration
is supported, we can avoid the server restart when users have changed any client settings.Test Plan
Add integration tests and recorded videos on the user experience in various editors:
VS Code
For VS Code users, the settings experience is unchanged because the extension defines it's own interface on how the user can specify the server setting. This means everything is under the
ty.*
namespace as usual.Screen.Recording.2025-08-04.at.17.12.30.mov
Zed
For Zed, the settings experience has changed. Users can specify settings during initialization:
Or, can specify the options under the
settings
key:The
logLevel
andlogFile
setting still needs to go under the initialization options because they're required by the server during initialization.We can remove the nesting of the settings under the "ty" namespace by updating the return type of https://github.com/zed-extensions/ty/blob/db9ea0cdfd7352529748ef5bf729a152c0219805/src/tychecker.rs#L45-L49 to be wrapped inside
ty
directly so that users can avoid doing the double nesting.There's one issue here which is that if the
diagnosticMode
is specified in both the initialization option and settings key, then the resolution is a bit different - if either of them is set to beworkspace
, then it wins which means that in the following configuration, the diagnostic mode isworkspace
:This behavior is mainly a result of combining global options from various workspace configuration results. Users should not be able to provide global options in multiple workspaces but that restriction cannot be done on the server side. The ty VS Code extension restricts these global settings to only be set in the user settings and not in workspace settings but we do not control extensions in other editors.
Screen.Recording.2025-08-04.at.17.40.07.mov
Neovim
Same as in Zed.
Other
Other editors that do not support
workspace/configuration
, the users would need to provide the server settings during initialization.