[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.

Author: dcreager

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)))
 ```
 

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,
-        }
-
-        impl Display for DisplaySatisfiedClause<'_, '_> {
-            fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-                if self.clause.constraints.len() > 1 {
-                    f.write_str("(")?;
-                }
-                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(")")?;
+    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.
+
+        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()
                 }
-                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(" ∨ ")
     }
 }