fix: update --breaking now handles mixed renaming and pinning.

Author: torhovland

src/cargo/ops/cargo_update.rs

@@ -348,7 +348,7 @@ fn upgrade_dependency(
         return Ok(dependency);
     }
 
-    let Some(new_req_string) = upgrade_requirement(&current.to_string(), latest)? else {
+    let Some((new_req_string, _)) = upgrade_requirement(&current.to_string(), latest)? else {
         trace!("skipping dependency `{name}` because the version requirement didn't change");
         return Ok(dependency);
     };
@@ -369,8 +369,17 @@ fn upgrade_dependency(
     Ok(dep)
 }
 
-/// Update manifests with upgraded versions, and write to disk. Based on cargo-edit.
-/// Returns true if any file has changed.
+/// Update manifests with upgraded versions, and write to disk. Based on
+/// cargo-edit. Returns true if any file has changed.
+///
+/// Some of the checks here are duplicating checks already done in
+/// upgrade_manifests/upgrade_dependency. Why? Let's say upgrade_dependency has
+/// found that dependency foo was eligible for an upgrade. But foo can occur in
+/// multiple manifest files, and even multiple times in the same manifest file,
+/// and may be pinned, renamed, etc. in some of the instances. So we still need
+/// to check here which dependencies to actually modify. So why not drop the
+/// upgrade map and redo all checks here? Because then we'd have to query the
+/// registries again to find the latest versions.
 pub fn write_manifest_upgrades(
     ws: &Workspace<'_>,
     upgrades: &UpgradeMap,
@@ -407,6 +416,11 @@ pub fn write_manifest_upgrades(
                 )?;
                 let name = &dependency.name;
 
+                if let Some(renamed_to) = dependency.rename {
+                    trace!("skipping dependency renamed from `{name}` to `{renamed_to}`");
+                    continue;
+                }
+
                 let Some(current) = dependency.version() else {
                     trace!("skipping dependency without a version: {name}");
                     continue;
@@ -424,13 +438,27 @@ pub fn write_manifest_upgrades(
                     continue;
                 };
 
-                let Some(new_req_string) = upgrade_requirement(current, latest)? else {
+                let Some((new_req_string, new_req)) = upgrade_requirement(current, latest)? else {
                     trace!(
                         "skipping dependency `{name}` because the version requirement didn't change"
                     );
                     continue;
                 };
 
+                let [comparator] = &new_req.comparators[..] else {
+                    trace!(
+                        "skipping dependency `{}` with multiple version comparators: {:?}",
+                        name,
+                        new_req.comparators
+                    );
+                    continue;
+                };
+
+                if comparator.op != Op::Caret {
+                    trace!("skipping non-caret dependency `{}`: {}", name, comparator);
+                    continue;
+                }
+
                 let mut dep = dependency.clone();
                 let mut source = source.clone();
                 source.version = new_req_string;

src/cargo/util/toml_mut/upgrade.rs

@@ -7,7 +7,7 @@ use crate::CargoResult;
 pub(crate) fn upgrade_requirement(
     req: &str,
     version: &semver::Version,
-) -> CargoResult<Option<String>> {
+) -> CargoResult<Option<(String, semver::VersionReq)>> {
     let req_text = req.to_string();
     let raw_req = semver::VersionReq::parse(&req_text)
         .expect("semver to generate valid version requirements");
@@ -40,7 +40,7 @@ pub(crate) fn upgrade_requirement(
         if new_req_text == req_text {
             Ok(None)
         } else {
-            Ok(Some(new_req_text))
+            Ok(Some((new_req_text, new_req)))
         }
     }
 }
@@ -103,7 +103,9 @@ mod test {
         #[track_caller]
         fn assert_req_bump<'a, O: Into<Option<&'a str>>>(version: &str, req: &str, expected: O) {
             let version = semver::Version::parse(version).unwrap();
-            let actual = upgrade_requirement(req, &version).unwrap();
+            let actual = upgrade_requirement(req, &version)
+                .unwrap()
+                .map(|(actual, _req)| actual);
             let expected = expected.into();
             assert_eq!(actual.as_deref(), expected);
         }

tests/testsuite/update.rs

@@ -2460,19 +2460,6 @@ fn update_breaking_mixed_compatibility() {
 }
 
 #[cargo_test]
-/// In order to handle cases like this, it's not enough to keep an upgrade map
-/// keyed on dependency name and source, we also need to know which manifest
-/// files to update and which to keep unchanged, because they may be pinned or
-/// renamed in some but not others. But we don't know which manifest file a
-/// pinned/renamed dependency comes from. It might come from the workspace root
-/// manifest. (We could potentially find that out in or around
-/// to_real_manifest.)
-///
-/// Instead, when writing manifest changes, we will have to check if a
-/// dependency is pinned, renamed, etc., and should be skipped. This is
-/// unfortunate, as it means that ops::write_manifest_upgrades is duplicating
-/// some of the logic in ops::upgrade_manifests, and that is a potential source
-/// of bugs.
 fn update_breaking_mixed_pinning_renaming() {
     Package::new("mixed-pinned", "1.0.0").publish();
     Package::new("mixed-ws-pinned", "1.0.0").publish();
@@ -2564,21 +2551,19 @@ fn update_breaking_mixed_pinning_renaming() {
     let root_manifest = p.read_file("Cargo.toml");
     assert_e2e().eq(
         &root_manifest,
-        // FIXME: The pinned dependency should not be upgraded.
         str![[r#"
 
                 [workspace]
                 members = ["pinned", "unpinned", "mixed"]
 
                 [workspace.dependencies]
-                mixed-ws-pinned = "=2.0"
+                mixed-ws-pinned = "=1.0"
             "#]],
     );
 
     let pinned_manifest = p.read_file("pinned/Cargo.toml");
     assert_e2e().eq(
         &pinned_manifest,
-        // FIXME: The pinned and renamed dependencies should not be upgraded.
         str![[r#"
 
                 [package]
@@ -2588,9 +2573,9 @@ fn update_breaking_mixed_pinning_renaming() {
                 authors = []
 
                 [dependencies]
-                mixed-pinned = "=2.0"
+                mixed-pinned = "=1.0"
                 mixed-ws-pinned.workspace = true
-                renamed-to = { package = "renamed-from", version = "2.0" }
+                renamed-to = { package = "renamed-from", version = "1.0" }
             "#]],
     );
 
@@ -2615,7 +2600,6 @@ fn update_breaking_mixed_pinning_renaming() {
     let mixed_manifest = p.read_file("mixed/Cargo.toml");
     assert_e2e().eq(
         &mixed_manifest,
-        // FIXME: The pinned dependency should not be upgraded.
         str![[r#"
 
                 [package]
@@ -2628,7 +2612,7 @@ fn update_breaking_mixed_pinning_renaming() {
                 mixed-pinned = "2.0"
 
                 [target.'cfg(unix)'.dependencies]
-                mixed-pinned = "=2.0"
+                mixed-pinned = "=1.0"
             "#]],
     );
 }