Import cargo remove into cargo

[cassaundra]

What does this PR try to resolve?

This PR merges cargo remove from cargo-edit into cargo.

Motivation

• General approval from community, see Add cargo-add (from cargo-edit) to cargo proper #5586 and Add cargo-rm (from cargo-edit) to cargo proper  #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). 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

$ 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

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. feat: Import cargo-add into cargo #10472
2. Document cargo-add #10578
3. Completion support for cargo-add #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 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
  • Supports dry run
• JavaScript: yarn remove
  • Supports wildcards
• JavaScript: pnpm remove
• Go: go get
  • go get foo@none to remove
• Julia: pkg rm
  • Supports --all to remove all dependencies
• Ruby: bundle remove
• Dart: dart pub remove
  • Supports dry run
• Lua: luarocks remove
  • Supports force remove
• .NET: Uninstall-Package
  • Supports dry run
  • Supports removal of dependencies
  • Supports force remove (disregards dependencies)
• Haxe: haxelib remove
• Racket: raco pkg remove
  • 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), 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, 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 (Document cargo remove #11227)
• Add shell completions (Add completions for cargo remove #11204)
• Perform GC on workspace dependencies when they are no longer used (see Tracking issue for Deduplicate Cargo workspace information RFC #8415) (Clean up workspace dependencies after cargo remove #11242)
  • 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
• Perform GC for patch, replace, and profile references to dependenices (Clean profile, patch, and replace in cargo remove #11194)
• built-in alias is shadowing external subcommand but Cargo warns "user-defined alias is shadowing" #11149 Remove built-in alias warning

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: [feature] cargo rm remove deps with a dash instead of an underscore killercup/cargo-edit#690
• Remove unused dependencies: RFC: cargo rm --unused killercup/cargo-edit#415
• Clean up caches: cargo remove should clean up removed dependencies from the cache killercup/cargo-edit#647

Additional information

Fixes #10520.

[rust-highfive]

r? @weihanglo

(rust-highfive has picked a reviewer for you, use r? to override)

[cassaundra]

Fixed tests on Windows.

[epage]

I've not dug through all of the tests but overall, this looks good. None of my comments are blocking.

EDIT: With all of the prep work done, its crazy to see how little code this is

[cassaundra]

Something else that might be useful as future work: if the user tries to remove a package that doesn't exist in the [dependencies] table, but does exist in another (e.g. [dev-dependencies]), display an informative hint to the user. We could also automatically remove the dependency for them, but I think this might be too aggressive.

[weihanglo]

With all of the prep work done, its crazy to see how little code this is

Totally agree! Thanks epage so much for previous efforts and cassaundra for posting this PR as well.

[cassaundra]

Fixed all nits that were mentioned, and fixed the lock file test. Tests are failing due to some network error it looks like (edit: working now 🤷‍♀️).

[weihanglo]

Looks great! No outstanding issue from me. Thank you.

@epage you can r+ it whenever you feel it's about time.

Edited: Oh. I feel like we still need an FCP, no?

[cassaundra]

Hmm, I thought I fixed it, but the extra space in [ "semver/std"] issue came back again. This might be an issue with toml_edit. Will fix in a moment.

[cassaundra]

Hmm, I thought I fixed it, but the extra space in [ "semver/std"] issue came back again.

Newly added 56918df calls fmt on the feature values array after editing it.

[epage]

@rfcbot fcp merge

While this doesn't require insta-stablization like cargo-add did because of the rename from cargo-rm to cargo-remove, this is also a lot less risky than cargo-add, having a much narrower scope and getting more runtime on the design being proposed for merge. I feel like there is little for us to learn through a separate stablization for this.

Like with cargo-add, there is work deferred out of this PR

• Add documentation.
• Add shell completions.
• Perform GC on workspace dependencies when they are no longer used (see Tracking issue for Deduplicate Cargo workspace information RFC #8415).

Work has already started on these. So long as merging this PR doesn't happen near the cut to beta, we should be safe deferring these out of this PR (again, like with cargo-add).

[rfcbot]

Team member @epage has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Eh2406
• @ehuss
• @epage
• @joshtriplett
• @weihanglo

Concerns:

• cargo-rm alias resolved by Import cargo remove into cargo #11099 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[joshtriplett]

@rfcbot reviewed

@rfcbot concern cargo-rm alias

I do think we should have a cargo-rm alias, to help with transition and keep people's finger-memory working. I think that's worth the additional transition effort.

[ehuss]

A few questions:

• Does this rebuild Cargo.lock like cargo add does now?
• Does this remove all references to a dependency? That is, things like profiles, features, patch, replace, etc.
• The description mentions workspace inheritance, but also says it was deferred since it is unstable. I believe it has been stable for some time, so I'm not clear on what that means.

[epage]

Does this rebuild Cargo.lock like cargo add does now?

Yes

Does this remove all references to a dependency? That is, things like profiles, features, patch, replace, etc.

It removes it from features.

I hadn't though of cleaning up profiles, patch, and release. Would you consider that a blocker for merge, a blocker for going to beta, or important but non-blocker that can be put in an issue?

The description mentions workspace inheritance, but also says it was deferred since it is unstable. I believe it has been stable for some time, so I'm not clear on what that means.

The development happened in cargo-edit testing with released cargo. workspace inheritance will hit stable in 1.64.0 (tomorrow).

I figured that rather than blocking the last work and merge on 1.64.0, we could move forward as-is and then implement the workspace inheritance part in cargo master where we don't have to worry about what is in stable or not.

[epage]

@joshtriplett

I do think we should have a cargo-rm alias, to help with transition and keep people's finger-memory working. I think that's worth the additional transition effort.

I'm fine with that and fine with that forcing insta-stablization (since I was proposing that anyways).

Would you consider this a blocker for merge or ok if we just move it to the Deferred list. I had already been wondering if it should be in the Deferred list but decided to wait to see what interest there was.

[cassaundra]

Extending the GC operation to cover profile, patch, and replace makes sense to me. As far as the rm alias, should that be done in this PR? It's a very small change that I can just go ahead and add (edit: now realizing that's what the above comment was already asking).

[bors]

☀️ Test successful - checks-actions
Approved by: epage
Pushing c71f344 to master...

[bors]

☀️ Test successful - checks-actions
Approved by: epage
Pushing c71f344 to master...

[jujpenabe]

I just want to point out that a dependency can now be removed with the rm alias. But I don't see the suggestion in cargo remove --help or cargo help though, so maybe this should be added.

[weihanglo]

@jujpenabe. Nice catch. I just created an issue #11348 for it.