Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Encode mutually-incompatible pairs of markers #9444

Merged
merged 5 commits into from
Dec 7, 2024
Merged

Encode mutually-incompatible pairs of markers #9444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3bd96f7
Encode mutually-incompatible pairs of markers
charliermarsh Nov 26, 2024
343c6d7
Move into constructors
charliermarsh Nov 28, 2024
45173c9
Add disjoint
charliermarsh Nov 28, 2024
9233f99
Move into algebra
charliermarsh Nov 29, 2024
1d75afa
Sort
charliermarsh Dec 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
360 changes: 347 additions & 13 deletions crates/uv-pep508/src/marker/algebra.rs
View file
Open in desktop

Large diffs are not rendered by default.

32 changes: 25 additions & 7 deletions crates/uv-pep508/src/marker/lowering.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,26 +32,44 @@ impl From<CanonicalMarkerValueVersion> for MarkerValueVersion {
}
}

/// Those environment markers with an arbitrary string as value such as `sys_platform`
/// Those environment markers with an arbitrary string as value such as `sys_platform`.
///
/// As in [`crate::marker::algebra::Variable`], this `enum` also defines the variable ordering for
/// all ADDs, which is in turn used when translating the ADD to DNF. As such, modifying the ordering
/// will modify the output of marker expressions.
///
/// Critically, any variants that could be involved in a known-incompatible marker pair should
/// be at the top of the ordering, i.e., given the maximum priority.
#[derive(Copy, Clone, Debug, Eq, Hash, PartialEq, PartialOrd, Ord)]
pub enum CanonicalMarkerValueString {
/// `implementation_name`
ImplementationName,
/// `os_name`
OsName,
/// `sys_platform`
SysPlatform,
/// `platform_system`
PlatformSystem,
/// `platform_machine`
PlatformMachine,
/// Deprecated `platform.machine` from <https://peps.python.org/pep-0345/#environment-markers>
/// `platform_python_implementation`
PlatformPythonImplementation,
/// `platform_release`
PlatformRelease,
/// `platform_system`
PlatformSystem,
/// `platform_version`
PlatformVersion,
/// `sys_platform`
SysPlatform,
/// `implementation_name`
ImplementationName,
}

impl CanonicalMarkerValueString {
/// Returns `true` if the marker is known to be involved in _at least_ one conflicting
/// marker pair.
///
/// For example, `sys_platform == 'win32'` and `platform_system == 'Darwin'` are known to
/// never be true at the same time.
pub(crate) fn is_conflicting(self) -> bool {
self <= Self::PlatformSystem
}
}

impl From<MarkerValueString> for CanonicalMarkerValueString {
Expand Down
17 changes: 17 additions & 0 deletions crates/uv-pep508/src/marker/simplify.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ pub(crate) fn to_dnf(tree: MarkerTree) -> Vec<Vec<MarkerExpression>> {
let mut dnf = Vec::new();
collect_dnf(tree, &mut dnf, &mut Vec::new());
simplify(&mut dnf);
sort(&mut dnf);
dnf
}

Expand Down Expand Up @@ -278,6 +279,22 @@ fn simplify(dnf: &mut Vec<Vec<MarkerExpression>>) {
}
}

/// Sort the clauses in a DNF expression, for backwards compatibility. The goal is to avoid
/// unnecessary churn in the display output of the marker expressions, e.g., when modifying the
/// internal representations used in the marker algebra.
fn sort(dnf: &mut [Vec<MarkerExpression>]) {
// Sort each clause.
for clause in dnf.iter_mut() {
clause.sort_by_key(MarkerExpression::kind);
}
// Sort the clauses.
dnf.sort_by(|a, b| {
a.iter()
.map(MarkerExpression::kind)
.cmp(b.iter().map(MarkerExpression::kind))
});
}

/// Merge any edges that lead to identical subtrees into a single range.
pub(crate) fn collect_edges<'a, T>(
map: impl ExactSizeIterator<Item = (&'a Ranges<T>, MarkerTree)>,
Expand Down
64 changes: 40 additions & 24 deletions crates/uv-pep508/src/marker/tree.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@ use std::str::FromStr;

use itertools::Itertools;
use serde::{de, Deserialize, Deserializer, Serialize, Serializer};
use version_ranges::Ranges;

use uv_normalize::ExtraName;
use uv_pep440::{Version, VersionParseError, VersionSpecifier};
use version_ranges::Ranges;

use super::algebra::{Edges, NodeId, Variable, INTERNER};
use super::simplify;
Expand Down Expand Up @@ -493,6 +494,19 @@ pub enum MarkerExpression {
},
}

