From d9616e61b04fe9a09cb3399e86f83722cb5af4a7 Mon Sep 17 00:00:00 2001 From: David Peter Date: Mon, 31 Mar 2025 10:49:19 +0200 Subject: [PATCH] [red-knot] Disallow `todo_type!` without custom message (#17083) ## Summary Disallow empty `todo_type!()`s without a custom message. They can lead to spurious diffs in `mypy_primer` where the only thing that's changed is the file/line information. --- crates/red_knot_python_semantic/src/types.rs | 40 +++++-------------- .../src/types/infer.rs | 24 ++++++----- .../src/types/type_ordering.rs | 13 +----- 3 files changed, 24 insertions(+), 53 deletions(-) diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index 64032b633b..c8b9c31182 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -175,18 +175,12 @@ impl AttributeKind { /// Meta data for `Type::Todo`, which represents a known limitation in red-knot. #[cfg(debug_assertions)] #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] -pub enum TodoType { - FileAndLine(&'static str, u32), - Message(&'static str), -} +pub struct TodoType(pub &'static str); #[cfg(debug_assertions)] impl std::fmt::Display for TodoType { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - TodoType::FileAndLine(file, line) => write!(f, "[{file}:{line}]"), - TodoType::Message(msg) => write!(f, "({msg})"), - } + write!(f, "({msg})", msg = self.0) } } @@ -203,24 +197,18 @@ impl std::fmt::Display for TodoType { /// Create a `Type::Todo` variant to represent a known limitation in the type system. /// -/// It can be used with a custom message (preferred): `todo_type!("PEP 604 not supported")`, -/// or simply using `todo_type!()`, which will include information about the file and line. +/// It can be created by specifying a custom message: `todo_type!("PEP 604 not supported")`. #[cfg(debug_assertions)] macro_rules! todo_type { - () => { - $crate::types::Type::Dynamic($crate::types::DynamicType::Todo( - $crate::types::TodoType::FileAndLine(file!(), line!()), - )) - }; ($message:literal) => { - $crate::types::Type::Dynamic($crate::types::DynamicType::Todo( - $crate::types::TodoType::Message($message), - )) + $crate::types::Type::Dynamic($crate::types::DynamicType::Todo($crate::types::TodoType( + $message, + ))) }; ($message:ident) => { - $crate::types::Type::Dynamic($crate::types::DynamicType::Todo( - $crate::types::TodoType::Message($message), - )) + $crate::types::Type::Dynamic($crate::types::DynamicType::Todo($crate::types::TodoType( + $message, + ))) }; } @@ -5625,16 +5613,12 @@ pub(crate) mod tests { let todo1 = todo_type!("1"); let todo2 = todo_type!("2"); - let todo3 = todo_type!(); - let todo4 = todo_type!(); let int = KnownClass::Int.to_instance(&db); assert!(int.is_assignable_to(&db, todo1)); - assert!(int.is_assignable_to(&db, todo3)); assert!(todo1.is_assignable_to(&db, int)); - assert!(todo3.is_assignable_to(&db, int)); // We lose information when combining several `Todo` types. This is an // acknowledged limitation of the current implementation. We can not @@ -5646,21 +5630,17 @@ pub(crate) mod tests { // salsa, but that would mean we would have to pass in `db` everywhere. // A union of several `Todo` types collapses to a single `Todo` type: - assert!(UnionType::from_elements(&db, vec![todo1, todo2, todo3, todo4]).is_todo()); + assert!(UnionType::from_elements(&db, vec![todo1, todo2]).is_todo()); // And similar for intersection types: assert!(IntersectionBuilder::new(&db) .add_positive(todo1) .add_positive(todo2) - .add_positive(todo3) - .add_positive(todo4) .build() .is_todo()); assert!(IntersectionBuilder::new(&db) .add_positive(todo1) .add_negative(todo2) - .add_positive(todo3) - .add_negative(todo4) .build() .is_todo()); } diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 7f97c18535..161c498945 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -3596,8 +3596,7 @@ impl<'db> TypeInferenceBuilder<'db> { self.infer_first_comprehension_iter(generators); - // TODO generator type - todo_type!() + todo_type!("generator type") } fn infer_list_comprehension_expression(&mut self, listcomp: &ast::ExprListComp) -> Type<'db> { @@ -3609,8 +3608,7 @@ impl<'db> TypeInferenceBuilder<'db> { self.infer_first_comprehension_iter(generators); - // TODO list type - todo_type!() + todo_type!("list comprehension type") } fn infer_dict_comprehension_expression(&mut self, dictcomp: &ast::ExprDictComp) -> Type<'db> { @@ -3623,8 +3621,7 @@ impl<'db> TypeInferenceBuilder<'db> { self.infer_first_comprehension_iter(generators); - // TODO dict type - todo_type!() + todo_type!("dict comprehension type") } fn infer_set_comprehension_expression(&mut self, setcomp: &ast::ExprSetComp) -> Type<'db> { @@ -3636,8 +3633,7 @@ impl<'db> TypeInferenceBuilder<'db> { self.infer_first_comprehension_iter(generators); - // TODO set type - todo_type!() + todo_type!("set comprehension type") } fn infer_generator_expression_scope(&mut self, generator: &ast::ExprGenerator) { @@ -5979,7 +5975,9 @@ impl<'db> TypeInferenceBuilder<'db> { } } ast::ExprContext::Invalid => TypeAndQualifiers::unknown(), - ast::ExprContext::Store | ast::ExprContext::Del => todo_type!().into(), + ast::ExprContext::Store | ast::ExprContext::Del => { + todo_type!("Name expression annotation in Store/Del context").into() + } }, ast::Expr::Subscript(subscript @ ast::ExprSubscript { value, slice, .. }) => { @@ -6136,7 +6134,9 @@ impl<'db> TypeInferenceBuilder<'db> { .in_type_expression(self.db()) .unwrap_or_else(|error| error.into_fallback_type(&self.context, expression)), ast::ExprContext::Invalid => Type::unknown(), - ast::ExprContext::Store | ast::ExprContext::Del => todo_type!(), + ast::ExprContext::Store | ast::ExprContext::Del => { + todo_type!("Name expression annotation in Store/Del context") + } }, ast::Expr::Attribute(attribute_expression) => match attribute_expression.ctx { @@ -6145,7 +6145,9 @@ impl<'db> TypeInferenceBuilder<'db> { .in_type_expression(self.db()) .unwrap_or_else(|error| error.into_fallback_type(&self.context, expression)), ast::ExprContext::Invalid => Type::unknown(), - ast::ExprContext::Store | ast::ExprContext::Del => todo_type!(), + ast::ExprContext::Store | ast::ExprContext::Del => { + todo_type!("Attribute expression annotation in Store/Del context") + } }, ast::Expr::NoneLiteral(_literal) => Type::none(self.db()), diff --git a/crates/red_knot_python_semantic/src/types/type_ordering.rs b/crates/red_knot_python_semantic/src/types/type_ordering.rs index 79f0abe1d4..610c535663 100644 --- a/crates/red_knot_python_semantic/src/types/type_ordering.rs +++ b/crates/red_knot_python_semantic/src/types/type_ordering.rs @@ -325,18 +325,7 @@ fn dynamic_elements_ordering(left: DynamicType, right: DynamicType) -> Ordering (_, DynamicType::Unknown) => Ordering::Greater, #[cfg(debug_assertions)] - (DynamicType::Todo(left), DynamicType::Todo(right)) => match (left, right) { - ( - TodoType::FileAndLine(left_file, left_line), - TodoType::FileAndLine(right_file, right_line), - ) => left_file - .cmp(right_file) - .then_with(|| left_line.cmp(&right_line)), - (TodoType::FileAndLine(..), _) => Ordering::Less, - (_, TodoType::FileAndLine(..)) => Ordering::Greater, - - (TodoType::Message(left), TodoType::Message(right)) => left.cmp(right), - }, + (DynamicType::Todo(TodoType(left)), DynamicType::Todo(TodoType(right))) => left.cmp(right), #[cfg(not(debug_assertions))] (DynamicType::Todo(TodoType), DynamicType::Todo(TodoType)) => Ordering::Equal,