From 253a944aca7ed76348b50a766ab647b7c205527c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 17 Oct 2023 10:49:33 -0500 Subject: [PATCH 1/5] test(remove): Add a more extensive formatting test --- tests/testsuite/cargo_remove/mod.rs | 1 + .../optional_dep_feature_formatting/in | 1 + .../optional_dep_feature_formatting/mod.rs | 35 +++++++++++++++++++ .../out/Cargo.toml | 23 ++++++++++++ .../stderr.log | 1 + .../stdout.log | 0 6 files changed, 61 insertions(+) create mode 120000 tests/testsuite/cargo_remove/optional_dep_feature_formatting/in create mode 100644 tests/testsuite/cargo_remove/optional_dep_feature_formatting/mod.rs create mode 100644 tests/testsuite/cargo_remove/optional_dep_feature_formatting/out/Cargo.toml create mode 100644 tests/testsuite/cargo_remove/optional_dep_feature_formatting/stderr.log create mode 100644 tests/testsuite/cargo_remove/optional_dep_feature_formatting/stdout.log diff --git a/tests/testsuite/cargo_remove/mod.rs b/tests/testsuite/cargo_remove/mod.rs index 4403e242554..7b9190642ca 100644 --- a/tests/testsuite/cargo_remove/mod.rs +++ b/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; diff --git a/tests/testsuite/cargo_remove/optional_dep_feature_formatting/in b/tests/testsuite/cargo_remove/optional_dep_feature_formatting/in new file mode 120000 index 00000000000..7fd0ba5ebcc --- /dev/null +++ b/tests/testsuite/cargo_remove/optional_dep_feature_formatting/in @@ -0,0 +1 @@ +../remove-basic.in/ \ No newline at end of file diff --git a/tests/testsuite/cargo_remove/optional_dep_feature_formatting/mod.rs b/tests/testsuite/cargo_remove/optional_dep_feature_formatting/mod.rs new file mode 100644 index 00000000000..ce8fcf71227 --- /dev/null +++ b/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(["--dev", "serde"]) + .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); +} diff --git a/tests/testsuite/cargo_remove/optional_dep_feature_formatting/out/Cargo.toml b/tests/testsuite/cargo_remove/optional_dep_feature_formatting/out/Cargo.toml new file mode 100644 index 00000000000..63112d33424 --- /dev/null +++ b/tests/testsuite/cargo_remove/optional_dep_feature_formatting/out/Cargo.toml @@ -0,0 +1,23 @@ +[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 = "0.6" +rustc-serialize = "0.4" +semver = "0.1" +toml = "0.1" +clippy = "0.4" + +[dev-dependencies] +regex = "0.1.1" + +[features] +std = ["semver/std"] diff --git a/tests/testsuite/cargo_remove/optional_dep_feature_formatting/stderr.log b/tests/testsuite/cargo_remove/optional_dep_feature_formatting/stderr.log new file mode 100644 index 00000000000..d3656ec540e --- /dev/null +++ b/tests/testsuite/cargo_remove/optional_dep_feature_formatting/stderr.log @@ -0,0 +1 @@ + Removing serde from dev-dependencies diff --git a/tests/testsuite/cargo_remove/optional_dep_feature_formatting/stdout.log b/tests/testsuite/cargo_remove/optional_dep_feature_formatting/stdout.log new file mode 100644 index 00000000000..e69de29bb2d From b17a73df1d9f98dca5abf2d12f58ba34dd464b85 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 17 Oct 2023 10:55:14 -0500 Subject: [PATCH 2/5] test(remove): Extend the formatting tested --- .../optional_dep_feature_formatting/in | 1 - .../in/Cargo.toml | 42 +++++++++++++++++++ .../in/src/lib.rs | 1 + .../optional_dep_feature_formatting/mod.rs | 2 +- .../out/Cargo.toml | 9 ++-- .../stderr.log | 3 +- 6 files changed, 50 insertions(+), 8 deletions(-) delete mode 120000 tests/testsuite/cargo_remove/optional_dep_feature_formatting/in create mode 100644 tests/testsuite/cargo_remove/optional_dep_feature_formatting/in/Cargo.toml create mode 100644 tests/testsuite/cargo_remove/optional_dep_feature_formatting/in/src/lib.rs diff --git a/tests/testsuite/cargo_remove/optional_dep_feature_formatting/in b/tests/testsuite/cargo_remove/optional_dep_feature_formatting/in deleted file mode 120000 index 7fd0ba5ebcc..00000000000 --- a/tests/testsuite/cargo_remove/optional_dep_feature_formatting/in +++ /dev/null @@ -1 +0,0 @@ -../remove-basic.in/ \ No newline at end of file diff --git a/tests/testsuite/cargo_remove/optional_dep_feature_formatting/in/Cargo.toml b/tests/testsuite/cargo_remove/optional_dep_feature_formatting/in/Cargo.toml new file mode 100644 index 00000000000..01755d687a3 --- /dev/null +++ b/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 +] diff --git a/tests/testsuite/cargo_remove/optional_dep_feature_formatting/in/src/lib.rs b/tests/testsuite/cargo_remove/optional_dep_feature_formatting/in/src/lib.rs new file mode 100644 index 00000000000..8b137891791 --- /dev/null +++ b/tests/testsuite/cargo_remove/optional_dep_feature_formatting/in/src/lib.rs @@ -0,0 +1 @@ + diff --git a/tests/testsuite/cargo_remove/optional_dep_feature_formatting/mod.rs b/tests/testsuite/cargo_remove/optional_dep_feature_formatting/mod.rs index ce8fcf71227..69406387b81 100644 --- a/tests/testsuite/cargo_remove/optional_dep_feature_formatting/mod.rs +++ b/tests/testsuite/cargo_remove/optional_dep_feature_formatting/mod.rs @@ -24,7 +24,7 @@ fn case() { snapbox::cmd::Command::cargo_ui() .arg("remove") - .args(["--dev", "serde"]) + .args(["docopt", "toml"]) .current_dir(cwd) .assert() .success() diff --git a/tests/testsuite/cargo_remove/optional_dep_feature_formatting/out/Cargo.toml b/tests/testsuite/cargo_remove/optional_dep_feature_formatting/out/Cargo.toml index 63112d33424..c01931b22eb 100644 --- a/tests/testsuite/cargo_remove/optional_dep_feature_formatting/out/Cargo.toml +++ b/tests/testsuite/cargo_remove/optional_dep_feature_formatting/out/Cargo.toml @@ -10,14 +10,13 @@ path = "src/main.rs" semver = "0.1.0" [dependencies] -docopt = "0.6" -rustc-serialize = "0.4" +rustc-serialize = { version = "0.4", optional = true } semver = "0.1" -toml = "0.1" -clippy = "0.4" +clippy = { version = "0.4", optional = true } [dev-dependencies] regex = "0.1.1" +serde = "1.0.90" [features] -std = ["semver/std"] +std = ["dep:clippy", "dep:rustc-serialize", "serde/std", "semver/std"] diff --git a/tests/testsuite/cargo_remove/optional_dep_feature_formatting/stderr.log b/tests/testsuite/cargo_remove/optional_dep_feature_formatting/stderr.log index d3656ec540e..7bceb0f9445 100644 --- a/tests/testsuite/cargo_remove/optional_dep_feature_formatting/stderr.log +++ b/tests/testsuite/cargo_remove/optional_dep_feature_formatting/stderr.log @@ -1 +1,2 @@ - Removing serde from dev-dependencies + Removing docopt from dependencies + Removing toml from dependencies From 5374e8aa70fb15f614715c2c7a4b488bbb06a7e0 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 17 Oct 2023 11:02:52 -0500 Subject: [PATCH 3/5] fix(remove): Leave formatting to the user Fixes #11743 --- src/cargo/util/toml_mut/manifest.rs | 5 ----- .../cargo_remove/multiple_dev/out/Cargo.toml | 2 +- .../optional_dep_feature/out/Cargo.toml | 2 +- .../optional_dep_feature_formatting/out/Cargo.toml | 14 +++++++++++++- 4 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/cargo/util/toml_mut/manifest.rs b/src/cargo/util/toml_mut/manifest.rs index 7d51f6b5c05..1ac1007d954 100644 --- a/src/cargo/util/toml_mut/manifest.rs +++ b/src/cargo/util/toml_mut/manifest.rs @@ -496,11 +496,6 @@ fn fix_feature_activations( 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(); - } if status == DependencyStatus::Required { for value in feature_values.iter_mut() { diff --git a/tests/testsuite/cargo_remove/multiple_dev/out/Cargo.toml b/tests/testsuite/cargo_remove/multiple_dev/out/Cargo.toml index d961b2bb135..b435e33ebf2 100644 --- a/tests/testsuite/cargo_remove/multiple_dev/out/Cargo.toml +++ b/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"] diff --git a/tests/testsuite/cargo_remove/optional_dep_feature/out/Cargo.toml b/tests/testsuite/cargo_remove/optional_dep_feature/out/Cargo.toml index 63112d33424..f9613bd3092 100644 --- a/tests/testsuite/cargo_remove/optional_dep_feature/out/Cargo.toml +++ b/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"] diff --git a/tests/testsuite/cargo_remove/optional_dep_feature_formatting/out/Cargo.toml b/tests/testsuite/cargo_remove/optional_dep_feature_formatting/out/Cargo.toml index c01931b22eb..c535b19749d 100644 --- a/tests/testsuite/cargo_remove/optional_dep_feature_formatting/out/Cargo.toml +++ b/tests/testsuite/cargo_remove/optional_dep_feature_formatting/out/Cargo.toml @@ -19,4 +19,16 @@ regex = "0.1.1" serde = "1.0.90" [features] -std = ["dep:clippy", "dep:rustc-serialize", "serde/std", "semver/std"] +std = [ + # Leading clippy + "dep:clippy", # 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 toml +] From c2590639610fa3fb4dd4eddb03c5ea6ec59c7215 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 17 Oct 2023 11:09:02 -0500 Subject: [PATCH 4/5] refactor(remove): Pull out array removal code --- src/cargo/util/toml_mut/manifest.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/cargo/util/toml_mut/manifest.rs b/src/cargo/util/toml_mut/manifest.rs index 1ac1007d954..a1a8ba8f64c 100644 --- a/src/cargo/util/toml_mut/manifest.rs +++ b/src/cargo/util/toml_mut/manifest.rs @@ -494,7 +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); + remove_array_index(feature_values, *idx); } if status == DependencyStatus::Required { @@ -538,3 +538,7 @@ 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) { + array.remove(index); +} From 6281109e5b3720be140d167e76457a6a5677fe16 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 17 Oct 2023 11:56:54 -0500 Subject: [PATCH 5/5] fix(remove): Carry comments across removes --- src/cargo/util/toml_mut/manifest.rs | 39 ++++++++++++++++++- .../out/Cargo.toml | 10 ++++- 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/src/cargo/util/toml_mut/manifest.rs b/src/cargo/util/toml_mut/manifest.rs index a1a8ba8f64c..80562b03f49 100644 --- a/src/cargo/util/toml_mut/manifest.rs +++ b/src/cargo/util/toml_mut/manifest.rs @@ -540,5 +540,42 @@ fn non_existent_dependency_err( } fn remove_array_index(array: &mut toml_edit::Array, index: usize) { - array.remove(index); + 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); + } } diff --git a/tests/testsuite/cargo_remove/optional_dep_feature_formatting/out/Cargo.toml b/tests/testsuite/cargo_remove/optional_dep_feature_formatting/out/Cargo.toml index c535b19749d..0398c8beb06 100644 --- a/tests/testsuite/cargo_remove/optional_dep_feature_formatting/out/Cargo.toml +++ b/tests/testsuite/cargo_remove/optional_dep_feature_formatting/out/Cargo.toml @@ -21,7 +21,10 @@ serde = "1.0.90" [features] std = [ # Leading clippy - "dep:clippy", # trailing docopt + "dep:clippy", # trailing clippy + + # Leading docopt + # trailing docopt # Leading rustc-serialize "dep:rustc-serialize", # trailing rustc-serialize @@ -30,5 +33,8 @@ std = [ "serde/std", # trailing serde/std # Leading semver/std - "semver/std", # trailing toml + "semver/std", # trailing semver/std + + # Leading toml + # trailing toml ]