Skip to content

Commit ac8c6ab

Browse files
committed
Auto merge of #14089 - Eh2406:check_cycles, r=weihanglo
Simplify checking for dependency cycles ### What does this PR try to resolve? In my PubGrub work I had to try to understand the `check_cycles` post resolution pass. I found the code tricky to understand and doing some unnecessary reallocations. I cleaned up the code to remove the unnecessary data structures so that I could better understand. ### How should we test and review this PR? It's an internal re-factor, and the tests still pass. But this code is not extensively tested by our test suite. In addition I ran my "resolve every crate on crates.io" script against a local check out of this branch. With the old code as `check_cycles_old` and the PR code as `check_cycles_new` the following assertion did not hit: ```rust fn check_cycles(resolve: &Resolve) -> CargoResult<()> { let old = check_cycles_old(resolve); let new = check_cycles_new(resolve); assert_eq!(old.is_err(), new.is_err()); old } ``` That experiment also demonstrated that this was not a significant performance change in either direction. ### Additional information If you're having questions about this code while you're reviewing, this would be a perfect opportunity for better naming and comments. Please ask questions.
2 parents 103e675 + 2b4273d commit ac8c6ab

File tree

3 files changed

+47
-38
lines changed

3 files changed

+47
-38
lines changed

src/cargo/core/resolver/mod.rs

Lines changed: 26 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1000,69 +1000,57 @@ fn find_candidate(
10001000
}
10011001

