Auto merge of #12837 - epage:remove, r=weihanglo

fix(remove): Preserve feature comments

### What does this PR try to resolve?

We've been having a hard time balancing leaving the feature list in a good looking start and preserving formatting.  With our new formatting policy (#12836), we can just choose to preserve formatting instead.

Fixes #11743

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

The first commit copies an existing test.  The second is where the fun begins, customizing the test for some weird cases.  The follow up commits do the slow walk for improving it.

We ended up preserving some line-trailing comments because they come after the comma and toml_edit treats that as part of the prefix of the next item.  Tracking removal of that was going to require us to determine if the newline existed in the suffix or in the next item's prefix and edit accordingly and I decided to skip that to keep this initial implementation simpler.

### Additional information

Author: bors

src/cargo/util/toml_mut/manifest.rs

@@ -497,12 +497,7 @@ fn fix_feature_activations(
 
     // Remove found idx in revers order so we don't invalidate the idx.
     for idx in remove_list.iter().rev() {
-        feature_values.remove(*idx);
-    }
-    if !remove_list.is_empty() {
-        // HACK: Instead of cleaning up the users formatting from having removed a feature, we just
-        // re-format the whole feature list
-        feature_values.fmt();
+        remove_array_index(feature_values, *idx);
     }
 
     if status == DependencyStatus::Required {
@@ -546,3 +541,44 @@ fn non_existent_dependency_err(
 ) -> anyhow::Error {
     anyhow::format_err!("the dependency `{name}` could not be found in `{table}`.")
 }
+
+fn remove_array_index(array: &mut toml_edit::Array, index: usize) {
+    let value = array.remove(index);
+
+    // Captures all lines before leading whitespace
+    let prefix_lines = value
+        .decor()
+        .prefix()
+        .and_then(|p| p.as_str().expect("spans removed").rsplit_once('\n'))
+        .map(|(lines, _current)| lines);
+    // Captures all lines after trailing whitespace, before the next comma
+    let suffix_lines = value
+        .decor()
+        .suffix()
+        .and_then(|p| p.as_str().expect("spans removed").split_once('\n'))
+        .map(|(_current, lines)| lines);
+    let mut merged_lines = String::new();
+    if let Some(prefix_lines) = prefix_lines {
+        merged_lines.push_str(prefix_lines);
+        merged_lines.push('\n');
+    }
+    if let Some(suffix_lines) = suffix_lines {
+        merged_lines.push_str(suffix_lines);
+        merged_lines.push('\n');
+    }
+
+    let next_index = index; // Since `index` was removed, that effectively auto-advances us
+    if let Some(next) = array.get_mut(next_index) {
+        let next_decor = next.decor_mut();
+        let next_prefix = next_decor
+            .prefix()
+            .map(|s| s.as_str().expect("spans removed"))
+            .unwrap_or_default();
+        merged_lines.push_str(next_prefix);
+        next_decor.set_prefix(merged_lines);
+    } else {
+        let trailing = array.trailing().as_str().expect("spans removed");
+        merged_lines.push_str(trailing);
+        array.set_trailing(merged_lines);
+    }
+}

tests/testsuite/cargo_remove/mod.rs

@@ -20,6 +20,7 @@ mod multiple_dev;
 mod no_arg;
 mod offline;
 mod optional_dep_feature;
+mod optional_dep_feature_formatting;
 mod optional_feature;
 mod package;
 mod remove_basic;

tests/testsuite/cargo_remove/multiple_dev/out/Cargo.toml

@@ -17,4 +17,4 @@ toml = "0.1"
 clippy = "0.4"
 
 [features]
-std = ["semver/std"]
+std = [ "semver/std"]

tests/testsuite/cargo_remove/optional_dep_feature/out/Cargo.toml

@@ -20,4 +20,4 @@ clippy = "0.4"
 regex = "0.1.1"
 
 [features]
-std = ["semver/std"]
+std = [ "semver/std"]

tests/testsuite/cargo_remove/optional_dep_feature_formatting/in/Cargo.toml

@@ -0,0 +1,42 @@
+[package]
+name = "cargo-remove-test-fixture"
+version = "0.1.0"
+
+[[bin]]
+name = "main"
+path = "src/main.rs"
+
+[build-dependencies]
+semver = "0.1.0"
+
+[dependencies]
+docopt = { version = "0.6", optional = true }
+rustc-serialize = { version = "0.4", optional = true }
+semver = "0.1"
+toml = { version = "0.1", optional = true }
+clippy = { version = "0.4", optional = true }
+
+[dev-dependencies]
+regex = "0.1.1"
+serde = "1.0.90"
+
+[features]
+std = [
+  # Leading clippy
+  "dep:clippy",  # trailing clippy
+
+  # Leading docopt
+  "dep:docopt",  # trailing docopt
+
+  # Leading rustc-serialize
+  "dep:rustc-serialize",  # trailing rustc-serialize
+
+  # Leading serde/std
+  "serde/std",  # trailing serde/std
+
+  # Leading semver/std
+  "semver/std",  # trailing semver/std
+
+  # Leading toml
+  "dep:toml",  # trailing toml
+]

tests/testsuite/cargo_remove/optional_dep_feature_formatting/in/src/lib.rs

@@ -0,0 +1 @@
+

tests/testsuite/cargo_remove/optional_dep_feature_formatting/mod.rs

@@ -0,0 +1,35 @@
+use cargo_test_support::compare::assert_ui;
+use cargo_test_support::curr_dir;
+use cargo_test_support::CargoCommand;
+use cargo_test_support::Project;
+
+#[cargo_test]
+fn case() {
+    cargo_test_support::registry::init();
+    cargo_test_support::registry::Package::new("clippy", "0.4.0+my-package").publish();
+    cargo_test_support::registry::Package::new("docopt", "0.6.2+my-package").publish();
+    cargo_test_support::registry::Package::new("regex", "0.1.1+my-package").publish();
+    cargo_test_support::registry::Package::new("rustc-serialize", "0.4.0+my-package").publish();
+    cargo_test_support::registry::Package::new("toml", "0.1.1+my-package").publish();
+    cargo_test_support::registry::Package::new("semver", "0.1.1")
+        .feature("std", &[])
+        .publish();
+    cargo_test_support::registry::Package::new("serde", "1.0.90")
+        .feature("std", &[])
+        .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("remove")
+        .args(["docopt", "toml"])
+        .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_remove/optional_dep_feature_formatting/out/Cargo.toml

@@ -0,0 +1,40 @@
+[package]
+name = "cargo-remove-test-fixture"
+version = "0.1.0"
+
+[[bin]]
+name = "main"
+path = "src/main.rs"
+
+[build-dependencies]
+semver = "0.1.0"
+
+[dependencies]
+rustc-serialize = { version = "0.4", optional = true }
+semver = "0.1"
+clippy = { version = "0.4", optional = true }
+
+[dev-dependencies]
+regex = "0.1.1"
+serde = "1.0.90"
+
+[features]
+std = [
+  # Leading clippy
+  "dep:clippy",  # trailing clippy
+
+  # Leading docopt
+  # trailing docopt
+
+  # Leading rustc-serialize
+  "dep:rustc-serialize",  # trailing rustc-serialize
+
+  # Leading serde/std
+  "serde/std",  # trailing serde/std
+
+  # Leading semver/std
+  "semver/std",  # trailing semver/std
+
+  # Leading toml
+  # trailing toml
+]

tests/testsuite/cargo_remove/optional_dep_feature_formatting/stderr.log

@@ -0,0 +1,2 @@
+    Removing docopt from dependencies
+    Removing toml from dependencies