Auto merge of #18059 - Wilfred:config_cleanups, r=Veykril

fix: Updating settings should not clobber discovered projects

`linkedProjects` is owned by the user's configuration, so when users update this setting, `linkedProjects` is reset. This is problematic when `linkedProjects` also contains projects discovered with `discoverCommand`.

The buggy behaviour occurred when:

(1) The user configures `discoverCommand` and loads a Rust project.

(2) The user changes any setting in VS Code, so rust-analyzer receives `workspace/didChangeConfiguration`.

(3) `handle_did_change_configuration` ultimately calls `Client::apply_change_with_sink()`, which updates
`config.user_config` and discards any items we added in `linkedProjects`.

Instead, separate out `discovered_projects_from_filesystem` and `discovered_projects_from_command` from user configuration, so user settings cannot affect any type of discovered project.

This fixes the subtle issue mentioned here: #17246 (comment)

Author: bors

crates/project-model/src/lib.rs

@@ -52,6 +52,15 @@ pub use crate::{
 };
 pub use cargo_metadata::Metadata;
 
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct ProjectJsonFromCommand {
+    /// The data describing this project, such as its dependencies.
+    pub data: ProjectJsonData,
+    /// The build system specific file that describes this project,
+    /// such as a `my-project/BUCK` file.
+    pub buildfile: AbsPathBuf,
+}
+
 #[derive(Debug, Clone, PartialEq, Eq, Hash, Ord, PartialOrd)]
 pub enum ProjectManifest {
     ProjectJson(ManifestPath),

crates/project-model/src/project_json.rs

@@ -66,6 +66,8 @@ pub struct ProjectJson {
     /// e.g. `path/to/sysroot/lib/rustlib/src/rust`
     pub(crate) sysroot_src: Option<AbsPathBuf>,
     project_root: AbsPathBuf,
+    /// The path to the rust-project.json file. May be None if this
+    /// data was generated by the discoverConfig command.
     manifest: Option<ManifestPath>,
     crates: Vec<Crate>,
     /// Configuration for CLI commands.

crates/rust-analyzer/src/config.rs

@@ -24,7 +24,8 @@ use ide_db::{
 use itertools::Itertools;
 use paths::{Utf8Path, Utf8PathBuf};
 use project_model::{
-    CargoConfig, CargoFeatures, ProjectJson, ProjectJsonData, ProjectManifest, RustLibSource,
+    CargoConfig, CargoFeatures, ProjectJson, ProjectJsonData, ProjectJsonFromCommand,
+    ProjectManifest, RustLibSource,
 };
 use rustc_hash::{FxHashMap, FxHashSet};
 use semver::Version;
@@ -761,7 +762,13 @@ enum RatomlFile {
 
 #[derive(Debug, Clone)]
 pub struct Config {
-    discovered_projects: Vec<ProjectManifest>,
+    /// Projects that have a Cargo.toml or a rust-project.json in a
+    /// parent directory, so we can discover them by walking the
+    /// file system.
+    discovered_projects_from_filesystem: Vec<ProjectManifest>,
+    /// Projects whose configuration was generated by a command
+    /// configured in discoverConfig.
+    discovered_projects_from_command: Vec<ProjectJsonFromCommand>,
     /// The workspace roots as registered by the LSP client
     workspace_roots: Vec<AbsPathBuf>,
     caps: ClientCapabilities,
@@ -1054,19 +1061,19 @@ impl Config {
         (config, e, should_update)
     }
 
-    pub fn add_linked_projects(&mut self, data: ProjectJsonData, buildfile: AbsPathBuf) {
-        let linked_projects = &mut self.client_config.0.global.linkedProjects;
-
-        let new_project = ManifestOrProjectJson::DiscoveredProjectJson { data, buildfile };
-        match linked_projects {
-            Some(projects) => {
-                match projects.iter_mut().find(|p| p.manifest() == new_project.manifest()) {
-                    Some(p) => *p = new_project,
-                    None => projects.push(new_project),
-                }
+    pub fn add_discovered_project_from_command(
+        &mut self,
+        data: ProjectJsonData,
+        buildfile: AbsPathBuf,
+    ) {
+        for proj in self.discovered_projects_from_command.iter_mut() {
+            if proj.buildfile == buildfile {
+                proj.data = data;
+                return;
             }
-            None => *linked_projects = Some(vec![new_project]),
         }
+
+        self.discovered_projects_from_command.push(ProjectJsonFromCommand { data, buildfile });
     }
 }
 
@@ -1344,7 +1351,8 @@ impl Config {
 
         Config {
             caps: ClientCapabilities::new(caps),
-            discovered_projects: Vec::new(),
+            discovered_projects_from_filesystem: Vec::new(),
+            discovered_projects_from_command: Vec::new(),
             root_path,
             snippets: Default::default(),
             workspace_roots,
@@ -1365,7 +1373,7 @@ impl Config {
         if discovered.is_empty() {
             tracing::error!("failed to find any projects in {:?}", &self.workspace_roots);
         }
-        self.discovered_projects = discovered;
+        self.discovered_projects_from_filesystem = discovered;
     }
 
     pub fn remove_workspace(&mut self, path: &AbsPath) {
@@ -1687,42 +1695,59 @@ impl Config {
         self.workspace_discoverConfig().as_ref()
     }
 
-    pub fn linked_or_discovered_projects(&self) -> Vec<LinkedProject> {
-        match self.linkedProjects().as_slice() {
-            [] => {
-                let exclude_dirs: Vec<_> =
-                    self.files_excludeDirs().iter().map(|p| self.root_path.join(p)).collect();
-                self.discovered_projects
-                    .iter()
-                    .filter(|project| {
-                        !exclude_dirs.iter().any(|p| project.manifest_path().starts_with(p))
-                    })
-                    .cloned()
-                    .map(LinkedProject::from)
-                    .collect()
+    fn discovered_projects(&self) -> Vec<ManifestOrProjectJson> {
+        let exclude_dirs: Vec<_> =
+            self.files_excludeDirs().iter().map(|p| self.root_path.join(p)).collect();
+
+        let mut projects = vec![];
+        for fs_proj in &self.discovered_projects_from_filesystem {
+            let manifest_path = fs_proj.manifest_path();
+            if exclude_dirs.iter().any(|p| manifest_path.starts_with(p)) {
+                continue;
             }
-            linked_projects => linked_projects
-                .iter()
-                .filter_map(|linked_project| match linked_project {
-                    ManifestOrProjectJson::Manifest(it) => {
-                        let path = self.root_path.join(it);
-                        ProjectManifest::from_manifest_file(path)
-                            .map_err(|e| tracing::error!("failed to load linked project: {}", e))
-                            .ok()
-                            .map(Into::into)
-                    }
-                    ManifestOrProjectJson::DiscoveredProjectJson { data, buildfile } => {
-                        let root_path =
-                            buildfile.parent().expect("Unable to get parent of buildfile");
 
-                        Some(ProjectJson::new(None, root_path, data.clone()).into())
-                    }
-                    ManifestOrProjectJson::ProjectJson(it) => {
-                        Some(ProjectJson::new(None, &self.root_path, it.clone()).into())
-                    }
-                })
-                .collect(),
+            let buf: Utf8PathBuf = manifest_path.to_path_buf().into();
+            projects.push(ManifestOrProjectJson::Manifest(buf));
+        }
+
+        for dis_proj in &self.discovered_projects_from_command {
+            projects.push(ManifestOrProjectJson::DiscoveredProjectJson {
+                data: dis_proj.data.clone(),
+                buildfile: dis_proj.buildfile.clone(),
+            });
         }
+
+        projects
+    }
+
+    pub fn linked_or_discovered_projects(&self) -> Vec<LinkedProject> {
+        let linked_projects = self.linkedProjects();
+        let projects = if linked_projects.is_empty() {
+            self.discovered_projects()
+        } else {
+            linked_projects.clone()
+        };
+
+        projects
+            .iter()
+            .filter_map(|linked_project| match linked_project {
+                ManifestOrProjectJson::Manifest(it) => {
+                    let path = self.root_path.join(it);
+                    ProjectManifest::from_manifest_file(path)
+                        .map_err(|e| tracing::error!("failed to load linked project: {}", e))
+                        .ok()
+                        .map(Into::into)
+                }
+                ManifestOrProjectJson::DiscoveredProjectJson { data, buildfile } => {
+                    let root_path = buildfile.parent().expect("Unable to get parent of buildfile");
+
+                    Some(ProjectJson::new(None, root_path, data.clone()).into())
+                }
+                ManifestOrProjectJson::ProjectJson(it) => {
+                    Some(ProjectJson::new(None, &self.root_path, it.clone()).into())
+                }
+            })
+            .collect()
     }
 
     pub fn prefill_caches(&self) -> bool {
@@ -2282,18 +2307,6 @@ where
     se.serialize_str(path.as_str())
 }
 
-impl ManifestOrProjectJson {
-    fn manifest(&self) -> Option<&Utf8Path> {
-        match self {
-            ManifestOrProjectJson::Manifest(manifest) => Some(manifest),
-            ManifestOrProjectJson::DiscoveredProjectJson { buildfile, .. } => {
-                Some(buildfile.as_ref())
-            }
-            ManifestOrProjectJson::ProjectJson(_) => None,
-        }
-    }
-}
-
 #[derive(Serialize, Deserialize, Debug, Clone)]
 #[serde(rename_all = "snake_case")]
 enum ExprFillDefaultDef {

crates/rust-analyzer/src/main_loop.rs

@@ -875,7 +875,7 @@ impl GlobalState {
                 self.discover_workspace_queue.op_completed(());
 
                 let mut config = Config::clone(&*self.config);
-                config.add_linked_projects(project, buildfile);
+                config.add_discovered_project_from_command(project, buildfile);
                 self.update_configuration(config);
             }
             DiscoverProjectMessage::Progress { message } => {