From a69dfd4a7422a5b8dd0c51929bc43c2ff297f6bc Mon Sep 17 00:00:00 2001 From: David Peter Date: Tue, 3 Dec 2024 08:28:36 +0100 Subject: [PATCH] [red-knot] Simplify tuples containing `Never` (#14744) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Simplify tuples containing `Never` to `Never`: ```py from typing import Never def never() -> Never: ... reveal_type((1, never(), "foo")) # revealed: Never ``` I should note that mypy and pyright do *not* perform this simplification. I don't know why. There is [only one place](https://github.com/astral-sh/ruff/blob/5137fcc9c81610f687b6cb45413ef83c2c5eea73/crates/red_knot_python_semantic/src/types/infer.rs#L1477-L1484) where we use `TupleType::new` directly (instead of `Type::tuple`, which changes behavior here). This appears when creating `TypeVar` constraints, and it looks to me like it should stay this way, because we're using `TupleType` to store a list of constraints there, instead of an actual type. We also store `tuple[constraint1, constraint2, …]` as the type for the `constraint1, constraint2, …` tuple expression. This would mean that we infer a type of `tuple[str, Never]` for the following type variable constraints, without simplifying it to `Never`. This seems like a weird edge case that's maybe not worth looking further into?! ```py from typing import Never # vvvvvvvvvv def f[T: (str, Never)](x: T): pass ``` ## Test Plan - Added a new unit test. Did not add additional Markdown tests as that seems superfluous. - Tested the example above using red knot, mypy, pyright. - Verified that this allows us to remove `contains_never` from the property tests (https://github.com/astral-sh/ruff/pull/14178#discussion_r1866473192) --- crates/red_knot_python_semantic/src/types.rs | 36 ++++++++++++++++--- .../src/types/infer.rs | 2 +- .../src/types/unpacker.rs | 2 +- 3 files changed, 34 insertions(+), 6 deletions(-) diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index f499790f5a..7e412936db 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -574,8 +574,11 @@ impl<'db> Type<'db> { Self::BytesLiteral(BytesLiteralType::new(db, bytes)) } - pub fn tuple(db: &'db dyn Db, elements: &[Type<'db>]) -> Self { - Self::Tuple(TupleType::new(db, elements)) + pub fn tuple>>( + db: &'db dyn Db, + elements: impl IntoIterator, + ) -> Self { + TupleType::from_elements(db, elements) } #[must_use] @@ -3015,6 +3018,23 @@ pub struct TupleType<'db> { } impl<'db> TupleType<'db> { + pub fn from_elements>>( + db: &'db dyn Db, + types: impl IntoIterator, + ) -> Type<'db> { + let mut elements = vec![]; + + for ty in types { + let ty = ty.into(); + if ty.is_never() { + return Type::Never; + } + elements.push(ty); + } + + Type::Tuple(Self::new(db, elements.into_boxed_slice())) + } + pub fn get(&self, db: &'db dyn Db, index: usize) -> Option> { self.elements(db).get(index).copied() } @@ -3122,13 +3142,21 @@ pub(crate) mod tests { builder.build() } Ty::Tuple(tys) => { - let elements: Vec = tys.into_iter().map(|ty| ty.into_type(db)).collect(); - Type::tuple(db, &elements) + let elements = tys.into_iter().map(|ty| ty.into_type(db)); + Type::tuple(db, elements) } } } } + #[test_case(Ty::Tuple(vec![Ty::Never]))] + #[test_case(Ty::Tuple(vec![Ty::BuiltinInstance("str"), Ty::Never, Ty::BuiltinInstance("int")]))] + #[test_case(Ty::Tuple(vec![Ty::Tuple(vec![Ty::Never])]))] + fn tuple_containing_never_simplifies_to_never(ty: Ty) { + let db = setup_db(); + assert_eq!(ty.into_type(&db), Type::Never); + } + #[test_case(Ty::BuiltinInstance("str"), Ty::BuiltinInstance("object"))] #[test_case(Ty::BuiltinInstance("int"), Ty::BuiltinInstance("object"))] #[test_case(Ty::Unknown, Ty::IntLiteral(1))] diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index a27603d041..9471a6fb11 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -4537,7 +4537,7 @@ impl<'db> TypeInferenceBuilder<'db> { if element_could_alter_type_of_whole_tuple(single_element, single_element_ty) { todo_type!() } else { - Type::tuple(self.db, &[single_element_ty]) + Type::tuple(self.db, [single_element_ty]) } } } diff --git a/crates/red_knot_python_semantic/src/types/unpacker.rs b/crates/red_knot_python_semantic/src/types/unpacker.rs index 2e8c39a9b2..dc96fac252 100644 --- a/crates/red_knot_python_semantic/src/types/unpacker.rs +++ b/crates/red_knot_python_semantic/src/types/unpacker.rs @@ -95,7 +95,7 @@ impl<'db> Unpacker<'db> { // there would be a cost and it's not clear that it's worth it. let value_ty = Type::tuple( self.db, - &vec![Type::LiteralString; string_literal_ty.len(self.db)], + std::iter::repeat(Type::LiteralString).take(string_literal_ty.len(self.db)), ); self.unpack(target, value_ty, scope); }