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();
Copy link
Copy Markdown
Member

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

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

Copy link
Copy Markdown
Member

@0xPoe 0xPoe May 19, 2026

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.

Copy link
Copy Markdown
Contributor Author

@raushan728 raushan728 May 20, 2026

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
Copy link
Copy Markdown
Member

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

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

Copy link
Copy Markdown
Contributor Author

@raushan728 raushan728 May 20, 2026

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?

}
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