Skip to content

Commit

Permalink
fix(resolver): Remove unused public-deps error handling
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@epage
epage committed Nov 22, 2023
1 parent 71cd3a9 commit 444df0c
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 259 deletions.
205 changes: 3 additions & 202 deletions src/cargo/core/resolver/context.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,6 @@ pub struct Context {
pub resolve_features: im_rc::HashMap<PackageId, FeaturesSet>,
/// get the package that will be linking to a native library by its links attribute
pub links: im_rc::HashMap<InternedString, PackageId>,
/// 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<PublicDependency>,

/// a way to look up for a package in activations what packages required it
/// and all of the exact deps that it fulfilled.
Expand Down Expand Up @@ -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(),
}
Expand Down Expand Up @@ -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<ContextAge> {
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.
Expand All @@ -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)
}
Expand Down Expand Up @@ -280,158 +236,3 @@ impl Graph<PackageId, im_rc::HashSet<Dependency>> {
.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<InternedString, (PackageId, ContextAge, Option<ContextAge>)>,
>,
}

impl PublicDependency {
fn new() -> Self {
PublicDependency {
inner: im_rc::HashMap::new(),
}
}
fn publicly_exports(&self, candidate_pid: PackageId) -> Vec<PackageId> {
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<ContextAge> {
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<ContextAge> {
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<PackageId, im_rc::HashSet<Dependency>>,
) {
// 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<PackageId, im_rc::HashSet<Dependency>>,
) -> 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(())
}
}
58 changes: 5 additions & 53 deletions src/cargo/core/resolver/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Resolve> {
let _p = profile::start("resolving");
let first_version = match config {
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)?;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
};
Expand Down
4 changes: 0 additions & 4 deletions src/cargo/ops/resolve.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()
Expand Down

0 comments on commit 444df0c

Please sign in to comment.