improve tests for version parser (#696)

The high level goal here is to improve the tests for the version parser.
Namely, we now check not just that version strings parse successfully,
but that they parse to the expected result.

We also do a few other cleanups. Most notably, `Version` is now an
opaque type so that we can more easily change its representation going
forward.

Reviewing commit-by-commit is suggested. :-)

crates/pep440-rs/src/lib.rs

@@ -42,14 +42,15 @@
 //!   the version matching needs to catch all sorts of special cases
 #![deny(missing_docs)]
 
+pub use {
+    version::{LocalSegment, Operator, PreRelease, Version},
+    version_specifier::{parse_version_specifiers, VersionSpecifier, VersionSpecifiers},
+};
+
 #[cfg(feature = "pyo3")]
 use pyo3::{pymodule, types::PyModule, PyResult, Python};
-use std::error::Error;
-use std::fmt::{Display, Formatter};
 #[cfg(feature = "pyo3")]
 pub use version::PyVersion;
-pub use version::{LocalSegment, Operator, PreRelease, Version};
-pub use version_specifier::{parse_version_specifiers, VersionSpecifier, VersionSpecifiers};
 
 mod version;
 mod version_specifier;
@@ -67,8 +68,8 @@ pub struct Pep440Error {
     pub width: usize,
 }
 
