Normalize python_version markers to python_full_version (#6126)

## Summary

Normalize all `python_version` markers to their equivalent
`python_full_version` form. This avoids false positives in forking
because we currently cannot detect any relationships between the two
forms. It also avoids subtle bugs due to the truncating semantics of
`python_version`. For example, given `requires-python = ">3.12"`, we
currently simplify the marker `python_version <= 3.12` to `false`.
However, the version `3.12.1` will be truncated to `3.12` for
`python_version` comparisons, and thus it satisfies the python
requirement and evaluates to `true`.

It is possible to simplify back to `python_version` when writing markers
to the lockfile. However, the equivalent `python_full_version` markers
are often clearer and easier to simplify, so I lean towards leaving them
as `python_full_version`.

There are *a lot* of snapshot updates from this change. I'd like more
eyes on the transformation logic in `python_version_to_full_version` to
ensure that they are all correct.

Resolves #6125.

crates/pep440-rs/src/version.rs

@@ -119,6 +119,11 @@ impl Operator {
             _ => None,
         }
     }
+
+    /// Returns `true` if this operator represents a wildcard.
+    pub fn is_star(self) -> bool {
+        matches!(self, Self::EqualStar | Self::NotEqualStar)
+    }
 }
 
 impl FromStr for Operator {

crates/pep440-rs/src/version_specifier.rs

@@ -416,6 +416,23 @@ impl VersionSpecifier {
             version,
         }
     }
