From 268a8f896f28fef1dc853fad083abea15f183e20 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Thu, 18 Dec 2025 08:17:56 +0100 Subject: [PATCH 1/4] [ty] Small union builder nits --- .../ty_python_semantic/src/types/builder.rs | 55 ++++++++++++------- 1 file changed, 34 insertions(+), 21 deletions(-) diff --git a/crates/ty_python_semantic/src/types/builder.rs b/crates/ty_python_semantic/src/types/builder.rs index c279ddbd27b58..aea7aab325e84 100644 --- a/crates/ty_python_semantic/src/types/builder.rs +++ b/crates/ty_python_semantic/src/types/builder.rs @@ -549,18 +549,30 @@ impl<'db> UnionBuilder<'db> { // unpacking them. let should_simplify_full = !matches!(ty, Type::TypeAlias(_)) && !self.cycle_recovery; - let mut to_remove = SmallVec::<[usize; 2]>::new(); - let ty_negated = if should_simplify_full { - ty.negate(self.db) - } else { - Type::Never // won't be used + let mut ty_negated: Option = None; + + let mut i = 0; + let mut inserted = false; + + let mut remove_element = |i: &mut usize, elements: &mut Vec>| { + if inserted { + elements.swap_remove(*i); + } else { + elements[*i] = UnionElement::Type(ty); + *i += 1; + } + inserted = true; }; - for (index, element) in self.elements.iter_mut().enumerate() { + 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 { - to_remove.push(index); + remove_element(&mut i, &mut self.elements); + } else { + i += 1; } continue; } @@ -587,19 +599,23 @@ impl<'db> UnionBuilder<'db> { // 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 let (Type::TypedDict(element_td), Type::TypedDict(ty_td)) = (element_type, ty) { - if element_td == ty_td { - return; - } + if element_type.is_typed_dict() && ty.is_typed_dict() { + i += 1; continue; } if should_simplify_full && !matches!(element_type, Type::TypeAlias(_)) { if ty.is_redundant_with(self.db, element_type) { return; - } else if element_type.is_redundant_with(self.db, ty) { - to_remove.push(index); - } else if ty_negated.is_subtype_of(self.db, element_type) { + } + + if element_type.is_redundant_with(self.db, ty) { + remove_element(&mut i, &mut self.elements); + continue; + } + + let negated = ty_negated.get_or_insert_with(|| ty.negate(self.db)); + if negated.is_subtype_of(self.db, element_type) { // We add `ty` to the union. We just checked that `~ty` is a subtype of an // existing `element`. This also means that `~ty | ty` is a subtype of // `element | ty`, because both elements in the first union are subtypes of @@ -613,14 +629,11 @@ impl<'db> UnionBuilder<'db> { return; } } + + i += 1; } - if let Some((&first, rest)) = to_remove.split_first() { - self.elements[first] = UnionElement::Type(ty); - // We iterate in descending order to keep remaining indices valid after `swap_remove`. - for &index in rest.iter().rev() { - self.elements.swap_remove(index); - } - } else { + + if !inserted { self.elements.push(UnionElement::Type(ty)); } } From 0a937d2ad5bdbe0f3c2a5cbcd2e36c5990d45e6a Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Thu, 18 Dec 2025 09:01:08 +0100 Subject: [PATCH 2/4] Defer insertion --- .../ty_python_semantic/src/types/builder.rs | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/crates/ty_python_semantic/src/types/builder.rs b/crates/ty_python_semantic/src/types/builder.rs index aea7aab325e84..65396d04e9aa1 100644 --- a/crates/ty_python_semantic/src/types/builder.rs +++ b/crates/ty_python_semantic/src/types/builder.rs @@ -552,16 +552,14 @@ impl<'db> UnionBuilder<'db> { let mut ty_negated: Option = None; let mut i = 0; - let mut inserted = false; + let mut insertion_point: Option = None; - let mut remove_element = |i: &mut usize, elements: &mut Vec>| { - if inserted { - elements.swap_remove(*i); + let mut remove_or_replace = |i: usize, elements: &mut Vec>| { + if insertion_point.is_none() { + insertion_point = Some(i); } else { - elements[*i] = UnionElement::Type(ty); - *i += 1; + elements.swap_remove(i); } - inserted = true; }; while i < self.elements.len() { @@ -570,10 +568,9 @@ impl<'db> UnionBuilder<'db> { let element_type = match element.try_reduce(self.db, ty) { ReduceResult::KeepIf(keep) => { if !keep { - remove_element(&mut i, &mut self.elements); - } else { - i += 1; + remove_or_replace(i, &mut self.elements); } + i += 1; continue; } ReduceResult::Type(ty) => ty, @@ -610,7 +607,8 @@ impl<'db> UnionBuilder<'db> { } if element_type.is_redundant_with(self.db, ty) { - remove_element(&mut i, &mut self.elements); + remove_or_replace(i, &mut self.elements); + i += 1; continue; } @@ -633,7 +631,9 @@ impl<'db> UnionBuilder<'db> { i += 1; } - if !inserted { + if let Some(insertion_point) = insertion_point { + self.elements[insertion_point] = UnionElement::Type(ty); + } else { self.elements.push(UnionElement::Type(ty)); } } From 368405a67eeb21eb067ad6f0e0e8560121d19088 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Thu, 18 Dec 2025 09:17:00 +0100 Subject: [PATCH 3/4] More lazy negated computations --- .../ty_python_semantic/src/types/builder.rs | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/crates/ty_python_semantic/src/types/builder.rs b/crates/ty_python_semantic/src/types/builder.rs index 65396d04e9aa1..b5042d41be734 100644 --- a/crates/ty_python_semantic/src/types/builder.rs +++ b/crates/ty_python_semantic/src/types/builder.rs @@ -365,7 +365,7 @@ impl<'db> UnionBuilder<'db> { Type::StringLiteral(literal) => { let mut found = None; let mut to_remove = None; - let ty_negated = ty.negate(self.db); + let mut ty_negated = None; for (index, element) in self.elements.iter_mut().enumerate() { match element { UnionElement::StringLiterals(literals) => { @@ -383,8 +383,10 @@ impl<'db> UnionBuilder<'db> { } if existing.is_subtype_of(self.db, ty) { to_remove = Some(index); + continue; } - if ty_negated.is_subtype_of(self.db, *existing) { + let negated = ty_negated.get_or_insert_with(|| ty.negate(self.db)); + if negated.is_subtype_of(self.db, *existing) { // The type that includes both this new element, and its negation // (or a supertype of its negation), must be simply `object`. self.collapse_to_object(); @@ -410,7 +412,7 @@ impl<'db> UnionBuilder<'db> { Type::BytesLiteral(literal) => { let mut found = None; let mut to_remove = None; - let ty_negated = ty.negate(self.db); + let mut ty_negated = None; for (index, element) in self.elements.iter_mut().enumerate() { match element { UnionElement::BytesLiterals(literals) => { @@ -428,8 +430,11 @@ impl<'db> UnionBuilder<'db> { } if existing.is_subtype_of(self.db, ty) { to_remove = Some(index); + continue; } - if ty_negated.is_subtype_of(self.db, *existing) { + + let negated = ty_negated.get_or_insert_with(|| ty.negate(self.db)); + if negated.is_subtype_of(self.db, *existing) { // The type that includes both this new element, and its negation // (or a supertype of its negation), must be simply `object`. self.collapse_to_object(); @@ -455,7 +460,7 @@ impl<'db> UnionBuilder<'db> { Type::IntLiteral(literal) => { let mut found = None; let mut to_remove = None; - let ty_negated = ty.negate(self.db); + let mut ty_negated = None; for (index, element) in self.elements.iter_mut().enumerate() { match element { UnionElement::IntLiterals(literals) => { @@ -473,8 +478,11 @@ impl<'db> UnionBuilder<'db> { } if existing.is_subtype_of(self.db, ty) { to_remove = Some(index); + continue; } - if ty_negated.is_subtype_of(self.db, *existing) { + + let negated = ty_negated.get_or_insert_with(|| ty.negate(self.db)); + if negated.is_subtype_of(self.db, *existing) { // The type that includes both this new element, and its negation // (or a supertype of its negation), must be simply `object`. self.collapse_to_object(); From 3848ada83f41dc958675c54c546adf62d5752656 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Fri, 19 Dec 2025 08:04:02 +0100 Subject: [PATCH 4/4] Code review feedback, revert in-place removal changes --- .../ty_python_semantic/src/types/builder.rs | 36 +++++++------------ 1 file changed, 13 insertions(+), 23 deletions(-) diff --git a/crates/ty_python_semantic/src/types/builder.rs b/crates/ty_python_semantic/src/types/builder.rs index b5042d41be734..b7cc49497a177 100644 --- a/crates/ty_python_semantic/src/types/builder.rs +++ b/crates/ty_python_semantic/src/types/builder.rs @@ -378,6 +378,8 @@ impl<'db> UnionBuilder<'db> { continue; } UnionElement::Type(existing) => { + // e.g. `existing` could be `Literal[""] & Any`, + // and `ty` could be `Literal[""]` if ty.is_subtype_of(self.db, *existing) { return; } @@ -558,27 +560,14 @@ impl<'db> UnionBuilder<'db> { let should_simplify_full = !matches!(ty, Type::TypeAlias(_)) && !self.cycle_recovery; let mut ty_negated: Option = None; + let mut to_remove = SmallVec::<[usize; 2]>::new(); - let mut i = 0; - let mut insertion_point: Option = None; - - let mut remove_or_replace = |i: usize, elements: &mut Vec>| { - 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]; - + for (i, element) in self.elements.iter_mut().enumerate() { let element_type = match element.try_reduce(self.db, ty) { ReduceResult::KeepIf(keep) => { if !keep { - remove_or_replace(i, &mut self.elements); + to_remove.push(i); } - i += 1; continue; } ReduceResult::Type(ty) => ty, @@ -605,7 +594,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; } @@ -615,8 +603,7 @@ impl<'db> UnionBuilder<'db> { } if element_type.is_redundant_with(self.db, ty) { - remove_or_replace(i, &mut self.elements); - i += 1; + to_remove.push(i); continue; } @@ -635,12 +622,15 @@ impl<'db> UnionBuilder<'db> { return; } } - - i += 1; } - if let Some(insertion_point) = insertion_point { - self.elements[insertion_point] = UnionElement::Type(ty); + let mut to_remove = to_remove.into_iter(); + if let Some(first) = to_remove.next() { + self.elements[first] = UnionElement::Type(ty); + // We iterate in descending order to keep remaining indices valid after `swap_remove`. + for index in to_remove.rev() { + self.elements.swap_remove(index); + } } else { self.elements.push(UnionElement::Type(ty)); }