From d7c9b151fbf5c6a7ec2a80d14be6655d742cef43 Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Thu, 4 Jan 2024 12:28:36 -0500 Subject: [PATCH] pep440: some minor refactoring, mostly around error types (#780) This PR does a bit of refactoring to the pep440 crate, and in particular around the erorr types. This PR is meant to be a precursor to another PR that does some surgery (both in parsing and in `Version` representation) that benefits somewhat from this refactoring. As usual, please review commit-by-commit. --- crates/pep440-rs/src/lib.rs | 28 +- crates/pep440-rs/src/version.rs | 369 +++++++----- crates/pep440-rs/src/version_specifier.rs | 555 ++++++++++++++++--- crates/pep508-rs/src/lib.rs | 8 +- crates/pep508-rs/src/marker.rs | 2 +- crates/puffin-client/src/html.rs | 2 +- crates/pypi-types/src/lenient_requirement.rs | 4 +- crates/pypi-types/src/metadata.rs | 4 +- 8 files changed, 710 insertions(+), 262 deletions(-) diff --git a/crates/pep440-rs/src/lib.rs b/crates/pep440-rs/src/lib.rs index 839b751cb..3a9540d48 100644 --- a/crates/pep440-rs/src/lib.rs +++ b/crates/pep440-rs/src/lib.rs @@ -44,7 +44,9 @@ pub use { version::{LocalSegment, Operator, PreRelease, Version}, - version_specifier::{parse_version_specifiers, VersionSpecifier, VersionSpecifiers}, + version_specifier::{ + parse_version_specifiers, VersionSpecifier, VersionSpecifiers, VersionSpecifiersParseError, + }, }; #[cfg(feature = "pyo3")] @@ -55,30 +57,6 @@ pub use version::PyVersion; mod version; mod version_specifier; -/// Error with span information (unicode width) inside the parsed line -#[derive(Debug, Eq, PartialEq, Clone)] -pub struct Pep440Error { - /// The actual error message - pub message: String, - /// The string that failed to parse - pub line: String, - /// First character for underlining (unicode width) - pub start: usize, - /// Number of characters to underline (unicode width) - pub width: usize, -} - -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))?; - Ok(()) - } -} - -impl std::error::Error for Pep440Error {} - /// Python bindings shipped as `pep440_rs` #[cfg(feature = "pyo3")] #[pymodule] diff --git a/crates/pep440-rs/src/version.rs b/crates/pep440-rs/src/version.rs index 964178dd7..4a94fec0c 100644 --- a/crates/pep440-rs/src/version.rs +++ b/crates/pep440-rs/src/version.rs @@ -56,7 +56,7 @@ static VERSION_RE: Lazy = Lazy::new(|| Regex::new(&format!(r#"(?xi)^(?:\s*){VERSION_RE_INNER}(?:\s*)$"#)).unwrap()); /// One of `~=` `==` `!=` `<=` `>=` `<` `>` `===` -#[derive(Eq, PartialEq, Debug, Hash, Clone)] +#[derive(Eq, PartialEq, Debug, Hash, Clone, Copy)] #[cfg_attr(feature = "pyo3", pyclass)] pub enum Operator { /// `== 1.2.3` @@ -86,6 +86,41 @@ pub enum Operator { GreaterThanEqual, } +impl Operator { + /// Returns true if and only if this operator can be used in a version + /// specifier with a version containing a non-empty local segment. + /// + /// Specifically, this comes from the "Local version identifiers are + /// NOT permitted in this version specifier." phrasing in the version + /// specifiers [spec]. + /// + /// [spec]: https://packaging.python.org/en/latest/specifications/version-specifiers/ + pub(crate) fn is_local_compatible(&self) -> bool { + !matches!( + *self, + Operator::GreaterThan + | Operator::GreaterThanEqual + | Operator::LessThan + | Operator::LessThanEqual + | Operator::TildeEqual + | Operator::EqualStar + | Operator::NotEqualStar + ) + } + + /// Returns the wildcard version of this operator, if appropriate. + /// + /// This returns `None` when this operator doesn't have an analogous + /// wildcard operator. + pub(crate) fn to_star(self) -> Option { + match self { + Operator::Equal => Some(Operator::EqualStar), + Operator::NotEqual => Some(Operator::NotEqualStar), + _ => None, + } + } +} + impl FromStr for Operator { type Err = String; @@ -152,96 +187,67 @@ impl Operator { } } -/// Optional prerelease modifier (alpha, beta or release candidate) appended to version -/// -/// -#[derive(PartialEq, Eq, Debug, Hash, Clone, Copy, Ord, PartialOrd)] -#[cfg_attr(feature = "pyo3", pyclass)] -pub enum PreRelease { - /// alpha prerelease - Alpha, - /// beta prerelease - Beta, - /// release candidate prerelease - Rc, -} - -impl FromStr for PreRelease { - type Err = String; - - fn from_str(prerelease: &str) -> Result { - match prerelease.to_lowercase().as_str() { - "a" | "alpha" => Ok(Self::Alpha), - "b" | "beta" => Ok(Self::Beta), - "c" | "rc" | "pre" | "preview" => Ok(Self::Rc), - _ => Err(format!( - "'{prerelease}' isn't recognized as alpha, beta or release candidate", - )), - } - } -} - -impl std::fmt::Display for PreRelease { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - Self::Alpha => write!(f, "a"), - Self::Beta => write!(f, "b"), - Self::Rc => write!(f, "rc"), - } - } -} - -/// A part of the [local version identifier]() -/// -/// Local versions are a mess: -/// -/// > Comparison and ordering of local versions considers each segment of the local version -/// > (divided by a .) separately. If a segment consists entirely of ASCII digits then that section -/// > should be considered an integer for comparison purposes and if a segment contains any ASCII -/// > letters then that segment is compared lexicographically with case insensitivity. When -/// > comparing a numeric and lexicographic segment, the numeric section always compares as greater -/// > than the lexicographic segment. Additionally a local version with a great number of segments -/// > will always compare as greater than a local version with fewer segments, as long as the -/// > shorter local version’s segments match the beginning of the longer local version’s segments -/// > exactly. -/// -/// Luckily the default `Ord` implementation for `Vec` matches the PEP 440 rules. -#[derive(Eq, PartialEq, Debug, Clone, Hash)] -pub enum LocalSegment { - /// Not-parseable as integer segment of local version - String(String), - /// Inferred integer segment of local version - Number(u64), -} - -impl std::fmt::Display for LocalSegment { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - Self::String(string) => write!(f, "{string}"), - Self::Number(number) => write!(f, "{number}"), - } - } -} - -impl PartialOrd for LocalSegment { - fn partial_cmp(&self, other: &Self) -> Option { - Some(self.cmp(other)) - } -} - -impl FromStr for LocalSegment { - /// This can be a never type when stabilized - type Err = (); - - fn from_str(segment: &str) -> Result { - Ok(if let Ok(number) = segment.parse::() { - Self::Number(number) - } else { - // "and if a segment contains any ASCII letters then that segment is compared lexicographically with case insensitivity" - Self::String(segment.to_lowercase()) - }) - } -} +// NOTE: I did a little bit of experimentation to determine what most version +// numbers actually look like. The idea here is that if we know what most look +// like, then we can optimize our representation for the common case, while +// falling back to something more complete for any cases that fall outside of +// that. +// +// The experiment downloaded PyPI's distribution metadata from Google BigQuery, +// and then counted the number of versions with various qualities: +// +// total: 11264078 +// release counts: +// 01: 51204 (0.45%) +// 02: 754520 (6.70%) +// 03: 9757602 (86.63%) +// 04: 527403 (4.68%) +// 05: 77994 (0.69%) +// 06: 91346 (0.81%) +// 07: 1421 (0.01%) +// 08: 205 (0.00%) +// 09: 72 (0.00%) +// 10: 2297 (0.02%) +// 11: 5 (0.00%) +// 12: 2 (0.00%) +// 13: 4 (0.00%) +// 20: 2 (0.00%) +// 39: 1 (0.00%) +// JUST release counts: +// 01: 48297 (0.43%) +// 02: 604692 (5.37%) +// 03: 8460917 (75.11%) +// 04: 465354 (4.13%) +// 05: 49293 (0.44%) +// 06: 25909 (0.23%) +// 07: 1413 (0.01%) +// 08: 192 (0.00%) +// 09: 72 (0.00%) +// 10: 2292 (0.02%) +// 11: 5 (0.00%) +// 12: 2 (0.00%) +// 13: 4 (0.00%) +// 20: 2 (0.00%) +// 39: 1 (0.00%) +// non-zero epochs: 1902 (0.02%) +// pre-releases: 752184 (6.68%) +// post-releases: 134383 (1.19%) +// dev-releases: 765099 (6.79%) +// locals: 1 (0.00%) +// fitsu8: 10388430 (92.23%) +// sweetspot: 10236089 (90.87%) +// +// The "JUST release counts" corresponds to versions that only have a release +// component and nothing else. The "fitsu8" property indicates that all numbers +// (except for local numeric segments) fit into `u8`. The "sweetspot" property +// consists of any version number with no local part, 4 or fewer parts in the +// release version and *all* numbers fit into a u8. +// +// This somewhat confirms what one might expect: the vast majority of versions +// (75%) are precisely in the format of `x.y.z`. That is, a version with only a +// release version of 3 components. +// +// ---AG /// A version number such as `1.2.3` or `4!5.6.7-a8.post9.dev0`. /// @@ -290,29 +296,6 @@ pub struct Version { local: Option>, } -/// -#[cfg(feature = "serde")] -impl<'de> Deserialize<'de> for Version { - fn deserialize(deserializer: D) -> Result - where - D: Deserializer<'de>, - { - let s = String::deserialize(deserializer)?; - FromStr::from_str(&s).map_err(de::Error::custom) - } -} - -/// -#[cfg(feature = "serde")] -impl Serialize for Version { - fn serialize(&self, serializer: S) -> Result - where - S: Serializer, - { - serializer.collect_str(self) - } -} - impl Version { /// Create a new version from an iterator of segments in the release part /// of a version. @@ -584,6 +567,29 @@ impl Version { } } +/// +#[cfg(feature = "serde")] +impl<'de> Deserialize<'de> for Version { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let s = String::deserialize(deserializer)?; + FromStr::from_str(&s).map_err(de::Error::custom) + } +} + +/// +#[cfg(feature = "serde")] +impl Serialize for Version { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + serializer.collect_str(self) + } +} + /// Shows normalized version impl std::fmt::Display for Version { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { @@ -691,18 +697,6 @@ impl Ord for Version { } } -impl Ord for LocalSegment { - fn cmp(&self, other: &Self) -> Ordering { - // - match (self, other) { - (Self::Number(n1), Self::Number(n2)) => n1.cmp(n2), - (Self::String(s1), Self::String(s2)) => s1.cmp(s2), - (Self::Number(_), Self::String(_)) => Ordering::Greater, - (Self::String(_), Self::Number(_)) => Ordering::Less, - } - } -} - impl FromStr for Version { type Err = String; @@ -726,6 +720,109 @@ impl FromStr for Version { } } +/// Optional prerelease modifier (alpha, beta or release candidate) appended to version +/// +/// +#[derive(PartialEq, Eq, Debug, Hash, Clone, Copy, Ord, PartialOrd)] +#[cfg_attr(feature = "pyo3", pyclass)] +pub enum PreRelease { + /// alpha prerelease + Alpha, + /// beta prerelease + Beta, + /// release candidate prerelease + Rc, +} + +impl FromStr for PreRelease { + type Err = String; + + fn from_str(prerelease: &str) -> Result { + match prerelease.to_lowercase().as_str() { + "a" | "alpha" => Ok(Self::Alpha), + "b" | "beta" => Ok(Self::Beta), + "c" | "rc" | "pre" | "preview" => Ok(Self::Rc), + _ => Err(format!( + "'{prerelease}' isn't recognized as alpha, beta or release candidate", + )), + } + } +} + +impl std::fmt::Display for PreRelease { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Alpha => write!(f, "a"), + Self::Beta => write!(f, "b"), + Self::Rc => write!(f, "rc"), + } + } +} + +/// A part of the [local version identifier]() +/// +/// Local versions are a mess: +/// +/// > Comparison and ordering of local versions considers each segment of the local version +/// > (divided by a .) separately. If a segment consists entirely of ASCII digits then that section +/// > should be considered an integer for comparison purposes and if a segment contains any ASCII +/// > letters then that segment is compared lexicographically with case insensitivity. When +/// > comparing a numeric and lexicographic segment, the numeric section always compares as greater +/// > than the lexicographic segment. Additionally a local version with a great number of segments +/// > will always compare as greater than a local version with fewer segments, as long as the +/// > shorter local version’s segments match the beginning of the longer local version’s segments +/// > exactly. +/// +/// Luckily the default `Ord` implementation for `Vec` matches the PEP 440 rules. +#[derive(Eq, PartialEq, Debug, Clone, Hash)] +pub enum LocalSegment { + /// Not-parseable as integer segment of local version + String(String), + /// Inferred integer segment of local version + Number(u64), +} + +impl std::fmt::Display for LocalSegment { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::String(string) => write!(f, "{string}"), + Self::Number(number) => write!(f, "{number}"), + } + } +} + +impl PartialOrd for LocalSegment { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl FromStr for LocalSegment { + /// This can be a never type when stabilized + type Err = (); + + fn from_str(segment: &str) -> Result { + Ok(if let Ok(number) = segment.parse::() { + Self::Number(number) + } else { + // "and if a segment contains any ASCII letters then that segment is compared lexicographically with case insensitivity" + Self::String(segment.to_lowercase()) + }) + } +} + +impl Ord for LocalSegment { + fn cmp(&self, other: &Self) -> Ordering { + // + match (self, other) { + (Self::Number(n1), Self::Number(n2)) => n1.cmp(n2), + (Self::String(s1), Self::String(s2)) => s1.cmp(s2), + (Self::Number(_), Self::String(_)) => Ordering::Greater, + (Self::String(_), Self::Number(_)) => Ordering::Less, + } + } +} + /// Workaround for #[cfg(feature = "pyo3")] #[derive(Clone, Debug)] @@ -739,7 +836,7 @@ impl PyVersion { /// but you can increment it if you switched the versioning scheme. #[getter] pub fn epoch(&self) -> u64 { - self.0.epoch + self.0.epoch() } /// The normal number part of the version /// (["final release"](https://peps.python.org/pep-0440/#final-releases)), @@ -748,7 +845,7 @@ impl PyVersion { /// Note that we drop the * placeholder by moving it to `Operator` #[getter] pub fn release(&self) -> Vec { - self.0.release.clone() + self.0.release().to_vec() } /// The [prerelease](https://peps.python.org/pep-0440/#pre-releases), i.e. alpha, beta or rc /// plus a number @@ -757,35 +854,35 @@ impl PyVersion { /// range matching since normally we exclude all prerelease versions #[getter] pub fn pre(&self) -> Option<(PreRelease, u64)> { - self.0.pre + self.0.pre() } /// The [Post release version](https://peps.python.org/pep-0440/#post-releases), /// higher post version are preferred over lower post or none-post versions #[getter] pub fn post(&self) -> Option { - self.0.post + self.0.post() } /// The [developmental release](https://peps.python.org/pep-0440/#developmental-releases), /// if any #[getter] pub fn dev(&self) -> Option { - self.0.dev + self.0.dev() } /// The first item of release or 0 if unavailable. #[getter] #[allow(clippy::get_first)] pub fn major(&self) -> u64 { - self.0.release.get(0).copied().unwrap_or_default() + self.0.release().get(0).copied().unwrap_or_default() } /// The second item of release or 0 if unavailable. #[getter] pub fn minor(&self) -> u64 { - self.0.release.get(1).copied().unwrap_or_default() + self.0.release().get(1).copied().unwrap_or_default() } /// The third item of release or 0 if unavailable. #[getter] pub fn micro(&self) -> u64 { - self.0.release.get(2).copied().unwrap_or_default() + self.0.release().get(2).copied().unwrap_or_default() } /// Parses a PEP 440 version string @@ -1310,7 +1407,9 @@ mod tests { format!("Version `{version}` doesn't match PEP 440 rules") ); assert_eq!( - VersionSpecifier::from_str(&format!("=={version}")).unwrap_err(), + VersionSpecifier::from_str(&format!("=={version}")) + .unwrap_err() + .to_string(), format!("Version `{version}` doesn't match PEP 440 rules") ); } diff --git a/crates/pep440-rs/src/version_specifier.rs b/crates/pep440-rs/src/version_specifier.rs index 2cb640854..97fe72097 100644 --- a/crates/pep440-rs/src/version_specifier.rs +++ b/crates/pep440-rs/src/version_specifier.rs @@ -11,11 +11,10 @@ use pyo3::{ }; #[cfg(feature = "serde")] use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; -use unicode_width::UnicodeWidthStr; #[cfg(feature = "pyo3")] use crate::version::PyVersion; -use crate::{version, Operator, Pep440Error, Version}; +use crate::{version, Operator, Version}; /// A thin wrapper around `Vec` with a serde implementation /// @@ -60,7 +59,7 @@ impl FromIterator for VersionSpecifiers { } impl FromStr for VersionSpecifiers { - type Err = Pep440Error; + type Err = VersionSpecifiersParseError; fn from_str(s: &str) -> Result { parse_version_specifiers(s).map(Self) @@ -184,6 +183,49 @@ impl Serialize for VersionSpecifiers { } } +/// Error with span information (unicode width) inside the parsed line +#[derive(Debug, Eq, PartialEq, Clone)] +pub struct VersionSpecifiersParseError { + // Clippy complains about this error type being too big (at time of + // writing, over 150 bytes). That does seem a little big, so we box things. + inner: Box, +} + +#[derive(Debug, Eq, PartialEq, Clone)] +struct VersionSpecifiersParseErrorInner { + /// The underlying error that occurred. + err: VersionSpecifierParseError, + /// The string that failed to parse + line: String, + /// The starting byte offset into the original string where the error + /// occurred. + start: usize, + /// The ending byte offset into the original string where the error + /// occurred. + end: usize, +} + +impl std::fmt::Display for VersionSpecifiersParseError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + use unicode_width::UnicodeWidthStr; + + let VersionSpecifiersParseErrorInner { + ref err, + ref line, + start, + end, + } = *self.inner; + writeln!(f, "Failed to parse version: {}:", err)?; + writeln!(f, "{}", line)?; + let indent = line[..start].width(); + let point = line[start..end].width(); + writeln!(f, "{}{}", " ".repeat(indent), "^".repeat(point))?; + Ok(()) + } +} + +impl std::error::Error for VersionSpecifiersParseError {} + /// A version range such such as `>1.2.3`, `<=4!5.6.7-a8.post9.dev0` or `== 4.1.*`. Parse with /// `VersionSpecifier::from_str` /// @@ -211,7 +253,7 @@ impl VersionSpecifier { /// Parse a PEP 440 version #[new] pub fn parse(version_specifier: &str) -> PyResult { - Self::from_str(version_specifier).map_err(PyValueError::new_err) + Self::from_str(version_specifier).map_err(|e| PyValueError::new_err(e.to_string())) } /// See [VersionSpecifier::contains] @@ -279,40 +321,22 @@ impl Serialize for VersionSpecifier { impl VersionSpecifier { /// Build from parts, validating that the operator is allowed with that version. The last /// parameter indicates a trailing `.*`, to differentiate between `1.1.*` and `1.1` - pub fn new(operator: Operator, version: Version, star: bool) -> Result { + pub fn new( + operator: Operator, + version: Version, + star: bool, + ) -> Result { // "Local version identifiers are NOT permitted in this version specifier." - if let Some(local) = &version.local() { - if matches!( - operator, - Operator::GreaterThan - | Operator::GreaterThanEqual - | Operator::LessThan - | Operator::LessThanEqual - | Operator::TildeEqual - | Operator::EqualStar - | Operator::NotEqualStar - ) { - return Err(format!( - "You can't mix a {} operator with a local version (`+{}`)", - operator, - local - .iter() - .map(std::string::ToString::to_string) - .collect::>() - .join(".") - )); - } + if version.local().is_some() && !operator.is_local_compatible() { + return Err(BuildErrorKind::OperatorLocalCombo { operator, version }.into()); } // Check if there are star versions and if so, switch operator to star version let operator = if star { - match operator { - Operator::Equal => Operator::EqualStar, - Operator::NotEqual => Operator::NotEqualStar, - other => { - return Err(format!( - "Operator {other} must not be used in version ending with a star" - )) + match operator.to_star() { + Some(starop) => starop, + None => { + return Err(BuildErrorKind::OperatorWithStar { operator }.into()); } } } else { @@ -320,9 +344,7 @@ impl VersionSpecifier { }; if operator == Operator::TildeEqual && version.release().len() < 2 { - return Err( - "The ~= operator requires at least two parts in the release version".to_string(), - ); + return Err(BuildErrorKind::CompatibleRelease.into()); } Ok(Self { operator, version }) @@ -350,9 +372,7 @@ impl VersionSpecifier { pub fn any_prerelease(&self) -> bool { self.version.any_prerelease() } -} -impl VersionSpecifier { /// Whether the given version satisfies the version range /// /// e.g. `>=1.19,<2.0` and `1.21` -> true @@ -487,7 +507,7 @@ impl VersionSpecifier { } impl FromStr for VersionSpecifier { - type Err = String; + type Err = VersionSpecifierParseError; /// Parses a version such as `>= 1.19`, `== 1.1.*`,`~=1.0+abc.5` or `<=1!2012.2` fn from_str(spec: &str) -> Result { @@ -496,19 +516,21 @@ impl FromStr for VersionSpecifier { // operator but we don't know yet if it has a star let operator = s.eat_while(['=', '!', '~', '<', '>']); if operator.is_empty() { - return Err("Missing comparison operator".to_string()); + return Err(ParseErrorKind::MissingOperator.into()); } - let operator = Operator::from_str(operator)?; + let operator = Operator::from_str(operator).map_err(ParseErrorKind::InvalidOperator)?; s.eat_while(|c: char| c.is_whitespace()); let version = s.eat_while(|c: char| !c.is_whitespace()); if version.is_empty() { - return Err("Missing version".to_string()); + return Err(ParseErrorKind::MissingVersion.into()); } - let (version, star) = Version::from_str_star(version)?; - let version_specifier = VersionSpecifier::new(operator, version, star)?; + let (version, star) = + Version::from_str_star(version).map_err(ParseErrorKind::InvalidVersion)?; + let version_specifier = VersionSpecifier::new(operator, version, star) + .map_err(ParseErrorKind::InvalidSpecifier)?; s.eat_while(|c: char| c.is_whitespace()); if !s.done() { - return Err(format!("Trailing `{}` not allowed", s.after())); + return Err(ParseErrorKind::InvalidTrailing(s.after().to_string()).into()); } Ok(version_specifier) } @@ -523,6 +545,138 @@ impl std::fmt::Display for VersionSpecifier { } } +/// An error that can occur when constructing a version specifier. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct VersionSpecifierBuildError { + // We box to shrink the error type's size. This in turn keeps Result + // smaller and should lead to overall better codegen. + kind: Box, +} + +impl std::error::Error for VersionSpecifierBuildError {} + +impl std::fmt::Display for VersionSpecifierBuildError { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + match *self.kind { + BuildErrorKind::OperatorLocalCombo { + operator: ref op, + ref version, + } => { + let local = version + .local() + .iter() + .flat_map(|segments| segments.iter()) + .map(|segment| segment.to_string()) + .collect::>() + .join("."); + write!( + f, + "Operator {op} is incompatible with versions \ + containing non-empty local segments (`+{local}`)", + ) + } + BuildErrorKind::OperatorWithStar { operator: ref op } => { + write!( + f, + "Operator {op} cannot be used with a wildcard version specifier", + ) + } + BuildErrorKind::CompatibleRelease => { + write!( + f, + "The ~= operator requires at least two segments in the release version" + ) + } + } + } +} + +/// The specific kind of error that can occur when building a version specifier +/// from an operator and version pair. +#[derive(Clone, Debug, Eq, PartialEq)] +enum BuildErrorKind { + /// Occurs when one attempts to build a version specifier with + /// a version containing a non-empty local segment with and an + /// incompatible operator. + OperatorLocalCombo { + /// The operator given. + operator: Operator, + /// The version given. + version: Version, + }, + /// Occurs when a version specifier contains a wildcard, but is used with + /// an incompatible operator. + OperatorWithStar { + /// The operator given. + operator: Operator, + }, + /// Occurs when the compatible release operator (`~=`) is used with a + /// version that has fewer than 2 segments in its release version. + CompatibleRelease, +} + +impl From for VersionSpecifierBuildError { + fn from(kind: BuildErrorKind) -> VersionSpecifierBuildError { + VersionSpecifierBuildError { + kind: Box::new(kind), + } + } +} + +/// An error that can occur when parsing or constructing a version specifier. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct VersionSpecifierParseError { + // We box to shrink the error type's size. This in turn keeps Result + // smaller and should lead to overall better codegen. + kind: Box, +} + +impl std::error::Error for VersionSpecifierParseError {} + +impl std::fmt::Display for VersionSpecifierParseError { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + // Note that even though we have nested error types here, since we + // don't expose them through std::error::Error::source, we emit them + // as part of the error message here. This makes the error a bit + // more self-contained. And it's not clear how useful it is exposing + // internal errors. + match *self.kind { + ParseErrorKind::InvalidOperator(ref err) => err.fmt(f), + ParseErrorKind::InvalidVersion(ref err) => err.fmt(f), + ParseErrorKind::InvalidSpecifier(ref err) => err.fmt(f), + ParseErrorKind::MissingOperator => { + write!(f, "Unexpected end of version specifier, expected operator") + } + ParseErrorKind::MissingVersion => { + write!(f, "Unexpected end of version specifier, expected version") + } + ParseErrorKind::InvalidTrailing(ref trail) => { + write!(f, "Trailing `{trail}` is not allowed") + } + } + } +} + +/// The specific kind of error that occurs when parsing a single version +/// specifier from a string. +#[derive(Clone, Debug, Eq, PartialEq)] +enum ParseErrorKind { + InvalidOperator(String), + InvalidVersion(String), + InvalidSpecifier(VersionSpecifierBuildError), + MissingOperator, + MissingVersion, + InvalidTrailing(String), +} + +impl From for VersionSpecifierParseError { + fn from(kind: ParseErrorKind) -> VersionSpecifierParseError { + VersionSpecifierParseError { + kind: Box::new(kind), + } + } +} + /// Parses a list of specifiers such as `>= 1.0, != 1.3.*, < 2.0`. /// /// I recommend using [`VersionSpecifiers::from_str`] instead. @@ -535,7 +689,9 @@ impl std::fmt::Display for VersionSpecifier { /// let version_specifiers = parse_version_specifiers(">=1.16, <2.0").unwrap(); /// assert!(version_specifiers.iter().all(|specifier| specifier.contains(&version))); /// ``` -pub fn parse_version_specifiers(spec: &str) -> Result, Pep440Error> { +pub fn parse_version_specifiers( + spec: &str, +) -> Result, VersionSpecifiersParseError> { let mut version_ranges = Vec::new(); if spec.is_empty() { return Ok(version_ranges); @@ -545,19 +701,21 @@ pub fn parse_version_specifiers(spec: &str) -> Result, Pep for version_range_spec in spec.split(separator) { match VersionSpecifier::from_str(version_range_spec) { Err(err) => { - return Err(Pep440Error { - message: err, - line: spec.to_string(), - start, - width: version_range_spec.width(), + return Err(VersionSpecifiersParseError { + inner: Box::new(VersionSpecifiersParseErrorInner { + err, + line: spec.to_string(), + start, + end: start + version_range_spec.len(), + }), }); } Ok(version_range) => { version_ranges.push(version_range); } } - start += version_range_spec.width(); - start += separator.width(); + start += version_range_spec.len(); + start += separator.len(); } Ok(version_ranges) } @@ -568,7 +726,9 @@ mod tests { use indoc::indoc; - use crate::{Operator, Version, VersionSpecifier, VersionSpecifiers}; + use crate::LocalSegment; + + use super::*; /// #[test] @@ -1083,7 +1243,7 @@ mod tests { assert_eq!( result.unwrap_err().to_string(), indoc! {r#" - Failed to parse version: + Failed to parse version: Unexpected end of version specifier, expected operator: ~= 0.9, %‍= 1.0, != 1.3.4.* ^^^^^^^ "#} @@ -1094,8 +1254,11 @@ mod tests { fn test_non_star_after_star() { let result = VersionSpecifiers::from_str("== 0.9.*.1"); assert_eq!( - result.unwrap_err().message, - "Version `0.9.*.1` doesn't match PEP 440 rules" + result.unwrap_err().inner.err, + ParseErrorKind::InvalidVersion( + "Version `0.9.*.1` doesn't match PEP 440 rules".to_string() + ) + .into() ); } @@ -1103,15 +1266,24 @@ mod tests { fn test_star_wrong_operator() { let result = VersionSpecifiers::from_str(">= 0.9.1.*"); assert_eq!( - result.unwrap_err().message, - "Operator >= must not be used in version ending with a star" + result.unwrap_err().inner.err, + ParseErrorKind::InvalidSpecifier( + BuildErrorKind::OperatorWithStar { + operator: Operator::GreaterThanEqual, + } + .into() + ) + .into(), ); } #[test] fn test_regex_mismatch() { let result = VersionSpecifiers::from_str("blergh"); - assert_eq!(result.unwrap_err().message, "Missing comparison operator"); + assert_eq!( + result.unwrap_err().inner.err, + ParseErrorKind::MissingOperator.into(), + ); } /// @@ -1119,126 +1291,234 @@ mod tests { fn test_invalid_specifier() { let specifiers = [ // Operator-less specifier - ("2.0", Some("Missing comparison operator")), + ("2.0", ParseErrorKind::MissingOperator.into()), // Invalid operator ( "=>2.0", - Some("No such comparison operator '=>', must be one of ~= == != <= >= < > ==="), + ParseErrorKind::InvalidOperator( + "No such comparison operator '=>', must be one of ~= == != <= >= < > ===" + .to_string(), + ) + .into(), ), // Version-less specifier - ("==", Some("Missing version")), + ("==", ParseErrorKind::MissingVersion.into()), // Local segment on operators which don't support them ( "~=1.0+5", - Some("You can't mix a ~= operator with a local version (`+5`)"), + ParseErrorKind::InvalidSpecifier( + BuildErrorKind::OperatorLocalCombo { + operator: Operator::TildeEqual, + version: Version::new([1, 0]).with_local(vec![LocalSegment::Number(5)]), + } + .into(), + ) + .into(), ), ( ">=1.0+deadbeef", - Some("You can't mix a >= operator with a local version (`+deadbeef`)"), + ParseErrorKind::InvalidSpecifier( + BuildErrorKind::OperatorLocalCombo { + operator: Operator::GreaterThanEqual, + version: Version::new([1, 0]) + .with_local(vec![LocalSegment::String("deadbeef".to_string())]), + } + .into(), + ) + .into(), ), ( "<=1.0+abc123", - Some("You can't mix a <= operator with a local version (`+abc123`)"), + ParseErrorKind::InvalidSpecifier( + BuildErrorKind::OperatorLocalCombo { + operator: Operator::LessThanEqual, + version: Version::new([1, 0]) + .with_local(vec![LocalSegment::String("abc123".to_string())]), + } + .into(), + ) + .into(), ), ( ">1.0+watwat", - Some("You can't mix a > operator with a local version (`+watwat`)"), + ParseErrorKind::InvalidSpecifier( + BuildErrorKind::OperatorLocalCombo { + operator: Operator::GreaterThan, + version: Version::new([1, 0]) + .with_local(vec![LocalSegment::String("watwat".to_string())]), + } + .into(), + ) + .into(), ), ( "<1.0+1.0", - Some("You can't mix a < operator with a local version (`+1.0`)"), + ParseErrorKind::InvalidSpecifier( + BuildErrorKind::OperatorLocalCombo { + operator: Operator::LessThan, + version: Version::new([1, 0]) + .with_local(vec![LocalSegment::Number(1), LocalSegment::Number(0)]), + } + .into(), + ) + .into(), ), // Prefix matching on operators which don't support them ( "~=1.0.*", - Some("Operator ~= must not be used in version ending with a star"), + ParseErrorKind::InvalidSpecifier( + BuildErrorKind::OperatorWithStar { + operator: Operator::TildeEqual, + } + .into(), + ) + .into(), ), ( ">=1.0.*", - Some("Operator >= must not be used in version ending with a star"), + ParseErrorKind::InvalidSpecifier( + BuildErrorKind::OperatorWithStar { + operator: Operator::GreaterThanEqual, + } + .into(), + ) + .into(), ), ( "<=1.0.*", - Some("Operator <= must not be used in version ending with a star"), + ParseErrorKind::InvalidSpecifier( + BuildErrorKind::OperatorWithStar { + operator: Operator::LessThanEqual, + } + .into(), + ) + .into(), ), ( ">1.0.*", - Some("Operator > must not be used in version ending with a star"), + ParseErrorKind::InvalidSpecifier( + BuildErrorKind::OperatorWithStar { + operator: Operator::GreaterThan, + } + .into(), + ) + .into(), ), ( "<1.0.*", - Some("Operator < must not be used in version ending with a star"), + ParseErrorKind::InvalidSpecifier( + BuildErrorKind::OperatorWithStar { + operator: Operator::LessThan, + } + .into(), + ) + .into(), ), // Combination of local and prefix matching on operators which do // support one or the other ( "==1.0.*+5", - Some("Version `1.0.*+5` doesn't match PEP 440 rules"), + ParseErrorKind::InvalidVersion( + "Version `1.0.*+5` doesn't match PEP 440 rules".to_string(), + ) + .into(), ), ( "!=1.0.*+deadbeef", - Some("Version `1.0.*+deadbeef` doesn't match PEP 440 rules"), + ParseErrorKind::InvalidVersion( + "Version `1.0.*+deadbeef` doesn't match PEP 440 rules".to_string(), + ) + .into(), ), // Prefix matching cannot be used with a pre-release, post-release, // dev or local version ( "==2.0a1.*", - Some("You can't have both a trailing `.*` and a prerelease version"), + ParseErrorKind::InvalidVersion( + "You can't have both a trailing `.*` and a prerelease version".to_string(), + ) + .into(), ), ( "!=2.0a1.*", - Some("You can't have both a trailing `.*` and a prerelease version"), + ParseErrorKind::InvalidVersion( + "You can't have both a trailing `.*` and a prerelease version".to_string(), + ) + .into(), ), ( "==2.0.post1.*", - Some("You can't have both a trailing `.*` and a post version"), + ParseErrorKind::InvalidVersion( + "You can't have both a trailing `.*` and a post version".to_string(), + ) + .into(), ), ( "!=2.0.post1.*", - Some("You can't have both a trailing `.*` and a post version"), + ParseErrorKind::InvalidVersion( + "You can't have both a trailing `.*` and a post version".to_string(), + ) + .into(), ), ( "==2.0.dev1.*", - Some("You can't have both a trailing `.*` and a dev version"), + ParseErrorKind::InvalidVersion( + "You can't have both a trailing `.*` and a dev version".to_string(), + ) + .into(), ), ( "!=2.0.dev1.*", - Some("You can't have both a trailing `.*` and a dev version"), + ParseErrorKind::InvalidVersion( + "You can't have both a trailing `.*` and a dev version".to_string(), + ) + .into(), ), ( "==1.0+5.*", - Some("You can't have both a trailing `.*` and a local version"), + ParseErrorKind::InvalidVersion( + "You can't have both a trailing `.*` and a local version".to_string(), + ) + .into(), ), ( "!=1.0+deadbeef.*", - Some("You can't have both a trailing `.*` and a local version"), + ParseErrorKind::InvalidVersion( + "You can't have both a trailing `.*` and a local version".to_string(), + ) + .into(), ), // Prefix matching must appear at the end ( "==1.0.*.5", - Some("Version `1.0.*.5` doesn't match PEP 440 rules"), + ParseErrorKind::InvalidVersion( + "Version `1.0.*.5` doesn't match PEP 440 rules".to_string(), + ) + .into(), ), // Compatible operator requires 2 digits in the release operator ( "~=1", - Some("The ~= operator requires at least two parts in the release version"), + ParseErrorKind::InvalidSpecifier(BuildErrorKind::CompatibleRelease.into()).into(), ), // Cannot use a prefix matching after a .devN version ( "==1.0.dev1.*", - Some("You can't have both a trailing `.*` and a dev version"), + ParseErrorKind::InvalidVersion( + "You can't have both a trailing `.*` and a dev version".to_string(), + ) + .into(), ), ( "!=1.0.dev1.*", - Some("You can't have both a trailing `.*` and a dev version"), + ParseErrorKind::InvalidVersion( + "You can't have both a trailing `.*` and a dev version".to_string(), + ) + .into(), ), ]; for (specifier, error) in specifiers { - if let Some(error) = error { - assert_eq!(VersionSpecifier::from_str(specifier).unwrap_err(), error); - } else { - unreachable!("expected an error, but got valid version specifier") - } + assert_eq!(VersionSpecifier::from_str(specifier).unwrap_err(), error); } } @@ -1278,4 +1558,95 @@ mod tests { fn test_version_specifiers_empty() { assert_eq!(VersionSpecifiers::from_str("").unwrap().to_string(), ""); } + + /// All non-ASCII version specifiers are invalid, but the user can still + /// attempt to parse a non-ASCII string as a version specifier. This + /// ensures no panics occur and that the error reported has correct info. + #[test] + fn non_ascii_version_specifier() { + let s = "💩"; + let err = s.parse::().unwrap_err(); + assert_eq!(err.inner.start, 0); + assert_eq!(err.inner.end, 4); + + // The first test here is plain ASCII and it gives the + // expected result: the error starts at codepoint 12, + // which is the start of `>5.%`. + let s = ">=3.7, <4.0,>5.%"; + let err = s.parse::().unwrap_err(); + assert_eq!(err.inner.start, 12); + assert_eq!(err.inner.end, 16); + // In this case, we replace a single ASCII codepoint + // with U+3000 IDEOGRAPHIC SPACE. Its *visual* width is + // 2 despite it being a single codepoint. This causes + // the offsets in the error reporting logic to become + // incorrect. + // + // ... it did. This bug was fixed by switching to byte + // offsets. + let s = ">=3.7,\u{3000}<4.0,>5.%"; + let err = s.parse::().unwrap_err(); + assert_eq!(err.inner.start, 14); + assert_eq!(err.inner.end, 18); + } + + /// Tests the human readable error messages generated from an invalid + /// sequence of version specifiers. + #[test] + fn error_message_version_specifiers_parse_error() { + let specs = ">=1.2.3, 5.4.3, >=3.4.5"; + let err = VersionSpecifierParseError { + kind: Box::new(ParseErrorKind::MissingOperator), + }; + let inner = Box::new(VersionSpecifiersParseErrorInner { + err, + line: specs.to_string(), + start: 8, + end: 14, + }); + let err = VersionSpecifiersParseError { inner }; + assert_eq!(err, VersionSpecifiers::from_str(specs).unwrap_err()); + assert_eq!( + err.to_string(), + "\ +Failed to parse version: Unexpected end of version specifier, expected operator: +>=1.2.3, 5.4.3, >=3.4.5 + ^^^^^^ +" + ); + } + + /// Tests the human readable error messages generated when building an + /// invalid version specifier. + #[test] + fn error_message_version_specifier_build_error() { + let err = VersionSpecifierBuildError { + kind: Box::new(BuildErrorKind::CompatibleRelease), + }; + let op = Operator::TildeEqual; + let v = Version::new([5]); + assert_eq!(err, VersionSpecifier::new(op, v, false).unwrap_err()); + assert_eq!( + err.to_string(), + "The ~= operator requires at least two segments in the release version" + ); + } + + /// Tests the human readable error messages generated from parsing invalid + /// version specifier. + #[test] + fn error_message_version_specifier_parse_error() { + let err = VersionSpecifierParseError { + kind: Box::new(ParseErrorKind::InvalidSpecifier( + VersionSpecifierBuildError { + kind: Box::new(BuildErrorKind::CompatibleRelease), + }, + )), + }; + assert_eq!(err, VersionSpecifier::from_str("~=5").unwrap_err()); + assert_eq!( + err.to_string(), + "The ~= operator requires at least two segments in the release version" + ); + } } diff --git a/crates/pep508-rs/src/lib.rs b/crates/pep508-rs/src/lib.rs index 0d0a02764..65396dab9 100644 --- a/crates/pep508-rs/src/lib.rs +++ b/crates/pep508-rs/src/lib.rs @@ -702,7 +702,7 @@ fn parse_specifier( end: usize, ) -> Result { VersionSpecifier::from_str(buffer).map_err(|err| Pep508Error { - message: Pep508ErrorSource::String(err), + message: Pep508ErrorSource::String(err.to_string()), start, len: end - start, input: cursor.to_string(), @@ -1262,7 +1262,7 @@ mod tests { assert_err( r#"numpy >=1.1.*"#, indoc! {" - Operator >= must not be used in version ending with a star + Operator >= cannot be used with a wildcard version specifier numpy >=1.1.* ^^^^^^^" }, @@ -1442,7 +1442,7 @@ mod tests { assert_err( "name==", indoc! {" - Missing version + Unexpected end of version specifier, expected version name== ^^" }, @@ -1466,7 +1466,7 @@ mod tests { assert_err( "name >= 1.0 #", indoc! {" - Trailing `#` not allowed + Trailing `#` is not allowed name >= 1.0 # ^^^^^^^^" }, diff --git a/crates/pep508-rs/src/marker.rs b/crates/pep508-rs/src/marker.rs index 8a869ef85..0ac963b77 100644 --- a/crates/pep508-rs/src/marker.rs +++ b/crates/pep508-rs/src/marker.rs @@ -783,7 +783,7 @@ impl MarkerExpression { let compatible = python_versions.iter().any(|r_version| { // operator and right hand side make the specifier and in this case the // right hand is `python_version` so changes every iteration - match VersionSpecifier::new(operator.clone(), r_version.clone(), false) { + match VersionSpecifier::new(operator, r_version.clone(), false) { Ok(specifier) => specifier.contains(&l_version), Err(_) => true, } diff --git a/crates/puffin-client/src/html.rs b/crates/puffin-client/src/html.rs index 794dcc0b9..f43bc8817 100644 --- a/crates/puffin-client/src/html.rs +++ b/crates/puffin-client/src/html.rs @@ -191,7 +191,7 @@ pub enum Error { UnsupportedHashAlgorithm(String), #[error("Invalid `requires-python` specifier: {0}")] - Pep440(#[source] pep440_rs::Pep440Error), + Pep440(#[source] pep440_rs::VersionSpecifiersParseError), } #[cfg(test)] diff --git a/crates/pypi-types/src/lenient_requirement.rs b/crates/pypi-types/src/lenient_requirement.rs index 088a9b18b..027ff68c2 100644 --- a/crates/pypi-types/src/lenient_requirement.rs +++ b/crates/pypi-types/src/lenient_requirement.rs @@ -4,7 +4,7 @@ use once_cell::sync::Lazy; use regex::Regex; use serde::{de, Deserialize, Deserializer, Serialize}; -use pep440_rs::{Pep440Error, VersionSpecifiers}; +use pep440_rs::{VersionSpecifiers, VersionSpecifiersParseError}; use pep508_rs::{Pep508Error, Requirement}; use puffin_warnings::warn_user_once; @@ -99,7 +99,7 @@ impl From for Requirement { pub struct LenientVersionSpecifiers(VersionSpecifiers); impl FromStr for LenientVersionSpecifiers { - type Err = Pep440Error; + type Err = VersionSpecifiersParseError; fn from_str(input: &str) -> Result { Ok(Self(parse_with_fixups(input, "version specifier")?)) diff --git a/crates/pypi-types/src/metadata.rs b/crates/pypi-types/src/metadata.rs index 99fb2d4da..7f4ce64df 100644 --- a/crates/pypi-types/src/metadata.rs +++ b/crates/pypi-types/src/metadata.rs @@ -7,7 +7,7 @@ use mailparse::{MailHeaderMap, MailParseError}; use serde::{Deserialize, Serialize}; use thiserror::Error; -use pep440_rs::{Pep440Error, Version, VersionSpecifiers}; +use pep440_rs::{Version, VersionSpecifiers, VersionSpecifiersParseError}; use pep508_rs::{Pep508Error, Requirement}; use puffin_normalize::{ExtraName, InvalidNameError, PackageName}; @@ -63,7 +63,7 @@ pub enum Error { Pep440VersionError(String), /// Invalid VersionSpecifier #[error(transparent)] - Pep440Error(#[from] Pep440Error), + Pep440Error(#[from] VersionSpecifiersParseError), /// Invalid Requirement #[error(transparent)] Pep508Error(#[from] Pep508Error),