From 7c5f33c1cd5ae3a258e689bc60a004e9397ef997 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Sun, 21 Jan 2024 15:06:42 +1100 Subject: [PATCH 01/25] ConfigTree --- Cargo.lock | 17 ++ crates/rust-analyzer/Cargo.toml | 2 + crates/rust-analyzer/src/config.rs | 1 + crates/rust-analyzer/src/config/tree.rs | 241 ++++++++++++++++++++++++ 4 files changed, 261 insertions(+) create mode 100644 crates/rust-analyzer/src/config/tree.rs diff --git a/Cargo.lock b/Cargo.lock index d5cebd5102de..6265568b25b7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -792,6 +792,12 @@ dependencies = [ "serde", ] +[[package]] +name = "indextree" +version = "4.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c40411d0e5c63ef1323c3d09ce5ec6d84d71531e18daed0743fccea279d7deb6" + [[package]] name = "inotify" version = "0.9.6" @@ -1586,6 +1592,7 @@ dependencies = [ "ide-db", "ide-ssr", "indexmap 2.0.0", + "indextree", "itertools", "la-arena 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)", "load-cargo", @@ -1609,6 +1616,7 @@ dependencies = [ "scip", "serde", "serde_json", + "slotmap", "sourcegen", "stdx", "syntax", @@ -1785,6 +1793,15 @@ dependencies = [ "lazy_static", ] +[[package]] +name = "slotmap" +version = "1.0.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "dbff4acf519f630b3a3ddcfaea6c06b42174d9a44bc70c620e9ed1649d58b82a" +dependencies = [ + "version_check", +] + [[package]] name = "smallvec" version = "1.10.0" diff --git a/crates/rust-analyzer/Cargo.toml b/crates/rust-analyzer/Cargo.toml index 7d6326448d8c..4567f382f89b 100644 --- a/crates/rust-analyzer/Cargo.toml +++ b/crates/rust-analyzer/Cargo.toml @@ -75,6 +75,8 @@ toolchain.workspace = true vfs-notify.workspace = true vfs.workspace = true la-arena.workspace = true +indextree = "4.6.0" +slotmap = "1.0.7" [target.'cfg(windows)'.dependencies] winapi = "0.3.9" diff --git a/crates/rust-analyzer/src/config.rs b/crates/rust-analyzer/src/config.rs index 3c65356bb721..9a1f44010e05 100644 --- a/crates/rust-analyzer/src/config.rs +++ b/crates/rust-analyzer/src/config.rs @@ -42,6 +42,7 @@ use crate::{ }; mod patch_old_style; +mod tree; // Conventions for configuration keys to preserve maximal extendability without breakage: // - Toggles (be it binary true/false or with more options in-between) should almost always suffix as `_enable` diff --git a/crates/rust-analyzer/src/config/tree.rs b/crates/rust-analyzer/src/config/tree.rs new file mode 100644 index 000000000000..fb470ae5fe94 --- /dev/null +++ b/crates/rust-analyzer/src/config/tree.rs @@ -0,0 +1,241 @@ +use indextree::NodeId; +use parking_lot::{RwLock, RwLockUpgradableReadGuard}; +use rustc_hash::FxHashMap; +use slotmap::SlotMap; +use std::sync::Arc; +use vfs::{FileId, Vfs}; + +use super::{ConfigInput, LocalConfigData, RootLocalConfigData}; + +pub struct ConcurrentConfigTree { + // One rwlock on the whole thing is probably fine. + // If you have 40,000 crates and you need to edit your config 200x/second, let us know. + rwlock: RwLock, +} + +pub enum ConfigTreeError { + Removed, + NonExistent, + Utf8(vfs::VfsPath, std::str::Utf8Error), + TomlParse(vfs::VfsPath, toml::de::Error), + TomlDeserialize { path: vfs::VfsPath, field: String, error: toml::de::Error }, +} + +/// Some rust-analyzer.toml files have changed, and/or the LSP client sent a new configuration. +pub struct ConfigChanges { + ra_toml_changes: Vec, + client_change: Option>, +} + +impl ConcurrentConfigTree { + pub fn apply_changes(&self, changes: ConfigChanges, vfs: &Vfs) -> Vec { + let mut errors = Vec::new(); + self.rwlock.write().apply_changes(changes, vfs, &mut errors); + errors + } + pub fn read_config(&self, file_id: FileId) -> Result, ConfigTreeError> { + let reader = self.rwlock.upgradable_read(); + if let Some(computed) = reader.read_only(file_id)? { + return Ok(computed); + } else { + let mut writer = RwLockUpgradableReadGuard::upgrade(reader); + return writer.compute(file_id); + } + } +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] +enum ConfigSource { + ClientConfig, + RaToml(FileId), +} + +slotmap::new_key_type! { + struct ComputedIdx; +} + +struct ConfigNode { + src: ConfigSource, + input: Arc, + computed: ComputedIdx, +} + +struct ConfigTree { + tree: indextree::Arena, + client_config: NodeId, + ra_file_id_map: FxHashMap, + computed: SlotMap>>, +} + +fn parse_toml( + file_id: FileId, + vfs: &Vfs, + scratch: &mut Vec<(String, toml::de::Error)>, + errors: &mut Vec, +) -> Option> { + let content = vfs.file_contents(file_id); + let path = vfs.file_path(file_id); + let content_str = match std::str::from_utf8(content) { + Err(e) => { + tracing::error!("non-UTF8 TOML content for {path}: {e}"); + errors.push(ConfigTreeError::Utf8(path, e)); + return None; + } + Ok(str) => str, + }; + let table = match toml::from_str(content_str) { + Ok(table) => table, + Err(e) => { + errors.push(ConfigTreeError::TomlParse(path, e)); + return None; + } + }; + let input = Arc::new(ConfigInput::from_toml(table, scratch)); + scratch.drain(..).for_each(|(field, error)| { + errors.push(ConfigTreeError::TomlDeserialize { path: path.clone(), field, error }); + }); + Some(input) +} + +impl ConfigTree { + fn new() -> Self { + let mut tree = indextree::Arena::new(); + let mut computed = SlotMap::default(); + let client_config = tree.new_node(ConfigNode { + src: ConfigSource::ClientConfig, + input: Arc::new(ConfigInput::default()), + computed: computed.insert(Option::>::None), + }); + Self { client_config, ra_file_id_map: FxHashMap::default(), tree, computed } + } + + fn read_only(&self, file_id: FileId) -> Result>, ConfigTreeError> { + let node_id = *self.ra_file_id_map.get(&file_id).ok_or(ConfigTreeError::NonExistent)?; + // indextree does not check this during get(), probably for perf reasons? + // get() is apparently only a bounds check + if node_id.is_removed(&self.tree) { + return Err(ConfigTreeError::Removed); + } + let node = self.tree.get(node_id).ok_or(ConfigTreeError::NonExistent)?.get(); + Ok(self.computed[node.computed].clone()) + } + + fn compute(&mut self, file_id: FileId) -> Result, ConfigTreeError> { + let node_id = *self.ra_file_id_map.get(&file_id).ok_or(ConfigTreeError::NonExistent)?; + self.compute_inner(node_id) + } + fn compute_inner(&mut self, node_id: NodeId) -> Result, ConfigTreeError> { + if node_id.is_removed(&self.tree) { + return Err(ConfigTreeError::Removed); + } + let node = self.tree.get(node_id).ok_or(ConfigTreeError::NonExistent)?.get(); + let idx = node.computed; + let slot = &mut self.computed[idx]; + if let Some(slot) = slot { + Ok(slot.clone()) + } else { + let self_computed = if let Some(parent) = + self.tree.get(node_id).ok_or(ConfigTreeError::NonExistent)?.parent() + { + let self_input = node.input.clone(); + let parent_computed = self.compute_inner(parent)?; + Arc::new(parent_computed.clone_with_overrides(self_input.local.clone())) + } else { + // We have hit a root node + let self_input = node.input.clone(); + let root_local = RootLocalConfigData::from_root_input(self_input.local.clone()); + Arc::new(root_local.0) + }; + // Get a new &mut slot because self.compute(parent) also gets mut access + let slot = &mut self.computed[idx]; + slot.replace(self_computed.clone()); + Ok(self_computed) + } + } + + fn insert_toml(&mut self, file_id: FileId, input: Arc) -> NodeId { + let computed = self.computed.insert(None); + let node = + self.tree.new_node(ConfigNode { src: ConfigSource::RaToml(file_id), input, computed }); + self.ra_file_id_map.insert(file_id, node); + node + } + + fn update_toml( + &mut self, + file_id: FileId, + input: Arc, + ) -> Result<(), ConfigTreeError> { + let Some(node_id) = self.ra_file_id_map.get(&file_id).cloned() else { + return Err(ConfigTreeError::NonExistent); + }; + if node_id.is_removed(&self.tree) { + return Err(ConfigTreeError::Removed); + } + let node = self.tree.get_mut(node_id).ok_or(ConfigTreeError::NonExistent)?; + node.get_mut().input = input; + + self.invalidate_subtree(node_id); + Ok(()) + } + + fn invalidate_subtree(&mut self, node_id: NodeId) { + // + // This is why we need the computed values outside the indextree: we iterate immutably + // over the tree while holding a &mut self.computed. + node_id.descendants(&self.tree).for_each(|x| { + let Some(desc) = self.tree.get(x) else { + return; + }; + self.computed.get_mut(desc.get().computed).take(); + }); + } + + fn remove_toml(&mut self, file_id: FileId) -> Option<()> { + let node_id = self.ra_file_id_map.remove(&file_id)?; + if node_id.is_removed(&self.tree) { + return None; + } + let node = self.tree.get(node_id)?; + let idx = node.get().computed; + let _ = self.computed.remove(idx); + self.invalidate_subtree(node_id); + Some(()) + } + + fn apply_changes( + &mut self, + changes: ConfigChanges, + vfs: &Vfs, + errors: &mut Vec, + ) { + let mut scratch_errors = Vec::new(); + let ConfigChanges { client_change, ra_toml_changes } = changes; + if let Some(change) = client_change { + let node = + self.tree.get_mut(self.client_config).expect("client_config node should exist"); + node.get_mut().input = change; + self.invalidate_subtree(self.client_config); + } + for change in ra_toml_changes { + // turn and face the strain + match change.change_kind { + vfs::ChangeKind::Create => { + let input = parse_toml(change.file_id, vfs, &mut scratch_errors, errors) + .unwrap_or_default(); + let _new_node = self.insert_toml(change.file_id, input); + } + vfs::ChangeKind::Modify => { + let input = parse_toml(change.file_id, vfs, &mut scratch_errors, errors) + .unwrap_or_default(); + if let Err(e) = self.update_toml(change.file_id, input) { + errors.push(e); + } + } + vfs::ChangeKind::Delete => { + self.remove_toml(change.file_id); + } + } + } + } +} From 9c506a963a0d8c828cec6c2fd385d6a286d386ca Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Tue, 23 Jan 2024 20:49:59 +1100 Subject: [PATCH 02/25] Start building the tree construction --- crates/rust-analyzer/src/config/tree.rs | 80 +++++++++++++++++++++---- 1 file changed, 70 insertions(+), 10 deletions(-) diff --git a/crates/rust-analyzer/src/config/tree.rs b/crates/rust-analyzer/src/config/tree.rs index fb470ae5fe94..bc2cb33563eb 100644 --- a/crates/rust-analyzer/src/config/tree.rs +++ b/crates/rust-analyzer/src/config/tree.rs @@ -24,7 +24,47 @@ pub enum ConfigTreeError { /// Some rust-analyzer.toml files have changed, and/or the LSP client sent a new configuration. pub struct ConfigChanges { ra_toml_changes: Vec, + xdg_config_change: Option>, client_change: Option>, + parent_changes: Vec, +} + +pub struct ConfigParentChange { + /// The config node in question + pub node: FileId, + pub parent: ConfigParent, +} + +pub enum ConfigParent { + /// The node is now a root in its own right, but still inherits from the config in XDG_CONFIG_HOME + /// etc + UserDefault, + /// The node is now a child of another rust-analyzer.toml. Even if that one is a non-existent + /// file, it's fine. + /// + /// + /// ```ignore,text + /// /project_root/ + /// rust-analyzer.toml + /// crate_a/ + /// crate_b/ + /// rust-analyzer.toml + /// + /// ``` + /// + /// ```ignore + /// // imagine set_file_contents = vfs.set_file_contents() and then get the vfs.file_id() + /// + /// let root = vfs.set_file_contents("/project_root/rust-analyzer.toml", Some("...")); + /// let crate_a = vfs.set_file_contents("/project_root/crate_a/rust-analyzer.toml", None); + /// let crate_b = vfs.set_file_contents("/project_root/crate_a/crate_b/rust-analyzer.toml", Some("...")); + /// let config_parent_changes = [ + /// ConfigParentChange { node: root, parent: ConfigParent::UserDefault }, + /// ConfigParentChange { node: crate_a, parent: ConfigParent::Parent(root) }, + /// ConfigParentChange { node: crate_b, parent: ConfigParent::Parent(crate_a) } + /// ]; + /// ``` + Parent(FileId), } impl ConcurrentConfigTree { @@ -56,13 +96,15 @@ slotmap::new_key_type! { struct ConfigNode { src: ConfigSource, + // TODO: make option input: Arc, computed: ComputedIdx, } struct ConfigTree { tree: indextree::Arena, - client_config: NodeId, + client_config: Arc, + xdg_config_node_id: NodeId, ra_file_id_map: FxHashMap, computed: SlotMap>>, } @@ -98,15 +140,24 @@ fn parse_toml( } impl ConfigTree { - fn new() -> Self { + fn new(xdg_config_file_id: FileId) -> Self { let mut tree = indextree::Arena::new(); let mut computed = SlotMap::default(); - let client_config = tree.new_node(ConfigNode { - src: ConfigSource::ClientConfig, + let mut ra_file_id_map = FxHashMap::default(); + let xdg_config = tree.new_node(ConfigNode { + src: ConfigSource::RaToml(xdg_config_file_id), input: Arc::new(ConfigInput::default()), computed: computed.insert(Option::>::None), }); - Self { client_config, ra_file_id_map: FxHashMap::default(), tree, computed } + ra_file_id_map.insert(xdg_config_file_id, xdg_config); + + Self { + client_config: Arc::new(Default::default()), + xdg_config_node_id: xdg_config, + ra_file_id_map, + tree, + computed, + } } fn read_only(&self, file_id: FileId) -> Result>, ConfigTreeError> { @@ -205,18 +256,27 @@ impl ConfigTree { fn apply_changes( &mut self, - changes: ConfigChanges, + mut changes: ConfigChanges, vfs: &Vfs, errors: &mut Vec, ) { let mut scratch_errors = Vec::new(); - let ConfigChanges { client_change, ra_toml_changes } = changes; + let ConfigChanges { client_change, ra_toml_changes, xdg_config_change, parent_changes } = + changes; + if let Some(change) = client_change { - let node = - self.tree.get_mut(self.client_config).expect("client_config node should exist"); + self.client_config = change; + } + + if let Some(change) = xdg_config_change { + let node = self + .tree + .get_mut(self.xdg_config_node_id) + .expect("client_config node should exist"); node.get_mut().input = change; - self.invalidate_subtree(self.client_config); + self.invalidate_subtree(self.xdg_config_node_id); } + for change in ra_toml_changes { // turn and face the strain match change.change_kind { From c442ea1acbd74b2022fad9896856a98bb11cb5c4 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Tue, 23 Jan 2024 20:58:04 +1100 Subject: [PATCH 03/25] Store empty input as None --- crates/rust-analyzer/src/config/tree.rs | 52 ++++++++++++++----------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/crates/rust-analyzer/src/config/tree.rs b/crates/rust-analyzer/src/config/tree.rs index bc2cb33563eb..32e65afa68a5 100644 --- a/crates/rust-analyzer/src/config/tree.rs +++ b/crates/rust-analyzer/src/config/tree.rs @@ -24,8 +24,14 @@ pub enum ConfigTreeError { /// Some rust-analyzer.toml files have changed, and/or the LSP client sent a new configuration. pub struct ConfigChanges { ra_toml_changes: Vec, - xdg_config_change: Option>, - client_change: Option>, + /// - `None` => no change + /// - `Some(None)` => the XDG_CONFIG_HOME rust-analyzer.toml file was deleted + /// - `Some(Some(...))` => the XDG_CONFIG_HOME rust-analyzer.toml file was updated + xdg_config_change: Option>>, + /// - `None` => no change + /// - `Some(None)` => the client config was removed / reset or something + /// - `Some(Some(...))` => the client config was updated + client_change: Option>>, parent_changes: Vec, } @@ -96,14 +102,13 @@ slotmap::new_key_type! { struct ConfigNode { src: ConfigSource, - // TODO: make option - input: Arc, + input: Option>, computed: ComputedIdx, } struct ConfigTree { tree: indextree::Arena, - client_config: Arc, + client_config: Option>, xdg_config_node_id: NodeId, ra_file_id_map: FxHashMap, computed: SlotMap>>, @@ -117,6 +122,9 @@ fn parse_toml( ) -> Option> { let content = vfs.file_contents(file_id); let path = vfs.file_path(file_id); + if content.is_empty() { + return None; + } let content_str = match std::str::from_utf8(content) { Err(e) => { tracing::error!("non-UTF8 TOML content for {path}: {e}"); @@ -146,18 +154,12 @@ impl ConfigTree { let mut ra_file_id_map = FxHashMap::default(); let xdg_config = tree.new_node(ConfigNode { src: ConfigSource::RaToml(xdg_config_file_id), - input: Arc::new(ConfigInput::default()), + input: None, computed: computed.insert(Option::>::None), }); ra_file_id_map.insert(xdg_config_file_id, xdg_config); - Self { - client_config: Arc::new(Default::default()), - xdg_config_node_id: xdg_config, - ra_file_id_map, - tree, - computed, - } + Self { client_config: None, xdg_config_node_id: xdg_config, ra_file_id_map, tree, computed } } fn read_only(&self, file_id: FileId) -> Result>, ConfigTreeError> { @@ -190,12 +192,20 @@ impl ConfigTree { { let self_input = node.input.clone(); let parent_computed = self.compute_inner(parent)?; - Arc::new(parent_computed.clone_with_overrides(self_input.local.clone())) + if let Some(input) = self_input.as_deref() { + Arc::new(parent_computed.clone_with_overrides(input.local.clone())) + } else { + parent_computed + } } else { // We have hit a root node let self_input = node.input.clone(); - let root_local = RootLocalConfigData::from_root_input(self_input.local.clone()); - Arc::new(root_local.0) + if let Some(input) = self_input.as_deref() { + let root_local = RootLocalConfigData::from_root_input(input.local.clone()); + Arc::new(root_local.0) + } else { + Arc::new(LocalConfigData::default()) + } }; // Get a new &mut slot because self.compute(parent) also gets mut access let slot = &mut self.computed[idx]; @@ -204,7 +214,7 @@ impl ConfigTree { } } - fn insert_toml(&mut self, file_id: FileId, input: Arc) -> NodeId { + fn insert_toml(&mut self, file_id: FileId, input: Option>) -> NodeId { let computed = self.computed.insert(None); let node = self.tree.new_node(ConfigNode { src: ConfigSource::RaToml(file_id), input, computed }); @@ -215,7 +225,7 @@ impl ConfigTree { fn update_toml( &mut self, file_id: FileId, - input: Arc, + input: Option>, ) -> Result<(), ConfigTreeError> { let Some(node_id) = self.ra_file_id_map.get(&file_id).cloned() else { return Err(ConfigTreeError::NonExistent); @@ -281,13 +291,11 @@ impl ConfigTree { // turn and face the strain match change.change_kind { vfs::ChangeKind::Create => { - let input = parse_toml(change.file_id, vfs, &mut scratch_errors, errors) - .unwrap_or_default(); + let input = parse_toml(change.file_id, vfs, &mut scratch_errors, errors); let _new_node = self.insert_toml(change.file_id, input); } vfs::ChangeKind::Modify => { - let input = parse_toml(change.file_id, vfs, &mut scratch_errors, errors) - .unwrap_or_default(); + let input = parse_toml(change.file_id, vfs, &mut scratch_errors, errors); if let Err(e) = self.update_toml(change.file_id, input) { errors.push(e); } From 18ff646e80991b50a6d38c826d929dfca42351b2 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Tue, 23 Jan 2024 22:02:41 +1100 Subject: [PATCH 04/25] config tree gets a tree shape --- crates/rust-analyzer/src/config/tree.rs | 144 ++++++++++++++++++++++-- crates/vfs/src/lib.rs | 13 ++- 2 files changed, 143 insertions(+), 14 deletions(-) diff --git a/crates/rust-analyzer/src/config/tree.rs b/crates/rust-analyzer/src/config/tree.rs index 32e65afa68a5..643e3bc5a085 100644 --- a/crates/rust-analyzer/src/config/tree.rs +++ b/crates/rust-analyzer/src/config/tree.rs @@ -2,7 +2,7 @@ use indextree::NodeId; use parking_lot::{RwLock, RwLockUpgradableReadGuard}; use rustc_hash::FxHashMap; use slotmap::SlotMap; -use std::sync::Arc; +use std::{fmt, sync::Arc}; use vfs::{FileId, Vfs}; use super::{ConfigInput, LocalConfigData, RootLocalConfigData}; @@ -13,6 +13,19 @@ pub struct ConcurrentConfigTree { rwlock: RwLock, } +impl ConcurrentConfigTree { + fn new(config_tree: ConfigTree) -> Self { + Self { rwlock: RwLock::new(config_tree) } + } +} + +impl fmt::Debug for ConcurrentConfigTree { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.rwlock.read().fmt(f) + } +} + +#[derive(Debug)] pub enum ConfigTreeError { Removed, NonExistent, @@ -35,12 +48,14 @@ pub struct ConfigChanges { parent_changes: Vec, } +#[derive(Debug)] pub struct ConfigParentChange { /// The config node in question - pub node: FileId, + pub file_id: FileId, pub parent: ConfigParent, } +#[derive(Debug)] pub enum ConfigParent { /// The node is now a root in its own right, but still inherits from the config in XDG_CONFIG_HOME /// etc @@ -100,6 +115,7 @@ slotmap::new_key_type! { struct ComputedIdx; } +#[derive(Debug)] struct ConfigNode { src: ConfigSource, input: Option>, @@ -190,6 +206,7 @@ impl ConfigTree { let self_computed = if let Some(parent) = self.tree.get(node_id).ok_or(ConfigTreeError::NonExistent)?.parent() { + tracing::trace!("looking at parent of {node_id:?} -> {parent:?}"); let self_input = node.input.clone(); let parent_computed = self.compute_inner(parent)?; if let Some(input) = self_input.as_deref() { @@ -198,6 +215,7 @@ impl ConfigTree { parent_computed } } else { + tracing::trace!("{node_id:?} is a root node"); // We have hit a root node let self_input = node.input.clone(); if let Some(input) = self_input.as_deref() { @@ -218,7 +236,9 @@ impl ConfigTree { let computed = self.computed.insert(None); let node = self.tree.new_node(ConfigNode { src: ConfigSource::RaToml(file_id), input, computed }); - self.ra_file_id_map.insert(file_id, node); + if let Some(_removed) = self.ra_file_id_map.insert(file_id, node) { + panic!("ERROR: node should not have existed for {file_id:?} but it did"); + } node } @@ -226,9 +246,10 @@ impl ConfigTree { &mut self, file_id: FileId, input: Option>, - ) -> Result<(), ConfigTreeError> { + ) -> Result { let Some(node_id) = self.ra_file_id_map.get(&file_id).cloned() else { - return Err(ConfigTreeError::NonExistent); + let node_id = self.insert_toml(file_id, input); + return Ok(node_id); }; if node_id.is_removed(&self.tree) { return Err(ConfigTreeError::Removed); @@ -237,7 +258,14 @@ impl ConfigTree { node.get_mut().input = input; self.invalidate_subtree(node_id); - Ok(()) + Ok(node_id) + } + + fn ensure_node(&mut self, file_id: FileId) -> NodeId { + let Some(&node_id) = self.ra_file_id_map.get(&file_id) else { + return self.insert_toml(file_id, None); + }; + node_id } fn invalidate_subtree(&mut self, node_id: NodeId) { @@ -253,20 +281,19 @@ impl ConfigTree { } fn remove_toml(&mut self, file_id: FileId) -> Option<()> { - let node_id = self.ra_file_id_map.remove(&file_id)?; + let node_id = *self.ra_file_id_map.get(&file_id)?; if node_id.is_removed(&self.tree) { return None; } - let node = self.tree.get(node_id)?; - let idx = node.get().computed; - let _ = self.computed.remove(idx); + let node = self.tree.get_mut(node_id)?.get_mut(); + node.input = None; self.invalidate_subtree(node_id); Some(()) } fn apply_changes( &mut self, - mut changes: ConfigChanges, + changes: ConfigChanges, vfs: &Vfs, errors: &mut Vec, ) { @@ -287,12 +314,23 @@ impl ConfigTree { self.invalidate_subtree(self.xdg_config_node_id); } + for ConfigParentChange { file_id, parent } in parent_changes { + let node_id = self.ensure_node(file_id); + let parent_node_id = match parent { + ConfigParent::UserDefault => self.xdg_config_node_id, + ConfigParent::Parent(parent_file_id) => self.ensure_node(parent_file_id), + }; + // order of children within the parent node does not matter + tracing::trace!("appending child {node_id:?} to {parent_node_id:?}"); + parent_node_id.append(node_id, &mut self.tree); + } + for change in ra_toml_changes { // turn and face the strain match change.change_kind { vfs::ChangeKind::Create => { let input = parse_toml(change.file_id, vfs, &mut scratch_errors, errors); - let _new_node = self.insert_toml(change.file_id, input); + let _new_node = self.update_toml(change.file_id, input); } vfs::ChangeKind::Modify => { let input = parse_toml(change.file_id, vfs, &mut scratch_errors, errors); @@ -307,3 +345,85 @@ impl ConfigTree { } } } + +impl fmt::Debug for ConfigTree { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.tree.fmt(f) + } +} + +#[cfg(test)] +mod tests { + use std::path::PathBuf; + + use vfs::{AbsPathBuf, VfsPath}; + + fn alloc_file_id(vfs: &mut Vfs, s: &str) -> FileId { + let abs_path = AbsPathBuf::try_from(PathBuf::new().join(s)).unwrap(); + + let vfs_path = VfsPath::from(abs_path); + // FIXME: the vfs should expose this functionality more simply. + // We shouldn't have to clone the vfs path just to get a FileId. + let file_id = vfs.alloc_file_id(vfs_path); + vfs.set_file_id_contents(file_id, None); + file_id + } + + fn alloc_config(vfs: &mut Vfs, s: &str, config: &str) -> FileId { + let abs_path = AbsPathBuf::try_from(PathBuf::new().join(s)).unwrap(); + + let vfs_path = VfsPath::from(abs_path); + // FIXME: the vfs should expose this functionality more simply. + // We shouldn't have to clone the vfs path just to get a FileId. + let file_id = vfs.alloc_file_id(vfs_path); + vfs.set_file_id_contents(file_id, Some(config.to_string().into_bytes())); + file_id + } + + use super::*; + #[test] + fn basic() { + let mut vfs = Vfs::default(); + let xdg_config_file_id = + alloc_file_id(&mut vfs, "/home/.config/rust-analyzer/rust-analyzer.toml"); + let config_tree = ConcurrentConfigTree::new(ConfigTree::new(xdg_config_file_id)); + + let root = alloc_config( + &mut vfs, + "/root/rust-analyzer.toml", + r#" + [completion.autoself] + enable = false + "#, + ); + + let crate_a = alloc_config( + &mut vfs, + "/root/crate_a/rust-analyzer.toml", + r#" + [completion.autoimport] + enable = false + "#, + ); + + let parent_changes = + vec![ConfigParentChange { file_id: crate_a, parent: ConfigParent::Parent(root) }]; + + let changes = ConfigChanges { + // Normally you will filter these! + ra_toml_changes: vfs.take_changes(), + parent_changes, + xdg_config_change: None, + client_change: None, + }; + + dbg!(config_tree.apply_changes(changes, &vfs)); + dbg!(&config_tree); + + let local = config_tree.read_config(crate_a).unwrap(); + // from root + assert_eq!(local.completion_autoself_enable, false); + // from crate_a + assert_eq!(local.completion_autoimport_enable, false); + } +} diff --git a/crates/vfs/src/lib.rs b/crates/vfs/src/lib.rs index 06004adad34a..65882f72f5a7 100644 --- a/crates/vfs/src/lib.rs +++ b/crates/vfs/src/lib.rs @@ -133,6 +133,11 @@ impl Vfs { self.get(file_id).as_deref().unwrap() } + /// File content, but returns None if the file was deleted. + pub fn file_contents_opt(&self, file_id: FileId) -> Option<&[u8]> { + self.get(file_id).as_deref() + } + /// Returns the overall memory usage for the stored files. pub fn memory_usage(&self) -> usize { self.data.iter().flatten().map(|d| d.capacity()).sum() @@ -157,8 +162,12 @@ impl Vfs { /// /// If the path does not currently exists in the `Vfs`, allocates a new /// [`FileId`] for it. - pub fn set_file_contents(&mut self, path: VfsPath, mut contents: Option>) -> bool { + pub fn set_file_contents(&mut self, path: VfsPath, contents: Option>) -> bool { let file_id = self.alloc_file_id(path); + self.set_file_id_contents(file_id, contents) + } + + pub fn set_file_id_contents(&mut self, file_id: FileId, mut contents: Option>) -> bool { let change_kind = match (self.get(file_id), &contents) { (None, None) => return false, (Some(old), Some(new)) if old == new => return false, @@ -196,7 +205,7 @@ impl Vfs { /// - Else, returns `path`'s id. /// /// Does not record a change. - fn alloc_file_id(&mut self, path: VfsPath) -> FileId { + pub fn alloc_file_id(&mut self, path: VfsPath) -> FileId { let file_id = self.interner.intern(path); let idx = file_id.0 as usize; let len = self.data.len().max(idx + 1); From 47c7d45731be941430a399869b361751ce789b81 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Tue, 23 Jan 2024 22:22:33 +1100 Subject: [PATCH 05/25] Make the client config override everything --- crates/rust-analyzer/src/config/tree.rs | 38 ++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/crates/rust-analyzer/src/config/tree.rs b/crates/rust-analyzer/src/config/tree.rs index 643e3bc5a085..96a6c4e21d67 100644 --- a/crates/rust-analyzer/src/config/tree.rs +++ b/crates/rust-analyzer/src/config/tree.rs @@ -180,18 +180,38 @@ impl ConfigTree { fn read_only(&self, file_id: FileId) -> Result>, ConfigTreeError> { let node_id = *self.ra_file_id_map.get(&file_id).ok_or(ConfigTreeError::NonExistent)?; + let stored = self.read_only_inner(node_id)?; + Ok(stored.map(|stored| { + if let Some(client_config) = self.client_config.as_deref() { + stored.clone_with_overrides(client_config.local.clone()).into() + } else { + stored + } + })) + } + + fn read_only_inner( + &self, + node_id: NodeId, + ) -> Result>, ConfigTreeError> { // indextree does not check this during get(), probably for perf reasons? // get() is apparently only a bounds check if node_id.is_removed(&self.tree) { return Err(ConfigTreeError::Removed); } let node = self.tree.get(node_id).ok_or(ConfigTreeError::NonExistent)?.get(); - Ok(self.computed[node.computed].clone()) + let stored = self.computed[node.computed].clone(); + Ok(stored) } fn compute(&mut self, file_id: FileId) -> Result, ConfigTreeError> { let node_id = *self.ra_file_id_map.get(&file_id).ok_or(ConfigTreeError::NonExistent)?; - self.compute_inner(node_id) + let computed = self.compute_inner(node_id)?; + Ok(if let Some(client_config) = self.client_config.as_deref() { + computed.clone_with_overrides(client_config.local.clone()).into() + } else { + computed + }) } fn compute_inner(&mut self, node_id: NodeId) -> Result, ConfigTreeError> { if node_id.is_removed(&self.tree) { @@ -403,6 +423,9 @@ mod tests { r#" [completion.autoimport] enable = false + # will be overridden by client + [semanticHighlighting.strings] + enable = true "#, ); @@ -413,8 +436,13 @@ mod tests { // Normally you will filter these! ra_toml_changes: vfs.take_changes(), parent_changes, - xdg_config_change: None, - client_change: None, + client_change: Some(Some(Arc::new(ConfigInput { + local: crate::config::LocalConfigInput { + semanticHighlighting_strings_enable: Some(false), + ..Default::default() + }, + ..Default::default() + }))), }; dbg!(config_tree.apply_changes(changes, &vfs)); @@ -425,5 +453,7 @@ mod tests { assert_eq!(local.completion_autoself_enable, false); // from crate_a assert_eq!(local.completion_autoimport_enable, false); + // from client + assert_eq!(local.semanticHighlighting_strings_enable, false); } } From 69bdf4a85de3421a0978159e29df0489c39dae34 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Tue, 23 Jan 2024 22:22:51 +1100 Subject: [PATCH 06/25] Add failing test for the xdg config being inherited --- crates/rust-analyzer/src/config.rs | 2 +- crates/rust-analyzer/src/config/tree.rs | 45 ++++++++++++++++--------- 2 files changed, 31 insertions(+), 16 deletions(-) diff --git a/crates/rust-analyzer/src/config.rs b/crates/rust-analyzer/src/config.rs index 9a1f44010e05..5fce21ddce77 100644 --- a/crates/rust-analyzer/src/config.rs +++ b/crates/rust-analyzer/src/config.rs @@ -2195,7 +2195,7 @@ enum AdjustmentHintsDef { Reborrow, } -#[derive(Serialize, Deserialize, Debug, Clone)] +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] #[serde(untagged)] enum DiscriminantHintsDef { #[serde(with = "true_or_always")] diff --git a/crates/rust-analyzer/src/config/tree.rs b/crates/rust-analyzer/src/config/tree.rs index 96a6c4e21d67..adddb07038ab 100644 --- a/crates/rust-analyzer/src/config/tree.rs +++ b/crates/rust-analyzer/src/config/tree.rs @@ -38,10 +38,6 @@ pub enum ConfigTreeError { pub struct ConfigChanges { ra_toml_changes: Vec, /// - `None` => no change - /// - `Some(None)` => the XDG_CONFIG_HOME rust-analyzer.toml file was deleted - /// - `Some(Some(...))` => the XDG_CONFIG_HOME rust-analyzer.toml file was updated - xdg_config_change: Option>>, - /// - `None` => no change /// - `Some(None)` => the client config was removed / reset or something /// - `Some(Some(...))` => the client config was updated client_change: Option>>, @@ -318,22 +314,12 @@ impl ConfigTree { errors: &mut Vec, ) { let mut scratch_errors = Vec::new(); - let ConfigChanges { client_change, ra_toml_changes, xdg_config_change, parent_changes } = - changes; + let ConfigChanges { client_change, ra_toml_changes, parent_changes } = changes; if let Some(change) = client_change { self.client_config = change; } - if let Some(change) = xdg_config_change { - let node = self - .tree - .get_mut(self.xdg_config_node_id) - .expect("client_config node should exist"); - node.get_mut().input = change; - self.invalidate_subtree(self.xdg_config_node_id); - } - for ConfigParentChange { file_id, parent } in parent_changes { let node_id = self.ensure_node(file_id); let parent_node_id = match parent { @@ -455,5 +441,34 @@ mod tests { assert_eq!(local.completion_autoimport_enable, false); // from client assert_eq!(local.semanticHighlighting_strings_enable, false); + + vfs.set_file_id_contents( + xdg_config_file_id, + Some( + r#" + # default is "never" + [inlayHints.discriminantHints] + enable = "always" + "# + .to_string() + .into_bytes(), + ), + ); + + let changes = ConfigChanges { + ra_toml_changes: vfs.take_changes(), + parent_changes: vec![], + client_change: None, + }; + dbg!(config_tree.apply_changes(changes, &vfs)); + + let local2 = config_tree.read_config(crate_a).unwrap(); + // should have recomputed + assert!(!Arc::ptr_eq(&local, &local2)); + + assert_eq!( + local.inlayHints_discriminantHints_enable, + crate::config::DiscriminantHintsDef::Always + ); } } From ab0149d0c40d6d8480ebeb13fc9c549480f7e0f7 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Tue, 23 Jan 2024 22:57:14 +1100 Subject: [PATCH 07/25] fix config invalidation --- crates/rust-analyzer/src/config/tree.rs | 115 +++++++++++++++--------- 1 file changed, 74 insertions(+), 41 deletions(-) diff --git a/crates/rust-analyzer/src/config/tree.rs b/crates/rust-analyzer/src/config/tree.rs index adddb07038ab..ed5fef388ce8 100644 --- a/crates/rust-analyzer/src/config/tree.rs +++ b/crates/rust-analyzer/src/config/tree.rs @@ -41,7 +41,7 @@ pub struct ConfigChanges { /// - `Some(None)` => the client config was removed / reset or something /// - `Some(Some(...))` => the client config was updated client_change: Option>>, - parent_changes: Vec, + parent_changes: FxHashMap, } #[derive(Debug)] @@ -103,7 +103,7 @@ impl ConcurrentConfigTree { #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] enum ConfigSource { - ClientConfig, + XdgConfig(FileId), RaToml(FileId), } @@ -115,12 +115,13 @@ slotmap::new_key_type! { struct ConfigNode { src: ConfigSource, input: Option>, - computed: ComputedIdx, + computed_idx: ComputedIdx, } struct ConfigTree { tree: indextree::Arena, client_config: Option>, + xdg_config_file_id: FileId, xdg_config_node_id: NodeId, ra_file_id_map: FxHashMap, computed: SlotMap>>, @@ -164,14 +165,21 @@ impl ConfigTree { let mut tree = indextree::Arena::new(); let mut computed = SlotMap::default(); let mut ra_file_id_map = FxHashMap::default(); - let xdg_config = tree.new_node(ConfigNode { - src: ConfigSource::RaToml(xdg_config_file_id), + let xdg_config_node_id = tree.new_node(ConfigNode { + src: ConfigSource::XdgConfig(xdg_config_file_id), input: None, - computed: computed.insert(Option::>::None), + computed_idx: computed.insert(Option::>::None), }); - ra_file_id_map.insert(xdg_config_file_id, xdg_config); + ra_file_id_map.insert(xdg_config_file_id, xdg_config_node_id); - Self { client_config: None, xdg_config_node_id: xdg_config, ra_file_id_map, tree, computed } + Self { + client_config: None, + xdg_config_file_id, + xdg_config_node_id, + ra_file_id_map, + tree, + computed, + } } fn read_only(&self, file_id: FileId) -> Result>, ConfigTreeError> { @@ -196,7 +204,12 @@ impl ConfigTree { return Err(ConfigTreeError::Removed); } let node = self.tree.get(node_id).ok_or(ConfigTreeError::NonExistent)?.get(); - let stored = self.computed[node.computed].clone(); + let stored = self.computed[node.computed_idx].clone(); + tracing::trace!( + "read_only_inner on {:?} got {:?}", + node.src, + stored.as_ref().map(|_| "some stored value") + ); Ok(stored) } @@ -214,7 +227,8 @@ impl ConfigTree { return Err(ConfigTreeError::Removed); } let node = self.tree.get(node_id).ok_or(ConfigTreeError::NonExistent)?.get(); - let idx = node.computed; + tracing::trace!("compute_inner on {:?}", node.src); + let idx = node.computed_idx; let slot = &mut self.computed[idx]; if let Some(slot) = slot { Ok(slot.clone()) @@ -250,12 +264,17 @@ impl ConfigTree { fn insert_toml(&mut self, file_id: FileId, input: Option>) -> NodeId { let computed = self.computed.insert(None); - let node = - self.tree.new_node(ConfigNode { src: ConfigSource::RaToml(file_id), input, computed }); - if let Some(_removed) = self.ra_file_id_map.insert(file_id, node) { + let node_id = self.tree.new_node(ConfigNode { + src: ConfigSource::RaToml(file_id), + input, + computed_idx: computed, + }); + if let Some(_removed) = self.ra_file_id_map.insert(file_id, node_id) { panic!("ERROR: node should not have existed for {file_id:?} but it did"); } - node + // By default, everything is under the xdg_config_node_id + self.xdg_config_node_id.append(node_id, &mut self.tree); + node_id } fn update_toml( @@ -274,6 +293,7 @@ impl ConfigTree { node.get_mut().input = input; self.invalidate_subtree(node_id); + // tracing::trace!("invalidated subtree:\n{:#?}", node_id.debug_pretty_print(&self.tree)); Ok(node_id) } @@ -292,7 +312,16 @@ impl ConfigTree { let Some(desc) = self.tree.get(x) else { return; }; - self.computed.get_mut(desc.get().computed).take(); + let desc = desc.get(); + let Some(slot) = self.computed.get_mut(desc.computed_idx) else { + tracing::error!( + "computed_idx missing from computed local config slotmap: {:?}", + desc.computed_idx + ); + return; + }; + tracing::trace!("invalidating {x:?} / {:?}", desc.src); + slot.take(); }); } @@ -314,32 +343,22 @@ impl ConfigTree { errors: &mut Vec, ) { let mut scratch_errors = Vec::new(); - let ConfigChanges { client_change, ra_toml_changes, parent_changes } = changes; + let ConfigChanges { client_change, ra_toml_changes, mut parent_changes } = changes; if let Some(change) = client_change { self.client_config = change; } - for ConfigParentChange { file_id, parent } in parent_changes { - let node_id = self.ensure_node(file_id); - let parent_node_id = match parent { - ConfigParent::UserDefault => self.xdg_config_node_id, - ConfigParent::Parent(parent_file_id) => self.ensure_node(parent_file_id), - }; - // order of children within the parent node does not matter - tracing::trace!("appending child {node_id:?} to {parent_node_id:?}"); - parent_node_id.append(node_id, &mut self.tree); - } - for change in ra_toml_changes { // turn and face the strain match change.change_kind { - vfs::ChangeKind::Create => { - let input = parse_toml(change.file_id, vfs, &mut scratch_errors, errors); - let _new_node = self.update_toml(change.file_id, input); - } - vfs::ChangeKind::Modify => { + vfs::ChangeKind::Create | vfs::ChangeKind::Modify => { + if change.change_kind == vfs::ChangeKind::Create { + parent_changes.entry(change.file_id).or_insert(ConfigParent::UserDefault); + } let input = parse_toml(change.file_id, vfs, &mut scratch_errors, errors); + tracing::trace!("updating toml for {:?} to {:?}", change.file_id, input); + if let Err(e) = self.update_toml(change.file_id, input) { errors.push(e); } @@ -349,6 +368,18 @@ impl ConfigTree { } } } + + for (file_id, parent) in parent_changes { + let node_id = self.ensure_node(file_id); + let parent_node_id = match parent { + ConfigParent::Parent(parent_file_id) => self.ensure_node(parent_file_id), + ConfigParent::UserDefault if file_id == self.xdg_config_file_id => continue, + ConfigParent::UserDefault => self.xdg_config_node_id, + }; + // order of children within the parent node does not matter + tracing::trace!("appending child {node_id:?} to {parent_node_id:?}"); + parent_node_id.append(node_id, &mut self.tree); + } } } @@ -365,6 +396,7 @@ mod tests { use vfs::{AbsPathBuf, VfsPath}; fn alloc_file_id(vfs: &mut Vfs, s: &str) -> FileId { + tracing_subscriber::fmt().init(); let abs_path = AbsPathBuf::try_from(PathBuf::new().join(s)).unwrap(); let vfs_path = VfsPath::from(abs_path); @@ -391,7 +423,7 @@ mod tests { fn basic() { let mut vfs = Vfs::default(); let xdg_config_file_id = - alloc_file_id(&mut vfs, "/home/.config/rust-analyzer/rust-analyzer.toml"); + alloc_file_id(&mut vfs, "/home/username/.config/rust-analyzer/rust-analyzer.toml"); let config_tree = ConcurrentConfigTree::new(ConfigTree::new(xdg_config_file_id)); let root = alloc_config( @@ -415,8 +447,8 @@ mod tests { "#, ); - let parent_changes = - vec![ConfigParentChange { file_id: crate_a, parent: ConfigParent::Parent(root) }]; + let mut parent_changes = FxHashMap::default(); + parent_changes.insert(crate_a, ConfigParent::Parent(root)); let changes = ConfigChanges { // Normally you will filter these! @@ -432,7 +464,6 @@ mod tests { }; dbg!(config_tree.apply_changes(changes, &vfs)); - dbg!(&config_tree); let local = config_tree.read_config(crate_a).unwrap(); // from root @@ -442,6 +473,9 @@ mod tests { // from client assert_eq!(local.semanticHighlighting_strings_enable, false); + // -------------------------------------------------------- + + // Now let's modify the xdg_config_file_id, which should invalidate everything else vfs.set_file_id_contents( xdg_config_file_id, Some( @@ -456,15 +490,14 @@ mod tests { ); let changes = ConfigChanges { - ra_toml_changes: vfs.take_changes(), - parent_changes: vec![], + ra_toml_changes: dbg!(vfs.take_changes()), + parent_changes: Default::default(), client_change: None, }; dbg!(config_tree.apply_changes(changes, &vfs)); - let local2 = config_tree.read_config(crate_a).unwrap(); - // should have recomputed - assert!(!Arc::ptr_eq(&local, &local2)); + let prev = local; + let local = config_tree.read_config(crate_a).unwrap(); assert_eq!( local.inlayHints_discriminantHints_enable, From 851b9701c000233c6f9fd47e47630a24da8bc962 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Tue, 23 Jan 2024 23:26:29 +1100 Subject: [PATCH 08/25] Avoid recomputing the client-config-mixed configs --- crates/rust-analyzer/src/config/tree.rs | 51 ++++++++++++++++--------- 1 file changed, 33 insertions(+), 18 deletions(-) diff --git a/crates/rust-analyzer/src/config/tree.rs b/crates/rust-analyzer/src/config/tree.rs index ed5fef388ce8..0f7a12d6569e 100644 --- a/crates/rust-analyzer/src/config/tree.rs +++ b/crates/rust-analyzer/src/config/tree.rs @@ -125,6 +125,7 @@ struct ConfigTree { xdg_config_node_id: NodeId, ra_file_id_map: FxHashMap, computed: SlotMap>>, + with_client_config: slotmap::SecondaryMap>, } fn parse_toml( @@ -179,19 +180,13 @@ impl ConfigTree { ra_file_id_map, tree, computed, + with_client_config: Default::default(), } } fn read_only(&self, file_id: FileId) -> Result>, ConfigTreeError> { let node_id = *self.ra_file_id_map.get(&file_id).ok_or(ConfigTreeError::NonExistent)?; - let stored = self.read_only_inner(node_id)?; - Ok(stored.map(|stored| { - if let Some(client_config) = self.client_config.as_deref() { - stored.clone_with_overrides(client_config.local.clone()).into() - } else { - stored - } - })) + self.read_only_inner(node_id) } fn read_only_inner( @@ -204,7 +199,7 @@ impl ConfigTree { return Err(ConfigTreeError::Removed); } let node = self.tree.get(node_id).ok_or(ConfigTreeError::NonExistent)?.get(); - let stored = self.computed[node.computed_idx].clone(); + let stored = self.with_client_config.get(node.computed_idx).cloned(); tracing::trace!( "read_only_inner on {:?} got {:?}", node.src, @@ -215,14 +210,19 @@ impl ConfigTree { fn compute(&mut self, file_id: FileId) -> Result, ConfigTreeError> { let node_id = *self.ra_file_id_map.get(&file_id).ok_or(ConfigTreeError::NonExistent)?; - let computed = self.compute_inner(node_id)?; - Ok(if let Some(client_config) = self.client_config.as_deref() { - computed.clone_with_overrides(client_config.local.clone()).into() + let (computed, idx) = self.compute_recursive(node_id)?; + let out = if let Some(client_config) = self.client_config.as_deref() { + Arc::new(computed.clone_with_overrides(client_config.local.clone())) } else { computed - }) + }; + self.with_client_config.insert(idx, out.clone()); + Ok(out) } - fn compute_inner(&mut self, node_id: NodeId) -> Result, ConfigTreeError> { + fn compute_recursive( + &mut self, + node_id: NodeId, + ) -> Result<(Arc, ComputedIdx), ConfigTreeError> { if node_id.is_removed(&self.tree) { return Err(ConfigTreeError::Removed); } @@ -231,14 +231,14 @@ impl ConfigTree { let idx = node.computed_idx; let slot = &mut self.computed[idx]; if let Some(slot) = slot { - Ok(slot.clone()) + Ok((slot.clone(), idx)) } else { let self_computed = if let Some(parent) = self.tree.get(node_id).ok_or(ConfigTreeError::NonExistent)?.parent() { tracing::trace!("looking at parent of {node_id:?} -> {parent:?}"); let self_input = node.input.clone(); - let parent_computed = self.compute_inner(parent)?; + let (parent_computed, _) = self.compute_recursive(parent)?; if let Some(input) = self_input.as_deref() { Arc::new(parent_computed.clone_with_overrides(input.local.clone())) } else { @@ -258,7 +258,7 @@ impl ConfigTree { // Get a new &mut slot because self.compute(parent) also gets mut access let slot = &mut self.computed[idx]; slot.replace(self_computed.clone()); - Ok(self_computed) + Ok((self_computed, idx)) } } @@ -322,6 +322,9 @@ impl ConfigTree { }; tracing::trace!("invalidating {x:?} / {:?}", desc.src); slot.take(); + + // Also invalidate the secondary data + self.with_client_config.remove(desc.computed_idx); }); } @@ -346,7 +349,16 @@ impl ConfigTree { let ConfigChanges { client_change, ra_toml_changes, mut parent_changes } = changes; if let Some(change) = client_change { - self.client_config = change; + match (self.client_config.as_ref(), change.as_ref()) { + (None, None) => {} + (Some(a), Some(b)) if Arc::ptr_eq(a, b) => {} + _ => { + // invalidate the output table only, don't immediately need to recompute + // everything from scratch + self.with_client_config.clear(); + self.client_config = change; + } + } } for change in ra_toml_changes { @@ -498,6 +510,9 @@ mod tests { let prev = local; let local = config_tree.read_config(crate_a).unwrap(); + assert!(!Arc::ptr_eq(&prev, &local)); + let local2 = config_tree.read_config(crate_a).unwrap(); + assert!(Arc::ptr_eq(&local, &local2)); assert_eq!( local.inlayHints_discriminantHints_enable, From 77e61f7f6bcbd7bdb52276d5380822f6af986872 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Tue, 23 Jan 2024 23:33:27 +1100 Subject: [PATCH 09/25] Better test for config tree --- crates/rust-analyzer/src/config/tree.rs | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/crates/rust-analyzer/src/config/tree.rs b/crates/rust-analyzer/src/config/tree.rs index 0f7a12d6569e..edff0eac9cc0 100644 --- a/crates/rust-analyzer/src/config/tree.rs +++ b/crates/rust-analyzer/src/config/tree.rs @@ -292,6 +292,9 @@ impl ConfigTree { let node = self.tree.get_mut(node_id).ok_or(ConfigTreeError::NonExistent)?; node.get_mut().input = input; + // We won't do any funny business comparing the previous input to the new one, because + // that would require implementing PartialEq on the dozens and dozens of types that make + // up ConfigInput. self.invalidate_subtree(node_id); // tracing::trace!("invalidated subtree:\n{:#?}", node_id.debug_pretty_print(&self.tree)); Ok(node_id) @@ -495,6 +498,12 @@ mod tests { # default is "never" [inlayHints.discriminantHints] enable = "always" + [completion.autoself] + enable = true + [completion.autoimport] + enable = true + [semanticHighlighting.strings] + enable = true "# .to_string() .into_bytes(), @@ -510,13 +519,20 @@ mod tests { let prev = local; let local = config_tree.read_config(crate_a).unwrap(); + // Should have been recomputed assert!(!Arc::ptr_eq(&prev, &local)); - let local2 = config_tree.read_config(crate_a).unwrap(); - assert!(Arc::ptr_eq(&local, &local2)); + // But without changes in between, should give the same Arc back + assert!(Arc::ptr_eq(&local, &config_tree.read_config(crate_a).unwrap())); + // The newly added xdg_config_file_id should affect the output if nothing else touches + // this key assert_eq!( local.inlayHints_discriminantHints_enable, crate::config::DiscriminantHintsDef::Always ); + // But it should not win + assert_eq!(local.completion_autoself_enable, false); + assert_eq!(local.completion_autoimport_enable, false); + assert_eq!(local.semanticHighlighting_strings_enable, false); } } From 9d8cff2642ac4ba3e78350661de74608f5ed91f3 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Tue, 23 Jan 2024 23:34:58 +1100 Subject: [PATCH 10/25] rename read_config to local_config --- crates/rust-analyzer/src/config/tree.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/rust-analyzer/src/config/tree.rs b/crates/rust-analyzer/src/config/tree.rs index edff0eac9cc0..3d6e2c99a29d 100644 --- a/crates/rust-analyzer/src/config/tree.rs +++ b/crates/rust-analyzer/src/config/tree.rs @@ -90,7 +90,7 @@ impl ConcurrentConfigTree { self.rwlock.write().apply_changes(changes, vfs, &mut errors); errors } - pub fn read_config(&self, file_id: FileId) -> Result, ConfigTreeError> { + pub fn local_config(&self, file_id: FileId) -> Result, ConfigTreeError> { let reader = self.rwlock.upgradable_read(); if let Some(computed) = reader.read_only(file_id)? { return Ok(computed); @@ -480,7 +480,7 @@ mod tests { dbg!(config_tree.apply_changes(changes, &vfs)); - let local = config_tree.read_config(crate_a).unwrap(); + let local = config_tree.local_config(crate_a).unwrap(); // from root assert_eq!(local.completion_autoself_enable, false); // from crate_a @@ -518,11 +518,11 @@ mod tests { dbg!(config_tree.apply_changes(changes, &vfs)); let prev = local; - let local = config_tree.read_config(crate_a).unwrap(); + let local = config_tree.local_config(crate_a).unwrap(); // Should have been recomputed assert!(!Arc::ptr_eq(&prev, &local)); // But without changes in between, should give the same Arc back - assert!(Arc::ptr_eq(&local, &config_tree.read_config(crate_a).unwrap())); + assert!(Arc::ptr_eq(&local, &config_tree.local_config(crate_a).unwrap())); // The newly added xdg_config_file_id should affect the output if nothing else touches // this key From ce341dcef4a24caf35c8c668bc33ce1edebfc27a Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Tue, 23 Jan 2024 23:44:58 +1100 Subject: [PATCH 11/25] delete ConfigParentChange --- crates/rust-analyzer/src/config/tree.rs | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/crates/rust-analyzer/src/config/tree.rs b/crates/rust-analyzer/src/config/tree.rs index 3d6e2c99a29d..6b63f4a7b1e9 100644 --- a/crates/rust-analyzer/src/config/tree.rs +++ b/crates/rust-analyzer/src/config/tree.rs @@ -44,13 +44,6 @@ pub struct ConfigChanges { parent_changes: FxHashMap, } -#[derive(Debug)] -pub struct ConfigParentChange { - /// The config node in question - pub file_id: FileId, - pub parent: ConfigParent, -} - #[derive(Debug)] pub enum ConfigParent { /// The node is now a root in its own right, but still inherits from the config in XDG_CONFIG_HOME @@ -75,11 +68,11 @@ pub enum ConfigParent { /// let root = vfs.set_file_contents("/project_root/rust-analyzer.toml", Some("...")); /// let crate_a = vfs.set_file_contents("/project_root/crate_a/rust-analyzer.toml", None); /// let crate_b = vfs.set_file_contents("/project_root/crate_a/crate_b/rust-analyzer.toml", Some("...")); - /// let config_parent_changes = [ - /// ConfigParentChange { node: root, parent: ConfigParent::UserDefault }, - /// ConfigParentChange { node: crate_a, parent: ConfigParent::Parent(root) }, - /// ConfigParentChange { node: crate_b, parent: ConfigParent::Parent(crate_a) } - /// ]; + /// let parent_changes = FxHashMap::from_iter([ + /// (root, ConfigParent::UserDefault), + /// (crate_a, ConfigParent::Parent(root)), + /// (crate_b, ConfigParent::Parent(crate_a)), + /// ]); /// ``` Parent(FileId), } From 09a3888ea6306c407c2c9231608bd648723a9a25 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Wed, 24 Jan 2024 22:41:40 +1100 Subject: [PATCH 12/25] use salsa for the config tree --- Cargo.lock | 18 +- Cargo.toml | 1 + crates/base-db/Cargo.toml | 2 +- crates/rust-analyzer/Cargo.toml | 3 +- crates/rust-analyzer/src/config/tree.rs | 435 ++++++++---------------- 5 files changed, 147 insertions(+), 312 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6265568b25b7..aaedd6281a54 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -792,12 +792,6 @@ dependencies = [ "serde", ] -[[package]] -name = "indextree" -version = "4.6.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c40411d0e5c63ef1323c3d09ce5ec6d84d71531e18daed0743fccea279d7deb6" - [[package]] name = "inotify" version = "0.9.6" @@ -1592,7 +1586,6 @@ dependencies = [ "ide-db", "ide-ssr", "indexmap 2.0.0", - "indextree", "itertools", "la-arena 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)", "load-cargo", @@ -1613,10 +1606,10 @@ dependencies = [ "rayon", "rustc-dependencies", "rustc-hash", + "salsa", "scip", "serde", "serde_json", - "slotmap", "sourcegen", "stdx", "syntax", @@ -1793,15 +1786,6 @@ dependencies = [ "lazy_static", ] -[[package]] -name = "slotmap" -version = "1.0.7" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dbff4acf519f630b3a3ddcfaea6c06b42174d9a44bc70c620e9ed1649d58b82a" -dependencies = [ - "version_check", -] - [[package]] name = "smallvec" version = "1.10.0" diff --git a/Cargo.toml b/Cargo.toml index c382a5a37d23..e24ab0867236 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -100,6 +100,7 @@ nohash-hasher = "0.2.0" text-size = "1.1.0" serde = { version = "1.0.156", features = ["derive"] } serde_json = "1.0.96" +salsa = "0.17.0-pre.2" triomphe = { version = "0.1.8", default-features = false, features = ["std"] } # can't upgrade due to dashmap depending on 0.12.3 currently hashbrown = { version = "0.12.3", features = [ diff --git a/crates/base-db/Cargo.toml b/crates/base-db/Cargo.toml index 8435dba86d22..d6fd0047a22c 100644 --- a/crates/base-db/Cargo.toml +++ b/crates/base-db/Cargo.toml @@ -12,7 +12,7 @@ rust-version.workspace = true doctest = false [dependencies] -salsa = "0.17.0-pre.2" +salsa.workspace = true rustc-hash = "1.1.0" triomphe.workspace = true diff --git a/crates/rust-analyzer/Cargo.toml b/crates/rust-analyzer/Cargo.toml index 4567f382f89b..009c03b645f7 100644 --- a/crates/rust-analyzer/Cargo.toml +++ b/crates/rust-analyzer/Cargo.toml @@ -75,8 +75,7 @@ toolchain.workspace = true vfs-notify.workspace = true vfs.workspace = true la-arena.workspace = true -indextree = "4.6.0" -slotmap = "1.0.7" +salsa.workspace = true [target.'cfg(windows)'.dependencies] winapi = "0.3.9" diff --git a/crates/rust-analyzer/src/config/tree.rs b/crates/rust-analyzer/src/config/tree.rs index 6b63f4a7b1e9..26897f174641 100644 --- a/crates/rust-analyzer/src/config/tree.rs +++ b/crates/rust-analyzer/src/config/tree.rs @@ -1,37 +1,16 @@ -use indextree::NodeId; -use parking_lot::{RwLock, RwLockUpgradableReadGuard}; -use rustc_hash::FxHashMap; -use slotmap::SlotMap; -use std::{fmt, sync::Arc}; +use rustc_hash::{FxHashMap, FxHashSet}; +use std::sync::Arc; use vfs::{FileId, Vfs}; use super::{ConfigInput, LocalConfigData, RootLocalConfigData}; -pub struct ConcurrentConfigTree { - // One rwlock on the whole thing is probably fine. - // If you have 40,000 crates and you need to edit your config 200x/second, let us know. - rwlock: RwLock, -} - -impl ConcurrentConfigTree { - fn new(config_tree: ConfigTree) -> Self { - Self { rwlock: RwLock::new(config_tree) } - } -} - -impl fmt::Debug for ConcurrentConfigTree { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.rwlock.read().fmt(f) - } -} - #[derive(Debug)] pub enum ConfigTreeError { Removed, NonExistent, - Utf8(vfs::VfsPath, std::str::Utf8Error), - TomlParse(vfs::VfsPath, toml::de::Error), - TomlDeserialize { path: vfs::VfsPath, field: String, error: toml::de::Error }, + Utf8(FileId, std::str::Utf8Error), + TomlParse(FileId, toml::de::Error), + TomlDeserialize { file_id: FileId, field: String, error: toml::de::Error }, } /// Some rust-analyzer.toml files have changed, and/or the LSP client sent a new configuration. @@ -77,282 +56,116 @@ pub enum ConfigParent { Parent(FileId), } -impl ConcurrentConfigTree { - pub fn apply_changes(&self, changes: ConfigChanges, vfs: &Vfs) -> Vec { - let mut errors = Vec::new(); - self.rwlock.write().apply_changes(changes, vfs, &mut errors); - errors - } - pub fn local_config(&self, file_id: FileId) -> Result, ConfigTreeError> { - let reader = self.rwlock.upgradable_read(); - if let Some(computed) = reader.read_only(file_id)? { - return Ok(computed); - } else { - let mut writer = RwLockUpgradableReadGuard::upgrade(reader); - return writer.compute(file_id); - } +/// Easier and probably more performant than making ConfigInput implement Eq +#[derive(Debug)] +struct PointerCmp(Arc); +impl PointerCmp { + fn new(t: T) -> Self { + Self(Arc::new(t)) } } - -#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] -enum ConfigSource { - XdgConfig(FileId), - RaToml(FileId), +impl Clone for PointerCmp { + fn clone(&self) -> Self { + Self(self.0.clone()) + } } - -slotmap::new_key_type! { - struct ComputedIdx; +impl PartialEq for PointerCmp { + fn eq(&self, other: &Self) -> bool { + (Arc::as_ptr(&self.0) as *const ()).eq(&Arc::as_ptr(&other.0).cast()) + } } - -#[derive(Debug)] -struct ConfigNode { - src: ConfigSource, - input: Option>, - computed_idx: ComputedIdx, +impl Eq for PointerCmp {} +impl std::ops::Deref for PointerCmp { + type Target = T; + fn deref(&self) -> &T { + self.0.deref() + } } -struct ConfigTree { - tree: indextree::Arena, - client_config: Option>, - xdg_config_file_id: FileId, - xdg_config_node_id: NodeId, - ra_file_id_map: FxHashMap, - computed: SlotMap>>, - with_client_config: slotmap::SecondaryMap>, -} +#[salsa::query_group(ConfigTreeStorage)] +trait ConfigTreeQueries { + #[salsa::input] + fn client_config(&self) -> Option>; -fn parse_toml( - file_id: FileId, - vfs: &Vfs, - scratch: &mut Vec<(String, toml::de::Error)>, - errors: &mut Vec, -) -> Option> { - let content = vfs.file_contents(file_id); - let path = vfs.file_path(file_id); - if content.is_empty() { - return None; - } - let content_str = match std::str::from_utf8(content) { - Err(e) => { - tracing::error!("non-UTF8 TOML content for {path}: {e}"); - errors.push(ConfigTreeError::Utf8(path, e)); - return None; - } - Ok(str) => str, - }; - let table = match toml::from_str(content_str) { - Ok(table) => table, - Err(e) => { - errors.push(ConfigTreeError::TomlParse(path, e)); - return None; - } - }; - let input = Arc::new(ConfigInput::from_toml(table, scratch)); - scratch.drain(..).for_each(|(field, error)| { - errors.push(ConfigTreeError::TomlDeserialize { path: path.clone(), field, error }); - }); - Some(input) -} + #[salsa::input] + fn config_parent(&self, file_id: FileId) -> Option; -impl ConfigTree { - fn new(xdg_config_file_id: FileId) -> Self { - let mut tree = indextree::Arena::new(); - let mut computed = SlotMap::default(); - let mut ra_file_id_map = FxHashMap::default(); - let xdg_config_node_id = tree.new_node(ConfigNode { - src: ConfigSource::XdgConfig(xdg_config_file_id), - input: None, - computed_idx: computed.insert(Option::>::None), - }); - ra_file_id_map.insert(xdg_config_file_id, xdg_config_node_id); - - Self { - client_config: None, - xdg_config_file_id, - xdg_config_node_id, - ra_file_id_map, - tree, - computed, - with_client_config: Default::default(), - } - } + #[salsa::input] + fn config_input(&self, file_id: FileId) -> Option>; - fn read_only(&self, file_id: FileId) -> Result>, ConfigTreeError> { - let node_id = *self.ra_file_id_map.get(&file_id).ok_or(ConfigTreeError::NonExistent)?; - self.read_only_inner(node_id) - } + fn compute_recursive(&self, file_id: FileId) -> PointerCmp; - fn read_only_inner( - &self, - node_id: NodeId, - ) -> Result>, ConfigTreeError> { - // indextree does not check this during get(), probably for perf reasons? - // get() is apparently only a bounds check - if node_id.is_removed(&self.tree) { - return Err(ConfigTreeError::Removed); - } - let node = self.tree.get(node_id).ok_or(ConfigTreeError::NonExistent)?.get(); - let stored = self.with_client_config.get(node.computed_idx).cloned(); - tracing::trace!( - "read_only_inner on {:?} got {:?}", - node.src, - stored.as_ref().map(|_| "some stored value") - ); - Ok(stored) - } + fn local_config(&self, file_id: FileId) -> PointerCmp; +} - fn compute(&mut self, file_id: FileId) -> Result, ConfigTreeError> { - let node_id = *self.ra_file_id_map.get(&file_id).ok_or(ConfigTreeError::NonExistent)?; - let (computed, idx) = self.compute_recursive(node_id)?; - let out = if let Some(client_config) = self.client_config.as_deref() { - Arc::new(computed.clone_with_overrides(client_config.local.clone())) - } else { - computed - }; - self.with_client_config.insert(idx, out.clone()); - Ok(out) - } - fn compute_recursive( - &mut self, - node_id: NodeId, - ) -> Result<(Arc, ComputedIdx), ConfigTreeError> { - if node_id.is_removed(&self.tree) { - return Err(ConfigTreeError::Removed); - } - let node = self.tree.get(node_id).ok_or(ConfigTreeError::NonExistent)?.get(); - tracing::trace!("compute_inner on {:?}", node.src); - let idx = node.computed_idx; - let slot = &mut self.computed[idx]; - if let Some(slot) = slot { - Ok((slot.clone(), idx)) - } else { - let self_computed = if let Some(parent) = - self.tree.get(node_id).ok_or(ConfigTreeError::NonExistent)?.parent() - { - tracing::trace!("looking at parent of {node_id:?} -> {parent:?}"); - let self_input = node.input.clone(); - let (parent_computed, _) = self.compute_recursive(parent)?; - if let Some(input) = self_input.as_deref() { - Arc::new(parent_computed.clone_with_overrides(input.local.clone())) - } else { - parent_computed - } +fn compute_recursive(db: &dyn ConfigTreeQueries, file_id: FileId) -> PointerCmp { + let self_input = db.config_input(file_id); + tracing::trace!(?self_input, ?file_id); + match db.config_parent(file_id) { + Some(parent) if parent != file_id => { + let parent_computed = db.compute_recursive(parent); + if let Some(input) = self_input.as_deref() { + PointerCmp::new(parent_computed.clone_with_overrides(input.local.clone())) } else { - tracing::trace!("{node_id:?} is a root node"); - // We have hit a root node - let self_input = node.input.clone(); - if let Some(input) = self_input.as_deref() { - let root_local = RootLocalConfigData::from_root_input(input.local.clone()); - Arc::new(root_local.0) - } else { - Arc::new(LocalConfigData::default()) - } - }; - // Get a new &mut slot because self.compute(parent) also gets mut access - let slot = &mut self.computed[idx]; - slot.replace(self_computed.clone()); - Ok((self_computed, idx)) - } - } - - fn insert_toml(&mut self, file_id: FileId, input: Option>) -> NodeId { - let computed = self.computed.insert(None); - let node_id = self.tree.new_node(ConfigNode { - src: ConfigSource::RaToml(file_id), - input, - computed_idx: computed, - }); - if let Some(_removed) = self.ra_file_id_map.insert(file_id, node_id) { - panic!("ERROR: node should not have existed for {file_id:?} but it did"); + parent_computed + } } - // By default, everything is under the xdg_config_node_id - self.xdg_config_node_id.append(node_id, &mut self.tree); - node_id - } - - fn update_toml( - &mut self, - file_id: FileId, - input: Option>, - ) -> Result { - let Some(node_id) = self.ra_file_id_map.get(&file_id).cloned() else { - let node_id = self.insert_toml(file_id, input); - return Ok(node_id); - }; - if node_id.is_removed(&self.tree) { - return Err(ConfigTreeError::Removed); + _ => { + // this is a root node, or we just broke a cycle + if let Some(input) = self_input.as_deref() { + let root_local = RootLocalConfigData::from_root_input(input.local.clone()); + PointerCmp::new(root_local.0) + } else { + PointerCmp::new(LocalConfigData::default()) + } } - let node = self.tree.get_mut(node_id).ok_or(ConfigTreeError::NonExistent)?; - node.get_mut().input = input; - - // We won't do any funny business comparing the previous input to the new one, because - // that would require implementing PartialEq on the dozens and dozens of types that make - // up ConfigInput. - self.invalidate_subtree(node_id); - // tracing::trace!("invalidated subtree:\n{:#?}", node_id.debug_pretty_print(&self.tree)); - Ok(node_id) } +} - fn ensure_node(&mut self, file_id: FileId) -> NodeId { - let Some(&node_id) = self.ra_file_id_map.get(&file_id) else { - return self.insert_toml(file_id, None); - }; - node_id +fn local_config(db: &dyn ConfigTreeQueries, file_id: FileId) -> PointerCmp { + let computed = db.compute_recursive(file_id); + if let Some(client) = db.client_config() { + PointerCmp::new(computed.clone_with_overrides(client.local.clone())) + } else { + computed } +} - fn invalidate_subtree(&mut self, node_id: NodeId) { - // - // This is why we need the computed values outside the indextree: we iterate immutably - // over the tree while holding a &mut self.computed. - node_id.descendants(&self.tree).for_each(|x| { - let Some(desc) = self.tree.get(x) else { - return; - }; - let desc = desc.get(); - let Some(slot) = self.computed.get_mut(desc.computed_idx) else { - tracing::error!( - "computed_idx missing from computed local config slotmap: {:?}", - desc.computed_idx - ); - return; - }; - tracing::trace!("invalidating {x:?} / {:?}", desc.src); - slot.take(); +#[salsa::database(ConfigTreeStorage)] +pub struct ConfigDb { + storage: salsa::Storage, + known_file_ids: FxHashSet, + xdg_config_file_id: FileId, +} - // Also invalidate the secondary data - self.with_client_config.remove(desc.computed_idx); - }); - } +impl salsa::Database for ConfigDb {} - fn remove_toml(&mut self, file_id: FileId) -> Option<()> { - let node_id = *self.ra_file_id_map.get(&file_id)?; - if node_id.is_removed(&self.tree) { - return None; - } - let node = self.tree.get_mut(node_id)?.get_mut(); - node.input = None; - self.invalidate_subtree(node_id); - Some(()) +impl ConfigDb { + pub fn new(xdg_config_file_id: FileId) -> Self { + let mut this = Self { + storage: Default::default(), + known_file_ids: FxHashSet::default(), + xdg_config_file_id, + }; + this.set_client_config(None); + this.ensure_node(xdg_config_file_id); + this.set_config_parent(xdg_config_file_id, None); + this } - fn apply_changes( - &mut self, - changes: ConfigChanges, - vfs: &Vfs, - errors: &mut Vec, - ) { + pub fn apply_changes(&mut self, changes: ConfigChanges, vfs: &Vfs) -> Vec { let mut scratch_errors = Vec::new(); + let mut errors = Vec::new(); let ConfigChanges { client_change, ra_toml_changes, mut parent_changes } = changes; if let Some(change) = client_change { - match (self.client_config.as_ref(), change.as_ref()) { + let current = self.client_config(); + let change = change.map(PointerCmp); + match (current.as_ref(), change.as_ref()) { (None, None) => {} - (Some(a), Some(b)) if Arc::ptr_eq(a, b) => {} + (Some(a), Some(b)) if a == b => {} _ => { - // invalidate the output table only, don't immediately need to recompute - // everything from scratch - self.with_client_config.clear(); - self.client_config = change; + self.set_client_config(change); } } } @@ -364,37 +177,75 @@ impl ConfigTree { if change.change_kind == vfs::ChangeKind::Create { parent_changes.entry(change.file_id).or_insert(ConfigParent::UserDefault); } - let input = parse_toml(change.file_id, vfs, &mut scratch_errors, errors); + let input = parse_toml(change.file_id, vfs, &mut scratch_errors, &mut errors) + .map(PointerCmp); tracing::trace!("updating toml for {:?} to {:?}", change.file_id, input); - if let Err(e) = self.update_toml(change.file_id, input) { - errors.push(e); - } + self.ensure_node(change.file_id); + self.set_config_input(change.file_id, input); } vfs::ChangeKind::Delete => { - self.remove_toml(change.file_id); + self.ensure_node(change.file_id); + self.set_config_input(change.file_id, None); } } } for (file_id, parent) in parent_changes { - let node_id = self.ensure_node(file_id); + self.ensure_node(file_id); let parent_node_id = match parent { - ConfigParent::Parent(parent_file_id) => self.ensure_node(parent_file_id), + ConfigParent::Parent(parent_file_id) => { + self.ensure_node(parent_file_id); + parent_file_id + } ConfigParent::UserDefault if file_id == self.xdg_config_file_id => continue, - ConfigParent::UserDefault => self.xdg_config_node_id, + ConfigParent::UserDefault => self.xdg_config_file_id, }; // order of children within the parent node does not matter - tracing::trace!("appending child {node_id:?} to {parent_node_id:?}"); - parent_node_id.append(node_id, &mut self.tree); + tracing::trace!("appending child {file_id:?} to {parent_node_id:?}"); + self.set_config_parent(file_id, Some(parent_node_id)) + } + + errors + } + + fn ensure_node(&mut self, file_id: FileId) { + if self.known_file_ids.insert(file_id) { + self.set_config_input(file_id, None); } } } -impl fmt::Debug for ConfigTree { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.tree.fmt(f) +fn parse_toml( + file_id: FileId, + vfs: &Vfs, + scratch: &mut Vec<(String, toml::de::Error)>, + errors: &mut Vec, +) -> Option> { + let content = vfs.file_contents(file_id); + let content_str = match std::str::from_utf8(content) { + Err(e) => { + tracing::error!("non-UTF8 TOML content for {file_id:?}: {e}"); + errors.push(ConfigTreeError::Utf8(file_id, e)); + return None; + } + Ok(str) => str, + }; + if content_str.is_empty() { + return None; } + let table = match toml::from_str(content_str) { + Ok(table) => table, + Err(e) => { + errors.push(ConfigTreeError::TomlParse(file_id, e)); + return None; + } + }; + let input = Arc::new(ConfigInput::from_toml(table, scratch)); + scratch.drain(..).for_each(|(field, error)| { + errors.push(ConfigTreeError::TomlDeserialize { file_id, field, error }); + }); + Some(input) } #[cfg(test)] @@ -432,7 +283,7 @@ mod tests { let mut vfs = Vfs::default(); let xdg_config_file_id = alloc_file_id(&mut vfs, "/home/username/.config/rust-analyzer/rust-analyzer.toml"); - let config_tree = ConcurrentConfigTree::new(ConfigTree::new(xdg_config_file_id)); + let mut config_tree = ConfigDb::new(xdg_config_file_id); let root = alloc_config( &mut vfs, @@ -473,7 +324,7 @@ mod tests { dbg!(config_tree.apply_changes(changes, &vfs)); - let local = config_tree.local_config(crate_a).unwrap(); + let local = config_tree.local_config(crate_a); // from root assert_eq!(local.completion_autoself_enable, false); // from crate_a @@ -511,11 +362,11 @@ mod tests { dbg!(config_tree.apply_changes(changes, &vfs)); let prev = local; - let local = config_tree.local_config(crate_a).unwrap(); + let local = config_tree.local_config(crate_a); // Should have been recomputed - assert!(!Arc::ptr_eq(&prev, &local)); + assert_ne!(prev, local); // But without changes in between, should give the same Arc back - assert!(Arc::ptr_eq(&local, &config_tree.local_config(crate_a).unwrap())); + assert_eq!(local, config_tree.local_config(crate_a)); // The newly added xdg_config_file_id should affect the output if nothing else touches // this key From 359bf1da4e3b1ae415c617ba5314479647dec878 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Wed, 24 Jan 2024 23:11:01 +1100 Subject: [PATCH 13/25] Delte unused vfs method, document set_file_id_contents --- crates/vfs/src/lib.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/crates/vfs/src/lib.rs b/crates/vfs/src/lib.rs index 65882f72f5a7..0c8474f60753 100644 --- a/crates/vfs/src/lib.rs +++ b/crates/vfs/src/lib.rs @@ -133,11 +133,6 @@ impl Vfs { self.get(file_id).as_deref().unwrap() } - /// File content, but returns None if the file was deleted. - pub fn file_contents_opt(&self, file_id: FileId) -> Option<&[u8]> { - self.get(file_id).as_deref() - } - /// Returns the overall memory usage for the stored files. pub fn memory_usage(&self) -> usize { self.data.iter().flatten().map(|d| d.capacity()).sum() @@ -167,6 +162,12 @@ impl Vfs { self.set_file_id_contents(file_id, contents) } + /// Update the given `file_id` with the given `contents`. `None` means the file was deleted. + /// + /// Returns `true` if the file was modified, and saves the [change](ChangedFile). + /// + /// If the path does not currently exists in the `Vfs`, allocates a new + /// [`FileId`] for it. pub fn set_file_id_contents(&mut self, file_id: FileId, mut contents: Option>) -> bool { let change_kind = match (self.get(file_id), &contents) { (None, None) => return false, From f1cff1e5a197e482363779cff76219e22f84560e Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Thu, 25 Jan 2024 00:29:14 +1100 Subject: [PATCH 14/25] Add a test that generates a tree from running path.ancestors() on the roots Also renames a bunch of functions and hides PointerCmp from public API --- crates/rust-analyzer/src/config/tree.rs | 144 ++++++++++++++++++++---- 1 file changed, 122 insertions(+), 22 deletions(-) diff --git a/crates/rust-analyzer/src/config/tree.rs b/crates/rust-analyzer/src/config/tree.rs index 26897f174641..b1668f3d26ea 100644 --- a/crates/rust-analyzer/src/config/tree.rs +++ b/crates/rust-analyzer/src/config/tree.rs @@ -88,22 +88,23 @@ trait ConfigTreeQueries { fn client_config(&self) -> Option>; #[salsa::input] - fn config_parent(&self, file_id: FileId) -> Option; + fn parent(&self, file_id: FileId) -> Option; #[salsa::input] fn config_input(&self, file_id: FileId) -> Option>; - fn compute_recursive(&self, file_id: FileId) -> PointerCmp; + fn recursive_local(&self, file_id: FileId) -> PointerCmp; - fn local_config(&self, file_id: FileId) -> PointerCmp; + /// The output + fn computed_local_config(&self, file_id: FileId) -> PointerCmp; } -fn compute_recursive(db: &dyn ConfigTreeQueries, file_id: FileId) -> PointerCmp { +fn recursive_local(db: &dyn ConfigTreeQueries, file_id: FileId) -> PointerCmp { let self_input = db.config_input(file_id); tracing::trace!(?self_input, ?file_id); - match db.config_parent(file_id) { + match db.parent(file_id) { Some(parent) if parent != file_id => { - let parent_computed = db.compute_recursive(parent); + let parent_computed = db.recursive_local(parent); if let Some(input) = self_input.as_deref() { PointerCmp::new(parent_computed.clone_with_overrides(input.local.clone())) } else { @@ -122,8 +123,11 @@ fn compute_recursive(db: &dyn ConfigTreeQueries, file_id: FileId) -> PointerCmp< } } -fn local_config(db: &dyn ConfigTreeQueries, file_id: FileId) -> PointerCmp { - let computed = db.compute_recursive(file_id); +fn computed_local_config( + db: &dyn ConfigTreeQueries, + file_id: FileId, +) -> PointerCmp { + let computed = db.recursive_local(file_id); if let Some(client) = db.client_config() { PointerCmp::new(computed.clone_with_overrides(client.local.clone())) } else { @@ -149,14 +153,35 @@ impl ConfigDb { }; this.set_client_config(None); this.ensure_node(xdg_config_file_id); - this.set_config_parent(xdg_config_file_id, None); this } + /// Gets the value of LocalConfigData for a given `rust-analyzer.toml` FileId. + /// + /// The rust-analyzer.toml does not need to exist on disk. All values are the expression of + /// overriding the parent `rust-analyzer.toml`, set by adding an entry in + /// `ConfigChanges.parent_changes`. + /// + /// If the db is not aware of the given `rust-analyzer.toml` FileId, then the config is read + /// from the user's system-wide default config. + /// + /// Note that the client config overrides all configs. + pub fn local_config(&self, ra_toml_file_id: FileId) -> Arc { + if self.known_file_ids.contains(&ra_toml_file_id) { + self.computed_local_config(ra_toml_file_id).0 + } else { + tracing::warn!(?ra_toml_file_id, "called local_config with unknown file id"); + self.computed_local_config(self.xdg_config_file_id).0 + } + } + + /// Applies a bunch of [`ConfigChanges`]. The FileIds referred to in `ConfigChanges` do not + /// need to exist. You can generate the `parent_changes` hashmap by iterating ancestors of all + /// of the [`ide::SourceRoot`]s, slapping `.map(|path| path.join("rust-analyzer.toml"))`. pub fn apply_changes(&mut self, changes: ConfigChanges, vfs: &Vfs) -> Vec { let mut scratch_errors = Vec::new(); let mut errors = Vec::new(); - let ConfigChanges { client_change, ra_toml_changes, mut parent_changes } = changes; + let ConfigChanges { client_change, ra_toml_changes, parent_changes } = changes; if let Some(change) = client_change { let current = self.client_config(); @@ -174,9 +199,6 @@ impl ConfigDb { // turn and face the strain match change.change_kind { vfs::ChangeKind::Create | vfs::ChangeKind::Modify => { - if change.change_kind == vfs::ChangeKind::Create { - parent_changes.entry(change.file_id).or_insert(ConfigParent::UserDefault); - } let input = parse_toml(change.file_id, vfs, &mut scratch_errors, &mut errors) .map(PointerCmp); tracing::trace!("updating toml for {:?} to {:?}", change.file_id, input); @@ -203,15 +225,25 @@ impl ConfigDb { }; // order of children within the parent node does not matter tracing::trace!("appending child {file_id:?} to {parent_node_id:?}"); - self.set_config_parent(file_id, Some(parent_node_id)) + self.set_parent(file_id, Some(parent_node_id)) } errors } + /// Inserts default values into the salsa inputs for the given file_id + /// if it's never been seen before fn ensure_node(&mut self, file_id: FileId) { if self.known_file_ids.insert(file_id) { self.set_config_input(file_id, None); + self.set_parent( + file_id, + if file_id == self.xdg_config_file_id { + None + } else { + Some(self.xdg_config_file_id) + }, + ); } } } @@ -250,17 +282,15 @@ fn parse_toml( #[cfg(test)] mod tests { - use std::path::PathBuf; + use std::path::{Path, PathBuf}; + use itertools::Itertools; use vfs::{AbsPathBuf, VfsPath}; fn alloc_file_id(vfs: &mut Vfs, s: &str) -> FileId { - tracing_subscriber::fmt().init(); let abs_path = AbsPathBuf::try_from(PathBuf::new().join(s)).unwrap(); let vfs_path = VfsPath::from(abs_path); - // FIXME: the vfs should expose this functionality more simply. - // We shouldn't have to clone the vfs path just to get a FileId. let file_id = vfs.alloc_file_id(vfs_path); vfs.set_file_id_contents(file_id, None); file_id @@ -270,8 +300,6 @@ mod tests { let abs_path = AbsPathBuf::try_from(PathBuf::new().join(s)).unwrap(); let vfs_path = VfsPath::from(abs_path); - // FIXME: the vfs should expose this functionality more simply. - // We shouldn't have to clone the vfs path just to get a FileId. let file_id = vfs.alloc_file_id(vfs_path); vfs.set_file_id_contents(file_id, Some(config.to_string().into_bytes())); file_id @@ -280,6 +308,7 @@ mod tests { use super::*; #[test] fn basic() { + tracing_subscriber::fmt().try_init().ok(); let mut vfs = Vfs::default(); let xdg_config_file_id = alloc_file_id(&mut vfs, "/home/username/.config/rust-analyzer/rust-analyzer.toml"); @@ -364,9 +393,9 @@ mod tests { let prev = local; let local = config_tree.local_config(crate_a); // Should have been recomputed - assert_ne!(prev, local); + assert!(!Arc::ptr_eq(&prev, &local)); // But without changes in between, should give the same Arc back - assert_eq!(local, config_tree.local_config(crate_a)); + assert!(Arc::ptr_eq(&local, &config_tree.local_config(crate_a))); // The newly added xdg_config_file_id should affect the output if nothing else touches // this key @@ -379,4 +408,75 @@ mod tests { assert_eq!(local.completion_autoimport_enable, false); assert_eq!(local.semanticHighlighting_strings_enable, false); } + + #[test] + fn generated_parent_changes() { + tracing_subscriber::fmt().try_init().ok(); + let mut vfs = Vfs::default(); + + let xdg = + alloc_file_id(&mut vfs, "/home/username/.config/rust-analyzer/rust-analyzer.toml"); + let mut config_tree = ConfigDb::new(xdg); + + let project_root = Path::new("/root"); + let sourceroots = + [PathBuf::new().join("/root/crate_a"), PathBuf::new().join("/root/crate_a/crate_b")]; + let sourceroot_tomls = sourceroots + .iter() + .map(|dir| dir.join("rust-analyzer.toml")) + .map(|path| AbsPathBuf::try_from(path).unwrap()) + .map(|path| vfs.alloc_file_id(path.into())) + .collect_vec(); + let &[crate_a, crate_b] = &sourceroot_tomls[..] else { + panic!(); + }; + + let parent_changes = sourceroots + .iter() + .flat_map(|path| { + path.ancestors() + .take_while(|x| x.starts_with(project_root)) + .map(|dir| dir.join("rust-analyzer.toml")) + .map(|path| AbsPathBuf::try_from(path).unwrap()) + .map(|path| vfs.alloc_file_id(path.into())) + .collect_vec() + .into_iter() + .tuple_windows() + .map(|(a, b)| (a, ConfigParent::Parent(b))) + }) + .collect::>(); + + for (&a, parent) in &parent_changes { + eprintln!( + "{a:?} ({:?}): parent = {parent:?} ({:?})", + vfs.file_path(a), + match parent { + ConfigParent::Parent(p) => vfs.file_path(*p).to_string(), + ConfigParent::UserDefault => "xdg".to_string(), + } + ); + } + + vfs.set_file_id_contents( + xdg, + Some(b"[inlayHints.discriminantHints]\nenable = \"always\"".to_vec()), + ); + vfs.set_file_id_contents(crate_a, Some(b"[completion.autoself]\nenable = false".to_vec())); + // note that crate_b's rust-analyzer.toml doesn't exist + + let changes = ConfigChanges { + ra_toml_changes: dbg!(vfs.take_changes()), + parent_changes, + client_change: None, + }; + + dbg!(config_tree.apply_changes(changes, &vfs)); + let local = config_tree.local_config(crate_b); + + assert_eq!( + local.inlayHints_discriminantHints_enable, + crate::config::DiscriminantHintsDef::Always + ); + assert_eq!(local.completion_autoself_enable, false); + } } From 575ebbc5c0d38c37d07237f8c2406e80705ec644 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Thu, 25 Jan 2024 01:36:13 +1100 Subject: [PATCH 15/25] Add ancestors method to AbsPath --- crates/paths/src/lib.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/paths/src/lib.rs b/crates/paths/src/lib.rs index 88b8d0aee3a4..ecb1ed151c18 100644 --- a/crates/paths/src/lib.rs +++ b/crates/paths/src/lib.rs @@ -194,6 +194,10 @@ impl AbsPath { self.0.ends_with(&suffix.0) } + pub fn ancestors(&self) -> impl Iterator { + self.0.ancestors().map(AbsPath::assert) + } + pub fn name_and_extension(&self) -> Option<(&str, Option<&str>)> { Some(( self.file_stem()?.to_str()?, From cc8dce589104d1e7fbd4859057d166714fbcf38e Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Thu, 25 Jan 2024 01:36:36 +1100 Subject: [PATCH 16/25] Make the public api just "here's a list of source roots and a project root" --- crates/rust-analyzer/src/config/tree.rs | 157 ++++++++++++++++-------- 1 file changed, 104 insertions(+), 53 deletions(-) diff --git a/crates/rust-analyzer/src/config/tree.rs b/crates/rust-analyzer/src/config/tree.rs index b1668f3d26ea..2e79c45a7041 100644 --- a/crates/rust-analyzer/src/config/tree.rs +++ b/crates/rust-analyzer/src/config/tree.rs @@ -1,6 +1,7 @@ +use itertools::Itertools; use rustc_hash::{FxHashMap, FxHashSet}; use std::sync::Arc; -use vfs::{FileId, Vfs}; +use vfs::{AbsPathBuf, FileId, Vfs}; use super::{ConfigInput, LocalConfigData, RootLocalConfigData}; @@ -15,12 +16,23 @@ pub enum ConfigTreeError { /// Some rust-analyzer.toml files have changed, and/or the LSP client sent a new configuration. pub struct ConfigChanges { + /// - `None` => no change + /// - `Some(None)` => the client config was removed / reset or something + /// - `Some(Some(...))` => the client config was updated + client_change: Option>>, + set_project_root: Option, + set_source_roots: Option>, ra_toml_changes: Vec, +} + +/// Internal version +struct ConfigChangesInner { /// - `None` => no change /// - `Some(None)` => the client config was removed / reset or something /// - `Some(Some(...))` => the client config was updated client_change: Option>>, parent_changes: FxHashMap, + ra_toml_changes: Vec, } #[derive(Debug)] @@ -140,16 +152,20 @@ pub struct ConfigDb { storage: salsa::Storage, known_file_ids: FxHashSet, xdg_config_file_id: FileId, + source_roots: FxHashSet, + project_root: AbsPathBuf, } impl salsa::Database for ConfigDb {} impl ConfigDb { - pub fn new(xdg_config_file_id: FileId) -> Self { + pub fn new(xdg_config_file_id: FileId, project_root: AbsPathBuf) -> Self { let mut this = Self { storage: Default::default(), known_file_ids: FxHashSet::default(), xdg_config_file_id, + source_roots: FxHashSet::default(), + project_root, }; this.set_client_config(None); this.ensure_node(xdg_config_file_id); @@ -176,12 +192,68 @@ impl ConfigDb { } /// Applies a bunch of [`ConfigChanges`]. The FileIds referred to in `ConfigChanges` do not - /// need to exist. You can generate the `parent_changes` hashmap by iterating ancestors of all - /// of the [`ide::SourceRoot`]s, slapping `.map(|path| path.join("rust-analyzer.toml"))`. - pub fn apply_changes(&mut self, changes: ConfigChanges, vfs: &Vfs) -> Vec { + /// need to exist. + pub fn apply_changes(&mut self, changes: ConfigChanges, vfs: &mut Vfs) -> Vec { + if let Some(new_project_root) = &changes.set_project_root { + self.project_root = new_project_root.clone(); + } + let source_root_change = changes.set_source_roots.as_ref().or_else(|| { + if changes.set_project_root.is_some() { + Some(&self.source_roots) + } else { + None + } + }); + let parent_changes = if let Some(source_roots) = source_root_change { + source_roots + .iter() + .flat_map(|path: &AbsPathBuf| { + path.ancestors() + .take_while(|x| x.starts_with(&self.project_root)) + .map(|dir| dir.join("rust-analyzer.toml")) + .map(|path| vfs.alloc_file_id(path.into())) + .collect_vec() + // immediately get tuple_windows before returning from flat_map + .into_iter() + .tuple_windows() + .map(|(a, b)| (a, ConfigParent::Parent(b))) + }) + .collect::>() + } else { + Default::default() + }; + + if tracing::enabled!(tracing::Level::TRACE) { + for (&a, parent) in &parent_changes { + tracing::trace!( + "{a:?} ({:?}): parent = {parent:?} ({:?})", + vfs.file_path(a), + match parent { + ConfigParent::Parent(p) => vfs.file_path(*p).to_string(), + ConfigParent::UserDefault => "xdg".to_string(), + } + ); + } + } + + // Could delete (self.known_file_ids - parent_changes.keys) here. + + let inner = ConfigChangesInner { + ra_toml_changes: changes.ra_toml_changes, + client_change: changes.client_change, + parent_changes, + }; + self.apply_changes_inner(inner, vfs) + } + + fn apply_changes_inner( + &mut self, + changes: ConfigChangesInner, + vfs: &Vfs, + ) -> Vec { let mut scratch_errors = Vec::new(); let mut errors = Vec::new(); - let ConfigChanges { client_change, ra_toml_changes, parent_changes } = changes; + let ConfigChangesInner { client_change, ra_toml_changes, parent_changes } = changes; if let Some(change) = client_change { let current = self.client_config(); @@ -285,7 +357,7 @@ mod tests { use std::path::{Path, PathBuf}; use itertools::Itertools; - use vfs::{AbsPathBuf, VfsPath}; + use vfs::{AbsPath, AbsPathBuf, VfsPath}; fn alloc_file_id(vfs: &mut Vfs, s: &str) -> FileId { let abs_path = AbsPathBuf::try_from(PathBuf::new().join(s)).unwrap(); @@ -310,11 +382,14 @@ mod tests { fn basic() { tracing_subscriber::fmt().try_init().ok(); let mut vfs = Vfs::default(); + let project_root = AbsPath::assert(Path::new("/root")); let xdg_config_file_id = alloc_file_id(&mut vfs, "/home/username/.config/rust-analyzer/rust-analyzer.toml"); - let mut config_tree = ConfigDb::new(xdg_config_file_id); + let mut config_tree = ConfigDb::new(xdg_config_file_id, project_root.to_path_buf()); + + let source_roots = ["/root/crate_a"].map(Path::new).map(AbsPath::assert); - let root = alloc_config( + let _root = alloc_config( &mut vfs, "/root/rust-analyzer.toml", r#" @@ -335,13 +410,12 @@ mod tests { "#, ); - let mut parent_changes = FxHashMap::default(); - parent_changes.insert(crate_a, ConfigParent::Parent(root)); - + let new_source_roots = source_roots.into_iter().map(|abs| abs.to_path_buf()).collect(); let changes = ConfigChanges { // Normally you will filter these! ra_toml_changes: vfs.take_changes(), - parent_changes, + set_project_root: None, + set_source_roots: Some(new_source_roots), client_change: Some(Some(Arc::new(ConfigInput { local: crate::config::LocalConfigInput { semanticHighlighting_strings_enable: Some(false), @@ -351,7 +425,7 @@ mod tests { }))), }; - dbg!(config_tree.apply_changes(changes, &vfs)); + dbg!(config_tree.apply_changes(changes, &mut vfs)); let local = config_tree.local_config(crate_a); // from root @@ -384,11 +458,12 @@ mod tests { ); let changes = ConfigChanges { - ra_toml_changes: dbg!(vfs.take_changes()), - parent_changes: Default::default(), client_change: None, + set_project_root: None, + set_source_roots: None, + ra_toml_changes: dbg!(vfs.take_changes()), }; - dbg!(config_tree.apply_changes(changes, &vfs)); + dbg!(config_tree.apply_changes(changes, &mut vfs)); let prev = local; let local = config_tree.local_config(crate_a); @@ -410,53 +485,27 @@ mod tests { } #[test] - fn generated_parent_changes() { + fn set_source_roots() { tracing_subscriber::fmt().try_init().ok(); let mut vfs = Vfs::default(); + let project_root = AbsPath::assert(Path::new("/root")); let xdg = alloc_file_id(&mut vfs, "/home/username/.config/rust-analyzer/rust-analyzer.toml"); - let mut config_tree = ConfigDb::new(xdg); + let mut config_tree = ConfigDb::new(xdg, project_root.to_path_buf()); - let project_root = Path::new("/root"); - let sourceroots = - [PathBuf::new().join("/root/crate_a"), PathBuf::new().join("/root/crate_a/crate_b")]; - let sourceroot_tomls = sourceroots + let source_roots = + ["/root/crate_a", "/root/crate_a/crate_b"].map(Path::new).map(AbsPath::assert); + let source_root_tomls = source_roots .iter() .map(|dir| dir.join("rust-analyzer.toml")) .map(|path| AbsPathBuf::try_from(path).unwrap()) .map(|path| vfs.alloc_file_id(path.into())) .collect_vec(); - let &[crate_a, crate_b] = &sourceroot_tomls[..] else { + let &[crate_a, crate_b] = &source_root_tomls[..] else { panic!(); }; - let parent_changes = sourceroots - .iter() - .flat_map(|path| { - path.ancestors() - .take_while(|x| x.starts_with(project_root)) - .map(|dir| dir.join("rust-analyzer.toml")) - .map(|path| AbsPathBuf::try_from(path).unwrap()) - .map(|path| vfs.alloc_file_id(path.into())) - .collect_vec() - .into_iter() - .tuple_windows() - .map(|(a, b)| (a, ConfigParent::Parent(b))) - }) - .collect::>(); - - for (&a, parent) in &parent_changes { - eprintln!( - "{a:?} ({:?}): parent = {parent:?} ({:?})", - vfs.file_path(a), - match parent { - ConfigParent::Parent(p) => vfs.file_path(*p).to_string(), - ConfigParent::UserDefault => "xdg".to_string(), - } - ); - } - vfs.set_file_id_contents( xdg, Some(b"[inlayHints.discriminantHints]\nenable = \"always\"".to_vec()), @@ -464,13 +513,15 @@ mod tests { vfs.set_file_id_contents(crate_a, Some(b"[completion.autoself]\nenable = false".to_vec())); // note that crate_b's rust-analyzer.toml doesn't exist + let new_source_roots = source_roots.into_iter().map(|abs| abs.to_path_buf()).collect(); let changes = ConfigChanges { - ra_toml_changes: dbg!(vfs.take_changes()), - parent_changes, client_change: None, + set_project_root: None, // already set in ConfigDb::new(...) + set_source_roots: Some(new_source_roots), + ra_toml_changes: dbg!(vfs.take_changes()), }; - dbg!(config_tree.apply_changes(changes, &vfs)); + dbg!(config_tree.apply_changes(changes, &mut vfs)); let local = config_tree.local_config(crate_b); assert_eq!( From cbbfa3757358e5b5564131c0653d8d7d9c500ced Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Thu, 25 Jan 2024 01:49:11 +1100 Subject: [PATCH 17/25] Simplify test code --- crates/rust-analyzer/src/config/tree.rs | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/crates/rust-analyzer/src/config/tree.rs b/crates/rust-analyzer/src/config/tree.rs index 2e79c45a7041..6f23dab2485a 100644 --- a/crates/rust-analyzer/src/config/tree.rs +++ b/crates/rust-analyzer/src/config/tree.rs @@ -356,7 +356,6 @@ fn parse_toml( mod tests { use std::path::{Path, PathBuf}; - use itertools::Itertools; use vfs::{AbsPath, AbsPathBuf, VfsPath}; fn alloc_file_id(vfs: &mut Vfs, s: &str) -> FileId { @@ -496,15 +495,9 @@ mod tests { let source_roots = ["/root/crate_a", "/root/crate_a/crate_b"].map(Path::new).map(AbsPath::assert); - let source_root_tomls = source_roots - .iter() + let [crate_a, crate_b] = source_roots .map(|dir| dir.join("rust-analyzer.toml")) - .map(|path| AbsPathBuf::try_from(path).unwrap()) - .map(|path| vfs.alloc_file_id(path.into())) - .collect_vec(); - let &[crate_a, crate_b] = &source_root_tomls[..] else { - panic!(); - }; + .map(|path| vfs.alloc_file_id(path.into())); vfs.set_file_id_contents( xdg, From fb1149e3f8d5e0b0cd1f4814d18113676b960c95 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Thu, 25 Jan 2024 21:58:36 +1100 Subject: [PATCH 18/25] Another test where the source roots are changed --- crates/rust-analyzer/src/config/tree.rs | 26 +++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/crates/rust-analyzer/src/config/tree.rs b/crates/rust-analyzer/src/config/tree.rs index 6f23dab2485a..b3620212771d 100644 --- a/crates/rust-analyzer/src/config/tree.rs +++ b/crates/rust-analyzer/src/config/tree.rs @@ -522,5 +522,31 @@ mod tests { crate::config::DiscriminantHintsDef::Always ); assert_eq!(local.completion_autoself_enable, false); + + // ---- + + // Now move crate b to the root. This gives a new FileId for crate_b/ra.toml. + let source_roots = ["/root/crate_a", "/root/crate_b"].map(Path::new).map(AbsPath::assert); + let [crate_a, crate_b] = source_roots + .map(|dir| dir.join("rust-analyzer.toml")) + .map(|path| vfs.alloc_file_id(path.into())); + let new_source_roots = source_roots.into_iter().map(|abs| abs.to_path_buf()).collect(); + let changes = ConfigChanges { + client_change: None, + set_project_root: None, // already set in ConfigDb::new(...) + set_source_roots: Some(new_source_roots), + ra_toml_changes: dbg!(vfs.take_changes()), + }; + + dbg!(config_tree.apply_changes(changes, &mut vfs)); + let local = config_tree.local_config(crate_b); + + // Still inherits from xdg + assert_eq!( + local.inlayHints_discriminantHints_enable, + crate::config::DiscriminantHintsDef::Always + ); + // new crate_b does not inherit from crate_a + assert_eq!(local.completion_autoself_enable, true); } } From f253aa4cb1281fae3f0073c22e1b2ed09fbab215 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Thu, 25 Jan 2024 21:58:52 +1100 Subject: [PATCH 19/25] Test changing project root --- crates/rust-analyzer/src/config/tree.rs | 82 +++++++++++++++++++++---- 1 file changed, 71 insertions(+), 11 deletions(-) diff --git a/crates/rust-analyzer/src/config/tree.rs b/crates/rust-analyzer/src/config/tree.rs index b3620212771d..2c6555c3f3bd 100644 --- a/crates/rust-analyzer/src/config/tree.rs +++ b/crates/rust-analyzer/src/config/tree.rs @@ -209,6 +209,7 @@ impl ConfigDb { .iter() .flat_map(|path: &AbsPathBuf| { path.ancestors() + // Note that Path::new("/root2/abc").starts_with(Path::new("/root")) is false .take_while(|x| x.starts_with(&self.project_root)) .map(|dir| dir.join("rust-analyzer.toml")) .map(|path| vfs.alloc_file_id(path.into())) @@ -236,7 +237,17 @@ impl ConfigDb { } } - // Could delete (self.known_file_ids - parent_changes.keys) here. + // Remove source roots (& their parent config files) that are no longer part of the project root + self.known_file_ids + .iter() + .cloned() + .filter(|&x| x != self.xdg_config_file_id && !parent_changes.contains_key(&x)) + .collect_vec() + .into_iter() + .for_each(|deleted| { + self.known_file_ids.remove(&deleted); + self.reset_node(deleted); + }); let inner = ConfigChangesInner { ra_toml_changes: changes.ra_toml_changes, @@ -307,17 +318,17 @@ impl ConfigDb { /// if it's never been seen before fn ensure_node(&mut self, file_id: FileId) { if self.known_file_ids.insert(file_id) { - self.set_config_input(file_id, None); - self.set_parent( - file_id, - if file_id == self.xdg_config_file_id { - None - } else { - Some(self.xdg_config_file_id) - }, - ); + self.reset_node(file_id); } } + + fn reset_node(&mut self, file_id: FileId) { + self.set_config_input(file_id, None); + self.set_parent( + file_id, + if file_id == self.xdg_config_file_id { None } else { Some(self.xdg_config_file_id) }, + ); + } } fn parse_toml( @@ -527,7 +538,7 @@ mod tests { // Now move crate b to the root. This gives a new FileId for crate_b/ra.toml. let source_roots = ["/root/crate_a", "/root/crate_b"].map(Path::new).map(AbsPath::assert); - let [crate_a, crate_b] = source_roots + let [_crate_a, crate_b] = source_roots .map(|dir| dir.join("rust-analyzer.toml")) .map(|path| vfs.alloc_file_id(path.into())); let new_source_roots = source_roots.into_iter().map(|abs| abs.to_path_buf()).collect(); @@ -549,4 +560,53 @@ mod tests { // new crate_b does not inherit from crate_a assert_eq!(local.completion_autoself_enable, true); } + + #[test] + fn change_project_root() { + tracing_subscriber::fmt().try_init().ok(); + let mut vfs = Vfs::default(); + + let project_root = AbsPath::assert(Path::new("/root")); + let xdg = + alloc_file_id(&mut vfs, "/home/username/.config/rust-analyzer/rust-analyzer.toml"); + let mut config_tree = ConfigDb::new(xdg, project_root.to_path_buf()); + + let source_roots = ["/root/crate_a"].map(Path::new).map(AbsPath::assert); + let crate_a = vfs.alloc_file_id(source_roots[0].join("rust-analyzer.toml").into()); + + let _root = alloc_config( + &mut vfs, + "/root/rust-analyzer.toml", + r#" + [completion.autoself] + enable = false + "#, + ); + + let new_source_roots = source_roots.into_iter().map(|abs| abs.to_path_buf()).collect(); + let changes = ConfigChanges { + client_change: None, + set_project_root: None, // already set in ConfigDb::new(...) + set_source_roots: Some(new_source_roots), + ra_toml_changes: dbg!(vfs.take_changes()), + }; + config_tree.apply_changes(changes, &mut vfs); + let local = config_tree.local_config(crate_a); + // initially crate_a is part of the project root, so it does inherit + // from /root/rust-analyzer.toml + assert_eq!(local.completion_autoself_enable, false); + + // change project root + let changes = ConfigChanges { + client_change: None, + set_project_root: Some(AbsPath::assert(Path::new("/ro")).to_path_buf()), + set_source_roots: None, + ra_toml_changes: dbg!(vfs.take_changes()), + }; + config_tree.apply_changes(changes, &mut vfs); + // crate_a is now outside the project root and hence inherit (1) xdg (2) + // crate_a/ra.toml, but not /root/rust-analyzer.toml any more + let local = config_tree.local_config(crate_a); + assert_eq!(local.completion_autoself_enable, true); + } } From 407480397cc21976c84f4f89260aa5182fd7c68c Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Thu, 25 Jan 2024 22:06:46 +1100 Subject: [PATCH 20/25] add failing test --- crates/rust-analyzer/src/config/tree.rs | 48 +++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/crates/rust-analyzer/src/config/tree.rs b/crates/rust-analyzer/src/config/tree.rs index 2c6555c3f3bd..85bb22964930 100644 --- a/crates/rust-analyzer/src/config/tree.rs +++ b/crates/rust-analyzer/src/config/tree.rs @@ -609,4 +609,52 @@ mod tests { let local = config_tree.local_config(crate_a); assert_eq!(local.completion_autoself_enable, true); } + + #[test] + fn no_change_to_source_roots() { + tracing_subscriber::fmt().try_init().ok(); + let mut vfs = Vfs::default(); + + let project_root = AbsPath::assert(Path::new("/root")); + let xdg = + alloc_file_id(&mut vfs, "/home/username/.config/rust-analyzer/rust-analyzer.toml"); + let mut config_tree = ConfigDb::new(xdg, project_root.to_path_buf()); + + let source_roots = ["/root/crate_a"].map(Path::new).map(AbsPath::assert); + let crate_a = vfs.alloc_file_id(source_roots[0].join("rust-analyzer.toml").into()); + + let _root = alloc_config( + &mut vfs, + "/root/rust-analyzer.toml", + r#" + [completion.autoself] + enable = false + "#, + ); + + let new_source_roots = source_roots.into_iter().map(|abs| abs.to_path_buf()).collect(); + let changes = ConfigChanges { + client_change: None, + set_project_root: None, // already set in ConfigDb::new(...) + set_source_roots: Some(new_source_roots), + ra_toml_changes: dbg!(vfs.take_changes()), + }; + config_tree.apply_changes(changes, &mut vfs); + let local = config_tree.local_config(crate_a); + // initially crate_a is part of the project root, so it does inherit + // from /root/rust-analyzer.toml + assert_eq!(local.completion_autoself_enable, false); + + let changes = ConfigChanges { + client_change: None, + set_project_root: None, + set_source_roots: None, + ra_toml_changes: dbg!(vfs.take_changes()), + }; + config_tree.apply_changes(changes, &mut vfs); + let local = config_tree.local_config(crate_a); + // initially crate_a is part of the project root, so it does inherit + // from /root/rust-analyzer.toml + assert_eq!(local.completion_autoself_enable, false); + } } From 6906ae9e1e35ebe219c2d8bf7e8b2ae9655d01e2 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Thu, 25 Jan 2024 22:06:53 +1100 Subject: [PATCH 21/25] And fix it --- crates/rust-analyzer/src/config/tree.rs | 30 +++++++++++++------------ 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/crates/rust-analyzer/src/config/tree.rs b/crates/rust-analyzer/src/config/tree.rs index 85bb22964930..2e5ad04b934d 100644 --- a/crates/rust-analyzer/src/config/tree.rs +++ b/crates/rust-analyzer/src/config/tree.rs @@ -205,7 +205,7 @@ impl ConfigDb { } }); let parent_changes = if let Some(source_roots) = source_root_change { - source_roots + let parent_changes = source_roots .iter() .flat_map(|path: &AbsPathBuf| { path.ancestors() @@ -219,7 +219,21 @@ impl ConfigDb { .tuple_windows() .map(|(a, b)| (a, ConfigParent::Parent(b))) }) - .collect::>() + .collect::>(); + + // Remove source roots (& their parent config files) that are no longer part of the project root + self.known_file_ids + .iter() + .cloned() + .filter(|&x| x != self.xdg_config_file_id && !parent_changes.contains_key(&x)) + .collect_vec() + .into_iter() + .for_each(|deleted| { + self.known_file_ids.remove(&deleted); + self.reset_node(deleted); + }); + + parent_changes } else { Default::default() }; @@ -237,18 +251,6 @@ impl ConfigDb { } } - // Remove source roots (& their parent config files) that are no longer part of the project root - self.known_file_ids - .iter() - .cloned() - .filter(|&x| x != self.xdg_config_file_id && !parent_changes.contains_key(&x)) - .collect_vec() - .into_iter() - .for_each(|deleted| { - self.known_file_ids.remove(&deleted); - self.reset_node(deleted); - }); - let inner = ConfigChangesInner { ra_toml_changes: changes.ra_toml_changes, client_change: changes.client_change, From c2461358b1a7929fac3c4b35cb003567d81a0297 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Thu, 25 Jan 2024 22:08:14 +1100 Subject: [PATCH 22/25] Fix comment in test --- crates/rust-analyzer/src/config/tree.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/rust-analyzer/src/config/tree.rs b/crates/rust-analyzer/src/config/tree.rs index 2e5ad04b934d..a4cb4d4909ea 100644 --- a/crates/rust-analyzer/src/config/tree.rs +++ b/crates/rust-analyzer/src/config/tree.rs @@ -647,6 +647,7 @@ mod tests { // from /root/rust-analyzer.toml assert_eq!(local.completion_autoself_enable, false); + // Send in an empty change, should have no effect let changes = ConfigChanges { client_change: None, set_project_root: None, @@ -655,8 +656,6 @@ mod tests { }; config_tree.apply_changes(changes, &mut vfs); let local = config_tree.local_config(crate_a); - // initially crate_a is part of the project root, so it does inherit - // from /root/rust-analyzer.toml assert_eq!(local.completion_autoself_enable, false); } } From bf1419a846c0922cced39a7b46dbb2e9a25e5044 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Fri, 26 Jan 2024 01:24:40 +1100 Subject: [PATCH 23/25] Make AbsPath::assert work on plain string slices too --- crates/paths/src/lib.rs | 3 +- crates/rust-analyzer/src/config/tree.rs | 44 +++++++++++-------------- 2 files changed, 22 insertions(+), 25 deletions(-) diff --git a/crates/paths/src/lib.rs b/crates/paths/src/lib.rs index ecb1ed151c18..55bd5376dfa8 100644 --- a/crates/paths/src/lib.rs +++ b/crates/paths/src/lib.rs @@ -136,7 +136,8 @@ impl AbsPath { /// # Panics /// /// Panics if `path` is not absolute. - pub fn assert(path: &Path) -> &AbsPath { + pub fn assert + ?Sized>(path: &P) -> &AbsPath { + let path = path.as_ref(); assert!(path.is_absolute()); unsafe { &*(path as *const Path as *const AbsPath) } } diff --git a/crates/rust-analyzer/src/config/tree.rs b/crates/rust-analyzer/src/config/tree.rs index a4cb4d4909ea..de54cc6aa220 100644 --- a/crates/rust-analyzer/src/config/tree.rs +++ b/crates/rust-analyzer/src/config/tree.rs @@ -367,13 +367,12 @@ fn parse_toml( #[cfg(test)] mod tests { - use std::path::{Path, PathBuf}; + use std::path::PathBuf; use vfs::{AbsPath, AbsPathBuf, VfsPath}; fn alloc_file_id(vfs: &mut Vfs, s: &str) -> FileId { - let abs_path = AbsPathBuf::try_from(PathBuf::new().join(s)).unwrap(); - + let abs_path = AbsPath::assert(s).to_path_buf(); let vfs_path = VfsPath::from(abs_path); let file_id = vfs.alloc_file_id(vfs_path); vfs.set_file_id_contents(file_id, None); @@ -381,25 +380,26 @@ mod tests { } fn alloc_config(vfs: &mut Vfs, s: &str, config: &str) -> FileId { - let abs_path = AbsPathBuf::try_from(PathBuf::new().join(s)).unwrap(); - + let abs_path = AbsPath::assert(s).to_path_buf(); let vfs_path = VfsPath::from(abs_path); let file_id = vfs.alloc_file_id(vfs_path); vfs.set_file_id_contents(file_id, Some(config.to_string().into_bytes())); file_id } + const XDG_CONFIG_HOME_RATOML: &'static str = + "/home/username/.config/rust-analyzer/rust-analyzer.toml"; + use super::*; #[test] fn basic() { tracing_subscriber::fmt().try_init().ok(); let mut vfs = Vfs::default(); - let project_root = AbsPath::assert(Path::new("/root")); - let xdg_config_file_id = - alloc_file_id(&mut vfs, "/home/username/.config/rust-analyzer/rust-analyzer.toml"); + let project_root = AbsPath::assert("/root"); + let xdg_config_file_id = alloc_file_id(&mut vfs, XDG_CONFIG_HOME_RATOML); let mut config_tree = ConfigDb::new(xdg_config_file_id, project_root.to_path_buf()); - let source_roots = ["/root/crate_a"].map(Path::new).map(AbsPath::assert); + let source_roots = ["/root/crate_a"].map(AbsPath::assert); let _root = alloc_config( &mut vfs, @@ -501,13 +501,11 @@ mod tests { tracing_subscriber::fmt().try_init().ok(); let mut vfs = Vfs::default(); - let project_root = AbsPath::assert(Path::new("/root")); - let xdg = - alloc_file_id(&mut vfs, "/home/username/.config/rust-analyzer/rust-analyzer.toml"); + let project_root = AbsPath::assert("/root"); + let xdg = alloc_file_id(&mut vfs, XDG_CONFIG_HOME_RATOML); let mut config_tree = ConfigDb::new(xdg, project_root.to_path_buf()); - let source_roots = - ["/root/crate_a", "/root/crate_a/crate_b"].map(Path::new).map(AbsPath::assert); + let source_roots = ["/root/crate_a", "/root/crate_a/crate_b"].map(AbsPath::assert); let [crate_a, crate_b] = source_roots .map(|dir| dir.join("rust-analyzer.toml")) .map(|path| vfs.alloc_file_id(path.into())); @@ -539,7 +537,7 @@ mod tests { // ---- // Now move crate b to the root. This gives a new FileId for crate_b/ra.toml. - let source_roots = ["/root/crate_a", "/root/crate_b"].map(Path::new).map(AbsPath::assert); + let source_roots = ["/root/crate_a", "/root/crate_b"].map(AbsPath::assert); let [_crate_a, crate_b] = source_roots .map(|dir| dir.join("rust-analyzer.toml")) .map(|path| vfs.alloc_file_id(path.into())); @@ -568,12 +566,11 @@ mod tests { tracing_subscriber::fmt().try_init().ok(); let mut vfs = Vfs::default(); - let project_root = AbsPath::assert(Path::new("/root")); - let xdg = - alloc_file_id(&mut vfs, "/home/username/.config/rust-analyzer/rust-analyzer.toml"); + let project_root = AbsPath::assert("/root"); + let xdg = alloc_file_id(&mut vfs, XDG_CONFIG_HOME_RATOML); let mut config_tree = ConfigDb::new(xdg, project_root.to_path_buf()); - let source_roots = ["/root/crate_a"].map(Path::new).map(AbsPath::assert); + let source_roots = ["/root/crate_a"].map(AbsPath::assert); let crate_a = vfs.alloc_file_id(source_roots[0].join("rust-analyzer.toml").into()); let _root = alloc_config( @@ -601,7 +598,7 @@ mod tests { // change project root let changes = ConfigChanges { client_change: None, - set_project_root: Some(AbsPath::assert(Path::new("/ro")).to_path_buf()), + set_project_root: Some(AbsPath::assert("/ro").to_path_buf()), set_source_roots: None, ra_toml_changes: dbg!(vfs.take_changes()), }; @@ -617,12 +614,11 @@ mod tests { tracing_subscriber::fmt().try_init().ok(); let mut vfs = Vfs::default(); - let project_root = AbsPath::assert(Path::new("/root")); - let xdg = - alloc_file_id(&mut vfs, "/home/username/.config/rust-analyzer/rust-analyzer.toml"); + let project_root = AbsPath::assert("/root"); + let xdg = alloc_file_id(&mut vfs, XDG_CONFIG_HOME_RATOML); let mut config_tree = ConfigDb::new(xdg, project_root.to_path_buf()); - let source_roots = ["/root/crate_a"].map(Path::new).map(AbsPath::assert); + let source_roots = ["/root/crate_a"].map(AbsPath::assert); let crate_a = vfs.alloc_file_id(source_roots[0].join("rust-analyzer.toml").into()); let _root = alloc_config( From 8177b17502d9ac7b843be71ccd857c1dc4c6983a Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Fri, 26 Jan 2024 01:25:56 +1100 Subject: [PATCH 24/25] Add a failing test for irrelevant vfs changes --- crates/rust-analyzer/src/config/tree.rs | 29 ++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/crates/rust-analyzer/src/config/tree.rs b/crates/rust-analyzer/src/config/tree.rs index de54cc6aa220..ff194a027449 100644 --- a/crates/rust-analyzer/src/config/tree.rs +++ b/crates/rust-analyzer/src/config/tree.rs @@ -5,7 +5,7 @@ use vfs::{AbsPathBuf, FileId, Vfs}; use super::{ConfigInput, LocalConfigData, RootLocalConfigData}; -#[derive(Debug)] +#[derive(Debug, PartialEq)] pub enum ConfigTreeError { Removed, NonExistent, @@ -654,4 +654,31 @@ mod tests { let local = config_tree.local_config(crate_a); assert_eq!(local.completion_autoself_enable, false); } + + #[test] + fn ignore_irrelevant_vfs_changes() { + tracing_subscriber::fmt().try_init().ok(); + let mut vfs = Vfs::default(); + + let project_root = AbsPath::assert("/root"); + let xdg = alloc_file_id(&mut vfs, XDG_CONFIG_HOME_RATOML); + let mut config_tree = ConfigDb::new(xdg, project_root.to_path_buf()); + + // The main way an irrelevant vfs file change is going to show up is in TOML parse errors. + let invalid_utf8 = b"\xc3\x28"; + vfs.set_file_contents( + AbsPath::assert("/irrelevant/file.bin").to_path_buf().into(), + Some(invalid_utf8.to_vec()), + ); + let errors = config_tree.apply_changes( + ConfigChanges { + client_change: None, + set_project_root: None, + set_source_roots: None, + ra_toml_changes: vfs.take_changes(), + }, + &mut vfs, + ); + assert_eq!(errors, vec![]); + } } From 512295c0fd6d2ab55ab8416db2dd17d78c29227e Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Fri, 26 Jan 2024 01:32:21 +1100 Subject: [PATCH 25/25] Fix the failing test for irrelevant vfs changes --- crates/rust-analyzer/src/config/tree.rs | 35 +++++++++++++------------ 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/crates/rust-analyzer/src/config/tree.rs b/crates/rust-analyzer/src/config/tree.rs index ff194a027449..ef2803f3d1d7 100644 --- a/crates/rust-analyzer/src/config/tree.rs +++ b/crates/rust-analyzer/src/config/tree.rs @@ -280,7 +280,25 @@ impl ConfigDb { } } + for (file_id, parent) in parent_changes { + self.ensure_node(file_id); + let parent_node_id = match parent { + ConfigParent::Parent(parent_file_id) => { + self.ensure_node(parent_file_id); + parent_file_id + } + ConfigParent::UserDefault if file_id == self.xdg_config_file_id => continue, + ConfigParent::UserDefault => self.xdg_config_file_id, + }; + self.set_parent(file_id, Some(parent_node_id)) + } + for change in ra_toml_changes { + if !self.known_file_ids.contains(&change.file_id) { + // Irrelevant Vfs change. Ideally you would not pass these in at all, but it's not + // a problem to filter them out here. + continue; + } // turn and face the strain match change.change_kind { vfs::ChangeKind::Create | vfs::ChangeKind::Modify => { @@ -288,31 +306,14 @@ impl ConfigDb { .map(PointerCmp); tracing::trace!("updating toml for {:?} to {:?}", change.file_id, input); - self.ensure_node(change.file_id); self.set_config_input(change.file_id, input); } vfs::ChangeKind::Delete => { - self.ensure_node(change.file_id); self.set_config_input(change.file_id, None); } } } - for (file_id, parent) in parent_changes { - self.ensure_node(file_id); - let parent_node_id = match parent { - ConfigParent::Parent(parent_file_id) => { - self.ensure_node(parent_file_id); - parent_file_id - } - ConfigParent::UserDefault if file_id == self.xdg_config_file_id => continue, - ConfigParent::UserDefault => self.xdg_config_file_id, - }; - // order of children within the parent node does not matter - tracing::trace!("appending child {file_id:?} to {parent_node_id:?}"); - self.set_parent(file_id, Some(parent_node_id)) - } - errors }