From ac71d876d7c9e3fec65e4739540bc60c8d75ee9a Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Wed, 24 Apr 2024 21:14:01 +0100 Subject: [PATCH] fix(lsp): inherit missing fmt and lint config from parent scopes (#23547) --- cli/lsp/config.rs | 154 ++++++++++++++++++++++----------- tests/integration/lsp_tests.rs | 76 ++++++++++++++++ 2 files changed, 178 insertions(+), 52 deletions(-) diff --git a/cli/lsp/config.rs b/cli/lsp/config.rs index c58d2e86d8..2bbd3ea7e9 100644 --- a/cli/lsp/config.rs +++ b/cli/lsp/config.rs @@ -1169,6 +1169,7 @@ impl ConfigData { async fn load( config_file_specifier: Option<&ModuleSpecifier>, scope: &ModuleSpecifier, + parent: Option<(&ModuleSpecifier, &ConfigData)>, settings: &Settings, file_fetcher: Option<&FileFetcher>, ) -> Self { @@ -1182,8 +1183,14 @@ impl ConfigData { " Resolved Deno configuration file: \"{}\"", config_file.specifier.as_str() ); - Self::load_inner(Some(config_file), scope, settings, file_fetcher) - .await + Self::load_inner( + Some(config_file), + scope, + parent, + settings, + file_fetcher, + ) + .await } Err(err) => { lsp_warn!( @@ -1192,7 +1199,7 @@ impl ConfigData { err ); let mut data = - Self::load_inner(None, scope, settings, file_fetcher).await; + Self::load_inner(None, scope, parent, settings, file_fetcher).await; data .watched_files .insert(specifier.clone(), ConfigWatchedFileType::DenoJson); @@ -1210,13 +1217,14 @@ impl ConfigData { } } } else { - Self::load_inner(None, scope, settings, file_fetcher).await + Self::load_inner(None, scope, parent, settings, file_fetcher).await } } async fn load_inner( config_file: Option, scope: &ModuleSpecifier, + parent: Option<(&ModuleSpecifier, &ConfigData)>, settings: &Settings, file_fetcher: Option<&FileFetcher>, ) -> Self { @@ -1238,45 +1246,76 @@ impl ConfigData { .or_insert(ConfigWatchedFileType::DenoJson); } - // Resolve some config file fields ahead of time - let fmt_options = config_file - .as_ref() - .and_then(|config_file| { - config_file - .to_fmt_config() - .and_then(|o| { - let base_path = config_file - .specifier - .to_file_path() - .map_err(|_| anyhow!("Invalid base path."))?; - FmtOptions::resolve(o, None, &base_path) - }) - .inspect_err(|err| { - lsp_warn!(" Couldn't read formatter configuration: {}", err) - }) - .ok() - }) - .unwrap_or_default(); - let lint_options = config_file - .as_ref() - .and_then(|config_file| { - config_file - .to_lint_config() - .and_then(|o| { - let base_path = config_file - .specifier - .to_file_path() - .map_err(|_| anyhow!("Invalid base path."))?; - LintOptions::resolve(o, None, &base_path) - }) - .inspect_err(|err| { - lsp_warn!(" Couldn't read lint configuration: {}", err) - }) - .ok() - }) - .unwrap_or_default(); - let lint_rules = - get_configured_rules(lint_options.rules.clone(), config_file.as_ref()); + let mut fmt_options = None; + if let Some((_, parent_data)) = parent { + let has_own_fmt_options = config_file + .as_ref() + .is_some_and(|config_file| config_file.json.fmt.is_some()); + if !has_own_fmt_options { + fmt_options = Some(parent_data.fmt_options.clone()) + } + } + let fmt_options = fmt_options.unwrap_or_else(|| { + config_file + .as_ref() + .and_then(|config_file| { + config_file + .to_fmt_config() + .and_then(|o| { + let base_path = config_file + .specifier + .to_file_path() + .map_err(|_| anyhow!("Invalid base path."))?; + FmtOptions::resolve(o, None, &base_path) + }) + .inspect_err(|err| { + lsp_warn!(" Couldn't read formatter configuration: {}", err) + }) + .ok() + }) + .map(Arc::new) + .unwrap_or_default() + }); + + let mut lint_options_rules = None; + if let Some((_, parent_data)) = parent { + let has_own_lint_options = config_file + .as_ref() + .is_some_and(|config_file| config_file.json.lint.is_some()); + if !has_own_lint_options { + lint_options_rules = Some(( + parent_data.lint_options.clone(), + parent_data.lint_rules.clone(), + )) + } + } + let (lint_options, lint_rules) = lint_options_rules.unwrap_or_else(|| { + let lint_options = config_file + .as_ref() + .and_then(|config_file| { + config_file + .to_lint_config() + .and_then(|o| { + let base_path = config_file + .specifier + .to_file_path() + .map_err(|_| anyhow!("Invalid base path."))?; + LintOptions::resolve(o, None, &base_path) + }) + .inspect_err(|err| { + lsp_warn!(" Couldn't read lint configuration: {}", err) + }) + .ok() + }) + .map(Arc::new) + .unwrap_or_default(); + let lint_rules = Arc::new(get_configured_rules( + lint_options.rules.clone(), + config_file.as_ref(), + )); + (lint_options, lint_rules) + }); + let vendor_dir = config_file.as_ref().and_then(|c| c.vendor_dir_path()); // Load lockfile @@ -1449,9 +1488,9 @@ impl ConfigData { ConfigData { config_file: config_file.map(Arc::new), - fmt_options: Arc::new(fmt_options), - lint_options: Arc::new(lint_options), - lint_rules: Arc::new(lint_rules), + fmt_options, + lint_options, + lint_rules, ts_config: Arc::new(ts_config), byonm, node_modules_dir, @@ -1601,6 +1640,7 @@ impl ConfigTree { ConfigData::load( Some(&config_uri), folder_uri, + None, settings, Some(file_fetcher), ) @@ -1616,17 +1656,20 @@ impl ConfigTree { || specifier.path().ends_with("/deno.jsonc") { if let Ok(scope) = specifier.join(".") { - let entry = scopes.entry(scope.clone()); - #[allow(clippy::map_entry)] - if matches!(entry, std::collections::btree_map::Entry::Vacant(_)) { + if !scopes.contains_key(&scope) { + let parent = scopes + .iter() + .rev() + .find(|(s, _)| scope.as_str().starts_with(s.as_str())); let data = ConfigData::load( Some(specifier), &scope, + parent, settings, Some(file_fetcher), ) .await; - entry.or_insert(data); + scopes.insert(scope, data); } } } @@ -1639,8 +1682,14 @@ impl ConfigTree { { scopes.insert( folder_uri.clone(), - ConfigData::load(None, folder_uri, settings, Some(file_fetcher)) - .await, + ConfigData::load( + None, + folder_uri, + None, + settings, + Some(file_fetcher), + ) + .await, ); } } @@ -1654,6 +1703,7 @@ impl ConfigTree { let data = ConfigData::load_inner( Some(config_file), &scope, + None, &Default::default(), None, ) diff --git a/tests/integration/lsp_tests.rs b/tests/integration/lsp_tests.rs index 248b02faa7..1c5cc31f22 100644 --- a/tests/integration/lsp_tests.rs +++ b/tests/integration/lsp_tests.rs @@ -11468,6 +11468,8 @@ fn lsp_deno_json_scopes_fmt_config() { }) .to_string(), ); + temp_dir.create_dir_all("project2/project3"); + temp_dir.write("project2/project3/deno.json", json!({}).to_string()); let mut client = context.new_lsp_command().build(); client.initialize_default(); client.did_open(json!({ @@ -11530,6 +11532,38 @@ fn lsp_deno_json_scopes_fmt_config() { "newText": "''", }]) ); + // `project2/project3/file.ts` should use the fmt settings from + // `project2/deno.json`, since `project2/project3/deno.json` has no fmt field. + client.did_open(json!({ + "textDocument": { + "uri": temp_dir.uri().join("project2/project3/file.ts").unwrap(), + "languageId": "typescript", + "version": 1, + "text": "console.log(\"\");\n", + }, + })); + let res = client.write_request( + "textDocument/formatting", + json!({ + "textDocument": { + "uri": temp_dir.uri().join("project2/project3/file.ts").unwrap(), + }, + "options": { + "tabSize": 2, + "insertSpaces": true, + }, + }), + ); + assert_eq!( + res, + json!([{ + "range": { + "start": { "line": 0, "character": 12 }, + "end": { "line": 0, "character": 14 }, + }, + "newText": "''", + }]) + ); client.shutdown(); } @@ -11561,6 +11595,8 @@ fn lsp_deno_json_scopes_lint_config() { }) .to_string(), ); + temp_dir.create_dir_all("project2/project3"); + temp_dir.write("project2/project3/deno.json", json!({}).to_string()); let mut client = context.new_lsp_command().build(); client.initialize_default(); let diagnostics = client.did_open(json!({ @@ -11629,6 +11665,46 @@ fn lsp_deno_json_scopes_lint_config() { "version": 1, }) ); + client.write_notification( + "textDocument/didClose", + json!({ + "textDocument": { + "uri": temp_dir.uri().join("project2/file.ts").unwrap(), + }, + }), + ); + // `project2/project3/file.ts` should use the lint settings from + // `project2/deno.json`, since `project2/project3/deno.json` has no lint + // field. + let diagnostics = client.did_open(json!({ + "textDocument": { + "uri": temp_dir.uri().join("project2/project3/file.ts").unwrap(), + "languageId": "typescript", + "version": 1, + "text": r#" + // TODO: Unused var + const snake_case_var = 1; + console.log(snake_case_var); + "#, + }, + })); + assert_eq!( + json!(diagnostics.messages_with_source("deno-lint")), + json!({ + "uri": temp_dir.uri().join("project2/project3/file.ts").unwrap(), + "diagnostics": [{ + "range": { + "start": { "line": 1, "character": 8 }, + "end": { "line": 1, "character": 27 }, + }, + "severity": 2, + "code": "ban-untagged-todo", + "source": "deno-lint", + "message": "TODO should be tagged with (@username) or (#issue)\nAdd a user tag or issue reference to the TODO comment, e.g. TODO(@djones), TODO(djones), TODO(#123)", + }], + "version": 1, + }) + ); client.shutdown(); }