`cargo rm` unexpectedly removes patches

[dtolnay]

Problem

When I run cargo rm dependency, I expect Cargo to remove the crate dependency from the current crate's [dependencies], and other related necessary changes, like removing dependency from the right-hand side of features that previously enabled an optional dependency on "dependency" or one of its features "dependency/…".

Cargo does these things, but it also wipes out my patch.crates-io section from the workspace's manifest. I could not figure out how this seems related in any way to removing a single dependency, so I think this is a bug.

Steps

# Cargo.toml

[workspace]
members = ["serde", "serde_derive"]

[patch.crates-io]
serde = { path = "serde" }

# serde/Cargo.toml

[package]
name = "serde"
version = "1.0.0"

[dependencies]
serde_derive = { path = "../serde_derive" }

# serde_derive/Cargo.toml

[package]
name = "serde_derive"
version = "1.0.0"

+serde/src/lib.rs and serde_derive/src/lib.rs which can be empty files.

In the serde directory, run:

$ cargo check  # sanity check--the manifests are valid

$ cargo rm serde_derive

Expected diff:

diff --git a/serde/Cargo.toml b/serde/Cargo.toml
index 759de5d..10027d7 100644
--- a/serde/Cargo.toml
+++ b/serde/Cargo.toml
@@ -3,6 +3,3 @@
 [package]
 name = "serde"
 version = "1.0.0"
-
-[dependencies]
-serde_derive = { path = "../serde_derive" }

Actual diff produced by Cargo:

diff --git a/Cargo.toml b/Cargo.toml
index 583dd2f..60ffc4f 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -2,6 +2,3 @@
 
 [workspace]
 members = ["serde", "serde_derive"]
-
-[patch.crates-io]
-serde = { path = "serde" }
diff --git a/serde/Cargo.toml b/serde/Cargo.toml
index 759de5d..10027d7 100644
--- a/serde/Cargo.toml
+++ b/serde/Cargo.toml
@@ -3,6 +3,3 @@
 [package]
 name = "serde"
 version = "1.0.0"
-
-[dependencies]
-serde_derive = { path = "../serde_derive" }

Possible Solution(s)

No response

Notes

No response

Version

cargo 1.73.0-nightly (7ac9416d8 2023-07-24)
release: 1.73.0-nightly
commit-hash: 7ac9416d82cd4fc5e707c9ec3574d22dff6466e5
commit-date: 2023-07-24
host: x86_64-unknown-linux-gnu
libgit2: 1.6.4 (sys:0.17.2 vendored)
libcurl: 8.1.2-DEV (sys:0.4.63+curl-8.1.2 vendored ssl:OpenSSL/1.1.1u)
ssl: OpenSSL 1.1.1u  30 May 2023
os: Ubuntu 22.04 (jammy) [64-bit]

[epage]

If I'm reading the reproduction steps correctly, the serde patch is unused. This is from #11194.

Note the GC process was expanded to clean up not just references to the dependencies just removed, but also references of all dependencies. This was because there's not an easy way to determine which dependencies correspond to the given TOML keys, without either 1) figuring that out before the removal (therefore having to predict the behavior), or 2) returning that information from the remove function (somewhat unorthodox for an op).

Note that this is referencing some of the structure of cargo. This was implemented as remove and gc_workspace functions which adds some burden for connecting the pieces (but not terribly).

This seemed ok because the chance of this seemed low and people could selecting add/revert the relevant parts.

[dtolnay]

If I'm reading the reproduction steps correctly, the serde patch is unused.

In serde's repo where I hit this, the patch is definitely used.

$ cargo tree
serde v1.0.179 (/git/serde/serde)
[dev-dependencies]
└── serde_derive v1.0.179 (proc-macro) (/git/serde/serde_derive)
    ├── proc-macro2 v1.0.66
    │   └── unicode-ident v1.0.11
    ├── quote v1.0.32
    │   └── proc-macro2 v1.0.66 (*)
    └── syn v2.0.28
        ├── proc-macro2 v1.0.66 (*)
        ├── quote v1.0.32 (*)
        └── unicode-ident v1.0.11
    [dev-dependencies]
    └── serde v1.0.179 (/git/serde/serde) (*)

