Make > operator exclude post and local releases (#2471)

## Summary

This PR attempts to use a similar trick to that we added in
https://github.com/astral-sh/uv/pull/1878, but for post-releases.

In https://github.com/astral-sh/uv/pull/1878, we added a fake "minimum"
version to enable us to treat `< 1.0.0` as _excluding_ pre-releases of
1.0.0.

Today, on `main`, we accept post-releases and local versions in `>
1.0.0`. But per PEP 440, that should _exclude_ post-releases and local
versions, unless the specifier is itself a pre-release, in which case,
pre-releases are allowed (e.g., `> 1.0.0.post0` should allow `>
1.0.0.post1`).

To support this, we add a fake "maximum" version that's greater than all
the post and local releases for a given version. This leverages our last
remaining free bit in the compact representation.
This commit is contained in:
Charlie Marsh 2024-03-15 07:02:06 -07:00 committed by GitHub
parent c296da34bf
commit e69b76bc72
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 222 additions and 110 deletions

View file

@ -398,6 +398,19 @@ impl Version {
}
}
/// Returns the max-release part of this version, if it exists.
///
/// The "max" component is internal-only, and does not exist in PEP 440.
/// The version `1.0max0` is larger than all other `1.0` versions,
/// like `1.0.post1`, `1.0+local`, etc.
#[inline]
pub fn max(&self) -> Option<u64> {
match *self.inner {
VersionInner::Small { ref small } => small.max(),
VersionInner::Full { ref full } => full.max,
}
}
/// Set the release numbers and return the updated version.
///
/// Usually one can just use `Version::new` to create a new version with
@ -532,6 +545,8 @@ impl Version {
/// like `1.0a1`, `1.0dev0`, etc.
#[inline]
pub fn with_min(mut self, value: Option<u64>) -> Self {
debug_assert!(!self.is_pre(), "min is not allowed on pre-release versions");
debug_assert!(!self.is_dev(), "min is not allowed on dev versions");
if let VersionInner::Small { ref mut small } = Arc::make_mut(&mut self.inner) {
if small.set_min(value) {
return self;
@ -541,6 +556,27 @@ impl Version {
self
}
/// Set the max-release component and return the updated version.
///
/// The "max" component is internal-only, and does not exist in PEP 440.
/// The version `1.0max0` is larger than all other `1.0` versions,
/// like `1.0.post1`, `1.0+local`, etc.
#[inline]
pub fn with_max(mut self, value: Option<u64>) -> Self {
debug_assert!(
!self.is_post(),
"max is not allowed on post-release versions"
);
debug_assert!(!self.is_dev(), "max is not allowed on dev versions");
if let VersionInner::Small { ref mut small } = Arc::make_mut(&mut self.inner) {
if small.set_max(value) {
return self;
}
}
self.make_full().max = value;
self
}
/// Convert this version to a "full" representation in-place and return a
/// mutable borrow to the full type.
fn make_full(&mut self) -> &mut VersionFull {
@ -549,6 +585,7 @@ impl Version {
epoch: small.epoch(),
release: small.release().to_vec(),
min: small.min(),
max: small.max(),
pre: small.pre(),
post: small.post(),
dev: small.dev(),
@ -774,13 +811,15 @@ impl FromStr for Version {
/// * Bytes 5, 4 and 3 correspond to the second, third and fourth release
/// segments, respectively.
/// * Bytes 2, 1 and 0 represent *one* of the following:
/// `min, .devN, aN, bN, rcN, <no suffix>, .postN`.
/// `min, .devN, aN, bN, rcN, <no suffix>, .postN, max`.
/// Its representation is thus:
/// * The most significant 3 bits of Byte 2 corresponds to a value in
/// the range 0-6 inclusive, corresponding to min, dev, pre-a, pre-b, pre-rc,
/// no-suffix or post releases, respectively. `min` is a special version that
/// does not exist in PEP 440, but is used here to represent the smallest
/// possible version, preceding any `dev`, `pre`, `post` or releases.
/// possible version, preceding any `dev`, `pre`, `post` or releases. `max` is
/// an analogous concept for the largest possible version, following any `post`
/// or local releases.
/// * The low 5 bits combined with the bits in bytes 1 and 0 correspond
/// to the release number of the suffix, if one exists. If there is no
/// suffix, then this bits are always 0.
@ -789,7 +828,7 @@ impl FromStr for Version {
/// encoded at a less significant location than the release numbers, so that
/// `1.2.3 < 1.2.3.post4`.
///
/// In a previous representation, we tried to enocode the suffixes in different
/// In a previous representation, we tried to encode the suffixes in different
/// locations so that, in theory, you could represent `1.2.3.dev2.post3` in the
/// packed form. But getting the ordering right for this is difficult (perhaps
/// impossible without extra space?). So we limited to only storing one suffix.
@ -850,6 +889,7 @@ impl VersionSmall {
const SUFFIX_PRE_RC: u64 = 4;
const SUFFIX_NONE: u64 = 5;
const SUFFIX_POST: u64 = 6;
const SUFFIX_MAX: u64 = 7;
const SUFFIX_MAX_VERSION: u64 = 0x1FFFFF;
#[inline]
@ -922,7 +962,11 @@ impl VersionSmall {
#[inline]
fn set_post(&mut self, value: Option<u64>) -> bool {
if self.min().is_some() || self.pre().is_some() || self.dev().is_some() {
if self.min().is_some()
|| self.pre().is_some()
|| self.dev().is_some()
|| self.max().is_some()
{
return value.is_none();
}
match value {
@ -965,7 +1009,11 @@ impl VersionSmall {
#[inline]
fn set_pre(&mut self, value: Option<PreRelease>) -> bool {
if self.min().is_some() || self.dev().is_some() || self.post().is_some() {
if self.min().is_some()
|| self.dev().is_some()
|| self.post().is_some()
|| self.max().is_some()
{
return value.is_none();
}
match value {
@ -1004,7 +1052,11 @@ impl VersionSmall {
#[inline]
fn set_dev(&mut self, value: Option<u64>) -> bool {
if self.min().is_some() || self.pre().is_some() || self.post().is_some() {
if self.min().is_some()
|| self.pre().is_some()
|| self.post().is_some()
|| self.max().is_some()
{
return value.is_none();
}
match value {
@ -1033,7 +1085,11 @@ impl VersionSmall {
#[inline]
fn set_min(&mut self, value: Option<u64>) -> bool {
if self.dev().is_some() || self.pre().is_some() || self.post().is_some() {
if self.dev().is_some()
|| self.pre().is_some()
|| self.post().is_some()
|| self.max().is_some()
{
return value.is_none();
}
match value {
@ -1051,6 +1107,39 @@ impl VersionSmall {
true
}
#[inline]
fn max(&self) -> Option<u64> {
if self.suffix_kind() == Self::SUFFIX_MAX {
Some(self.suffix_version())
} else {
None
}
}
#[inline]
fn set_max(&mut self, value: Option<u64>) -> bool {
if self.dev().is_some()
|| self.pre().is_some()
|| self.post().is_some()
|| self.min().is_some()
{
return value.is_none();
}
match value {
None => {
self.set_suffix_kind(Self::SUFFIX_NONE);
}
Some(number) => {
if number > Self::SUFFIX_MAX_VERSION {
return false;
}
self.set_suffix_kind(Self::SUFFIX_MAX);
self.set_suffix_version(number);
}
}
true
}
#[inline]
fn local(&self) -> &[LocalSegment] {
// A "small" version is never used if the version has a non-zero number
@ -1061,13 +1150,13 @@ impl VersionSmall {
#[inline]
fn suffix_kind(&self) -> u64 {
let kind = (self.repr >> 21) & 0b111;
debug_assert!(kind <= Self::SUFFIX_POST);
debug_assert!(kind <= Self::SUFFIX_MAX);
kind
}
#[inline]
fn set_suffix_kind(&mut self, kind: u64) {
debug_assert!(kind <= Self::SUFFIX_POST);
debug_assert!(kind <= Self::SUFFIX_MAX);
self.repr &= !0xE00000;
self.repr |= kind << 21;
if kind == Self::SUFFIX_NONE {
@ -1146,6 +1235,10 @@ struct VersionFull {
/// represent the smallest possible version of a release, preceding any
/// `dev`, `pre`, `post` or releases.
min: Option<u64>,
/// An internal-only segment that does not exist in PEP 440, used to
/// represent the largest possible version of a release, following any
/// `post` or local releases.
max: Option<u64>,
}
/// A version number pattern.
@ -2320,7 +2413,13 @@ pub(crate) fn compare_release(this: &[u64], other: &[u64]) -> Ordering {
///
/// [pep440-suffix-ordering]: https://peps.python.org/pep-0440/#summary-of-permitted-suffixes-and-relative-ordering
fn sortable_tuple(version: &Version) -> (u64, u64, Option<u64>, u64, &[LocalSegment]) {
match (version.pre(), version.post(), version.dev(), version.min()) {
// If the version is a "max" version, use a post version larger than any possible post version.
let post = if version.max().is_some() {
Some(u64::MAX)
} else {
version.post()
};
match (version.pre(), post, version.dev(), version.min()) {
// min release
(_pre, post, _dev, Some(n)) => (0, 0, post, n, version.local()),
// dev release
@ -3626,6 +3725,87 @@ mod tests {
}
}
#[test]
fn max_version() {
// Ensure that the `.max` suffix succeeds all other suffixes.
let greater = Version::new([1, 0]).with_max(Some(0));
let versions = &[
"1.dev0",
"1.0.dev456",
"1.0a1",
"1.0a2.dev456",
"1.0a12.dev456",
"1.0a12",
"1.0b1.dev456",
"1.0b2",
"1.0b2.post345.dev456",
"1.0b2.post345",
"1.0rc1.dev456",
"1.0rc1",
"1.0",
"1.0+abc.5",
"1.0+abc.7",
"1.0+5",
"1.0.post456.dev34",
"1.0.post456",
"1.0",
];
for less in versions.iter() {
let less = less.parse::<Version>().unwrap();
assert_eq!(
less.cmp(&greater),
Ordering::Less,
"less: {:?}\ngreater: {:?}",
less.as_bloated_debug(),
greater.as_bloated_debug()
);
}
// Ensure that the `.max` suffix plays nicely with pre-release versions.
let greater = Version::new([1, 0])
.with_pre(Some(PreRelease {
kind: PreReleaseKind::Alpha,
number: 1,
}))
.with_max(Some(0));
let versions = &["1.0a1", "1.0a1+local", "1.0a1.post1"];
for less in versions.iter() {
let less = less.parse::<Version>().unwrap();
assert_eq!(
less.cmp(&greater),
Ordering::Less,
"less: {:?}\ngreater: {:?}",
less.as_bloated_debug(),
greater.as_bloated_debug()
);
}
// Ensure that the `.max` suffix plays nicely with pre-release versions.
let less = Version::new([1, 0])
.with_pre(Some(PreRelease {
kind: PreReleaseKind::Alpha,
number: 1,
}))
.with_max(Some(0));
let versions = &["1.0b1", "1.0b1+local", "1.0b1.post1", "1.0"];
for greater in versions.iter() {
let greater = greater.parse::<Version>().unwrap();
assert_eq!(
less.cmp(&greater),
Ordering::Less,
"less: {:?}\ngreater: {:?}",
less.as_bloated_debug(),
greater.as_bloated_debug()
);
}
}
// Tests our bespoke u64 decimal integer parser.
#[test]
fn parse_number_u64() {
@ -3694,6 +3874,7 @@ mod tests {
.field("dev", &self.0.dev())
.field("local", &self.0.local())
.field("min", &self.0.min())
.field("max", &self.0.max())
.finish()
}
}

View file

@ -537,7 +537,7 @@ impl CacheBucket {
Self::FlatIndex => "flat-index-v0",
Self::Git => "git-v0",
Self::Interpreter => "interpreter-v0",
Self::Simple => "simple-v3",
Self::Simple => "simple-v4",
Self::Wheels => "wheels-v0",
Self::Archive => "archive-v0",
}

View file

@ -62,9 +62,14 @@ impl TryFrom<&VersionSpecifier> for PubGrubSpecifier {
Operator::GreaterThan => {
// Per PEP 440: "The exclusive ordered comparison >V MUST NOT allow a post-release of
// the given version unless V itself is a post release."
// TODO(charlie): This needs to exclude post and local releases.
let version = specifier.version().clone();
Range::strictly_higher_than(version)
if let Some(dev) = version.dev() {
Range::higher_than(version.with_dev(Some(dev + 1)))
} else if let Some(post) = version.post() {
Range::higher_than(version.with_post(Some(post + 1)))
} else {
Range::strictly_higher_than(version.with_max(Some(0)))
}
}
Operator::GreaterThanEqual => {
let version = specifier.version().clone();

View file

@ -1507,28 +1507,24 @@ fn local_transitive_greater_than() {
.arg("local-transitive-greater-than-a")
.arg("local-transitive-greater-than-b==2.0.0+foo")
, @r###"
success: true
exit_code: 0
success: false
exit_code: 1
----- stdout -----
----- stderr -----
Resolved 2 packages in [TIME]
Downloaded 2 packages in [TIME]
Installed 2 packages in [TIME]
+ package-a==1.0.0
+ package-b==2.0.0+foo
× No solution found when resolving dependencies:
Because only package-a==1.0.0 is available and package-a==1.0.0 depends on package-b>2.0.0, we can conclude that all versions of package-a depend on package-b>2.0.0.
And because you require package-a and you require package-b==2.0.0+foo, we can conclude that the requirements are unsatisfiable.
"###);
assert_installed(
assert_not_installed(
&context.venv,
"local_transitive_greater_than_a",
"1.0.0",
&context.temp_dir,
);
assert_installed(
assert_not_installed(
&context.venv,
"local_transitive_greater_than_b",
"2.0.0+foo",
&context.temp_dir,
);
}
@ -1889,23 +1885,16 @@ fn local_greater_than() {
uv_snapshot!(filters, command(&context)
.arg("local-greater-than-a>1.2.3")
, @r###"
success: true
exit_code: 0
success: false
exit_code: 1
----- stdout -----
----- stderr -----
Resolved 1 package in [TIME]
Downloaded 1 package in [TIME]
Installed 1 package in [TIME]
+ package-a==1.2.3+foo
× No solution found when resolving dependencies:
Because only package-a<=1.2.3 is available and you require package-a>1.2.3, we can conclude that the requirements are unsatisfiable.
"###);
assert_installed(
&context.venv,
"local_greater_than_a",
"1.2.3+foo",
&context.temp_dir,
);
assert_not_installed(&context.venv, "local_greater_than_a", &context.temp_dir);
}
/// A local version should be included in inclusive ordered comparisons.
@ -2127,23 +2116,16 @@ fn post_greater_than() {
uv_snapshot!(filters, command(&context)
.arg("post-greater-than-a>1.2.3")
, @r###"
success: true
exit_code: 0
success: false
exit_code: 1
----- stdout -----
----- stderr -----
Resolved 1 package in [TIME]
Downloaded 1 package in [TIME]
Installed 1 package in [TIME]
+ package-a==1.2.3.post1
× No solution found when resolving dependencies:
Because only package-a<=1.2.3 is available and you require package-a>1.2.3, we can conclude that the requirements are unsatisfiable.
"###);
assert_installed(
&context.venv,
"post_greater_than_a",
"1.2.3.post1",
&context.temp_dir,
);
assert_not_installed(&context.venv, "post_greater_than_a", &context.temp_dir);
}
/// A greater-than version constraint should match a post-release version if the
@ -2336,21 +2318,18 @@ fn post_local_greater_than() {
uv_snapshot!(filters, command(&context)
.arg("post-local-greater-than-a>1.2.3")
, @r###"
success: true
exit_code: 0
success: false
exit_code: 1
----- stdout -----
----- stderr -----
Resolved 1 package in [TIME]
Downloaded 1 package in [TIME]
Installed 1 package in [TIME]
+ package-a==1.2.3.post1+local
× No solution found when resolving dependencies:
Because only package-a<=1.2.3 is available and you require package-a>1.2.3, we can conclude that the requirements are unsatisfiable.
"###);
assert_installed(
assert_not_installed(
&context.venv,
"post_local_greater_than_a",
"1.2.3.post1+local",
&context.temp_dir,
);
}

View file

@ -152,59 +152,6 @@ def main(scenarios: list[Path], snapshot_update: bool = True):
expected[
"explanation"
] = "We do not have correct behavior for local version identifiers yet"
elif scenario["name"] == "local-greater-than":
expected["satisfiable"] = True
expected["packages"] = [
{
"name": "local-greater-than-a",
"version": "1.2.3+foo",
"module_name": "local_greater_than_a",
}
]
expected["explanation"] = (
"We do not have correct behavior for local version identifiers yet"
)
elif scenario["name"] == "local-transitive-greater-than":
expected["satisfiable"] = True
expected["packages"] = [
{
"name": "local-transitive-greater-than-a",
"version": "1.0.0",
"module_name": "local_transitive_greater_than_a",
},
{
"name": "local-transitive-greater-than-b",
"version": "2.0.0+foo",
"module_name": "local_transitive_greater_than_b",
}
]
expected["explanation"] = (
"We do not have correct behavior for local version identifiers yet"
)
elif scenario["name"] == 'post-greater-than':
expected["satisfiable"] = True
expected["packages"] = [
{
"name": "post-greater-than-a",
"version": "1.2.3.post1",
"module_name": "post_greater_than_a",
}
]
expected["explanation"] = (
"We do not have correct behavior for local version identifiers yet"
)
elif scenario["name"] == 'post-local-greater-than':
expected["satisfiable"] = True
expected["packages"] = [
{
"name": "post-local-greater-than-a",
"version": "1.2.3.post1+local",
"module_name": "post_local_greater_than_a",
}
]
expected["explanation"] = (
"We do not have correct behavior for local version identifiers yet"
)
# Split scenarios into `install` and `compile` cases
install_scenarios = []

View file

@ -1,2 +1,2 @@
chevron-blue
packse>=0.3.10
packse>=0.3.11