Skip to content

Commit 03d0e95

Browse files
konstinibraheemdev
authored andcommitted
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 <ibraheem@ibraheem.ca>
1 parent 07c6e07 commit 03d0e95

File tree

6 files changed

+154
-2
lines changed

6 files changed

+154
-2
lines changed

crates/distribution-types/src/resolution.rs

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,11 @@ pub enum ResolutionDiagnostic {
9292
/// The reason that the version was yanked, if any.
9393
reason: Option<String>,
9494
},
95+
MissingLowerBound {
96+
/// The name of the package that had no lower bound from any other package in the
97+
/// resolution. For example, `black`.
98+
package_name: PackageName,
99+
},
95100
}
96101

97102
impl Diagnostic for ResolutionDiagnostic {
@@ -111,6 +116,13 @@ impl Diagnostic for ResolutionDiagnostic {
111116
format!("`{dist}` is yanked")
112117
}
113118
}
119+
Self::MissingLowerBound { package_name: name } => {
120+
format!(
121+
"The transitive dependency `{name}` is unpinned. \
122+
Consider setting a lower bound with a constraint when using \
123+
`--resolution-strategy lowest` to avoid using outdated versions."
124+
)
125+
}
114126
}
115127
}
116128

@@ -120,6 +132,7 @@ impl Diagnostic for ResolutionDiagnostic {
120132
Self::MissingExtra { dist, .. } => name == dist.name(),
121133
Self::MissingDev { dist, .. } => name == dist.name(),
122134
Self::YankedVersion { dist, .. } => name == dist.name(),
135+
Self::MissingLowerBound { package_name } => name == package_name,
123136
}
124137
}
125138
}

crates/pep440-rs/src/version_specifier.rs

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -624,6 +624,22 @@ impl VersionSpecifier {
624624

625625
other > this
626626
}
627+
628+
/// Whether this version specifier rejects versions below a lower cutoff.
629+
pub fn has_lower_bound(&self) -> bool {
630+
match self.operator() {
631+
Operator::Equal
632+
| Operator::EqualStar
633+
| Operator::ExactEqual
634+
| Operator::TildeEqual
635+
| Operator::GreaterThan
636+
| Operator::GreaterThanEqual => true,
637+
Operator::LessThanEqual
638+
| Operator::LessThan
639+
| Operator::NotEqualStar
640+
| Operator::NotEqual => false,
641+
}
642+
}
627643
}
628644

