From 7d3b7c57545282cebfe63215d3fba98fca34db4d Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Mon, 15 Dec 2025 14:24:08 -0500 Subject: [PATCH] [ty] Consistent ordering of constraint set specializations, take 2 (#21983) In https://github.com/astral-sh/ruff/pull/21957, we tried to use `union_or_intersection_elements_ordering` to provide a stable ordering of the union and intersection elements that are created when determining which type a typevar should specialize to. @AlexWaygood [pointed out](https://github.com/astral-sh/ruff/pull/21551#discussion_r2616543762) that this won't work, since that provides a consistent ordering within a single process run, but does not provide a stable ordering across runs. This is an attempt to produce a proper stable ordering for constraint sets, so that we end up with consistent diagnostic and test output. We do this by maintaining a new `source_order` field on each interior BDD node, which records when that node's constraint was added to the set. Several of the BDD operators (`and`, `or`, etc) now have `_with_offset` variants, which update each `source_order` in the rhs to be larger than any of the `source_order`s in the lhs. This is what causes that field to be in line with (a) when you add each constraint to the set, and (b) the order of the parameters you provide to `and`, `or`, etc. Then we sort by that new field before constructing the union/intersection types when creating a specialization. --- .../ty_python_semantic/src/types/builder.rs | 10 - .../src/types/constraints.rs | 806 ++++++++++++------ 2 files changed, 568 insertions(+), 248 deletions(-) diff --git a/crates/ty_python_semantic/src/types/builder.rs b/crates/ty_python_semantic/src/types/builder.rs index ae2c21759b..c279ddbd27 100644 --- a/crates/ty_python_semantic/src/types/builder.rs +++ b/crates/ty_python_semantic/src/types/builder.rs @@ -689,20 +689,10 @@ impl<'db> IntersectionBuilder<'db> { } } - pub(crate) fn order_elements(mut self, val: bool) -> Self { - self.order_elements = val; - self - } - pub(crate) fn add_positive(self, ty: Type<'db>) -> Self { self.add_positive_impl(ty, &mut vec![]) } - pub(crate) fn add_positive_in_place(&mut self, ty: Type<'db>) { - let updated = std::mem::replace(self, Self::empty(self.db)).add_positive(ty); - *self = updated; - } - pub(crate) fn add_positive_impl( mut self, ty: Type<'db>, diff --git a/crates/ty_python_semantic/src/types/constraints.rs b/crates/ty_python_semantic/src/types/constraints.rs index d4f3e436f6..82d6c3b0ad 100644 --- a/crates/ty_python_semantic/src/types/constraints.rs +++ b/crates/ty_python_semantic/src/types/constraints.rs @@ -78,8 +78,8 @@ use salsa::plumbing::AsId; use crate::types::generics::{GenericContext, InferableTypeVars, Specialization}; use crate::types::visitor::{TypeCollector, TypeVisitor, walk_type_with_recursion_guard}; use crate::types::{ - BoundTypeVarIdentity, BoundTypeVarInstance, IntersectionBuilder, IntersectionType, Type, - TypeVarBoundOrConstraints, UnionBuilder, UnionType, walk_bound_type_var_type, + BoundTypeVarIdentity, BoundTypeVarInstance, IntersectionType, Type, TypeVarBoundOrConstraints, + UnionType, walk_bound_type_var_type, }; use crate::{Db, FxOrderSet}; @@ -172,6 +172,11 @@ where /// /// This is called a "set of constraint sets", and denoted _๐’ฎ_, in [[POPL2015][]]. /// +/// 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)] pub struct ConstraintSet<'db> { @@ -283,7 +288,7 @@ impl<'db> ConstraintSet<'db> { BoundTypeVarIdentity<'db>, FxHashSet>, > = FxHashMap::default(); - self.node.for_each_constraint(db, &mut |constraint| { + self.node.for_each_constraint(db, &mut |constraint, _| { let visitor = CollectReachability::default(); visitor.visit_type(db, constraint.lower(db)); visitor.visit_type(db, constraint.upper(db)); @@ -345,14 +350,20 @@ 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.node = self.node.or_with_offset(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.node = self.node.and_with_offset(db, other.node); *self } @@ -366,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()); @@ -376,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()); @@ -384,13 +401,20 @@ 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), + node: self.node.iff_with_offset(db, other.node), } } @@ -496,7 +520,7 @@ impl<'db> ConstrainedTypeVar<'db> { if let Type::Union(lower_union) = lower { let mut result = Node::AlwaysTrue; for lower_element in lower_union.elements(db) { - result = result.and( + result = result.and_with_offset( db, ConstrainedTypeVar::new_node(db, typevar, *lower_element, upper), ); @@ -511,13 +535,13 @@ impl<'db> ConstrainedTypeVar<'db> { { let mut result = Node::AlwaysTrue; for upper_element in upper_intersection.iter_positive(db) { - result = result.and( + result = result.and_with_offset( db, ConstrainedTypeVar::new_node(db, typevar, lower, upper_element), ); } for upper_element in upper_intersection.iter_negative(db) { - result = result.and( + result = result.and_with_offset( db, ConstrainedTypeVar::new_node(db, typevar, lower, upper_element.negate(db)), ); @@ -552,6 +576,7 @@ impl<'db> ConstrainedTypeVar<'db> { return Node::new_constraint( db, ConstrainedTypeVar::new(db, typevar, Type::Never, Type::object()), + 1, ) .negate(db); } @@ -603,6 +628,7 @@ impl<'db> ConstrainedTypeVar<'db> { Type::TypeVar(bound), Type::TypeVar(bound), ), + 1, ) } @@ -613,10 +639,12 @@ impl<'db> ConstrainedTypeVar<'db> { let lower = Node::new_constraint( db, ConstrainedTypeVar::new(db, lower, Type::Never, Type::TypeVar(typevar)), + 1, ); let upper = Node::new_constraint( db, ConstrainedTypeVar::new(db, upper, Type::TypeVar(typevar), Type::object()), + 1, ); lower.and(db, upper) } @@ -626,6 +654,7 @@ impl<'db> ConstrainedTypeVar<'db> { let lower = Node::new_constraint( db, ConstrainedTypeVar::new(db, lower, Type::Never, Type::TypeVar(typevar)), + 1, ); let upper = if upper.is_object() { Node::AlwaysTrue @@ -645,11 +674,12 @@ impl<'db> ConstrainedTypeVar<'db> { let upper = Node::new_constraint( db, ConstrainedTypeVar::new(db, upper, Type::TypeVar(typevar), Type::object()), + 1, ); lower.and(db, upper) } - _ => Node::new_constraint(db, ConstrainedTypeVar::new(db, typevar, lower, upper)), + _ => Node::new_constraint(db, ConstrainedTypeVar::new(db, typevar, lower, upper), 1), } } @@ -816,6 +846,14 @@ impl<'db> ConstrainedTypeVar<'db> { /// BDD nodes are also _ordered_, meaning that every path from the root of a BDD to a terminal node /// visits variables in the same order. [`ConstrainedTypeVar::ordering`] defines the variable /// 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 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, @@ -830,6 +868,7 @@ impl<'db> Node<'db> { constraint: ConstrainedTypeVar<'db>, if_true: Node<'db>, if_false: Node<'db>, + source_order: usize, ) -> Self { debug_assert!((if_true.root_constraint(db)).is_none_or(|root_constraint| { root_constraint.ordering(db) > constraint.ordering(db) @@ -842,36 +881,60 @@ impl<'db> Node<'db> { if if_true == Node::AlwaysFalse && if_false == Node::AlwaysFalse { return Node::AlwaysFalse; } - Self::Interior(InteriorNode::new(db, constraint, if_true, if_false)) + let max_source_order = source_order + .max(if_true.max_source_order(db)) + .max(if_false.max_source_order(db)); + Self::Interior(InteriorNode::new( + db, + constraint, + if_true, + if_false, + source_order, + max_source_order, + )) } /// Creates a new BDD node for an individual constraint. (The BDD will evaluate to `true` when /// the constraint holds, and to `false` when it does not.) - fn new_constraint(db: &'db dyn Db, constraint: ConstrainedTypeVar<'db>) -> Self { + fn new_constraint( + db: &'db dyn Db, + constraint: ConstrainedTypeVar<'db>, + source_order: usize, + ) -> Self { Self::Interior(InteriorNode::new( db, constraint, Node::AlwaysTrue, Node::AlwaysFalse, + source_order, + source_order, )) } /// Creates a new BDD node for a positive or negative individual constraint. (For a positive /// constraint, this returns the same BDD node as [`new_constraint`][Self::new_constraint]. For /// a negative constraint, it returns the negation of that BDD node.) - fn new_satisfied_constraint(db: &'db dyn Db, constraint: ConstraintAssignment<'db>) -> Self { + fn new_satisfied_constraint( + db: &'db dyn Db, + constraint: ConstraintAssignment<'db>, + source_order: usize, + ) -> Self { match constraint { ConstraintAssignment::Positive(constraint) => Self::Interior(InteriorNode::new( db, constraint, Node::AlwaysTrue, Node::AlwaysFalse, + source_order, + source_order, )), ConstraintAssignment::Negative(constraint) => Self::Interior(InteriorNode::new( db, constraint, Node::AlwaysFalse, Node::AlwaysTrue, + source_order, + source_order, )), } } @@ -885,6 +948,31 @@ impl<'db> Node<'db> { } } + fn max_source_order(self, db: &'db dyn Db) -> usize { + match self { + Node::Interior(interior) => interior.max_source_order(db), + Node::AlwaysTrue | Node::AlwaysFalse => 0, + } + } + + /// Returns a copy of this BDD node with all `source_order`s adjusted by the given amount. + fn with_adjusted_source_order(self, db: &'db dyn Db, delta: usize) -> Self { + if delta == 0 { + return self; + } + match self { + Node::AlwaysTrue => Node::AlwaysTrue, + Node::AlwaysFalse => Node::AlwaysFalse, + Node::Interior(interior) => Node::new( + db, + interior.constraint(db), + interior.if_true(db).with_adjusted_source_order(db, delta), + interior.if_false(db).with_adjusted_source_order(db, delta), + interior.source_order(db) + delta, + ), + } + } + /// Returns whether this BDD represent the constant function `true`. fn is_always_satisfied(self, db: &'db dyn Db) -> bool { match self { @@ -991,41 +1079,86 @@ 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_with_offset(self, db: &'db dyn Db, other: Self) -> Self { + // 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. + // + // TODO: If we store `other_offset` as a new field on InteriorNode, we might be able to + // avoid all of the extra work in the calls to with_adjusted_source_order, and apply the + // adjustment lazily when walking a BDD tree. (ditto below in the other _with_offset + // methods) + let other_offset = self.max_source_order(db); + self.or_inner(db, other, other_offset) + } + fn or(self, db: &'db dyn Db, other: Self) -> Self { + self.or_inner(db, other, 0) + } + + fn or_inner(self, db: &'db dyn Db, other: Self, other_offset: usize) -> Self { match (self, other) { (Node::AlwaysTrue, Node::AlwaysTrue) => Node::AlwaysTrue, - (Node::AlwaysFalse, other) | (other, Node::AlwaysFalse) => other, - (Node::AlwaysTrue, Node::Interior(interior)) - | (Node::Interior(interior), Node::AlwaysTrue) => Node::new( + (Node::AlwaysTrue, Node::Interior(other_interior)) => Node::new( db, - interior.constraint(db), + other_interior.constraint(db), Node::AlwaysTrue, Node::AlwaysTrue, + other_interior.source_order(db) + other_offset, ), - (Node::Interior(a), Node::Interior(b)) => { - // OR is commutative, which lets us halve the cache requirements - let (a, b) = if b.0 < a.0 { (b, a) } else { (a, b) }; - a.or(db, b) + (Node::Interior(self_interior), Node::AlwaysTrue) => Node::new( + db, + self_interior.constraint(db), + Node::AlwaysTrue, + Node::AlwaysTrue, + self_interior.source_order(db), + ), + (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_with_offset(self, db: &'db dyn Db, other: Self) -> Self { + // 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(self, db: &'db dyn Db, other: Self) -> Self { + self.and_inner(db, other, 0) + } + + fn and_inner(self, db: &'db dyn Db, other: Self, other_offset: usize) -> Self { match (self, other) { (Node::AlwaysFalse, Node::AlwaysFalse) => Node::AlwaysFalse, - (Node::AlwaysTrue, other) | (other, Node::AlwaysTrue) => other, - (Node::AlwaysFalse, Node::Interior(interior)) - | (Node::Interior(interior), Node::AlwaysFalse) => Node::new( + (Node::AlwaysFalse, Node::Interior(other_interior)) => Node::new( db, - interior.constraint(db), + other_interior.constraint(db), Node::AlwaysFalse, Node::AlwaysFalse, + other_interior.source_order(db) + other_offset, ), - (Node::Interior(a), Node::Interior(b)) => { - // AND is commutative, which lets us halve the cache requirements - let (a, b) = if b.0 < a.0 { (b, a) } else { (a, b) }; - a.and(db, b) + (Node::Interior(self_interior), Node::AlwaysFalse) => Node::new( + db, + self_interior.constraint(db), + Node::AlwaysFalse, + Node::AlwaysFalse, + self_interior.source_order(db), + ), + (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) } } } @@ -1037,7 +1170,21 @@ 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_with_offset(self, db: &'db dyn Db, other: Self) -> Self { + // 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(self, db: &'db dyn Db, other: Self) -> Self { + self.iff_inner(db, other, 0) + } + + 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 @@ -1048,20 +1195,18 @@ impl<'db> Node<'db> { (Node::AlwaysTrue | Node::AlwaysFalse, Node::Interior(interior)) => Node::new( db, interior.constraint(db), - self.iff(db, interior.if_true(db)), - self.iff(db, interior.if_false(db)), + 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(db, other), - interior.if_false(db).iff(db, other), + 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)) => { - // IFF is commutative, which lets us halve the cache requirements - let (a, b) = if b.0 < a.0 { (b, a) } else { (a, b) }; - a.iff(db, b) - } + (Node::Interior(a), Node::Interior(b)) => a.iff(db, b, other_offset), } } @@ -1112,7 +1257,7 @@ impl<'db> Node<'db> { } let mut typevars = FxHashSet::default(); - self.for_each_constraint(db, &mut |constraint| { + self.for_each_constraint(db, &mut |constraint, _| { typevars.insert(constraint.typevar(db)); }); @@ -1229,36 +1374,47 @@ impl<'db> Node<'db> { self, db: &'db dyn Db, bound_typevar: BoundTypeVarIdentity<'db>, - mut f: impl FnMut(Option<(Type<'db>, Type<'db>)>), + mut f: impl FnMut(Option<&[RepresentativeBounds<'db>]>), ) { self.retain_one(db, bound_typevar) - .find_representative_types_inner(db, None, &mut f); + .find_representative_types_inner(db, &mut Vec::default(), &mut f); } fn find_representative_types_inner( self, db: &'db dyn Db, - current_bounds: Option<(Type<'db>, Type<'db>)>, - f: &mut dyn FnMut(Option<(Type<'db>, Type<'db>)>), + current_bounds: &mut Vec>, + f: &mut dyn FnMut(Option<&[RepresentativeBounds<'db>]>), ) { match self { Node::AlwaysTrue => { // If we reach the `true` terminal, the path we've been following represents one // representative type. + if current_bounds.is_empty() { + f(None); + return; + } // If `lower โ‰ฐ upper`, then this path somehow represents in invalid specialization. // That should have been removed from the BDD domain as part of the simplification - // process. - debug_assert!(current_bounds.is_none_or( - |(greatest_lower_bound, least_upper_bound)| { - greatest_lower_bound.is_assignable_to(db, least_upper_bound) - } - )); + // process. (Here we are just checking assignability, so we don't need to construct + // the lower and upper bounds in a consistent order.) + debug_assert!({ + let greatest_lower_bound = UnionType::from_elements( + db, + current_bounds.iter().map(|bounds| bounds.lower), + ); + let least_upper_bound = IntersectionType::from_elements( + db, + current_bounds.iter().map(|bounds| bounds.upper), + ); + greatest_lower_bound.is_assignable_to(db, least_upper_bound) + }); // We've been tracking the lower and upper bound that the types for this path must // satisfy. Pass those bounds along and let the caller choose a representative type // from within that range. - f(current_bounds); + f(Some(current_bounds)); } Node::AlwaysFalse => { @@ -1267,8 +1423,7 @@ impl<'db> Node<'db> { } Node::Interior(interior) => { - let (greatest_lower_bound, least_upper_bound) = - current_bounds.unwrap_or((Type::Never, Type::object())); + let reset_point = current_bounds.len(); // For an interior node, there are two outgoing paths: one for the `if_true` // branch, and one for the `if_false` branch. @@ -1277,16 +1432,11 @@ impl<'db> Node<'db> { // on the types that satisfy the current path through the BDD. So we intersect the // current glb/lub with the constraint's bounds to get the new glb/lub for the // recursive call. - let constraint = interior.constraint(db); - let new_greatest_lower_bound = - UnionType::from_elements(db, [greatest_lower_bound, constraint.lower(db)]); - let new_least_upper_bound = - IntersectionType::from_elements(db, [least_upper_bound, constraint.upper(db)]); - interior.if_true(db).find_representative_types_inner( - db, - Some((new_greatest_lower_bound, new_least_upper_bound)), - f, - ); + current_bounds.push(RepresentativeBounds::from_interior_node(db, interior)); + interior + .if_true(db) + .find_representative_types_inner(db, current_bounds, f); + current_bounds.truncate(reset_point); // For the `if_false` branch, then the types that satisfy the current path through // the BDD do _not_ satisfy the node's constraint. Because we used `retain_one` to @@ -1298,11 +1448,9 @@ impl<'db> Node<'db> { // without updating the lower/upper bounds, relying on the other constraints along // the path to incorporate that negative "hole" in the set of valid types for this // path. - interior.if_false(db).find_representative_types_inner( - db, - Some((greatest_lower_bound, least_upper_bound)), - f, - ); + interior + .if_false(db) + .find_representative_types_inner(db, current_bounds, f); } } } @@ -1343,7 +1491,9 @@ impl<'db> Node<'db> { self, db: &'db dyn Db, left: ConstraintAssignment<'db>, + left_source_order: usize, right: ConstraintAssignment<'db>, + right_source_order: usize, replacement: Node<'db>, ) -> Self { // We perform a Shannon expansion to find out what the input BDD evaluates to when: @@ -1377,8 +1527,8 @@ impl<'db> Node<'db> { // false // // (Note that the `else` branch shouldn't be reachable, but we have to provide something!) - let left_node = Node::new_satisfied_constraint(db, left); - let right_node = Node::new_satisfied_constraint(db, right); + let left_node = Node::new_satisfied_constraint(db, left, left_source_order); + let right_node = Node::new_satisfied_constraint(db, right, right_source_order); let right_result = right_node.ite(db, Node::AlwaysFalse, when_left_but_not_right); let left_result = left_node.ite(db, right_result, when_not_left); let result = replacement.ite(db, when_left_and_right, left_result); @@ -1401,7 +1551,9 @@ impl<'db> Node<'db> { self, db: &'db dyn Db, left: ConstraintAssignment<'db>, + left_source_order: usize, right: ConstraintAssignment<'db>, + right_source_order: usize, replacement: Node<'db>, ) -> Self { // We perform a Shannon expansion to find out what the input BDD evaluates to when: @@ -1441,8 +1593,8 @@ impl<'db> Node<'db> { // Lastly, verify that the result is consistent with the input. (It must produce the same // results when `left โˆจ right`.) If it doesn't, the substitution isn't valid, and we should // return the original BDD unmodified. - let left_node = Node::new_satisfied_constraint(db, left); - let right_node = Node::new_satisfied_constraint(db, right); + let left_node = Node::new_satisfied_constraint(db, left, left_source_order); + let right_node = Node::new_satisfied_constraint(db, right, right_source_order); let validity = replacement.iff(db, left_node.or(db, right_node)); let constrained_original = self.and(db, validity); let constrained_replacement = result.and(db, validity); @@ -1457,11 +1609,15 @@ impl<'db> Node<'db> { /// constraint can appear multiple times in different paths from the root; we do not /// deduplicate those constraints, and will instead invoke the callback each time we encounter /// the constraint.) - fn for_each_constraint(self, db: &'db dyn Db, f: &mut dyn FnMut(ConstrainedTypeVar<'db>)) { + fn for_each_constraint( + self, + db: &'db dyn Db, + f: &mut dyn FnMut(ConstrainedTypeVar<'db>, usize), + ) { let Node::Interior(interior) = self else { return; }; - f(interior.constraint(db)); + f(interior.constraint(db), interior.source_order(db)); interior.if_true(db).for_each_constraint(db, f); interior.if_false(db).for_each_constraint(db, f); } @@ -1593,7 +1749,13 @@ impl<'db> Node<'db> { Node::AlwaysTrue => write!(f, "always"), Node::AlwaysFalse => write!(f, "never"), Node::Interior(interior) => { - interior.constraint(self.db).display(self.db).fmt(f)?; + write!( + f, + "{} {}/{}", + interior.constraint(self.db).display(self.db), + interior.source_order(self.db), + interior.max_source_order(self.db), + )?; // Calling display_graph recursively here causes rustc to claim that the // expect(unused) up above is unfulfilled! write!( @@ -1626,12 +1788,43 @@ impl<'db> Node<'db> { } } +#[derive(Clone, Copy, Debug)] +struct RepresentativeBounds<'db> { + lower: Type<'db>, + upper: Type<'db>, + source_order: usize, +} + +impl<'db> RepresentativeBounds<'db> { + fn from_interior_node(db: &'db dyn Db, interior: InteriorNode<'db>) -> Self { + let constraint = interior.constraint(db); + let lower = constraint.lower(db); + let upper = constraint.upper(db); + let source_order = interior.source_order(db); + Self { + lower, + upper, + source_order, + } + } +} + /// An interior node of a BDD #[salsa::interned(debug, heap_size=ruff_memory_usage::heap_size)] 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, } // The Salsa heap is tracked separately. @@ -1646,83 +1839,105 @@ impl<'db> InteriorNode<'db> { self.constraint(db), self.if_true(db).negate(db), self.if_false(db).negate(db), + self.source_order(db), ) } #[salsa::tracked(heap_size=ruff_memory_usage::heap_size)] - fn or(self, db: &'db dyn Db, other: Self) -> 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(db, other.if_true(db)), - self.if_false(db).or(db, other.if_false(db)), + self.if_true(db) + .or_inner(db, other.if_true(db), other_offset), + self.if_false(db) + .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(db, Node::Interior(other)), - self.if_false(db).or(db, Node::Interior(other)), + self.if_true(db) + .or_inner(db, Node::Interior(other), other_offset), + self.if_false(db) + .or_inner(db, Node::Interior(other), other_offset), + self.source_order(db), ), Ordering::Greater => Node::new( db, other_constraint, - Node::Interior(self).or(db, other.if_true(db)), - Node::Interior(self).or(db, other.if_false(db)), + 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) -> 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)) { Ordering::Equal => Node::new( db, self_constraint, - self.if_true(db).and(db, other.if_true(db)), - self.if_false(db).and(db, other.if_false(db)), + self.if_true(db) + .and_inner(db, other.if_true(db), other_offset), + self.if_false(db) + .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(db, Node::Interior(other)), - self.if_false(db).and(db, Node::Interior(other)), + self.if_true(db) + .and_inner(db, Node::Interior(other), other_offset), + self.if_false(db) + .and_inner(db, Node::Interior(other), other_offset), + self.source_order(db), ), Ordering::Greater => Node::new( db, other_constraint, - Node::Interior(self).and(db, other.if_true(db)), - Node::Interior(self).and(db, other.if_false(db)), + 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) -> 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)) { Ordering::Equal => Node::new( db, self_constraint, - self.if_true(db).iff(db, other.if_true(db)), - self.if_false(db).iff(db, other.if_false(db)), + self.if_true(db) + .iff_inner(db, other.if_true(db), other_offset), + self.if_false(db) + .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(db, Node::Interior(other)), - self.if_false(db).iff(db, Node::Interior(other)), + self.if_true(db) + .iff_inner(db, Node::Interior(other), other_offset), + self.if_false(db) + .iff_inner(db, Node::Interior(other), other_offset), + self.source_order(db), ), Ordering::Greater => Node::new( db, other_constraint, - Node::Interior(self).iff(db, other.if_true(db)), - Node::Interior(self).iff(db, other.if_false(db)), + 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, ), } } @@ -1797,7 +2012,13 @@ impl<'db> InteriorNode<'db> { // // We also have to check if there are any derived facts that depend on the constraint // we're about to remove. If so, we need to "remember" them by AND-ing them in with the - // corresponding branch. + // corresponding branch. We currently reuse the `source_order` of the constraint being + // removed when we add these derived facts. + // + // TODO: This might not be stable enough, if we add more than one derived fact for this + // constraint. If we still see inconsistent test output, we might need a more complex + // way of tracking source order for derived facts. + let self_source_order = self.source_order(db); let if_true = path .walk_edge(db, map, self_constraint.when_true(), |path, new_range| { let branch = self @@ -1811,7 +2032,10 @@ impl<'db> InteriorNode<'db> { !should_remove(assignment.constraint()) }) .fold(branch, |branch, assignment| { - branch.and(db, Node::new_satisfied_constraint(db, *assignment)) + branch.and( + db, + Node::new_satisfied_constraint(db, *assignment, self_source_order), + ) }) }) .unwrap_or(Node::AlwaysFalse); @@ -1828,7 +2052,10 @@ impl<'db> InteriorNode<'db> { !should_remove(assignment.constraint()) }) .fold(branch, |branch, assignment| { - branch.and(db, Node::new_satisfied_constraint(db, *assignment)) + branch.and( + db, + Node::new_satisfied_constraint(db, *assignment, self_source_order), + ) }) }) .unwrap_or(Node::AlwaysFalse); @@ -1850,7 +2077,8 @@ impl<'db> InteriorNode<'db> { // NB: We cannot use `Node::new` here, because the recursive calls might introduce new // derived constraints into the result, and those constraints might appear before this // one in the BDD ordering. - Node::new_constraint(db, self_constraint).ite(db, if_true, if_false) + Node::new_constraint(db, self_constraint, self.source_order(db)) + .ite(db, if_true, if_false) } } @@ -1878,7 +2106,13 @@ impl<'db> InteriorNode<'db> { let (if_true, found_in_true) = self.if_true(db).restrict_one(db, assignment); let (if_false, found_in_false) = self.if_false(db).restrict_one(db, assignment); ( - Node::new(db, self_constraint, if_true, if_false), + Node::new( + db, + self_constraint, + if_true, + if_false, + self.source_order(db), + ), found_in_true || found_in_false, ) } @@ -1898,7 +2132,7 @@ impl<'db> InteriorNode<'db> { "create sequent map", ); let mut map = SequentMap::default(); - Node::Interior(self).for_each_constraint(db, &mut |constraint| { + Node::Interior(self).for_each_constraint(db, &mut |constraint, _| { map.add(db, constraint); }); map @@ -1927,17 +2161,26 @@ impl<'db> InteriorNode<'db> { // visit queue with all pairs of those constraints. (We use "combinations" because we don't // need to compare a constraint against itself, and because ordering doesn't matter.) let mut seen_constraints = FxHashSet::default(); - Node::Interior(self).for_each_constraint(db, &mut |constraint| { + let mut source_orders = FxHashMap::default(); + Node::Interior(self).for_each_constraint(db, &mut |constraint, source_order| { seen_constraints.insert(constraint); + source_orders.insert(constraint, source_order); }); let mut to_visit: Vec<(_, _)> = (seen_constraints.iter().copied()) .tuple_combinations() .collect(); // Repeatedly pop constraint pairs off of the visit queue, checking whether each pair can - // be simplified. + // be simplified. If we add any derived constraints, we will place them at the end in + // source order. (We do not have any test cases that depend on constraint sets being + // displayed in a consistent ordering, so we don't need to be clever in assigning these + // `source_order`s.) let mut simplified = Node::Interior(self); + let mut next_source_order = self.max_source_order(db) + 1; while let Some((left_constraint, right_constraint)) = to_visit.pop() { + let left_source_order = source_orders[&left_constraint]; + let right_source_order = source_orders[&right_constraint]; + // If the constraints refer to different typevars, the only simplifications we can make // are of the form `S โ‰ค T โˆง T โ‰ค int โ†’ S โ‰ค int`. let left_typevar = left_constraint.typevar(db); @@ -2003,11 +2246,18 @@ impl<'db> InteriorNode<'db> { if seen_constraints.contains(&new_constraint) { continue; } - let new_node = Node::new_constraint(db, new_constraint); - let positive_left_node = - Node::new_satisfied_constraint(db, left_constraint.when_true()); - let positive_right_node = - Node::new_satisfied_constraint(db, right_constraint.when_true()); + let new_node = Node::new_constraint(db, new_constraint, next_source_order); + next_source_order += 1; + let positive_left_node = Node::new_satisfied_constraint( + db, + left_constraint.when_true(), + left_source_order, + ); + let positive_right_node = Node::new_satisfied_constraint( + db, + right_constraint.when_true(), + right_source_order, + ); let lhs = positive_left_node.and(db, positive_right_node); let intersection = new_node.ite(db, lhs, Node::AlwaysFalse); simplified = simplified.and(db, intersection); @@ -2030,23 +2280,47 @@ impl<'db> InteriorNode<'db> { // Containment: The range of one constraint might completely contain the range of the // other. If so, there are several potential simplifications. let larger_smaller = if left_constraint.implies(db, right_constraint) { - Some((right_constraint, left_constraint)) + Some(( + right_constraint, + right_source_order, + left_constraint, + left_source_order, + )) } else if right_constraint.implies(db, left_constraint) { - Some((left_constraint, right_constraint)) + Some(( + left_constraint, + left_source_order, + right_constraint, + right_source_order, + )) } else { None }; - if let Some((larger_constraint, smaller_constraint)) = larger_smaller { - let positive_larger_node = - Node::new_satisfied_constraint(db, larger_constraint.when_true()); - let negative_larger_node = - Node::new_satisfied_constraint(db, larger_constraint.when_false()); + if let Some(( + larger_constraint, + larger_source_order, + smaller_constraint, + smaller_source_order, + )) = larger_smaller + { + let positive_larger_node = Node::new_satisfied_constraint( + db, + larger_constraint.when_true(), + larger_source_order, + ); + let negative_larger_node = Node::new_satisfied_constraint( + db, + larger_constraint.when_false(), + larger_source_order, + ); // larger โˆจ smaller = larger simplified = simplified.substitute_union( db, larger_constraint.when_true(), + larger_source_order, smaller_constraint.when_true(), + smaller_source_order, positive_larger_node, ); @@ -2054,7 +2328,9 @@ impl<'db> InteriorNode<'db> { simplified = simplified.substitute_intersection( db, larger_constraint.when_false(), + larger_source_order, smaller_constraint.when_false(), + smaller_source_order, negative_larger_node, ); @@ -2063,7 +2339,9 @@ impl<'db> InteriorNode<'db> { simplified = simplified.substitute_intersection( db, larger_constraint.when_false(), + larger_source_order, smaller_constraint.when_true(), + smaller_source_order, Node::AlwaysFalse, ); @@ -2072,7 +2350,9 @@ impl<'db> InteriorNode<'db> { simplified = simplified.substitute_union( db, larger_constraint.when_true(), + larger_source_order, smaller_constraint.when_false(), + smaller_source_order, Node::AlwaysTrue, ); } @@ -2088,32 +2368,54 @@ impl<'db> InteriorNode<'db> { // represent that intersection. We also need to add the new constraint to our // seen set and (if we haven't already seen it) to the to-visit queue. if seen_constraints.insert(intersection_constraint) { + source_orders.insert(intersection_constraint, next_source_order); to_visit.extend( (seen_constraints.iter().copied()) .filter(|seen| *seen != intersection_constraint) .map(|seen| (seen, intersection_constraint)), ); } - let positive_intersection_node = - Node::new_satisfied_constraint(db, intersection_constraint.when_true()); - let negative_intersection_node = - Node::new_satisfied_constraint(db, intersection_constraint.when_false()); + let positive_intersection_node = Node::new_satisfied_constraint( + db, + intersection_constraint.when_true(), + next_source_order, + ); + let negative_intersection_node = Node::new_satisfied_constraint( + db, + intersection_constraint.when_false(), + next_source_order, + ); + next_source_order += 1; - let positive_left_node = - Node::new_satisfied_constraint(db, left_constraint.when_true()); - let negative_left_node = - Node::new_satisfied_constraint(db, left_constraint.when_false()); + let positive_left_node = Node::new_satisfied_constraint( + db, + left_constraint.when_true(), + left_source_order, + ); + let negative_left_node = Node::new_satisfied_constraint( + db, + left_constraint.when_false(), + left_source_order, + ); - let positive_right_node = - Node::new_satisfied_constraint(db, right_constraint.when_true()); - let negative_right_node = - Node::new_satisfied_constraint(db, right_constraint.when_false()); + let positive_right_node = Node::new_satisfied_constraint( + db, + right_constraint.when_true(), + right_source_order, + ); + let negative_right_node = Node::new_satisfied_constraint( + db, + right_constraint.when_false(), + right_source_order, + ); // left โˆง right = intersection simplified = simplified.substitute_intersection( db, left_constraint.when_true(), + left_source_order, right_constraint.when_true(), + right_source_order, positive_intersection_node, ); @@ -2121,7 +2423,9 @@ impl<'db> InteriorNode<'db> { simplified = simplified.substitute_union( db, left_constraint.when_false(), + left_source_order, right_constraint.when_false(), + right_source_order, negative_intersection_node, ); @@ -2131,7 +2435,9 @@ impl<'db> InteriorNode<'db> { simplified = simplified.substitute_intersection( db, left_constraint.when_true(), + left_source_order, right_constraint.when_false(), + right_source_order, positive_left_node.and(db, negative_intersection_node), ); @@ -2140,7 +2446,9 @@ impl<'db> InteriorNode<'db> { simplified = simplified.substitute_intersection( db, left_constraint.when_false(), + left_source_order, right_constraint.when_true(), + right_source_order, positive_right_node.and(db, negative_intersection_node), ); @@ -2150,7 +2458,9 @@ impl<'db> InteriorNode<'db> { simplified = simplified.substitute_union( db, left_constraint.when_true(), + left_source_order, right_constraint.when_false(), + right_source_order, negative_right_node.or(db, positive_intersection_node), ); @@ -2159,7 +2469,9 @@ impl<'db> InteriorNode<'db> { simplified = simplified.substitute_union( db, left_constraint.when_false(), + left_source_order, right_constraint.when_true(), + right_source_order, negative_left_node.or(db, positive_intersection_node), ); } @@ -2172,16 +2484,24 @@ impl<'db> InteriorNode<'db> { // All of the below hold because we just proved that the intersection of left // and right is empty. - let positive_left_node = - Node::new_satisfied_constraint(db, left_constraint.when_true()); - let positive_right_node = - Node::new_satisfied_constraint(db, right_constraint.when_true()); + let positive_left_node = Node::new_satisfied_constraint( + db, + left_constraint.when_true(), + left_source_order, + ); + let positive_right_node = Node::new_satisfied_constraint( + db, + right_constraint.when_true(), + right_source_order, + ); // left โˆง right = false simplified = simplified.substitute_intersection( db, left_constraint.when_true(), + left_source_order, right_constraint.when_true(), + right_source_order, Node::AlwaysFalse, ); @@ -2189,7 +2509,9 @@ impl<'db> InteriorNode<'db> { simplified = simplified.substitute_union( db, left_constraint.when_false(), + left_source_order, right_constraint.when_false(), + right_source_order, Node::AlwaysTrue, ); @@ -2198,7 +2520,9 @@ impl<'db> InteriorNode<'db> { simplified = simplified.substitute_intersection( db, left_constraint.when_true(), + left_source_order, right_constraint.when_false(), + right_source_order, positive_left_node, ); @@ -2207,7 +2531,9 @@ impl<'db> InteriorNode<'db> { simplified = simplified.substitute_intersection( db, left_constraint.when_false(), + left_source_order, right_constraint.when_true(), + right_source_order, positive_right_node, ); } @@ -3266,7 +3592,7 @@ impl<'db> BoundTypeVarInstance<'db> { for constraint in constraints.elements(db) { let constraint_lower = constraint.bottom_materialization(db); let constraint_upper = constraint.top_materialization(db); - specializations = specializations.or( + specializations = specializations.or_with_offset( db, ConstrainedTypeVar::new_node(db, self, constraint_lower, constraint_upper), ); @@ -3316,7 +3642,8 @@ impl<'db> BoundTypeVarInstance<'db> { let constraint = ConstrainedTypeVar::new_node(db, self, constraint_lower, constraint_upper); if constraint_lower == constraint_upper { - non_gradual_constraints = non_gradual_constraints.or(db, constraint); + non_gradual_constraints = + non_gradual_constraints.or_with_offset(db, constraint); } else { gradual_constraints.push(constraint); } @@ -3356,9 +3683,10 @@ impl<'db> GenericContext<'db> { // each typevar. let abstracted = self .variables(db) - .fold(constraints.node, |constraints, bound_typevar| { - constraints.and(db, bound_typevar.valid_specializations(db)) - }); + .fold(Node::AlwaysTrue, |constraints, bound_typevar| { + constraints.and_with_offset(db, bound_typevar.valid_specializations(db)) + }) + .and_with_offset(db, constraints.node); tracing::debug!( target: "ty_python_semantic::types::constraints::specialize_constrained", valid = %abstracted.display(db), @@ -3367,99 +3695,99 @@ impl<'db> GenericContext<'db> { // Then we find all of the "representative types" for each typevar in the constraint set. let mut error_occurred = false; - let types = self.variables(db).map(|bound_typevar| { - // Each representative type represents one of the ways that the typevar can satisfy the - // constraint, expressed as a lower/upper bound on the types that the typevar can - // specialize to. - // - // If there are multiple paths in the BDD, they technically represent independent - // possible specializations. If there's a type that satisfies all of them, we will - // return that as the specialization. If not, then the constraint set is ambiguous. - // (This happens most often with constrained typevars.) We could in the future turn - // _each_ of the paths into separate specializations, but it's not clear what we would - // do with that, so instead we just report the ambiguity as a specialization failure. - let mut satisfied = false; - let mut unconstrained = false; - let mut greatest_lower_bound = UnionBuilder::new(db).order_elements(true); - let mut least_upper_bound = IntersectionBuilder::new(db).order_elements(true); - let identity = bound_typevar.identity(db); - tracing::trace!( - target: "ty_python_semantic::types::constraints::specialize_constrained", - bound_typevar = %identity.display(db), - abstracted = %abstracted.retain_one(db, identity).display(db), - "find specialization for typevar", - ); - abstracted.find_representative_types(db, identity, |bounds| { - satisfied = true; - match bounds { - Some((lower_bound, upper_bound)) => { - tracing::trace!( - target: "ty_python_semantic::types::constraints::specialize_constrained", - bound_typevar = %identity.display(db), - lower_bound = %lower_bound.display(db), - upper_bound = %upper_bound.display(db), - "found representative type", - ); - greatest_lower_bound.add_in_place(lower_bound); - least_upper_bound.add_positive_in_place(upper_bound); - } - None => { - unconstrained = true; + let mut representatives = Vec::new(); + let types = + self.variables(db).map(|bound_typevar| { + // Each representative type represents one of the ways that the typevar can satisfy the + // constraint, expressed as a lower/upper bound on the types that the typevar can + // specialize to. + // + // If there are multiple paths in the BDD, they technically represent independent + // possible specializations. If there's a type that satisfies all of them, we will + // return that as the specialization. If not, then the constraint set is ambiguous. + // (This happens most often with constrained typevars.) We could in the future turn + // _each_ of the paths into separate specializations, but it's not clear what we would + // do with that, so instead we just report the ambiguity as a specialization failure. + let mut unconstrained = false; + let identity = bound_typevar.identity(db); + tracing::trace!( + target: "ty_python_semantic::types::constraints::specialize_constrained", + bound_typevar = %identity.display(db), + abstracted = %abstracted.retain_one(db, identity).display(db), + "find specialization for typevar", + ); + representatives.clear(); + abstracted.find_representative_types(db, identity, |representative| { + match representative { + Some(representative) => { + representatives.extend_from_slice(representative); + } + None => { + unconstrained = true; + } } + }); + + // The BDD is satisfiable, but the typevar is unconstrained, then we use `None` to tell + // specialize_recursive to fall back on the typevar's default. + if unconstrained { + tracing::debug!( + target: "ty_python_semantic::types::constraints::specialize_constrained", + bound_typevar = %identity.display(db), + "typevar is unconstrained", + ); + return None; } + + // If there are no satisfiable paths in the BDD, then there is no valid specialization + // for this constraint set. + if representatives.is_empty() { + // TODO: Construct a useful error here + tracing::debug!( + target: "ty_python_semantic::types::constraints::specialize_constrained", + bound_typevar = %identity.display(db), + "typevar cannot be satisfied", + ); + error_occurred = true; + return None; + } + + // Before constructing the final lower and upper bound, sort the constraints by + // their source order. This should give us a consistently ordered specialization, + // regardless of the variable ordering of the original BDD. + representatives.sort_unstable_by_key(|bounds| bounds.source_order); + let greatest_lower_bound = + UnionType::from_elements(db, representatives.iter().map(|bounds| bounds.lower)); + let least_upper_bound = IntersectionType::from_elements( + db, + representatives.iter().map(|bounds| bounds.upper), + ); + + // If `lower โ‰ฐ upper`, then there is no type that satisfies all of the paths in the + // BDD. That's an ambiguous specialization, as described above. + if !greatest_lower_bound.is_assignable_to(db, least_upper_bound) { + tracing::debug!( + target: "ty_python_semantic::types::constraints::specialize_constrained", + bound_typevar = %identity.display(db), + greatest_lower_bound = %greatest_lower_bound.display(db), + least_upper_bound = %least_upper_bound.display(db), + "typevar bounds are incompatible", + ); + error_occurred = true; + return None; + } + + // Of all of the types that satisfy all of the paths in the BDD, we choose the + // "largest" one (i.e., "closest to `object`") as the specialization. + tracing::debug!( + target: "ty_python_semantic::types::constraints::specialize_constrained", + bound_typevar = %identity.display(db), + specialization = %least_upper_bound.display(db), + "found specialization for typevar", + ); + Some(least_upper_bound) }); - // If there are no satisfiable paths in the BDD, then there is no valid specialization - // for this constraint set. - if !satisfied { - // TODO: Construct a useful error here - tracing::debug!( - target: "ty_python_semantic::types::constraints::specialize_constrained", - bound_typevar = %identity.display(db), - "typevar cannot be satisfied", - ); - error_occurred = true; - return None; - } - - // The BDD is satisfiable, but the typevar is unconstrained, then we use `None` to tell - // specialize_recursive to fall back on the typevar's default. - if unconstrained { - tracing::debug!( - target: "ty_python_semantic::types::constraints::specialize_constrained", - bound_typevar = %identity.display(db), - "typevar is unconstrained", - ); - return None; - } - - // If `lower โ‰ฐ upper`, then there is no type that satisfies all of the paths in the - // BDD. That's an ambiguous specialization, as described above. - let greatest_lower_bound = greatest_lower_bound.build(); - let least_upper_bound = least_upper_bound.build(); - if !greatest_lower_bound.is_assignable_to(db, least_upper_bound) { - tracing::debug!( - target: "ty_python_semantic::types::constraints::specialize_constrained", - bound_typevar = %identity.display(db), - greatest_lower_bound = %greatest_lower_bound.display(db), - least_upper_bound = %least_upper_bound.display(db), - "typevar bounds are incompatible", - ); - error_occurred = true; - return None; - } - - // Of all of the types that satisfy all of the paths in the BDD, we choose the - // "largest" one (i.e., "closest to `object`") as the specialization. - tracing::debug!( - target: "ty_python_semantic::types::constraints::specialize_constrained", - bound_typevar = %identity.display(db), - specialization = %least_upper_bound.display(db), - "found specialization for typevar", - ); - Some(least_upper_bound) - }); - let specialization = self.specialize_recursive(db, types); if error_occurred { return Err(()); @@ -3481,28 +3809,28 @@ mod tests { #[test] fn test_display_graph_output() { let expected = indoc! {r#" - (T = str) - โ”กโ”โ‚ (T = bool) - โ”‚ โ”กโ”โ‚ (U = str) - โ”‚ โ”‚ โ”กโ”โ‚ (U = bool) + (T = str) 3/4 + โ”กโ”โ‚ (T = bool) 4/4 + โ”‚ โ”กโ”โ‚ (U = str) 1/2 + โ”‚ โ”‚ โ”กโ”โ‚ (U = bool) 2/2 โ”‚ โ”‚ โ”‚ โ”กโ”โ‚ always โ”‚ โ”‚ โ”‚ โ””โ”€โ‚€ always - โ”‚ โ”‚ โ””โ”€โ‚€ (U = bool) + โ”‚ โ”‚ โ””โ”€โ‚€ (U = bool) 2/2 โ”‚ โ”‚ โ”กโ”โ‚ always โ”‚ โ”‚ โ””โ”€โ‚€ never - โ”‚ โ””โ”€โ‚€ (U = str) - โ”‚ โ”กโ”โ‚ (U = bool) + โ”‚ โ””โ”€โ‚€ (U = str) 1/2 + โ”‚ โ”กโ”โ‚ (U = bool) 2/2 โ”‚ โ”‚ โ”กโ”โ‚ always โ”‚ โ”‚ โ””โ”€โ‚€ always - โ”‚ โ””โ”€โ‚€ (U = bool) + โ”‚ โ””โ”€โ‚€ (U = bool) 2/2 โ”‚ โ”กโ”โ‚ always โ”‚ โ””โ”€โ‚€ never - โ””โ”€โ‚€ (T = bool) - โ”กโ”โ‚ (U = str) - โ”‚ โ”กโ”โ‚ (U = bool) + โ””โ”€โ‚€ (T = bool) 4/4 + โ”กโ”โ‚ (U = str) 1/2 + โ”‚ โ”กโ”โ‚ (U = bool) 2/2 โ”‚ โ”‚ โ”กโ”โ‚ always โ”‚ โ”‚ โ””โ”€โ‚€ always - โ”‚ โ””โ”€โ‚€ (U = bool) + โ”‚ โ””โ”€โ‚€ (U = bool) 2/2 โ”‚ โ”กโ”โ‚ always โ”‚ โ””โ”€โ‚€ never โ””โ”€โ‚€ never @@ -3518,7 +3846,9 @@ mod tests { let t_bool = ConstraintSet::range(&db, bool_type, t, bool_type); let u_str = ConstraintSet::range(&db, str_type, u, str_type); let u_bool = ConstraintSet::range(&db, bool_type, u, bool_type); - let constraints = (t_str.or(&db, || t_bool)).and(&db, || u_str.or(&db, || u_bool)); + // Construct this in a different order than above to make the source_orders more + // interesting. + let constraints = (u_str.or(&db, || u_bool)).and(&db, || t_str.or(&db, || t_bool)); let actual = constraints.node.display_graph(&db, &"").to_string(); assert_eq!(actual, expected); }