`cargo rm` unexpectedly removes patches #12419

dtolnay · 2023-07-31T01:36:48Z

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]

The text was updated successfully, but these errors were encountered:

epage · 2023-07-31T14:08:00Z

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 · 2023-07-31T15:21:13Z

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 · 2023-07-31T15:25:03Z

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 · 2023-07-31T15:28:04Z

Mentioning @cassaundra since you worked on the relevant PR.

epage · 2023-07-31T17:47:28Z

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 · 2023-07-31T17:49:53Z

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 · 2023-08-04T08:55:37Z

@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 · 2023-08-04T14:22:57Z

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 · 2023-08-04T16:36:27Z

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

@rustbot claim

alcolmenar · 2023-08-06T18:15:15Z

@epage Looks like the source is that the gc_workspace function removes patches where no workspace members have an explicit dependency for the patched crate. This doesn't appear to be the way it should work since the patch can technically override a crate that is a nested dependency of an explicit dependency. Seems like we can't reliably determine if a patch is used until after the workspace has been resolved.

I'm thinking I could break out gc'ing patches to a separate function and run it after resolve_ws using its unused_patches result to determine what to remove. If something is removed, then run resolve_ws again. What do you think?

Additionally, I think there's a bug in unused_patches for path dependencies

cargo/src/cargo/core/resolver/resolve.rs

Line 142 in b644a40

pub fn register_used_patches(&mut self, patches: &[Summary]) {

This marks the package unused only if the graph doesn't contain it, but since the patch is a member it'll always be in the graph.

epage · 2023-08-07T15:54:46Z

I'm thinking I could break out gc'ing patches to a separate function and run it after resolve_ws using its unused_patches result to determine what to remove. If something is removed, then run resolve_ws again. What do you think?

Ah, I had overlooked that we already track used patches and can reuse that, nice! I was assuming we'd just walk resolve_wss graph.

This marks the package unused only if the graph doesn't contain it, but since the patch is a member it'll always be in the graph.

I'm not quite following what the problem is.

alcolmenar · 2023-08-07T16:04:30Z

I'm not quite following what the problem is.

Commenting out the gc patch code, Resolve's unused_patches field from resolve_ws isn't being populated with the patch in the failing example even though it's unused. It seems like there a bug in the code which adds the patch to the unused field

epage · 2023-08-07T17:24:09Z

If I'm understanding correctly, your proposed fix would mean we never GC patches for workspace members? I can live with that, at least for now. This is a minority case (workspace member that is never depended on in the workspace) and people might want to keep that around.

I'd simplify the test case to the minimum number of variable which is just a transitive dependency that is being patched.

alcolmenar · 2023-08-07T21:23:18Z

I'm proposing the following:

Fix the issue where unused patches are not being added to the resolve properly for path dependencies
Move patch gc logic to a separate function to be executed after resolve_ws
Use resolve_ws result's unused_patches in gc logic function to automatically remove unused patches
Re-execute resolve_ws if something was removed in previous step

This'll automatically remove unused patches but leave ones that have an actual nested dependency. In its current form, patches are being removed if it isn't overriding a workspace member's dependency.

I do wonder if we should be automatically removing unused patches without an explicit flag from the user. Maybe a flag could be added that'll enable this? --gc-unused-patches?

epage · 2023-08-07T21:36:42Z

Fix the issue where unused patches are not being added to the resolve properly for path dependencies

If I'm understanding correctly, this is unrelated to the current issue and should be handled separately. Feel free to create an issue and investigate it further (I'd want input from others more involved with the resolver to even decide whether this is a bug or not) but I don't think this should be part of the discussion fro this issue, in the same PR, or investigation of it blocking progress on this issue.

epage · 2023-08-07T21:37:03Z

I do wonder if we should be automatically removing unused patches without an explicit flag from the user. Maybe a flag could be added that'll enable this? --gc-unused-patches?

That is a separate discussion as well

alcolmenar · 2023-08-07T21:48:35Z

Ok, fair enough. I was thinking they should be separate discussions as well. My proposal then is:

Move patch gc logic to a separate function to be executed after resolve_ws
Walk resolve_ws's graph to determine if a patch is used and remove the patch if it isn't
Re-execute resolve_ws if something was removed in previous step

dtolnay added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels Jul 31, 2023

epage added the Command-remove label Jul 31, 2023

epage added S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review E-medium Experience: Medium and removed S-triage Status: This issue is waiting on initial triage. labels Jul 31, 2023

rustbot assigned alcolmenar Aug 4, 2023

alcolmenar mentioned this issue Aug 6, 2023

Fix cargo remove incorrectly removing used patches #12454

Merged

alcolmenar mentioned this issue Aug 7, 2023

Unused path dependency patches don't emit warning or show up in lock file on update #12464

Open

bors closed this as completed in af431e1 Aug 10, 2023

dtolnay mentioned this issue Aug 14, 2023

Adopt cargo rm after Cargo bugfix serde-rs/serde#2572

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`cargo rm` unexpectedly removes patches #12419

`cargo rm` unexpectedly removes patches #12419

dtolnay commented Jul 31, 2023 •

edited by rustbot

Loading

epage commented Jul 31, 2023

dtolnay commented Jul 31, 2023

dtolnay commented Jul 31, 2023

dtolnay commented Jul 31, 2023

epage commented Jul 31, 2023

epage commented Jul 31, 2023

alcolmenar commented Aug 4, 2023

epage commented Aug 4, 2023 •

edited

Loading

alcolmenar commented Aug 4, 2023

alcolmenar commented Aug 6, 2023

epage commented Aug 7, 2023

alcolmenar commented Aug 7, 2023

epage commented Aug 7, 2023

alcolmenar commented Aug 7, 2023 •

edited

Loading

epage commented Aug 7, 2023

epage commented Aug 7, 2023

alcolmenar commented Aug 7, 2023 •

edited

Loading

cargo rm unexpectedly removes patches #12419

cargo rm unexpectedly removes patches #12419

Comments

dtolnay commented Jul 31, 2023 • edited by rustbot Loading

Problem

Steps

Possible Solution(s)

Notes

Version

epage commented Jul 31, 2023

dtolnay commented Jul 31, 2023

dtolnay commented Jul 31, 2023

dtolnay commented Jul 31, 2023

epage commented Jul 31, 2023

epage commented Jul 31, 2023

alcolmenar commented Aug 4, 2023

epage commented Aug 4, 2023 • edited Loading

alcolmenar commented Aug 4, 2023

alcolmenar commented Aug 6, 2023

epage commented Aug 7, 2023

alcolmenar commented Aug 7, 2023

epage commented Aug 7, 2023

alcolmenar commented Aug 7, 2023 • edited Loading

epage commented Aug 7, 2023

epage commented Aug 7, 2023

alcolmenar commented Aug 7, 2023 • edited Loading

`cargo rm` unexpectedly removes patches #12419

`cargo rm` unexpectedly removes patches #12419

dtolnay commented Jul 31, 2023 •

edited by rustbot

Loading

epage commented Aug 4, 2023 •

edited

Loading

alcolmenar commented Aug 7, 2023 •

edited

Loading

alcolmenar commented Aug 7, 2023 •

edited

Loading