Auto merge of #12779 - calavera:workspace_adds_implicit_members, r=epage

Add new packages to [workspace.members] automatically

### What does this PR try to resolve?

If a new package is created in a workspace, this change adds the package's path to the workspace's members list automatically.

It doesn't add the package to the list if the path is in the workspace's exclude list, or if the members list doesn't exist already. I noticed that a `cargo_new` test broke if I create the members list when it doesn't exist. This is because the workspace's manifest can be used for package templating. I think it's better to not break that use case.

Fixes #6378

### How should we test and review this PR?

I've included tests in the `cargo_new` suite.

Author: bors

Cargo.lock

@@ -92,7 +92,7 @@ tempfile = "3.8.0"
 thiserror = "1.0.49"
 time = { version = "0.3", features = ["parsing", "formatting", "serde"] }
 toml = "0.8.2"
-toml_edit = { version = "0.20.2", features = ["serde"] }
+toml_edit = { version = "0.20.7", features = ["serde"] }
 tracing = "0.1.37"
 tracing-subscriber = { version = "0.3.17", features = ["env-filter"] }
 unicase = "2.7.0"

Cargo.toml

@@ -31,6 +31,7 @@ use crate::util::toml_mut::dependency::MaybeWorkspace;
 use crate::util::toml_mut::dependency::PathSource;
 use crate::util::toml_mut::dependency::Source;
 use crate::util::toml_mut::dependency::WorkspaceSource;
+use crate::util::toml_mut::is_sorted;
 use crate::util::toml_mut::manifest::DepTable;
 use crate::util::toml_mut::manifest::LocalManifest;
 use crate::util::RustVersion;
@@ -1004,22 +1005,6 @@ fn format_features_version_suffix(dep: &DependencyUI) -> String {
     }
 }
 
