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