Skip to content

fix(resolve): detect unused patches when multiple patches share a PackageId #16987

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
raushan728 wants to merge 2 commits into
base: master
Choose a base branch
from
+97 −38
Open

fix(resolve): detect unused patches when multiple patches share a PackageId #16987

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
130238c
test(resolve): add baseline test for unused patch with same dependency
raushan728 May 11, 2026
43e378c
fix(resolve): detect unused patches when multiple patches share a Pac…
raushan728 May 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions src/cargo/core/resolver/resolve.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use cargo_util_schemas::manifest::RustVersion;

use crate::core::dependency::DepKind;
use crate::core::{Dependency, PackageId, PackageIdSpec, PackageIdSpecQuery, Summary, Target};
use crate::util::CanonicalUrl;
use crate::util::Graph;
use crate::util::errors::CargoResult;
use crate::util::interning::InternedString;
Expand Down Expand Up @@ -201,11 +202,24 @@ impl Resolve {
self.graph.path_to_top(pkg)
}

pub fn register_used_patches<'a>(&mut self, patches: impl Iterator<Item = &'a Summary>) {
for summary in patches {
if !self.graph.contains(&summary.package_id()) {
self.unused_patches.push(summary.package_id())
};
pub fn register_used_patches(&mut self, patches: &HashMap<CanonicalUrl, Vec<Summary>>) {
let mut used_patches = HashSet::new();
for pkg in self.iter() {
for (dep_pkg_id, deps) in self.deps_not_replaced(pkg) {
for dep in deps {
used_patches.insert((dep.source_id().canonical_url().clone(), dep_pkg_id));
}
}
}

for (url, summaries) in patches {
for summary in summaries {
if !used_patches.contains(&(url.clone(), summary.package_id()))
&& !self.unused_patches.contains(&summary.package_id())
{
self.unused_patches.push(summary.package_id());
}
}
}
}

Expand Down
67 changes: 34 additions & 33 deletions src/cargo/ops/resolve.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,8 +511,7 @@ pub fn resolve_with_previous<'gctx>(
Some(ws.gctx()),
)?;

let patches = registry.patches().values().flat_map(|v| v.iter());
resolved.register_used_patches(patches);
resolved.register_used_patches(registry.patches());

if register_patches && !resolved.unused_patches().is_empty() {
emit_warnings_of_unused_patches(ws, &resolved, registry)?;
Expand Down Expand Up @@ -818,14 +817,12 @@ fn emit_warnings_of_unused_patches(
) -> CargoResult<()> {
const MESSAGE: &str = "was not used in the crate graph";

// Patch package with the source URLs being patch
let mut patch_pkgid_to_urls = HashMap::new();
for (url, summaries) in registry.patches().iter() {
for summary in summaries.iter() {
patch_pkgid_to_urls
.entry(summary.package_id())
.or_insert_with(HashSet::new)
.insert(url);
let mut used_patches = HashSet::new();

@0xPoe 0xPoe May 19, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

View changes since the review

@0xPoe 0xPoe May 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@raushan728 raushan728 May 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

for pkg in resolve.iter() {
for (dep_pkg_id, deps) in resolve.deps_not_replaced(pkg) {
for dep in deps {
used_patches.insert((dep.source_id().canonical_url().clone(), dep_pkg_id));
}
}
}

Expand All @@ -839,34 +836,38 @@ fn emit_warnings_of_unused_patches(
}

let mut unemitted_unused_patches = Vec::new();
for unused in resolve.unused_patches().iter() {
// 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()),
patch_pkgid_to_urls.get(unused),
) {
(Some(ids), Some(patched_urls))
if ids
.iter()
.all(|id| !patched_urls.contains(id.canonical_url())) =>
{
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());
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),
Comment on lines +839 to +861

@0xPoe 0xPoe May 19, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@raushan728 raushan728 May 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@0xPoe 0xPoe Jun 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@raushan728 raushan728 Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :

  1. 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.

  2. 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?

}
ws.gctx().shell().print_report(
&[Level::WARNING
.secondary_title(format!("patch `{unused}` {MESSAGE}"))
.element(Level::HELP.message(help))],
false,
)?;
}
_ => unemitted_unused_patches.push(unused),
}
}

// Deduplicate general warnings by PackageId
unemitted_unused_patches.sort();
unemitted_unused_patches.dedup();

// Show general help message.
if !unemitted_unused_patches.is_empty() {
let mut warnings: Vec<_> = unemitted_unused_patches
Expand Down
44 changes: 44 additions & 0 deletions tests/testsuite/patch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3430,3 +3430,47 @@ foo v0.5.0 ([ROOT]/foo/foo)
"#]])
.run();
}

#[cargo_test]
fn unused_patch_same_dependency_different_sources() {
Package::new("bar", "0.1.0").publish();

let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.1"
edition = "2015"

[dependencies]
bar = "0.1.0"

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

[patch.'https://github.com/example/bar']
bar = { path = "my-bar" }
"#,
)
.file("src/lib.rs", "")
.file("my-bar/Cargo.toml", &basic_manifest("bar", "0.1.0"))
.file("my-bar/src/lib.rs", "")
.build();

p.cargo("check")
.with_stderr_data(str![[r#"
[UPDATING] `dummy-registry` index
[WARNING] patch `bar v0.1.0 ([ROOT]/foo/my-bar)` was not used in the crate graph
|
= [HELP] perhaps you meant one of the following:
[ROOT]/foo/my-bar
[LOCKING] 1 package to latest compatible version
[CHECKING] bar v0.1.0 ([ROOT]/foo/my-bar)
[CHECKING] foo v0.0.1 ([ROOT]/foo)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s

"#]])
.run();
}
Loading