Skip to content

Enforce correctness of self-dependencies #9705

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.

Sign up for GitHub

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
charliermarsh merged 3 commits into from
Dec 8, 2024
Merged

Enforce correctness of self-dependencies #9705

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3f29997
Self deps
charliermarsh Dec 7, 2024
f9d9341
Reverts
charliermarsh Dec 7, 2024
b1fdfc8
Add hints
charliermarsh Dec 7, 2024
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
50 changes: 15 additions & 35 deletions crates/uv-resolver/src/pubgrub/dependencies.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ use std::iter;

use either::Either;
use pubgrub::Ranges;
use tracing::warn;

use uv_normalize::{ExtraName, GroupName, PackageName};
use uv_pep440::{Version, VersionSpecifiers};
Expand Down Expand Up @@ -91,42 +90,23 @@ impl PubGrubDependency {

// Add the package, plus any extra variants.
iter.map(|(extra, group)| PubGrubRequirement::from_requirement(requirement, extra, group))
.filter_map(move |requirement| {
.map(move |requirement| {
let PubGrubRequirement {
package,
version,
url,
} = requirement;
match &*package {
PubGrubPackageInner::Package { name, .. } => {
// Detect self-dependencies.
if dev.is_none() {
if source_name.is_some_and(|source_name| source_name == name) {
warn!("{name} has a dependency on itself");
return None;
}
}

Some(PubGrubDependency {
package: package.clone(),
version: version.clone(),
url,
})
}
PubGrubPackageInner::Marker { name, .. } => {
// Detect self-dependencies.
if dev.is_none() {
if source_name.is_some_and(|source_name| source_name == name) {
return None;
}
}

Some(PubGrubDependency {
package: package.clone(),
version: version.clone(),
url,
})
}
PubGrubPackageInner::Package { .. } => PubGrubDependency {
package: package.clone(),
version: version.clone(),
url,
},
PubGrubPackageInner::Marker { .. } => PubGrubDependency {
package: package.clone(),
version: version.clone(),
url,
},
PubGrubPackageInner::Extra { name, .. } => {
// Detect self-dependencies.
if dev.is_none() {
Expand All @@ -135,11 +115,11 @@ impl PubGrubDependency {
"extras not flattened for {name}"
);
}
Some(PubGrubDependency {
PubGrubDependency {
package: package.clone(),
version: version.clone(),
url,
})
}
}
PubGrubPackageInner::Dev { name, .. } => {
// Detect self-dependencies.
Expand All @@ -149,11 +129,11 @@ impl PubGrubDependency {
"group not flattened for {name}"
);
}
Some(PubGrubDependency {
PubGrubDependency {
package: package.clone(),
version: version.clone(),
url,
})
}
}
PubGrubPackageInner::Root(_) => unreachable!("root package in dependencies"),
PubGrubPackageInner::Python(_) => {
Expand Down
65 changes: 65 additions & 0 deletions crates/uv-resolver/src/pubgrub/report.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,16 @@ impl ReportFormatter<PubGrubPackage, Range<Version>, UnavailableReason>
External::FromDependencyOf(package, package_set, dependency, dependency_set) => {
let package_set = self.simplify_set(package_set, package);
let dependency_set = self.simplify_set(dependency_set, dependency);

if package == dependency {
if let Some(member) = self.format_workspace_member(package) {
return format!(
"{member} depends on itself at an incompatible version ({})",
PackageRange::dependency(package, &dependency_set, None)
);
}
}

if let Some(root) = self.format_root_requires(package) {
return format!(
"{root} {}",
Expand Down Expand Up @@ -407,6 +417,24 @@ impl PubGrubReportFormatter<'_> {
}
}

/// Return whether the given package is the root package.
fn is_root(package: &PubGrubPackage) -> bool {
matches!(&**package, PubGrubPackageInner::Root(_))
}

/// Return whether the given package is a workspace member.
fn is_single_project_workspace_member(&self, package: &PubGrubPackage) -> bool {
match &**package {
// TODO(zanieb): Improve handling of dev and extra for single-project workspaces
PubGrubPackageInner::Package {
name, extra, dev, ..
} if self.workspace_members.contains(name) => {
self.is_single_project_workspace() && extra.is_none() && dev.is_none()
}
_ => false,
}
}

/// Create a [`PackageRange::compatibility`] display with this formatter attached.
fn compatible_range<'a>(
&'a self,
Expand Down Expand Up @@ -467,6 +495,18 @@ impl PubGrubReportFormatter<'_> {
.and(dependency2.package, &dependency_set2),
)
}
(.., External::FromDependencyOf(package, _, dependency, _))
if Self::is_root(package)
&& self.is_single_project_workspace_member(dependency) =>
{
self.format_external(external1)
}
(External::FromDependencyOf(package, _, dependency, _), ..)
if Self::is_root(package)
&& self.is_single_project_workspace_member(dependency) =>
{
self.format_external(external2)
}
_ => {
let external1 = self.format_external(external1);
let external2 = self.format_external(external2);
Expand Down Expand Up @@ -570,6 +610,16 @@ impl PubGrubReportFormatter<'_> {
workspace: self.is_workspace() && !self.is_single_project_workspace(),
});
}

if package_name == dependency_name
&& (dependency.extra().is_none() || package.extra() == dependency.extra())
&& (dependency.dev().is_none() || dependency.dev() == package.dev())
&& workspace_members.contains(package_name)
{
output_hints.insert(PubGrubHint::DependsOnItself {
package: package.clone(),
});
}
}
// Check for no versions due to `Requires-Python`.
if matches!(
Expand Down Expand Up @@ -899,6 +949,8 @@ pub(crate) enum PubGrubHint {
dependency: PubGrubPackage,
workspace: bool,
},
/// A package depends on itself at an incompatible version.
DependsOnItself { package: PubGrubPackage },
/// A package was available on an index, but not at the correct version, and at least one
/// subsequent index was not queried. As such, a compatible version may be available on an
/// one of the remaining indexes.
Expand Down Expand Up @@ -963,6 +1015,9 @@ enum PubGrubHintCore {
dependency: PubGrubPackage,
workspace: bool,
},
DependsOnItself {
package: PubGrubPackage,
},
UncheckedIndex {
package: PubGrubPackage,
},
Expand Down Expand Up @@ -1027,6 +1082,7 @@ impl From<PubGrubHint> for PubGrubHintCore {
dependency,
workspace,
},
PubGrubHint::DependsOnItself { package } => Self::DependsOnItself { package },
PubGrubHint::UncheckedIndex { package, .. } => Self::UncheckedIndex { package },
PubGrubHint::UnauthorizedIndex { index } => Self::UnauthorizedIndex { index },
PubGrubHint::ForbiddenIndex { index } => Self::ForbiddenIndex { index },
Expand Down Expand Up @@ -1269,6 +1325,15 @@ impl std::fmt::Display for PubGrubHint {
dependency.cyan(),
)
}
Self::DependsOnItself { package } => {
write!(
f,
"{}{} The package `{}` depends on itself. This is likely a mistake. Consider removing the dependency.",
"hint".bold().cyan(),
":".bold(),
package.cyan(),
)
}
Self::UncheckedIndex {
package,
range,
Expand Down
41 changes: 37 additions & 4 deletions crates/uv-resolver/src/resolver/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2453,8 +2453,24 @@ impl ForkState {
extra: ref dependency_extra,
dev: ref dependency_dev,
marker: ref dependency_marker,
..
} => {
debug_assert!(
dependency_extra.is_none(),
"Packages should depend on an extra proxy"
);
debug_assert!(
dependency_dev.is_none(),
"Packages should depend on a group proxy"
);

// Ignore self-dependencies (e.g., `tensorflow-macos` depends on `tensorflow-macos`),
// but allow groups to depend on other groups, or on the package itself.
if self_dev.is_none() {
if self_name == Some(dependency_name) {
continue;
}
}

let to_url = self.fork_urls.get(dependency_name);
let to_index = self.fork_indexes.get(dependency_name);
let edge = ResolutionDependencyEdge {
Expand All @@ -2478,8 +2494,15 @@ impl ForkState {
PubGrubPackageInner::Marker {
name: ref dependency_name,
marker: ref dependency_marker,
..
} => {
// Ignore self-dependencies (e.g., `tensorflow-macos` depends on `tensorflow-macos`),
// but allow groups to depend on other groups, or on the package itself.
if self_dev.is_none() {
if self_name == Some(dependency_name) {
continue;
}
}

let to_url = self.fork_urls.get(dependency_name);
let to_index = self.fork_indexes.get(dependency_name);
let edge = ResolutionDependencyEdge {
Expand All @@ -2504,8 +2527,14 @@ impl ForkState {
name: ref dependency_name,
extra: ref dependency_extra,
marker: ref dependency_marker,
..
} => {
if self_dev.is_none() {
debug_assert!(
self_name != Some(dependency_name),
"Extras should be flattened"
);
}

// Insert an edge from the dependent package to the extra package.
let to_url = self.fork_urls.get(dependency_name);
let to_index = self.fork_indexes.get(dependency_name);
Expand Down Expand Up @@ -2551,8 +2580,12 @@ impl ForkState {
name: ref dependency_name,
dev: ref dependency_dev,
marker: ref dependency_marker,
..
} => {
debug_assert!(
self_name != Some(dependency_name),
"Groups should be flattened"
);

// Add an edge from the dependent package to the dev package, but _not_ the
// base package.
let to_url = self.fork_urls.get(dependency_name);
Expand Down
Loading