Skip to content
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

fix(lsp): properly handle disabled configuration requests #20358

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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