-// Based on Iterator::is_sorted from nightly std; remove in favor of that when stabilized.
-fn is_sorted(mut it: impl Iterator<Item = impl PartialOrd>) -> bool {
-    let Some(mut last) = it.next() else {
-        return true;
-    };
-
-    for curr in it {
-        if curr < last {
-            return false;
-        }
-        last = curr;
-    }
-
-    true
-}
-
 fn find_workspace_dep(toml_key: &str, root_manifest: &Path) -> CargoResult<Dependency> {
     let manifest = LocalManifest::try_new(root_manifest)?;
     let manifest = manifest

src/cargo/ops/cargo_add/mod.rs

@@ -1,10 +1,11 @@
 use crate::core::{Edition, Shell, Workspace};
 use crate::util::errors::CargoResult;
 use crate::util::important_paths::find_root_manifest_for_wd;
+use crate::util::toml_mut::is_sorted;
 use crate::util::{existing_vcs_repo, FossilRepo, GitRepo, HgRepo, PijulRepo};
 use crate::util::{restricted_names, Config};
-use anyhow::{anyhow, Context as _};
-use cargo_util::paths;
+use anyhow::{anyhow, Context};
+use cargo_util::paths::{self, write_atomic};
 use serde::de;
 use serde::Deserialize;
 use std::collections::BTreeMap;
@@ -13,6 +14,7 @@ use std::io::{BufRead, BufReader, ErrorKind};
 use std::path::{Path, PathBuf};
 use std::str::FromStr;
 use std::{fmt, slice};
+use toml_edit::{Array, Value};
 
 #[derive(Clone, Copy, Debug, PartialEq)]
 pub enum VersionControl {
@@ -809,7 +811,7 @@ fn mk(config: &Config, opts: &MkOptions<'_>) -> CargoResult<()> {
         // Sometimes the root manifest is not a valid manifest, so we only try to parse it if it is.
         // This should not block the creation of the new project. It is only a best effort to
         // inherit the workspace package keys.
-        if let Ok(workspace_document) = root_manifest.parse::<toml_edit::Document>() {
+        if let Ok(mut workspace_document) = root_manifest.parse::<toml_edit::Document>() {
             if let Some(workspace_package_keys) = workspace_document
                 .get("workspace")
                 .and_then(|workspace| workspace.get("package"))
@@ -832,6 +834,13 @@ fn mk(config: &Config, opts: &MkOptions<'_>) -> CargoResult<()> {
                 table["workspace"] = toml_edit::value(true);
                 manifest["lints"] = toml_edit::Item::Table(table);
             }
+
+            // Try to add the new package to the workspace members.
+            update_manifest_with_new_member(
+                &root_manifest_path,
+                &mut workspace_document,
+                opts.path,
+            )?;
         }
     }
 
@@ -931,3 +940,95 @@ fn update_manifest_with_inherited_workspace_package_keys(
         try_remove_and_inherit_package_key(key, manifest);
     }
 }
+
+/// Adds the new package member to the [workspace.members] array.
+/// - It first checks if the name matches any element in [workspace.exclude],
+///  and it ignores the name if there is a match.
+/// - Then it check if the name matches any element already in [workspace.members],
+/// and it ignores the name if there is a match.
+/// - If [workspace.members] doesn't exist in the manifest, it will add a new section
+/// with the new package in it.
+fn update_manifest_with_new_member(
+    root_manifest_path: &Path,
+    workspace_document: &mut toml_edit::Document,
+    package_path: &Path,
+) -> CargoResult<()> {
+    // Find the relative path for the package from the workspace root directory.
+    let workspace_root = root_manifest_path.parent().with_context(|| {
+        format!(
+            "workspace root manifest doesn't have a parent directory `{}`",
+            root_manifest_path.display()
+        )
+    })?;
+    let relpath = pathdiff::diff_paths(package_path, workspace_root).with_context(|| {
+        format!(
+            "path comparison requires two absolute paths; package_path: `{}`, workspace_path: `{}`",
+            package_path.display(),
+            workspace_root.display()
+        )
+    })?;
+
+    let mut components = Vec::new();
+    for comp in relpath.iter() {
+        let comp = comp.to_str().with_context(|| {
+            format!("invalid unicode component in path `{}`", relpath.display())
+        })?;
+        components.push(comp);
+    }
+    let display_path = components.join("/");
+
+    // Don't add the new package to the workspace's members
+    // if there is an exclusion match for it.
+    if let Some(exclude) = workspace_document
+        .get("workspace")
+        .and_then(|workspace| workspace.get("exclude"))
+        .and_then(|exclude| exclude.as_array())
+    {
+        for member in exclude {
+            let pat = member
+                .as_str()
+                .with_context(|| format!("invalid non-string exclude path `{}`", member))?;
+            if pat == display_path {
+                return Ok(());
+            }
+        }
+    }
+
+    // If the members element already exist, check if one of the patterns
+    // in the array already includes the new package's relative path.
+    // - Add the relative path if the members don't match the new package's path.
+    // - Create a new members array if there are no members element in the workspace yet.
+    if let Some(members) = workspace_document
+        .get_mut("workspace")
+        .and_then(|workspace| workspace.get_mut("members"))
+        .and_then(|members| members.as_array_mut())
+    {
+        for member in members.iter() {
+            let pat = member
+                .as_str()
+                .with_context(|| format!("invalid non-string member `{}`", member))?;
+            let pattern = glob::Pattern::new(pat)
+                .with_context(|| format!("cannot build glob pattern from `{}`", pat))?;
+
+            if pattern.matches(&display_path) {
+                return Ok(());
+            }
+        }
+
+        let was_sorted = is_sorted(members.iter().map(Value::as_str));
+        members.push(&display_path);
+        if was_sorted {
+            members.sort_by(|lhs, rhs| lhs.as_str().cmp(&rhs.as_str()));
+        }
+    } else {
+        let mut array = Array::new();
+        array.push(&display_path);
+
+        workspace_document["workspace"]["members"] = toml_edit::value(array);
+    }
+
+    write_atomic(
+        &root_manifest_path,
+        workspace_document.to_string().to_string().as_bytes(),
+    )
+}

src/cargo/ops/cargo_new.rs

@@ -11,3 +11,19 @@
 
 pub mod dependency;
 pub mod manifest;
+
+// Based on Iterator::is_sorted from nightly std; remove in favor of that when stabilized.
+pub fn is_sorted(mut it: impl Iterator<Item = impl PartialOrd>) -> bool {
+    let Some(mut last) = it.next() else {
+        return true;
+    };
+
+    for curr in it {
+        if curr < last {
+            return false;
+        }
+        last = curr;
+    }
+
+    true
+}

src/cargo/util/toml_mut/mod.rs

@@ -42,3 +42,4 @@ mod simple_hg_ignore_exists;
 mod simple_lib;
 mod unknown_flags;
 mod with_argument;
+mod workspace_add_member;

tests/testsuite/cargo_init/mod.rs

@@ -0,0 +1,2 @@
+[workspace]
+resolver = "2"

tests/testsuite/cargo_init/workspace_add_member/in/Cargo.toml

@@ -0,0 +1,21 @@
+use cargo_test_support::compare::assert_ui;
+use cargo_test_support::prelude::*;
+use cargo_test_support::Project;
+
+use cargo_test_support::curr_dir;
+
+#[cargo_test]
+fn case() {
+    let project = Project::from_template(curr_dir!().join("in"));
+    let project_root = &project.root();
+
+    snapbox::cmd::Command::cargo_ui()
+        .arg_line("init --bin --vcs none")
+        .current_dir(project_root.join("crates").join("foo"))
+        .assert()
+        .success()
+        .stdout_matches_path(curr_dir!().join("stdout.log"))
+        .stderr_matches_path(curr_dir!().join("stderr.log"));
+
+    assert_ui().subset_matches(curr_dir!().join("out"), &project_root);
+}

tests/testsuite/cargo_init/empty_dir/.keep renamed to tests/testsuite/cargo_init/workspace_add_member/in/crates/foo/.keep

@@ -0,0 +1,3 @@
+[workspace]
+resolver = "2"
+members = ["crates/foo"]