fix(resolver): Remove unused public-deps error handling

epage · epage · commit 444df0c8479d · 2023-11-22T15:20:10.000-06:00
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.
diff --git 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<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.
@@ -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<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.
@@ -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<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(())
-    }
-}
diff --git 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<Resolve> {
     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
@@ -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()
