refactor(rome_lsp): refactor the initialization and configuration loading logic #4044
Conversation
✅ Deploy Preview for docs-rometools ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
| if let Some(uri) = params.root_uri { | ||
| self.session.root_uri.write().unwrap().replace(uri); | ||
| if params.root_path.is_some() { | ||
| warn!("The Rome Server was initialized with the deprecated `root_path` parameter: this is not supported, use `root_uri` instead"); |
There was a problem hiding this comment.
Out of curiosity, who sends root_path? A client that still uses an old version of the LSP?
There was a problem hiding this comment.
From the LSP specification, rootPath was deprecated in version 3.0 of the protocol in favor of rootUri. The rootUri field was later deprecated in version 3.16 in favor of workspaceFolders, but we don't support that yet
crates/rome_lsp/src/session.rs
Outdated
| let client_configurations = match self.client.configuration(vec![item]).await { | ||
| Ok(client_configurations) => client_configurations, | ||
| Err(err) => { | ||
| error!("Cannot read configuration from the client: {err}"); |
There was a problem hiding this comment.
nit: I suggest normalizing the use of the verbs: some messages use "Couldn't" and other messages use "Cannot"
There was a problem hiding this comment.
I normalized the error messages to use the past tense, that seems to be what we use the most
Summary
This PR refactors the initialization and configuration loading logic of the language server Session to limit the use of
MutexandRwLock:client_capabilities,client_informationandroot_urifields are only written to when the session is initialized, with all following access being read-only. I've moved these fields to a newInitializeParamsstruct stored in aOnceCellcontainer (I'm using the implementation fromtokioto avoid having to pull in another crate untilstd::sync::OnceLockis stabilized) that should provide a similar "write-once read-many" synchronization primitive at a lower performance and memory cost thanMutexorRwLock.update_configurationbut applied to the workspace inupdate_workspace_settings, requiring it to be stored in anRwLockin the meantime. I've separated the logic for loading the configuration intoload_workspace_settings(load therome.jsonfile and apply it to the workspace) andload_client_configuration(load the editor configuration and store it in the session config) to eliminate the need for an intermediate storage.In addition to these changes, I also added or modified some of the tracing logs related to initialization and configuration loading to help us debug potential issues using the output of
rome rageTest Plan
This is intended as an internal change only, it should not modify the observable behavior of the language server. In particular the
rome_lsptest suite should continue to pass, as well as therome_clitests that make use of the daemon service infrastructure.