Apply requested changes round 3

rust-lang · Apr 29, 2024 · 861713e · 861713e
1 parent 09c9963
commit 861713e
Show file tree

Hide file tree

Showing 7 changed files with 36 additions and 31 deletions.
diff --git a/crates/rust-analyzer/src/bin/main.rs b/crates/rust-analyzer/src/bin/main.rs
@@ -228,8 +228,10 @@ fn run_server() -> anyhow::Result<()> {
     if let Some(json) = initialization_options {
         let mut change = ConfigChange::default();
         change.change_client_config(json);
-        let mut error_sink = ConfigError::default();
-        (config, _) = config.apply_change(change, &mut error_sink);
+
+        let error_sink: ConfigError;
+        (config, error_sink, _) = config.apply_change(change);
+
         if !error_sink.is_empty() {
             use lsp_types::{
                 notification::{Notification, ShowMessage},

diff --git a/crates/rust-analyzer/src/cli/scip.rs b/crates/rust-analyzer/src/cli/scip.rs
@@ -14,7 +14,7 @@ use tracing::error;
 
 use crate::{
     cli::flags,
-    config::{ConfigChange, ConfigError},
+    config::ConfigChange,
     line_index::{LineEndings, LineIndex, PositionEncoding},
 };
 
@@ -45,8 +45,10 @@ impl flags::Scip {
             let json = serde_json::from_reader(&mut file)?;
             let mut change = ConfigChange::default();
             change.change_client_config(json);
-            let mut error_sink = ConfigError::default();
-            (config, _) = config.apply_change(change, &mut error_sink);
+
+            let error_sink;
+            (config, error_sink, _) = config.apply_change(change);
+
             // FIXME @alibektas : What happens to errors without logging?
             error!(?error_sink, "Config Error(s)");
         }

diff --git a/crates/rust-analyzer/src/config.rs b/crates/rust-analyzer/src/config.rs
@@ -701,7 +701,7 @@ impl Config {
     // FIXME @alibektas : Server's health uses error sink but in other places it is not used atm.
     /// Changes made to client and global configurations will partially not be reflected even after `.apply_change()` was called.
     /// The return tuple's bool component signals whether the `GlobalState` should call its `update_configuration()` method.
-    pub fn apply_change(
+    fn apply_change_with_sink(
         &self,
         change: ConfigChange,
         error_sink: &mut ConfigError,
@@ -805,10 +805,13 @@ impl Config {
         (config, should_update)
     }
 
-    pub fn apply_change_whatever(&self, change: ConfigChange) -> (Config, ConfigError) {
+    /// Given `change` this generates a new `Config`, thereby collecting errors of type `ConfigError`.
+    /// If there are changes that have global/client level effect, the last component of the return type
+    /// will be set to `true`, which should be used by the `GlobalState` to update itself.
+    pub fn apply_change(&self, change: ConfigChange) -> (Config, ConfigError, bool) {
         let mut e = ConfigError(vec![]);
-        let (config, _) = self.apply_change(change, &mut e);
-        (config, e)
+        let (config, should_update) = self.apply_change_with_sink(change, &mut e);
+        (config, e, should_update)
     }
 }
 
@@ -3293,8 +3296,7 @@ mod tests {
                 "server": null,
         }}));
 
-        let mut error_sink = ConfigError::default();
-        (config, _) = config.apply_change(change, &mut error_sink);
+        (config, _, _) = config.apply_change(change);
         assert_eq!(config.proc_macro_srv(), None);
     }
 
@@ -3313,8 +3315,7 @@ mod tests {
             "server": project_root().display().to_string(),
         }}));
 
-        let mut error_sink = ConfigError::default();
-        (config, _) = config.apply_change(change, &mut error_sink);
+        (config, _, _) = config.apply_change(change);
         assert_eq!(config.proc_macro_srv(), Some(AbsPathBuf::try_from(project_root()).unwrap()));
     }
 
@@ -3335,8 +3336,7 @@ mod tests {
             "server": "./server"
         }}));
 
-        let mut error_sink = ConfigError::default();
-        (config, _) = config.apply_change(change, &mut error_sink);
+        (config, _, _) = config.apply_change(change);
 
         assert_eq!(
             config.proc_macro_srv(),
@@ -3360,8 +3360,7 @@ mod tests {
             "rust" : { "analyzerTargetDir" : null }
         }));
 
