fix(resolve): detect unused patches when multiple patches share a PackageId by raushan728 · Pull Request #16987 · rust-lang/cargo

raushan728 · 2026-05-11T06:51:10Z

Previously, Cargo determined patch usage solely by checking if the PackageId existed in the crate graph, which caused false positives if multiple patch sources pointed to the same dependency.

Updated register_used_patches and emit_warnings_of_unused_patches to independently verify patch usage by inspecting graph edges for specific (CanonicalUrl, PackageId) pairs.

Left Resolve::unused_patches as Vec<PackageId> to ensure strict Cargo.lock backward compatibility without altering the serialized format.

rustbot · 2026-05-11T06:51:15Z

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

Owners of files modified in this PR: @ehuss, @epage, @weihanglo
@ehuss, @epage, @weihanglo expanded to ehuss, epage, weihanglo
Random selection from ehuss, epage, weihanglo

0xPoe · 2026-05-19T20:55:29Z

-                .entry(summary.package_id())
-                .or_insert_with(HashSet::new)
-                .insert(url);
+    let mut used_patches = HashSet::new();


It seems we literally build the same set twice: once in register_used_patches and once here.

View changes since the review

One solution could be to maintain an unused_patches_with_url that also includes the URL. But we need to consider this issue first to evaluate the real impact of this change.

I intentionally built the set twice locally to strictly avoid altering the Resolve struct, because modifying or replacing unused_patches could affect how [patch.unused] is encoded in cargo.lock via encode.rs.

If we add unused_patches_with_url to Resolve, should we keep it as a purely in-memory field (and skip serializing it), or are you open to modifying the lockfile structure to store the URLs as well?

0xPoe · 2026-05-19T20:58:02Z

