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

Code before:
    StringLiterals(FxOrderSet<StringLiteralType<'db>>),
    BytesLiterals(FxOrderSet<BytesLiteralType<'db>>),
    Type(Type<'db>),
}

impl<'db> UnionElement<'db> {
    const fn to_type_element(&self) -> Option<Type<'db>> {
        match self {
            UnionElement::Type(ty) => Some(*ty),
            _ => None,
        }
    }

    /// Try reducing this `UnionElement` given the presence in the same union of `other_type`.
    fn try_reduce(&mut self, db: &'db dyn Db, other_type: Type<'db>) -> ReduceResult<'db> {
        let mut other_type_negated_cache = None;
        let mut other_type_negated =
            || *other_type_negated_cache.get_or_insert_with(|| other_type.negate(db));

        let mut collapse = false;
        let mut ignore = false;
        let mut should_retain_type = |ty| {
            // it's tempting to use `|=` but then the r.h.s. wouldn't be lazily evaluated
            ignore = ignore || other_type.is_subtype_of(db, ty);
            collapse = collapse || ignore || other_type_negated().is_subtype_of(db, ty);
            ignore || collapse || !ty.is_subtype_of(db, other_type)
        };

        let should_keep = match self {
            UnionElement::IntLiterals(literals) => {
                if other_type.splits_literals(db, LiteralKind::Int) {
                    literals.retain(|literal| should_retain_type(Type::IntLiteral(*literal)));
                    !literals.is_empty()
                } else {
                    !Type::IntLiteral(literals[0]).is_subtype_of(db, other_type)
                }
            }
            UnionElement::StringLiterals(literals) => {
                if other_type.splits_literals(db, LiteralKind::String) {
                    literals.retain(|literal| should_retain_type(Type::StringLiteral(*literal)));
                    !literals.is_empty()
                } else {
                    !Type::StringLiteral(literals[0]).is_subtype_of(db, other_type)
                }
            }
            UnionElement::BytesLiterals(literals) => {
                if other_type.splits_literals(db, LiteralKind::Bytes) {
                    literals.retain(|literal| should_retain_type(Type::BytesLiteral(*literal)));
                    !literals.is_empty()
                } else {
                    !Type::BytesLiteral(literals[0]).is_subtype_of(db, other_type)

Review comment (suggestion):
I think I would write this as
```suggestion
				let mut should_retain_type = |ty| {
            if ignore {
                return true;
            }

            if other_type.is_subtype_of(db, ty) {
                ignore = true;
                return true;
            }

            if collapse {
                return true;
            }

            if other_type_negated().is_subtype_of(db, ty) {
                collapse = true;
                return true;
            }

            !ty.is_subtype_of(db, other_type)
        };
```

or

```
						let mut should_retain_type = |ty| {
            if ignore || other_type.is_subtype_of(db, ty) {
								ignore = true;
                return true;
            }

            if collapse || other_type_negated().is_subtype_of(db, ty) {
                collapse = true;
                return true;
            }

            !ty.is_subtype_of(db, other_type)
        };
```

and add some comments before the early returns (why is it okay to keep retaining the elements when returning `true`)

Code after:
    StringLiterals(FxOrderSet<StringLiteralType<'db>>),
    BytesLiterals(FxOrderSet<BytesLiteralType<'db>>),
    Type(Type<'db>),
}

impl<'db> UnionElement<'db> {
    const fn to_type_element(&self) -> Option<Type<'db>> {
        match self {
            UnionElement::Type(ty) => Some(*ty),
            _ => None,
        }
    }

    /// Try reducing this `UnionElement` given the presence in the same union of `other_type`.
    fn try_reduce(&mut self, db: &'db dyn Db, other_type: Type<'db>) -> ReduceResult<'db> {
        let mut other_type_negated_cache = None;
        let mut other_type_negated =
            || *other_type_negated_cache.get_or_insert_with(|| other_type.negate(db));

        let mut collapse = false;
        let mut ignore = false;

        // A closure called for each element in a set of literals
        // to determine whether the element should be retained in the set.
        //
        // If `ignore` or `collapse` is `true` for any element in the set,
        // we no longer need to do any expensive subtyping checks for any
        // further elements in the set:
        //
        // - if `ignore` is `true`, this indicates that `other_type` is a
        //   subtype of one of the literals in this set. Given this fact,
        //   it cannot be possible for any other literals in this set to be
        //   a subtype of `other_type`.
        // - if `collapse` is `true`, all literals of this kind will be
        //   removed from the union, so it's irrelevant to answer the
        //   question of which literals should remain in this set.
        //
        // We therefore only ask if `ty` is a subtype of `other_type` if
        // both `ignore` and `collapse` are `false`. If either is `true`,
        // we skip the expensive subtype check and return `true`.
        let mut should_retain_type = |ty| {
            if ignore || other_type.is_subtype_of(db, ty) {
                ignore = true;
                return true;
            }
            if collapse || other_type_negated().is_subtype_of(db, ty) {
                collapse = true;
                return true;
            }
            !ty.is_subtype_of(db, other_type)
        };