diff options
author | Nayeem Rahman <nayeemrmn99@gmail.com> | 2023-09-02 16:36:04 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2023-09-02 16:36:04 +0100 |
commit | 12f6ad32c2108efb1492df6192a1f8cdd5eafa76 (patch) | |
tree | 8afd5f0a4890f62eba658850e294663540e949c4 | |
parent | 83426be6eead06c680ae527468aeaf8723543ff2 (diff) |
fix(lsp): properly handle disabled configuration requests (#20358)
Fixes #19802.
Properly respect when clients do not have the `workspace/configuration`
capability, a.k.a. when an editor cannot provide scoped settings on
request from the LSP.
- Fix one spot where we weren't checking for the capability before
sending this request.
- For `enablePaths`, fall back to the settings passed in the
initialization options in more cases.
- Respect the `workspace/configuration` capability in the test harness
client.
See:
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_configuration.
-rw-r--r-- | cli/lsp/config.rs | 13 | ||||
-rw-r--r-- | cli/lsp/language_server.rs | 15 | ||||
-rw-r--r-- | cli/tests/integration/lsp_tests.rs | 81 | ||||
-rw-r--r-- | test_util/src/lsp.rs | 18 |
4 files changed, 111 insertions, 16 deletions
diff --git a/cli/lsp/config.rs b/cli/lsp/config.rs index b70af6519..2d77abaee 100644 --- a/cli/lsp/config.rs +++ b/cli/lsp/config.rs @@ -718,13 +718,12 @@ impl Config { if let Some(workspace_folders) = self.workspace_folders.clone() { let mut touched = false; for (workspace, _) in workspace_folders { - if let Some(settings) = self.settings.specifiers.get(&workspace) { - if self.update_enabled_paths_entry( - workspace, - settings.enable_paths.clone(), - ) { - touched = true; - } + let enabled_paths = match self.settings.specifiers.get(&workspace) { + Some(settings) => settings.enable_paths.clone(), + None => self.settings.workspace.enable_paths.clone(), + }; + if self.update_enabled_paths_entry(workspace, enabled_paths) { + touched = true; } } touched diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index dc85eb1cc..d4e5b3da9 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -1275,6 +1275,7 @@ impl Inner { error!("Cannot set workspace settings: {}", err); LspError::internal_error() })?; + self.config.update_enabled_paths(); } self.config.workspace_folders = params.workspace_folders.map(|folders| { folders @@ -1471,6 +1472,7 @@ impl Inner { if let Err(err) = self.config.set_workspace_settings(value) { error!("failed to update settings: {}", err); } + self.config.update_enabled_paths(); } self.update_debug_flag(); @@ -3125,7 +3127,7 @@ impl tower_lsp::LanguageServer for LanguageServer { return; } - let (client, client_uri, specifier, had_specifier_settings) = { + let (client, client_uri, specifier, should_get_specifier_settings) = { let mut inner = self.0.write().await; let client = inner.client.clone(); let client_uri = LspClientUrl::new(params.text_document.uri.clone()); @@ -3133,24 +3135,25 @@ impl tower_lsp::LanguageServer for LanguageServer { .url_map .normalize_url(client_uri.as_url(), LspUrlKind::File); let document = inner.did_open(&specifier, params).await; - let has_specifier_settings = - inner.config.has_specifier_settings(&specifier); + let should_get_specifier_settings = + !inner.config.has_specifier_settings(&specifier) + && inner.config.client_capabilities.workspace_configuration; if document.is_diagnosable() { inner.refresh_npm_specifiers().await; let specifiers = inner.documents.dependents(&specifier); inner.diagnostics_server.invalidate(&specifiers); // don't send diagnostics yet if we don't have the specifier settings - if has_specifier_settings { + if !should_get_specifier_settings { inner.send_diagnostics_update(); inner.send_testing_update(); } } - (client, client_uri, specifier, has_specifier_settings) + (client, client_uri, specifier, should_get_specifier_settings) }; // retrieve the specifier settings outside the lock if // they haven't been asked for yet - if !had_specifier_settings { + if should_get_specifier_settings { let response = client .when_outside_lsp_lock() .specifier_configuration(&client_uri) diff --git a/cli/tests/integration/lsp_tests.rs b/cli/tests/integration/lsp_tests.rs index 218b1db40..19b968602 100644 --- a/cli/tests/integration/lsp_tests.rs +++ b/cli/tests/integration/lsp_tests.rs @@ -643,6 +643,87 @@ fn lsp_import_map_node_specifiers() { client.shutdown(); } +// Regression test for https://github.com/denoland/deno/issues/19802. +// Disable the `workspace/configuration` capability. Ensure the LSP falls back +// to using `enablePaths` from the `InitializationOptions`. +#[test] +fn lsp_workspace_enable_paths_no_workspace_configuration() { + let context = TestContextBuilder::new().use_temp_cwd().build(); + let temp_dir = context.temp_dir(); + temp_dir.write("main_disabled.ts", "Date.now()"); + temp_dir.write("main_enabled.ts", "Date.now()"); + + let mut client = context.new_lsp_command().build(); + client.initialize(|builder| { + builder.with_capabilities(|capabilities| { + capabilities.workspace.as_mut().unwrap().configuration = Some(false); + }); + builder.set_workspace_folders(vec![lsp::WorkspaceFolder { + uri: temp_dir.uri(), + name: "project".to_string(), + }]); + builder.set_root_uri(temp_dir.uri()); + builder.set_enable_paths(vec!["./main_enabled.ts".to_string()]); + }); + + client.did_open(json!({ + "textDocument": { + "uri": temp_dir.uri().join("main_disabled.ts").unwrap(), + "languageId": "typescript", + "version": 1, + "text": temp_dir.read_to_string("main_disabled.ts"), + } + })); + + client.did_open(json!({ + "textDocument": { + "uri": temp_dir.uri().join("main_enabled.ts").unwrap(), + "languageId": "typescript", + "version": 1, + "text": temp_dir.read_to_string("main_enabled.ts"), + } + })); + + let res = client.write_request( + "textDocument/hover", + json!({ + "textDocument": { + "uri": temp_dir.uri().join("main_disabled.ts").unwrap(), + }, + "position": { "line": 0, "character": 5 } + }), + ); + assert_eq!(res, json!(null)); + + let res = client.write_request( + "textDocument/hover", + json!({ + "textDocument": { + "uri": temp_dir.uri().join("main_enabled.ts").unwrap(), + }, + "position": { "line": 0, "character": 5 } + }), + ); + assert_eq!( + res, + json!({ + "contents": [ + { + "language": "typescript", + "value": "(method) DateConstructor.now(): number", + }, + "Returns the number of milliseconds elapsed since midnight, January 1, 1970 Universal Coordinated Time (UTC)." + ], + "range": { + "start": { "line": 0, "character": 5, }, + "end": { "line": 0, "character": 8, } + } + }) + ); + + client.shutdown(); +} + #[test] fn lsp_deno_task() { let context = TestContextBuilder::new().use_temp_cwd().build(); diff --git a/test_util/src/lsp.rs b/test_util/src/lsp.rs index 8a1f24b32..5369d6361 100644 --- a/test_util/src/lsp.rs +++ b/test_util/src/lsp.rs @@ -591,6 +591,7 @@ impl LspClientBuilder { writer, deno_dir, stderr_lines_rx, + supports_workspace_configuration: false, }) } } @@ -604,6 +605,7 @@ pub struct LspClient { deno_dir: TempDir, context: TestContext, stderr_lines_rx: Option<mpsc::Receiver<String>>, + supports_workspace_configuration: bool, } impl Drop for LspClient { @@ -689,9 +691,17 @@ impl LspClient { let mut builder = InitializeParamsBuilder::new(); builder.set_root_uri(self.context.temp_dir().uri()); do_build(&mut builder); - self.write_request("initialize", builder.build()); + let params: InitializeParams = builder.build(); + self.supports_workspace_configuration = match ¶ms.capabilities.workspace + { + Some(workspace) => workspace.configuration == Some(true), + _ => false, + }; + self.write_request("initialize", params); self.write_notification("initialized", json!({})); - self.handle_configuration_request(config); + if self.supports_workspace_configuration { + self.handle_configuration_request(config); + } } pub fn did_open(&mut self, params: Value) -> CollectedDiagnostics { @@ -712,7 +722,9 @@ impl LspClient { config: Value, ) -> CollectedDiagnostics { self.did_open_raw(params); - self.handle_configuration_request(config); + if self.supports_workspace_configuration { + self.handle_configuration_request(config); + } self.read_diagnostics() } |