-        let mut error_sink = ConfigError::default();
-        (config, _) = config.apply_change(change, &mut error_sink);
+        (config, _, _) = config.apply_change(change);
         assert_eq!(config.cargo_targetDir(), &None);
         assert!(
             matches!(config.flycheck(), FlycheckConfig::CargoCommand { options, .. } if options.target_dir.is_none())
@@ -3383,8 +3382,7 @@ mod tests {
             "rust" : { "analyzerTargetDir" : true }
         }));
 
-        let mut error_sink = ConfigError::default();
-        (config, _) = config.apply_change(change, &mut error_sink);
+        (config, _, _) = config.apply_change(change);
 
         assert_eq!(config.cargo_targetDir(), &Some(TargetDirectory::UseSubdirectory(true)));
         assert!(
@@ -3407,8 +3405,7 @@ mod tests {
             "rust" : { "analyzerTargetDir" : "other_folder" }
         }));
 
-        let mut error_sink = ConfigError::default();
-        (config, _) = config.apply_change(change, &mut error_sink);
+        (config, _, _) = config.apply_change(change);
 
         assert_eq!(
             config.cargo_targetDir(),

diff --git a/crates/rust-analyzer/src/global_state.rs b/crates/rust-analyzer/src/global_state.rs
@@ -435,8 +435,8 @@ impl GlobalState {
 
                 change
             };
-            let mut error_sink = ConfigError::default();
-            let (config, should_update) = self.config.apply_change(config_change, &mut error_sink);
+
+            let (config, _, should_update) = self.config.apply_change(config_change);
 
             if should_update {
                 self.update_configuration(config);

diff --git a/crates/rust-analyzer/src/handlers/notification.rs b/crates/rust-analyzer/src/handlers/notification.rs
@@ -13,7 +13,7 @@ use triomphe::Arc;
 use vfs::{AbsPathBuf, ChangeKind, VfsPath};
 
 use crate::{
-    config::{Config, ConfigChange, ConfigError},
+    config::{Config, ConfigChange},
     global_state::GlobalState,
     lsp::{from_proto, utils::apply_document_changes},
     lsp_ext::{self, RunFlycheckParams},
@@ -200,8 +200,9 @@ pub(crate) fn handle_did_change_configuration(
                         let mut config = Config::clone(&*this.config);
                         let mut change = ConfigChange::default();
                         change.change_client_config(json.take());
-                        let mut error_sink = ConfigError::default();
-                        (config, _) = config.apply_change(change, &mut error_sink);
+
+                        (config, _, _) = config.apply_change(change);
+
                         // Client config changes neccesitates .update_config method to be called.
                         this.update_configuration(config);
                     }

diff --git a/crates/rust-analyzer/src/reload.rs b/crates/rust-analyzer/src/reload.rs
@@ -33,7 +33,7 @@ use triomphe::Arc;
 use vfs::{AbsPath, AbsPathBuf, ChangeKind};
 
 use crate::{
-    config::{Config, ConfigChange, ConfigError, FilesWatcher, LinkedProject},
+    config::{Config, ConfigChange, FilesWatcher, LinkedProject},
     global_state::GlobalState,
     lsp_ext,
     main_loop::Task,
@@ -621,8 +621,9 @@ impl GlobalState {
 
         let mut config_change = ConfigChange::default();
         config_change.change_source_root_parent_map(self.local_roots_parent_map.clone());
-        let mut error_sink = ConfigError::default();
-        let (config, _) = self.config.apply_change(config_change, &mut error_sink);
+
+        let (config, _, _) = self.config.apply_change(config_change);
+
         self.config = Arc::new(config);
 
         self.recreate_crate_graph(cause);

diff --git a/crates/rust-analyzer/tests/slow-tests/support.rs b/crates/rust-analyzer/tests/slow-tests/support.rs
@@ -206,9 +206,11 @@ impl Project<'_> {
         let mut change = ConfigChange::default();
 
         change.change_client_config(self.config);
-        let mut error_sink = ConfigError::default();
+
+        let error_sink: ConfigError;
+        (config, error_sink, _) = config.apply_change(change);
         assert!(error_sink.is_empty(), "{error_sink:?}");
-        (config, _) = config.apply_change(change, &mut error_sink);
+
         config.rediscover_workspaces();
 
         Server::new(tmp_dir.keep(), config)