Add capability of making breaking changes in update --precise

src/bin/cargo/commands/update.rs

@@ -1,8 +1,11 @@
+use std::collections::HashMap;
+
 use crate::command_prelude::*;
 
 use anyhow::anyhow;
 use cargo::ops::{self, UpdateOptions};
 use cargo::util::print_available_packages;
+use tracing::trace;
 
 pub fn cli() -> Command {
     subcommand("update")
@@ -92,28 +95,39 @@ pub fn exec(gctx: &mut GlobalContext, args: &ArgMatches) -> CliResult {
     let update_opts = UpdateOptions {
         recursive: args.flag("recursive"),
         precise: args.get_one::<String>("precise").map(String::as_str),
+        breaking: args.flag("breaking"),
         to_update,
         dry_run: args.dry_run(),
         workspace: args.flag("workspace"),
         gctx,
     };
 
-    if args.flag("breaking") {
-        gctx.cli_unstable()
-            .fail_if_stable_opt("--breaking", 12425)?;
-
-        let upgrades = ops::upgrade_manifests(&mut ws, &update_opts.to_update)?;
-        ops::resolve_ws(&ws, update_opts.dry_run)?;
-        ops::write_manifest_upgrades(&ws, &upgrades, update_opts.dry_run)?;
+    let breaking_update = update_opts.breaking
+        || (update_opts.precise.is_some() && gctx.cli_unstable().unstable_options);
 
-        if update_opts.dry_run {
-            update_opts
-                .gctx
-                .shell()
-                .warn("aborting update due to dry run")?;
+    // We are using the term "upgrade" here, which is the typical case, but it
+    // can also be a downgrade (in the case of a precise update). In general, it
+    // is a change to a version req matching a precise version.
+    let upgrades = if breaking_update {
+        if update_opts.breaking {
+            gctx.cli_unstable()
+                .fail_if_stable_opt("--breaking", 12425)?;
         }
+
+        trace!("allowing breaking updates");
+        ops::upgrade_manifests(&mut ws, &update_opts.to_update, &update_opts.precise)?
     } else {
-        ops::update_lockfile(&ws, &update_opts)?;
+        HashMap::new()
+    };
+
+    ops::update_lockfile(&ws, &update_opts, &upgrades)?;
+    ops::write_manifest_upgrades(&ws, &upgrades, update_opts.dry_run)?;
+
+    if update_opts.dry_run {
+        update_opts
+            .gctx
+            .shell()
+            .warn("aborting update due to dry run")?;
     }
 
     Ok(())

src/cargo/ops/cargo_update.rs

@@ -9,23 +9,38 @@ use crate::ops;
 use crate::sources::source::QueryKind;
 use crate::util::cache_lock::CacheLockMode;
 use crate::util::context::GlobalContext;
+use crate::util::interning::InternedString;
 use crate::util::toml_mut::dependency::{MaybeWorkspace, Source};
 use crate::util::toml_mut::manifest::LocalManifest;
 use crate::util::toml_mut::upgrade::upgrade_requirement;
 use crate::util::{style, OptVersionReq};
 use crate::util::{CargoResult, VersionExt};
+use anyhow::Context as _;
 use itertools::Itertools;
 use semver::{Op, Version, VersionReq};
 use std::cmp::Ordering;
 use std::collections::{BTreeMap, HashMap, HashSet};
 use tracing::{debug, trace};
 
-pub type UpgradeMap = HashMap<(String, SourceId), Version>;
+/// A map of all breaking upgrades which is filled in by
+/// upgrade_manifests/upgrade_dependency when going through workspace member
+/// manifests, and later used by write_manifest_upgrades in order to know which
+/// upgrades to write to manifest files on disk. Also used by update_lockfile to
+/// know which dependencies to keep unchanged if any have been upgraded (we will
+/// do either breaking or non-breaking updates, but not both).
+pub type UpgradeMap = HashMap<
+    // The key is a package identifier consisting of the name and the source id.
+    (InternedString, SourceId),
+    // The value is the original version requirement before upgrade, and the
+    // upgraded version.
+    (VersionReq, Version),
+>;
 
 pub struct UpdateOptions<'a> {
     pub gctx: &'a GlobalContext,
     pub to_update: Vec<String>,
     pub precise: Option<&'a str>,
+    pub breaking: bool,
     pub recursive: bool,
     pub dry_run: bool,
     pub workspace: bool,
@@ -49,7 +64,11 @@ pub fn generate_lockfile(ws: &Workspace<'_>) -> CargoResult<()> {
     Ok(())
 }
 
-pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoResult<()> {
+pub fn update_lockfile(
+    ws: &Workspace<'_>,
+    opts: &UpdateOptions<'_>,
+    upgrades: &UpgradeMap,
+) -> CargoResult<()> {
     if opts.recursive && opts.precise.is_some() {
         anyhow::bail!("cannot specify both recursive and precise simultaneously")
     }
@@ -90,8 +109,40 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes
     };
     let mut registry = ws.package_registry()?;
     let mut to_avoid = HashSet::new();
+    let breaking_update = !upgrades.is_empty();
 
-    if opts.to_update.is_empty() {
+    if breaking_update {
+        // We don't necessarily want to update all specified packages. If we are
+        // doing a breaking update or precise upgrade, we don't want to touch
+        // any packages that have no breaking updates. So we want to only avoid
+        // all packages that got upgraded.
+
+        for name in opts.to_update.iter() {
+            // We still want to query any specified package, for the sake of
+            // outputting errors if they don't exist.
+            previous_resolve.query(name)?;
+        }
+
+        for ((name, source_id), (version_req, _)) in upgrades.iter() {
+            if let Some(matching_dep) = previous_resolve.iter().find(|dep| {
+                dep.name() == *name
+                    && dep.source_id() == *source_id
+                    && version_req.matches(dep.version())
+            }) {
+                let spec = PackageIdSpec::new(name.to_string())
+                    .with_url(source_id.url().clone())
+                    .with_version(matching_dep.version().clone().into())
+                    .to_string();
+                let pid = previous_resolve.query(&spec)?;
+                to_avoid.insert(pid);
+            } else {
+                // Should never happen
+                anyhow::bail!(
+                    "no package named `{name}` with source `{source_id}` and version matching `{version_req}` in the previous lockfile",
+                )
+            }
+        }
+    } else if opts.to_update.is_empty() {
         if !opts.workspace {
             to_avoid.extend(previous_resolve.iter());
             to_avoid.extend(previous_resolve.unused_patches());
@@ -185,11 +236,7 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes
         opts.precise.is_some(),
         &mut registry,
     )?;
-    if opts.dry_run {
-        opts.gctx
-            .shell()
-            .warn("not updating lockfile due to dry run")?;
-    } else {
+    if !opts.dry_run {
         ops::write_pkg_lockfile(ws, &mut resolve)?;
     }
     Ok(())
@@ -217,6 +264,7 @@ pub fn print_lockfile_changes(
 pub fn upgrade_manifests(
     ws: &mut Workspace<'_>,
     to_update: &Vec<String>,
+    precise: &Option<&str>,
 ) -> CargoResult<UpgradeMap> {
     let gctx = ws.gctx();
     let mut upgrades = HashMap::new();
@@ -245,6 +293,7 @@ pub fn upgrade_manifests(
                 upgrade_dependency(
                     &gctx,
                     &to_update,
+                    precise,
                     &mut registry,
                     &mut upgrades,
                     &mut upgrade_messages,
@@ -259,6 +308,7 @@ pub fn upgrade_manifests(
 fn upgrade_dependency(
     gctx: &GlobalContext,
     to_update: &Vec<PackageIdSpec>,
+    precise: &Option<&str>,
     registry: &mut PackageRegistry<'_>,
     upgrades: &mut UpgradeMap,
     upgrade_messages: &mut HashSet<String>,
@@ -316,7 +366,7 @@ fn upgrade_dependency(
     let query =
         crate::core::dependency::Dependency::parse(name, None, dependency.source_id().clone())?;
 
-    let possibilities = {
+    let possibilities = if precise.is_none() {
         loop {
             match registry.query_vec(&query, QueryKind::Exact) {
                 std::task::Poll::Ready(res) => {
@@ -325,6 +375,8 @@ fn upgrade_dependency(
                 std::task::Poll::Pending => registry.block_until_ready()?,
             }
         }
+    } else {
+        Vec::new()
     };
 
     let latest = if !possibilities.is_empty() {
@@ -338,32 +390,60 @@ fn upgrade_dependency(
         None
     };
 
-    let Some(latest) = latest else {
+    let new_version = if let Some(precise) = precise {
+        Version::parse(precise)
+            .with_context(|| format!("invalid version format for precise version `{precise}`"))?
+    } else if let Some(latest) = latest {
+        latest.clone()
+    } else {
+        // Breaking (not precise) upgrade did not find a latest version
         trace!("skipping dependency `{name}` without any published versions");
         return Ok(dependency);
     };
 
-    if current.matches(&latest) {
+    if current.matches(&new_version) {
         trace!("skipping dependency `{name}` without a breaking update available");
         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(), &new_version)? else {
         trace!("skipping dependency `{name}` because the version requirement didn't change");
         return Ok(dependency);
     };
 
     let upgrade_message = format!("{name} {current} -> {new_req_string}");
     trace!(upgrade_message);
 
+    let old_version = semver::Version::new(
+        comparator.major,
+        comparator.minor.unwrap_or_default(),
+        comparator.patch.unwrap_or_default(),
+    );
+    let is_downgrade = new_version < old_version;
+    let status = if is_downgrade {
+        "Downgrading"
+    } else {
+        "Upgrading"
+    };
+
     if upgrade_messages.insert(upgrade_message.clone()) {
         gctx.shell()
-            .status_with_color("Upgrading", &upgrade_message, &style::GOOD)?;
+            .status_with_color(status, &upgrade_message, &style::WARN)?;
     }
 
-    upgrades.insert((name.to_string(), dependency.source_id()), latest.clone());
+    upgrades.insert(
+        (name, dependency.source_id()),
+        (current.clone(), new_version.clone()),
+    );
+
+    let new_version_req = VersionReq::parse(&new_version.to_string())?;
+
+    let req = if precise.is_some() {
+        OptVersionReq::Precise(new_version, new_version_req)
+    } else {
+        OptVersionReq::Req(new_version_req)
+    };
 
-    let req = OptVersionReq::Req(VersionReq::parse(&latest.to_string())?);
     let mut dep = dependency.clone();
     dep.set_version_req(req);
     Ok(dep)
@@ -433,7 +513,7 @@ pub fn write_manifest_upgrades(
                     continue;
                 };
 
-                let Some(latest) = upgrades.get(&(name.to_owned(), source_id)) else {
+                let Some((_, latest)) = upgrades.get(&(name.into(), source_id)) else {
                     trace!("skipping dependency without an upgrade: {name}");
                     continue;
                 };

src/cargo/util/semver_ext.rs

@@ -117,9 +117,15 @@ impl OptVersionReq {
     /// and we're not sure if this part of the functionality should be implemented in semver or cargo.
     pub fn matches_prerelease(&self, version: &Version) -> bool {
         if version.is_prerelease() {
-            let mut version = version.clone();
-            version.pre = semver::Prerelease::EMPTY;
-            return self.matches(&version);
+            // Only in the case of "ordinary" version requirements with pre-release
+            // versions do we need to help the version matching. In the case of Any,
+            // Locked, or Precise, the `matches()` function is already doing the
+            // correct handling.
+            if let OptVersionReq::Req(_) = self {
+                let mut version = version.clone();
+                version.pre = semver::Prerelease::EMPTY;
+                return self.matches(&version);
+            }
         }
         self.matches(version)
     }
@@ -178,6 +184,8 @@ impl From<VersionReq> for OptVersionReq {
 
 #[cfg(test)]
 mod matches_prerelease {
+    use semver::{Version, VersionReq};
+
     use super::OptVersionReq;
 
     #[test]
@@ -238,4 +246,13 @@ mod matches_prerelease {
             assert_eq!(expected, matched, "req: {req}; ver: {ver}");
         }
     }
+
+    #[test]
+    fn precise_prerelease() {
+        let version_req = VersionReq::parse("1.2.3-pre").unwrap();
+        let version = Version::parse("1.2.3-pre").unwrap();
+        let matched =
+            OptVersionReq::Precise(version.clone(), version_req).matches_prerelease(&version);
+        assert!(matched, "a version must match its own precise requirement");
+    }
 }

src/cargo/util/toml_mut/upgrade.rs

@@ -21,22 +21,18 @@ pub(crate) fn upgrade_requirement(
             .map(|p| set_comparator(p, version))
             .collect();
         let comparators = comparators?;
-        let new_req = semver::VersionReq { comparators };
+        let mut new_req = semver::VersionReq { comparators };
+        // Validate contract
+        if !new_req.matches(version) {
+            // If req is ^0.1 and version is 0.2.0-beta, new_req becomes ^0.2.
+            // This does not match version. In such cases, we should let new_req
+            // be ^version.
+            new_req = semver::VersionReq::parse(&format!("^{version}"))?;
+        }
         let mut new_req_text = new_req.to_string();
         if new_req_text.starts_with('^') && !req.starts_with('^') {
             new_req_text.remove(0);
         }
-        // Validate contract
-        #[cfg(debug_assertions)]
-        {
-            assert!(
-                new_req.matches(version),
-                "New req {} is invalid, because {} does not match {}",
-                new_req_text,
-                new_req,
-                version
-            )
-        }
         if new_req_text == req_text {
             Ok(None)
         } else {

tests/testsuite/main.rs

@@ -175,6 +175,7 @@ mod tree;
 mod tree_graph_features;
 mod unit_graph;
 mod update;
+mod update_duplicated_with_precise_breaking_feature;
 mod vendor;
 mod verify_project;
 mod version;

tests/testsuite/precise_pre_release.rs

@@ -66,11 +66,12 @@ fn update_pre_release() {
     p.cargo("update my-dependency --precise 0.1.2-pre.0 -Zunstable-options")
         .masquerade_as_nightly_cargo(&["precise-pre-release"])
         .with_stderr_data(str![[r#"
+[UPGRADING] my-dependency ^0.1.1 -> ^0.1.2-pre.0
 [UPDATING] `dummy-registry` index
-[UPDATING] my-dependency v0.1.1 -> v0.1.2-pre.0
 
 "#]])
         .run();
+
     let lockfile = p.read_lockfile();
     assert!(lockfile.contains("\nname = \"my-dependency\"\nversion = \"0.1.2-pre.0\""));
 }
@@ -99,8 +100,8 @@ fn update_pre_release_differ() {
     p.cargo("update -p my-dependency --precise 0.1.2-pre.0 -Zunstable-options")
         .masquerade_as_nightly_cargo(&["precise-pre-release"])
         .with_stderr_data(str![[r#"
+[DOWNGRADING] my-dependency ^0.1.2 -> ^0.1.2-pre.0
 [UPDATING] `dummy-registry` index
-[DOWNGRADING] my-dependency v0.1.2 -> v0.1.2-pre.0
 
 "#]])
         .run();