Make required dependency as future an error, remove RcList

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),
-}