From 444df0c8479df56176c05ca08beb97762f06bb4c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 22 Nov 2023 15:20:10 -0600 Subject: [PATCH] fix(resolver): Remove unused public-deps error handling To implement rust-lang/rfcs#3516, we need to decouple the resolver's behavior from the unstable flag. Since the code path is now dead, I went ahead and removed it. --- src/cargo/core/resolver/context.rs | 205 +---------------------------- src/cargo/core/resolver/mod.rs | 58 +------- src/cargo/ops/resolve.rs | 4 - 3 files changed, 8 insertions(+), 259 deletions(-) diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs index 09b16b39cd99..cfeea209ae13 100644 --- a/src/cargo/core/resolver/context.rs +++ b/src/cargo/core/resolver/context.rs @@ -22,9 +22,6 @@ pub struct Context { 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 whether the name is public. - pub public_dependency: Option, /// a way to look up for a package in activations what packages required it /// and all of the exact deps that it fulfilled. @@ -74,16 +71,11 @@ impl PackageId { } impl Context { - pub fn new(check_public_visible_dependencies: bool) -> Context { + pub fn new() -> Context { Context { age: 0, resolve_features: im_rc::HashMap::new(), links: im_rc::HashMap::new(), - public_dependency: if check_public_visible_dependencies { - Some(PublicDependency::new()) - } else { - None - }, parents: Graph::new(), activations: im_rc::HashMap::new(), } @@ -192,42 +184,6 @@ impl Context { .and_then(|(s, l)| if s.package_id() == id { Some(*l) } else { None }) } - /// If the conflict reason on the package still applies returns the `ContextAge` when it was added - pub fn still_applies(&self, id: PackageId, reason: &ConflictReason) -> Option { - self.is_active(id).and_then(|mut max| { - match reason { - ConflictReason::PublicDependency(name) => { - if &id == name { - return Some(max); - } - max = std::cmp::max(max, self.is_active(*name)?); - max = std::cmp::max( - max, - self.public_dependency - .as_ref() - .unwrap() - .can_see_item(*name, id)?, - ); - } - ConflictReason::PubliclyExports(name) => { - if &id == name { - return Some(max); - } - max = std::cmp::max(max, self.is_active(*name)?); - max = std::cmp::max( - max, - self.public_dependency - .as_ref() - .unwrap() - .publicly_exports_item(*name, id)?, - ); - } - _ => {} - } - Some(max) - }) - } - /// Checks whether all of `parent` and the keys of `conflicting activations` /// are still active. /// If so returns the `ContextAge` when the newest one was added. @@ -241,8 +197,8 @@ impl Context { max = std::cmp::max(max, self.is_active(parent)?); } - for (id, reason) in conflicting_activations.iter() { - max = std::cmp::max(max, self.still_applies(*id, reason)?); + for id in conflicting_activations.keys() { + max = std::cmp::max(max, self.is_active(*id)?); } Some(max) } @@ -280,158 +236,3 @@ impl Graph> { .map(|(grand, d)| (*grand, d.iter().any(|x| x.is_public()))) } } - -#[derive(Clone, Debug, Default)] -pub struct PublicDependency { - /// For each active package the set of all the names it can see, - /// for each name the exact package that name resolves to, - /// the `ContextAge` when it was first visible, - /// and the `ContextAge` when it was first exported. - inner: im_rc::HashMap< - PackageId, - im_rc::HashMap)>, - >, -} - -impl PublicDependency { - fn new() -> Self { - PublicDependency { - inner: im_rc::HashMap::new(), - } - } - fn publicly_exports(&self, candidate_pid: PackageId) -> Vec { - self.inner - .get(&candidate_pid) // if we have seen it before - .iter() - .flat_map(|x| x.values()) // all the things we have stored - .filter(|x| x.2.is_some()) // as publicly exported - .map(|x| x.0) - .chain(Some(candidate_pid)) // but even if not we know that everything exports itself - .collect() - } - fn publicly_exports_item( - &self, - candidate_pid: PackageId, - target: PackageId, - ) -> Option { - debug_assert_ne!(candidate_pid, target); - let out = self - .inner - .get(&candidate_pid) - .and_then(|names| names.get(&target.name())) - .filter(|(p, _, _)| *p == target) - .and_then(|(_, _, age)| *age); - debug_assert_eq!( - out.is_some(), - self.publicly_exports(candidate_pid).contains(&target) - ); - out - } - pub fn can_see_item(&self, candidate_pid: PackageId, target: PackageId) -> Option { - self.inner - .get(&candidate_pid) - .and_then(|names| names.get(&target.name())) - .filter(|(p, _, _)| *p == target) - .map(|(_, age, _)| *age) - } - pub fn add_edge( - &mut self, - candidate_pid: PackageId, - parent_pid: PackageId, - is_public: bool, - age: ContextAge, - parents: &Graph>, - ) { - // 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 - // publicly exported dependencies. - for c in self.publicly_exports(candidate_pid) { - // for each (transitive) parent that can newly see `t` - let mut stack = vec![(parent_pid, is_public)]; - while let Some((p, public)) = stack.pop() { - match self.inner.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().2.is_some() { - // 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 - let old_age = o.get().1; - o.insert((c, old_age, if public { Some(age) } else { None })); - } - } - im_rc::hashmap::Entry::Vacant(v) => { - // The (transitive) parent does not have anything by `c`s name, - // so we add `c`. - v.insert((c, age, if public { Some(age) } else { None })); - } - } - // 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 - stack.extend(parents.parents_of(p)); - } - } - } - } - pub fn can_add_edge( - &self, - b_id: PackageId, - parent: PackageId, - is_public: bool, - parents: &Graph>, - ) -> Result< - (), - ( - ((PackageId, ConflictReason), (PackageId, ConflictReason)), - Option<(PackageId, ConflictReason)>, - ), - > { - // one tricky part is that `candidate_pid` may already be active and - // have public dependencies of its own. So we not only need to check - // `b_id` as visible to its parents but also all of its existing - // publicly exported dependencies. - for t in self.publicly_exports(b_id) { - // for each (transitive) parent that can newly see `t` - let mut stack = vec![(parent, is_public)]; - while let Some((p, public)) = stack.pop() { - // TODO: don't look at the same thing more than once - if let Some(o) = self.inner.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`. - return Err(( - (o.0, ConflictReason::PublicDependency(p)), // p can see the other version and - (parent, ConflictReason::PublicDependency(p)), // p can see us - )) - .map_err(|e| { - if t == b_id { - (e, None) - } else { - (e, Some((t, ConflictReason::PubliclyExports(b_id)))) - } - }); - } - if o.2.is_some() { - // The previous time the parent saw `t`, it was a public dependency. - // So all of its parents already know about `t` - // and we can save some time by stopping now. - continue; - } - } - // 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 - stack.extend(parents.parents_of(p)); - } - } - } - Ok(()) - } -} diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index ecb6f36e6124..4b12f2cf32cc 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -120,24 +120,12 @@ mod version_prefs; /// /// * `config` - a location to print warnings and such, or `None` if no warnings /// should be printed -/// -/// * `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, ResolveOpts)], replacements: &[(PackageIdSpec, Dependency)], registry: &mut dyn Registry, version_prefs: &VersionPreferences, config: Option<&Config>, - check_public_visible_dependencies: bool, ) -> CargoResult { let _p = profile::start("resolving"); let first_version = match config { @@ -148,7 +136,7 @@ pub fn resolve( }; let mut registry = RegistryQueryer::new(registry, replacements, version_prefs); let cx = loop { - let cx = Context::new(check_public_visible_dependencies); + let cx = Context::new(); let cx = activate_deps_loop(cx, &mut registry, summaries, first_version, config)?; if registry.reset_pending() { break cx; @@ -286,12 +274,7 @@ fn activate_deps_loop( let mut backtracked = false; loop { - let next = remaining_candidates.next( - &mut conflicting_activations, - &cx, - &dep, - parent.package_id(), - ); + let next = remaining_candidates.next(&mut conflicting_activations, &cx); let (candidate, has_another) = next.ok_or(()).or_else(|_| { // If we get here then our `remaining_candidates` was just @@ -649,15 +632,6 @@ fn activate( .link(candidate_pid, parent_pid) // and associate dep with that edge .insert(dep.clone()); - if let Some(public_dependency) = cx.public_dependency.as_mut() { - public_dependency.add_edge( - candidate_pid, - parent_pid, - dep.is_public(), - cx.age, - &cx.parents, - ); - } } let activated = cx.flag_activated(&candidate, opts, parent)?; @@ -772,8 +746,6 @@ impl RemainingCandidates { &mut self, conflicting_prev_active: &mut ConflictMap, cx: &Context, - dep: &Dependency, - parent: PackageId, ) -> Option<(Summary, bool)> { for b in self.remaining.by_ref() { let b_id = b.package_id(); @@ -808,23 +780,6 @@ 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() { - if let Err(((c1, c2), c3)) = - public_dependency.can_add_edge(b_id, parent, dep.is_public(), &cx.parents) - { - conflicting_prev_active.insert(c1.0, c1.1); - conflicting_prev_active.insert(c2.0, c2.1); - if let Some(c3) = c3 { - conflicting_prev_active.insert(c3.0, c3.1); - } - continue; - } - } // 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 @@ -1001,12 +956,9 @@ fn find_candidate( }; 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 next = frame + .remaining_candidates + .next(&mut frame.conflicting_activations, &frame.context); let Some((candidate, has_another)) = next else { continue; }; diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index 00d3b1144500..c3ae6b2def84 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -64,7 +64,6 @@ use crate::core::resolver::{ self, HasDevUnits, Resolve, ResolveOpts, ResolveVersion, VersionOrdering, VersionPreferences, }; use crate::core::summary::Summary; -use crate::core::Feature; use crate::core::{GitReference, PackageId, PackageIdSpec, PackageSet, SourceId, Workspace}; use crate::ops; use crate::sources::PathSource; @@ -512,9 +511,6 @@ pub fn resolve_with_previous<'cfg>( registry, &version_prefs, Some(ws.config()), - ws.unstable_features() - .require(Feature::public_dependency()) - .is_ok(), )?; let patches: Vec<_> = registry .patches()