Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify checking for dependency cycles #14089

Merged
merged 1 commit into from
Jun 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 26 additions & 38 deletions src/cargo/core/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1004,69 +1004,57 @@ fn find_candidate(
}

fn check_cycles(resolve: &Resolve) -> CargoResult<()> {
// Create a simple graph representation alternative of `resolve` which has
// only the edges we care about. Note that `BTree*` is used to produce
// deterministic error messages here. Also note that the main reason for
// this copy of the resolve graph is to avoid edges between a crate and its
// dev-dependency since that doesn't count for cycles.
let mut graph = BTreeMap::new();
for id in resolve.iter() {
let map = graph.entry(id).or_insert_with(BTreeMap::new);
for (dep_id, listings) in resolve.deps_not_replaced(id) {
let transitive_dep = listings.iter().find(|d| d.is_transitive());

if let Some(transitive_dep) = transitive_dep.cloned() {
map.insert(dep_id, transitive_dep.clone());
resolve
.replacement(dep_id)
.map(|p| map.insert(p, transitive_dep));
}
}
}

// After we have the `graph` that we care about, perform a simple cycle
// check by visiting all nodes. We visit each node at most once and we keep
// Perform a simple cycle check by visiting all nodes.
// We visit each node at most once and we keep
// track of the path through the graph as we walk it. If we walk onto the
// same node twice that's a cycle.
let mut checked = HashSet::new();
let mut path = Vec::new();
let mut visited = HashSet::new();
for pkg in graph.keys() {
if !checked.contains(pkg) {
visit(&graph, *pkg, &mut visited, &mut path, &mut checked)?
let mut checked = HashSet::with_capacity(resolve.len());
let mut path = Vec::with_capacity(4);
let mut visited = HashSet::with_capacity(4);
Comment on lines +1012 to +1013
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these initialized with a capacity of 4?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Random guess. Initializing them with a capacity of 0 (::new()) seems silly when we know were going to push an item to it. Initializing them to the largest possible length (::with_capacity(resolve.len());) prevents a reallocation ever being needed, but is excessive as dependency trees tend to be wide not deep. So this was a compromise value I came up with off the top of my head. In effect it's just moving the allocation from "the first time we push" up to here. Given that none of this PR seems to have affected perf, I'd be happy to change it to whatever seems clear. What would you prefer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with merge this as-is, as the rationale has been called out here.

for pkg in resolve.iter() {
if !checked.contains(&pkg) {
visit(&resolve, pkg, &mut visited, &mut path, &mut checked)?
}
}
return Ok(());

fn visit(
graph: &BTreeMap<PackageId, BTreeMap<PackageId, Dependency>>,
resolve: &Resolve,
id: PackageId,
visited: &mut HashSet<PackageId>,
path: &mut Vec<PackageId>,
checked: &mut HashSet<PackageId>,
) -> CargoResult<()> {
path.push(id);
if !visited.insert(id) {
let iter = path.iter().rev().skip(1).scan(id, |child, parent| {
let dep = graph.get(parent).and_then(|adjacent| adjacent.get(child));
// We found a cycle and need to construct an error. Performance is no longer top priority.
let iter = path.iter().rev().scan(id, |child, parent| {
let dep = resolve.transitive_deps_not_replaced(*parent).find_map(
|(dep_id, transitive_dep)| {
(*child == dep_id || Some(*child) == resolve.replacement(dep_id))
.then_some(transitive_dep)
},
);
*child = *parent;
Some((parent, dep))
});
let iter = std::iter::once((&id, None)).chain(iter);
let describe_path = errors::describe_path(iter);
anyhow::bail!(
"cyclic package dependency: package `{}` depends on itself. Cycle:\n{}",
id,
errors::describe_path(iter),
"cyclic package dependency: package `{id}` depends on itself. Cycle:\n{describe_path}"
);
}

if checked.insert(id) {
for dep in graph[&id].keys() {
visit(graph, *dep, visited, path, checked)?;
path.push(id);
for (dep_id, _transitive_dep) in resolve.transitive_deps_not_replaced(id) {
visit(resolve, dep_id, visited, path, checked)?;
if let Some(replace_id) = resolve.replacement(dep_id) {
visit(resolve, replace_id, visited, path, checked)?;
}
}
path.pop();
}

path.pop();
visited.remove(&id);
Ok(())
}
Expand Down
17 changes: 17 additions & 0 deletions src/cargo/core/resolver/resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,10 @@ unable to verify that `{0}` is the same as when the lockfile was generated
self.graph.iter().cloned()
}

pub fn len(&self) -> usize {
self.graph.len()
}

pub fn deps(&self, pkg: PackageId) -> impl Iterator<Item = (PackageId, &HashSet<Dependency>)> {
self.deps_not_replaced(pkg)
.map(move |(id, deps)| (self.replacement(id).unwrap_or(id), deps))
Expand All @@ -336,6 +340,19 @@ unable to verify that `{0}` is the same as when the lockfile was generated
self.graph.edges(&pkg).map(|(id, deps)| (*id, deps))
}

// Only edges that are transitive, filtering out edges between a crate and its dev-dependency
// since that doesn't count for cycles.
pub fn transitive_deps_not_replaced(
&self,
pkg: PackageId,
) -> impl Iterator<Item = (PackageId, &Dependency)> {
self.graph.edges(&pkg).filter_map(|(id, deps)| {
deps.iter()
.find(|d| d.is_transitive())
.map(|transitive_dep| (*id, transitive_dep))
})
}

pub fn replacement(&self, pkg: PackageId) -> Option<PackageId> {
self.replacements.get(&pkg).cloned()
}
Expand Down
4 changes: 4 additions & 0 deletions src/cargo/util/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,10 @@ impl<N: Eq + Ord + Clone, E: Default + Clone> Graph<N, E> {
self.nodes.keys()
}

pub fn len(&self) -> usize {
self.nodes.len()
}

/// Checks if there is a path from `from` to `to`.
pub fn is_path_from_to<'a>(&'a self, from: &'a N, to: &'a N) -> bool {
let mut stack = vec![from];
Expand Down