From a422716267709c63ad7075b47bfc97eb0cd10114 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Wed, 1 Oct 2025 03:14:35 -0400 Subject: [PATCH] [ty] Fix flaky constraint set rendering (#20653) This doesn't seem to be flaky in the sense of tests failing non-deterministically, but they are flaky in the sense of unrelated changes causing testing failures from the clauses of a constraint set being rendered in different orders. This flakiness is because we're using Salsa IDs to determine the order in which typevars appear in a constraint set BDD, and those IDs are assigned non-deterministically. The fix is ham-fisted but effective: sort the constraints in each clause, and the clauses in each set, as part of the rendering process. Constraint sets are only rendered in our test cases, so we don't need to over-optimize this. --- .../mdtest/type_properties/constraints.md | 12 +-- .../src/types/constraints.rs | 88 +++++++++---------- 2 files changed, 46 insertions(+), 54 deletions(-) diff --git a/crates/ty_python_semantic/resources/mdtest/type_properties/constraints.md b/crates/ty_python_semantic/resources/mdtest/type_properties/constraints.md index fb3254f87a..5791ba00de 100644 --- a/crates/ty_python_semantic/resources/mdtest/type_properties/constraints.md +++ b/crates/ty_python_semantic/resources/mdtest/type_properties/constraints.md @@ -305,9 +305,9 @@ range. ```py def _[T]() -> None: - # revealed: ty_extensions.ConstraintSet[(¬(Sub ≤ T@_ ≤ Base) ∧ (SubSub ≤ T@_ ≤ Base))] + # revealed: ty_extensions.ConstraintSet[((SubSub ≤ T@_ ≤ Base) ∧ ¬(Sub ≤ T@_ ≤ Base))] reveal_type(range_constraint(SubSub, T, Base) & negated_range_constraint(Sub, T, Super)) - # revealed: ty_extensions.ConstraintSet[(¬(Sub ≤ T@_ ≤ Base) ∧ (SubSub ≤ T@_ ≤ Super))] + # revealed: ty_extensions.ConstraintSet[((SubSub ≤ T@_ ≤ Super) ∧ ¬(Sub ≤ T@_ ≤ Base))] reveal_type(range_constraint(SubSub, T, Super) & negated_range_constraint(Sub, T, Base)) ``` @@ -360,7 +360,7 @@ that type _is_ in `SubSub ≤ T ≤ Super`, it is not correct to simplify the un ```py def _[T]() -> None: - # revealed: ty_extensions.ConstraintSet[(¬(SubSub ≤ T@_ ≤ Base) ∧ ¬(Sub ≤ T@_ ≤ Super))] + # revealed: ty_extensions.ConstraintSet[(¬(Sub ≤ T@_ ≤ Super) ∧ ¬(SubSub ≤ T@_ ≤ Base))] reveal_type(negated_range_constraint(SubSub, T, Base) & negated_range_constraint(Sub, T, Super)) ``` @@ -385,7 +385,7 @@ We cannot simplify the union of constraints that refer to different typevars. def _[T, U]() -> None: # revealed: ty_extensions.ConstraintSet[(Sub ≤ T@_ ≤ Base) ∨ (Sub ≤ U@_ ≤ Base)] reveal_type(range_constraint(Sub, T, Base) | range_constraint(Sub, U, Base)) - # revealed: ty_extensions.ConstraintSet[¬(Sub ≤ U@_ ≤ Base) ∨ ¬(Sub ≤ T@_ ≤ Base)] + # revealed: ty_extensions.ConstraintSet[¬(Sub ≤ T@_ ≤ Base) ∨ ¬(Sub ≤ U@_ ≤ Base)] reveal_type(negated_range_constraint(Sub, T, Base) | negated_range_constraint(Sub, U, Base)) ``` @@ -437,7 +437,7 @@ not include `Sub`. That means it should not be in the union. Since that type _is ```py def _[T]() -> None: - # revealed: ty_extensions.ConstraintSet[(SubSub ≤ T@_ ≤ Base) ∨ (Sub ≤ T@_ ≤ Super)] + # revealed: ty_extensions.ConstraintSet[(Sub ≤ T@_ ≤ Super) ∨ (SubSub ≤ T@_ ≤ Base)] reveal_type(range_constraint(SubSub, T, Base) | range_constraint(Sub, T, Super)) ``` @@ -575,7 +575,7 @@ class Base: ... class Unrelated: ... def _[T, U]() -> None: - # revealed: ty_extensions.ConstraintSet[¬(U@_ ≤ Base) ∨ ¬(T@_ ≤ Base)] + # revealed: ty_extensions.ConstraintSet[¬(T@_ ≤ Base) ∨ ¬(U@_ ≤ Base)] reveal_type(~(range_constraint(Never, T, Base) & range_constraint(Never, U, Base))) ``` diff --git a/crates/ty_python_semantic/src/types/constraints.rs b/crates/ty_python_semantic/src/types/constraints.rs index cb4e504196..1db37c3e56 100644 --- a/crates/ty_python_semantic/src/types/constraints.rs +++ b/crates/ty_python_semantic/src/types/constraints.rs @@ -1209,38 +1209,37 @@ impl<'db> SatisfiedClause<'db> { false } - fn display(&self, db: &'db dyn Db) -> impl Display { - struct DisplaySatisfiedClause<'a, 'db> { - clause: &'a SatisfiedClause<'db>, - db: &'db dyn Db, - } + fn display(&self, db: &'db dyn Db) -> String { + // This is a bit heavy-handed, but we need to output the constraints in a consistent order + // even though Salsa IDs are assigned non-deterministically. This Display output is only + // used in test cases, so we don't need to over-optimize it. - impl Display for DisplaySatisfiedClause<'_, '_> { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - if self.clause.constraints.len() > 1 { - f.write_str("(")?; + let mut constraints: Vec<_> = self + .constraints + .iter() + .map(|constraint| match constraint { + ConstraintAssignment::Positive(constraint) => constraint.display(db).to_string(), + ConstraintAssignment::Negative(constraint) => { + constraint.display_negated(db).to_string() } - for (i, constraint) in self.clause.constraints.iter().enumerate() { - if i > 0 { - f.write_str(" ∧ ")?; - } - match constraint { - ConstraintAssignment::Positive(constraint) => { - write!(f, "{}", constraint.display(self.db))?; - } - ConstraintAssignment::Negative(constraint) => { - write!(f, "{}", constraint.display_negated(self.db))?; - } - } - } - if self.clause.constraints.len() > 1 { - f.write_str(")")?; - } - Ok(()) + }) + .collect(); + constraints.sort(); + + let mut result = String::new(); + if constraints.len() > 1 { + result.push('('); + } + for (i, constraint) in constraints.iter().enumerate() { + if i > 0 { + result.push_str(" ∧ "); } + result.push_str(constraint); } - - DisplaySatisfiedClause { clause: self, db } + if constraints.len() > 1 { + result.push(')'); + } + result } } @@ -1324,27 +1323,20 @@ impl<'db> SatisfiedClauses<'db> { false } - fn display(&self, db: &'db dyn Db) -> impl Display { - struct DisplaySatisfiedClauses<'a, 'db> { - clauses: &'a SatisfiedClauses<'db>, - db: &'db dyn Db, - } + fn display(&self, db: &'db dyn Db) -> String { + // This is a bit heavy-handed, but we need to output the clauses in a consistent order + // even though Salsa IDs are assigned non-deterministically. This Display output is only + // used in test cases, so we don't need to over-optimize it. - impl Display for DisplaySatisfiedClauses<'_, '_> { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - if self.clauses.clauses.is_empty() { - return f.write_str("always"); - } - for (i, clause) in self.clauses.clauses.iter().enumerate() { - if i > 0 { - f.write_str(" ∨ ")?; - } - clause.display(self.db).fmt(f)?; - } - Ok(()) - } + if self.clauses.is_empty() { + return String::from("always"); } - - DisplaySatisfiedClauses { clauses: self, db } + let mut clauses: Vec<_> = self + .clauses + .iter() + .map(|clause| clause.display(db)) + .collect(); + clauses.sort(); + clauses.join(" ∨ ") } }