Repository: astral-sh/ruff
File: crates/ty_python_semantic/src/types/builder.rs (Rust)

Code before:
        // shouldn't try to simplify aliases out of the union, because that will require
        // unpacking them.
        let should_simplify_full = !matches!(ty, Type::TypeAlias(_)) && !self.cycle_recovery;

        let mut ty_negated: Option<Type> = None;

        let mut i = 0;
        let mut insertion_point: Option<usize> = None;

        let mut remove_or_replace = |i: usize, elements: &mut Vec<UnionElement<'db>>| {
            if insertion_point.is_none() {
                insertion_point = Some(i);
            } else {
                elements.swap_remove(i);
            }
        };

        while i < self.elements.len() {
            let element = &mut self.elements[i];

            let element_type = match element.try_reduce(self.db, ty) {
                ReduceResult::KeepIf(keep) => {
                    if !keep {
                        remove_or_replace(i, &mut self.elements);
                    }
                    i += 1;
                    continue;
                }
                ReduceResult::Type(ty) => ty,
                ReduceResult::CollapseToObject => {
                    self.collapse_to_object();
                    return;
                }
                ReduceResult::Ignore => {
                    return;
                }
            };

            if ty == element_type {
                return;
            }

            if Some(element_type) == bool_pair {
                self.add_in_place_impl(KnownClass::Bool.to_instance(self.db), seen_aliases);
                return;
            }

            // Comparing `TypedDict`s for redundancy requires iterating over their fields, which is
            // problematic if some of those fields point to recursive `Union`s. To avoid cycles,
            // compare `TypedDict`s by name/identity instead of using the `has_relation_to`
            // machinery.

Review comment (suggestion):
I think I'd prefer a design where `i` is only incremented in a single place, at the start of the loop body. It feels more robust because you don't have to remember to do `i += 1` every time you have a `continue` in the loop:

```diff
diff --git a/crates/ty_python_semantic/src/types/builder.rs b/crates/ty_python_semantic/src/types/builder.rs
index b5042d41be..89dc16c561 100644
--- a/crates/ty_python_semantic/src/types/builder.rs
+++ b/crates/ty_python_semantic/src/types/builder.rs
@@ -560,6 +560,7 @@ impl<'db> UnionBuilder<'db> {
         let mut ty_negated: Option<Type> = None;
 
         let mut i = 0;
+        let mut is_first = true;
         let mut insertion_point: Option<usize> = None;
 
         let mut remove_or_replace = |i: usize, elements: &mut Vec<UnionElement<'db>>| {
@@ -570,7 +571,17 @@ impl<'db> UnionBuilder<'db> {
             }
         };
 
-        while i < self.elements.len() {
+        loop {
+            if !is_first {
+                i += 1;
+            }
+
+            if i >= self.elements.len() {
+                break;
+            }
+
+            is_first = false;
+
             let element = &mut self.elements[i];
 
             let element_type = match element.try_reduce(self.db, ty) {
@@ -578,7 +589,6 @@ impl<'db> UnionBuilder<'db> {
                     if !keep {
                         remove_or_replace(i, &mut self.elements);
                     }
-                    i += 1;
                     continue;
                 }
                 ReduceResult::Type(ty) => ty,
@@ -605,7 +615,6 @@ impl<'db> UnionBuilder<'db> {
             // compare `TypedDict`s by name/identity instead of using the `has_relation_to`
             // machinery.
             if element_type.is_typed_dict() && ty.is_typed_dict() {
-                i += 1;
                 continue;
             }
 
@@ -616,7 +625,6 @@ impl<'db> UnionBuilder<'db> {
 
                 if element_type.is_redundant_with(self.db, ty) {
                     remove_or_replace(i, &mut self.elements);
-                    i += 1;
                     continue;
                 }
 
@@ -635,8 +643,6 @@ impl<'db> UnionBuilder<'db> {
                     return;
                 }
             }
-
-            i += 1;
         }
```

Code after:

        // If an alias gets here, it means we aren't unpacking aliases, and we also
        // shouldn't try to simplify aliases out of the union, because that will require
        // unpacking them.
        let should_simplify_full = !matches!(ty, Type::TypeAlias(_)) && !self.cycle_recovery;

        let mut ty_negated: Option<Type> = None;
        let mut to_remove = SmallVec::<[usize; 2]>::new();

        for (i, element) in self.elements.iter_mut().enumerate() {
            let element_type = match element.try_reduce(self.db, ty) {
                ReduceResult::KeepIf(keep) => {
                    if !keep {
                        to_remove.push(i);
                    }
                    continue;
                }
                ReduceResult::Type(ty) => ty,
                ReduceResult::CollapseToObject => {
                    self.collapse_to_object();
                    return;
                }
                ReduceResult::Ignore => {
                    return;
                }
            };

            if ty == element_type {
                return;
            }

            if Some(element_type) == bool_pair {
                self.add_in_place_impl(KnownClass::Bool.to_instance(self.db), seen_aliases);
                return;
            }

            // Comparing `TypedDict`s for redundancy requires iterating over their fields, which is
            // problematic if some of those fields point to recursive `Union`s. To avoid cycles,
            // compare `TypedDict`s by name/identity instead of using the `has_relation_to`
            // machinery.
            if element_type.is_typed_dict() && ty.is_typed_dict() {
                continue;
            }

            if should_simplify_full && !matches!(element_type, Type::TypeAlias(_)) {
                if ty.is_redundant_with(self.db, element_type) {
                    return;
                }

                if element_type.is_redundant_with(self.db, ty) {
                    to_remove.push(i);