From a129cf7d7ee6dc2dd534634df6f5bfd092253d61 Mon Sep 17 00:00:00 2001 From: konsti Date: Fri, 9 Aug 2024 19:55:17 +0200 Subject: [PATCH] Warn when there are missing bounds on transitive deps in lowest (#5953) Warn when there are missing bounds on transitive dependencies with `--resolution lowest`. Implemented as a lazy resolution graph check. Dev deps are odd because they are missing the edge from the root that extras have (they are currently orphans in the resolution graph), but this is more complex to solve properly because we can put dev dep information in a `Requirement` so i special cased them here. Closes #2797 Should help with #1718 --------- Co-authored-by: Ibraheem Ahmed --- crates/distribution-types/src/resolution.rs | 13 ++++ crates/pep440-rs/src/version_specifier.rs | 16 +++++ crates/pypi-types/src/requirement.rs | 11 +++ crates/uv-resolver/src/resolution/graph.rs | 77 ++++++++++++++++++++- crates/uv-resolver/src/resolver/mod.rs | 7 +- crates/uv/tests/lock.rs | 32 +++++++++ 6 files changed, 154 insertions(+), 2 deletions(-) diff --git a/crates/distribution-types/src/resolution.rs b/crates/distribution-types/src/resolution.rs index 9ff450ee8..064333559 100644 --- a/crates/distribution-types/src/resolution.rs +++ b/crates/distribution-types/src/resolution.rs @@ -92,6 +92,11 @@ pub enum ResolutionDiagnostic { /// The reason that the version was yanked, if any. reason: Option, }, + MissingLowerBound { + /// The name of the package that had no lower bound from any other package in the + /// resolution. For example, `black`. + package_name: PackageName, + }, } impl Diagnostic for ResolutionDiagnostic { @@ -111,6 +116,13 @@ impl Diagnostic for ResolutionDiagnostic { format!("`{dist}` is yanked") } } + Self::MissingLowerBound { package_name: name } => { + format!( + "The transitive dependency `{name}` is unpinned. \ + Consider setting a lower bound with a constraint when using \ + `--resolution-strategy lowest` to avoid using outdated versions." + ) + } } } @@ -120,6 +132,7 @@ impl Diagnostic for ResolutionDiagnostic { Self::MissingExtra { dist, .. } => name == dist.name(), Self::MissingDev { dist, .. } => name == dist.name(), Self::YankedVersion { dist, .. } => name == dist.name(), + Self::MissingLowerBound { package_name } => name == package_name, } } } diff --git a/crates/pep440-rs/src/version_specifier.rs b/crates/pep440-rs/src/version_specifier.rs index 72fb36830..973d188ee 100644 --- a/crates/pep440-rs/src/version_specifier.rs +++ b/crates/pep440-rs/src/version_specifier.rs @@ -624,6 +624,22 @@ impl VersionSpecifier { other > this } + + /// Whether this version specifier rejects versions below a lower cutoff. + pub fn has_lower_bound(&self) -> bool { + match self.operator() { + Operator::Equal + | Operator::EqualStar + | Operator::ExactEqual + | Operator::TildeEqual + | Operator::GreaterThan + | Operator::GreaterThanEqual => true, + Operator::LessThanEqual + | Operator::LessThan + | Operator::NotEqualStar + | Operator::NotEqual => false, + } + } } impl FromStr for VersionSpecifier { diff --git a/crates/pypi-types/src/requirement.rs b/crates/pypi-types/src/requirement.rs index 7a7fa4bd6..dc7003fe0 100644 --- a/crates/pypi-types/src/requirement.rs +++ b/crates/pypi-types/src/requirement.rs @@ -466,6 +466,17 @@ impl RequirementSource { pub fn is_editable(&self) -> bool { matches!(self, Self::Directory { editable: true, .. }) } + + /// If the source is the registry, return the version specifiers + pub fn version_specifiers(&self) -> Option<&VersionSpecifiers> { + match self { + RequirementSource::Registry { specifier, .. } => Some(specifier), + RequirementSource::Url { .. } + | RequirementSource::Git { .. } + | RequirementSource::Path { .. } + | RequirementSource::Directory { .. } => None, + } + } } impl Display for RequirementSource { diff --git a/crates/uv-resolver/src/resolution/graph.rs b/crates/uv-resolver/src/resolution/graph.rs index be736c8a6..7e2b21510 100644 --- a/crates/uv-resolver/src/resolution/graph.rs +++ b/crates/uv-resolver/src/resolution/graph.rs @@ -3,7 +3,7 @@ use std::collections::BTreeSet; use indexmap::IndexSet; use petgraph::{ graph::{Graph, NodeIndex}, - Directed, + Directed, Direction, }; use pubgrub::Range; use rustc_hash::{FxBuildHasher, FxHashMap, FxHashSet}; @@ -25,6 +25,7 @@ use crate::preferences::Preferences; use crate::python_requirement::PythonTarget; use crate::redirect::url_to_precise; use crate::resolution::AnnotatedDist; +use crate::resolution_mode::ResolutionStrategy; use crate::resolver::{Resolution, ResolutionDependencyEdge, ResolutionPackage}; use crate::{ InMemoryIndex, MetadataResponse, Options, PythonRequirement, RequiresPython, ResolveError, @@ -86,6 +87,7 @@ impl ResolutionGraph { index: &InMemoryIndex, git: &GitResolver, python: &PythonRequirement, + resolution_strategy: &ResolutionStrategy, options: Options, ) -> Result { let size_guess = resolutions[0].nodes.len(); @@ -194,6 +196,10 @@ impl ResolutionGraph { ) }; + if matches!(resolution_strategy, ResolutionStrategy::Lowest) { + report_missing_lower_bounds(&petgraph, &mut diagnostics); + } + Ok(Self { petgraph, requires_python, @@ -657,3 +663,72 @@ impl From for distribution_types::Resolution { ) } } + +/// Find any packages that don't have any lower bound on them when in resolution-lowest mode. +fn report_missing_lower_bounds( + petgraph: &Graph>, + diagnostics: &mut Vec, +) { + for node_index in petgraph.node_indices() { + let ResolutionGraphNode::Dist(dist) = petgraph.node_weight(node_index).unwrap() else { + // Ignore the root package. + continue; + }; + if dist.dev.is_some() { + // TODO(konsti): Dev dependencies are modelled incorrectly in the graph. There should + // be an edge from root to project-with-dev, just like to project-with-extra, but + // currently there is only an edge from project to to project-with-dev that we then + // have to drop. + continue; + } + if !has_lower_bound(node_index, dist.name(), petgraph) { + diagnostics.push(ResolutionDiagnostic::MissingLowerBound { + package_name: dist.name().clone(), + }); + } + } +} + +/// Whether the given package has a lower version bound by another package. +fn has_lower_bound( + node_index: NodeIndex, + package_name: &PackageName, + petgraph: &Graph>, +) -> bool { + for neighbor_index in petgraph.neighbors_directed(node_index, Direction::Incoming) { + let neighbor_dist = match petgraph.node_weight(neighbor_index).unwrap() { + ResolutionGraphNode::Root => { + // We already handled direct dependencies with a missing constraint + // separately. + return true; + } + ResolutionGraphNode::Dist(neighbor_dist) => neighbor_dist, + }; + + if neighbor_dist.name() == package_name { + // Only warn for real packages, not for virtual packages such as dev nodes. + return true; + } + + // Get all individual specifier for the current package and check if any has a lower + // bound. + for requirement in neighbor_dist + .metadata + .requires_dist + .iter() + .chain(neighbor_dist.metadata.dev_dependencies.values().flatten()) + { + if requirement.name != *package_name { + continue; + } + let Some(specifiers) = requirement.source.version_specifiers() else { + // URL requirements are a bound. + return true; + }; + if specifiers.iter().any(VersionSpecifier::has_lower_bound) { + return true; + } + } + } + false +} diff --git a/crates/uv-resolver/src/resolver/mod.rs b/crates/uv-resolver/src/resolver/mod.rs index 1850988cf..c6b060050 100644 --- a/crates/uv-resolver/src/resolver/mod.rs +++ b/crates/uv-resolver/src/resolver/mod.rs @@ -627,6 +627,7 @@ impl ResolverState Result<()> { assert!(!lock.contains("environment-markers")); Ok(()) } + +/// Warn when there are missing bounds on transitive dependencies with `--resolution lowest`. +#[test] +fn warn_missing_transitive_lower_bounds() -> Result<()> { + let context = TestContext::new("3.12"); + + let pyproject_toml = context.temp_dir.child("pyproject.toml"); + pyproject_toml.write_str( + r#" + [project] + name = "foo" + version = "0.1.0" + requires-python = ">=3.12" + dependencies = ["pytest>8"] + "#, + )?; + + uv_snapshot!(context.filters(), context.lock().arg("--resolution").arg("lowest"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + warning: `uv lock` is experimental and may change without warning + Resolved 6 packages in [TIME] + warning: The transitive dependency `packaging` is unpinned. Consider setting a lower bound with a constraint when using `--resolution-strategy lowest` to avoid using outdated versions. + warning: The transitive dependency `colorama` is unpinned. Consider setting a lower bound with a constraint when using `--resolution-strategy lowest` to avoid using outdated versions. + warning: The transitive dependency `iniconfig` is unpinned. Consider setting a lower bound with a constraint when using `--resolution-strategy lowest` to avoid using outdated versions. + "###); + + Ok(()) +}