Skip to content

Commit

Permalink
Remove uses of Option<MarkerTree> (#5978)
Browse files Browse the repository at this point in the history
## Summary

Follow up to #5898. This should fix
some of the failures in #5887 where
`uv lock --locked` is failing due to `Some(true)` and `None` markers not
comparing equal.
  • Loading branch information
ibraheemdev authored Aug 10, 2024
1 parent 4eced1b commit f5110f7
Show file tree
Hide file tree
Showing 45 changed files with 228 additions and 330 deletions.
3 changes: 2 additions & 1 deletion crates/distribution-types/src/resolution.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use distribution_filename::DistExtension;
use pep508_rs::MarkerTree;
use pypi_types::{HashDigest, Requirement, RequirementSource};
use std::collections::BTreeMap;
use uv_normalize::{ExtraName, GroupName, PackageName};
Expand Down Expand Up @@ -211,7 +212,7 @@ impl From<&ResolvedDist> for Requirement {
Requirement {
name: resolved_dist.name().clone(),
extras: vec![],
marker: None,
marker: MarkerTree::TRUE,
source,
origin: None,
}
Expand Down
61 changes: 18 additions & 43 deletions crates/pep508-rs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ pub use verbatim_url::{
};

mod cursor;
mod marker;
pub mod marker;
mod origin;
#[cfg(feature = "non-pep508-extensions")]
mod unnamed;
Expand Down Expand Up @@ -149,7 +149,7 @@ pub struct Requirement<T: Pep508Url = VerbatimUrl> {
/// The markers such as `python_version > "3.8"` in
/// `requests [security,tests] >= 2.8.1, == 2.8.* ; python_version > "3.8"`.
/// Those are a nested and/or tree.
pub marker: Option<MarkerTree>,
pub marker: MarkerTree,
/// The source file containing the requirement.
pub origin: Option<RequirementOrigin>,
}
Expand Down Expand Up @@ -190,7 +190,7 @@ impl<T: Pep508Url + Display> Display for Requirement<T> {
}
}
}
if let Some(marker) = self.marker.as_ref().and_then(MarkerTree::contents) {
if let Some(marker) = self.marker.contents() {
write!(f, " ; {marker}")?;
}
Ok(())
Expand Down Expand Up @@ -256,10 +256,7 @@ impl PyRequirement {
/// `requests [security,tests] >= 2.8.1, == 2.8.* ; python_version > "3.8"`
#[getter]
pub fn marker(&self) -> Option<String> {
self.marker
.as_ref()
.and_then(MarkerTree::contents)
.map(|marker| marker.to_string())
self.marker.try_to_string()
}

/// Parses a PEP 440 string
Expand Down Expand Up @@ -375,11 +372,7 @@ impl PyRequirement {
impl<T: Pep508Url> Requirement<T> {
/// Returns whether the markers apply for the given environment
pub fn evaluate_markers(&self, env: &MarkerEnvironment, extras: &[ExtraName]) -> bool {
if let Some(marker) = &self.marker {
marker.evaluate(env, extras)
} else {
true
}
self.marker.evaluate(env, extras)
}

/// Returns whether the requirement would be satisfied, independent of environment markers, i.e.
Expand All @@ -393,11 +386,8 @@ impl<T: Pep508Url> Requirement<T> {
extras: &HashSet<ExtraName>,
python_versions: &[Version],
) -> bool {
if let Some(marker) = &self.marker {
marker.evaluate_extras_and_python_version(extras, python_versions)
} else {
true
}
self.marker
.evaluate_extras_and_python_version(extras, python_versions)
}

/// Returns whether the markers apply for the given environment.
Expand All @@ -406,38 +396,22 @@ impl<T: Pep508Url> Requirement<T> {
env: &MarkerEnvironment,
extras: &[ExtraName],
) -> (bool, Vec<MarkerWarning>) {
if let Some(marker) = &self.marker {
marker.evaluate_collect_warnings(env, extras)
} else {
(true, Vec::new())
}
self.marker.evaluate_collect_warnings(env, extras)
}

/// Return the requirement with an additional marker added, to require the given extra.
///
/// For example, given `flask >= 2.0.2`, calling `with_extra_marker("dotenv")` would return
/// `flask >= 2.0.2 ; extra == "dotenv"`.
#[must_use]
pub fn with_extra_marker(self, extra: &ExtraName) -> Self {
let marker = match self.marker {
Some(mut marker) => {
let extra = MarkerTree::expression(MarkerExpression::Extra {
operator: ExtraOperator::Equal,
name: extra.clone(),
});
marker.and(extra);
marker
}
None => MarkerTree::expression(MarkerExpression::Extra {
pub fn with_extra_marker(mut self, extra: &ExtraName) -> Self {
self.marker
.and(MarkerTree::expression(MarkerExpression::Extra {
operator: ExtraOperator::Equal,
name: extra.clone(),
}),
};
}));

Self {
marker: Some(marker),
..self
}
self
}

/// Set the source file containing the requirement.
Expand Down Expand Up @@ -1053,6 +1027,7 @@ fn parse_pep508_requirement<T: Pep508Url>(
} else {
None
};

// wsp*
cursor.eat_whitespace();
if let Some((pos, char)) = cursor.next() {
Expand Down Expand Up @@ -1085,7 +1060,7 @@ fn parse_pep508_requirement<T: Pep508Url>(
name,
extras,
version_or_url: requirement_kind,
marker,
marker: marker.unwrap_or_default(),
origin: None,
})
}
Expand Down Expand Up @@ -1222,14 +1197,14 @@ mod tests {
.into_iter()
.collect(),
)),
marker: Some(MarkerTree::expression(MarkerExpression::Version {
marker: MarkerTree::expression(MarkerExpression::Version {
key: MarkerValueVersion::PythonVersion,
specifier: VersionSpecifier::from_pattern(
pep440_rs::Operator::LessThan,
"2.7".parse().unwrap(),
)
.unwrap(),
})),
}),
origin: None,
};
assert_eq!(requests, expected);
Expand Down Expand Up @@ -1455,7 +1430,7 @@ mod tests {
let expected = Requirement {
name: PackageName::from_str("pip").unwrap(),
extras: vec![],
marker: None,
marker: MarkerTree::TRUE,
version_or_url: Some(VersionOrUrl::Url(Url::parse(url).unwrap())),
origin: None,
};
Expand Down
21 changes: 21 additions & 0 deletions crates/pep508-rs/src/marker/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,24 @@ pub use tree::{
MarkerOperator, MarkerTree, MarkerTreeContents, MarkerTreeKind, MarkerValue, MarkerValueString,
MarkerValueVersion, MarkerWarningKind, StringMarkerTree, StringVersion, VersionMarkerTree,
};

/// `serde` helpers for [`MarkerTree`].
pub mod ser {
use super::MarkerTree;
use serde::Serialize;

/// A helper for `serde(skip_serializing_if)`.
pub fn is_empty(marker: &MarkerTree) -> bool {
marker.contents().is_none()
}

/// A helper for `serde(serialize_with)`.
///
/// Note this will panic if `marker.contents()` is `None`, and so should be paired with `is_empty`.
pub fn serialize<S>(marker: &MarkerTree, s: S) -> Result<S::Ok, S::Error>
where
S: serde::Serializer,
{
marker.contents().unwrap().serialize(s)
}
}
6 changes: 6 additions & 0 deletions crates/pep508-rs/src/marker/tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,12 @@ impl Display for MarkerExpression {
#[derive(Clone, Eq, Hash, PartialEq, PartialOrd, Ord)]
pub struct MarkerTree(NodeId);

impl Default for MarkerTree {
fn default() -> Self {
MarkerTree::TRUE
}
}

impl<'de> Deserialize<'de> for MarkerTree {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
Expand Down
12 changes: 4 additions & 8 deletions crates/pep508-rs/src/unnamed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ pub struct UnnamedRequirement<Url: UnnamedRequirementUrl = VerbatimUrl> {
/// The markers such as `python_version > "3.8"` in
/// `requests [security,tests] >= 2.8.1, == 2.8.* ; python_version > "3.8"`.
/// Those are a nested and/or tree.
pub marker: Option<MarkerTree>,
pub marker: MarkerTree,
/// The source file containing the requirement.
pub origin: Option<RequirementOrigin>,
}
Expand All @@ -92,11 +92,7 @@ impl<Url: UnnamedRequirementUrl> UnnamedRequirement<Url> {
env: Option<&MarkerEnvironment>,
extras: &[ExtraName],
) -> bool {
if let Some(marker) = &self.marker {
marker.evaluate_optional_environment(env, extras)
} else {
true
}
self.marker.evaluate_optional_environment(env, extras)
}

/// Set the source file containing the requirement.
Expand Down Expand Up @@ -136,7 +132,7 @@ impl<Url: UnnamedRequirementUrl> Display for UnnamedRequirement<Url> {
.join(",")
)?;
}
if let Some(marker) = self.marker.as_ref().and_then(MarkerTree::contents) {
if let Some(marker) = self.marker.contents() {
write!(f, " ; {marker}")?;
}
Ok(())
Expand Down Expand Up @@ -207,7 +203,7 @@ fn parse_unnamed_requirement<Url: UnnamedRequirementUrl>(
Ok(UnnamedRequirement {
url,
extras,
marker,
marker: marker.unwrap_or_default(),
origin: None,
})
}
Expand Down
36 changes: 11 additions & 25 deletions crates/pypi-types/src/requirement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,13 @@ use std::path::{Path, PathBuf};
use std::str::FromStr;

use distribution_filename::DistExtension;
use serde::Serialize;
use thiserror::Error;
use url::Url;

use pep440_rs::VersionSpecifiers;
use pep508_rs::{MarkerEnvironment, MarkerTree, RequirementOrigin, VerbatimUrl, VersionOrUrl};
use pep508_rs::{
marker, MarkerEnvironment, MarkerTree, RequirementOrigin, VerbatimUrl, VersionOrUrl,
};
use uv_fs::PortablePathBuf;
use uv_git::{GitReference, GitSha, GitUrl};
use uv_normalize::{ExtraName, PackageName};
Expand Down Expand Up @@ -40,40 +41,25 @@ pub struct Requirement {
#[serde(skip_serializing_if = "Vec::is_empty", default)]
pub extras: Vec<ExtraName>,
#[serde(
skip_serializing_if = "marker_is_empty",
serialize_with = "serialize_marker",
skip_serializing_if = "marker::ser::is_empty",
serialize_with = "marker::ser::serialize",
default
)]
pub marker: Option<MarkerTree>,
pub marker: MarkerTree,
#[serde(flatten)]
pub source: RequirementSource,
#[serde(skip)]
pub origin: Option<RequirementOrigin>,
}

fn marker_is_empty(marker: &Option<MarkerTree>) -> bool {
marker.as_ref().and_then(MarkerTree::contents).is_none()
}

fn serialize_marker<S>(marker: &Option<MarkerTree>, s: S) -> Result<S::Ok, S::Error>
where
S: serde::Serializer,
{
marker.as_ref().unwrap().contents().unwrap().serialize(s)
}

impl Requirement {
/// Returns whether the markers apply for the given environment.
///
/// When `env` is `None`, this specifically evaluates all marker
/// expressions based on the environment to `true`. That is, this provides
/// environment independent marker evaluation.
pub fn evaluate_markers(&self, env: Option<&MarkerEnvironment>, extras: &[ExtraName]) -> bool {
if let Some(marker) = &self.marker {
marker.evaluate_optional_environment(env, extras)
} else {
true
}
self.marker.evaluate_optional_environment(env, extras)
}

/// Returns `true` if the requirement is editable.
Expand Down Expand Up @@ -256,7 +242,7 @@ impl Display for Requirement {
write!(f, " @ {url}")?;
}
}
if let Some(marker) = self.marker.as_ref().and_then(MarkerTree::contents) {
if let Some(marker) = self.marker.contents() {
write!(f, " ; {marker}")?;
}
Ok(())
Expand Down Expand Up @@ -715,7 +701,7 @@ impl TryFrom<RequirementSourceWire> for RequirementSource {
mod tests {
use std::path::{Path, PathBuf};

use pep508_rs::VerbatimUrl;
use pep508_rs::{MarkerTree, VerbatimUrl};

use crate::{Requirement, RequirementSource};

Expand All @@ -724,7 +710,7 @@ mod tests {
let requirement = Requirement {
name: "foo".parse().unwrap(),
extras: vec![],
marker: None,
marker: MarkerTree::TRUE,
source: RequirementSource::Registry {
specifier: ">1,<2".parse().unwrap(),
index: None,
Expand All @@ -744,7 +730,7 @@ mod tests {
let requirement = Requirement {
name: "foo".parse().unwrap(),
extras: vec![],
marker: None,
marker: MarkerTree::TRUE,
source: RequirementSource::Directory {
install_path: PathBuf::from(path),
lock_path: PathBuf::from(path),
Expand Down
Loading

0 comments on commit f5110f7

Please sign in to comment.