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

[alcolmenar]

Problem

While investigating #12419, a solution is to use the unused_patches field of the Resolve returned from resolve_ws. It turns out this field wasn't being populated for path dependency patches. As a result, an unused patch doesn't emit a warning or show up in the lock file as an unused patch during cargo update

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"

Same test case in #12419, but running cargo update instead.

Current:
No warning and lock file below:

# This file is automatically @generated by Cargo.
# It is not intended for manual editing.
version = 3

[[package]]
name = "serde"
version = "1.0.0"
dependencies = [
 "serde_derive",
]

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

Expectation:
The expectation is if a patch is unused, a warning would be emitted and would show up as a patch.unused section in the lock file

The following warning is emitted:

warning: Patch `serde v1.0.0 (/Users/alcolmenar/dev/3rd/test-patch/serde)` was not used in the crate graph.
Perhaps you misspelled the source URL being patched.
Possible URLs for `[patch.<URL>]`:
    /Users/alcolmenar/dev/3rd/test-patch/serde

Cargo file:

# This file is automatically @generated by Cargo.
# It is not intended for manual editing.
version = 3

[[package]]
name = "serde"
version = "1.0.0"
dependencies = [
 "serde_derive",
]

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

[[patch.unused]]
name = "serde"
version = "1.0.0"

Possible Solution(s)

Haven't really looked into how to do it but I think the errant code is here:

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

Lines 142 to 148 in 211fd7e

	pub fn register_used_patches(&mut self, patches: &[Summary]) {
	for summary in patches {
	if !self.graph.contains(&summary.package_id()) {
	self.unused_patches.push(summary.package_id())
	};
	}
	}

Notes

No response

Version

cargo 1.73.0-nightly (45782b6b8 2023-07-05)
release: 1.73.0-nightly
commit-hash: 45782b6b8afd1da042d45c2daeec9c0744f72cc7
commit-date: 2023-07-05
host: aarch64-apple-darwin
libgit2: 1.6.4 (sys:0.17.2 vendored)
libcurl: 7.79.1 (sys:0.4.63+curl-8.1.2 system ssl:(SecureTransport) LibreSSL/3.3.6)
ssl: OpenSSL 1.1.1u  30 May 2023
os: Mac OS 12.5.1 [64-bit]

[epage]

Is this issue that all unused patches aren't detected or only with patches of path dependencies? The title and description mostly sound like all patches except in a place or two so its unclear if this is a more general problem.

[alcolmenar]

It might just an issue with path dependency patches. I just tried git dependency patches:

diff --git a/Cargo.toml b/Cargo.toml
index 28b0284..60a77f9 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -5,4 +5,5 @@ members = ["serde", "serde_derive"]
 
 [patch.crates-io]
 serde = { path = "serde" }
+uuid = { git = 'https://github.com/uuid-rs/uuid.git' }

and cargo update gives:

    Updating git repository `https://github.com/uuid-rs/uuid.git`
warning: Patch `uuid v1.4.1 (https://github.com/uuid-rs/uuid.git#97b7f07b)` was not used in the crate graph.

I'll update the title

[epage]

I've also made a slight change to the description, It make it sound like this is a blocker for #12419.

[weihanglo]

if !self.graph.contains(&summary.package_id()) {

This could be the reason: PackageId of the patch dep and the dep to patch are the same. Not limited to path dependencies. In other words, the package of the “unused” patch is already in use in other place.

This should be recorded in Cargo.lock as it is really unused. However, I am not sure about the warning. What is the possible situation when this happens, and how could Cargo suggest fixing it? Haven't thought about it thoroughly but it seems that one needs to make several mistakes to go there.

[weihanglo]

Just note that if we decide to make this change, this could cause a churn in version control, as somebody will need to commit a new section of [[patch.unused]]. We could do it along with #12120, so that it won't cause too many churns.

Another approach is we can emit a warning without touching the lockfile at this time. Something like,

patch `foo 0.1.0 (/path/to/foo)` was not used because there is already a same package in the dependency graph.
This unused patch wasn't previously added to the lockfile, but will be in the future.

---

@rustbot label +S-needs-design -S-triage +A-patch +A-errors +A-lockfile

[alcolmenar]

This could be the reason: PackageId of the patch dep and the dep to patch are the same. Not limited to path dependencies. In other words, the package of the “unused” patch is already in use in other place.

In the toy example, the patch dependency isn't patching anything. I may be wrong, but I think the reason why it shows up in the graph is because the patch dependency is a member of the workspace.

This should be recorded in Cargo.lock as it is really unused. However, I am not sure about the warning.

In this comment's example, I created a toy example of a known unused patch with a git dependency and it generated a warning because the resolver marked it as unused.

What is the possible situation when this happens

Good question. The cases where this might happen are probably limited actually: typos or thinking a package is used as a dependency but it isn't so the patch isn't applied. For some context, we fixed an issue where used patches were being cleaned up during cargo remove and in the PR we used the unused_patches field, but found in same cases this field isn't being populated correctly.

how could Cargo suggest fixing it?

Probably the easiest is to emit a warning that suggests to remove the patch manually from the toml. Or we can add a flag to cargo update providing users to explicitly say to remove it automatically.

[weihanglo]

why it shows up in the graph is because the patch dependency is a member of the workspace.

That's true IIRC. Cargo creates only one dependency graph per cargo invocation for a workspace. That's also why I called out this line of code as a culprit, and consider it has the same root cause as #12471:

if !self.graph.contains(&summary.package_id()) {

For #12464 (comment) the example, they have different PackageId so the graph doesn't contain uuid and Cargo marks it as unused.

Feel like action items here would be:

• Have a proper warning for this.
• Adding this and Unused patch with same dependency as a used patch isn't marked unused #12471 unused patches to the lockfile.