Make required dependency as future an error, remove RcList

src/cargo/core/resolver/context.rs

@@ -9,13 +9,12 @@ use log::debug;
 
 use crate::core::interning::InternedString;
 use crate::core::{Dependency, FeatureValue, PackageId, SourceId, Summary};
+use crate::util::config::Config;
 use crate::util::CargoResult;
 use crate::util::Graph;
 
 use super::errors::ActivateResult;
-use super::types::{
-    ConflictMap, ConflictReason, DepInfo, GraphNode, Method, RcList, RegistryQueryer,
-};
+use super::types::{ConflictMap, ConflictReason, DepInfo, Method, RegistryQueryer};
 
 pub use super::encode::{EncodableDependency, EncodablePackageId, EncodableResolve};
 pub use super::encode::{Metadata, WorkspaceResolve};
@@ -37,20 +36,9 @@ pub struct Context {
     pub public_dependency:
         Option<im_rc::HashMap<PackageId, im_rc::HashMap<InternedString, (PackageId, bool)>>>,
 
-    // This is somewhat redundant with the `resolve_graph` that stores the same data,
-    //   but for querying in the opposite order.
     /// a way to look up for a package in activations what packages required it
     /// and all of the exact deps that it fulfilled.
     pub parents: Graph<PackageId, Rc<Vec<Dependency>>>,
-
-    // These are two cheaply-cloneable lists (O(1) clone) which are effectively
-    // hash maps but are built up as "construction lists". We'll iterate these
-    // at the very end and actually construct the map that we're making.
-    pub resolve_graph: RcList<GraphNode>,
-    pub resolve_replacements: RcList<(PackageId, PackageId)>,
-
-    // These warnings are printed after resolution.
-    pub warnings: RcList<String>,
 }
 
 /// When backtracking it can be useful to know how far back to go.
