Skip to content

Commit

Permalink
fix(lsp): properly handle disabled configuration requests (#20358)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
nayeemrmn authored Sep 2, 2023
1 parent 83426be commit 12f6ad3
Show file tree
Hide file tree
Showing 4 changed files with 111 additions and 16 deletions.
13 changes: 6 additions & 7 deletions cli/lsp/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
15 changes: 9 additions & 6 deletions cli/lsp/language_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -3125,32 +3127,33 @@ 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());
let specifier = inner
.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)
Expand Down
81 changes: 81 additions & 0 deletions cli/tests/integration/lsp_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
18 changes: 15 additions & 3 deletions test_util/src/lsp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,7 @@ impl LspClientBuilder {
writer,
deno_dir,
stderr_lines_rx,
supports_workspace_configuration: false,
})
}
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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 &params.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 {
Expand All @@ -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()
}

Expand Down

0 comments on commit 12f6ad3

Please sign in to comment.