diff --git a/crates/ty_python_semantic/src/types/constraints.rs b/crates/ty_python_semantic/src/types/constraints.rs index 3f63735f68..edc40c3a22 100644 --- a/crates/ty_python_semantic/src/types/constraints.rs +++ b/crates/ty_python_semantic/src/types/constraints.rs @@ -172,9 +172,10 @@ where /// /// This is called a "set of constraint sets", and denoted _𝒮_, in [[POPL2015][]]. /// -/// The underlying representation tracks the order that individual constraints appear in the -/// underlying Python source. For this to work, you should ensure that you call "combining" -/// operators like [`and`][Self::and] and [`or`][Self::or] in a consistent order. +/// The underlying representation tracks the order that individual constraints are added to the +/// constraint set, which typically tracks when they appear in the underlying Python source. For +/// this to work, you should ensure that you call "combining" operators like [`and`][Self::and] and +/// [`or`][Self::or] in a consistent order. /// /// [POPL2015]: https://doi.org/10.1145/2676726.2676991 #[derive(Clone, Copy, Debug, Eq, Hash, PartialEq, get_size2::GetSize, salsa::Update)] @@ -349,12 +350,18 @@ impl<'db> ConstraintSet<'db> { } /// Updates this constraint set to hold the union of itself and another constraint set. + /// + /// In the result, `self` will appear before `other` according to the `source_order` of the BDD + /// nodes. pub(crate) fn union(&mut self, db: &'db dyn Db, other: Self) -> Self { self.node = self.node.or(db, other.node); *self } /// Updates this constraint set to hold the intersection of itself and another constraint set. + /// + /// In the result, `self` will appear before `other` according to the `source_order` of the BDD + /// nodes. pub(crate) fn intersect(&mut self, db: &'db dyn Db, other: Self) -> Self { self.node = self.node.and(db, other.node); *self @@ -370,6 +377,9 @@ impl<'db> ConstraintSet<'db> { /// Returns the intersection of this constraint set and another. The other constraint set is /// provided as a thunk, to implement short-circuiting: the thunk is not forced if the /// constraint set is already saturated. + /// + /// In the result, `self` will appear before `other` according to the `source_order` of the BDD + /// nodes. pub(crate) fn and(mut self, db: &'db dyn Db, other: impl FnOnce() -> Self) -> Self { if !self.is_never_satisfied(db) { self.intersect(db, other()); @@ -380,6 +390,9 @@ impl<'db> ConstraintSet<'db> { /// Returns the union of this constraint set and another. The other constraint set is provided /// as a thunk, to implement short-circuiting: the thunk is not forced if the constraint set is /// already saturated. + /// + /// In the result, `self` will appear before `other` according to the `source_order` of the BDD + /// nodes. pub(crate) fn or(mut self, db: &'db dyn Db, other: impl FnOnce() -> Self) -> Self { if !self.is_always_satisfied(db) { self.union(db, other()); @@ -388,10 +401,17 @@ impl<'db> ConstraintSet<'db> { } /// Returns a constraint set encoding that this constraint set implies another. + /// + /// In the result, `self` will appear before `other` according to the `source_order` of the BDD + /// nodes. pub(crate) fn implies(self, db: &'db dyn Db, other: impl FnOnce() -> Self) -> Self { self.negate(db).or(db, other) } + /// Returns a constraint set encoding that this constraint set is equivalent to another. + /// + /// In the result, `self` will appear before `other` according to the `source_order` of the BDD + /// nodes. pub(crate) fn iff(self, db: &'db dyn Db, other: Self) -> Self { ConstraintSet { node: self.node.iff(db, other.node), @@ -828,12 +848,12 @@ impl<'db> ConstrainedTypeVar<'db> { /// ordering that we use for constraint set BDDs. /// /// In addition to this BDD variable ordering, we also track a `source_order` for each individual -/// constraint. This tracks the order in which constraints appear in the underlying Python source -/// code. This provides an ordering that is stable across multiple runs, for consistent test and -/// diagnostic output. (We cannot use this ordering as our BDD variable ordering, since we might -/// have different `source_order`s for the same constraints if they appear in multiple constraint -/// sets, but we need the BDD variable ordering to be consistent across all BDDs that we create in -/// the process.) +/// constraint. This records the order in which constraints are added to the constraint set, which +/// typically tracks when they appear in the underlying Python source code. This provides an +/// ordering that is stable across multiple runs, for consistent test and diagnostic output. (We +/// cannot use this ordering as our BDD variable ordering, since we calculate it from already +/// constructed BDDs, and we need the BDD variable ordering to be fixed and available before +/// construction starts.) #[derive(Clone, Copy, Debug, Eq, Hash, PartialEq, get_size2::GetSize, salsa::Update)] enum Node<'db> { AlwaysFalse, @@ -1059,60 +1079,74 @@ impl<'db> Node<'db> { } /// Returns the `or` or union of two BDDs. + /// + /// In the result, `self` will appear before `other` according to the `source_order` of the BDD + /// nodes. fn or(self, db: &'db dyn Db, other: Self) -> Self { - let rhs_offset = self.max_source_order(db); - self.or_inner(db, other, rhs_offset) + // To ensure that `self` appears before `other` in `source_order`, we add the maximum + // `source_order` of the lhs to all of the `source_order`s in the rhs. + let other_offset = self.max_source_order(db); + self.or_inner(db, other, other_offset) } - fn or_inner(self, db: &'db dyn Db, other: Self, rhs_offset: usize) -> Self { + fn or_inner(self, db: &'db dyn Db, other: Self, other_offset: usize) -> Self { match (self, other) { (Node::AlwaysTrue, Node::AlwaysTrue) => Node::AlwaysTrue, - (Node::AlwaysTrue, Node::Interior(rhs)) => Node::new( + (Node::AlwaysTrue, Node::Interior(other_interior)) => Node::new( db, - rhs.constraint(db), + other_interior.constraint(db), Node::AlwaysTrue, Node::AlwaysTrue, - rhs.source_order(db) + rhs_offset, + other_interior.source_order(db) + other_offset, ), - (Node::Interior(lhs), Node::AlwaysTrue) => Node::new( + (Node::Interior(self_interior), Node::AlwaysTrue) => Node::new( db, - lhs.constraint(db), + self_interior.constraint(db), Node::AlwaysTrue, Node::AlwaysTrue, - lhs.source_order(db), + self_interior.source_order(db), ), - (Node::AlwaysFalse, rhs) => rhs.with_adjusted_source_order(db, rhs_offset), - (lhs, Node::AlwaysFalse) => lhs, - (Node::Interior(lhs), Node::Interior(rhs)) => lhs.or(db, rhs, rhs_offset), + (Node::AlwaysFalse, _) => other.with_adjusted_source_order(db, other_offset), + (_, Node::AlwaysFalse) => self, + (Node::Interior(self_interior), Node::Interior(other_interior)) => { + self_interior.or(db, other_interior, other_offset) + } } } /// Returns the `and` or intersection of two BDDs. + /// + /// In the result, `self` will appear before `other` according to the `source_order` of the BDD + /// nodes. fn and(self, db: &'db dyn Db, other: Self) -> Self { - let rhs_offset = self.max_source_order(db); - self.and_inner(db, other, rhs_offset) + // To ensure that `self` appears before `other` in `source_order`, we add the maximum + // `source_order` of the lhs to all of the `source_order`s in the rhs. + let other_offset = self.max_source_order(db); + self.and_inner(db, other, other_offset) } - fn and_inner(self, db: &'db dyn Db, other: Self, rhs_offset: usize) -> Self { + fn and_inner(self, db: &'db dyn Db, other: Self, other_offset: usize) -> Self { match (self, other) { (Node::AlwaysFalse, Node::AlwaysFalse) => Node::AlwaysFalse, - (Node::AlwaysFalse, Node::Interior(rhs)) => Node::new( + (Node::AlwaysFalse, Node::Interior(other_interior)) => Node::new( db, - rhs.constraint(db), + other_interior.constraint(db), Node::AlwaysFalse, Node::AlwaysFalse, - rhs.source_order(db) + rhs_offset, + other_interior.source_order(db) + other_offset, ), - (Node::Interior(lhs), Node::AlwaysFalse) => Node::new( + (Node::Interior(self_interior), Node::AlwaysFalse) => Node::new( db, - lhs.constraint(db), + self_interior.constraint(db), Node::AlwaysFalse, Node::AlwaysFalse, - lhs.source_order(db), + self_interior.source_order(db), ), - (Node::AlwaysTrue, rhs) => rhs.with_adjusted_source_order(db, rhs_offset), - (lhs, Node::AlwaysTrue) => lhs, - (Node::Interior(lhs), Node::Interior(rhs)) => lhs.and(db, rhs, rhs_offset), + (Node::AlwaysTrue, _) => other.with_adjusted_source_order(db, other_offset), + (_, Node::AlwaysTrue) => self, + (Node::Interior(self_interior), Node::Interior(other_interior)) => { + self_interior.and(db, other_interior, other_offset) + } } } @@ -1123,12 +1157,17 @@ impl<'db> Node<'db> { /// Returns a new BDD that evaluates to `true` when both input BDDs evaluate to the same /// result. + /// + /// In the result, `self` will appear before `other` according to the `source_order` of the BDD + /// nodes. fn iff(self, db: &'db dyn Db, other: Self) -> Self { - let rhs_offset = self.max_source_order(db); - self.iff_inner(db, other, rhs_offset) + // To ensure that `self` appears before `other` in `source_order`, we add the maximum + // `source_order` of the lhs to all of the `source_order`s in the rhs. + let other_offset = self.max_source_order(db); + self.iff_inner(db, other, other_offset) } - fn iff_inner(self, db: &'db dyn Db, other: Self, rhs_offset: usize) -> Self { + fn iff_inner(self, db: &'db dyn Db, other: Self, other_offset: usize) -> Self { match (self, other) { (Node::AlwaysFalse, Node::AlwaysFalse) | (Node::AlwaysTrue, Node::AlwaysTrue) => { Node::AlwaysTrue @@ -1139,18 +1178,18 @@ impl<'db> Node<'db> { (Node::AlwaysTrue | Node::AlwaysFalse, Node::Interior(interior)) => Node::new( db, interior.constraint(db), - self.iff_inner(db, interior.if_true(db), rhs_offset), - self.iff_inner(db, interior.if_false(db), rhs_offset), - interior.source_order(db) + rhs_offset, + self.iff_inner(db, interior.if_true(db), other_offset), + self.iff_inner(db, interior.if_false(db), other_offset), + interior.source_order(db) + other_offset, ), (Node::Interior(interior), Node::AlwaysTrue | Node::AlwaysFalse) => Node::new( db, interior.constraint(db), - interior.if_true(db).iff_inner(db, other, rhs_offset), - interior.if_false(db).iff_inner(db, other, rhs_offset), + interior.if_true(db).iff_inner(db, other, other_offset), + interior.if_false(db).iff_inner(db, other, other_offset), interior.source_order(db), ), - (Node::Interior(a), Node::Interior(b)) => a.iff(db, b, rhs_offset), + (Node::Interior(a), Node::Interior(b)) => a.iff(db, b, other_offset), } } @@ -1758,7 +1797,15 @@ struct InteriorNode<'db> { constraint: ConstrainedTypeVar<'db>, if_true: Node<'db>, if_false: Node<'db>, + + /// Represents the order in which this node's constraint was added to the containing constraint + /// set, relative to all of the other constraints in the set. This starts off at 1 for a simple + /// single-constraint set (e.g. created with [`Node::new_constraint`] or + /// [`Node::new_satisfied_constraint`]). It will get incremented, if needed, as that simple BDD + /// is combined into larger BDDs. source_order: usize, + + /// The maximum `source_order` across this node and all of its descendants. max_source_order: usize, } @@ -1779,39 +1826,40 @@ impl<'db> InteriorNode<'db> { } #[salsa::tracked(heap_size=ruff_memory_usage::heap_size)] - fn or(self, db: &'db dyn Db, other: Self, rhs_offset: usize) -> Node<'db> { + fn or(self, db: &'db dyn Db, other: Self, other_offset: usize) -> Node<'db> { let self_constraint = self.constraint(db); let other_constraint = other.constraint(db); match (self_constraint.ordering(db)).cmp(&other_constraint.ordering(db)) { Ordering::Equal => Node::new( db, self_constraint, - self.if_true(db).or_inner(db, other.if_true(db), rhs_offset), + self.if_true(db) + .or_inner(db, other.if_true(db), other_offset), self.if_false(db) - .or_inner(db, other.if_false(db), rhs_offset), + .or_inner(db, other.if_false(db), other_offset), self.source_order(db), ), Ordering::Less => Node::new( db, self_constraint, self.if_true(db) - .or_inner(db, Node::Interior(other), rhs_offset), + .or_inner(db, Node::Interior(other), other_offset), self.if_false(db) - .or_inner(db, Node::Interior(other), rhs_offset), + .or_inner(db, Node::Interior(other), other_offset), self.source_order(db), ), Ordering::Greater => Node::new( db, other_constraint, - Node::Interior(self).or_inner(db, other.if_true(db), rhs_offset), - Node::Interior(self).or_inner(db, other.if_false(db), rhs_offset), - other.source_order(db) + rhs_offset, + Node::Interior(self).or_inner(db, other.if_true(db), other_offset), + Node::Interior(self).or_inner(db, other.if_false(db), other_offset), + other.source_order(db) + other_offset, ), } } #[salsa::tracked(heap_size=ruff_memory_usage::heap_size)] - fn and(self, db: &'db dyn Db, other: Self, rhs_offset: usize) -> Node<'db> { + fn and(self, db: &'db dyn Db, other: Self, other_offset: usize) -> Node<'db> { let self_constraint = self.constraint(db); let other_constraint = other.constraint(db); match (self_constraint.ordering(db)).cmp(&other_constraint.ordering(db)) { @@ -1819,32 +1867,32 @@ impl<'db> InteriorNode<'db> { db, self_constraint, self.if_true(db) - .and_inner(db, other.if_true(db), rhs_offset), + .and_inner(db, other.if_true(db), other_offset), self.if_false(db) - .and_inner(db, other.if_false(db), rhs_offset), + .and_inner(db, other.if_false(db), other_offset), self.source_order(db), ), Ordering::Less => Node::new( db, self_constraint, self.if_true(db) - .and_inner(db, Node::Interior(other), rhs_offset), + .and_inner(db, Node::Interior(other), other_offset), self.if_false(db) - .and_inner(db, Node::Interior(other), rhs_offset), + .and_inner(db, Node::Interior(other), other_offset), self.source_order(db), ), Ordering::Greater => Node::new( db, other_constraint, - Node::Interior(self).and_inner(db, other.if_true(db), rhs_offset), - Node::Interior(self).and_inner(db, other.if_false(db), rhs_offset), - other.source_order(db) + rhs_offset, + Node::Interior(self).and_inner(db, other.if_true(db), other_offset), + Node::Interior(self).and_inner(db, other.if_false(db), other_offset), + other.source_order(db) + other_offset, ), } } #[salsa::tracked(heap_size=ruff_memory_usage::heap_size)] - fn iff(self, db: &'db dyn Db, other: Self, rhs_offset: usize) -> Node<'db> { + fn iff(self, db: &'db dyn Db, other: Self, other_offset: usize) -> Node<'db> { let self_constraint = self.constraint(db); let other_constraint = other.constraint(db); match (self_constraint.ordering(db)).cmp(&other_constraint.ordering(db)) { @@ -1852,26 +1900,26 @@ impl<'db> InteriorNode<'db> { db, self_constraint, self.if_true(db) - .iff_inner(db, other.if_true(db), rhs_offset), + .iff_inner(db, other.if_true(db), other_offset), self.if_false(db) - .iff_inner(db, other.if_false(db), rhs_offset), + .iff_inner(db, other.if_false(db), other_offset), self.source_order(db), ), Ordering::Less => Node::new( db, self_constraint, self.if_true(db) - .iff_inner(db, Node::Interior(other), rhs_offset), + .iff_inner(db, Node::Interior(other), other_offset), self.if_false(db) - .iff_inner(db, Node::Interior(other), rhs_offset), + .iff_inner(db, Node::Interior(other), other_offset), self.source_order(db), ), Ordering::Greater => Node::new( db, other_constraint, - Node::Interior(self).iff_inner(db, other.if_true(db), rhs_offset), - Node::Interior(self).iff_inner(db, other.if_false(db), rhs_offset), - other.source_order(db) + rhs_offset, + Node::Interior(self).iff_inner(db, other.if_true(db), other_offset), + Node::Interior(self).iff_inner(db, other.if_false(db), other_offset), + other.source_order(db) + other_offset, ), } }