+    for (url, summaries) in registry.patches().iter() {
+        for summary in summaries.iter() {
+            let unused = summary.package_id();
+            if !used_patches.contains(&(url.clone(), unused)) {
+                // Show alternative source URLs if the source URLs being patched
+                // cannot be found in the crate graph.
+                match source_ids_grouped_by_pkg_name.get(&unused.name()) {
+                    Some(ids) if ids.iter().all(|id| id.canonical_url() != url) => {
+                        let mut ids: Vec<_> = ids.into_iter().collect();
+                        ids.sort_by_key(|id| id.url()); // SourceId implements Ord but url() is safe
+                        let mut help = "perhaps you meant one of the following:".to_owned();
+                        for id in ids {
+                            help.push_str("\n\t");
+                            help.push_str(&id.display_registry_name());
+                        }
+                        ws.gctx().shell().print_report(
+                            &[Level::WARNING
+                                .secondary_title(format!("patch `{unused}` {MESSAGE}"))
+                                .element(Level::HELP.message(help))],
+                            false,
+                        )?;
+                    }
+                    _ => unemitted_unused_patches.push(unused),


This change means X is in unused_patches if and only if there is at least one URL where X is declared as a patch but is not used there. Before the change, PackageId X was in unused_patches if and only if X was never used under any URL.

Will this cause any issues for consumers of unused_patches?

View changes since the review

From my exploration, the main consumers of unused_patches() are the warning emitter itself and encode.rs (which writes the [[patch.unused]] table in cargo.lock).

With this change, if a PackageId is used from one URL but unused from another, it will indeed be recorded in the lockfile's unused section. Since the lockfile only stores the PackageId, it loses the source context, which is technically inaccurate but doesn't break the parser.

To fully fix this ambiguity without confusion, we might actually need to make cargo.lock source aware for unused patches. How would you prefer to handle this?

To fully fix this ambiguity without confusion, we might actually need to make cargo.lock source aware for unused patches. How would you prefer to handle this?

I think once we decide to include it in the cargo.lock, this may require bumping the lockfile. Because if one user upgrades their toolchain, we will have lockfile changes, and they will need to submit that change. So this should be a lockfile version bump required change. And we need to use this mechanism to maintain compatibility.

This issue is far more complex than I thought. It is not technically challenging, but it requires a lot of design work. That is also why we marked it as needing design.

So I personally prefer to rework the design to make sure we are on the same page with everyone else about how to deliver this improvement. We should decide whether we need to update the lockfile first, because the implementation would be very different based on these decisions.

But I also saw @weihanglo's comment from #12464 (comment). I'm not sure that simply adding some warnings would be enough to move some parts forward first. I would love to hear your thoughts on this.

Regarding @weihanglo comment
#12464 (comment)

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.

Based on this, we have two ways to move :

Warning-only (Phased): We use the in-memory (CanonicalUrl, PackageId) logic only to emit the warning to the user. We intentionally exclude these specific overlaps from Resolve::unused_patches so they do not leak into Cargo.lock. This solves the UX issue now without requiring a lockfile version bump.

Full lockfile bump (Needs Design): We pause, design how [[patch.unused]] should encode the URL, bump the lockfile version, and implement the complete fix.

I can go with 1 if we want to deliver the warning first, or hold off for the full design. What do you think is the best path forward?

…kageId Previously, Cargo tracked used patches solely by PackageId. If multiple sources patched the same dependency, unused patches were incorrectly marked as used. This updates the logic to track usage by both source URL and PackageId in-memory, ensuring accurate warnings while keeping Cargo.lock backward-compatible. Fixes rust-lang#12471

rustbot · 2026-05-29T06:04:52Z

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

raushan728 · 2026-06-07T06:00:33Z

@epage Sorry for the direct ping! The review has been inactive for a few days, and we are a bit stuck on an architectural decision here. Could you please share your thoughts and guide me on how to move forward?

0xPoe · 2026-06-09T19:31:44Z

@epage Sorry for the direct ping! The review has been inactive for a few days, and we are a bit stuck on an architectural decision here. Could you please share your thoughts and guide me on how to move forward?

Sorry for the delayed response.

I will have another look. Please do not ping other maintainers if one maintainer is already involved in the PR. I had a business trip last week. I will find some time to check it this week or this weekend.

rustbot assigned weihanglo May 11, 2026

rustbot added A-dependency-resolution Area: dependency resolution and the resolver S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 11, 2026

0xPoe reviewed May 11, 2026

View reviewed changes

Comment thread src/cargo/core/resolver/resolve.rs Outdated

0xPoe reviewed May 11, 2026

View reviewed changes

Comment thread src/cargo/core/resolver/resolve.rs Outdated

raushan728 force-pushed the Fix-12471 branch from 7685161 to 58ebe89 Compare May 12, 2026 06:09

raushan728 requested a review from 0xPoe May 12, 2026 07:12

0xPoe reviewed May 19, 2026

View reviewed changes

raushan728 added 2 commits May 29, 2026 05:24

test(resolve): add baseline test for unused patch with same dependency

130238c

raushan728 force-pushed the Fix-12471 branch from 58ebe89 to 43e378c Compare May 29, 2026 06:04

raushan728 requested a review from 0xPoe May 29, 2026 06:06

Uh oh!

Conversation

raushan728 commented May 11, 2026

Uh oh!

rustbot commented May 11, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

0xPoe May 19, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xPoe May 19, 2026

Choose a reason for hiding this comment

Uh oh!

raushan728 May 20, 2026

Choose a reason for hiding this comment

Uh oh!

0xPoe May 19, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raushan728 May 20, 2026

Choose a reason for hiding this comment

Uh oh!

0xPoe Jun 21, 2026

Choose a reason for hiding this comment

Uh oh!

raushan728 Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

rustbot commented May 29, 2026

Uh oh!

raushan728 commented Jun 7, 2026

Uh oh!

0xPoe commented Jun 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

0xPoe May 19, 2026 •

edited by rustbot

Loading

0xPoe May 19, 2026 •

edited by rustbot

Loading