-impl Display for Pep440Error {
-    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+impl std::fmt::Display for Pep440Error {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         writeln!(f, "Failed to parse version:")?;
         writeln!(f, "{}", self.line)?;
         writeln!(f, "{}{}", " ".repeat(self.start), "^".repeat(self.width))?;
@@ -76,7 +77,7 @@ impl Display for Pep440Error {
     }
 }
 
-impl Error for Pep440Error {}
+impl std::error::Error for Pep440Error {}
 
 /// Python bindings shipped as `pep440_rs`
 #[cfg(feature = "pyo3")]

crates/pep440-rs/src/version_specifier.rs

@@ -1,6 +1,7 @@
 #[cfg(feature = "pyo3")]
-use crate::version::PyVersion;
-use crate::{version, Operator, Pep440Error, Version};
+use std::hash::{Hash, Hasher};
+use std::{cmp::Ordering, str::FromStr};
+
 #[cfg(feature = "pyo3")]
 use pyo3::{
     exceptions::{PyIndexError, PyNotImplementedError, PyValueError},
@@ -10,20 +11,11 @@ use pyo3::{
 };
 #[cfg(feature = "serde")]
 use serde::{de, Deserialize, Deserializer, Serialize, Serializer};
-use std::cmp::Ordering;
-#[cfg(feature = "pyo3")]
-use std::collections::hash_map::DefaultHasher;
-use std::fmt::Formatter;
-use std::fmt::{Debug, Display};
-#[cfg(feature = "pyo3")]
-use std::hash::{Hash, Hasher};
-use std::ops::Deref;
-use std::str::FromStr;
 use unicode_width::UnicodeWidthStr;
 
-#[cfg(feature = "tracing")]
-use tracing::warn;
-use unscanny::Scanner;
+#[cfg(feature = "pyo3")]
+use crate::version::PyVersion;
+use crate::{version, Operator, Pep440Error, Version};
 
 /// A thin wrapper around `Vec<VersionSpecifier>` with a serde implementation
 ///
@@ -46,7 +38,7 @@ use unscanny::Scanner;
 #[cfg_attr(feature = "pyo3", pyclass(sequence))]
 pub struct VersionSpecifiers(Vec<VersionSpecifier>);
 
-impl Deref for VersionSpecifiers {
+impl std::ops::Deref for VersionSpecifiers {
     type Target = [VersionSpecifier];
 
     fn deref(&self) -> &Self::Target {
@@ -81,8 +73,8 @@ impl From<VersionSpecifier> for VersionSpecifiers {
     }
 }
 
-impl Display for VersionSpecifiers {
-    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+impl std::fmt::Display for VersionSpecifiers {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         for (idx, version_specifier) in self.0.iter().enumerate() {
             // Separate version specifiers by comma, but we need one comma less than there are
             // specifiers
@@ -255,7 +247,7 @@ impl VersionSpecifier {
 
     /// Returns the normalized representation
     pub fn __hash__(&self) -> u64 {
-        let mut hasher = DefaultHasher::new();
+        let mut hasher = std::collections::hash_map::DefaultHasher::new();
         self.hash(&mut hasher);
         hasher.finish()
     }
@@ -289,7 +281,7 @@ impl VersionSpecifier {
     /// parameter indicates a trailing `.*`, to differentiate between `1.1.*` and `1.1`
     pub fn new(operator: Operator, version: Version, star: bool) -> Result<Self, String> {
         // "Local version identifiers are NOT permitted in this version specifier."
-        if let Some(local) = &version.local {
+        if let Some(local) = &version.local() {
             if matches!(
                 operator,
                 Operator::GreaterThan
@@ -327,7 +319,7 @@ impl VersionSpecifier {
             operator
         };
 
-        if operator == Operator::TildeEqual && version.release.len() < 2 {
+        if operator == Operator::TildeEqual && version.release().len() < 2 {
             return Err(
                 "The ~= operator requires at least two parts in the release version".to_string(),
             );
@@ -379,39 +371,42 @@ impl VersionSpecifier {
         // "Except where specifically noted below, local version identifiers MUST NOT be permitted
         // in version specifiers, and local version labels MUST be ignored entirely when checking
         // if candidate versions match a given version specifier."
-        let (this, other) = if self.version.local.is_some() {
+        let (this, other) = if self.version.local().is_some() {
             (self.version.clone(), version.clone())
         } else {
             // self is already without local
-            (self.version.without_local(), version.without_local())
+            (
+                self.version.clone().without_local(),
+                version.clone().without_local(),
+            )
         };
 
         match self.operator {
             Operator::Equal => other == this,
             Operator::EqualStar => {
-                this.epoch == other.epoch
+                this.epoch() == other.epoch()
                     && self
                         .version
-                        .release
+                        .release()
                         .iter()
-                        .zip(&other.release)
+                        .zip(other.release())
                         .all(|(this, other)| this == other)
             }
             #[allow(deprecated)]
             Operator::ExactEqual => {
                 #[cfg(feature = "tracing")]
                 {
-                    warn!("Using arbitrary equality (`===`) is discouraged");
+                    tracing::warn!("Using arbitrary equality (`===`) is discouraged");
                 }
                 self.version.to_string() == version.to_string()
             }
             Operator::NotEqual => other != this,
             Operator::NotEqualStar => {
-                this.epoch != other.epoch
+                this.epoch() != other.epoch()
                     || !this
-                        .release
+                        .release()
                         .iter()
-                        .zip(&version.release)
+                        .zip(version.release())
                         .all(|(this, other)| this == other)
             }
             Operator::TildeEqual => {
@@ -419,14 +414,14 @@ impl VersionSpecifier {
                 // approximately equivalent to the pair of comparison clauses: `>= V.N, == V.*`"
                 // First, we test that every but the last digit matches.
                 // We know that this must hold true since we checked it in the constructor
-                assert!(this.release.len() > 1);
-                if this.epoch != other.epoch {
+                assert!(this.release().len() > 1);
+                if this.epoch() != other.epoch() {
                     return false;
                 }
 
-                if !this.release[..this.release.len() - 1]
+                if !this.release()[..this.release().len() - 1]
                     .iter()
-                    .zip(&other.release)
+                    .zip(other.release())
                     .all(|(this, other)| this == other)
                 {
                     return false;
@@ -440,7 +435,8 @@ impl VersionSpecifier {
             Operator::GreaterThanEqual => Self::greater_than(&this, &other) || other >= this,
             Operator::LessThan => {
                 Self::less_than(&this, &other)
-                    && !(version::compare_release(&this.release, &other.release) == Ordering::Equal
+                    && !(version::compare_release(this.release(), other.release())
+                        == Ordering::Equal
                         && other.any_prerelease())
             }
             Operator::LessThanEqual => Self::less_than(&this, &other) || other <= this,
@@ -448,7 +444,7 @@ impl VersionSpecifier {
     }
 
     fn less_than(this: &Version, other: &Version) -> bool {
-        if other.epoch < this.epoch {
+        if other.epoch() < this.epoch() {
             return true;
         }
 
@@ -458,7 +454,7 @@ impl VersionSpecifier {
         // not match 3.1.dev0, but should match 3.0.dev0).
         if !this.any_prerelease()
             && other.is_pre()
-            && version::compare_release(&this.release, &other.release) == Ordering::Equal
+            && version::compare_release(this.release(), other.release()) == Ordering::Equal
         {
             return false;
         }
@@ -467,11 +463,11 @@ impl VersionSpecifier {
     }
 
     fn greater_than(this: &Version, other: &Version) -> bool {
-        if other.epoch > this.epoch {
+        if other.epoch() > this.epoch() {
             return true;
         }
 
-        if version::compare_release(&this.release, &other.release) == Ordering::Equal {
+        if version::compare_release(this.release(), other.release()) == Ordering::Equal {
             // This special case is here so that, unless the specifier itself
             // includes is a post-release version, that we do not accept
             // post-release versions for the version mentioned in the specifier
@@ -495,7 +491,7 @@ impl FromStr for VersionSpecifier {
 
     /// Parses a version such as `>= 1.19`, `== 1.1.*`,`~=1.0+abc.5` or `<=1!2012.2`
     fn from_str(spec: &str) -> Result<Self, Self::Err> {
-        let mut s = Scanner::new(spec);
+        let mut s = unscanny::Scanner::new(spec);
         s.eat_while(|c: char| c.is_whitespace());
         // operator but we don't know yet if it has a star
         let operator = s.eat_while(['=', '!', '~', '<', '>']);
@@ -518,8 +514,8 @@ impl FromStr for VersionSpecifier {
     }
 }
 
-impl Display for VersionSpecifier {
-    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+impl std::fmt::Display for VersionSpecifier {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         if self.operator == Operator::EqualStar || self.operator == Operator::NotEqualStar {
             return write!(f, "{}{}.*", self.operator, self.version);
         }
@@ -567,11 +563,12 @@ pub fn parse_version_specifiers(spec: &str) -> Result<Vec<VersionSpecifier>, Pep
 }
 
 #[cfg(test)]
-mod test {
-    use crate::{Operator, Version, VersionSpecifier, VersionSpecifiers};
+mod tests {
+    use std::{cmp::Ordering, str::FromStr};
+
     use indoc::indoc;
-    use std::cmp::Ordering;
-    use std::str::FromStr;
+
+    use crate::{Operator, Version, VersionSpecifier, VersionSpecifiers};
 
     /// <https://peps.python.org/pep-0440/#version-matching>
     #[test]
@@ -1062,47 +1059,19 @@ mod test {
             [
                 VersionSpecifier {
                     operator: Operator::TildeEqual,
-                    version: Version {
-                        epoch: 0,
-                        release: vec![0, 9],
-                        pre: None,
-                        post: None,
-                        dev: None,
-                        local: None
-                    }
+                    version: Version::new([0, 9]),
                 },
                 VersionSpecifier {
                     operator: Operator::GreaterThanEqual,
-                    version: Version {
-                        epoch: 0,
-                        release: vec![1, 0],
-                        pre: None,
-                        post: None,
-                        dev: None,
-                        local: None
-                    }
+                    version: Version::new([1, 0]),
                 },
                 VersionSpecifier {
                     operator: Operator::NotEqualStar,
-                    version: Version {
-                        epoch: 0,
-                        release: vec![1, 3, 4],
-                        pre: None,
-                        post: None,
-                        dev: None,
-                        local: None
-                    }
+                    version: Version::new([1, 3, 4]),
                 },
                 VersionSpecifier {
                     operator: Operator::LessThan,
-                    version: Version {
-                        epoch: 0,
-                        release: vec![2, 0],
-                        pre: None,
-                        post: None,
-                        dev: None,
-                        local: None
-                    }
+                    version: Version::new([2, 0]),
                 }
             ]
         );

crates/pep508-rs/src/lib.rs

@@ -977,30 +977,11 @@ mod tests {
                 [
                     VersionSpecifier::new(
                         Operator::GreaterThanEqual,
-                        Version {
-                            epoch: 0,
-                            release: vec![2, 8, 1],
-                            pre: None,
-                            post: None,
-                            dev: None,
-                            local: None,
-                        },
+                        Version::new([2, 8, 1]),
                         false,
                     )
                     .unwrap(),
-                    VersionSpecifier::new(
-                        Operator::Equal,
-                        Version {
-                            epoch: 0,
-                            release: vec![2, 8],
-                            pre: None,
-                            post: None,
-                            dev: None,
-                            local: None,
-                        },
-                        true,
-                    )
-                    .unwrap(),
+                    VersionSpecifier::new(Operator::Equal, Version::new([2, 8]), true).unwrap(),
                 ]
                 .into_iter()
                 .collect(),

crates/pep508-rs/src/marker.rs

@@ -715,14 +715,14 @@ impl MarkerExpression {
     ///
     /// # fn main() -> Result<(), Pep508Error> {
     /// let marker_tree = MarkerTree::from_str(r#"("linux" in sys_platform) and extra == 'day'"#)?;
-    /// let versions: Vec<Version> = (8..12).map(|minor| Version::from_release(vec![3, minor])).collect();
+    /// let versions: Vec<Version> = (8..12).map(|minor| Version::new([3, minor])).collect();
     /// assert!(marker_tree.evaluate_extras_and_python_version(&["day".to_string()].into(), &versions));
     /// assert!(!marker_tree.evaluate_extras_and_python_version(&["night".to_string()].into(), &versions));
     ///
     /// let marker_tree = MarkerTree::from_str(r#"extra == 'day' and python_version < '3.11' and '3.10' <= python_version"#)?;
-    /// assert!(!marker_tree.evaluate_extras_and_python_version(&["day".to_string()].into(), &vec![Version::from_release(vec![3, 9])]));
-    /// assert!(marker_tree.evaluate_extras_and_python_version(&["day".to_string()].into(), &vec![Version::from_release(vec![3, 10])]));
-    /// assert!(!marker_tree.evaluate_extras_and_python_version(&["day".to_string()].into(), &vec![Version::from_release(vec![3, 11])]));
+    /// assert!(!marker_tree.evaluate_extras_and_python_version(&["day".to_string()].into(), &vec![Version::new([3, 9])]));
+    /// assert!(marker_tree.evaluate_extras_and_python_version(&["day".to_string()].into(), &vec![Version::new([3, 10])]));
+    /// assert!(!marker_tree.evaluate_extras_and_python_version(&["day".to_string()].into(), &vec![Version::new([3, 11])]));
     /// # Ok(())
     /// # }
     /// ```
@@ -1556,7 +1556,7 @@ mod test {
     fn test_marker_environment_from_json() {
         let _env: MarkerEnvironment = serde_json::from_str(
             r##"{
-                "implementation_name": "cpython", 
+                "implementation_name": "cpython",
                 "implementation_version": "3.7.13",
                 "os_name": "posix",
                 "platform_machine": "x86_64",

crates/puffin-cli/src/python_version.rs

@@ -18,18 +18,18 @@ impl FromStr for PythonVersion {
         if version.is_local() {
             return Err(format!("Python version {s} is a local version"));
         }
-        if version.epoch != 0 {
+        if version.epoch() != 0 {
             return Err(format!("Python version {s} has a non-zero epoch"));
         }
-        if version.version < Version::from_release(vec![3, 7]) {
+        if version.version < Version::new([3, 7]) {
             return Err(format!("Python version {s} must be >= 3.7"));
         }
-        if version.version >= Version::from_release(vec![4, 0]) {
+        if version.version >= Version::new([4, 0]) {
             return Err(format!("Python version {s} must be < 4.0"));
         }
 
         // If the version lacks a patch, assume the most recent known patch for that minor version.
-        match version.release.as_slice() {
+        match version.release() {
             [3, 7] => {
                 debug!("Assuming Python 3.7.17");
                 Ok(Self(StringVersion::from_str("3.7.17")?))

crates/puffin-interpreter/src/interpreter.rs

@@ -82,8 +82,8 @@ impl Interpreter {
     /// Returns the Python version as a simple tuple.
     pub fn simple_version(&self) -> (u8, u8) {
         (
-            u8::try_from(self.version().release[0]).expect("invalid major version"),
-            u8::try_from(self.version().release[1]).expect("invalid minor version"),
+            u8::try_from(self.version().release()[0]).expect("invalid major version"),
+            u8::try_from(self.version().release()[1]).expect("invalid minor version"),
         )
     }
 

crates/puffin-resolver/src/pubgrub/specifier.rs

@@ -36,21 +36,14 @@ impl TryFrom<&VersionSpecifier> for PubGrubSpecifier {
                 Range::singleton(version).complement()
             }
             Operator::TildeEqual => {
-                let [rest @ .., last, _] = specifier.version().release.as_slice() else {
+                let [rest @ .., last, _] = specifier.version().release() else {
                     return Err(ResolveError::InvalidTildeEquals(specifier.clone()));
                 };
-                let upper = PubGrubVersion::from(pep440_rs::Version {
-                    dev: Some(0),
-                    epoch: specifier.version().epoch,
-                    local: None,
-                    post: None,
-                    pre: None,
-                    release: rest
-                        .iter()
-                        .chain(std::iter::once(&(last + 1)))
-                        .copied()
-                        .collect(),
-                });
+                let upper = PubGrubVersion::from(
+                    pep440_rs::Version::new(rest.iter().chain([&(last + 1)]))
+                        .with_epoch(specifier.version().epoch())
+                        .with_dev(Some(0)),
+                );
                 let version = PubGrubVersion::from(specifier.version().clone());
                 Range::from_range_bounds(version..upper)
             }
@@ -65,14 +58,6 @@ 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."
-                let mut low = specifier.version().clone();
-                if let Some(dev) = low.dev {
-                    low.dev = Some(dev + 1);
-                } else if let Some(post) = low.post {
-                    low.post = Some(post + 1);
-                } else {
-                    low.post = Some(u64::MAX);
-                }
                 let version = PubGrubVersion::from(specifier.version().clone());
                 Range::strictly_higher_than(version)
             }
@@ -81,46 +66,38 @@ impl TryFrom<&VersionSpecifier> for PubGrubSpecifier {
                 Range::higher_than(version)
             }
             Operator::EqualStar => {
-                let low = pep440_rs::Version {
-                    dev: Some(0),
-                    ..specifier.version().clone()
-                };
-                let mut high = pep440_rs::Version {
-                    dev: Some(0),
-                    ..specifier.version().clone()
-                };
-                if let Some(post) = high.post {
-                    high.post = Some(post + 1);
-                } else if let Some(pre) = high.pre {
-                    high.pre = Some(match pre {
+                let low = specifier.version().clone().with_dev(Some(0));
+                let mut high = low.clone();
+                if let Some(post) = high.post() {
+                    high = high.with_post(Some(post + 1));
+                } else if let Some(pre) = high.pre() {
+                    high = high.with_pre(Some(match pre {
                         (pep440_rs::PreRelease::Rc, n) => (pep440_rs::PreRelease::Rc, n + 1),
                         (pep440_rs::PreRelease::Alpha, n) => (pep440_rs::PreRelease::Alpha, n + 1),
                         (pep440_rs::PreRelease::Beta, n) => (pep440_rs::PreRelease::Beta, n + 1),
-                    });
+                    }));
                 } else {
-                    *high.release.last_mut().unwrap() += 1;
+                    let mut release = high.release().to_vec();
+                    *release.last_mut().unwrap() += 1;
+                    high = high.with_release(release);
                 }
                 Range::from_range_bounds(PubGrubVersion::from(low)..PubGrubVersion::from(high))
             }
             Operator::NotEqualStar => {
-                let low = pep440_rs::Version {
-                    dev: Some(0),
-                    ..specifier.version().clone()
-                };
-                let mut high = pep440_rs::Version {
-                    dev: Some(0),
-                    ..specifier.version().clone()
-                };
-                if let Some(post) = high.post {
-                    high.post = Some(post + 1);
-                } else if let Some(pre) = high.pre {
-                    high.pre = Some(match pre {
+                let low = specifier.version().clone().with_dev(Some(0));
+                let mut high = low.clone();
+                if let Some(post) = high.post() {
+                    high = high.with_post(Some(post + 1));
+                } else if let Some(pre) = high.pre() {
+                    high = high.with_pre(Some(match pre {
                         (pep440_rs::PreRelease::Rc, n) => (pep440_rs::PreRelease::Rc, n + 1),
                         (pep440_rs::PreRelease::Alpha, n) => (pep440_rs::PreRelease::Alpha, n + 1),
                         (pep440_rs::PreRelease::Beta, n) => (pep440_rs::PreRelease::Beta, n + 1),
-                    });
+                    }));
                 } else {
-                    *high.release.last_mut().unwrap() += 1;
+                    let mut release = high.release().to_vec();
+                    *release.last_mut().unwrap() += 1;
+                    high = high.with_release(release);
                 }
                 Range::from_range_bounds(PubGrubVersion::from(low)..PubGrubVersion::from(high))
                     .complement()

crates/puffin-resolver/src/pubgrub/version.rs

@@ -10,13 +10,12 @@ impl PubGrubVersion {
     /// Returns the smallest PEP 440 version that is larger than `self`.
     pub fn next(&self) -> PubGrubVersion {
         let mut next = self.clone();
-        if let Some(dev) = &mut next.0.dev {
-            *dev += 1;
-        } else if let Some(post) = &mut next.0.post {
-            *post += 1;
+        if let Some(dev) = next.0.dev() {
+            next.0 = next.0.with_dev(Some(dev + 1));
+        } else if let Some(post) = next.0.post() {
+            next.0 = next.0.with_post(Some(post + 1));
         } else {
-            next.0.post = Some(0);
-            next.0.dev = Some(0);
+            next.0 = next.0.with_post(Some(0)).with_dev(Some(0));
         }
         next
     }