629645
impl FromStr for VersionSpecifier {

crates/pypi-types/src/requirement.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -466,6 +466,17 @@ impl RequirementSource {
466466
pub fn is_editable(&self) -> bool {
467467
matches!(self, Self::Directory { editable: true, .. })
468468
}
469+
470+
/// If the source is the registry, return the version specifiers
471+
pub fn version_specifiers(&self) -> Option<&VersionSpecifiers> {
472+
match self {
473+
RequirementSource::Registry { specifier, .. } => Some(specifier),
474+
RequirementSource::Url { .. }
475+
| RequirementSource::Git { .. }
476+
| RequirementSource::Path { .. }
477+
| RequirementSource::Directory { .. } => None,
478+
}
479+
}
469480
}
470481

471482
impl Display for RequirementSource {

crates/uv-resolver/src/resolution/graph.rs

Lines changed: 76 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ use std::collections::BTreeSet;
33
use indexmap::IndexSet;
44
use petgraph::{
55
graph::{Graph, NodeIndex},
6-
Directed,
6+
Directed, Direction,
77
};
88
use pubgrub::Range;
99
use rustc_hash::{FxBuildHasher, FxHashMap, FxHashSet};
@@ -25,6 +25,7 @@ use crate::preferences::Preferences;
2525
use crate::python_requirement::PythonTarget;
2626
use crate::redirect::url_to_precise;
2727
use crate::resolution::AnnotatedDist;
28+
use crate::resolution_mode::ResolutionStrategy;
2829
use crate::resolver::{Resolution, ResolutionDependencyEdge, ResolutionPackage};
2930
use crate::{
3031
InMemoryIndex, MetadataResponse, Options, PythonRequirement, RequiresPython, ResolveError,
@@ -86,6 +87,7 @@ impl ResolutionGraph {
8687
index: &InMemoryIndex,
8788
git: &GitResolver,
8889
python: &PythonRequirement,
90+
resolution_strategy: &ResolutionStrategy,
8991
options: Options,
9092
) -> Result<Self, ResolveError> {
9193
let size_guess = resolutions[0].nodes.len();
@@ -194,6 +196,10 @@ impl ResolutionGraph {
194196
)
195197
};
196198

199+
if matches!(resolution_strategy, ResolutionStrategy::Lowest) {
200+
report_missing_lower_bounds(&petgraph, &mut diagnostics);
201+
}
202+
197203
Ok(Self {
198204
petgraph,
199205
requires_python,
@@ -657,3 +663,72 @@ impl From<ResolutionGraph> for distribution_types::Resolution {
657663
)
658664
}
659665
}
666+
667+
/// Find any packages that don't have any lower bound on them when in resolution-lowest mode.
668+
fn report_missing_lower_bounds(
669+
petgraph: &Graph<ResolutionGraphNode, Option<MarkerTree>>,
670+
diagnostics: &mut Vec<ResolutionDiagnostic>,
671+
) {
672+
for node_index in petgraph.node_indices() {
673+
let ResolutionGraphNode::Dist(dist) = petgraph.node_weight(node_index).unwrap() else {
674+
// Ignore the root package.
675+
continue;
676+
};
677+
if dist.dev.is_some() {
678+
// TODO(konsti): Dev dependencies are modelled incorrectly in the graph. There should
679+
// be an edge from root to project-with-dev, just like to project-with-extra, but
680+
// currently there is only an edge from project to to project-with-dev that we then
681+
// have to drop.
682+
continue;
683+
}
684+
if !has_lower_bound(node_index, dist.name(), petgraph) {
685+
diagnostics.push(ResolutionDiagnostic::MissingLowerBound {
686+
package_name: dist.name().clone(),
687+
});
688+
}
689+
}
690+
}
691+
692+
/// Whether the given package has a lower version bound by another package.
693+
fn has_lower_bound(
694+
node_index: NodeIndex,
695+
package_name: &PackageName,
696+
petgraph: &Graph<ResolutionGraphNode, Option<MarkerTree>>,
697+
) -> bool {
698+
for neighbor_index in petgraph.neighbors_directed(node_index, Direction::Incoming) {
699+
let neighbor_dist = match petgraph.node_weight(neighbor_index).unwrap() {
700+
ResolutionGraphNode::Root => {
701+
// We already handled direct dependencies with a missing constraint
702+
// separately.
703+
return true;
704+
}
705+
ResolutionGraphNode::Dist(neighbor_dist) => neighbor_dist,
706+
};
707+
708+
if neighbor_dist.name() == package_name {
709+
// Only warn for real packages, not for virtual packages such as dev nodes.
710+
return true;
711+
}
712+
713+
// Get all individual specifier for the current package and check if any has a lower
714+
// bound.
715+
for requirement in neighbor_dist
716+
.metadata
717+
.requires_dist
718+
.iter()
719+
.chain(neighbor_dist.metadata.dev_dependencies.values().flatten())
720+
{
721+
if requirement.name != *package_name {
722+
continue;
723+
}
724+
let Some(specifiers) = requirement.source.version_specifiers() else {
725+
// URL requirements are a bound.
726+
return true;
727+
};
728+
if specifiers.iter().any(VersionSpecifier::has_lower_bound) {
729+
return true;
730+
}
731+
}
732+
}
733+
false
734+
}

crates/uv-resolver/src/resolver/mod.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -627,6 +627,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
627627
&self.index,
628628
&self.git,
629629
&self.python_requirement,
630+
self.selector.resolution_strategy(),
630631
self.options,
631632
)
632633
}
@@ -2156,7 +2157,11 @@ impl ForkState {
21562157
ResolutionStrategy::Lowest | ResolutionStrategy::LowestDirect(..)
21572158
);
21582159
if !has_url && missing_lower_bound && strategy_lowest {
2159-
warn_user_once!("The direct dependency `{package}` is unpinned. Consider setting a lower bound when using `--resolution-strategy lowest` to avoid using outdated versions.");
2160+
warn_user_once!(
2161+
"The direct dependency `{package}` is unpinned. \
2162+
Consider setting a lower bound when using `--resolution-strategy lowest` \
2163+
to avoid using outdated versions."
2164+
);
21602165
}
21612166
}
21622167

crates/uv/tests/lock.rs

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5839,3 +5839,35 @@ fn lock_upgrade_drop_fork_markers() -> Result<()> {
58395839
assert!(!lock.contains("environment-markers"));
58405840
Ok(())
58415841
}
5842+
5843+
/// Warn when there are missing bounds on transitive dependencies with `--resolution lowest`.
5844+
#[test]
5845+
fn warn_missing_transitive_lower_bounds() -> Result<()> {
5846+
let context = TestContext::new("3.12");
5847+
5848+
let pyproject_toml = context.temp_dir.child("pyproject.toml");
5849+
pyproject_toml.write_str(
5850+
r#"
5851+
[project]
5852+
name = "foo"
5853+
version = "0.1.0"
5854+
requires-python = ">=3.12"
5855+
dependencies = ["pytest>8"]
5856+
"#,
5857+
)?;
5858+
5859+
uv_snapshot!(context.filters(), context.lock().arg("--resolution").arg("lowest"), @r###"
5860+
success: true
5861+
exit_code: 0
5862+
----- stdout -----
5863+
5864+
----- stderr -----
5865+
warning: `uv lock` is experimental and may change without warning
5866+
Resolved 6 packages in [TIME]
5867+
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.
5868+
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.
5869+
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.
5870+
"###);
5871+
5872+
Ok(())
5873+
}

0 commit comments

Comments
 (0)