Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add cargo-rm (from cargo-edit) to cargo proper #10520

Closed
epage opened this issue Mar 30, 2022 · 5 comments · Fixed by #11099
Closed

Add cargo-rm (from cargo-edit) to cargo proper #10520

epage opened this issue Mar 30, 2022 · 5 comments · Fixed by #11099
Labels
A-new-subcommand Area: new subcommand C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted`

Comments

@epage
Copy link
Contributor

epage commented Mar 30, 2022

Problem

With #5586, users will have an "IDE-like experience" for adding dependencies but not removing them where they have to deal with updating feature tables, etc.

Proposed Solution

Merge cargo-rm

Notes

No response

@epage epage added A-new-subcommand Area: new subcommand C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` labels Mar 30, 2022
@epage
Copy link
Contributor Author

epage commented Mar 30, 2022

This will require

  • An evaluation of prior art
  • A re-evaluation of what the behavior should be
  • Updates on behavior
  • Port to cargo's code base in a merge-rm branch
  • Actual merge PR

killercup/cargo-edit#682 is where I'm starting to organize thoughts on this.

bors added a commit that referenced this issue Apr 18, 2022
feat: Import cargo-add into cargo

### Motivation

The reasons I'm aware of are:
- Large interest, see #5586
- Make it easier to add a dependency when you don't care about the version (instead of having to find it or just using the major version if thats all you remember)
- Provide a guided experience, including
  - Catch or prevent errors earlier in the process
  - Bring the Manifest format documentation into the terminal via `cargo add --help`
  - Using `version` and `path` for `dependencies` but `path` only for `dev-dependencies` (see crate-ci/cargo-release#288 which led to killercup/cargo-edit#480)

### Drawbacks

1. This is another area of consideration for new RFCs, like rust-lang/rfcs#3143 (this PR supports it) or rust-lang/rfcs#2906 (implementing it will require updating `cargo-add`)

2. This is a high UX feature that will draw a lot of attention (ie Issue influx)

e.g.
- killercup/cargo-edit#521
- killercup/cargo-edit#126
- killercup/cargo-edit#217

We've tried to reduce the UX influx by focusing the scope to preserving semantic information (custom sort order, comments, etc) but being opinionated on syntax (style of strings, etc)

### Behavior

Help output
<details>

```console
$ cargo run -- add --help
cargo-add                                                                                                                                  [4/4594]
Add dependencies to a Cargo.toml manifest file

USAGE:
    cargo add [OPTIONS] <DEP>[`@<VERSION>]` ...
    cargo add [OPTIONS] --path <PATH> ...
    cargo add [OPTIONS] --git <URL> ...

ARGS:
    <DEP_ID>...    Reference to a package to add as a dependency

OPTIONS:
        --no-default-features     Disable the default features
        --default-features        Re-enable the default features
    -F, --features <FEATURES>     Space-separated list of features to add
        --optional                Mark the dependency as optional
    -v, --verbose                 Use verbose output (-vv very verbose/build.rs output)
        --no-optional             Mark the dependency as required
        --color <WHEN>            Coloring: auto, always, never
        --rename <NAME>           Rename the dependency
        --frozen                  Require Cargo.lock and cache are up to date
        --manifest-path <PATH>    Path to Cargo.toml
        --locked                  Require Cargo.lock is up to date
    -p, --package <SPEC>          Package to modify
        --offline                 Run without accessing the network
        --config <KEY=VALUE>      Override a configuration value (unstable)
    -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

SOURCE:
        --path <PATH>        Filesystem path to local crate to add
        --git <URI>          Git repository location
        --branch <BRANCH>    Git branch to download the crate from
        --tag <TAG>          Git tag to download the crate from
        --rev <REV>          Git reference to download the crate from
        --registry <NAME>    Package registry for this dependency

SECTION:
        --dev                Add as development dependency
        --build              Add as build dependency
        --target <TARGET>    Add as dependency to the given target platform

EXAMPLES:
  $ cargo add regex --build
  $ cargo add trycmd --dev
  $ cargo add --path ./crate/parser/
  $ cargo add serde serde_json -F serde/derive
```

