Auto merge of #7186 - ehuss:resolve-method, r=alexcrichton

Refactor resolve `Method`

This changes the enum `Method` to a struct called `ResolveOpts`. The intent is to try to make it a little easier to read and understand.

If this doesn't actually look better to anyone, I'm fine with discarding this (I can resubmit just with documentation). It only seems marginally better, but I have had difficulty with understanding the subtleties of `Method` for a while. I wasn't able to measure any performance difference.

This also has a few other changes:
- Adds some documentation.
- Removes `resolve_ws_precisely`, cutting down the number of resolve functions from 4 to 3.

Author: bors

crates/resolver-tests/src/lib.rs

@@ -8,7 +8,7 @@ use std::rc::Rc;
 use std::time::Instant;
 
 use cargo::core::dependency::Kind;
-use cargo::core::resolver::{self, Method};
+use cargo::core::resolver::{self, ResolveOpts};
 use cargo::core::source::{GitReference, SourceId};
 use cargo::core::Resolve;
 use cargo::core::{Dependency, PackageId, Registry, Summary};
@@ -175,10 +175,10 @@ pub fn resolve_with_config_raw(
         false,
     )
     .unwrap();
-    let method = Method::Everything;
+    let opts = ResolveOpts::everything();
     let start = Instant::now();
     let resolve = resolver::resolve(
-        &[(summary, method)],
+        &[(summary, opts)],
         &[],
         &mut registry,
         &HashSet::new(),

src/cargo/core/compiler/layout.rs

@@ -3,7 +3,7 @@
 //! The directory layout is a little tricky at times, hence a separate file to
 //! house this logic. The current layout looks like this:
 //!
-//! ```ignore
+//! ```text
 //! # This is the root directory for all output, the top-level package
 //! # places all of its output here.
 //! target/

src/cargo/core/resolver/context.rs

@@ -13,7 +13,7 @@ use crate::util::CargoResult;
 use crate::util::Graph;
 
 use super::dep_cache::RegistryQueryer;
-use super::types::{ConflictMap, FeaturesSet, Method};
+use super::types::{ConflictMap, FeaturesSet, ResolveOpts};
 
 pub use super::encode::Metadata;
 pub use super::encode::{EncodableDependency, EncodablePackageId, EncodableResolve};
@@ -103,11 +103,11 @@ impl Context {
     /// cased `summary` to get activated. This may not be present for the root
     /// crate, for example.
     ///
-    /// Returns `true` if this summary with the given method is already activated.
+    /// Returns `true` if this summary with the given features is already activated.
     pub fn flag_activated(
         &mut self,
         summary: &Summary,
-        method: &Method,
+        opts: &ResolveOpts,
         parent: Option<(&Summary, &Dependency)>,
     ) -> CargoResult<bool> {
         let id = summary.package_id();
@@ -158,25 +158,21 @@ impl Context {
             }
         }
         debug!("checking if {} is already activated", summary.package_id());
-        let (features, use_default) = match method {
-            Method::Everything
-            | Method::Required {
-                all_features: true, ..
-            } => return Ok(false),
-            Method::Required {
-                features,
-                uses_default_features,
-                ..
-            } => (features, uses_default_features),
-        };
+        if opts.all_features {
+            return Ok(false);
+        }
 
         let has_default_feature = summary.features().contains_key("default");
         Ok(match self.resolve_features.get(&id) {
             Some(prev) => {
-                features.is_subset(prev)
-                    && (!use_default || prev.contains("default") || !has_default_feature)
+                opts.features.is_subset(prev)
+                    && (!opts.uses_default_features
+                        || prev.contains("default")
+                        || !has_default_feature)
+            }
+            None => {
+                opts.features.is_empty() && (!opts.uses_default_features || !has_default_feature)
             }
-            None => features.is_empty() && (!use_default || !has_default_feature),
         })
     }
 

src/cargo/core/resolver/dep_cache.rs

@@ -20,7 +20,7 @@ use crate::core::{Dependency, FeatureValue, PackageId, PackageIdSpec, Registry,
 use crate::util::errors::CargoResult;
 
 use crate::core::resolver::types::{ConflictReason, DepInfo, FeaturesSet};
-use crate::core::resolver::{ActivateResult, Method};
+use crate::core::resolver::{ActivateResult, ResolveOpts};
 
 pub struct RegistryQueryer<'a> {
     pub registry: &'a mut (dyn Registry + 'a),
@@ -34,7 +34,7 @@ pub struct RegistryQueryer<'a> {
     registry_cache: HashMap<Dependency, Rc<Vec<Summary>>>,
     /// a cache of `Dependency`s that are required for a `Summary`
     summary_cache: HashMap<
-        (Option<PackageId>, Summary, Method),
+        (Option<PackageId>, Summary, ResolveOpts),
         Rc<(HashSet<InternedString>, Rc<Vec<DepInfo>>)>,
     >,
     /// all the cases we ended up using a supplied replacement
@@ -192,28 +192,28 @@ impl<'a> RegistryQueryer<'a> {
     }
 
     /// Find out what dependencies will be added by activating `candidate`,
-    /// with features described in `method`. Then look up in the `registry`
+    /// with features described in `opts`. Then look up in the `registry`
     /// the candidates that will fulfil each of these dependencies, as it is the
     /// next obvious question.
     pub fn build_deps(
         &mut self,
         parent: Option<PackageId>,
         candidate: &Summary,
-        method: &Method,
+        opts: &ResolveOpts,
     ) -> 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.
         if let Some(out) = self
             .summary_cache
-            .get(&(parent, candidate.clone(), method.clone()))
+            .get(&(parent, candidate.clone(), opts.clone()))
             .cloned()
         {
             return Ok(out);
         }
         // First, figure out our set of dependencies based on the requested set
         // of features. This also calculates what features we're going to enable
         // for our own dependencies.
-        let (used_features, deps) = resolve_features(parent, candidate, method)?;
+        let (used_features, deps) = resolve_features(parent, candidate, opts)?;
 
         // Next, transform all dependencies into a list of possible candidates
         // which can satisfy that dependency.
@@ -236,7 +236,7 @@ impl<'a> RegistryQueryer<'a> {
         // If we succeed we add the result to the cache so we can use it again next time.
         // We dont cache the failure cases as they dont impl Clone.
         self.summary_cache
-            .insert((parent, candidate.clone(), method.clone()), out.clone());
+            .insert((parent, candidate.clone(), opts.clone()), out.clone());
 
         Ok(out)
     }
@@ -247,18 +247,13 @@ impl<'a> RegistryQueryer<'a> {
 pub fn resolve_features<'b>(
     parent: Option<PackageId>,
     s: &'b Summary,
-    method: &'b Method,
+    opts: &'b ResolveOpts,
 ) -> ActivateResult<(HashSet<InternedString>, Vec<(Dependency, FeaturesSet)>)> {
-    let dev_deps = match *method {
-        Method::Everything => true,
-        Method::Required { dev_deps, .. } => dev_deps,
-    };
-
     // First, filter by dev-dependencies.
     let deps = s.dependencies();
-    let deps = deps.iter().filter(|d| d.is_transitive() || dev_deps);
+    let deps = deps.iter().filter(|d| d.is_transitive() || opts.dev_deps);
 
-    let reqs = build_requirements(s, method)?;
+    let reqs = build_requirements(s, opts)?;
     let mut ret = Vec::new();
     let mut used_features = HashSet::new();
     let default_dep = (false, BTreeSet::new());
@@ -336,52 +331,34 @@ pub fn resolve_features<'b>(
     Ok((reqs.into_used(), ret))
 }
 
-/// Takes requested features for a single package from the input `Method` and
+/// Takes requested features for a single package from the input `ResolveOpts` and
 /// recurses to find all requested features, dependencies and requested
 /// dependency features in a `Requirements` object, returning it to the resolver.
 fn build_requirements<'a, 'b: 'a>(
     s: &'a Summary,
-    method: &'b Method,
+    opts: &'b ResolveOpts,
 ) -> CargoResult<Requirements<'a>> {
     let mut reqs = Requirements::new(s);
 
