From 5a2d3cda3dd542f060cc0b09f3eff128753f4e0e Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Thu, 18 Dec 2025 19:00:20 -0500 Subject: [PATCH] [ty] Remove some nondeterminism in constraint set tests (#22064) We're seeing a lot of nondeterminism in the ecosystem tests at the moment, which started (or at least got worse) once `Callable` inference landed. This PR attempts to remove this nondeterminism. We recently (https://github.com/astral-sh/ruff/pull/21983) added a `source_order` field to BDD nodes, which tracks when their constraint was added to the BDD. Since we build up constraints based on the order that they appear in the underlying source, that gives us a stable ordering even though we use an arbitrary salsa-derived ordering for the BDD variables. The issue (at least for some of the flakiness) is that we add "derived" constraints when walking a BDD tree, and those derived constraints inherit or borrow the `source_order` of the "real" constraint that implied them. That means we can get multiple constraints in our specialization that all have the same `source_order`. If we're not careful, those "tied" constraints can be ordered arbitrarily. The fix requires ~three~ ~four~ several steps: - When starting to construct a sequent map (the data structure that stores the derived constraints), we first sort all of the "real" constraints by their `source_order`. That ensures that we insert things into the sequent map in a stable order. - During sequent map construction, derived facts are discovered by a deterministic process applied to constraints in a (now) stable order. So derived facts are now also inserted in a stable order. - We update the fields of `SequentMap` to use `FxOrderSet` instead of `FxHashSet`, so that we retain that stable insertion order. - When walking BDD paths when constructing a specialization, we were already sorting the constraints by their `source_order`. However, we were not considering that we might get derived constraints, and therefore constraints with "ties". Because of that, we need to make sure to use a _stable_ sort, that retains the insertion order for those ties. All together, this...should...fix the nondeterminism. (Unfortunately, I haven't been able to effectively test this, since I haven't been able to coerce local tests to flop into the other order that we sometimes see in CI.) --- .../src/types/constraints.rs | 23 ++++++++++++++----- .../ty_python_semantic/src/types/generics.rs | 9 ++++++-- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/crates/ty_python_semantic/src/types/constraints.rs b/crates/ty_python_semantic/src/types/constraints.rs index 77b96bd74b..bd78d94c9b 100644 --- a/crates/ty_python_semantic/src/types/constraints.rs +++ b/crates/ty_python_semantic/src/types/constraints.rs @@ -83,7 +83,7 @@ use crate::types::{ BoundTypeVarIdentity, BoundTypeVarInstance, IntersectionType, Type, TypeVarBoundOrConstraints, UnionType, walk_bound_type_var_type, }; -use crate::{Db, FxOrderMap}; +use crate::{Db, FxOrderMap, FxOrderSet}; /// An extension trait for building constraint sets from [`Option`] values. pub(crate) trait OptionConstraintsExtension { @@ -2222,10 +2222,21 @@ impl<'db> InteriorNode<'db> { constraints = %Node::Interior(self).display(db), "create sequent map", ); - let mut map = SequentMap::default(); - Node::Interior(self).for_each_constraint(db, &mut |constraint, _| { - map.add(db, constraint); + + // Sort the constraints in this BDD by their `source_order`s before adding them to the + // sequent map. This ensures that constraints appear in the sequent map in a stable order. + // The constraints mentioned in a BDD should all have distinct `source_order`s, so an + // unstable sort is fine. + let mut constraints = Vec::new(); + Node::Interior(self).for_each_constraint(db, &mut |constraint, source_order| { + constraints.push((constraint, source_order)); }); + constraints.sort_unstable_by_key(|(_, source_order)| *source_order); + + let mut map = SequentMap::default(); + for (constraint, _) in constraints { + map.add(db, constraint); + } map } @@ -2781,10 +2792,10 @@ struct SequentMap<'db> { /// Sequents of the form `C₁ ∧ C₂ → D` pair_implications: FxHashMap< (ConstrainedTypeVar<'db>, ConstrainedTypeVar<'db>), - FxHashSet>, + FxOrderSet>, >, /// Sequents of the form `C → D` - single_implications: FxHashMap, FxHashSet>>, + single_implications: FxHashMap, FxOrderSet>>, /// Constraints that we have already processed processed: FxHashSet>, /// Constraints that enqueued to be processed diff --git a/crates/ty_python_semantic/src/types/generics.rs b/crates/ty_python_semantic/src/types/generics.rs index ca6a700bd2..36e89553a0 100644 --- a/crates/ty_python_semantic/src/types/generics.rs +++ b/crates/ty_python_semantic/src/types/generics.rs @@ -1590,14 +1590,19 @@ impl<'db> SpecializationBuilder<'db> { upper: FxOrderSet>, } + // Sort the constraints in each path by their `source_order`s, to ensure that we construct + // any unions or intersections in our type mappings in a stable order. Constraints might + // come out of `PathAssignment`s with identical `source_order`s, but if they do, those + // "tied" constraints will still be ordered in a stable way. So we need a stable sort to + // retain that stable per-tie ordering. let constraints = constraints.limit_to_valid_specializations(self.db); let mut sorted_paths = Vec::new(); constraints.for_each_path(self.db, |path| { let mut path: Vec<_> = path.positive_constraints().collect(); - path.sort_unstable_by_key(|(_, source_order)| *source_order); + path.sort_by_key(|(_, source_order)| *source_order); sorted_paths.push(path); }); - sorted_paths.sort_unstable_by(|path1, path2| { + sorted_paths.sort_by(|path1, path2| { let source_orders1 = path1.iter().map(|(_, source_order)| *source_order); let source_orders2 = path2.iter().map(|(_, source_order)| *source_order); source_orders1.cmp(source_orders2)