diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index ebb815a6e..a257f6e7e 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -22,3 +22,5 @@ judgement to produce the clearest code organization. Keep the main logic as unnested as possible. Favour Rust's `let ... else` syntax to return early or continue a loop in the `else` clause, over `if let`. + +Always prefer importing with `use` instead of qualifying with `::`, unless specifically requested in these instructions or by the user, or you see existing `::` usages in the file you're editing. diff --git a/Cargo.lock b/Cargo.lock index 7f1dcf4a8..0f6dcd098 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -319,7 +319,6 @@ dependencies = [ "serde", "serde_json", "stdext", - "struct-field-names-as-array", "strum 0.26.2", "strum_macros 0.26.4", "tempfile", @@ -2744,26 +2743,6 @@ version = "0.10.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "73473c0e59e6d5812c5dfe2a064a6444949f089e20eec9a2e5506596494e4623" -[[package]] -name = "struct-field-names-as-array" -version = "0.3.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "22ba4bae771f9cc992c4f403636c54d2ef13acde6367583e99d06bb336674dd9" -dependencies = [ - "struct-field-names-as-array-derive", -] - -[[package]] -name = "struct-field-names-as-array-derive" -version = "0.3.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a2dbf8b57f3ce20e4bb171a11822b283bdfab6c4bb0fe64fa729f045f23a0938" -dependencies = [ - "proc-macro2", - "quote", - "syn 2.0.32", -] - [[package]] name = "strum" version = "0.24.1" diff --git a/crates/ark/Cargo.toml b/crates/ark/Cargo.toml index 3893dd3d1..2a8b1b6e3 100644 --- a/crates/ark/Cargo.toml +++ b/crates/ark/Cargo.toml @@ -53,7 +53,6 @@ url = "2.4.1" walkdir = "2" yaml-rust = "0.4.5" winsafe = { version = "0.0.19", features = ["kernel"] } -struct-field-names-as-array = "0.3.0" strum = "0.26.2" strum_macros = "0.26.2" futures = "0.3.30" diff --git a/crates/ark/src/lsp/config.rs b/crates/ark/src/lsp/config.rs index e7041b048..0628ec8c2 100644 --- a/crates/ark/src/lsp/config.rs +++ b/crates/ark/src/lsp/config.rs @@ -1,10 +1,77 @@ use serde::Deserialize; use serde::Serialize; -use struct_field_names_as_array::FieldNamesAsArray; +use serde_json::Value; -use crate::lsp; use crate::lsp::diagnostics::DiagnosticsConfig; +pub struct Setting { + pub key: &'static str, + pub set: fn(&mut T, Value), +} + +/// List of LSP settings for which clients can send `didChangeConfiguration` +/// notifications. We register our interest in watching over these settings in +/// our `initialized` handler. The `set` methods convert from a json `Value` to +/// the expected type, using a default value if the conversion fails. +/// +/// This array is for global settings. If the setting should only affect a given +/// document URI, add it to `DOCUMENT_SETTINGS` instead. +pub static GLOBAL_SETTINGS: &[Setting] = &[ + Setting { + key: "positron.r.diagnostics.enable", + set: |cfg, v| { + cfg.diagnostics.enable = v + .as_bool() + .unwrap_or_else(|| DiagnosticsConfig::default().enable) + }, + }, + Setting { + key: "positron.r.symbols.includeAssignmentsInBlocks", + set: |cfg, v| { + cfg.symbols.include_assignments_in_blocks = v + .as_bool() + .unwrap_or_else(|| SymbolsConfig::default().include_assignments_in_blocks) + }, + }, +]; + +/// These document settings are updated on a URI basis. Each document has its +/// own value of the setting. +pub static DOCUMENT_SETTINGS: &[Setting] = &[ + Setting { + key: "editor.insertSpaces", + set: |cfg, v| { + let default_style = IndentationConfig::default().indent_style; + cfg.indent.indent_style = if v + .as_bool() + .unwrap_or_else(|| default_style == IndentStyle::Space) + { + IndentStyle::Space + } else { + IndentStyle::Tab + } + }, + }, + Setting { + key: "editor.indentSize", + set: |cfg, v| { + cfg.indent.indent_size = v + .as_u64() + .map(|n| n as usize) + .unwrap_or_else(|| IndentationConfig::default().indent_size) + }, + }, + Setting { + key: "editor.tabSize", + set: |cfg, v| { + cfg.indent.tab_width = v + .as_u64() + .map(|n| n as usize) + .unwrap_or_else(|| IndentationConfig::default().tab_width) + }, + }, +]; + /// Configuration of the LSP #[derive(Clone, Default, Debug)] pub(crate) struct LspConfig { @@ -39,40 +106,12 @@ pub struct IndentationConfig { pub tab_width: usize, } -#[derive(Serialize, Deserialize, Clone, Debug)] +#[derive(PartialEq, Serialize, Deserialize, Clone, Debug)] pub enum IndentStyle { Tab, Space, } -/// VS Code representation of a document configuration -#[derive(Serialize, Deserialize, FieldNamesAsArray, Clone, Debug)] -pub(crate) struct VscDocumentConfig { - // DEV NOTE: Update `section_from_key()` method after adding a field - pub insert_spaces: bool, - pub indent_size: VscIndentSize, - pub tab_size: usize, -} - -#[derive(Serialize, Deserialize, FieldNamesAsArray, Clone, Debug)] -pub(crate) struct VscDiagnosticsConfig { - // DEV NOTE: Update `section_from_key()` method after adding a field - pub enable: bool, -} - -#[derive(Serialize, Deserialize, FieldNamesAsArray, Clone, Debug)] -pub(crate) struct VscSymbolsConfig { - // DEV NOTE: Update `section_from_key()` method after adding a field - pub include_assignments_in_blocks: bool, -} - -#[derive(Serialize, Deserialize, Clone, Debug)] -#[serde(untagged)] -pub(crate) enum VscIndentSize { - Alias(String), - Size(usize), -} - impl Default for SymbolsConfig { fn default() -> Self { Self { @@ -91,79 +130,6 @@ impl Default for IndentationConfig { } } -impl VscDocumentConfig { - pub(crate) fn section_from_key(key: &str) -> &str { - match key { - "insert_spaces" => "editor.insertSpaces", - "indent_size" => "editor.indentSize", - "tab_size" => "editor.tabSize", - _ => "unknown", // To be caught via downstream errors - } - } -} - -/// Convert from VS Code representation of a document config to our own -/// representation. Currently one-to-one. -impl From for DocumentConfig { - fn from(x: VscDocumentConfig) -> Self { - let indent_style = indent_style_from_lsp(x.insert_spaces); - - let indent_size = match x.indent_size { - VscIndentSize::Size(size) => size, - VscIndentSize::Alias(var) => { - if var == "tabSize" { - x.tab_size - } else { - lsp::log_warn!("Unknown indent alias {var}, using default"); - 2 - } - }, - }; - - Self { - indent: IndentationConfig { - indent_style, - indent_size, - tab_width: x.tab_size, - }, - } - } -} - -impl VscDiagnosticsConfig { - pub(crate) fn section_from_key(key: &str) -> &str { - match key { - "enable" => "positron.r.diagnostics.enable", - _ => "unknown", // To be caught via downstream errors - } - } -} - -impl From for DiagnosticsConfig { - fn from(value: VscDiagnosticsConfig) -> Self { - Self { - enable: value.enable, - } - } -} - -impl VscSymbolsConfig { - pub(crate) fn section_from_key(key: &str) -> &str { - match key { - "include_assignments_in_blocks" => "positron.r.symbols.includeAssignmentsInBlocks", - _ => "unknown", // To be caught via downstream errors - } - } -} - -impl From for SymbolsConfig { - fn from(value: VscSymbolsConfig) -> Self { - Self { - include_assignments_in_blocks: value.include_assignments_in_blocks, - } - } -} - pub(crate) fn indent_style_from_lsp(insert_spaces: bool) -> IndentStyle { if insert_spaces { IndentStyle::Space diff --git a/crates/ark/src/lsp/handlers.rs b/crates/ark/src/lsp/handlers.rs index 7cb115e7c..64bca86f3 100644 --- a/crates/ark/src/lsp/handlers.rs +++ b/crates/ark/src/lsp/handlers.rs @@ -9,7 +9,6 @@ use anyhow::anyhow; use serde_json::Value; use stdext::unwrap; use stdext::unwrap::IntoResult; -use struct_field_names_as_array::FieldNamesAsArray; use tower_lsp::lsp_types::CodeActionParams; use tower_lsp::lsp_types::CodeActionResponse; use tower_lsp::lsp_types::CompletionItem; @@ -46,9 +45,6 @@ use crate::lsp; use crate::lsp::code_action::code_actions; use crate::lsp::completions::provide_completions; use crate::lsp::completions::resolve_completion; -use crate::lsp::config::VscDiagnosticsConfig; -use crate::lsp::config::VscDocumentConfig; -use crate::lsp::config::VscSymbolsConfig; use crate::lsp::definitions::goto_definition; use crate::lsp::document_context::DocumentContext; use crate::lsp::encoding::convert_lsp_range_to_tree_sitter_range; @@ -106,22 +102,21 @@ pub(crate) async fn handle_initialized( // Note that some settings, such as editor indentation properties, may be // changed by extensions or by the user without changing the actual // underlying setting. Unfortunately we don't receive updates in that case. - let mut config_document_regs = collect_regs( - VscDocumentConfig::FIELD_NAMES_AS_ARRAY.to_vec(), - VscDocumentConfig::section_from_key, - ); - let mut config_symbols_regs: Vec = collect_regs( - VscSymbolsConfig::FIELD_NAMES_AS_ARRAY.to_vec(), - VscSymbolsConfig::section_from_key, - ); - let mut config_diagnostics_regs: Vec = collect_regs( - VscDiagnosticsConfig::FIELD_NAMES_AS_ARRAY.to_vec(), - VscDiagnosticsConfig::section_from_key, - ); - - regs.append(&mut config_document_regs); - regs.append(&mut config_symbols_regs); - regs.append(&mut config_diagnostics_regs); + + for setting in crate::lsp::config::GLOBAL_SETTINGS { + regs.push(Registration { + id: uuid::Uuid::new_v4().to_string(), + method: String::from("workspace/didChangeConfiguration"), + register_options: Some(serde_json::json!({ "section": setting.key })), + }); + } + for setting in crate::lsp::config::DOCUMENT_SETTINGS { + regs.push(Registration { + id: uuid::Uuid::new_v4().to_string(), + method: String::from("workspace/didChangeConfiguration"), + register_options: Some(serde_json::json!({ "section": setting.key })), + }); + } } client @@ -131,17 +126,6 @@ pub(crate) async fn handle_initialized( Ok(()) } -fn collect_regs(fields: Vec<&str>, into_section: impl Fn(&str) -> &str) -> Vec { - fields - .into_iter() - .map(|field| Registration { - id: uuid::Uuid::new_v4().to_string(), - method: String::from("workspace/didChangeConfiguration"), - register_options: Some(serde_json::json!({ "section": into_section(field) })), - }) - .collect() -} - #[tracing::instrument(level = "info", skip_all)] pub(crate) fn handle_symbol( params: WorkspaceSymbolParams, diff --git a/crates/ark/src/lsp/state_handlers.rs b/crates/ark/src/lsp/state_handlers.rs index 1bca6fcc0..2fb24ac55 100644 --- a/crates/ark/src/lsp/state_handlers.rs +++ b/crates/ark/src/lsp/state_handlers.rs @@ -8,11 +8,9 @@ use std::path::Path; use anyhow::anyhow; -use serde_json::Value; -use struct_field_names_as_array::FieldNamesAsArray; +use tower_lsp::lsp_types; use tower_lsp::lsp_types::CompletionOptions; use tower_lsp::lsp_types::CompletionOptionsCompletionItem; -use tower_lsp::lsp_types::ConfigurationItem; use tower_lsp::lsp_types::DidChangeConfigurationParams; use tower_lsp::lsp_types::DidChangeTextDocumentParams; use tower_lsp::lsp_types::DidCloseTextDocumentParams; @@ -42,12 +40,8 @@ use url::Url; use crate::lsp; use crate::lsp::capabilities::Capabilities; use crate::lsp::config::indent_style_from_lsp; -use crate::lsp::config::DocumentConfig; -use crate::lsp::config::SymbolsConfig; -use crate::lsp::config::VscDiagnosticsConfig; -use crate::lsp::config::VscDocumentConfig; -use crate::lsp::config::VscSymbolsConfig; -use crate::lsp::diagnostics::DiagnosticsConfig; +use crate::lsp::config::DOCUMENT_SETTINGS; +use crate::lsp::config::GLOBAL_SETTINGS; use crate::lsp::documents::Document; use crate::lsp::encoding::get_position_encoding_kind; use crate::lsp::indexer; @@ -289,50 +283,44 @@ async fn update_config( client: &tower_lsp::Client, state: &mut WorldState, ) -> anyhow::Result<()> { - let mut items: Vec = vec![]; + // Keep track of existing config to detect whether it was changed + let diagnostics_config = state.config.diagnostics.clone(); - let diagnostics_keys = VscDiagnosticsConfig::FIELD_NAMES_AS_ARRAY; - let mut diagnostics_items: Vec = diagnostics_keys + // Build the configuration request for global and document settings + let mut items: Vec<_> = vec![]; + + // This should be first because we first handle the global settings below, + // splitting them off the response array + let mut global_items: Vec<_> = GLOBAL_SETTINGS .iter() - .map(|key| ConfigurationItem { + .map(|mapping| lsp_types::ConfigurationItem { scope_uri: None, - section: Some(VscDiagnosticsConfig::section_from_key(key).into()), + section: Some(mapping.key.to_string()), }) .collect(); - items.append(&mut diagnostics_items); - let symbols_keys = VscSymbolsConfig::FIELD_NAMES_AS_ARRAY; - let mut symbols_items: Vec = symbols_keys + // For document items we create a n_uris * n_document_settings array that we'll + // handle by batch in a double loop over URIs and document settings + let mut document_items: Vec<_> = uris .iter() - .map(|key| ConfigurationItem { - scope_uri: None, - section: Some(VscSymbolsConfig::section_from_key(key).into()), + .flat_map(|uri| { + DOCUMENT_SETTINGS + .iter() + .map(|mapping| lsp_types::ConfigurationItem { + scope_uri: Some(uri.clone()), + section: Some(mapping.key.to_string()), + }) }) .collect(); - items.append(&mut symbols_items); - - // For document configs we collect all pairs of URIs and config keys of - // interest in a flat vector - let document_keys = VscDocumentConfig::FIELD_NAMES_AS_ARRAY; - let mut document_items: Vec = - itertools::iproduct!(uris.iter(), document_keys.iter()) - .map(|(uri, key)| ConfigurationItem { - scope_uri: Some(uri.clone()), - section: Some(VscDocumentConfig::section_from_key(key).into()), - }) - .collect(); + + // Concatenate everything into a flat array that we'll send in one request + items.append(&mut global_items); items.append(&mut document_items); - let configs = client.configuration(items).await?; + // The response better match the number of items we send in + let n_items = items.len(); - // We got the config items in a flat vector that's guaranteed to be - // ordered in the same way it was sent in. Be defensive and check that - // we've got the expected number of items before we process them chunk - // by chunk - let n_document_items = document_keys.len(); - let n_diagnostics_items = diagnostics_keys.len(); - let n_symbols_items = symbols_keys.len(); - let n_items = n_diagnostics_items + n_symbols_items + (n_document_items * uris.len()); + let mut configs = client.configuration(items).await?; if configs.len() != n_items { return Err(anyhow!( @@ -342,63 +330,32 @@ async fn update_config( )); } - let mut configs = configs.into_iter(); - - // --- Diagnostics - let keys = diagnostics_keys.into_iter(); - let items: Vec = configs.by_ref().take(n_diagnostics_items).collect(); - - // Create a new `serde_json::Value::Object` manually to convert it - // to a `VscDocumentConfig` with `from_value()`. This way serde_json - // can type-check the dynamic JSON value we got from the client. - let mut map = serde_json::Map::new(); - std::iter::zip(keys, items).for_each(|(key, item)| { - map.insert(key.into(), item); - }); - - // Deserialise the VS Code configuration - let config: VscDiagnosticsConfig = serde_json::from_value(serde_json::Value::Object(map))?; - let config: DiagnosticsConfig = config.into(); - - let changed = state.config.diagnostics != config; - state.config.diagnostics = config; + let document_configs = configs.split_off(GLOBAL_SETTINGS.len()); + let global_configs = configs; - if changed { - lsp::spawn_diagnostics_refresh_all(state.clone()); + for (mapping, value) in GLOBAL_SETTINGS.into_iter().zip(global_configs) { + (mapping.set)(&mut state.config, value); } - // --- Symbols - let keys = symbols_keys.into_iter(); - let items: Vec = configs.by_ref().take(n_symbols_items).collect(); + let mut remaining = document_configs; - let mut map = serde_json::Map::new(); - std::iter::zip(keys, items).for_each(|(key, item)| { - map.insert(key.into(), item); - }); - - let config: VscSymbolsConfig = serde_json::from_value(serde_json::Value::Object(map))?; - let config: SymbolsConfig = config.into(); - state.config.symbols = config; - - // --- Documents - // For each document, deserialise the vector of JSON values into a typed config for uri in uris.into_iter() { - let keys = document_keys.into_iter(); - let items: Vec = configs.by_ref().take(n_document_items).collect(); - - let mut map = serde_json::Map::new(); - std::iter::zip(keys, items).for_each(|(key, item)| { - map.insert(key.into(), item); - }); - - // Deserialise the VS Code configuration - let config: VscDocumentConfig = serde_json::from_value(serde_json::Value::Object(map))?; - - // Now convert the VS Code specific type into our own type - let config: DocumentConfig = config.into(); + // Need to juggle a bit because `split_off()` returns the tail of the + // split and updates the vector with the head + let tail = remaining.split_off(DOCUMENT_SETTINGS.len()); + let head = std::mem::replace(&mut remaining, tail); + + for (mapping, value) in DOCUMENT_SETTINGS.iter().zip(head) { + if let Ok(doc) = state.get_document_mut(&uri) { + (mapping.set)(&mut doc.config, value); + } + } + } - // Finally, update the document's config - state.get_document_mut(&uri)?.config = config; + // Refresh diagnostics if the configuration changed + if state.config.diagnostics != diagnostics_config { + tracing::info!("Refreshing diagnostics after configuration changed"); + lsp::spawn_diagnostics_refresh_all(state.clone()); } Ok(())