Auto merge of #14461 - epage:actionable, r=Muscraft

fix(resolve): With `latest` message, differentiate actionable updates ### What does this PR try to resolve? Instead of always listing the absolutely latest version as a warning color, we now differentiate - compatible updates are always actionable - incompatible, direct deps are always actionable These get reported and made yellow while non-actionable messages are unstyled. ### How should we test and review this PR? I just used a broad stroke to say "compatible" in the message means "semver compatible" and use `^` - We could focus on "compatible with dependent version reqs" which is what will be most actionable but seeing if we can get away without having to track all in-coming version reqs. - We could be more nuanced in language but the more verbose we are, the more we take away from higher priority messages ### Additional information This is not intended as *the* solution for #13908 though it experiments with what to do for that. This is prep work for improved MSRV reporting where we will differentiate this further by only considering MSRV-compatible updates as actionable (or rustc-compatible when not using MSRV-aware reslver).
rust-lang · Aug 29, 2024 · c1fa840 · c1fa840
2 parents 33714c4 + 353cd87
commit c1fa840
Show file tree

Hide file tree

Showing 7 changed files with 83 additions and 37 deletions.
diff --git a/src/cargo/ops/cargo_update.rs b/src/cargo/ops/cargo_update.rs
@@ -523,9 +523,8 @@ fn print_lockfile_generation(
                     vec![]
                 };
 
-                let package_id = change.package_id;
                 let required_rust_version = report_required_rust_version(resolve, change);