</details>

Example commands
```rust
cargo add regex
cargo add regex serde
cargo add regex@1
cargo add regex@~1.0
cargo add --path ../dependency
```
For an exhaustive set of examples, see [tests](https://github.com/killercup/cargo-edit/blob/merge-add/crates/cargo-add/tests/testsuite/cargo_add.rs) and associated snapshots

Particular points
- Effectively there are two modes
  - Fill in any relevant field for one package
  - Add multiple packages, erroring for fields that are package-specific (`--rename`)
  - Note that `--git` and `--path` only accept multiple packages from that one source
- We infer if the `dependencies` table is sorted and preserve that sorting when adding a new dependency
- Adding a workspace dependency
  - dev-dependencies always use path
  - all other dependencies use version + path
- Behavior is idempotent, allowing you to run `cargo add serde serde_json -F serde/derive` safely if you already had a dependency on `serde` but without `serde_json`
- When a registry dependency's version req is unspecified, we'll first reuse the version req from another dependency section in the manifest.  If that doesn't exist, we'll use the latest version in the registry as the version req

### Additional decisions

Accepting the proposed `cargo-add` as-is assumes the acceptance of the following:
- Add the `-F` short-hand for `--features` to all relevant cargo commands
- Support ``@`` in pkgids in other commands where we accept `:`
- Add support for `<name>`@<version>`` in more commands, like `cargo yank` and `cargo install`

### Alternatives

- Use `:` instead of ``@`` for versions
- Flags like `--features`, `--optional`, `--no-default-features` would be position-sensitive, ie they would only apply to the crate immediate preceding them
  - This removes the dual-mode nature of the command and remove the need for the `+feature` syntax (`cargo add serde -F derive serde_json`)
  - There was concern over the rarity of position-sensitive flags in CLIs for adopting it here
- Support a `--sort` flag to sort the dependencies (existed previously)
  - To keep the scope small, we didn't want general manifest editing capabilities
- `--upgrade <POLICY>` flag to choose constraint (existed previously)
  - The flag was confusing as-is and we feel we should instead encourage people towards `^`
- `--allow-prerelease` so a `cargo add clap` can choose among pre-releases as well
  - We felt the pre-release story is too weak in cargo-generally atm for making it first class in `cargo-add`
- Offer `cargo add serde +derive serde_json` as a shorthand
- Infer path from a positional argument

### Prior Art

- *(Python)* [poetry add](https://python-poetry.org/docs/cli/#add)
  - `git+` is needed for inferring git dependencies, no separate  `--git` flags
  - git branch is specified via a URL fragment, instead of a `--branch`
- *(Javascript)* [yarn add](https://yarnpkg.com/cli/add)
  - `name@data` where data can be version, git (with fragment for branch), etc
  - `-E` / `--exact`, `-T` / `--tilde`, `-C` / `--caret` to control version requirement operator instead of `--upgrade <policy>` (also controlled through `defaultSemverRangePrefix` in config)
  - `--cached` for using the lock file (killercup/cargo-edit#41)
  - In addition to `--dev`, it has `--prefer-dev` which will only add the dependency if it doesn't already exist in `dependencies` as well as `dev-dependencies`
  - `--mode update-lockfile` will ensure the lock file gets updated as well
- *(Javascript)* [pnpm-add](https://pnpm.io/cli/add)
- *(Javascript)* npm doesn't have a native solution
  - Specify version with ``@<version>``
  - Also overloads `<name>[`@<version>]`` with path and repo
    - Supports a git host-specific protocol for shorthand, like `github:user/repo`
    - Uses fragment for git ref, seems to have some kind of special semver syntax for tags?
  - Only supports `--save-exact` / `-E` for operators outside of the default
- *(Go)* [go get](https://go.dev/ref/mod#go-get)
  - Specify version with ``@<version>``
  - Remove dependency with ``@none``
- *(Haskell)* stack doesn't seem to have a native solution
- *(Julia)* [pkg Add](https://docs.julialang.org/en/v1/stdlib/Pkg/)
- *(Ruby)* [bundle add](https://bundler.io/v2.2/man/bundle-add.1.html)
  - Uses `--version` / `-v` instead of `--vers` (we use `--vers` because of `--version` / `-V`)
  - `--source` instead of `path` (`path` correlates to manifest field)
  - Uses `--git` / `--branch` like `cargo-add`
- *(Dart)* [pub add](https://dart.dev/tools/pub/cmd/pub-add)
  - Uses `--git-url` instead of `--git`
  - Uses `--git-ref` instead of `--branch`, `--tag`, `--rev`

### Future Possibilities

- Update lock file accordingly
- Exploring the idea of a [`--local` flag](killercup/cargo-edit#590)
- Take the MSRV into account when automatically creating version req (killercup/cargo-edit#587)
- Integrate rustsec to report advisories on new dependencies (killercup/cargo-edit#512)
- Integrate with licensing to report license, block add, etc (e.g. killercup/cargo-edit#386)
- Pull version from lock file (killercup/cargo-edit#41)
- Exploring if any vendoring integration would be beneficial (currently errors)
- Upstream `cargo-rm` (#10520), `cargo-upgrade` (#10498), and `cargo-set-version` (in that order of priority)
- Update crates.io with `cargo add` snippets in addition to or replacing the manifest snippets

For more, see https://github.com/killercup/cargo-edit/issues?q=is%3Aissue+is%3Aopen+label%3Acargo-add

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

This is intentionally broken up into several commits to help reviewing
1. Import of production code from cargo-edit's `merge-add` branch, with only changes made to let it compile (e.g. fixing up of `use` statements).
2. Import of test code / snapshots.  The only changes outside of the import were to add the `snapbox` dev-dependency and to `mod cargo_add` into the testsuite
3. This extends the work in #10425 so I could add back in the color highlighting I had to remove as part of switching `cargo-add` from direct termcolor calls to calling into `Shell`

Structure-wise, this is similar to other commands
- `bin` only defines a CLI and adapts it to an `AddOptions`
- `ops` contains a focused API with everything buried under it

The "op" contains a directory, instead of just a file, because of the amount of content.  Currently, all editing code is contained in there.  Most of this will be broken out and reused when other `cargo-edit` commands are added but holding off on that for now to separate out the editing API discussions from just getting the command in.

Within the github UI, I'd recommend looking at individual commits (and the `merge-add` branch if interested), skipping commit 2.  Commit 2 would be easier to browse locally.

`cargo-add` is mostly covered by end-to-end tests written using `snapbox`, including error cases.

There is additional cleanup that would ideally happen that was excluded intentionally from this PR to keep it better scoped, including
- Consolidating environment variables for end-to-end tests of `cargo`
- Pulling out the editing API, as previously mentioned
  - Where the editing API should live (`cargo::edit`?)
  - Any more specific naming of types to reduce clashes (e.g. `Dependency` or `Manifest` being fairly generic).
- Possibly sharing `SourceId` creation between `cargo install` and `cargo edit`
- Explore using `snapbox` in more of cargo's tests

Implementation justifications:
- `dunce` and `pathdiff` dependencies: needed for taking paths relative to the user and make them relative to the manifest being edited
- `indexmap` dependency (already a transitive dependency): Useful for preserving uniqueness while preserving order, like with feature values
- `snapbox` dev-dependency: Originally it was used to make it easy to update tests as the UX changed in prep for merging but it had the added benefit of making some UX bugs easier to notice so they got fixed.  Overall, I'd like to see it become the cargo-agnostic version of `cargo-test-support` so there is a larger impact when improvements are made
- `parse_feature` function: `CliFeatures` forces items through a `BTreeSet`, losing the users specified order which we wanted to preserve.

### Additional Information

See also [the internals thread](https://internals.rust-lang.org/t/feedback-on-cargo-add-before-its-merged/16024).

Fixes #5586
@epage

This comment was marked as resolved.

@stshine
Copy link

stshine commented May 5, 2022

Can we call this cargo remove instead of rm?

@tshepang
Copy link
Member

tshepang commented May 5, 2022

am with @stshine... rm tripped me up a number of times

@epage
Copy link
Contributor Author

epage commented May 5, 2022

Yes, most prior art uses remove (see the linked cargo-edit issue) though I expect we would provide an rm alias, whether visible or not.

cassaundra added a commit to cassaundra/cargo that referenced this issue Sep 6, 2022
Move cargo add utils out for future use by other subcommands (namely cargo
remove, per rust-lang#10520).
cassaundra added a commit to cassaundra/cargo that referenced this issue Sep 7, 2022
Move cargo add utils out for future use by other subcommands (namely cargo
remove, per rust-lang#10520).
cassaundra added a commit to cassaundra/cargo that referenced this issue Sep 13, 2022
Move cargo add utils out for future use by other subcommands (namely cargo
remove, per rust-lang#10520).
bors added a commit that referenced this issue Sep 14, 2022
Expose cargo add internals as edit API

Move the manifest editing utilities from cargo add to a new `cargo::util::edit` module as part of prep work for `cargo remove` (#10520). No other substantive changes have been made, as this PR is intended only to reduce the refactoring surface area of the implementation of the feature itself.

## Background

In cargo edit, there are a number of top-level modules which enable editing of Cargo manifest files (including `src/dependency.rs` and `src/manifest.rs`). In #10472, these files were added instead as a submodule of the cargo add command, with the stated intention of breaking them out later for subsequent `cargo-edit` subcommands. This PR follows through on that expectation.

## Decisions

Concerns raised in #10472 regarding this change:

- Where should the editing API should live?
  - Proposal: `cargo::ops::edit`
  - Justification: precedent has been set by `cargo::ops::resolve` and others to have utils shared by multiple ops live in `cargo::ops`. This is also serves to be a rather conservative API change.
  - Concerns: the name `edit` could be overly general for those unfamiliar with the cargo edit project (see alternatives)
  - Alternatives:
    - `cargo::edit`: this seems to me to be too top level, and would confuse users trying to discover the cargo API
    - `cargo::util::edit`: if we want to expose this at a higher level, perhaps renaming to act as a counterpart to `crate::util::toml`
    - For each of these, replace `edit` with `toml_edit`, `toml_mut`, `manifest_edit`, `manifest_mut`, `edit_toml`, `edit_manifest` etc. for a more specific module name
- Any more specific naming of types reduce clashes (e.g. `Dependency` or `Manifest` being fairly generic)
  - Currently the only thing distinguishing these similarly named types is their path, which the `edit` module makes more clear
  - Alternatives: rename to `EditDependency`/`EditManifest`, `TomlDependency`/`TomlManifest`, etc.
Hezuikn pushed a commit to Hezuikn/cargo that referenced this issue Sep 22, 2022
Move cargo add utils out for future use by other subcommands (namely cargo
remove, per rust-lang#10520).
Hezuikn pushed a commit to Hezuikn/cargo that referenced this issue Sep 22, 2022
Move cargo add utils out for future use by other subcommands (namely cargo
remove, per rust-lang#10520).
bors added a commit that referenced this issue Oct 6, 2022
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.
bors added a commit that referenced this issue Oct 6, 2022
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.
@bors bors closed this as completed in c71f344 Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-new-subcommand Area: new subcommand C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted`
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants