[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.)
This commit is contained in:
Douglas Creager
2025-12-18 19:00:20 -05:00
committed by GitHub
parent fa57253980
commit 5a2d3cda3d
2 changed files with 24 additions and 8 deletions

View File

@@ -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<T> {
@@ -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<ConstrainedTypeVar<'db>>,
FxOrderSet<ConstrainedTypeVar<'db>>,
>,
/// Sequents of the form `C → D`
single_implications: FxHashMap<ConstrainedTypeVar<'db>, FxHashSet<ConstrainedTypeVar<'db>>>,
single_implications: FxHashMap<ConstrainedTypeVar<'db>, FxOrderSet<ConstrainedTypeVar<'db>>>,
/// Constraints that we have already processed
processed: FxHashSet<ConstrainedTypeVar<'db>>,
/// Constraints that enqueued to be processed

View File

@@ -1590,14 +1590,19 @@ impl<'db> SpecializationBuilder<'db> {
upper: FxOrderSet<Type<'db>>,
}
// 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)