Auto merge of #12838 - epage:add, r=weihanglo

fix(add): Preserve more comments

### What does this PR try to resolve?

This was fixed in killercup/cargo-edit#725 and I forgot to carry it over to here.

I kept in some auto-formatting because there is little to preserve until the TOML 1.1 spec is out which will allow mult-line inline-tables which means there might be comments interspersed.  We'll see which comes first, `cargo fmt` support for `Cargo.toml` or the 1.1 spec.

Fixes #10850

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

First commit adds a test demonstrating the problem.  The second makes a lot of noise that the third cleans up, so its a mixed bag as to which commit to look at.

### Additional information

Author: bors

src/cargo/util/toml_mut/dependency.rs

@@ -464,17 +464,17 @@ impl Dependency {
         } else if let Some(table) = item.as_table_like_mut() {
             match &self.source {
                 Some(Source::Registry(src)) => {
-                    table.insert("version", toml_edit::value(src.version.as_str()));
+                    overwrite_value(table, "version", src.version.as_str());
 
                     for key in ["path", "git", "branch", "tag", "rev", "workspace"] {
                         table.remove(key);
                     }
                 }
                 Some(Source::Path(src)) => {
                     let relpath = path_field(crate_root, &src.path);
-                    table.insert("path", toml_edit::value(relpath));
+                    overwrite_value(table, "path", relpath);
                     if let Some(r) = src.version.as_deref() {
-                        table.insert("version", toml_edit::value(r));
+                        overwrite_value(table, "version", r);
                     } else {
                         table.remove("version");
                     }
@@ -484,24 +484,24 @@ impl Dependency {
                     }
                 }
                 Some(Source::Git(src)) => {
-                    table.insert("git", toml_edit::value(src.git.as_str()));
+                    overwrite_value(table, "git", src.git.as_str());
                     if let Some(branch) = src.branch.as_deref() {
-                        table.insert("branch", toml_edit::value(branch));
+                        overwrite_value(table, "branch", branch);
                     } else {
                         table.remove("branch");
                     }
                     if let Some(tag) = src.tag.as_deref() {
-                        table.insert("tag", toml_edit::value(tag));
+                        overwrite_value(table, "tag", tag);
                     } else {
                         table.remove("tag");
                     }
                     if let Some(rev) = src.rev.as_deref() {
-                        table.insert("rev", toml_edit::value(rev));
+                        overwrite_value(table, "rev", rev);
                     } else {
                         table.remove("rev");
                     }
                     if let Some(r) = src.version.as_deref() {
-                        table.insert("version", toml_edit::value(r));
+                        overwrite_value(table, "version", r);
                     } else {
                         table.remove("version");
                     }
@@ -511,7 +511,7 @@ impl Dependency {
                     }
                 }
                 Some(Source::Workspace(_)) => {
-                    table.insert("workspace", toml_edit::value(true));
+                    overwrite_value(table, "workspace", true);
                     table.set_dotted(true);
                     key.fmt();
                     for key in [
@@ -533,7 +533,7 @@ impl Dependency {
             }
             if table.contains_key("version") {
                 if let Some(r) = self.registry.as_deref() {
-                    table.insert("registry", toml_edit::value(r));
+                    overwrite_value(table, "registry", r);
                 } else {
                     table.remove("registry");
                 }
@@ -542,11 +542,11 @@ impl Dependency {
             }
 
             if self.rename.is_some() {
-                table.insert("package", toml_edit::value(self.name.as_str()));
+                overwrite_value(table, "package", self.name.as_str());
             }
             match self.default_features {
                 Some(v) => {
-                    table.insert("default-features", toml_edit::value(v));
+                    overwrite_value(table, "default-features", v);
                 }
                 None => {
                     table.remove("default-features");
@@ -564,29 +564,40 @@ impl Dependency {
                     })
                     .unwrap_or_default();
                 features.extend(new_features.iter().map(|s| s.as_str()));
-                let features = toml_edit::value(features.into_iter().collect::<toml_edit::Value>());
+                let features = features.into_iter().collect::<toml_edit::Value>();
                 table.set_dotted(false);
-                table.insert("features", features);
+                overwrite_value(table, "features", features);
             } else {
                 table.remove("features");
             }
             match self.optional {
                 Some(v) => {
                     table.set_dotted(false);
-                    table.insert("optional", toml_edit::value(v));
+                    overwrite_value(table, "optional", v);
                 }
                 None => {
                     table.remove("optional");
                 }
             }
-
-            table.fmt();
         } else {
             unreachable!("Invalid dependency type: {}", item.type_name());
         }
     }
 }
 
+fn overwrite_value(
+    table: &mut dyn toml_edit::TableLike,
+    key: &str,
+    value: impl Into<toml_edit::Value>,
+) {
+    let mut value = value.into();
+    let existing = table.entry(key).or_insert_with(|| Default::default());
+    if let Some(existing_value) = existing.as_value() {
+        *value.decor_mut() = existing_value.decor().clone();
+    }
+    *existing = toml_edit::Item::Value(value);
+}
+
 fn invalid_type(dep: &str, key: &str, actual: &str, expected: &str) -> anyhow::Error {
     anyhow::format_err!("Found {actual} for {key} when {expected} was expected for {dep}")
 }

src/cargo/util/toml_mut/manifest.rs

@@ -349,13 +349,16 @@ impl LocalManifest {
             .get_key_value_mut(dep_key)
         {
             dep.update_toml(&crate_root, &mut dep_key, dep_item);
+            if let Some(table) = dep_item.as_inline_table_mut() {
+                // So long as we don't have `Cargo.toml` auto-formatting and inline-tables can only
+                // be on one line, there isn't really much in the way of interesting formatting to
+                // include (no comments), so let's just wipe it clean
+                table.fmt();
+            }
         } else {
             let new_dependency = dep.to_toml(&crate_root);
             table[dep_key] = new_dependency;
         }
-        if let Some(t) = table.as_inline_table_mut() {
-            t.fmt()
-        }
 
         Ok(())
     }

tests/testsuite/cargo_add/mod.rs

@@ -99,6 +99,7 @@ mod path_dev;
 mod path_inferred_name;
 mod path_inferred_name_conflicts_full_feature;
 mod path_normalized_name;
+mod preserve_dep_std_table;
 mod preserve_features_table;
 mod preserve_sorted;
 mod preserve_unsorted;

tests/testsuite/cargo_add/preserve_dep_std_table/in/Cargo.toml

@@ -0,0 +1,14 @@
+[package]
+name = "xxx"
+version = "0.1.0"
+edition = "2021"
+
+# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
+
+[dependencies.your-face]
+# Leading version
+version = "99999.0.0"  # Trailing version
+# Leading optional
+optional = true  # Trailing optional
+# Leading features
+features = []  # Trailing features

tests/testsuite/cargo_add/preserve_dep_std_table/in/src/lib.rs

@@ -0,0 +1,31 @@
+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() {
+    cargo_test_support::registry::init();
+    cargo_test_support::registry::Package::new("your-face", "99999.0.0+my-package")
+        .feature("nose", &[])
+        .feature("mouth", &[])
+        .feature("eyes", &[])
+        .feature("ears", &[])
+        .publish();
+
+    let project = Project::from_template(curr_dir!().join("in"));
+    let project_root = project.root();
+    let cwd = &project_root;
+
+    snapbox::cmd::Command::cargo_ui()
+        .arg("add")
+        .arg_line("your-face --no-optional")
+        .current_dir(cwd)
+        .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_add/preserve_dep_std_table/mod.rs

@@ -0,0 +1,12 @@
+[package]
+name = "xxx"
+version = "0.1.0"
+edition = "2021"
+
+# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
+
+[dependencies.your-face]
+# Leading version
+version = "99999.0.0"  # Trailing version
+# Leading features
+features = []  # Trailing features

tests/testsuite/cargo_add/preserve_dep_std_table/out/Cargo.toml

@@ -0,0 +1,7 @@
+    Updating `dummy-registry` index
+      Adding your-face v99999.0.0 to dependencies.
+             Features:
+             - ears
+             - eyes
+             - mouth
+             - nose