add tests for NP-complete marker simplification cases

astral-sh · Aug 8, 2024 · f86e792 · f86e792
1 parent 20386bd
commit f86e792
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 12 deletions.
diff --git a/crates/pep508-rs/src/marker/simplify.rs b/crates/pep508-rs/src/marker/simplify.rs
@@ -193,13 +193,23 @@ fn simplify(dnf: &mut Vec<Vec<MarkerExpression>>) {
                 // For example, `A or (not A and B)` can be simplified to `A or B`,
                 // eliminating the `not A` term.
                 if other_clause.iter().all(|term| {
+                    // For the term to be redundant in this clause, the other clause can
+                    // contain the negation of the term but not the term itself.
                     if term == skipped_term {
                         return false;
                     }
+                    if is_negation(term, skipped_term) {
+                        return true;
+                    }
 
                     // TODO(ibraheem): if we intern variables we could reduce this
                     // from a linear search to an integer `HashSet` lookup
-                    clause.contains(term) || is_negated(term, skipped_term)
+                    clause
+                        .iter()
+                        .position(|x| x == term)
+                        // If the term was already removed from this one, we cannot
+                        // depend on it for further simplification.
+                        .is_some_and(|i| !redundant_terms.contains(&i))
                 }) {
                     redundant_terms.push(skipped);
                     continue 'term;
@@ -308,7 +318,7 @@ where
 }
 
 /// Returns `true` if the LHS is the negation of the RHS, or vice versa.
-fn is_negated(left: &MarkerExpression, right: &MarkerExpression) -> bool {
+fn is_negation(left: &MarkerExpression, right: &MarkerExpression) -> bool {
     match left {
         MarkerExpression::Version { key, specifier } => {
             let MarkerExpression::Version {

diff --git a/crates/pep508-rs/src/marker/tree.rs b/crates/pep508-rs/src/marker/tree.rs
@@ -2047,6 +2047,36 @@ mod test {
 
     #[test]
     fn test_complex_marker_simplification() {
+        // This expression should simplify to:
+        // `(implementation_name == 'pypy' and sys_platform != 'win32')
+        //   or (sys_platform == 'win32' or os_name != 'nt')
+        //   or (implementation != 'pypy' or os_name == 'nt')`
+        //
+        // However, simplifying this expression is NP-complete and requires an exponential
+        // algorithm such as Quine-McCluskey, which is not currently implemented.
+        assert_simplifies(
+            "(implementation_name == 'pypy' and sys_platform != 'win32')
+                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') \
+                or (os_name == 'nt' and sys_platform != 'win32')",
+        );
+
+        // This is another case we cannot simplify fully, depending 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')
+                or (os_name == 'Linux' and platform_system == 'win32' and sys_platform == 'a')
+                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')
+                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')"
+        );
+
         assert_simplifies("python_version > '3.7'", "python_version > '3.7'");
 
         assert_simplifies(
@@ -2063,16 +2093,6 @@ mod test {
                 or (python_version == '3.8' and sys_platform == 'win32')",
         );
 
-        assert_simplifies(
-            "(os_name == 'Linux' and platform_system == 'win32')
-                or (os_name == 'Linux' and platform_system == 'win32' and sys_platform == 'a')
-                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')
-                or (os_name != 'Linux' and platform_system != 'win32' and sys_platform == 'x')",
-            "sys_platform == 'x' or (os_name == 'Linux' and platform_system == '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')"