Auto merge of #11688 - epage:minimal, r=Eh2406

feat(resolver): `-Zdirect-minimal-versions`

This is an alternative to `-Zminimal-versions` as discussed in #5657.

Problems with `-Zminimal-versions` includes
- Requires the root most dependencies to verify it and we then percolate that up the stack.  This requires a massive level of cooperation to accomplish and so far there have been mixed results with it to the point that cargo's unstable
 documentation discourages its use.
- Users expect `cargo check -Zminimal-versions` to force resolving to minimal but it doesn't as the default maximal resolve is compatible and requires `cargo update -Zminimal-versions`
- Different compatible versions might be selected, breaking interop between crates, changing feature unification, and breaking `-sys` crates without bad `links`

`-Zdirect-minimal-versions` instead only applies this rule to your
direct dependencies, allowing anyone in the stack to immediately adopt
it, independent of everyone else.

Special notes
- Living up to the name and the existing design, this ignores yanked
  crates.  This makes sense for `^1.1` version requirements but might
  look weird for `^1.1.1` version requirements as it could select
  `1.1.2`.
- This will error if an indirect dependency requires a newer version.
  Your version requirement will need to capture what you use **and** all
  of you dependencies.  An alternative design would have tried to merge
  the result of minimum versions for direct dependencies and maximum
  versions for indirect dependencies.  This would have been complex and
  led to weird corner cases, making it harder to predict.  I also suspect
  the value gained would be relatively low as you can't verify that
  version requirement in any other way.
  - This also means discrepancies between `dependencies` and `dev-dependencies` are errors
  - The error could be improved to call out that this was from minimal
    versions but I felt getting this out now and starting to collect
    feedback was more important.

One advantage of this approach over `-Zminimal-versions` is that it removes most of the problems that [cargo-minimal-versions](https://github.com/taiki-e/cargo-minimal-versions) tried to workaround.

As for the implementation, this might not be the most elegant solution but it works and we can always iterate and improve on it in the future.
- We keep the state as a `bool` throughout but compensate for that by explicitly creating a variable to abstract away constants
- The name changes depending on the context, from `direct_minimal_version` when dealing with the unstable flag to `first_minimal_version` when the concept of "direct" is lost to `first_version` when we split off the ordering concept into a separate variable
- Packages that respect `direct_minimal_versions` are determined by whether they are the top-level `summaries` that get past into `resolve`

### What does this PR try to resolve?

The primary use case is verifying version requirements to avoid depending on something newer than might be available in a dependent

For this to help the MSRV use case, the crate author must directly depend on all indirect dependencies where their latest release has too new of an MSRV but at least they can do so with the `^` operator, rather than `<` and breaking the ecosystem.

### How should we test and review this PR?

The first two commits add tests using `-Zminimal-versions`.  The commit that adds `-Zdirect-minimal-versions` updates the tests, highlighting the differences in behavior.

### Additional information

Potential areas of conversation for stablization
- Flag name
- Handling of yanked (pick first non-yanked, pick yanked, error)
- Quality of error message
- Should the package have a "memory" of this flag being set by writing it to the lockfile?

Potential future work
- Stablize this
- Remove `-Zminimal-versions`
- Update `cargo publish`s `--verify` step to use this.
  - The challenge is this won't be using the packaged `Cargo.lock` which probably should also be verified.

Author: bors

crates/resolver-tests/tests/resolve.rs

@@ -100,6 +100,64 @@ proptest! {
         }
     }
 