serde_derive v1.0.179 (proc-macro) (/git/serde/serde_derive) (*)

serde_derive_internals v0.28.0 (/git/serde/serde_derive_internals)
├── proc-macro2 v1.0.66 (*)
├── quote v1.0.32 (*)
└── syn v2.0.28 (*)

serde_test_suite v0.0.0 (/git/serde/test_suite)
└── serde v1.0.179 (/git/serde/serde) (*)
[dev-dependencies]
├── automod v1.0.12 (proc-macro)
│   ├── proc-macro2 v1.0.66 (*)
│   ├── quote v1.0.32 (*)
│   └── syn v2.0.28 (*)
├── fnv v1.0.7
├── rustversion v1.0.14 (proc-macro)
├── serde v1.0.179 (/git/serde/serde) (*)
├── serde_derive v1.0.179 (proc-macro) (/git/serde/serde_derive) (*)
├── serde_test v1.0.176
│   └── serde v1.0.179 (/git/serde/serde) (*)
└── trybuild v1.0.82
    ├── basic-toml v0.1.4
    │   └── serde v1.0.179 (/git/serde/serde) (*)
    ├── dissimilar v1.0.7
    ├── glob v0.3.1
    ├── once_cell v1.18.0
    ├── serde v1.0.179 (/git/serde/serde) (*)
    ├── serde_derive v1.0.179 (proc-macro)
    ├── serde_json v1.0.104
    │   ├── itoa v1.0.9
    │   ├── ryu v1.0.15
    │   └── serde v1.0.179 (/git/serde/serde) (*)
    └── termcolor v1.2.0

Notice how all of serde_test, trybuild, basic-toml, and serde_json (all from crates.io) depend on the local serde instead of the crates.io serde. That is because of the patch.

[dtolnay]

Updated repro:

# serde/Cargo.toml

[package]
name = "serde"
version = "1.0.0"

[dependencies]
serde_derive = { path = "../serde_derive" }

[dev-dependencies]
serde_json = "=1.0.0"

cargo rm serde_derive still incorrectly removes the patch despite the patch being used.

Before:

$ cargo tree
serde v1.0.0 (/git/repro/workspace/serde)
└── serde_derive v1.0.0 (/git/repro/workspace/serde_derive)
[dev-dependencies]
└── serde_json v1.0.0
    ├── dtoa v0.4.8
    ├── itoa v0.3.4
    ├── num-traits v0.1.43
    │   └── num-traits v0.2.16
    │       [build-dependencies]
    │       └── autocfg v1.1.0
    └── serde v1.0.0 (/git/repro/workspace/serde) (*)

After:

$ cargo tree
serde v1.0.0 (/git/repro/workspace/serde)
[dev-dependencies]
└── serde_json v1.0.0
    ├── dtoa v0.4.8
    ├── itoa v0.3.4
    ├── num-traits v0.1.43
    │   └── num-traits v0.2.16
    │       [build-dependencies]
    │       └── autocfg v1.1.0
    └── serde v1.0.179

[dtolnay]

Mentioning @cassaundra since you worked on the relevant PR.

[epage]

Thanks for clarifying the reproduction case!

Looks like the bug is that we only check workspace member dependencies for a patch applying. That is correct for workspace inheritance but not for patches.

[epage]

I wonder if a simple way of resolving this is to generate the lockfile (resolve_ws) before we run gc_workspace and then we can look to see if the patched result is present in what was resolved.

[alcolmenar]

@epage hey, I'm a first time contributor here. I was wondering if I could claim this issue? Also, if it's ok, are there any glaring gotchas that you see and I should be on the look out for here?

[epage]

I'd start with a test that demonstrates the bug, making this its own commit. This way when the commit with the fix will show the change in behavior, making it easier for a reviewer to see that this has the intended affect.

The start of the code in question is here. For the APIs you'll be working with resolve_ws is a good start.

You are also welcome to come to office hours to talk it over.

[alcolmenar]

@epage awesome! thank you for pointing me in the right direction. I'll try working through it.

@rustbot claim