10021002
fn check_cycles(resolve: &Resolve) -> CargoResult<()> {
1003-
// Create a simple graph representation alternative of `resolve` which has
1004-
// only the edges we care about. Note that `BTree*` is used to produce
1005-
// deterministic error messages here. Also note that the main reason for
1006-
// this copy of the resolve graph is to avoid edges between a crate and its
1007-
// dev-dependency since that doesn't count for cycles.
1008-
let mut graph = BTreeMap::new();
1009-
for id in resolve.iter() {
1010-
let map = graph.entry(id).or_insert_with(BTreeMap::new);
1011-
for (dep_id, listings) in resolve.deps_not_replaced(id) {
1012-
let transitive_dep = listings.iter().find(|d| d.is_transitive());
1013-
1014-
if let Some(transitive_dep) = transitive_dep.cloned() {
1015-
map.insert(dep_id, transitive_dep.clone());
1016-
resolve
1017-
.replacement(dep_id)
1018-
.map(|p| map.insert(p, transitive_dep));
1019-
}
1020-
}
1021-
}
1022-
1023-
// After we have the `graph` that we care about, perform a simple cycle
1024-
// check by visiting all nodes. We visit each node at most once and we keep
1003+
// Perform a simple cycle check by visiting all nodes.
1004+
// We visit each node at most once and we keep
10251005
// track of the path through the graph as we walk it. If we walk onto the
10261006
// same node twice that's a cycle.
1027-
let mut checked = HashSet::new();
1028-
let mut path = Vec::new();
1029-
let mut visited = HashSet::new();
1030-
for pkg in graph.keys() {
1031-
if !checked.contains(pkg) {
1032-
visit(&graph, *pkg, &mut visited, &mut path, &mut checked)?
1007+
let mut checked = HashSet::with_capacity(resolve.len());
1008+
let mut path = Vec::with_capacity(4);
1009+
let mut visited = HashSet::with_capacity(4);
1010+
for pkg in resolve.iter() {
1011+
if !checked.contains(&pkg) {
1012+
visit(&resolve, pkg, &mut visited, &mut path, &mut checked)?
10331013
}
10341014
}
10351015
return Ok(());
10361016

10371017
fn visit(
1038-
graph: &BTreeMap<PackageId, BTreeMap<PackageId, Dependency>>,
1018+
resolve: &Resolve,
10391019
id: PackageId,
10401020
visited: &mut HashSet<PackageId>,
10411021
path: &mut Vec<PackageId>,
10421022
checked: &mut HashSet<PackageId>,
10431023
) -> CargoResult<()> {
1044-
path.push(id);
10451024
if !visited.insert(id) {
1046-
let iter = path.iter().rev().skip(1).scan(id, |child, parent| {
1047-
let dep = graph.get(parent).and_then(|adjacent| adjacent.get(child));
1025+
// We found a cycle and need to construct an error. Performance is no longer top priority.
1026+
let iter = path.iter().rev().scan(id, |child, parent| {
1027+
let dep = resolve.transitive_deps_not_replaced(*parent).find_map(
1028+
|(dep_id, transitive_dep)| {
1029+
(*child == dep_id || Some(*child) == resolve.replacement(dep_id))
1030+
.then_some(transitive_dep)
1031+
},
1032+
);
10481033
*child = *parent;
10491034
Some((parent, dep))
10501035
});
10511036
let iter = std::iter::once((&id, None)).chain(iter);
1037+
let describe_path = errors::describe_path(iter);
10521038
anyhow::bail!(
1053-
"cyclic package dependency: package `{}` depends on itself. Cycle:\n{}",
1054-
id,
1055-
errors::describe_path(iter),
1039+
"cyclic package dependency: package `{id}` depends on itself. Cycle:\n{describe_path}"
10561040
);
10571041
}
10581042

10591043
if checked.insert(id) {
1060-
for dep in graph[&id].keys() {
1061-
visit(graph, *dep, visited, path, checked)?;
1044+
path.push(id);
1045+
for (dep_id, _transitive_dep) in resolve.transitive_deps_not_replaced(id) {
1046+
visit(resolve, dep_id, visited, path, checked)?;
1047+
if let Some(replace_id) = resolve.replacement(dep_id) {
1048+
visit(resolve, replace_id, visited, path, checked)?;
1049+
}
10621050
}
1051+
path.pop();
10631052
}
10641053

1065-
path.pop();
10661054
visited.remove(&id);
10671055
Ok(())
10681056
}

src/cargo/core/resolver/resolve.rs

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -324,6 +324,10 @@ unable to verify that `{0}` is the same as when the lockfile was generated
324324
self.graph.iter().cloned()
325325
}
326326

327+
pub fn len(&self) -> usize {
328+
self.graph.len()
329+
}
330+
327331
pub fn deps(&self, pkg: PackageId) -> impl Iterator<Item = (PackageId, &HashSet<Dependency>)> {
328332
self.deps_not_replaced(pkg)
329333
.map(move |(id, deps)| (self.replacement(id).unwrap_or(id), deps))
@@ -336,6 +340,19 @@ unable to verify that `{0}` is the same as when the lockfile was generated
336340
self.graph.edges(&pkg).map(|(id, deps)| (*id, deps))
337341
}
338342

343+
// Only edges that are transitive, filtering out edges between a crate and its dev-dependency
344+
// since that doesn't count for cycles.
345+
pub fn transitive_deps_not_replaced(
346+
&self,
347+
pkg: PackageId,
348+
) -> impl Iterator<Item = (PackageId, &Dependency)> {
349+
self.graph.edges(&pkg).filter_map(|(id, deps)| {
350+
deps.iter()
351+
.find(|d| d.is_transitive())
352+
.map(|transitive_dep| (*id, transitive_dep))
353+
})
354+
}
355+
339356
pub fn replacement(&self, pkg: PackageId) -> Option<PackageId> {
340357
self.replacements.get(&pkg).cloned()
341358
}

src/cargo/util/graph.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,10 @@ impl<N: Eq + Ord + Clone, E: Default + Clone> Graph<N, E> {
6969
self.nodes.keys()
7070
}
7171

72+
pub fn len(&self) -> usize {
73+
self.nodes.len()
74+
}
75+
7276
/// Checks if there is a path from `from` to `to`.
7377
pub fn is_path_from_to<'a>(&'a self, from: &'a N, to: &'a N) -> bool {
7478
let mut stack = vec![from];

0 commit comments

Comments
 (0)