+    /// NOTE: if you think this test has failed spuriously see the note at the top of this macro.
+    #[test]
+    fn prop_direct_minimum_version_error_implications(
+            PrettyPrintRegistry(input) in registry_strategy(50, 20, 60)
+    ) {
+        let mut config = Config::default().unwrap();
+        config.nightly_features_allowed = true;
+        config
+            .configure(
+                1,
+                false,
+                None,
+                false,
+                false,
+                false,
+                &None,
+                &["direct-minimal-versions".to_string()],
+                &[],
+            )
+            .unwrap();
+
+        let reg = registry(input.clone());
+        // there is only a small chance that any one
+        // crate will be interesting.
+        // So we try some of the most complicated.
+        for this in input.iter().rev().take(10) {
+            // direct-minimal-versions reduces the number of available solutions, so we verify that
+            // we do not come up with solutions maximal versions does not
+            let res = resolve(
+                vec![dep_req(&this.name(), &format!("={}", this.version()))],
+                &reg,
+            );
+
+            let mres = resolve_with_config(
+                vec![dep_req(&this.name(), &format!("={}", this.version()))],
+                &reg,
+                &config,
+            );
+
+            if res.is_err() {
+                prop_assert!(
+                    mres.is_err(),
+                    "direct-minimal-versions should not have more solutions than the regular, maximal resolver but found one when resolving `{} = \"={}\"`",
+                    this.name(),
+                    this.version()
+                )
+            }
+            if mres.is_ok() {
+                prop_assert!(
+                    res.is_ok(),
+                    "direct-minimal-versions should not have more solutions than the regular, maximal resolver but found one when resolving `{} = \"={}\"`",
+                    this.name(),
+                    this.version()
+                )
+            }
+        }
+    }
+
     /// NOTE: if you think this test has failed spuriously see the note at the top of this macro.
     #[test]
     fn prop_removing_a_dep_cant_break(

src/cargo/core/features.rs

@@ -717,6 +717,7 @@ unstable_cli_options!(
     dual_proc_macros: bool = ("Build proc-macros for both the host and the target"),
     features: Option<Vec<String>>  = (HIDDEN),
     minimal_versions: bool = ("Resolve minimal dependency versions instead of maximum"),
+    direct_minimal_versions: bool = ("Resolve minimal dependency versions instead of maximum (direct dependencies only)"),
     mtime_on_use: bool = ("Configure Cargo to update the mtime of used files"),
     no_index_update: bool = ("Do not update the registry index even if the cache is outdated"),
     panic_abort_tests: bool = ("Enable support to run tests with -Cpanic=abort"),
@@ -947,6 +948,7 @@ impl CliUnstable {
             "no-index-update" => self.no_index_update = parse_empty(k, v)?,
             "avoid-dev-deps" => self.avoid_dev_deps = parse_empty(k, v)?,
             "minimal-versions" => self.minimal_versions = parse_empty(k, v)?,
+            "direct-minimal-versions" => self.direct_minimal_versions = parse_empty(k, v)?,
             "advanced-env" => self.advanced_env = parse_empty(k, v)?,
             "config-include" => self.config_include = parse_empty(k, v)?,
             "check-cfg" => {

src/cargo/core/resolver/dep_cache.rs

@@ -36,9 +36,13 @@ pub struct RegistryQueryer<'a> {
     /// versions first. That allows `cargo update -Z minimal-versions` which will
     /// specify minimum dependency versions to be used.
     minimal_versions: bool,
-    /// a cache of `Candidate`s that fulfil a `Dependency`
-    registry_cache: HashMap<Dependency, Poll<Rc<Vec<Summary>>>>,
+    /// a cache of `Candidate`s that fulfil a `Dependency` (and whether `first_minimal_version`)
+    registry_cache: HashMap<(Dependency, bool), Poll<Rc<Vec<Summary>>>>,
     /// a cache of `Dependency`s that are required for a `Summary`
+    ///
+    /// HACK: `first_minimal_version` is not kept in the cache key is it is 1:1 with
+    /// `parent.is_none()` (the first element of the cache key) as it doesn't change through
+    /// execution.
     summary_cache: HashMap<
         (Option<PackageId>, Summary, ResolveOpts),
         (Rc<(HashSet<InternedString>, Rc<Vec<DepInfo>>)>, bool),
@@ -96,8 +100,13 @@ impl<'a> RegistryQueryer<'a> {
     /// any candidates are returned which match an override then the override is
     /// applied by performing a second query for what the override should
     /// return.
-    pub fn query(&mut self, dep: &Dependency) -> Poll<CargoResult<Rc<Vec<Summary>>>> {
-        if let Some(out) = self.registry_cache.get(dep).cloned() {
+    pub fn query(
+        &mut self,
+        dep: &Dependency,
+        first_minimal_version: bool,
+    ) -> Poll<CargoResult<Rc<Vec<Summary>>>> {
+        let registry_cache_key = (dep.clone(), first_minimal_version);
+        if let Some(out) = self.registry_cache.get(&registry_cache_key).cloned() {
             return out.map(Result::Ok);
         }
 
@@ -106,7 +115,8 @@ impl<'a> RegistryQueryer<'a> {
             ret.push(s);
         })?;
         if ready.is_pending() {
-            self.registry_cache.insert(dep.clone(), Poll::Pending);
+            self.registry_cache
+                .insert((dep.clone(), first_minimal_version), Poll::Pending);
             return Poll::Pending;
         }
         for summary in ret.iter() {
@@ -128,7 +138,8 @@ impl<'a> RegistryQueryer<'a> {
             let mut summaries = match self.registry.query_vec(dep, QueryKind::Exact)? {
                 Poll::Ready(s) => s.into_iter(),
                 Poll::Pending => {
-                    self.registry_cache.insert(dep.clone(), Poll::Pending);
+                    self.registry_cache
+                        .insert((dep.clone(), first_minimal_version), Poll::Pending);
                     return Poll::Pending;
                 }
             };
@@ -192,18 +203,18 @@ impl<'a> RegistryQueryer<'a> {
 
         // When we attempt versions for a package we'll want to do so in a sorted fashion to pick
         // the "best candidates" first. VersionPreferences implements this notion.
-        self.version_prefs.sort_summaries(
-            &mut ret,
-            if self.minimal_versions {
-                VersionOrdering::MinimumVersionsFirst
-            } else {
-                VersionOrdering::MaximumVersionsFirst
-            },
-        );
+        let ordering = if first_minimal_version || self.minimal_versions {
+            VersionOrdering::MinimumVersionsFirst
+        } else {
+            VersionOrdering::MaximumVersionsFirst
+        };
+        let first_version = first_minimal_version;
+        self.version_prefs
+            .sort_summaries(&mut ret, ordering, first_version);
 
         let out = Poll::Ready(Rc::new(ret));
 
-        self.registry_cache.insert(dep.clone(), out.clone());
+        self.registry_cache.insert(registry_cache_key, out.clone());
 
         out.map(Result::Ok)
     }
@@ -218,6 +229,7 @@ impl<'a> RegistryQueryer<'a> {
         parent: Option<PackageId>,
         candidate: &Summary,
         opts: &ResolveOpts,
+        first_minimal_version: bool,
     ) -> ActivateResult<Rc<(HashSet<InternedString>, Rc<Vec<DepInfo>>)>> {
         // if we have calculated a result before, then we can just return it,
         // as it is a "pure" query of its arguments.
@@ -237,22 +249,24 @@ impl<'a> RegistryQueryer<'a> {
         let mut all_ready = true;
         let mut deps = deps
             .into_iter()
-            .filter_map(|(dep, features)| match self.query(&dep) {
-                Poll::Ready(Ok(candidates)) => Some(Ok((dep, candidates, features))),
-                Poll::Pending => {
-                    all_ready = false;
-                    // we can ignore Pending deps, resolve will be repeatedly called
-                    // until there are none to ignore
-                    None
-                }
-                Poll::Ready(Err(e)) => Some(Err(e).with_context(|| {
-                    format!(
-                        "failed to get `{}` as a dependency of {}",
-                        dep.package_name(),
-                        describe_path_in_context(cx, &candidate.package_id()),
-                    )
-                })),
-            })
+            .filter_map(
+                |(dep, features)| match self.query(&dep, first_minimal_version) {
+                    Poll::Ready(Ok(candidates)) => Some(Ok((dep, candidates, features))),
+                    Poll::Pending => {
+                        all_ready = false;
+                        // we can ignore Pending deps, resolve will be repeatedly called
+                        // until there are none to ignore
+                        None
+                    }
+                    Poll::Ready(Err(e)) => Some(Err(e).with_context(|| {
+                        format!(
+                            "failed to get `{}` as a dependency of {}",
+                            dep.package_name(),
+                            describe_path_in_context(cx, &candidate.package_id()),
+                        )
+                    })),
+                },
+            )
             .collect::<CargoResult<Vec<DepInfo>>>()?;
 
         // Attempt to resolve dependencies with fewer candidates before trying

src/cargo/core/resolver/mod.rs

@@ -133,11 +133,21 @@ pub fn resolve(
         Some(config) => config.cli_unstable().minimal_versions,
         None => false,
     };
+    let direct_minimal_versions = match config {
+        Some(config) => config.cli_unstable().direct_minimal_versions,
+        None => false,
+    };
     let mut registry =
         RegistryQueryer::new(registry, replacements, version_prefs, minimal_versions);
     let cx = loop {
         let cx = Context::new(check_public_visible_dependencies);
-        let cx = activate_deps_loop(cx, &mut registry, summaries, config)?;
+        let cx = activate_deps_loop(
+            cx,
+            &mut registry,
+            summaries,
+            direct_minimal_versions,
+            config,
+        )?;
         if registry.reset_pending() {
             break cx;
         } else {
@@ -189,6 +199,7 @@ fn activate_deps_loop(
     mut cx: Context,
     registry: &mut RegistryQueryer<'_>,
     summaries: &[(Summary, ResolveOpts)],
+    direct_minimal_versions: bool,
     config: Option<&Config>,
 ) -> CargoResult<Context> {
     let mut backtrack_stack = Vec::new();
@@ -201,7 +212,14 @@ fn activate_deps_loop(
     // Activate all the initial summaries to kick off some work.
     for &(ref summary, ref opts) in summaries {
         debug!("initial activation: {}", summary.package_id());
-        let res = activate(&mut cx, registry, None, summary.clone(), opts);
+        let res = activate(
+            &mut cx,
+            registry,
+            None,
+            summary.clone(),
+            direct_minimal_versions,
+            opts,
+        );
         match res {
             Ok(Some((frame, _))) => remaining_deps.push(frame),
             Ok(None) => (),
@@ -399,7 +417,15 @@ fn activate_deps_loop(
                 dep.package_name(),
                 candidate.version()
             );
-            let res = activate(&mut cx, registry, Some((&parent, &dep)), candidate, &opts);
+            let direct_minimal_version = false; // this is an indirect dependency
+            let res = activate(
+                &mut cx,
+                registry,
+                Some((&parent, &dep)),
+                candidate,
+                direct_minimal_version,
+                &opts,
+            );
 
             let successfully_activated = match res {
                 // Success! We've now activated our `candidate` in our context
@@ -611,6 +637,7 @@ fn activate(
     registry: &mut RegistryQueryer<'_>,
     parent: Option<(&Summary, &Dependency)>,
     candidate: Summary,
+    first_minimal_version: bool,
     opts: &ResolveOpts,
 ) -> ActivateResult<Option<(DepsFrame, Duration)>> {
     let candidate_pid = candidate.package_id();
@@ -662,8 +689,13 @@ fn activate(
     };
 
     let now = Instant::now();
-    let (used_features, deps) =
-        &*registry.build_deps(cx, parent.map(|p| p.0.package_id()), &candidate, opts)?;
+    let (used_features, deps) = &*registry.build_deps(
+        cx,
+        parent.map(|p| p.0.package_id()),
+        &candidate,
+        opts,
+        first_minimal_version,
+    )?;
 
     // Record what list of features is active for this package.
     if !used_features.is_empty() {
@@ -856,11 +888,14 @@ fn generalize_conflicting(
         })
     {
         for critical_parents_dep in critical_parents_deps.iter() {
+            // We only want `first_minimal_version=true` for direct dependencies of workspace
+            // members which isn't the case here as this has a `parent`
+            let first_minimal_version = false;
             // A dep is equivalent to one of the things it can resolve to.
             // Thus, if all the things it can resolve to have already ben determined
             // to be conflicting, then we can just say that we conflict with the parent.
             if let Some(others) = registry
-                .query(critical_parents_dep)
+                .query(critical_parents_dep, first_minimal_version)
                 .expect("an already used dep now error!?")
                 .expect("an already used dep now pending!?")
                 .iter()

src/cargo/core/resolver/version_prefs.rs

@@ -42,7 +42,12 @@ impl VersionPreferences {
     /// Sort the given vector of summaries in-place, with all summaries presumed to be for
     /// the same package.  Preferred versions appear first in the result, sorted by
     /// `version_ordering`, followed by non-preferred versions sorted the same way.
-    pub fn sort_summaries(&self, summaries: &mut Vec<Summary>, version_ordering: VersionOrdering) {
+    pub fn sort_summaries(
+        &self,
+        summaries: &mut Vec<Summary>,
+        version_ordering: VersionOrdering,
+        first_version: bool,
+    ) {
         let should_prefer = |pkg_id: &PackageId| {
             self.try_to_use.contains(pkg_id)
                 || self
@@ -66,6 +71,9 @@ impl VersionPreferences {
                 _ => previous_cmp,
             }
         });
+        if first_version {
+            let _ = summaries.split_off(1);
+        }
     }
 }
 
@@ -115,13 +123,13 @@ mod test {
             summ("foo", "1.0.9"),
         ];
 
-        vp.sort_summaries(&mut summaries, VersionOrdering::MaximumVersionsFirst);
+        vp.sort_summaries(&mut summaries, VersionOrdering::MaximumVersionsFirst, false);
         assert_eq!(
             describe(&summaries),
             "foo/1.2.3, foo/1.2.4, foo/1.1.0, foo/1.0.9".to_string()
         );
 
-        vp.sort_summaries(&mut summaries, VersionOrdering::MinimumVersionsFirst);
+        vp.sort_summaries(&mut summaries, VersionOrdering::MinimumVersionsFirst, false);
         assert_eq!(
             describe(&summaries),
             "foo/1.2.3, foo/1.0.9, foo/1.1.0, foo/1.2.4".to_string()
@@ -140,13 +148,13 @@ mod test {
             summ("foo", "1.0.9"),
         ];
 
-        vp.sort_summaries(&mut summaries, VersionOrdering::MaximumVersionsFirst);
+        vp.sort_summaries(&mut summaries, VersionOrdering::MaximumVersionsFirst, false);
         assert_eq!(
             describe(&summaries),
             "foo/1.2.3, foo/1.2.4, foo/1.1.0, foo/1.0.9".to_string()
         );
 
-        vp.sort_summaries(&mut summaries, VersionOrdering::MinimumVersionsFirst);
+        vp.sort_summaries(&mut summaries, VersionOrdering::MinimumVersionsFirst, false);
         assert_eq!(
             describe(&summaries),
             "foo/1.2.3, foo/1.0.9, foo/1.1.0, foo/1.2.4".to_string()
@@ -166,13 +174,13 @@ mod test {
             summ("foo", "1.0.9"),
         ];
 
-        vp.sort_summaries(&mut summaries, VersionOrdering::MaximumVersionsFirst);
+        vp.sort_summaries(&mut summaries, VersionOrdering::MaximumVersionsFirst, false);
         assert_eq!(
             describe(&summaries),
             "foo/1.2.3, foo/1.1.0, foo/1.2.4, foo/1.0.9".to_string()
         );
 
-        vp.sort_summaries(&mut summaries, VersionOrdering::MinimumVersionsFirst);
+        vp.sort_summaries(&mut summaries, VersionOrdering::MinimumVersionsFirst, false);
         assert_eq!(
             describe(&summaries),
             "foo/1.1.0, foo/1.2.3, foo/1.0.9, foo/1.2.4".to_string()