Skip to content

Commit

Permalink
Remove Type::tuple in favor of TupleType::from_elements (astral-s…
Browse files Browse the repository at this point in the history
…h#15218)

## Summary

Remove `Type::tuple` in favor of `TupleType::from_elements`, avoid a few
intermediate `Vec`tors. Resolves an old [review
comment](astral-sh#14744 (comment)).

## Test Plan

New regression test for something I ran into while implementing this.
  • Loading branch information
sharkdp authored Jan 2, 2025
1 parent 11e873e commit 7671a3b
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 27 deletions.
32 changes: 13 additions & 19 deletions crates/red_knot_python_semantic/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -705,13 +705,6 @@ impl<'db> Type<'db> {
Self::BytesLiteral(BytesLiteralType::new(db, bytes))
}

pub fn tuple<T: Into<Type<'db>>>(
db: &'db dyn Db,
elements: impl IntoIterator<Item = T>,
) -> Self {
TupleType::from_elements(db, elements)
}

#[must_use]
pub fn negate(&self, db: &'db dyn Db) -> Type<'db> {
IntersectionBuilder::new(db).add_negative(*self).build()
Expand Down Expand Up @@ -2118,15 +2111,16 @@ impl<'db> Type<'db> {
Type::Union(UnionType::new(db, elements))
};

let version_info_elements = &[
Type::IntLiteral(python_version.major.into()),
Type::IntLiteral(python_version.minor.into()),
int_instance_ty,
release_level_ty,
int_instance_ty,
];

Self::tuple(db, version_info_elements)
TupleType::from_elements(
db,
[
Type::IntLiteral(python_version.major.into()),
Type::IntLiteral(python_version.minor.into()),
int_instance_ty,
release_level_ty,
int_instance_ty,
],
)
}

/// Given a type that is assumed to represent an instance of a class,
Expand Down Expand Up @@ -3435,8 +3429,8 @@ impl<'db> Class<'db> {
/// The member resolves to a member on the class itself or any of its proper superclasses.
pub(crate) fn class_member(self, db: &'db dyn Db, name: &str) -> Symbol<'db> {
if name == "__mro__" {
let tuple_elements: Vec<Type<'db>> = self.iter_mro(db).map(Type::from).collect();
return Type::tuple(db, &tuple_elements).into();
let tuple_elements = self.iter_mro(db).map(Type::from);
return TupleType::from_elements(db, tuple_elements).into();
}

for superclass in self.iter_mro(db) {
Expand Down Expand Up @@ -3846,7 +3840,7 @@ pub(crate) mod tests {
}
Ty::Tuple(tys) => {
let elements = tys.into_iter().map(|ty| ty.into_type(db));
Type::tuple(db, elements)
TupleType::from_elements(db, elements)
}
Ty::SubclassOfAny => Type::subclass_of_base(ClassBase::Any),
Ty::SubclassOfUnknown => Type::subclass_of_base(ClassBase::Unknown),
Expand Down
15 changes: 8 additions & 7 deletions crates/red_knot_python_semantic/src/types/infer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2672,10 +2672,12 @@ impl<'db> TypeInferenceBuilder<'db> {
parenthesized: _,
} = tuple;

let element_types: Vec<Type<'db>> =
elts.iter().map(|elt| self.infer_expression(elt)).collect();
// Collecting all elements is necessary to infer all sub-expressions even if some
// element types are `Never` (which leads `from_elements` to return early without
// consuming the whole iterator).
let element_types: Vec<_> = elts.iter().map(|elt| self.infer_expression(elt)).collect();

Type::tuple(self.db(), &element_types)
TupleType::from_elements(self.db(), element_types)
}

fn infer_list_expression(&mut self, list: &ast::ExprList) -> Type<'db> {
Expand Down Expand Up @@ -4239,8 +4241,7 @@ impl<'db> TypeInferenceBuilder<'db> {
let (start, stop, step) = slice_ty.as_tuple(self.db());

if let Ok(new_elements) = elements.py_slice(start, stop, step) {
let new_elements: Vec<_> = new_elements.copied().collect();
Type::tuple(self.db(), &new_elements)
TupleType::from_elements(self.db(), new_elements)
} else {
report_slice_step_size_zero(&self.context, value_node.into());
Type::Unknown
Expand Down Expand Up @@ -4842,7 +4843,7 @@ impl<'db> TypeInferenceBuilder<'db> {
let ty = if return_todo {
todo_type!("full tuple[...] support")
} else {
Type::tuple(self.db(), &element_types)
TupleType::from_elements(self.db(), element_types)
};

// Here, we store the type for the inner `int, str` tuple-expression,
Expand All @@ -4857,7 +4858,7 @@ impl<'db> TypeInferenceBuilder<'db> {
if element_could_alter_type_of_whole_tuple(single_element, single_element_ty) {
todo_type!("full tuple[...] support")
} else {
Type::tuple(self.db(), [single_element_ty])
TupleType::from_elements(self.db(), std::iter::once(single_element_ty))
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/red_knot_python_semantic/src/types/unpacker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ impl<'db> Unpacker<'db> {
// with each individual character, instead of just an array of
// `LiteralString`, but there would be a cost and it's not clear that
// it's worth it.
Type::tuple(
TupleType::from_elements(
self.db(),
std::iter::repeat(Type::LiteralString)
.take(string_literal_ty.python_len(self.db())),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
"""
Regression test that makes sure we do not short-circuit here after
determining that the overall type will be `Never` and still infer
a type for the second tuple element `2`.
Relevant discussion:
https://github.com/astral-sh/ruff/pull/15218#discussion_r1900811073
"""

from typing_extensions import Never


def never() -> Never:
return never()


(never(), 2)

0 comments on commit 7671a3b

Please sign in to comment.