+
+    /// `==<version>.*`
+    pub fn equals_star_version(version: Version) -> Self {
+        Self {
+            operator: Operator::EqualStar,
+            version,
+        }
+    }
+
+    /// `!=<version>.*`
+    pub fn not_equals_star_version(version: Version) -> Self {
+        Self {
+            operator: Operator::NotEqualStar,
+            version,
+        }
+    }
+
     /// `!=<version>`
     pub fn not_equals_version(version: Version) -> Self {
         Self {
@@ -465,19 +482,36 @@ impl VersionSpecifier {
         &self.version
     }
 
+    /// Get the operator and version parts of this specifier.
+    pub fn into_parts(self) -> (Operator, Version) {
+        (self.operator, self.version)
+    }
+
     /// Whether the version marker includes a prerelease.
     pub fn any_prerelease(&self) -> bool {
         self.version.any_prerelease()
     }
 
     /// Returns the version specifiers whose union represents the given range.
-    pub fn from_bounds(
+    ///
+    /// This function is not applicable to ranges involving pre-release versions.
+    pub fn from_release_only_bounds(
         bounds: (&Bound<Version>, &Bound<Version>),
     ) -> impl Iterator<Item = VersionSpecifier> {
         let (b1, b2) = match bounds {
             (Bound::Included(v1), Bound::Included(v2)) if v1 == v2 => {
                 (Some(VersionSpecifier::equals_version(v1.clone())), None)
             }
+            // `v >= 3.7 && v < 3.8` is equivalent to `v == 3.7.*`
+            (Bound::Included(v1), Bound::Excluded(v2))
+                if v1.release().len() == 2
+                    && v2.release() == [v1.release()[0], v1.release()[1] + 1] =>
+            {
+                (
+                    Some(VersionSpecifier::equals_star_version(v1.clone())),
+                    None,
+                )
+            }
             (lower, upper) => (
                 VersionSpecifier::from_lower_bound(lower),
                 VersionSpecifier::from_upper_bound(upper),

crates/pep508-rs/src/lib.rs

@@ -1172,7 +1172,7 @@ mod tests {
 
     #[test]
     fn basic_examples() {
-        let input = r"requests[security,tests]>=2.8.1,==2.8.* ; python_version < '2.7'";
+        let input = r"requests[security,tests]>=2.8.1,==2.8.* ; python_full_version < '2.7'";
         let requests = Requirement::<Url>::from_str(input).unwrap();
         assert_eq!(input, requests.to_string());
         let expected = Requirement {
@@ -1198,7 +1198,7 @@ mod tests {
                 .collect(),
             )),
             marker: MarkerTree::expression(MarkerExpression::Version {
-                key: MarkerValueVersion::PythonVersion,
+                key: MarkerValueVersion::PythonFullVersion,
                 specifier: VersionSpecifier::from_pattern(
                     pep440_rs::Operator::LessThan,
                     "2.7".parse().unwrap(),
@@ -1788,10 +1788,16 @@ mod tests {
     #[test]
     fn no_space_after_operator() {
         let requirement = Requirement::<Url>::from_str("pytest;python_version<='4.0'").unwrap();
-        assert_eq!(requirement.to_string(), "pytest ; python_version <= '4.0'");
+        assert_eq!(
+            requirement.to_string(),
+            "pytest ; python_full_version < '4.1'"
+        );
 
         let requirement = Requirement::<Url>::from_str("pytest;'4.0'>=python_version").unwrap();
-        assert_eq!(requirement.to_string(), "pytest ; python_version <= '4.0'");
+        assert_eq!(
+            requirement.to_string(),
+            "pytest ; python_full_version < '4.1'"
+        );
     }
 
     #[test]

crates/pep508-rs/src/marker/algebra.rs

@@ -51,6 +51,7 @@ use std::sync::Mutex;
 use std::sync::MutexGuard;
 
 use itertools::Either;
+use pep440_rs::Operator;
 use pep440_rs::{Version, VersionSpecifier};
 use pubgrub::Range;
 use rustc_hash::FxHashMap;
@@ -156,10 +157,21 @@ impl InternerGuard<'_> {
     /// Returns a decision node for a single marker expression.
     pub(crate) fn expression(&mut self, expr: MarkerExpression) -> NodeId {
         let (var, children) = match expr {
+            // Normalize `python_version` markers to `python_full_version` nodes.
+            MarkerExpression::Version {
+                key: MarkerValueVersion::PythonVersion,
+                specifier,
+            } => match python_version_to_full_version(normalize_specifier(specifier)) {
+                Ok(specifier) => (
+                    Variable::Version(MarkerValueVersion::PythonFullVersion),
+                    Edges::from_specifier(specifier),
+                ),
+                Err(node) => return node,
+            },
             // A variable representing the output of a version key. Edges correspond
             // to disjoint version ranges.
             MarkerExpression::Version { key, specifier } => {
-                (Variable::Version(key), Edges::from_specifier(&specifier))
+                (Variable::Version(key), Edges::from_specifier(specifier))
             }
             // The `in` and `contains` operators are a bit different than other operators.
             // In particular, they do not represent a particular value for the corresponding
@@ -532,18 +544,9 @@ impl Edges {
     }
 
     /// Returns the [`Edges`] for a version specifier.
-    fn from_specifier(specifier: &VersionSpecifier) -> Edges {
-        // The decision diagram relies on the assumption that the negation of a marker tree is
-        // the complement of the marker space. However, pre-release versions violate this assumption.
-        // For example, the marker `python_full_version > '3.9' or python_full_version <= '3.9'`
-        // does not match `python_full_version == 3.9.0a0`. However, it's negation,
-        // `python_full_version > '3.9' and python_full_version <= '3.9'` also does not include
-        // `3.9.0a0`, and is actually `false`.
-        //
-        // For this reason we ignore pre-release versions entirely when evaluating markers.
-        // Note that `python_version` cannot take on pre-release values so this is necessary for
-        // simplifying ranges, but for `python_full_version` this decision is a semantic change.
-        let specifier = PubGrubSpecifier::from_release_specifier(specifier).unwrap();
+    fn from_specifier(specifier: VersionSpecifier) -> Edges {
+        let specifier =
+            PubGrubSpecifier::from_release_specifier(&normalize_specifier(specifier)).unwrap();
         Edges::Version {
             edges: Edges::from_range(&specifier.into()),
         }
@@ -748,6 +751,110 @@ impl Edges {
     }
 }
 
+// Normalize a [`VersionSpecifier`] before adding it to the tree.
+fn normalize_specifier(specifier: VersionSpecifier) -> VersionSpecifier {
+    let (operator, version) = specifier.into_parts();
+
+    // The decision diagram relies on the assumption that the negation of a marker tree is
+    // the complement of the marker space. However, pre-release versions violate this assumption.
+    // For example, the marker `python_full_version > '3.9' or python_full_version <= '3.9'`
+    // does not match `python_full_version == 3.9.0a0`. However, it's negation,
+    // `python_full_version > '3.9' and python_full_version <= '3.9'` also does not include
+    // `3.9.0a0`, and is actually `false`.
+    //
+    // For this reason we ignore pre-release versions entirely when evaluating markers.
+    // Note that `python_version` cannot take on pre-release values so this is necessary for
+    // simplifying ranges, but for `python_full_version` this decision is a semantic change.
+    let mut release = version.release();
+
+    // Strip any trailing `0`s.
+    //
+    // The [`Version`] type ignores trailing `0`s for equality, but still preserves them in it's
+    // [`Display`] output. We must normalize all versions by stripping trailing `0`s to remove the
+    // distinction between versions like `3.9` and `3.9.0`, whose output will depend on which form
+    // was added to the global marker interner first.
+    //
+    // Note that we cannot strip trailing `0`s for star equality, as `==3.0.*` is different from `==3.*`.
+    if !operator.is_star() {
+        if let Some(end) = release.iter().rposition(|segment| *segment != 0) {
+            if end > 0 {
+                release = &release[..=end];
+            }
+        }
+    }
+
+    VersionSpecifier::from_version(operator, Version::new(release)).unwrap()
+}
+
+/// Returns the equivalent `python_full_version` specifier for a `python_version` comparison.
+///
+/// Returns `Err` with a constant node if the equivalent comparison is always `true` or `false`.
+fn python_version_to_full_version(specifier: VersionSpecifier) -> Result<VersionSpecifier, NodeId> {
+    let major_minor = match *specifier.version().release() {
+        // `python_version == 3.*` is equivalent to `python_full_version == 3.*`
+        // and adding a trailing `0` would be incorrect.
+        [_major] if specifier.operator().is_star() => return Ok(specifier),
+        // Note that `python_version == 3` matches `3.0.1`, `3.0.2`, etc.
+        [major] => Some((major, 0)),
+        [major, minor] => Some((major, minor)),
+        _ => None,
+    };
+
+    if let Some((major, minor)) = major_minor {
+        let version = Version::new([major, minor]);
+
+        Ok(match specifier.operator() {
+            // `python_version == 3.7` is equivalent to `python_full_version == 3.7.*`.
+            Operator::Equal | Operator::ExactEqual => {
+                VersionSpecifier::equals_star_version(version)
+            }
+            // `python_version != 3.7` is equivalent to `python_full_version != 3.7.*`.
+            Operator::NotEqual => VersionSpecifier::not_equals_star_version(version),
+
+            // `python_version > 3.7` is equivalent to `python_full_version >= 3.8`.
+            Operator::GreaterThan => {
+                VersionSpecifier::greater_than_equal_version(Version::new([major, minor + 1]))
+            }
+            // `python_version < 3.7` is equivalent to `python_full_version < 3.7`.
+            Operator::LessThan => specifier,
+            // `python_version >= 3.7` is equivalent to `python_full_version >= 3.7`.
+            Operator::GreaterThanEqual => specifier,
+            // `python_version <= 3.7` is equivalent to `python_full_version < 3.8`.
+            Operator::LessThanEqual => {
+                VersionSpecifier::less_than_version(Version::new([major, minor + 1]))
+            }
+
+            // `==3.7.*`, `!=3.7.*`, `~=3.7` already represent the equivalent `python_full_version`
+            // comparison.
+            Operator::EqualStar | Operator::NotEqualStar | Operator::TildeEqual => specifier,
+        })
+    } else {
+        let &[major, minor, ..] = specifier.version().release() else {
+            unreachable!()
+        };
+
+        Ok(match specifier.operator() {
+            // `python_version` cannot have more than two release segments, so equality is impossible.
+            Operator::Equal | Operator::ExactEqual | Operator::EqualStar | Operator::TildeEqual => {
+                return Err(NodeId::FALSE)
+            }
+
+            // Similarly, inequalities are always `true`.
+            Operator::NotEqual | Operator::NotEqualStar => return Err(NodeId::TRUE),
+
+            // `python_version {<,<=} 3.7.8` is equivalent to `python_full_version < 3.8`.
+            Operator::LessThan | Operator::LessThanEqual => {
+                VersionSpecifier::less_than_version(Version::new([major, minor + 1]))
+            }
+
+            // `python_version {>,>=} 3.7.8` is equivalent to `python_full_version >= 3.8`.
+            Operator::GreaterThan | Operator::GreaterThanEqual => {
+                VersionSpecifier::greater_than_equal_version(Version::new([major, minor + 1]))
+            }
+        })
+    }
+}
+
 /// Compares the start of two ranges that are known to be disjoint.
 fn compare_disjoint_range_start<T>(range1: &Range<T>, range2: &Range<T>) -> Ordering
 where

crates/pep508-rs/src/marker/simplify.rs

@@ -3,7 +3,7 @@ use std::ops::Bound;
 
 use indexmap::IndexMap;
 use itertools::Itertools;
-use pep440_rs::VersionSpecifier;
+use pep440_rs::{Version, VersionSpecifier};
 use pubgrub::Range;
 use rustc_hash::FxBuildHasher;
 
@@ -61,9 +61,21 @@ fn collect_dnf(
                     continue;
                 }
 
+                // Detect whether the range for this edge can be simplified as a star inequality.
+                if let Some(specifier) = star_range_inequality(&range) {
+                    path.push(MarkerExpression::Version {
+                        key: marker.key().clone(),
+                        specifier,
+                    });
+
+                    collect_dnf(&tree, dnf, path);
+                    path.pop();
+                    continue;
+                }
+
                 for bounds in range.iter() {
                     let current = path.len();
-                    for specifier in VersionSpecifier::from_bounds(bounds) {
+                    for specifier in VersionSpecifier::from_release_only_bounds(bounds) {
                         path.push(MarkerExpression::Version {
                             key: marker.key().clone(),
                             specifier,
@@ -307,7 +319,7 @@ where
 /// Returns `Some` if the expression can be simplified as an inequality consisting
 /// of the given values.
 ///
-/// For example, `os_name < 'Linux'` and `os_name > 'Linux'` can be simplified to
+/// For example, `os_name < 'Linux' or os_name > 'Linux'` can be simplified to
 /// `os_name != 'Linux'`.
 fn range_inequality<T>(range: &Range<T>) -> Option<Vec<&T>>
 where
@@ -328,6 +340,25 @@ where
     Some(excluded)
 }
 
+/// Returns `Some` if the version expression can be simplified as a star inequality with the given
+/// specifier.
+///
+/// For example, `python_full_version < '3.8' or python_full_version >= '3.9'` can be simplified to
+/// `python_full_version != '3.8.*'`.
+fn star_range_inequality(range: &Range<Version>) -> Option<VersionSpecifier> {
+    let (b1, b2) = range.iter().collect_tuple()?;
+
+    match (b1, b2) {
+        ((Bound::Unbounded, Bound::Excluded(v1)), (Bound::Included(v2), Bound::Unbounded))
+            if v1.release().len() == 2
+                && v2.release() == [v1.release()[0], v1.release()[1] + 1] =>
+        {
+            Some(VersionSpecifier::not_equals_star_version(v1.clone()))
+        }
+        _ => None,
+    }
+}
+
 /// Returns `true` if the LHS is the negation of the RHS, or vice versa.
 fn is_negation(left: &MarkerExpression, right: &MarkerExpression) -> bool {
     match left {