/// The kind of a [`MarkerExpression`].
#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq, PartialOrd, Ord)]
pub(crate) enum MarkerExpressionKind {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this only used for backcompat sorting? If so, it might be useful to add a comment indicating as such. And that the order here matters?

/// A version expression, e.g. `<version key> <version op> <quoted PEP 440 version>`.
Version(MarkerValueVersion),
/// A version "in" expression, e.g. `<version key> in <quoted list of PEP 440 versions>`.
VersionIn(MarkerValueVersion),
/// A string marker comparison, e.g. `sys_platform == '...'`.
String(MarkerValueString),
/// An extra expression, e.g. `extra == '...'`.
Extra,
}

/// The operator for an extra expression, either '==' or '!='.
#[derive(Clone, Debug, Eq, Hash, PartialEq, PartialOrd, Ord)]
pub enum ExtraOperator {
Expand Down Expand Up @@ -563,6 +577,16 @@ impl MarkerExpression {
pub fn from_str(s: &str) -> Result<Option<Self>, Pep508Error> {
MarkerExpression::parse_reporter(s, &mut TracingReporter)
}

/// Return the kind of this marker expression.
pub(crate) fn kind(&self) -> MarkerExpressionKind {
match self {
MarkerExpression::Version { key, .. } => MarkerExpressionKind::Version(*key),
MarkerExpression::VersionIn { key, .. } => MarkerExpressionKind::VersionIn(*key),
MarkerExpression::String { key, .. } => MarkerExpressionKind::String(*key),
MarkerExpression::Extra { .. } => MarkerExpressionKind::Extra,
}
}
}

impl Display for MarkerExpression {
Expand Down Expand Up @@ -691,13 +715,11 @@ impl MarkerTree {
}

/// Combine this marker tree with the one given via a conjunction.
#[allow(clippy::needless_pass_by_value)]
pub fn and(&mut self, tree: MarkerTree) {
self.0 = INTERNER.lock().and(self.0, tree.0);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it better to hold the lock and re-use it in simplify, or re-lock() as I'm doing here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably re-use it, these are pretty much only used by a single thread.

}

/// Combine this marker tree with the one given via a disjunction.
#[allow(clippy::needless_pass_by_value)]
pub fn or(&mut self, tree: MarkerTree) {
self.0 = INTERNER.lock().or(self.0, tree.0);
}
Expand All @@ -707,7 +729,6 @@ impl MarkerTree {
///
/// If the marker set is always `true`, then it can be said that `self`
/// implies `consequent`.
#[allow(clippy::needless_pass_by_value)]
pub fn implies(&mut self, consequent: MarkerTree) {
// This could probably be optimized, but is clearly
// correct, since logical implication is `-P or Q`.
Expand Down Expand Up @@ -1014,7 +1035,6 @@ impl MarkerTree {
/// results of that simplification. (If `requires-python` changes, then one
/// should reconstitute all relevant markers from the source data.)
#[must_use]
#[allow(clippy::needless_pass_by_value)]
pub fn simplify_python_versions(
self,
lower: Bound<&Version>,
Expand All @@ -1035,7 +1055,6 @@ impl MarkerTree {
/// `python_full_version <= '3.10'`, this would result in a marker of
/// `python_full_version >= '3.8' and python_full_version <= '3.10'`.
#[must_use]
#[allow(clippy::needless_pass_by_value)]
pub fn complexify_python_versions(
self,
lower: Bound<&Version>,
Expand Down Expand Up @@ -2353,7 +2372,7 @@ mod test {

assert_simplifies(
"((extra == 'a' or extra == 'b') and extra == 'c') or extra == 'b'",
"(extra == 'a' and extra == 'c') or extra == 'b'",
"extra == 'b' or (extra == 'a' and extra == 'c')",
);

// post-normalization filtering
Expand Down Expand Up @@ -2575,13 +2594,13 @@ mod test {
or (implementation_name != 'pypy' and sys_platform == 'win32')
or (sys_platform == 'win32' and os_name != 'nt')
or (sys_platform != 'win32' and os_name == 'nt')",
"(os_name != 'nt' and sys_platform == 'win32') \
or (implementation_name != 'pypy' and os_name == 'nt') \
or (implementation_name == 'pypy' and os_name != 'nt') \
"(implementation_name == 'pypy' and sys_platform != 'win32') \
or (implementation_name != 'pypy' and sys_platform == 'win32') \
or (os_name != 'nt' and sys_platform == 'win32') \
or (os_name == 'nt' and sys_platform != 'win32')",
);

// This is another case we cannot simplify fully, depending on the variable order.
// This is a case we can simplify fully, but it's dependent on the variable order.
// The expression is equivalent to `sys_platform == 'x' or (os_name == 'Linux' and platform_system == 'win32')`.
assert_simplifies(
"(os_name == 'Linux' and platform_system == 'win32')
Expand All @@ -2590,14 +2609,14 @@ mod test {
or (os_name != 'Linux' and platform_system == 'win32' and sys_platform == 'x')
or (os_name == 'Linux' and platform_system != 'win32' and sys_platform == 'x')
or (os_name != 'Linux' and platform_system != 'win32' and sys_platform == 'x')",
"(os_name != 'Linux' and sys_platform == 'x') or (platform_system != 'win32' and sys_platform == 'x') or (os_name == 'Linux' and platform_system == 'win32')",
"(os_name == 'Linux' and platform_system == 'win32') or sys_platform == 'x'",
);

assert_simplifies("python_version > '3.7'", "python_full_version >= '3.8'");

assert_simplifies(
"(python_version <= '3.7' and os_name == 'Linux') or python_version > '3.7'",
"os_name == 'Linux' or python_full_version >= '3.8'",
"python_full_version >= '3.8' or os_name == 'Linux'",
);

// Again, the extra `<3.7` and `>=3.9` expressions cannot be seen as redundant due to them being interdependent.
Expand All @@ -2606,34 +2625,31 @@ mod test {
"(os_name == 'Linux' and sys_platform == 'win32') \
or (os_name != 'Linux' and sys_platform == 'win32' and python_version == '3.7') \
or (os_name != 'Linux' and sys_platform == 'win32' and python_version == '3.8')",
"(python_full_version < '3.7' and os_name == 'Linux' and sys_platform == 'win32') \
or (python_full_version >= '3.9' and os_name == 'Linux' and sys_platform == 'win32') \
or (python_full_version >= '3.7' and python_full_version < '3.9' and sys_platform == 'win32')",
"(python_full_version >= '3.7' and python_full_version < '3.9' and sys_platform == 'win32') or (os_name == 'Linux' and sys_platform == 'win32')",
);

assert_simplifies(
"(implementation_name != 'pypy' and os_name == 'nt' and sys_platform == 'darwin') or (os_name == 'nt' and sys_platform == 'win32')",
"(implementation_name != 'pypy' and os_name == 'nt' and sys_platform == 'darwin') or (os_name == 'nt' and sys_platform == 'win32')",
"os_name == 'nt' and sys_platform == 'win32'",
);

assert_simplifies(
"(sys_platform == 'darwin' or sys_platform == 'win32')
and ((implementation_name != 'pypy' and os_name == 'nt' and sys_platform == 'darwin') or (os_name == 'nt' and sys_platform == 'win32'))",
"(implementation_name != 'pypy' and os_name == 'nt' and sys_platform == 'darwin') or (os_name == 'nt' and sys_platform == 'win32')",
"(sys_platform == 'darwin' or sys_platform == 'win32') and ((implementation_name != 'pypy' and os_name == 'nt' and sys_platform == 'darwin') or (os_name == 'nt' and sys_platform == 'win32'))",
"os_name == 'nt' and sys_platform == 'win32'",
);

assert_simplifies(
"(sys_platform == 'darwin' or sys_platform == 'win32')
and ((platform_version != '1' and os_name == 'nt' and sys_platform == 'darwin') or (os_name == 'nt' and sys_platform == 'win32'))",
"(os_name == 'nt' and platform_version != '1' and sys_platform == 'darwin') or (os_name == 'nt' and sys_platform == 'win32')",
"os_name == 'nt' and sys_platform == 'win32'",
);

assert_simplifies(
"(os_name == 'nt' and sys_platform == 'win32') \
or (os_name != 'nt' and platform_version == '1' and (sys_platform == 'win32' or sys_platform == 'win64'))",
"(platform_version == '1' and sys_platform == 'win32') \
or (os_name != 'nt' and platform_version == '1' and sys_platform == 'win64') \
or (os_name == 'nt' and sys_platform == 'win32')",
"(os_name != 'nt' and platform_version == '1' and sys_platform == 'win64') \
or (os_name == 'nt' and sys_platform == 'win32') \
or (platform_version == '1' and sys_platform == 'win32')",
);

assert_simplifies(
Expand Down
Loading
Loading