@@ -98,7 +86,6 @@ impl PackageId {
 impl Context {
     pub fn new(check_public_visible_dependencies: bool) -> Context {
         Context {
-            resolve_graph: RcList::new(),
             resolve_features: im_rc::HashMap::new(),
             links: im_rc::HashMap::new(),
             public_dependency: if check_public_visible_dependencies {
@@ -107,9 +94,7 @@ impl Context {
                 None
             },
             parents: Graph::new(),
-            resolve_replacements: RcList::new(),
             activations: im_rc::HashMap::new(),
-            warnings: RcList::new(),
         }
     }
 
@@ -128,7 +113,6 @@ impl Context {
                 );
             }
             im_rc::hashmap::Entry::Vacant(v) => {
-                self.resolve_graph.push(GraphNode::Add(id));
                 if let Some(link) = summary.links() {
                     ensure!(
                         self.links.insert(link, id).is_none(),
@@ -174,7 +158,7 @@ impl Context {
         // 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 deps = self.resolve_features(parent, candidate, method)?;
+        let deps = self.resolve_features(parent, candidate, method, None)?;
 
         // Next, transform all dependencies into a list of possible candidates
         // which can satisfy that dependency.
@@ -229,11 +213,12 @@ impl Context {
     }
 
     /// Returns all dependencies and the features we want from them.
-    fn resolve_features<'b>(
+    pub fn resolve_features<'b>(
         &mut self,
         parent: Option<&Summary>,
         s: &'b Summary,
         method: &'b Method<'_>,
+        config: Option<&Config>,
     ) -> ActivateResult<Vec<(Dependency, Vec<InternedString>)>> {
         let dev_deps = match *method {
             Method::Everything => true,
@@ -261,20 +246,23 @@ impl Context {
             // name.
             let base = reqs.deps.get(&dep.name_in_toml()).unwrap_or(&default_dep);
             used_features.insert(dep.name_in_toml());
-            let always_required = !dep.is_optional()
-                && !s
-                    .dependencies()
-                    .iter()
-                    .any(|d| d.is_optional() && d.name_in_toml() == dep.name_in_toml());
-            if always_required && base.0 {
-                self.warnings.push(format!(
-                    "Package `{}` does not have feature `{}`. It has a required dependency \
-                     with that name, but only optional dependencies can be used as features. \
-                     This is currently a warning to ease the transition, but it will become an \
-                     error in the future.",
-                    s.package_id(),
-                    dep.name_in_toml()
-                ));
+            if let Some(config) = config {
+                let mut shell = config.shell();
+                let always_required = !dep.is_optional()
+                    && !s
+                        .dependencies()
+                        .iter()
+                        .any(|d| d.is_optional() && d.name_in_toml() == dep.name_in_toml());
+                if always_required && base.0 {
+                    shell.warn(&format!(
+                        "Package `{}` does not have feature `{}`. It has a required dependency \
+                         with that name, but only optional dependencies can be used as features. \
+                         This is currently a warning to ease the transition, but it will become an \
+                         error in the future.",
+                        s.package_id(),
+                        dep.name_in_toml()
+                    ))?
+                }
             }
             let mut base = base.1.clone();
             base.extend(dep.features().iter());
@@ -331,28 +319,28 @@ impl Context {
         Ok(ret)
     }
 
-    pub fn resolve_replacements(&self) -> HashMap<PackageId, PackageId> {
-        let mut replacements = HashMap::new();
-        let mut cur = &self.resolve_replacements;
-        while let Some(ref node) = cur.head {
-            let (k, v) = node.0;
-            replacements.insert(k, v);
-            cur = &node.1;
-        }
-        replacements
+    pub fn resolve_replacements(
+        &self,
+        registry: &RegistryQueryer<'_>,
+    ) -> HashMap<PackageId, PackageId> {
+        self.activations
+            .values()
+            .filter_map(|(s, _)| registry.used_replacement_for(s.package_id()))
+            .collect()
     }
 
     pub fn graph(&self) -> Graph<PackageId, Vec<Dependency>> {
         let mut graph: Graph<PackageId, Vec<Dependency>> = Graph::new();
-        let mut cur = &self.resolve_graph;
-        while let Some(ref node) = cur.head {
-            match node.0 {
-                GraphNode::Add(ref p) => graph.add(p.clone()),
-                GraphNode::Link(ref a, ref b, ref dep) => {
-                    graph.link(a.clone(), b.clone()).push(dep.clone());
-                }
+        self.activations
+            .values()
+            .for_each(|(r, _)| graph.add(r.package_id()));
+        for i in self.parents.iter() {
+            graph.add(*i);
+            for (o, e) in self.parents.edges(i) {
+                let old_link = graph.link(*o, *i);
+                debug_assert!(old_link.is_empty());
+                *old_link = e.to_vec();
             }
-            cur = &node.1;
         }
         graph
     }

src/cargo/core/resolver/errors.rs

@@ -50,6 +50,7 @@ impl fmt::Display for ResolveError {
 
 pub type ActivateResult<T> = Result<T, ActivateError>;
 
+#[derive(Debug)]
 pub enum ActivateError {
     Fatal(failure::Error),
     Conflict(PackageId, ConflictReason),

src/cargo/core/resolver/mod.rs

@@ -62,7 +62,7 @@ use crate::util::errors::CargoResult;
 use crate::util::profile;
 
 use self::context::{Activations, Context};
-use self::types::{Candidate, ConflictMap, ConflictReason, DepsFrame, GraphNode};
+use self::types::{Candidate, ConflictMap, ConflictReason, DepsFrame};
 use self::types::{RcVecIter, RegistryQueryer, RemainingDeps, ResolverProgress};
 
 pub use self::encode::{EncodableDependency, EncodablePackageId, EncodableResolve};
@@ -143,7 +143,7 @@ pub fn resolve(
     }
     let resolve = Resolve::new(
         cx.graph(),
-        cx.resolve_replacements(),
+        cx.resolve_replacements(&registry),
         cx.resolve_features
             .iter()
             .map(|(k, v)| (*k, v.iter().map(|x| x.to_string()).collect()))
@@ -158,15 +158,8 @@ pub fn resolve(
     trace!("resolved: {:?}", resolve);
 
     // If we have a shell, emit warnings about required deps used as feature.
-    if let Some(config) = config {
-        if print_warnings {
-            let mut shell = config.shell();
-            let mut warnings = &cx.warnings;
-            while let Some(ref head) = warnings.head {
-                shell.warn(&head.0)?;
-                warnings = &head.1;
-            }
-        }
+    if print_warnings && config.is_some() {
+        emit_warnings(&cx, &resolve, summaries, config)
     }
 
     Ok(resolve)
@@ -613,8 +606,6 @@ fn activate(
     let candidate_pid = candidate.summary.package_id();
     if let Some((parent, dep)) = parent {
         let parent_pid = parent.package_id();
-        cx.resolve_graph
-            .push(GraphNode::Link(parent_pid, candidate_pid, dep.clone()));
         Rc::make_mut(
             // add a edge from candidate to parent in the parents graph
             cx.parents.link(candidate_pid, parent_pid),
@@ -675,8 +666,6 @@ fn activate(
 
     let candidate = match candidate.replace {
         Some(replace) => {
-            cx.resolve_replacements
-                .push((candidate_pid, replace.package_id()));
             if cx.flag_activated(&replace, method)? && activated {
                 return Ok(None);
             }
@@ -1098,3 +1087,59 @@ fn check_duplicate_pkgs_in_lockfile(resolve: &Resolve) -> CargoResult<()> {
     }
     Ok(())
 }
+
+/// Re-run all used `resolve_features` so it can print warnings.
+/// Within the resolution loop we do not pass a `config` to `resolve_features`
+/// this tells it not to print warnings. Hear we do pass `config` triggering it to print them.
+/// This is done as the resolution is NP-Hard so the loop can potentially call `resolve features`
+/// an exponential number of times, but this pass is linear in the number of dependencies.
+fn emit_warnings(
+    cx: &Context,
+    resolve: &Resolve,
+    summaries: &[(Summary, Method<'_>)],
+    config: Option<&Config>,
+) {
+    let mut new_cx = cx.clone();
+    new_cx.resolve_features = im_rc::HashMap::new();
+    let mut features_from_dep = HashMap::new();
+    for (summary, method) in summaries {
+        for (dep, features) in new_cx
+            .resolve_features(None, summary, &method, config)
+            .expect("can not resolve_features for a required summary")
+        {
+            features_from_dep.insert((summary.package_id(), dep), features);
+        }
+    }
+    // crates enable features for their dependencies.
+    // So by iterating reverse topologically we process all of the parents before each child.
+    // Then we know all the needed features for each crate.
+    let topological_sort = resolve.sort();
+    for summary in topological_sort.iter().rev().map(|id| {
+        cx.activations
+            .get(&id.as_activations_key())
+            .expect("id in dependency graph but not in activations")
+            .0
+            .clone()
+    }) {
+        for (parent, deps) in cx.parents.edges(&summary.package_id()) {
+            for dep in deps.iter() {
+                let features = features_from_dep
+                    .remove(&(*parent, dep.clone()))
+                    .expect("fulfilled a dep that was not needed");
+                let method = Method::Required {
+                    dev_deps: false,
+                    features: &features,
+                    all_features: false,
+                    uses_default_features: dep.uses_default_features(),
+                };
+                for (dep, features) in new_cx
+                    .resolve_features(None, &summary, &method, config)
+                    .expect("can not resolve_features for a used dep")
+                {
+                    features_from_dep.insert((summary.package_id(), dep), features);
+                }
+            }
+        }
+    }
+    assert_eq!(cx.resolve_features, new_cx.resolve_features);
+}

src/cargo/core/resolver/types.rs

@@ -95,11 +95,12 @@ pub struct RegistryQueryer<'a> {
     pub registry: &'a mut (dyn Registry + 'a),
     replacements: &'a [(PackageIdSpec, Dependency)],
     try_to_use: &'a HashSet<PackageId>,
-    cache: HashMap<Dependency, Rc<Vec<Candidate>>>,
     // If set the list of dependency candidates will be sorted by minimal
     // versions first. That allows `cargo update -Z minimal-versions` which will
     // specify minimum dependency versions to be used.
     minimal_versions: bool,
+    cache: HashMap<Dependency, Rc<Vec<Candidate>>>,
+    used_replacements: HashMap<PackageId, PackageId>,
 }
 
 impl<'a> RegistryQueryer<'a> {
@@ -112,12 +113,17 @@ impl<'a> RegistryQueryer<'a> {
         RegistryQueryer {
             registry,
             replacements,
-            cache: HashMap::new(),
             try_to_use,
             minimal_versions,
+            cache: HashMap::new(),
+            used_replacements: HashMap::new(),
         }
     }
 
+    pub fn used_replacement_for(&self, p: PackageId) -> Option<(PackageId, PackageId)> {
+        self.used_replacements.get(&p).map(|&r| (p, r))
+    }
+
     /// Queries the `registry` to return a list of candidates for `dep`.
     ///
     /// This method is the location where overrides are taken into account. If
@@ -212,6 +218,23 @@ impl<'a> RegistryQueryer<'a> {
             for dep in summary.dependencies() {
                 debug!("\t{} => {}", dep.package_name(), dep.version_req());
             }
+            if let Some(r) = &replace {
+                if let Some(old) = self
+                    .used_replacements
+                    .insert(summary.package_id(), r.package_id())
+                {
+                    debug_assert_eq!(
+                        r.package_id(),
+                        old,
+                        "we are inconsistent about witch replacement is used for {:?}, \
+                         one time we used {:?}, \
+                         now we are adding {:?}.",
+                        summary.package_id(),
+                        old,
+                        r.package_id()
+                    )
+                }
+            }
 
             candidate.replace = replace;
         }
@@ -481,50 +504,3 @@ where
 }
 
 impl<T: Clone> ExactSizeIterator for RcVecIter<T> {}
-
-pub struct RcList<T> {
-    pub head: Option<Rc<(T, RcList<T>)>>,
-}
-
-impl<T> RcList<T> {
-    pub fn new() -> RcList<T> {
-        RcList { head: None }
-    }
-
-    pub fn push(&mut self, data: T) {
-        let node = Rc::new((
-            data,
-            RcList {
-                head: self.head.take(),
-            },
-        ));
-        self.head = Some(node);
-    }
-}
-
-// Not derived to avoid `T: Clone`
-impl<T> Clone for RcList<T> {
-    fn clone(&self) -> RcList<T> {
-        RcList {
-            head: self.head.clone(),
-        }
-    }
-}
-
-// Avoid stack overflows on drop by turning recursion into a loop
-impl<T> Drop for RcList<T> {
-    fn drop(&mut self) {
-        let mut cur = self.head.take();
-        while let Some(head) = cur {
-            match Rc::try_unwrap(head) {
-                Ok((_data, mut next)) => cur = next.head.take(),
-                Err(_) => break,
-            }
-        }
-    }
-}
-
-pub enum GraphNode {
-    Add(PackageId),
-    Link(PackageId, PackageId, Dependency),
-}