From 7fd1cb994e0a09617bf2674056e438fed107f615 Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Wed, 27 Nov 2024 08:30:56 -0500 Subject: [PATCH] uv-pep508: add more routines for manipulating extras In the course of working on #9289, I've had to devise some additions to our markers. While we are still staying strictly compatible with the PEP 508 format, we will be abusing the `extra` expression to carry a lot more information. Specifically, we want the following additional operations: * Simplify `extra != 'foo'` * Remove all extra expressions * Remove everything except extra expressions My work on #9289 requires all of these (which will be in a future in PR). --- crates/uv-pep508/src/marker/algebra.rs | 61 ++++++++- crates/uv-pep508/src/marker/tree.rs | 173 +++++++++++++++++++++++++ 2 files changed, 233 insertions(+), 1 deletion(-) diff --git a/crates/uv-pep508/src/marker/algebra.rs b/crates/uv-pep508/src/marker/algebra.rs index 1258663e75908..0c7ff0ddc1836 100644 --- a/crates/uv-pep508/src/marker/algebra.rs +++ b/crates/uv-pep508/src/marker/algebra.rs @@ -412,7 +412,7 @@ impl InternerGuard<'_> { // Restrict this variable to the given output by merging it // with the relevant child. let node = if value { high } else { low }; - return node.negate(i); + return self.restrict(node.negate(i), f); } } @@ -421,6 +421,65 @@ impl InternerGuard<'_> { self.create_node(node.var.clone(), children) } + /// Returns a new tree where the only nodes remaining are non-`extra` + /// nodes. + /// + /// If there are only `extra` nodes, then this returns a tree that is + /// always true. + /// + /// This works by assuming all `extra` nodes are always true. + pub(crate) fn without_extras(&mut self, mut i: NodeId) -> NodeId { + if matches!(i, NodeId::TRUE | NodeId::FALSE) { + return i; + } + + let parent = i; + let node = self.shared.node(i); + if matches!(node.var, Variable::Extra(_)) { + i = NodeId::FALSE; + for child in node.children.nodes() { + i = self.or(i, child.negate(parent)); + } + if i.is_true() { + return NodeId::TRUE; + } + self.without_extras(i) + } else { + // Restrict all nodes recursively. + let children = node.children.map(i, |node| self.without_extras(node)); + self.create_node(node.var.clone(), children) + } + } + + /// Returns a new tree where the only nodes remaining are `extra` nodes. + /// + /// If there are no extra nodes, then this returns a tree that is always + /// true. + /// + /// This works by assuming all non-`extra` nodes are always true. + pub(crate) fn only_extras(&mut self, mut i: NodeId) -> NodeId { + if matches!(i, NodeId::TRUE | NodeId::FALSE) { + return i; + } + + let parent = i; + let node = self.shared.node(i); + if !matches!(node.var, Variable::Extra(_)) { + i = NodeId::FALSE; + for child in node.children.nodes() { + i = self.or(i, child.negate(parent)); + } + if i.is_true() { + return NodeId::TRUE; + } + self.only_extras(i) + } else { + // Restrict all nodes recursively. + let children = node.children.map(i, |node| self.only_extras(node)); + self.create_node(node.var.clone(), children) + } + } + /// Simplify this tree by *assuming* that the Python version range provided /// is true and that the complement of it is false. /// diff --git a/crates/uv-pep508/src/marker/tree.rs b/crates/uv-pep508/src/marker/tree.rs index ab0fd356e8a0f..b42ba76b99ba6 100644 --- a/crates/uv-pep508/src/marker/tree.rs +++ b/crates/uv-pep508/src/marker/tree.rs @@ -1064,6 +1064,21 @@ impl MarkerTree { self.simplify_extras_with(|name| extras.contains(name)) } + /// Remove negated extras from a marker, returning `None` if the marker + /// tree evaluates to `true`. + /// + /// Any negated `extra` markers that are always `true` given the provided + /// extras will be removed. Any `extra` markers that are always `false` + /// given the provided extras will be left unchanged. + /// + /// For example, if `dev` is a provided extra, given `sys_platform + /// == 'linux' and extra != 'dev'`, the marker will be simplified to + /// `sys_platform == 'linux'`. + #[must_use] + pub fn simplify_not_extras(self, extras: &[ExtraName]) -> MarkerTree { + self.simplify_not_extras_with(|name| extras.contains(name)) + } + /// Remove the extras from a marker, returning `None` if the marker tree evaluates to `true`. /// /// Any `extra` markers that are always `true` given the provided predicate will be removed. @@ -1083,12 +1098,57 @@ impl MarkerTree { self.simplify_extras_with_impl(&is_extra) } + /// Remove negated extras from a marker, returning `None` if the marker tree evaluates to + /// `true`. + /// + /// Any negated `extra` markers that are always `true` given the provided + /// predicate will be removed. Any `extra` markers that are always `false` + /// given the provided predicate will be left unchanged. + /// + /// For example, if `is_extra('dev')` is true, given + /// `sys_platform == 'linux' and extra != 'dev'`, the marker will be simplified to + /// `sys_platform == 'linux'`. + #[must_use] + pub fn simplify_not_extras_with(self, is_extra: impl Fn(&ExtraName) -> bool) -> MarkerTree { + // Because `simplify_extras_with_impl` is recursive, and we need to use + // our predicate in recursive calls, we need the predicate itself to + // have some indirection (or else we'd have to clone it). To avoid a + // recursive type at codegen time, we just introduce the indirection + // here, but keep the calling API ergonomic. + self.simplify_not_extras_with_impl(&is_extra) + } + + /// Returns a new `MarkerTree` where all `extra` expressions are removed. + /// + /// If the marker only consisted of `extra` expressions, then a marker that + /// is always true is returned. + #[must_use] + pub fn without_extras(self) -> MarkerTree { + MarkerTree(INTERNER.lock().without_extras(self.0)) + } + + /// Returns a new `MarkerTree` where only `extra` expressions are removed. + /// + /// If the marker did not contain any `extra` expressions, then a marker + /// that is always true is returned. + #[must_use] + pub fn only_extras(self) -> MarkerTree { + MarkerTree(INTERNER.lock().only_extras(self.0)) + } + fn simplify_extras_with_impl(self, is_extra: &impl Fn(&ExtraName) -> bool) -> MarkerTree { MarkerTree(INTERNER.lock().restrict(self.0, &|var| match var { Variable::Extra(name) => is_extra(name.extra()).then_some(true), _ => None, })) } + + fn simplify_not_extras_with_impl(self, is_extra: &impl Fn(&ExtraName) -> bool) -> MarkerTree { + MarkerTree(INTERNER.lock().restrict(self.0, &|var| match var { + Variable::Extra(name) => is_extra(name.extra()).then_some(false), + _ => None, + })) + } } impl fmt::Debug for MarkerTree { @@ -2071,6 +2131,59 @@ mod test { assert_eq!(simplified, expected); } + #[test] + fn test_simplify_not_extras() { + // Given `os_name == "nt" and extra != "dev"`, simplify to `os_name == "nt"`. + let markers = MarkerTree::from_str(r#"os_name == "nt" and extra != "dev""#).unwrap(); + let simplified = markers.simplify_not_extras(&[ExtraName::from_str("dev").unwrap()]); + let expected = MarkerTree::from_str(r#"os_name == "nt""#).unwrap(); + assert_eq!(simplified, expected); + + // Given `os_name == "nt" or extra != "dev"`, remove the marker entirely. + let markers = MarkerTree::from_str(r#"os_name == "nt" or extra != "dev""#).unwrap(); + let simplified = markers.simplify_not_extras(&[ExtraName::from_str("dev").unwrap()]); + assert_eq!(simplified, MarkerTree::TRUE); + + // Given `extra != "dev"`, remove the marker entirely. + let markers = MarkerTree::from_str(r#"extra != "dev""#).unwrap(); + let simplified = markers.simplify_not_extras(&[ExtraName::from_str("dev").unwrap()]); + assert_eq!(simplified, MarkerTree::TRUE); + + // Given `extra != "dev" and extra != "test"`, simplify to `extra != "test"`. + let markers = MarkerTree::from_str(r#"extra != "dev" and extra != "test""#).unwrap(); + let simplified = markers.simplify_not_extras(&[ExtraName::from_str("dev").unwrap()]); + let expected = MarkerTree::from_str(r#"extra != "test""#).unwrap(); + assert_eq!(simplified, expected); + + // Given `os_name == "nt" and extra != "test"`, don't simplify. + let markers = MarkerTree::from_str(r#"os_name != "nt" and extra != "test""#).unwrap(); + let simplified = markers + .clone() + .simplify_not_extras(&[ExtraName::from_str("dev").unwrap()]); + assert_eq!(simplified, markers); + + // Given `os_name == "nt" and (python_version == "3.7" or extra != "dev")`, simplify to + // `os_name == "nt". + let markers = MarkerTree::from_str( + r#"os_name == "nt" and (python_version == "3.7" or extra != "dev")"#, + ) + .unwrap(); + let simplified = markers.simplify_not_extras(&[ExtraName::from_str("dev").unwrap()]); + let expected = MarkerTree::from_str(r#"os_name == "nt""#).unwrap(); + assert_eq!(simplified, expected); + + // Given `os_name == "nt" or (python_version == "3.7" and extra != "dev")`, simplify to + // `os_name == "nt" or python_version == "3.7"`. + let markers = MarkerTree::from_str( + r#"os_name == "nt" or (python_version == "3.7" and extra != "dev")"#, + ) + .unwrap(); + let simplified = markers.simplify_not_extras(&[ExtraName::from_str("dev").unwrap()]); + let expected = + MarkerTree::from_str(r#"os_name == "nt" or python_version == "3.7""#).unwrap(); + assert_eq!(simplified, expected); + } + #[test] fn test_marker_simplification() { assert_false("python_version == '3.9.1'"); @@ -3011,4 +3124,64 @@ mod test { m("python_full_version >= '3.9'"), ); } + + #[test] + fn without_extras() { + assert_eq!( + m("os_name == 'Linux'").without_extras(), + m("os_name == 'Linux'"), + ); + assert!(m("extra == 'foo'").without_extras().is_true()); + assert_eq!( + m("os_name == 'Linux' and extra == 'foo'").without_extras(), + m("os_name == 'Linux'"), + ); + + assert!(m(" + (os_name == 'Linux' and extra == 'foo') + or (os_name != 'Linux' and extra == 'bar')",) + .without_extras() + .is_true()); + + assert_eq!( + m("os_name == 'Linux' and extra != 'foo'").without_extras(), + m("os_name == 'Linux'"), + ); + + assert!( + m("extra != 'extra-project-bar' and extra == 'extra-project-foo'") + .without_extras() + .is_true() + ); + } + + #[test] + fn only_extras() { + assert!(m("os_name == 'Linux'").only_extras().is_true()); + assert_eq!(m("extra == 'foo'").only_extras(), m("extra == 'foo'")); + assert_eq!( + m("os_name == 'Linux' and extra == 'foo'").only_extras(), + m("extra == 'foo'"), + ); + assert!(m(" + (os_name == 'foo' and extra == 'foo') + or (os_name == 'bar' and extra != 'foo')",) + .only_extras() + .is_true()); + assert_eq!( + m(" + (os_name == 'Linux' and extra == 'foo') + or (os_name != 'Linux' and extra == 'bar')") + .only_extras(), + m("extra == 'foo' or extra == 'bar'"), + ); + + assert_eq!( + m(" + (implementation_name != 'pypy' and os_name == 'nt' and sys_platform == 'darwin') + or (os_name == 'nt' and sys_platform == 'win32')") + .only_extras(), + m("os_name == 'Linux' or os_name != 'Linux'"), + ); + } }