Sort

astral-sh · Dec 1, 2024 · b42371b · b42371b
1 parent 5e67207
commit b42371b
Show file tree

Hide file tree

Showing 12 changed files with 183 additions and 150 deletions.
diff --git a/crates/uv-pep508/src/marker/simplify.rs b/crates/uv-pep508/src/marker/simplify.rs
@@ -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
 }
 
@@ -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)>,

diff --git a/crates/uv-pep508/src/marker/tree.rs b/crates/uv-pep508/src/marker/tree.rs
@@ -494,6 +494,19 @@ pub enum MarkerExpression {
     },
 }
 
+/// The kind of a [`MarkerExpression`].
+#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq, PartialOrd, Ord)]
+pub(crate) enum MarkerExpressionKind {
+    /// 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 {
@@ -564,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 {
@@ -692,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);
     }
 
     /// 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);
     }
@@ -708,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`.
@@ -723,10 +743,8 @@ impl MarkerTree {
     /// never both evaluate to `true` in a given environment. However, this method may return
     /// false negatives, i.e. it may not be able to detect that two markers are disjoint for
     /// complex expressions.
-    pub fn is_disjoint(&self, other: MarkerTree) -> bool {
-        let mutex = &*MUTUAL_EXCLUSIONS;
-        let node = INTERNER.lock().and(self.0, other.0);
-        node.is_false() || INTERNER.lock().is_disjoint(node, mutex.0)
+    pub fn is_disjoint(self, other: MarkerTree) -> bool {
+        INTERNER.lock().is_disjoint(self.0, other.0)
     }
 
     /// Returns the contents of this marker tree, if it contains at least one expression.
@@ -1019,7 +1037,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>,
@@ -1040,7 +1057,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>,
@@ -2247,7 +2263,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
@@ -2469,10 +2485,10 @@ 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')",
-            "(sys_platform != 'win32' and implementation_name == 'pypy') \
+            "(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') \
-                or (sys_platform == 'win32' and implementation_name != 'pypy')",
+                or (os_name == 'nt' and sys_platform != 'win32')",
         );
 
         // This is a case we can simplify fully, but it's dependent on the variable order.
@@ -2500,7 +2516,7 @@ 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')",
-            "(sys_platform == 'win32' and python_full_version >= '3.7' and python_full_version < '3.9') or (os_name == 'Linux' 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(
@@ -2522,9 +2538,9 @@ mod test {
         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'))",
-            "(sys_platform == 'win32' and platform_version == '1') \
-                or (os_name != 'nt' and sys_platform == 'win64' and platform_version == '1') \
-                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(

diff --git a/crates/uv/tests/it/edit.rs b/crates/uv/tests/it/edit.rs
@@ -2958,7 +2958,7 @@ fn add_update_marker() -> Result<()> {
         dependencies = [
             "requests>=2.0,<2.20 ; python_full_version < '3.11'",
             "requests>=2.30; python_version >= '3.11'",
-            "requests>=2.31 ; sys_platform == 'win32' and python_full_version >= '3.12'",
+            "requests>=2.31 ; python_full_version >= '3.12' and sys_platform == 'win32'",
         ]
 
         [build-system]
@@ -2999,7 +2999,7 @@ fn add_update_marker() -> Result<()> {
             "requests>=2.0,<2.20 ; python_full_version < '3.11'",
             "requests>=2.10 ; sys_platform == 'win32'",
             "requests>=2.30; python_version >= '3.11'",
-            "requests>=2.31 ; sys_platform == 'win32' and python_full_version >= '3.12'",
+            "requests>=2.31 ; python_full_version >= '3.12' and sys_platform == 'win32'",
         ]
 
         [build-system]

diff --git a/crates/uv/tests/it/export.rs b/crates/uv/tests/it/export.rs
@@ -442,15 +442,15 @@ fn dependency_multiple_markers() -> Result<()> {
     attrs==23.2.0 ; python_full_version >= '3.12' or sys_platform == 'win32' \
         --hash=sha256:935dc3b529c262f6cf76e50877d35a4bd3c1de194fd41f47a2b7ae8f19971f30 \
         --hash=sha256:99b87a485a5820b23b879f04c2305b44b951b502fd64be915879d77a7e8fc6f1
-    cffi==1.16.0 ; (os_name == 'nt' and implementation_name != 'pypy' and python_full_version >= '3.12') or (os_name == 'nt' and sys_platform == 'win32' and implementation_name != 'pypy') \
+    cffi==1.16.0 ; (python_full_version >= '3.12' and implementation_name != 'pypy' and os_name == 'nt') or (implementation_name != 'pypy' and os_name == 'nt' and sys_platform == 'win32') \
         --hash=sha256:2c56b361916f390cd758a57f2e16233eb4f64bcbeee88a4881ea90fca14dc6ab \
         --hash=sha256:68678abf380b42ce21a5f2abde8efee05c114c2fdb2e9eef2efdb0257fba1235 \
         --hash=sha256:9f90389693731ff1f659e55c7d1640e2ec43ff725cc61b04b2f9c6d8d017df6a \
         --hash=sha256:b2ca4e77f9f47c55c194982e10f058db063937845bb2b7a86c84a6cfe0aefa8b \
         --hash=sha256:bcb3ef43e58665bbda2fb198698fcae6776483e0c4a631aa5647806c25e02cc0 \
         --hash=sha256:db8e577c19c0fda0beb7e0d4e09e0ba74b1e4c092e0e40bfa12fe05b6f6d75ba \
         --hash=sha256:e6024675e67af929088fda399b2094574609396b1decb609c55fa58b028a32a1
-    exceptiongroup==1.2.0 ; sys_platform == 'win32' and python_full_version < '3.11' \
+    exceptiongroup==1.2.0 ; python_full_version < '3.11' and sys_platform == 'win32' \
         --hash=sha256:4bfd3996ac73b41e9b9628b04e079f193850720ea5945fc96a08633c66912f14 \
         --hash=sha256:91f5c769735f051a4290d52edd0858999b57e5876e9f85937691bd4c9fa3ed68
     idna==3.6 ; python_full_version >= '3.12' or sys_platform == 'win32' \
@@ -459,7 +459,7 @@ fn dependency_multiple_markers() -> Result<()> {
     outcome==1.3.0.post0 ; python_full_version >= '3.12' or sys_platform == 'win32' \
         --hash=sha256:9dcf02e65f2971b80047b377468e72a268e15c0af3cf1238e6ff14f7f91143b8 \
         --hash=sha256:e771c5ce06d1415e356078d3bdd68523f284b4ce5419828922b6871e65eda82b
-    pycparser==2.21 ; (os_name == 'nt' and implementation_name != 'pypy' and python_full_version >= '3.12') or (os_name == 'nt' and sys_platform == 'win32' and implementation_name != 'pypy') \
+    pycparser==2.21 ; (python_full_version >= '3.12' and implementation_name != 'pypy' and os_name == 'nt') or (implementation_name != 'pypy' and os_name == 'nt' and sys_platform == 'win32') \
         --hash=sha256:8ee45429555515e1f6b185e78100aea234072576aa43ab53aefcae078162fca9 \
         --hash=sha256:e644fdec12f7872f86c58ff790da456218b10f863970249516d60a5eaca77206
     sniffio==1.3.1 ; python_full_version >= '3.12' or sys_platform == 'win32' \

diff --git a/crates/uv/tests/it/lock.rs b/crates/uv/tests/it/lock.rs
@@ -1014,7 +1014,7 @@ fn lock_wheel_url() -> Result<()> {
             { name = "trio", marker = "extra == 'trio'", specifier = ">=0.23" },
             { name = "trustme", marker = "extra == 'test'" },
             { name = "typing-extensions", marker = "python_full_version < '3.11'", specifier = ">=4.1" },
-            { name = "uvloop", marker = "platform_system != 'Windows' and platform_python_implementation == 'CPython' and extra == 'test'", specifier = ">=0.17" },
+            { name = "uvloop", marker = "platform_python_implementation == 'CPython' and platform_system != 'Windows' and extra == 'test'", specifier = ">=0.17" },
         ]
 
         [[package]]
@@ -1159,7 +1159,7 @@ fn lock_sdist_url() -> Result<()> {
             { name = "trio", marker = "extra == 'trio'", specifier = ">=0.23" },
             { name = "trustme", marker = "extra == 'test'" },
             { name = "typing-extensions", marker = "python_full_version < '3.11'", specifier = ">=4.1" },
-            { name = "uvloop", marker = "platform_system != 'Windows' and platform_python_implementation == 'CPython' and extra == 'test'", specifier = ">=0.17" },
+            { name = "uvloop", marker = "platform_python_implementation == 'CPython' and platform_system != 'Windows' and extra == 'test'", specifier = ">=0.17" },
         ]
 
         [[package]]
@@ -12368,7 +12368,7 @@ fn lock_overlapping_environment() -> Result<()> {
     ----- stderr -----
     error: Supported environments must be disjoint, but the following markers overlap: `platform_system != 'Windows'` and `python_full_version >= '3.11'`.
 
-    hint: replace `python_full_version >= '3.11'` with `platform_system == 'Windows' and python_full_version >= '3.11'`.
+    hint: replace `python_full_version >= '3.11'` with `python_full_version >= '3.11' and platform_system == 'Windows'`.
     "###);
 
     Ok(())
@@ -16537,9 +16537,9 @@ fn lock_multiple_sources_conflict() -> Result<()> {
       |
     9 |         iniconfig = [
       |                     ^
-    Source markers must be disjoint, but the following markers overlap: `sys_platform == 'win32' and python_full_version == '3.12.*'` and `sys_platform == 'win32'`.
+    Source markers must be disjoint, but the following markers overlap: `python_full_version == '3.12.*' and sys_platform == 'win32'` and `sys_platform == 'win32'`.
 
-    hint: replace `sys_platform == 'win32'` with `sys_platform == 'win32' and python_full_version != '3.12.*'`.
+    hint: replace `sys_platform == 'win32'` with `python_full_version != '3.12.*' and sys_platform == 'win32'`.
     "###);
 
     Ok(())
@@ -17168,11 +17168,11 @@ fn lock_multiple_sources_respect_marker() -> Result<()> {
         version = "0.1.0"
         source = { virtual = "." }
         dependencies = [
-            { name = "iniconfig", marker = "sys_platform != 'darwin' and platform_system == 'Windows'" },
+            { name = "iniconfig", marker = "platform_system == 'Windows' and sys_platform != 'darwin'" },
         ]
 
         [package.metadata]
-        requires-dist = [{ name = "iniconfig", marker = "sys_platform != 'darwin' and platform_system == 'Windows'" }]
+        requires-dist = [{ name = "iniconfig", marker = "platform_system == 'Windows' and sys_platform != 'darwin'" }]
         "###
         );
     });
@@ -17777,7 +17777,7 @@ fn lock_group_include() -> Result<()> {
         source = { registry = "https://pypi.org/simple" }
         dependencies = [
             { name = "attrs" },
-            { name = "cffi", marker = "os_name == 'nt' and implementation_name != 'pypy'" },
+            { name = "cffi", marker = "implementation_name != 'pypy' and os_name == 'nt'" },
             { name = "idna" },
             { name = "outcome" },
             { name = "sniffio" },
@@ -18959,7 +18959,7 @@ fn lock_recursive_extra() -> Result<()> {
             { name = "iniconfig" },
         ]
         qux = [
-            { name = "iniconfig", marker = "sys_platform == 'darwin' and python_full_version < '3.13'" },
+            { name = "iniconfig", marker = "python_full_version < '3.13' and sys_platform == 'darwin'" },
         ]
 
         [package.metadata]