fix(remove): Preserve feature comments

src/cargo/util/toml_mut/manifest.rs

@@ -494,12 +494,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 {
@@ -543,3 +538,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