Auto merge of #11099 - cassaundra:cargo-remove, r=epage

Import `cargo remove` into cargo

## What does this PR try to resolve?

This PR merges `cargo remove` from [cargo-edit](https://github.com/killercup/cargo-edit) into cargo.

### Motivation

- General approval from community, see #5586 and #10520.
- Satisfying symmetry between add and remove.
- Help users clean up their manifests (for example, when users forget to remove optional dependencies from feature lists).

With #10472, cargo-add was added to cargo. As part of that discussion, it was also proposed that `cargo rm` (now `cargo remove`) eventually be added as well.

### Drawbacks

- Additional code always opens the door for more bugs and features
  - The scope of this command is fairly small though
  - Known bugs and most known features were resolved before this merge proposal

### Behavior

`cargo remove` operates on one or more dependencies from a manifest, removing them from a specified dependencies section (using the same flags as `cargo-add`) and from `[features]` activations if the dependency is optional. Feature lists themselves are not automatically removed when made empty.  Like with cargo-add, the lock file is automatically updated.

Note: like `cargo add`, `cargo remove` refers to dependency names, rather than crate names, which can be different with the presence of the `name` field.

Note: `cargo rm` has been renamed to `cargo remove`, based on prior art and user feedback (see [discussion](#10520)). Although this renaming is arguably an improvement, adding an `rm` alias could make the switch easier for existing users of cargo-edit (at the cost of a naming conflict which would merit insta-stabilization).

#### Help output

<details>

  ```shell
  $ cargo run -- remove --help
  cargo-remove
  Remove dependencies from a Cargo.toml manifest file

  USAGE:
      cargo remove [OPTIONS] <DEP_ID>...

  ARGS:
      <DEP_ID>...    Dependencies to be removed

  OPTIONS:
      -p, --package [<SPEC>...]     Package to remove from
      -v, --verbose                 Use verbose output (-vv very verbose/build.rs output)
          --manifest-path <PATH>    Path to Cargo.toml
          --offline                 Run without accessing the network
      -q, --quiet                   Do not print cargo log messages
          --dry-run                 Don't actually write the manifest
      -Z <FLAG>                     Unstable (nightly-only) flags to Cargo, see 'cargo -Z help' for details
      -h, --help                    Print help information

  SECTION:
          --dev                Remove as development dependency
          --build              Remove as build dependency
          --target <TARGET>    Remove as dependency from the given target platform
  ```

</details>

#### Example usage

```
cargo remove serde
cargo remove criterion httpmock --dev
cargo remove winhttp --target x86_64-pc-windows-gnu
cargo remove --package core toml
```

## How should we test and review this PR?

This is following the pattern from cargo-add which was implemented in three different PRs (implementation, documentation, and completions), in the interest of reducing the focusing discussions in each PR and allowing cargo-add's behavior to settle to avoid documentation churn.

1. #10472
2. #10578
3. #10577

The remaining changes (documentation and shell completions) will follow shortly after.

Some work has already begun on this feature in #11059.

Work on this feature was carried out on the [`merge-rm`](killercup/cargo-edit@master...merge-rm) branch of cargo-edit with PRs reviewed by `@epage.` If you are interested in seeing how this feature evolved to better match cargo's internals, you might find the commit history there to be helpful. As this PR is reviewed, changes will be made both here and on that branch, with the commit history being fully maintained on the latter.

`cargo remove` is structured like most other subcommands:

- `src/bin/cargo/commands/remove.rs` contains the cli handling and top-level execution.
- `src/cargo/ops/cargo_remove.rs` contains the implementation of the feature itself.

In order to support this feature, the `remove_from_table` util was added to `util::toml_mut::manifest::LocalManifest`.

Tests are split out into a separate commit to make it easier to review the production code and tests.  Tests have been implemented with `snapbox`, structured similarly to the tests of `cargo add`.

### Prior art

- Python: [`poetry remove`](https://python-poetry.org/docs/cli/#remove)
  - Supports dry run
- JavaScript: [`yarn remove`](https://yarnpkg.com/cli/remove)
  - Supports wildcards
- JavaScript: [`pnpm remove`](https://pnpm.io/cli/remove)
- Go: [`go get`](https://go.dev/ref/mod#go-get)
  - `go get foo@none` to remove
- Julia: [`pkg rm`](https://docs.julialang.org/en/v1/stdlib/Pkg/)
  - Supports `--all` to remove all dependencies
- Ruby: [`bundle remove`](https://bundler.io/v2.2/man/bundle-remove.1.html)
- Dart: [`dart pub remove`](https://dart.dev/tools/pub/cmd/pub-remove)
  - Supports dry run
- Lua: [`luarocks remove`](https://github.com/luarocks/luarocks/wiki/remove)
  - Supports force remove
- .NET: [`Uninstall-Package`](https://docs.microsoft.com/en-us/nuget/reference/ps-reference/ps-ref-uninstall-package)
  - Supports dry run
  - Supports removal of dependencies
  - Supports force remove (disregards dependencies)
- Haxe: [`haxelib remove`](https://lib.haxe.org/documentation/using-haxelib/#remove)
- Racket: [`raco pkg remove`](https://docs.racket-lang.org/pkg/cmdline.html#%28part._raco-pkg-remove%29)
  - Supports dry run
  - Supports force remove (disregards dependencies)
  - Supports demotion to weak dependency (sort of a corollary of force remove)

### Insta-stabilization

In the discussion of `cargo add`'s stabilization story ([Zulip stream](https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Stablizing.20cargo-add)), it was brought up that the feature might benefit from being insta-stabilized to avoid making the cargo-edit version of the binary hard to access. Since `cargo rm` (from cargo-edit) was renamed to `cargo remove` here, [such a conflict no longer exists](https://crates.io/search?q=cargo%20remove), so this is less of a concern.

Since this feature is already has a had a long run of user testing in cargo-edit and doesn't have unsettled UI questions like cargo-add did, it might still be a candidate for insta-stabilization.

### Deferred work

Necessary future work:

- Add documentation.
- Add shell completions.
- Perform GC on workspace dependencies when they are no longer used (see #8415).
  - This is inspired by a feature from the RFC that was dropped (unused dependencies triggering a warning)
  - This was deferred out to avoid challenges with testing nightly features

It was found in the review of `cargo add` that it was best to defer these first two items to focus the discussion and as there was still behavior churn during the review of cargo-add.

### Future Possibilities

The following are features which we might want to add to `cargo remove` in the future:

- Add a `cargo rm` alias to ease transition for current cargo-edit users
- Automatically convert between dash and underscores in deps: killercup/cargo-edit#690
- Remove unused dependencies: killercup/cargo-edit#415
- Clean up caches: killercup/cargo-edit#647

### Additional information

Fixes #10520.

src/bin/cargo/commands/mod.rs

@@ -26,6 +26,7 @@ pub fn builtin() -> Vec<Command> {
         pkgid::cli(),
         publish::cli(),
         read_manifest::cli(),
+        remove::cli(),
         report::cli(),
         run::cli(),
         rustc::cli(),
@@ -68,6 +69,7 @@ pub fn builtin_exec(cmd: &str) -> Option<fn(&mut Config, &ArgMatches) -> CliResu
         "pkgid" => pkgid::exec,
         "publish" => publish::exec,
         "read-manifest" => read_manifest::exec,
+        "remove" => remove::exec,
         "report" => report::exec,
         "run" => run::exec,
         "rustc" => rustc::exec,
@@ -110,6 +112,7 @@ pub mod package;
 pub mod pkgid;
 pub mod publish;
 pub mod read_manifest;
+pub mod remove;
 pub mod report;
 pub mod run;
 pub mod rustc;

src/bin/cargo/commands/remove.rs

@@ -0,0 +1,118 @@
+use cargo::core::dependency::DepKind;
+use cargo::ops::cargo_remove::remove;
+use cargo::ops::cargo_remove::RemoveOptions;
+use cargo::ops::resolve_ws;
+use cargo::util::command_prelude::*;
+use cargo::util::toml_mut::manifest::DepTable;
+
+pub fn cli() -> clap::Command<'static> {
+    clap::Command::new("remove")
+        // Subcommand aliases are handled in `aliased_command()`.
+        // .alias("rm")
+        .setting(clap::AppSettings::DeriveDisplayOrder)
+        .about("Remove dependencies from a Cargo.toml manifest file")
+        .args([clap::Arg::new("dependencies")
+            .action(clap::ArgAction::Append)
+            .required(true)
+            .multiple_values(true)
+            .takes_value(true)
+            .value_name("DEP_ID")
+            .help("Dependencies to be removed")])
+        .arg_package("Package to remove from")
+        .arg_manifest_path()
+        .arg_quiet()
+        .arg_dry_run("Don't actually write the manifest")
+        .next_help_heading("SECTION")
+        .args([
+            clap::Arg::new("dev")
+                .long("dev")
+                .conflicts_with("build")
+                .action(clap::ArgAction::SetTrue)
+                .group("section")
+                .help("Remove as development dependency"),
+            clap::Arg::new("build")
+                .long("build")
+                .conflicts_with("dev")
+                .action(clap::ArgAction::SetTrue)
+                .group("section")
+                .help("Remove as build dependency"),
+            clap::Arg::new("target")
+                .long("target")
+                .takes_value(true)
+                .value_name("TARGET")
+                .value_parser(clap::builder::NonEmptyStringValueParser::new())
+                .help("Remove as dependency from the given target platform"),
+        ])
+}
+
+pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
+    let dry_run = args.dry_run();
+
+    let workspace = args.workspace(config)?;
+    let packages = args.packages_from_flags()?;
+    let packages = packages.get_packages(&workspace)?;
+    let spec = match packages.len() {
+        0 => {
+            return Err(CliError::new(
+                anyhow::format_err!("no packages selected. Please specify one with `-p <PKG_ID>`"),
+                101,
+            ));
+        }
+        1 => packages[0],
+        len => {
+            return Err(CliError::new(
+                anyhow::format_err!(
+                    "{len} packages selected. Please specify one with `-p <PKG_ID>`",
+                ),
+                101,
+            ));
+        }
+    };
+
+    let dependencies = args
+        .get_many::<String>("dependencies")
+        .expect("required(true)")
+        .cloned()
+        .collect();
+
+    let section = parse_section(args);
+
+    let options = RemoveOptions {
+        config,
+        spec,
+        dependencies,
+        section,
+        dry_run,
+    };
+    remove(&options)?;
+
+    if !dry_run {
+        // Reload the workspace since we've changed dependencies
+        let ws = args.workspace(config)?;
+        resolve_ws(&ws)?;
+    }
+
+    Ok(())
+}
+
+fn parse_section(args: &ArgMatches) -> DepTable {
+    let dev = args.flag("dev");
+    let build = args.flag("build");
+
+    let kind = if dev {
+        DepKind::Development
+    } else if build {
+        DepKind::Build
+    } else {
+        DepKind::Normal
+    };
+
+    let mut table = DepTable::new().set_kind(kind);
+
+    if let Some(target) = args.get_one::<String>("target") {
+        assert!(!target.is_empty(), "Target specification may not be empty");
+        table = table.set_target(target);
+    }
+
+    table
+}

src/bin/cargo/main.rs

@@ -39,12 +39,13 @@ fn main() {
 
 /// Table for defining the aliases which come builtin in `Cargo`.
 /// The contents are structured as: `(alias, aliased_command, description)`.
-const BUILTIN_ALIASES: [(&str, &str, &str); 5] = [
+const BUILTIN_ALIASES: [(&str, &str, &str); 6] = [
     ("b", "build", "alias: build"),
     ("c", "check", "alias: check"),
     ("d", "doc", "alias: doc"),
     ("r", "run", "alias: run"),
     ("t", "test", "alias: test"),
+    ("rm", "remove", "alias: remove"),
 ];
 
 /// Function which contains the list of all of the builtin aliases and it's

src/cargo/ops/cargo_remove.rs

@@ -0,0 +1,65 @@
+//! Core of cargo-remove command
+
+use crate::core::Package;
+use crate::util::toml_mut::manifest::DepTable;
+use crate::util::toml_mut::manifest::LocalManifest;
+use crate::CargoResult;
+use crate::Config;
+
+/// Remove a dependency from a Cargo.toml manifest file.
+#[derive(Debug)]
+pub struct RemoveOptions<'a> {
+    /// Configuration information for Cargo operations
+    pub config: &'a Config,
+    /// Package to remove dependencies from
+    pub spec: &'a Package,
+    /// Dependencies to remove
+    pub dependencies: Vec<String>,
+    /// Which dependency section to remove these from
+    pub section: DepTable,
+    /// Whether or not to actually write the manifest
+    pub dry_run: bool,
+}
+
+/// Remove dependencies from a manifest
+pub fn remove(options: &RemoveOptions<'_>) -> CargoResult<()> {
+    let dep_table = options
+        .section
+        .to_table()
+        .into_iter()
+        .map(String::from)
+        .collect::<Vec<_>>();
+
+    let manifest_path = options.spec.manifest_path().to_path_buf();
+    let mut manifest = LocalManifest::try_new(&manifest_path)?;
+
+    for dep in &options.dependencies {
+        let section = if dep_table.len() >= 3 {
+            format!("{} for target `{}`", &dep_table[2], &dep_table[1])
+        } else {
+            dep_table[0].clone()
+        };
+        options
+            .config
+            .shell()
+            .status("Removing", format!("{dep} from {section}"))?;
+
+        manifest.remove_from_table(&dep_table, dep)?;
+
+        // Now that we have removed the crate, if that was the last reference to that
+        // crate, then we need to drop any explicitly activated features on
+        // that crate.
+        manifest.gc_dep(dep);
+    }
+
+    if options.dry_run {
+        options
+            .config
+            .shell()
+            .warn("aborting remove due to dry run")?;
+    } else {
+        manifest.write()?;
+    }
+
+    Ok(())
+}

src/cargo/ops/mod.rs

@@ -45,6 +45,7 @@ mod cargo_output_metadata;
 mod cargo_package;
 mod cargo_pkgid;
 mod cargo_read_manifest;
+pub mod cargo_remove;
 mod cargo_run;
 mod cargo_test;
 mod cargo_uninstall;

src/cargo/util/toml_mut/manifest.rs

@@ -368,6 +368,26 @@ impl LocalManifest {
         Ok(())
     }
 
+    /// Remove entry from a Cargo.toml.
+    pub fn remove_from_table(&mut self, table_path: &[String], name: &str) -> CargoResult<()> {
+        let parent_table = self.get_table_mut(table_path)?;
+
+        let dep = parent_table
+            .get_mut(name)
+            .filter(|t| !t.is_none())
+            .ok_or_else(|| non_existent_dependency_err(name, table_path.join(".")))?;
+
+        // remove the dependency
+        *dep = toml_edit::Item::None;
+
+        // remove table if empty
+        if parent_table.as_table_like().unwrap().is_empty() {
+            *parent_table = toml_edit::Item::None;
+        }
+
+        Ok(())
+    }
+
     /// Remove references to `dep_key` if its no longer present.
     pub fn gc_dep(&mut self, dep_key: &str) {
         let explicit_dep_activation = self.is_explicit_dep_activation(dep_key);
@@ -504,6 +524,8 @@ fn fix_feature_activations(
             }
         }
     }
+
+    feature_values.fmt();
 }
 
 pub fn str_or_1_len_table(item: &toml_edit::Item) -> bool {
@@ -517,3 +539,10 @@ fn parse_manifest_err() -> anyhow::Error {
 fn non_existent_table_err(table: impl std::fmt::Display) -> anyhow::Error {
     anyhow::format_err!("the table `{table}` could not be found.")
 }
+
+fn non_existent_dependency_err(
+    name: impl std::fmt::Display,
+    table: impl std::fmt::Display,
+) -> anyhow::Error {
+    anyhow::format_err!("the dependency `{name}` could not be found in `{table}`.")
+}

tests/testsuite/cargo_remove/avoid_empty_tables/in

@@ -0,0 +1 @@
+../remove-basic.in/

tests/testsuite/cargo_remove/avoid_empty_tables/mod.rs

@@ -0,0 +1,25 @@
+use cargo_test_support::compare::assert_ui;
+use cargo_test_support::curr_dir;
+use cargo_test_support::CargoCommand;
+use cargo_test_support::Project;
+
+use crate::cargo_remove::init_registry;
+
+#[cargo_test]
+fn case() {
+    init_registry();
+    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(["clippy"])
+        .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/avoid_empty_tables/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"
+
+[dev-dependencies]
+regex = "0.1.1"
+serde = "1.0.90"
+
+[features]
+std = ["serde/std", "semver/std"]

tests/testsuite/cargo_remove/avoid_empty_tables/stderr.log

@@ -0,0 +1,2 @@
+    Removing clippy from dependencies
+    Updating `dummy-registry` index

tests/testsuite/cargo_remove/build/in

@@ -0,0 +1 @@
+../remove-basic.in/

tests/testsuite/cargo_remove/build/mod.rs

@@ -0,0 +1,25 @@
+use cargo_test_support::compare::assert_ui;
+use cargo_test_support::curr_dir;
+use cargo_test_support::CargoCommand;
+use cargo_test_support::Project;
+
+use crate::cargo_remove::init_registry;
+
+#[cargo_test]
+fn case() {
+    init_registry();
+    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(["--build", "semver"])
+        .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/build/out/Cargo.toml

@@ -0,0 +1,21 @@
+[package]
+name = "cargo-remove-test-fixture"
+version = "0.1.0"
+
+[[bin]]
+name = "main"
+path = "src/main.rs"
+
+[dependencies]
+docopt = "0.6"
+rustc-serialize = "0.4"
+semver = "0.1"
+toml = "0.1"
+clippy = "0.4"
+
+[dev-dependencies]
+regex = "0.1.1"
+serde = "1.0.90"
+
+[features]
+std = ["serde/std", "semver/std"]

tests/testsuite/cargo_remove/build/stderr.log

@@ -0,0 +1,2 @@
+    Removing semver from build-dependencies
+    Updating `dummy-registry` index

tests/testsuite/cargo_remove/dev/in

@@ -0,0 +1 @@
+../remove-basic.in/