fix(resolve): detect unused patches when multiple patches share a PackageId#16987
fix(resolve): detect unused patches when multiple patches share a PackageId#16987raushan728 wants to merge 2 commits into
Conversation
rustbot
added
A-dependency-resolution
|
r? @weihanglo rustbot has assigned @weihanglo. Use Why was this reviewer chosen?The reviewer was selected based on:
|
…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
| if !used_patches.contains(&(url.clone(), summary.package_id())) { | ||
| if !self.unused_patches.contains(&summary.package_id()) { |
There was a problem hiding this comment.
I guess we could collapse nested if statements.
There was a problem hiding this comment.
I will combine these.
There was a problem hiding this comment.
Once we finalize the other points.
| .entry(summary.package_id()) | ||
| .or_insert_with(HashSet::new) | ||
| .insert(url); | ||
| let mut used_patches = HashSet::new(); |
There was a problem hiding this comment.
It seems we literally build the same set twice: once in register_used_patches and once here.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 (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), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
4 participants
Fixes #12471
Previously, Cargo determined patch usage solely by checking if the
PackageIdexisted in the crate graph, which caused false positives if multiple patch sources pointed to the same dependency.Updated
register_used_patchesandemit_warnings_of_unused_patchesto independently verify patch usage by inspecting graph edges for specific(CanonicalUrl, PackageId)pairs.Left
Resolve::unused_patchesasVec<PackageId>to ensure strictCargo.lockbackward compatibility without altering the serialized format.