-                let latest = report_latest(&possibilities, package_id);
+                let latest = report_latest(&possibilities, change);
                 let note = required_rust_version.or(latest);
 
                 if let Some(note) = note {
@@ -587,9 +586,8 @@ fn print_lockfile_sync(
                     vec![]
                 };
 
-                let package_id = change.package_id;
                 let required_rust_version = report_required_rust_version(resolve, change);
-                let latest = report_latest(&possibilities, package_id);
+                let latest = report_latest(&possibilities, change);
                 let note = required_rust_version.or(latest).unwrap_or_default();
 
                 ws.gctx().shell().status_with_color(
@@ -641,9 +639,8 @@ fn print_lockfile_updates(
             PackageChangeKind::Added
             | PackageChangeKind::Upgraded
             | PackageChangeKind::Downgraded => {
-                let package_id = change.package_id;
                 let required_rust_version = report_required_rust_version(resolve, change);
-                let latest = report_latest(&possibilities, package_id);
+                let latest = report_latest(&possibilities, change);
                 let note = required_rust_version.or(latest).unwrap_or_default();
 
                 ws.gctx().shell().status_with_color(
@@ -660,9 +657,8 @@ fn print_lockfile_updates(
                 )?;
             }
             PackageChangeKind::Unchanged => {
-                let package_id = change.package_id;
                 let required_rust_version = report_required_rust_version(resolve, change);
-                let latest = report_latest(&possibilities, package_id);
+                let latest = report_latest(&possibilities, change);
                 let note = required_rust_version.as_deref().or(latest.as_deref());
 
                 if let Some(note) = note {
@@ -754,23 +750,42 @@ fn report_required_rust_version(resolve: &Resolve, change: &PackageChange) -> Op
     ))
 }
 
-fn report_latest(possibilities: &[IndexSummary], package: PackageId) -> Option<String> {
-    if !package.source_id().is_registry() {
+fn report_latest(possibilities: &[IndexSummary], change: &PackageChange) -> Option<String> {
+    let package_id = change.package_id;
+    if !package_id.source_id().is_registry() {
         return None;
     }
 
-    possibilities
+    let version_req = package_id.version().to_caret_req();
+    if let Some(version) = possibilities
         .iter()
         .map(|s| s.as_summary())
-        .filter(|s| is_latest(s.version(), package.version()))
+        .filter(|s| package_id.version() != s.version() && version_req.matches(s.version()))
         .map(|s| s.version().clone())
         .max()
-        .map(format_latest)
-}
+    {
+        let warn = style::WARN;
+        let report = format!(" {warn}(latest compatible: v{version}){warn:#}");
+        return Some(report);
+    }
+
+    if let Some(version) = possibilities
+        .iter()
+        .map(|s| s.as_summary())
+        .filter(|s| is_latest(s.version(), package_id.version()))
+        .map(|s| s.version().clone())
+        .max()
+    {
+        let warn = if change.is_transitive.unwrap_or(true) {
+            Default::default()
+        } else {
+            style::WARN
+        };
+        let report = format!(" {warn}(latest: v{version}){warn:#}");
+        return Some(report);
+    }
 
-fn format_latest(version: semver::Version) -> String {
-    let warn = style::WARN;
-    format!(" {warn}(latest: v{version}){warn:#}")
+    None
 }
 
 fn is_latest(candidate: &semver::Version, current: &semver::Version) -> bool {
@@ -803,13 +818,14 @@ struct PackageChange {
     previous_id: Option<PackageId>,
     kind: PackageChangeKind,
     is_member: Option<bool>,
+    is_transitive: Option<bool>,
     required_rust_version: Option<PartialVersion>,
 }
 
 impl PackageChange {
     pub fn new(ws: &Workspace<'_>, resolve: &Resolve) -> IndexMap<PackageId, Self> {
         let diff = PackageDiff::new(resolve);
-        Self::with_diff(diff, ws)
+        Self::with_diff(diff, ws, resolve)
     }
 
     pub fn diff(
@@ -818,12 +834,13 @@ impl PackageChange {
         resolve: &Resolve,
     ) -> IndexMap<PackageId, Self> {
         let diff = PackageDiff::diff(previous_resolve, resolve);
-        Self::with_diff(diff, ws)
+        Self::with_diff(diff, ws, resolve)
     }
 
     fn with_diff(
         diff: impl Iterator<Item = PackageDiff>,
         ws: &Workspace<'_>,
+        resolve: &Resolve,
     ) -> IndexMap<PackageId, Self> {
         let member_ids: HashSet<_> = ws.members().map(|p| p.package_id()).collect();
 
@@ -842,35 +859,41 @@ impl PackageChange {
                     PackageChangeKind::Upgraded
                 };
                 let is_member = Some(member_ids.contains(&package_id));
+                let is_transitive = Some(true);
                 let change = Self {
                     package_id,
                     previous_id: Some(previous_id),
                     kind,
                     is_member,
+                    is_transitive,
                     required_rust_version: None,
                 };
                 changes.insert(change.package_id, change);
             } else {
                 for package_id in diff.removed {
                     let kind = PackageChangeKind::Removed;
                     let is_member = None;
+                    let is_transitive = None;
                     let change = Self {
                         package_id,
                         previous_id: None,
                         kind,
                         is_member,
+                        is_transitive,
                         required_rust_version: None,
                     };
                     changes.insert(change.package_id, change);
                 }
                 for package_id in diff.added {
                     let kind = PackageChangeKind::Added;
                     let is_member = Some(member_ids.contains(&package_id));
+                    let is_transitive = Some(true);
                     let change = Self {
                         package_id,
                         previous_id: None,
                         kind,
                         is_member,
+                        is_transitive,
                         required_rust_version: None,
                     };
                     changes.insert(change.package_id, change);
@@ -879,17 +902,32 @@ impl PackageChange {
             for package_id in diff.unchanged {
                 let kind = PackageChangeKind::Unchanged;
                 let is_member = Some(member_ids.contains(&package_id));
+                let is_transitive = Some(true);
                 let change = Self {
                     package_id,
                     previous_id: None,
                     kind,
                     is_member,
+                    is_transitive,
                     required_rust_version: None,
                 };
                 changes.insert(change.package_id, change);
             }
         }
 
+        for member_id in &member_ids {
+            let Some(change) = changes.get_mut(member_id) else {
+                continue;
+            };
+            change.is_transitive = Some(false);
+            for (direct_dep_id, _) in resolve.deps(*member_id) {
+                let Some(change) = changes.get_mut(&direct_dep_id) else {
+                    continue;
+                };
+                change.is_transitive = Some(false);
+            }
+        }
+
         changes
     }
 

diff --git a/src/cargo/util/semver_ext.rs b/src/cargo/util/semver_ext.rs
@@ -5,18 +5,26 @@ use std::fmt::{self, Display};
 pub trait VersionExt {
     fn is_prerelease(&self) -> bool;
 
-    fn to_exact_req(&self) -> VersionReq;
+    fn to_req(&self, op: Op) -> VersionReq;
+
+    fn to_exact_req(&self) -> VersionReq {
+        self.to_req(Op::Exact)
+    }
+
+    fn to_caret_req(&self) -> VersionReq {
+        self.to_req(Op::Caret)
+    }
 }
 
 impl VersionExt for Version {
     fn is_prerelease(&self) -> bool {
         !self.pre.is_empty()
     }
 
-    fn to_exact_req(&self) -> VersionReq {
+    fn to_req(&self, op: Op) -> VersionReq {
         VersionReq {
             comparators: vec![Comparator {
-                op: Op::Exact,
+                op,
                 major: self.major,
                 minor: Some(self.minor),
                 patch: Some(self.patch),

diff --git a/tests/testsuite/direct_minimal_versions.rs b/tests/testsuite/direct_minimal_versions.rs
@@ -33,7 +33,7 @@ fn simple() {
         .with_stderr_data(str![[r#"
 [UPDATING] `dummy-registry` index
 [LOCKING] 1 package
-[ADDING] dep v1.0.0 (latest: v1.1.0)
+[ADDING] dep v1.0.0 (latest compatible: v1.1.0)
 
 "#]])
         .run();
@@ -122,7 +122,7 @@ fn yanked() {
         .with_stderr_data(str![[r#"
 [UPDATING] `dummy-registry` index
 [LOCKING] 1 package
-[ADDING] dep v1.1.0 (latest: v1.2.0)
+[ADDING] dep v1.1.0 (latest compatible: v1.2.0)
 
 "#]])
         .run();
@@ -176,7 +176,7 @@ fn indirect() {
         .with_stderr_data(str![[r#"
 [UPDATING] `dummy-registry` index
 [LOCKING] 2 packages
-[ADDING] direct v1.0.0 (latest: v1.1.0)
+[ADDING] direct v1.0.0 (latest compatible: v1.1.0)
 
 "#]])
         .run();

diff --git a/tests/testsuite/minimal_versions.rs b/tests/testsuite/minimal_versions.rs
@@ -35,7 +35,7 @@ fn minimal_version_cli() {
         .with_stderr_data(str![[r#"
 [UPDATING] `dummy-registry` index
 [LOCKING] 1 package to earliest compatible version
-[ADDING] dep v1.0.0 (latest: v1.1.0)
+[ADDING] dep v1.0.0 (latest compatible: v1.1.0)
 
 "#]])
         .run();

diff --git a/tests/testsuite/rust_version.rs b/tests/testsuite/rust_version.rs
@@ -242,7 +242,7 @@ foo v0.0.1 ([ROOT]/foo)
         .with_stderr_data(str![[r#"
 [UPDATING] `dummy-registry` index
 [LOCKING] 2 packages to latest Rust 1.60.0 compatible versions
-[ADDING] newer-and-older v1.5.0 (latest: v1.6.0)
+[ADDING] newer-and-older v1.5.0 (latest compatible: v1.6.0)
 [ADDING] only-newer v1.6.0 (requires Rust 1.65.0)
 
 "#]])
@@ -319,7 +319,7 @@ foo v0.0.1 ([ROOT]/foo)
         .with_stderr_data(str![[r#"
 [UPDATING] `dummy-registry` index
 [LOCKING] 2 packages to latest Rust 1.60.0 compatible versions
-[ADDING] newer-and-older v1.5.0 (latest: v1.6.0)
+[ADDING] newer-and-older v1.5.0 (latest compatible: v1.6.0)
 [ADDING] only-newer v1.6.0 (requires Rust 1.2345)
 
 "#]])
@@ -490,7 +490,7 @@ higher v0.0.1 ([ROOT]/foo)
         .with_stderr_data(str![[r#"
 [UPDATING] `dummy-registry` index
 [LOCKING] 2 packages to latest Rust 1.50.0 compatible versions
-[ADDING] newer-and-older v1.5.0 (latest: v1.6.0)
+[ADDING] newer-and-older v1.5.0 (latest compatible: v1.6.0)
 [ADDING] only-newer v1.6.0 (requires Rust 1.65.0)
 
 "#]])
@@ -619,7 +619,7 @@ fn resolve_edition2024() {
         .with_stderr_data(str![[r#"
 [UPDATING] `dummy-registry` index
 [LOCKING] 2 packages to latest Rust 1.60.0 compatible versions
-[ADDING] newer-and-older v1.5.0 (latest: v1.6.0)
+[ADDING] newer-and-older v1.5.0 (latest compatible: v1.6.0)
 [ADDING] only-newer v1.6.0 (requires Rust 1.65.0)
 
 "#]])
@@ -723,7 +723,7 @@ fn resolve_v3() {
         .with_stderr_data(str![[r#"
 [UPDATING] `dummy-registry` index
 [LOCKING] 2 packages to latest Rust 1.60.0 compatible versions
-[ADDING] newer-and-older v1.5.0 (latest: v1.6.0)
+[ADDING] newer-and-older v1.5.0 (latest compatible: v1.6.0)
 [ADDING] only-newer v1.6.0 (requires Rust 1.65.0)
 
 "#]])
@@ -871,7 +871,7 @@ fn update_msrv_resolve() {
         .with_stderr_data(str![[r#"
 [UPDATING] `dummy-registry` index
 [LOCKING] 1 package to latest Rust 1.60.0 compatible version
-[ADDING] bar v1.5.0 (latest: v1.6.0)
+[ADDING] bar v1.5.0 (latest compatible: v1.6.0)
 
 "#]])
         .run();
@@ -932,7 +932,7 @@ fn update_precise_overrides_msrv_resolver() {
         .with_stderr_data(str![[r#"
 [UPDATING] `dummy-registry` index
 [LOCKING] 1 package to latest Rust 1.60.0 compatible version
-[ADDING] bar v1.5.0 (latest: v1.6.0)
+[ADDING] bar v1.5.0 (latest compatible: v1.6.0)
 
 "#]])
         .run();
@@ -1019,7 +1019,7 @@ foo v0.0.1 ([ROOT]/foo)
         .with_stderr_data(str![[r#"
 [UPDATING] `dummy-registry` index
 [LOCKING] 2 packages to latest Rust 1.60.0 compatible versions
-[ADDING] newer-and-older v1.5.0 (latest: v1.6.0)
+[ADDING] newer-and-older v1.5.0 (latest compatible: v1.6.0)
 [ADDING] only-newer v1.6.0 (requires Rust 1.65.0)
 [DOWNLOADING] crates ...
 [DOWNLOADED] newer-and-older v1.5.0 (registry `dummy-registry`)

diff --git a/tests/testsuite/update.rs b/tests/testsuite/update.rs
@@ -1534,7 +1534,7 @@ fn report_behind() {
 [UPDATING] `dummy-registry` index
 [LOCKING] 1 package to latest compatible version
 [UPDATING] breaking v0.1.0 -> v0.1.1 (latest: v0.2.0)
-[UNCHANGED] pre v1.0.0-alpha.0 (latest: v1.0.0-alpha.1)
+[UNCHANGED] pre v1.0.0-alpha.0 (latest compatible: v1.0.0-alpha.1)
 [UNCHANGED] two-ver v0.1.0 (latest: v0.2.0)
 [NOTE] to see how you depend on a package, run `cargo tree --invert --package <dep>@<ver>`
 [WARNING] not updating lockfile due to dry run
@@ -1559,7 +1559,7 @@ fn report_behind() {
 [UPDATING] `dummy-registry` index
 [LOCKING] 0 packages to latest compatible versions
 [UNCHANGED] breaking v0.1.1 (latest: v0.2.0)
-[UNCHANGED] pre v1.0.0-alpha.0 (latest: v1.0.0-alpha.1)
+[UNCHANGED] pre v1.0.0-alpha.0 (latest compatible: v1.0.0-alpha.1)
 [UNCHANGED] two-ver v0.1.0 (latest: v0.2.0)
 [NOTE] to see how you depend on a package, run `cargo tree --invert --package <dep>@<ver>`
 [WARNING] not updating lockfile due to dry run

diff --git a/tests/testsuite/workspaces.rs b/tests/testsuite/workspaces.rs
@@ -697,7 +697,7 @@ fn share_dependencies() {
         .with_stderr_data(str![[r#"
 [UPDATING] `dummy-registry` index
 [LOCKING] 1 package to latest compatible version
-[ADDING] dep1 v0.1.3 (latest: v0.1.8)
+[ADDING] dep1 v0.1.3 (latest compatible: v0.1.8)
 [DOWNLOADING] crates ...
 [DOWNLOADED] dep1 v0.1.3 (registry `dummy-registry`)
 [CHECKING] dep1 v0.1.3