diff --git a/src/cargo/core/dependency.rs b/src/cargo/core/dependency.rs index e2a27e0431e..b779d77c693 100644 --- a/src/cargo/core/dependency.rs +++ b/src/cargo/core/dependency.rs @@ -40,6 +40,7 @@ struct Inner { explicit_name_in_toml: Option, optional: bool, + public: bool, default_features: bool, features: Vec, @@ -217,6 +218,7 @@ impl Dependency { kind: Kind::Normal, only_match_name: true, optional: false, + public: false, features: Vec::new(), default_features: true, specified_req: false, @@ -293,6 +295,16 @@ impl Dependency { self.inner.kind } + pub fn is_public(&self) -> bool { + self.inner.public + } + + /// Sets whether the dependency is public. + pub fn set_public(&mut self, public: bool) -> &mut Dependency { + Rc::make_mut(&mut self.inner).public = public; + self + } + pub fn specified_req(&self) -> bool { self.inner.specified_req } diff --git a/src/cargo/core/resolver/conflict_cache.rs b/src/cargo/core/resolver/conflict_cache.rs index 6e14dade150..58beb6da17f 100644 --- a/src/cargo/core/resolver/conflict_cache.rs +++ b/src/cargo/core/resolver/conflict_cache.rs @@ -2,7 +2,7 @@ use std::collections::{BTreeMap, HashMap, HashSet}; use log::trace; -use super::types::ConflictReason; +use super::types::{ConflictMap, ConflictReason}; use crate::core::resolver::Context; use crate::core::{Dependency, PackageId}; @@ -10,7 +10,7 @@ use crate::core::{Dependency, PackageId}; /// efficiently see if any of the stored sets are a subset of a search set. enum ConflictStoreTrie { /// One of the stored sets. - Leaf(BTreeMap), + Leaf(ConflictMap), /// A map from an element to a subtrie where /// all the sets in the subtrie contains that element. Node(BTreeMap), @@ -23,7 +23,7 @@ impl ConflictStoreTrie { &self, cx: &Context, must_contain: Option, - ) -> Option<&BTreeMap> { + ) -> Option<&ConflictMap> { match self { ConflictStoreTrie::Leaf(c) => { if must_contain.is_none() { @@ -57,18 +57,14 @@ impl ConflictStoreTrie { } } - fn insert( - &mut self, - mut iter: impl Iterator, - con: BTreeMap, - ) { + fn insert(&mut self, mut iter: impl Iterator, con: ConflictMap) { if let Some(pid) = iter.next() { if let ConflictStoreTrie::Node(p) = self { p.entry(pid) .or_insert_with(|| ConflictStoreTrie::Node(BTreeMap::new())) .insert(iter, con); } - // Else, we already have a subset of this in the `ConflictStore`. + // Else, we already have a subset of this in the `ConflictStore`. } else { // We are at the end of the set we are adding, there are three cases for what to do // next: @@ -147,7 +143,7 @@ impl ConflictCache { cx: &Context, dep: &Dependency, must_contain: Option, - ) -> Option<&BTreeMap> { + ) -> Option<&ConflictMap> { let out = self .con_from_dep .get(dep)? @@ -161,18 +157,19 @@ impl ConflictCache { } out } - pub fn conflicting( - &self, - cx: &Context, - dep: &Dependency, - ) -> Option<&BTreeMap> { + pub fn conflicting(&self, cx: &Context, dep: &Dependency) -> Option<&ConflictMap> { self.find_conflicting(cx, dep, None) } /// Adds to the cache a conflict of the form: /// `dep` is known to be unresolvable if /// all the `PackageId` entries are activated. - pub fn insert(&mut self, dep: &Dependency, con: &BTreeMap) { + pub fn insert(&mut self, dep: &Dependency, con: &ConflictMap) { + if con.values().any(|c| *c == ConflictReason::PublicDependency) { + // TODO: needs more info for back jumping + // for now refuse to cache it. + return; + } self.con_from_dep .entry(dep.clone()) .or_insert_with(|| ConflictStoreTrie::Node(BTreeMap::new())) diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs index eb13dfa08e2..0c8b69ebf6f 100644 --- a/src/cargo/core/resolver/context.rs +++ b/src/cargo/core/resolver/context.rs @@ -1,4 +1,4 @@ -use std::collections::{BTreeMap, HashMap, HashSet}; +use std::collections::{HashMap, HashSet}; use std::rc::Rc; // "ensure" seems to require "bail" be in scope (macro hygiene issue?). @@ -12,7 +12,9 @@ use crate::util::CargoResult; use crate::util::Graph; use super::errors::ActivateResult; -use super::types::{ConflictReason, DepInfo, GraphNode, Method, RcList, RegistryQueryer}; +use super::types::{ + ConflictMap, ConflictReason, DepInfo, GraphNode, Method, RcList, RegistryQueryer, +}; pub use super::encode::{EncodableDependency, EncodablePackageId, EncodableResolve}; pub use super::encode::{Metadata, WorkspaceResolve}; @@ -25,8 +27,20 @@ pub use super::resolve::Resolve; #[derive(Clone)] pub struct Context { pub activations: Activations, + /// list the features that are activated for each package pub resolve_features: im_rc::HashMap>>, + /// get the package that will be linking to a native library by its links attribute pub links: im_rc::HashMap, + /// for each package the list of names it can see, + /// then for each name the exact version that name represents and weather the name is public. + pub public_dependency: + Option>>, + + // 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>>, // 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 @@ -38,14 +52,21 @@ pub struct Context { pub warnings: RcList, } +/// list all the activated versions of a particular crate name from a source pub type Activations = im_rc::HashMap<(InternedString, SourceId), Rc>>; impl Context { - pub fn new() -> 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 { + Some(im_rc::HashMap::new()) + } else { + None + }, + parents: Graph::new(), resolve_replacements: RcList::new(), activations: im_rc::HashMap::new(), warnings: RcList::new(), @@ -147,7 +168,7 @@ impl Context { pub fn is_conflicting( &self, parent: Option, - conflicting_activations: &BTreeMap, + conflicting_activations: &ConflictMap, ) -> bool { conflicting_activations .keys() diff --git a/src/cargo/core/resolver/errors.rs b/src/cargo/core/resolver/errors.rs index 20c8d888776..071423690de 100644 --- a/src/cargo/core/resolver/errors.rs +++ b/src/cargo/core/resolver/errors.rs @@ -1,4 +1,3 @@ -use std::collections::BTreeMap; use std::fmt; use crate::core::{Dependency, PackageId, Registry, Summary}; @@ -8,7 +7,7 @@ use failure::{Error, Fail}; use semver; use super::context::Context; -use super::types::{Candidate, ConflictReason}; +use super::types::{Candidate, ConflictMap, ConflictReason}; /// Error during resolution providing a path of `PackageId`s. pub struct ResolveError { @@ -73,16 +72,15 @@ pub(super) fn activation_error( registry: &mut dyn Registry, parent: &Summary, dep: &Dependency, - conflicting_activations: &BTreeMap, + conflicting_activations: &ConflictMap, candidates: &[Candidate], config: Option<&Config>, ) -> ResolveError { - let graph = cx.graph(); let to_resolve_err = |err| { ResolveError::new( err, - graph - .path_to_top(&parent.package_id()) + cx.parents + .path_to_bottom(&parent.package_id()) .into_iter() .cloned() .collect(), @@ -92,7 +90,9 @@ pub(super) fn activation_error( if !candidates.is_empty() { let mut msg = format!("failed to select a version for `{}`.", dep.package_name()); msg.push_str("\n ... required by "); - msg.push_str(&describe_path(&graph.path_to_top(&parent.package_id()))); + msg.push_str(&describe_path( + &cx.parents.path_to_bottom(&parent.package_id()), + )); msg.push_str("\nversions that meet the requirements `"); msg.push_str(&dep.version_req().to_string()); @@ -123,7 +123,7 @@ pub(super) fn activation_error( msg.push_str(link); msg.push_str("` as well:\n"); } - msg.push_str(&describe_path(&graph.path_to_top(p))); + msg.push_str(&describe_path(&cx.parents.path_to_bottom(p))); } let (features_errors, other_errors): (Vec<_>, Vec<_>) = other_errors @@ -154,7 +154,7 @@ pub(super) fn activation_error( for &(p, _) in other_errors.iter() { msg.push_str("\n\n previously selected "); - msg.push_str(&describe_path(&graph.path_to_top(p))); + msg.push_str(&describe_path(&cx.parents.path_to_bottom(p))); } msg.push_str("\n\nfailed to select a version for `"); @@ -204,7 +204,9 @@ pub(super) fn activation_error( registry.describe_source(dep.source_id()), ); msg.push_str("required by "); - msg.push_str(&describe_path(&graph.path_to_top(&parent.package_id()))); + msg.push_str(&describe_path( + &cx.parents.path_to_bottom(&parent.package_id()), + )); // If we have a path dependency with a locked version, then this may // indicate that we updated a sub-package and forgot to run `cargo @@ -265,7 +267,9 @@ pub(super) fn activation_error( msg.push_str("\n"); } msg.push_str("required by "); - msg.push_str(&describe_path(&graph.path_to_top(&parent.package_id()))); + msg.push_str(&describe_path( + &cx.parents.path_to_bottom(&parent.package_id()), + )); msg }; diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index ba1ef7d0744..a0b57173af1 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/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, ConflictReason, DepsFrame, GraphNode}; +use self::types::{Candidate, ConflictMap, ConflictReason, DepsFrame, GraphNode}; use self::types::{RcVecIter, RegistryQueryer, RemainingDeps, ResolverProgress}; pub use self::encode::{EncodableDependency, EncodablePackageId, EncodableResolve}; @@ -107,6 +107,17 @@ mod types; /// /// * `print_warnings` - whether or not to print backwards-compatibility /// warnings and such +/// +/// * `check_public_visible_dependencies` - a flag for whether to enforce the restrictions +/// introduced in the "public & private dependencies" RFC (1977). The current implementation +/// makes sure that there is only one version of each name visible to each package. +/// +/// But there are 2 stable ways to directly depend on different versions of the same name. +/// 1. Use the renamed dependencies functionality +/// 2. Use 'cfg({})' dependencies functionality +/// +/// 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<'_>)], replacements: &[(PackageIdSpec, Dependency)], @@ -114,8 +125,9 @@ pub fn resolve( try_to_use: &HashSet, config: Option<&Config>, print_warnings: bool, + check_public_visible_dependencies: bool, ) -> CargoResult { - let cx = Context::new(); + let cx = Context::new(check_public_visible_dependencies); let _p = profile::start("resolving"); let minimal_versions = match config { Some(config) => config.cli_unstable().minimal_versions, @@ -247,7 +259,7 @@ fn activate_deps_loop( // // This is a map of package ID to a reason why that packaged caused a // conflict for us. - let mut conflicting_activations = BTreeMap::new(); + let mut conflicting_activations = ConflictMap::new(); // When backtracking we don't fully update `conflicting_activations` // especially for the cases that we didn't make a backtrack frame in the @@ -257,7 +269,12 @@ fn activate_deps_loop( let mut backtracked = false; loop { - let next = remaining_candidates.next(&mut conflicting_activations, &cx, &dep); + let next = remaining_candidates.next( + &mut conflicting_activations, + &cx, + &dep, + parent.package_id(), + ); let (candidate, has_another) = next.ok_or(()).or_else(|_| { // If we get here then our `remaining_candidates` was just @@ -585,12 +602,65 @@ fn activate( candidate: Candidate, method: &Method<'_>, ) -> ActivateResult> { + let candidate_pid = candidate.summary.package_id(); if let Some((parent, dep)) = parent { - cx.resolve_graph.push(GraphNode::Link( - parent.package_id(), - candidate.summary.package_id(), - dep.clone(), - )); + 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), + ) + // and associate dep with that edge + .push(dep.clone()); + if let Some(public_dependency) = cx.public_dependency.as_mut() { + // one tricky part is that `candidate_pid` may already be active and + // have public dependencies of its own. So we not only need to mark + // `candidate_pid` as visible to its parents but also all of its existing + // public dependencies. + let existing_public_deps: Vec = public_dependency + .get(&candidate_pid) + .iter() + .flat_map(|x| x.values()) + .filter_map(|x| if x.1 { Some(&x.0) } else { None }) + .chain(&Some(candidate_pid)) + .cloned() + .collect(); + for c in existing_public_deps { + // for each (transitive) parent that can newly see `t` + let mut stack = vec![(parent_pid, dep.is_public())]; + while let Some((p, public)) = stack.pop() { + match public_dependency.entry(p).or_default().entry(c.name()) { + im_rc::hashmap::Entry::Occupied(mut o) => { + // the (transitive) parent can already see something by `c`s name, it had better be `c`. + assert_eq!(o.get().0, c); + if o.get().1 { + // The previous time the parent saw `c`, it was a public dependency. + // So all of its parents already know about `c` + // and we can save some time by stopping now. + continue; + } + if public { + // Mark that `c` has now bean seen publicly + o.insert((c, public)); + } + } + im_rc::hashmap::Entry::Vacant(v) => { + // The (transitive) parent does not have anything by `c`s name, + // so we add `c`. + v.insert((c, public)); + } + } + // if `candidate_pid` was a private dependency of `p` then `p` parents can't see `c` thru `p` + if public { + // if it was public, then we add all of `p`s parents to be checked + for &(grand, ref d) in cx.parents.edges(&p) { + stack.push((grand, d.iter().any(|x| x.is_public()))); + } + } + } + } + } } let activated = cx.flag_activated(&candidate.summary, method)?; @@ -598,14 +668,14 @@ fn activate( let candidate = match candidate.replace { Some(replace) => { cx.resolve_replacements - .push((candidate.summary.package_id(), replace.package_id())); + .push((candidate_pid, replace.package_id())); if cx.flag_activated(&replace, method)? && activated { return Ok(None); } trace!( "activating {} (replacing {})", replace.package_id(), - candidate.summary.package_id() + candidate_pid ); replace } @@ -613,7 +683,7 @@ fn activate( if activated { return Ok(None); } - trace!("activating {}", candidate.summary.package_id()); + trace!("activating {}", candidate_pid); candidate.summary } }; @@ -637,7 +707,7 @@ struct BacktrackFrame { parent: Summary, dep: Dependency, features: Rc>, - conflicting_activations: BTreeMap, + conflicting_activations: ConflictMap, } /// A helper "iterator" used to extract candidates within a current `Context` of @@ -684,13 +754,14 @@ impl RemainingCandidates { /// original list for the reason listed. fn next( &mut self, - conflicting_prev_active: &mut BTreeMap, + conflicting_prev_active: &mut ConflictMap, cx: &Context, dep: &Dependency, + parent: PackageId, ) -> Option<(Candidate, bool)> { let prev_active = cx.prev_active(dep); - for (_, b) in self.remaining.by_ref() { + 'main: for (_, b) in self.remaining.by_ref() { // The `links` key in the manifest dictates that there's only one // package in a dependency graph, globally, with that particular // `links` key. If this candidate links to something that's already @@ -725,6 +796,43 @@ impl RemainingCandidates { continue; } } + // We may still have to reject do to a public dependency conflict. If one of any of our + // ancestors that can see us already knows about a different crate with this name then + // we have to reject this candidate. Additionally this candidate may already have been + // activated and have public dependants of its own, + // all of witch also need to be checked the same way. + if let Some(public_dependency) = cx.public_dependency.as_ref() { + let existing_public_deps: Vec = public_dependency + .get(&b.summary.package_id()) + .iter() + .flat_map(|x| x.values()) + .filter_map(|x| if x.1 { Some(&x.0) } else { None }) + .chain(&Some(b.summary.package_id())) + .cloned() + .collect(); + for t in existing_public_deps { + // for each (transitive) parent that can newly see `t` + let mut stack = vec![(parent, dep.is_public())]; + while let Some((p, public)) = stack.pop() { + // TODO: dont look at the same thing more then once + if let Some(o) = public_dependency.get(&p).and_then(|x| x.get(&t.name())) { + if o.0 != t { + // the (transitive) parent can already see a different version by `t`s name. + // So, adding `b` will cause `p` to have a public dependency conflict on `t`. + conflicting_prev_active.insert(p, ConflictReason::PublicDependency); + continue 'main; + } + } + // if `b` was a private dependency of `p` then `p` parents can't see `t` thru `p` + if public { + // if it was public, then we add all of `p`s parents to be checked + for &(grand, ref d) in cx.parents.edges(&p) { + stack.push((grand, d.iter().any(|x| x.is_public()))); + } + } + } + } + } // Well if we made it this far then we've got a valid dependency. We // want this iterator to be inherently "peekable" so we don't @@ -777,13 +885,14 @@ fn find_candidate( backtrack_stack: &mut Vec, parent: &Summary, backtracked: bool, - conflicting_activations: &BTreeMap, + conflicting_activations: &ConflictMap, ) -> Option<(Candidate, bool, BacktrackFrame)> { while let Some(mut frame) = backtrack_stack.pop() { let next = frame.remaining_candidates.next( &mut frame.conflicting_activations, &frame.context, &frame.dep, + frame.parent.package_id(), ); let (candidate, has_another) = match next { Some(pair) => pair, @@ -791,14 +900,23 @@ fn find_candidate( }; // When we're calling this method we know that `parent` failed to // activate. That means that some dependency failed to get resolved for - // whatever reason, and all of those reasons (plus maybe some extras) - // are listed in `conflicting_activations`. + // whatever reason. Normally, that means that all of those reasons + // (plus maybe some extras) are listed in `conflicting_activations`. // // This means that if all members of `conflicting_activations` are still // active in this back up we know that we're guaranteed to not actually // make any progress. As a result if we hit this condition we can // completely skip this backtrack frame and move on to the next. + // + // The abnormal situations are things that do not put all of the reasons in `conflicting_activations`: + // If we backtracked we do not know how our `conflicting_activations` related to + // the cause of that backtrack, so we do not update it. + // If we had a PublicDependency conflict, then we do not yet have a compact way to + // represent all the parts of the problem, so `conflicting_activations` is incomplete. if !backtracked + && !conflicting_activations + .values() + .any(|c| *c == ConflictReason::PublicDependency) && frame .context .is_conflicting(Some(parent.package_id()), conflicting_activations) diff --git a/src/cargo/core/resolver/resolve.rs b/src/cargo/core/resolver/resolve.rs index 87117af254a..04261d1f398 100644 --- a/src/cargo/core/resolver/resolve.rs +++ b/src/cargo/core/resolver/resolve.rs @@ -1,7 +1,6 @@ use std::borrow::Borrow; use std::collections::{HashMap, HashSet}; use std::fmt; -use std::hash::Hash; use std::iter::FromIterator; use url::Url; @@ -163,7 +162,7 @@ unable to verify that `{0}` is the same as when the lockfile was generated pub fn contains(&self, k: &Q) -> bool where PackageId: Borrow, - Q: Hash + Eq, + Q: Ord + Eq, { self.graph.contains(k) } @@ -179,11 +178,11 @@ unable to verify that `{0}` is the same as when the lockfile was generated pub fn deps(&self, pkg: PackageId) -> impl Iterator { self.graph .edges(&pkg) - .map(move |(&id, deps)| (self.replacement(id).unwrap_or(id), deps.as_slice())) + .map(move |&(id, ref deps)| (self.replacement(id).unwrap_or(id), deps.as_slice())) } pub fn deps_not_replaced<'a>(&'a self, pkg: PackageId) -> impl Iterator + 'a { - self.graph.edges(&pkg).map(|(&id, _)| id) + self.graph.edges(&pkg).map(|&(id, _)| id) } pub fn replacement(&self, pkg: PackageId) -> Option { diff --git a/src/cargo/core/resolver/types.rs b/src/cargo/core/resolver/types.rs index cdef344805b..452032597c2 100644 --- a/src/cargo/core/resolver/types.rs +++ b/src/cargo/core/resolver/types.rs @@ -1,5 +1,5 @@ use std::cmp::Ordering; -use std::collections::{HashMap, HashSet}; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::ops::Range; use std::rc::Rc; use std::time::{Duration, Instant}; @@ -402,6 +402,11 @@ pub enum ConflictReason { /// candidate. For example we tried to activate feature `foo` but the /// candidate we're activating didn't actually have the feature `foo`. MissingFeatures(String), + + // TODO: needs more info for errors maneges + // TODO: needs more info for back jumping + /// pub dep errore + PublicDependency, } impl ConflictReason { @@ -420,6 +425,8 @@ impl ConflictReason { } } +pub type ConflictMap = BTreeMap; + pub struct RcVecIter { vec: Rc>, rest: Range, diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index 5d87bc4d2d7..f14abf1821d 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -338,6 +338,7 @@ pub fn resolve_with_previous<'cfg>( &try_to_use, Some(ws.config()), warn, + false, // TODO: use "public and private dependencies" feature flag )?; resolved.register_used_patches(registry.patches()); if register_patches { diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index 48fc0a227f0..603edf14f9c 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -1,27 +1,28 @@ use std::borrow::Borrow; -use std::collections::{HashMap, HashSet}; +use std::collections::BTreeSet; use std::fmt; -use std::hash::Hash; -pub struct Graph { - nodes: HashMap>, +use im_rc; + +pub struct Graph { + nodes: im_rc::OrdMap>, } -impl Graph { +impl Graph { pub fn new() -> Graph { Graph { - nodes: HashMap::new(), + nodes: im_rc::OrdMap::new(), } } pub fn add(&mut self, node: N) { - self.nodes.entry(node).or_insert_with(HashMap::new); + self.nodes.entry(node).or_insert_with(im_rc::OrdMap::new); } pub fn link(&mut self, node: N, child: N) -> &mut E { self.nodes .entry(node) - .or_insert_with(HashMap::new) + .or_insert_with(im_rc::OrdMap::new) .entry(child) .or_insert_with(Default::default) } @@ -29,7 +30,7 @@ impl Graph { pub fn contains(&self, k: &Q) -> bool where N: Borrow, - Q: Hash + Eq, + Q: Ord + Eq, { self.nodes.contains_key(k) } @@ -38,14 +39,14 @@ impl Graph { self.nodes.get(from)?.get(to) } - pub fn edges(&self, from: &N) -> impl Iterator { + pub fn edges(&self, from: &N) -> impl Iterator { self.nodes.get(from).into_iter().flat_map(|x| x.iter()) } /// A topological sort of the `Graph` pub fn sort(&self) -> Vec { let mut ret = Vec::new(); - let mut marks = HashSet::new(); + let mut marks = BTreeSet::new(); for node in self.nodes.keys() { self.sort_inner_visit(node, &mut ret, &mut marks); @@ -54,7 +55,7 @@ impl Graph { ret } - fn sort_inner_visit(&self, node: &N, dst: &mut Vec, marks: &mut HashSet) { + fn sort_inner_visit(&self, node: &N, dst: &mut Vec, marks: &mut BTreeSet) { if !marks.insert(node.clone()) { return; } @@ -70,6 +71,23 @@ impl Graph { self.nodes.keys() } + /// Resolves one of the paths from the given dependent package down to + /// a leaf. + pub fn path_to_bottom<'a>(&'a self, mut pkg: &'a N) -> Vec<&'a N> { + let mut result = vec![pkg]; + while let Some(p) = self.nodes.get(pkg).and_then(|p| { + p.iter() + // Note that we can have "cycles" introduced through dev-dependency + // edges, so make sure we don't loop infinitely. + .find(|&(node, _)| !result.contains(&node)) + .map(|(ref p, _)| p) + }) { + result.push(p); + pkg = p; + } + result + } + /// Resolves one of the paths from the given dependent package up to /// the root. pub fn path_to_top<'a>(&'a self, mut pkg: &'a N) -> Vec<&'a N> { @@ -84,7 +102,7 @@ impl Graph { // Note that we can have "cycles" introduced through dev-dependency // edges, so make sure we don't loop infinitely. .find(|&(node, _)| !res.contains(&node)) - .map(|p| p.0) + .map(|(ref p, _)| p) }; while let Some(p) = first_pkg_depending_on(pkg, &result) { result.push(p); @@ -94,13 +112,13 @@ impl Graph { } } -impl Default for Graph { +impl Default for Graph { fn default() -> Graph { Graph::new() } } -impl fmt::Debug for Graph { +impl fmt::Debug for Graph { fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { writeln!(fmt, "Graph {{")?; @@ -118,14 +136,14 @@ impl fmt::Debug for Graph { } } -impl PartialEq for Graph { +impl PartialEq for Graph { fn eq(&self, other: &Graph) -> bool { self.nodes.eq(&other.nodes) } } -impl Eq for Graph {} +impl Eq for Graph {} -impl Clone for Graph { +impl Clone for Graph { fn clone(&self) -> Graph { Graph { nodes: self.nodes.clone(), diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index 84aa81304a5..33c25e3fe54 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -7,8 +7,8 @@ use cargo::util::Config; use crate::support::project; use crate::support::registry::Package; use crate::support::resolver::{ - assert_contains, assert_same, dep, dep_kind, dep_loc, dep_req, loc_names, names, pkg, pkg_dep, - pkg_id, pkg_loc, registry, registry_strategy, remove_dep, resolve, resolve_and_validated, + assert_contains, assert_same, dep, dep_kind, dep_loc, dep_req, dep_req_kind, loc_names, names, + pkg, pkg_dep, pkg_id, pkg_loc, registry, registry_strategy, remove_dep, resolve, resolve_and_validated, resolve_with_config, PrettyPrintRegistry, ToDep, ToPkgId, }; @@ -235,6 +235,136 @@ proptest! { } } +#[test] +fn basic_public_dependency() { + let reg = registry(vec![ + pkg!(("A", "0.1.0")), + pkg!(("A", "0.2.0")), + pkg!("B" => [dep_req_kind("A", "0.1", Kind::Normal, true)]), + pkg!("C" => [dep("A"), dep("B")]), + ]); + + let res = resolve_and_validated(&pkg_id("root"), vec![dep("C")], ®).unwrap(); + assert_same( + &res, + &names(&[ + ("root", "1.0.0"), + ("C", "1.0.0"), + ("B", "1.0.0"), + ("A", "0.1.0"), + ]), + ); +} + +#[test] +fn public_dependency_filling_in() { + // The resolver has an optimization where if a candidate to resolve a dependency + // has already bean activated then we skip looking at the candidates dependencies. + // However, we have to be careful as the new path may make pub dependencies invalid. + + // Triggering this case requires dependencies to be resolved in a specific order. + // Fuzzing found this unintuitive case, that triggers this unfortunate order of operations: + // 1. `d`'s dep on `c` is resolved + // 2. `d`'s dep on `a` is resolved with `0.1.1` + // 3. `c`'s dep on `b` is resolved with `0.0.2` + // 4. `b`'s dep on `a` is resolved with `0.0.6` no pub dev conflict as `b` is private to `c` + // 5. `d`'s dep on `b` is resolved with `0.0.2` triggering the optimization. + // Do we notice that `d` has a pub dep conflict on `a`? Lets try it and see. + let reg = registry(vec![ + pkg!(("a", "0.0.6")), + pkg!(("a", "0.1.1")), + pkg!(("b", "0.0.0") => [dep("bad")]), + pkg!(("b", "0.0.1") => [dep("bad")]), + pkg!(("b", "0.0.2") => [dep_req_kind("a", "=0.0.6", Kind::Normal, true)]), + pkg!("c" => [dep_req("b", ">=0.0.1")]), + pkg!("d" => [dep("c"), dep("a"), dep("b")]), + ]); + + let res = resolve_and_validated(&pkg_id("root"), vec![dep("d")], ®).unwrap(); + assert_same( + &res, + &names(&[ + ("root", "1.0.0"), + ("d", "1.0.0"), + ("c", "1.0.0"), + ("b", "0.0.2"), + ("a", "0.0.6"), + ]), + ); +} + +#[test] +fn public_dependency_filling_in_and_update() { + // The resolver has an optimization where if a candidate to resolve a dependency + // has already bean activated then we skip looking at the candidates dependencies. + // However, we have to be careful as the new path may make pub dependencies invalid. + + // Triggering this case requires dependencies to be resolved in a specific order. + // Fuzzing found this unintuitive case, that triggers this unfortunate order of operations: + // 1. `D`'s dep on `B` is resolved + // 2. `D`'s dep on `C` is resolved + // 3. `B`'s dep on `A` is resolved with `0.0.0` + // 4. `C`'s dep on `B` triggering the optimization. + // So did we add `A 0.0.0` to the deps `C` can see? + // Or are we going to resolve `C`'s dep on `A` with `0.0.2`? + // Lets try it and see. + let reg = registry(vec![ + pkg!(("A", "0.0.0")), + pkg!(("A", "0.0.2")), + pkg!("B" => [dep_req_kind("A", "=0.0.0", Kind::Normal, true),]), + pkg!("C" => [dep("A"),dep("B")]), + pkg!("D" => [dep("B"),dep("C")]), + ]); + let res = resolve_and_validated(&pkg_id("root"), vec![dep("D")], ®).unwrap(); + assert_same( + &res, + &names(&[ + ("root", "1.0.0"), + ("D", "1.0.0"), + ("C", "1.0.0"), + ("B", "1.0.0"), + ("A", "0.0.0"), + ]), + ); +} + +#[test] +fn public_dependency_skiping() { + // When backtracking due to a failed dependency, if Cargo is + // trying to be clever and skip irrelevant dependencies, care must + // the effects of pub dep must be accounted for. + let input = vec![ + pkg!(("a", "0.2.0")), + pkg!(("a", "2.0.0")), + pkg!(("b", "0.0.0") => [dep("bad")]), + pkg!(("b", "0.2.1") => [dep_req_kind("a", "0.2.0", Kind::Normal, true)]), + pkg!("c" => [dep("a"),dep("b")]), + ]; + let reg = registry(input.clone()); + + resolve(&pkg_id("root"), vec![dep("c")], ®).unwrap(); +} + +#[test] +fn public_dependency_skiping_in_backtracking() { + // When backtracking due to a failed dependency, if Cargo is + // trying to be clever and skip irrelevant dependencies, care must + // the effects of pub dep must be accounted for. + let input = vec![ + pkg!(("A", "0.0.0") => [dep("bad")]), + pkg!(("A", "0.0.1") => [dep("bad")]), + pkg!(("A", "0.0.2") => [dep("bad")]), + pkg!(("A", "0.0.3") => [dep("bad")]), + pkg!(("A", "0.0.4")), + pkg!(("A", "0.0.5")), + pkg!("B" => [dep_req_kind("A", ">= 0.0.3", Kind::Normal, true)]), + pkg!("C" => [dep_req("A", "<= 0.0.4"), dep("B")]), + ]; + let reg = registry(input.clone()); + + resolve(&pkg_id("root"), vec![dep("C")], ®).unwrap(); +} + #[test] #[should_panic(expected = "assertion failed: !name.is_empty()")] fn test_dependency_with_empty_name() { diff --git a/tests/testsuite/support/resolver.rs b/tests/testsuite/support/resolver.rs index 1db28a16953..bd2fb75b622 100644 --- a/tests/testsuite/support/resolver.rs +++ b/tests/testsuite/support/resolver.rs @@ -115,6 +115,7 @@ pub fn resolve_with_config_raw( &HashSet::new(), config, false, + true, ); // The largest test in our suite takes less then 30 sec. @@ -250,9 +251,10 @@ pub fn dep(name: &str) -> Dependency { pub fn dep_req(name: &str, req: &str) -> Dependency { Dependency::parse_no_deprecated(name, Some(req), registry_loc()).unwrap() } -pub fn dep_req_kind(name: &str, req: &str, kind: Kind) -> Dependency { +pub fn dep_req_kind(name: &str, req: &str, kind: Kind, public: bool) -> Dependency { let mut dep = dep_req(name, req); dep.set_kind(kind); + dep.set_public(public); dep } @@ -297,9 +299,12 @@ impl fmt::Debug for PrettyPrintRegistry { } else { write!(f, "pkg!((\"{}\", \"{}\") => [", s.name(), s.version())?; for d in s.dependencies() { - if d.kind() == Kind::Normal && &d.version_req().to_string() == "*" { + if d.kind() == Kind::Normal + && &d.version_req().to_string() == "*" + && !d.is_public() + { write!(f, "dep(\"{}\"),", d.name_in_toml())?; - } else if d.kind() == Kind::Normal { + } else if d.kind() == Kind::Normal && !d.is_public() { write!( f, "dep_req(\"{}\", \"{}\"),", @@ -309,14 +314,15 @@ impl fmt::Debug for PrettyPrintRegistry { } else { write!( f, - "dep_req_kind(\"{}\", \"{}\", {}),", + "dep_req_kind(\"{}\", \"{}\", {}, {}),", d.name_in_toml(), d.version_req(), match d.kind() { Kind::Development => "Kind::Development", Kind::Build => "Kind::Build", Kind::Normal => "Kind::Normal", - } + }, + d.is_public() )?; } } @@ -341,8 +347,10 @@ fn meta_test_deep_pretty_print_registry() { pkg!(("bar", "2.0.0") => [dep_req("baz", "=1.0.1")]), pkg!(("baz", "1.0.2") => [dep_req("other", "2")]), pkg!(("baz", "1.0.1")), - pkg!(("cat", "1.0.2") => [dep_req_kind("other", "2", Kind::Build)]), - pkg!(("cat", "1.0.2") => [dep_req_kind("other", "2", Kind::Development)]), + pkg!(("cat", "1.0.2") => [dep_req_kind("other", "2", Kind::Build, false)]), + pkg!(("cat", "1.0.3") => [dep_req_kind("other", "2", Kind::Development, false)]), + pkg!(("cat", "1.0.4") => [dep_req_kind("other", "2", Kind::Build, true)]), + pkg!(("cat", "1.0.5") => [dep_req_kind("other", "2", Kind::Development, true)]), pkg!(("dep_req", "1.0.0")), pkg!(("dep_req", "2.0.0")), ]) @@ -354,8 +362,10 @@ fn meta_test_deep_pretty_print_registry() { pkg!((\"bar\", \"2.0.0\") => [dep_req(\"baz\", \"= 1.0.1\"),]),\ pkg!((\"baz\", \"1.0.2\") => [dep_req(\"other\", \"^2\"),]),\ pkg!((\"baz\", \"1.0.1\")),\ - pkg!((\"cat\", \"1.0.2\") => [dep_req_kind(\"other\", \"^2\", Kind::Build),]),\ - pkg!((\"cat\", \"1.0.2\") => [dep_req_kind(\"other\", \"^2\", Kind::Development),]),\ + pkg!((\"cat\", \"1.0.2\") => [dep_req_kind(\"other\", \"^2\", Kind::Build, false),]),\ + pkg!((\"cat\", \"1.0.3\") => [dep_req_kind(\"other\", \"^2\", Kind::Development, false),]),\ + pkg!((\"cat\", \"1.0.4\") => [dep_req_kind(\"other\", \"^2\", Kind::Build, true),]),\ + pkg!((\"cat\", \"1.0.5\") => [dep_req_kind(\"other\", \"^2\", Kind::Development, true),]),\ pkg!((\"dep_req\", \"1.0.0\")),\ pkg!((\"dep_req\", \"2.0.0\")),]" ) @@ -405,7 +415,14 @@ pub fn registry_strategy( let max_deps = max_versions * (max_crates * (max_crates - 1)) / shrinkage; let raw_version_range = (any::(), any::()); - let raw_dependency = (any::(), any::(), raw_version_range, 0..=1); + let raw_dependency = ( + any::(), + any::(), + raw_version_range, + 0..=1, + Just(false), + // TODO: ^ this needs to be set back to `any::()` and work before public & private dependencies can stabilize + ); fn order_index(a: Index, b: Index, size: usize) -> (usize, usize) { let (a, b) = (a.index(size), b.index(size)); @@ -432,7 +449,7 @@ pub fn registry_strategy( .collect(); let len_all_pkgid = list_of_pkgid.len(); let mut dependency_by_pkgid = vec![vec![]; len_all_pkgid]; - for (a, b, (c, d), k) in raw_dependencies { + for (a, b, (c, d), k, p) in raw_dependencies { let (a, b) = order_index(a, b, len_all_pkgid); let (a, b) = if reverse_alphabetical { (b, a) } else { (a, b) }; let ((dep_name, _), _) = list_of_pkgid[a]; @@ -459,9 +476,10 @@ pub fn registry_strategy( match k { 0 => Kind::Normal, 1 => Kind::Build, - // => Kind::Development, // Development has not impact so don't gen + // => Kind::Development, // Development has no impact so don't gen _ => panic!("bad index for Kind"), }, + p, )) }