Skip to content

relax error checking around unconditional enabling of conflicting extras #10875

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

Merged
merged 5 commits into from
Jan 22, 2025
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
9 changes: 1 addition & 8 deletions crates/uv-resolver/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use tracing::trace;
use uv_distribution_types::{
DerivationChain, DistErrorKind, IndexCapabilities, IndexLocations, IndexUrl, RequestedDist,
};
use uv_normalize::{ExtraName, PackageName};
use uv_normalize::PackageName;
use uv_pep440::{LocalVersionSlice, Version};
use uv_platform_tags::Tags;
use uv_static::EnvVars;
Expand Down Expand Up @@ -48,13 +48,6 @@ pub enum ResolveError {
#[error("Attempted to wait on an unregistered task: `{_0}`")]
UnregisteredTask(String),

#[error("Found conflicting extra `{extra}` unconditionally enabled in `{requirement}`")]
ConflictingExtra {
// Boxed because `Requirement` is large.
requirement: Box<uv_pypi_types::Requirement>,
extra: ExtraName,
},

#[error(
"Requirements contain conflicting URLs for package `{package_name}`{}:\n- {}",
if env.marker_environment().is_some() {
Expand Down
40 changes: 40 additions & 0 deletions crates/uv-resolver/src/lock/installable.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
use std::borrow::Cow;
use std::collections::hash_map::Entry;
use std::collections::BTreeSet;
use std::collections::VecDeque;
use std::path::Path;

use either::Either;
use itertools::Itertools;
use petgraph::Graph;
use rustc_hash::{FxBuildHasher, FxHashMap, FxHashSet};

Expand Down Expand Up @@ -264,6 +267,8 @@ pub trait Installable<'lock> {
}
}

let mut all_activated_extras: BTreeSet<(&PackageName, &ExtraName)> =
activated_extras.iter().copied().collect();
while let Some((package, extra)) = queue.pop_front() {
let deps = if let Some(extra) = extra {
Either::Left(
Expand All @@ -277,6 +282,15 @@ pub trait Installable<'lock> {
Either::Right(package.dependencies.iter())
};
for dep in deps {
let mut activated_extras = Cow::Borrowed(&*activated_extras);
if !dep.extra.is_empty() {
let mut extended = activated_extras.to_vec();
for extra in &dep.extra {
extended.push((&dep.package_id.name, extra));
all_activated_extras.insert((&dep.package_id.name, extra));
}
activated_extras = Cow::Owned(extended);
}
if !dep.complexified_marker.evaluate(
marker_env,
&activated_extras,
Expand Down Expand Up @@ -329,6 +343,32 @@ pub trait Installable<'lock> {
}
}

// At time of writing, it's somewhat expected that the set of
// conflicting extras is pretty small. With that said, the
// time complexity of the following routine is pretty gross.
// Namely, `set.contains` is linear in the size of the set,
// iteration over all conflicts is also obviously linear in
// the number of conflicting sets and then for each of those,
// we visit every possible pair of activated extra from above,
// which is quadratic in the total number of extras enabled. I
// believe the simplest improvement here, if it's necessary, is
// to adjust the `Conflicts` internals to own these sorts of
// checks. ---AG
for set in self.lock().conflicts().iter() {
for ((pkg1, extra1), (pkg2, extra2)) in all_activated_extras.iter().tuple_combinations()
{
if set.contains(pkg1, *extra1) && set.contains(pkg2, *extra2) {
return Err(LockErrorKind::ConflictingExtra {
package1: (*pkg1).clone(),
extra1: (*extra1).clone(),
package2: (*pkg2).clone(),
extra2: (*extra2).clone(),
}
.into());
}
}
}

Ok(Resolution::new(petgraph))
}

Expand Down
10 changes: 10 additions & 0 deletions crates/uv-resolver/src/lock/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4903,6 +4903,16 @@ enum LockErrorKind {
#[source]
err: uv_distribution::Error,
},
#[error(
"Found conflicting extras `{package1}[{extra1}]` \
and `{package2}[{extra2}]` enabled simultaneously"
)]
ConflictingExtra {
package1: PackageName,
extra1: ExtraName,
package2: PackageName,
extra2: ExtraName,
},
}

/// An error that occurs when a source string could not be parsed.
Expand Down
40 changes: 0 additions & 40 deletions crates/uv-resolver/src/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1682,16 +1682,6 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
}
};

if let Some(err) = find_conflicting_extra(&self.conflicts, &metadata.requires_dist)
{
return Err(err);
}
for dependencies in metadata.dependency_groups.values() {
if let Some(err) = find_conflicting_extra(&self.conflicts, dependencies) {
return Err(err);
}
}

let requirements = self.flatten_requirements(
&metadata.requires_dist,
&metadata.dependency_groups,
Expand Down Expand Up @@ -3610,36 +3600,6 @@ pub(crate) struct VersionFork {
version: Option<Version>,
}

/// Returns an error if a conflicting extra is found in the given requirements.
///
/// Specifically, if there is any conflicting extra (just one is enough) that
/// is unconditionally enabled as part of a dependency specification, then this
/// returns an error.
///
/// The reason why we're so conservative here is because it avoids us needing
/// the look at the entire dependency tree at once.
///
/// For example, consider packages `root`, `a`, `b` and `c`, where `c` has
/// declared conflicting extras of `x1` and `x2`.
///
/// Now imagine `root` depends on `a` and `b`, `a` depends on `c[x1]` and `b`
/// depends on `c[x2]`. That's a conflict, but not easily detectable unless
/// you reject either `c[x1]` or `c[x2]` on the grounds that `x1` and `x2` are
/// conflicting and thus cannot be enabled unconditionally.
fn find_conflicting_extra(conflicting: &Conflicts, reqs: &[Requirement]) -> Option<ResolveError> {
for req in reqs {
for extra in &req.extras {
if conflicting.contains(&req.name, extra) {
return Some(ResolveError::ConflictingExtra {
requirement: Box::new(req.clone()),
extra: extra.clone(),
});
}
}
}
None
}

#[derive(Debug, Default, Clone)]
struct ConflictTracker {
/// How often a decision on the package was discarded due to another package decided earlier.
Expand Down
Loading
Loading