Auto merge of #6860 - Eh2406:no-RcList, r=alexcrichton

Make required dependency as future an error, remove RcList

This makes it an error to set a feature on a dependency if it is the name of a required dependency not a feature. This has been a warning for a long time. The resolver will backtrack to find a version that works, if one is available. So I think it is safe to make the change.

If you need to make a optional dependency into a required dependency without a semver breaking change this can be done with rename dependencies.

The result is that we can remove 3 data structures that get cloned once per tick of the resolver.

src/cargo/core/resolver/context.rs

@@ -13,9 +13,7 @@ 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 +35,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 +85,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 +93,7 @@ impl Context {
                 None
             },
             parents: Graph::new(),
-            resolve_replacements: RcList::new(),
             activations: im_rc::HashMap::new(),
-            warnings: RcList::new(),
         }
     }
 
@@ -128,7 +112,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(),
@@ -229,7 +212,7 @@ 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,
@@ -267,14 +250,20 @@ impl Context {
                     .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()
-                ));
+                return Err(match parent {
+                    None => failure::format_err!(
+                        "Package `{}` does not have feature `{}`. It has a required dependency \
+                         with that name, but only optional dependencies can be used as features.",
+                        s.package_id(),
+                        dep.name_in_toml()
+                    )
+                    .into(),
+                    Some(p) => (
+                        p.package_id(),
+                        ConflictReason::RequiredDependencyAsFeatures(dep.name_in_toml()),
+                    )
+                        .into(),
+                });
             }
             let mut base = base.1.clone();
             base.extend(dep.features().iter());
@@ -331,28 +320,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);
+                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),
@@ -126,7 +127,7 @@ pub(super) fn activation_error(
             msg.push_str(&describe_path(&cx.parents.path_to_bottom(p)));
         }
 
-        let (features_errors, other_errors): (Vec<_>, Vec<_>) = other_errors
+        let (features_errors, mut other_errors): (Vec<_>, Vec<_>) = other_errors
             .drain(..)
             .partition(|&(_, r)| r.is_missing_features());
 
@@ -145,6 +146,29 @@ pub(super) fn activation_error(
             // p == parent so the full path is redundant.
         }
 
+        let (required_dependency_as_features_errors, other_errors): (Vec<_>, Vec<_>) = other_errors
+            .drain(..)
+            .partition(|&(_, r)| r.is_required_dependency_as_features());
+
+        for &(p, r) in required_dependency_as_features_errors.iter() {
+            if let ConflictReason::RequiredDependencyAsFeatures(ref features) = *r {
+                msg.push_str("\n\nthe package `");
+                msg.push_str(&*p.name());
+                msg.push_str("` depends on `");
+                msg.push_str(&*dep.package_name());
+                msg.push_str("`, with features: `");
+                msg.push_str(features);
+                msg.push_str("` but `");
+                msg.push_str(&*dep.package_name());
+                msg.push_str("` does not have these features.\n");
+                msg.push_str(
+                    " It has a required dependency with that name, \
+                     but only optional dependencies can be used as features.\n",
+                );
+            }
+            // p == parent so the full path is redundant.
+        }
+
         if !other_errors.is_empty() {
             msg.push_str(
                 "\n\nall possible versions conflict with \

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};
@@ -124,7 +124,6 @@ pub fn resolve(
     registry: &mut dyn Registry,
     try_to_use: &HashSet<PackageId>,
     config: Option<&Config>,
-    print_warnings: bool,
     check_public_visible_dependencies: bool,
 ) -> CargoResult<Resolve> {
     let cx = Context::new(check_public_visible_dependencies);
@@ -143,7 +142,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()))
@@ -157,18 +156,6 @@ pub fn resolve(
     check_duplicate_pkgs_in_lockfile(&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;
-            }
-        }
-    }
-
     Ok(resolve)
 }
 
@@ -613,8 +600,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 +660,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);
             }

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;
         }
@@ -403,6 +426,12 @@ pub enum ConflictReason {
     /// candidate we're activating didn't actually have the feature `foo`.
     MissingFeatures(String),
 
+    /// A dependency listed features that ended up being a required dependency.
+    /// For example we tried to activate feature `foo` but the
+    /// candidate we're activating didn't actually have the feature `foo`
+    /// it had a dependency `foo` instead.
+    RequiredDependencyAsFeatures(InternedString),
+
     // TODO: needs more info for `activation_error`
     // TODO: needs more info for `find_candidate`
     /// pub dep error
@@ -423,6 +452,13 @@ impl ConflictReason {
         }
         false
     }
+
+    pub fn is_required_dependency_as_features(&self) -> bool {
+        if let ConflictReason::RequiredDependencyAsFeatures(_) = *self {
+            return true;
+        }
+        false
+    }
 }
 
 /// A list of packages that have gotten in the way of resolving a dependency.
@@ -481,50 +517,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),
-}