-    match method {
-        Method::Everything
-        | Method::Required {
-            all_features: true, ..
-        } => {
-            for key in s.features().keys() {
-                reqs.require_feature(*key)?;
-            }
-            for dep in s.dependencies().iter().filter(|d| d.is_optional()) {
-                reqs.require_dependency(dep.name_in_toml());
-            }
+    if opts.all_features {
+        for key in s.features().keys() {
+            reqs.require_feature(*key)?;
         }
-        Method::Required {
-            all_features: false,
-            features: requested,
-            ..
-        } => {
-            for &f in requested.iter() {
-                reqs.require_value(&FeatureValue::new(f, s))?;
-            }
+        for dep in s.dependencies().iter().filter(|d| d.is_optional()) {
+            reqs.require_dependency(dep.name_in_toml());
+        }
+    } else {
+        for &f in opts.features.iter() {
+            reqs.require_value(&FeatureValue::new(f, s))?;
         }
     }
-    match *method {
-        Method::Everything
-        | Method::Required {
-            uses_default_features: true,
-            ..
-        } => {
-            if s.features().contains_key("default") {
-                reqs.require_feature(InternedString::new("default"))?;
-            }
+
+    if opts.uses_default_features {
+        if s.features().contains_key("default") {
+            reqs.require_feature(InternedString::new("default"))?;
         }
-        Method::Required {
-            uses_default_features: false,
-            ..
-        } => {}
     }
+
     Ok(reqs)
 }
 

src/cargo/core/resolver/encode.rs

@@ -105,6 +105,7 @@ use crate::util::{internal, Graph};
 
 use super::{Resolve, ResolveVersion};
 
+/// The `Cargo.lock` structure.
 #[derive(Serialize, Deserialize, Debug)]
 pub struct EncodableResolve {
     package: Option<Vec<EncodableDependency>>,
@@ -123,6 +124,14 @@ struct Patch {
 pub type Metadata = BTreeMap<String, String>;
 
 impl EncodableResolve {
+    /// Convert a `Cargo.lock` to a Resolve.
+    ///
+    /// Note that this `Resolve` is not "complete". For example, the
+    /// dependencies do not know the difference between regular/dev/build
+    /// dependencies, so they are not filled in. It also does not include
+    /// `features`. Care should be taken when using this Resolve. One of the
+    /// primary uses is to be used with `resolve_with_previous` to guide the
+    /// resolver to create a complete Resolve.
     pub fn into_resolve(self, ws: &Workspace<'_>) -> CargoResult<Resolve> {
         let path_deps = build_path_deps(ws);
         let mut checksums = HashMap::new();

src/cargo/core/resolver/mod.rs

@@ -69,7 +69,7 @@ pub use self::encode::Metadata;
 pub use self::encode::{EncodableDependency, EncodablePackageId, EncodableResolve};
 pub use self::errors::{ActivateError, ActivateResult, ResolveError};
 pub use self::resolve::{Resolve, ResolveVersion};
-pub use self::types::Method;
+pub use self::types::ResolveOpts;
 
 mod conflict_cache;
 mod context;
@@ -120,7 +120,7 @@ mod types;
 ///     When we have a decision for how to implement is without breaking existing functionality
 ///     this flag can be removed.
 pub fn resolve(
-    summaries: &[(Summary, Method)],
+    summaries: &[(Summary, ResolveOpts)],
     replacements: &[(PackageIdSpec, Dependency)],
     registry: &mut dyn Registry,
     try_to_use: &HashSet<PackageId>,
@@ -169,7 +169,7 @@ pub fn resolve(
 fn activate_deps_loop(
     mut cx: Context,
     registry: &mut RegistryQueryer<'_>,
-    summaries: &[(Summary, Method)],
+    summaries: &[(Summary, ResolveOpts)],
     config: Option<&Config>,
 ) -> CargoResult<Context> {
     let mut backtrack_stack = Vec::new();
@@ -180,9 +180,9 @@ fn activate_deps_loop(
     let mut past_conflicting_activations = conflict_cache::ConflictCache::new();
 
     // Activate all the initial summaries to kick off some work.
-    for &(ref summary, ref method) in summaries {
+    for &(ref summary, ref opts) in summaries {
         debug!("initial activation: {}", summary.package_id());
-        let res = activate(&mut cx, registry, None, summary.clone(), method.clone());
+        let res = activate(&mut cx, registry, None, summary.clone(), opts.clone());
         match res {
             Ok(Some((frame, _))) => remaining_deps.push(frame),
             Ok(None) => (),
@@ -366,7 +366,7 @@ fn activate_deps_loop(
             };
 
             let pid = candidate.package_id();
-            let method = Method::Required {
+            let opts = ResolveOpts {
                 dev_deps: false,
                 features: Rc::clone(&features),
                 all_features: false,
@@ -379,7 +379,7 @@ fn activate_deps_loop(
                 dep.package_name(),
                 candidate.version()
             );
-            let res = activate(&mut cx, registry, Some((&parent, &dep)), candidate, method);
+            let res = activate(&mut cx, registry, Some((&parent, &dep)), candidate, opts);
 
             let successfully_activated = match res {
                 // Success! We've now activated our `candidate` in our context
@@ -583,15 +583,15 @@ fn activate_deps_loop(
 /// Attempts to activate the summary `candidate` in the context `cx`.
 ///
 /// This function will pull dependency summaries from the registry provided, and
-/// the dependencies of the package will be determined by the `method` provided.
+/// the dependencies of the package will be determined by the `opts` provided.
 /// If `candidate` was activated, this function returns the dependency frame to
 /// iterate through next.
 fn activate(
     cx: &mut Context,
     registry: &mut RegistryQueryer<'_>,
     parent: Option<(&Summary, &Dependency)>,
     candidate: Summary,
-    method: Method,
+    opts: ResolveOpts,
 ) -> ActivateResult<Option<(DepsFrame, Duration)>> {
     let candidate_pid = candidate.package_id();
     if let Some((parent, dep)) = parent {
@@ -652,7 +652,7 @@ fn activate(
         }
     }
 
-    let activated = cx.flag_activated(&candidate, &method, parent)?;
+    let activated = cx.flag_activated(&candidate, &opts, parent)?;
 
     let candidate = match registry.replacement_summary(candidate_pid) {
         Some(replace) => {
@@ -661,7 +661,7 @@ fn activate(
             // does. TBH it basically cause panics in the test suite if
             // `parent` is passed through here and `[replace]` is otherwise
             // on life support so it's not critical to fix bugs anyway per se.
-            if cx.flag_activated(replace, &method, None)? && activated {
+            if cx.flag_activated(replace, &opts, None)? && activated {
                 return Ok(None);
             }
             trace!(
@@ -682,7 +682,7 @@ fn activate(
 
     let now = Instant::now();
     let (used_features, deps) =
-        &*registry.build_deps(parent.map(|p| p.0.package_id()), &candidate, &method)?;
+        &*registry.build_deps(parent.map(|p| p.0.package_id()), &candidate, &opts)?;
 
     // Record what list of features is active for this package.
     if !used_features.is_empty() {