From 62331fc0d7d7f3f134786d30d96b13f3827836ac Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Wed, 4 Sep 2024 11:49:37 +0100 Subject: [PATCH 1/5] Refactor `Type::iterate()` so that it always emits a diagnostic if the type is not iterable --- crates/red_knot_python_semantic/src/types.rs | 48 +++++++++++-------- .../src/types/infer.rs | 24 ++++++---- 2 files changed, 42 insertions(+), 30 deletions(-) diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index acb7c480259d2d..ffca8eb5bd84c1 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -401,27 +401,35 @@ impl<'db> Type<'db> { /// pass /// ``` /// - /// Returns `None` if `self` represents a type that is not iterable. - fn iterate(&self, db: &'db dyn Db) -> Option> { - // `self` represents the type of the iterable; - // `__iter__` and `__next__` are both looked up on the class of the iterable: - let type_of_class = self.to_meta_type(db); - - let dunder_iter_method = type_of_class.member(db, "__iter__"); - if !dunder_iter_method.is_unbound() { - let iterator_ty = dunder_iter_method.call(db)?; - let dunder_next_method = iterator_ty.to_meta_type(db).member(db, "__next__"); - return dunder_next_method.call(db); - } + /// Emits a diagnostic and returns `Unknown` if `self` represents a type that is not iterable. + fn iterate(&self, db: &'db dyn Db, mut diagnostic_fn: impl FnMut()) -> Type<'db> { + fn loop_var_from_iterable<'db>( + db: &'db dyn Db, + iterable_ty: &Type<'db>, + ) -> Option> { + // `__iter__` and `__next__` are both looked up on the class of the iterable: + let iterable_meta_type = iterable_ty.to_meta_type(db); + + let dunder_iter_method = iterable_meta_type.member(db, "__iter__"); + if !dunder_iter_method.is_unbound() { + let iterator_ty = dunder_iter_method.call(db)?; + let dunder_next_method = iterator_ty.to_meta_type(db).member(db, "__next__"); + return dunder_next_method.call(db); + } - // Although it's not considered great practice, - // classes that define `__getitem__` are also iterable, - // even if they do not define `__iter__`. - // - // TODO this is only valid if the `__getitem__` method is annotated as - // accepting `int` or `SupportsIndex` - let dunder_get_item_method = type_of_class.member(db, "__getitem__"); - dunder_get_item_method.call(db) + // Although it's not considered great practice, + // classes that define `__getitem__` are also iterable, + // even if they do not define `__iter__`. + // + // TODO this is only valid if the `__getitem__` method is annotated as + // accepting `int` or `SupportsIndex` + let dunder_get_item_method = iterable_meta_type.member(db, "__getitem__"); + dunder_get_item_method.call(db) + } + loop_var_from_iterable(db, self).unwrap_or_else(|| { + diagnostic_fn(); + Type::Unknown + }) } #[must_use] diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 98a038afb0c6d6..db282e9f2d5e79 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -1029,6 +1029,18 @@ impl<'db> TypeInferenceBuilder<'db> { self.infer_body(orelse); } + /// Emit a diagnostic declaring that the object represented by `node` is not iterable + fn not_iterable_diagnostic(&mut self, node: AnyNodeRef, iterable_ty: Type<'db>) { + self.add_diagnostic( + node, + "not-iterable", + format_args!( + "Object of type '{}' is not iterable", + iterable_ty.display(self.db) + ), + ); + } + fn infer_for_statement_definition( &mut self, target: &ast::ExprName, @@ -1042,16 +1054,8 @@ impl<'db> TypeInferenceBuilder<'db> { .types .expression_ty(iterable.scoped_ast_id(self.db, self.scope)); - let loop_var_value_ty = iterable_ty.iterate(self.db).unwrap_or_else(|| { - self.add_diagnostic( - iterable.into(), - "not-iterable", - format_args!( - "Object of type '{}' is not iterable", - iterable_ty.display(self.db) - ), - ); - Type::Unknown + let loop_var_value_ty = iterable_ty.iterate(self.db, || { + self.not_iterable_diagnostic(iterable.into(), iterable_ty); }); self.types From 7e87f12ce736c8968a78af520202a82c7301bfbc Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Wed, 4 Sep 2024 12:37:21 +0100 Subject: [PATCH 2/5] emit diagnostics if the value of a starred expression or a `yield from` expression is not iterable --- crates/red_knot_python_semantic/src/types.rs | 61 +++++++++++++++++++ .../src/types/infer.rs | 12 +++- 2 files changed, 70 insertions(+), 3 deletions(-) diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index ffca8eb5bd84c1..012ca806bc4bd3 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -797,4 +797,65 @@ mod tests { &["Object of type 'NotIterable' is not iterable"], ); } + + #[test] + fn starred_expressions_must_be_iterable() { + let mut db = setup_db(); + + db.write_dedented( + "src/a.py", + " + class NotIterable: pass + + class Iterator: + def __next__(self) -> int: + return 42 + + class Iterable: + def __iter__(self) -> Iterator: + + x = [*NotIterable()] + y = [*Iterable()] + ", + ) + .unwrap(); + + let a_file = system_path_to_file(&db, "/src/a.py").unwrap(); + let a_file_diagnostics = super::check_types(&db, a_file); + assert_diagnostic_messages( + &a_file_diagnostics, + &["Object of type 'NotIterable' is not iterable"], + ); + } + + #[test] + fn yield_from_expression_must_be_iterable() { + let mut db = setup_db(); + + db.write_dedented( + "src/a.py", + " + class NotIterable: pass + + class Iterator: + def __next__(self) -> int: + return 42 + + class Iterable: + def __iter__(self) -> Iterator: + + def generator_function(): + yield from Iterable() + yield from NotIterable() + ", + ) + .unwrap(); + + let a_file = system_path_to_file(&db, "/src/a.py").unwrap(); + let a_file_diagnostics = super::check_types(&db, a_file); + assert_diagnostic_messages( + &a_file_diagnostics, + &["Object of type 'NotIterable' is not iterable"], + ); + } } diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index db282e9f2d5e79..13e122d7fbaaee 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -1816,7 +1816,10 @@ impl<'db> TypeInferenceBuilder<'db> { ctx: _, } = starred; - self.infer_expression(value); + let iterable_ty = self.infer_expression(value); + iterable_ty.iterate(self.db, || { + self.not_iterable_diagnostic(value.as_ref().into(), iterable_ty); + }); // TODO Type::Unknown @@ -1834,9 +1837,12 @@ impl<'db> TypeInferenceBuilder<'db> { fn infer_yield_from_expression(&mut self, yield_from: &ast::ExprYieldFrom) -> Type<'db> { let ast::ExprYieldFrom { range: _, value } = yield_from; - self.infer_expression(value); + let iterable_ty = self.infer_expression(value); + iterable_ty.iterate(self.db, || { + self.not_iterable_diagnostic(value.as_ref().into(), iterable_ty); + }); - // TODO get type from awaitable + // TODO get type from `SendType` of generator/awaitable Type::Unknown } From bfd3d4ff24b8d6a8d639bf1a21b76d9efa1ec640 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Wed, 4 Sep 2024 14:05:19 +0100 Subject: [PATCH 3/5] Don't use callbacks --- crates/red_knot_python_semantic/src/types.rs | 36 +++++++++++++++---- .../src/types/infer.rs | 22 ++++++------ 2 files changed, 41 insertions(+), 17 deletions(-) diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index 012ca806bc4bd3..6311242aa22999 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -1,5 +1,6 @@ +use infer::TypeInferenceBuilder; use ruff_db::files::File; -use ruff_python_ast as ast; +use ruff_python_ast::{self as ast, AnyNodeRef}; use crate::semantic_index::ast_ids::HasScopedAstId; use crate::semantic_index::definition::{Definition, DefinitionKind}; @@ -402,7 +403,7 @@ impl<'db> Type<'db> { /// ``` /// /// Emits a diagnostic and returns `Unknown` if `self` represents a type that is not iterable. - fn iterate(&self, db: &'db dyn Db, mut diagnostic_fn: impl FnMut()) -> Type<'db> { + fn iterate(&self, db: &'db dyn Db) -> IterableElementTypeResult<'db> { fn loop_var_from_iterable<'db>( db: &'db dyn Db, iterable_ty: &Type<'db>, @@ -426,10 +427,10 @@ impl<'db> Type<'db> { let dunder_get_item_method = iterable_meta_type.member(db, "__getitem__"); dunder_get_item_method.call(db) } - loop_var_from_iterable(db, self).unwrap_or_else(|| { - diagnostic_fn(); - Type::Unknown - }) + IterableElementTypeResult { + element_ty: loop_var_from_iterable(db, self), + iterable_ty: *self, + } } #[must_use] @@ -471,6 +472,29 @@ impl<'db> Type<'db> { } } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct IterableElementTypeResult<'db> { + element_ty: Option>, + iterable_ty: Type<'db>, +} + +impl<'db> IterableElementTypeResult<'db> { + fn unwrap_with_diagnostic( + self, + iterable_node: AnyNodeRef, + inference_builder: &mut TypeInferenceBuilder<'db>, + ) -> Type<'db> { + let IterableElementTypeResult { + element_ty, + iterable_ty, + } = self; + element_ty.unwrap_or_else(|| { + inference_builder.not_iterable_diagnostic(iterable_node, iterable_ty); + Type::Unknown + }) + } +} + #[salsa::interned] pub struct FunctionType<'db> { /// name of the function at definition diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 13e122d7fbaaee..1253549723a504 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -243,7 +243,7 @@ impl<'db> TypeInference<'db> { /// Similarly, when we encounter a standalone-inferable expression (right-hand side of an /// assignment, type narrowing guard), we use the [`infer_expression_types()`] query to ensure we /// don't infer its types more than once. -struct TypeInferenceBuilder<'db> { +pub(super) struct TypeInferenceBuilder<'db> { db: &'db dyn Db, index: &'db SemanticIndex<'db>, region: InferenceRegion<'db>, @@ -1030,7 +1030,7 @@ impl<'db> TypeInferenceBuilder<'db> { } /// Emit a diagnostic declaring that the object represented by `node` is not iterable - fn not_iterable_diagnostic(&mut self, node: AnyNodeRef, iterable_ty: Type<'db>) { + pub(super) fn not_iterable_diagnostic(&mut self, node: AnyNodeRef, iterable_ty: Type<'db>) { self.add_diagnostic( node, "not-iterable", @@ -1054,9 +1054,9 @@ impl<'db> TypeInferenceBuilder<'db> { .types .expression_ty(iterable.scoped_ast_id(self.db, self.scope)); - let loop_var_value_ty = iterable_ty.iterate(self.db, || { - self.not_iterable_diagnostic(iterable.into(), iterable_ty); - }); + let loop_var_value_ty = iterable_ty + .iterate(self.db) + .unwrap_with_diagnostic(iterable.into(), self); self.types .expressions @@ -1817,9 +1817,9 @@ impl<'db> TypeInferenceBuilder<'db> { } = starred; let iterable_ty = self.infer_expression(value); - iterable_ty.iterate(self.db, || { - self.not_iterable_diagnostic(value.as_ref().into(), iterable_ty); - }); + iterable_ty + .iterate(self.db) + .unwrap_with_diagnostic(value.as_ref().into(), self); // TODO Type::Unknown @@ -1838,9 +1838,9 @@ impl<'db> TypeInferenceBuilder<'db> { let ast::ExprYieldFrom { range: _, value } = yield_from; let iterable_ty = self.infer_expression(value); - iterable_ty.iterate(self.db, || { - self.not_iterable_diagnostic(value.as_ref().into(), iterable_ty); - }); + iterable_ty + .iterate(self.db) + .unwrap_with_diagnostic(value.as_ref().into(), self); // TODO get type from `SendType` of generator/awaitable Type::Unknown From 82b89bbc0c39d3cff73c5f1e3c8d6608ea887187 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Wed, 4 Sep 2024 14:49:57 +0100 Subject: [PATCH 4/5] address review --- crates/red_knot_python_semantic/src/types.rs | 87 ++++++++++--------- .../src/types/infer.rs | 4 +- 2 files changed, 48 insertions(+), 43 deletions(-) diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index 6311242aa22999..c8ddde305139bb 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -401,36 +401,42 @@ impl<'db> Type<'db> { /// for y in x: /// pass /// ``` - /// - /// Emits a diagnostic and returns `Unknown` if `self` represents a type that is not iterable. - fn iterate(&self, db: &'db dyn Db) -> IterableElementTypeResult<'db> { - fn loop_var_from_iterable<'db>( - db: &'db dyn Db, - iterable_ty: &Type<'db>, - ) -> Option> { - // `__iter__` and `__next__` are both looked up on the class of the iterable: - let iterable_meta_type = iterable_ty.to_meta_type(db); - - let dunder_iter_method = iterable_meta_type.member(db, "__iter__"); - if !dunder_iter_method.is_unbound() { - let iterator_ty = dunder_iter_method.call(db)?; - let dunder_next_method = iterator_ty.to_meta_type(db).member(db, "__next__"); - return dunder_next_method.call(db); - } - - // Although it's not considered great practice, - // classes that define `__getitem__` are also iterable, - // even if they do not define `__iter__`. - // - // TODO this is only valid if the `__getitem__` method is annotated as - // accepting `int` or `SupportsIndex` - let dunder_get_item_method = iterable_meta_type.member(db, "__getitem__"); - dunder_get_item_method.call(db) - } - IterableElementTypeResult { - element_ty: loop_var_from_iterable(db, self), - iterable_ty: *self, + fn iterate(&self, db: &'db dyn Db) -> IterationOutcome<'db> { + // `self` represents the type of the iterable; + // `__iter__` and `__next__` are both looked up on the class of the iterable: + let iterable_meta_type = self.to_meta_type(db); + + let dunder_iter_method = iterable_meta_type.member(db, "__iter__"); + if !dunder_iter_method.is_unbound() { + let Some(iterator_ty) = dunder_iter_method.call(db) else { + return IterationOutcome::NotIterable { + not_iterable_ty: *self, + }; + }; + + let dunder_next_method = iterator_ty.to_meta_type(db).member(db, "__next__"); + return dunder_next_method + .call(db) + .map(|element_ty| IterationOutcome::Iterable { element_ty }) + .unwrap_or(IterationOutcome::NotIterable { + not_iterable_ty: *self, + }); } + + // Although it's not considered great practice, + // classes that define `__getitem__` are also iterable, + // even if they do not define `__iter__`. + // + // TODO(Alex) this is only valid if the `__getitem__` method is annotated as + // accepting `int` or `SupportsIndex` + let dunder_get_item_method = iterable_meta_type.member(db, "__getitem__"); + + dunder_get_item_method + .call(db) + .map(|element_ty| IterationOutcome::Iterable { element_ty }) + .unwrap_or(IterationOutcome::NotIterable { + not_iterable_ty: *self, + }) } #[must_use] @@ -473,25 +479,24 @@ impl<'db> Type<'db> { } #[derive(Debug, Clone, Copy, PartialEq, Eq)] -struct IterableElementTypeResult<'db> { - element_ty: Option>, - iterable_ty: Type<'db>, +enum IterationOutcome<'db> { + Iterable { element_ty: Type<'db> }, + NotIterable { not_iterable_ty: Type<'db> }, } -impl<'db> IterableElementTypeResult<'db> { +impl<'db> IterationOutcome<'db> { fn unwrap_with_diagnostic( self, iterable_node: AnyNodeRef, inference_builder: &mut TypeInferenceBuilder<'db>, ) -> Type<'db> { - let IterableElementTypeResult { - element_ty, - iterable_ty, - } = self; - element_ty.unwrap_or_else(|| { - inference_builder.not_iterable_diagnostic(iterable_node, iterable_ty); - Type::Unknown - }) + match self { + Self::Iterable { element_ty } => element_ty, + Self::NotIterable { not_iterable_ty } => { + inference_builder.not_iterable_diagnostic(iterable_node, not_iterable_ty); + Type::Unknown + } + } } } diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 1253549723a504..c323984fa5c162 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -1030,13 +1030,13 @@ impl<'db> TypeInferenceBuilder<'db> { } /// Emit a diagnostic declaring that the object represented by `node` is not iterable - pub(super) fn not_iterable_diagnostic(&mut self, node: AnyNodeRef, iterable_ty: Type<'db>) { + pub(super) fn not_iterable_diagnostic(&mut self, node: AnyNodeRef, not_iterable_ty: Type<'db>) { self.add_diagnostic( node, "not-iterable", format_args!( "Object of type '{}' is not iterable", - iterable_ty.display(self.db) + not_iterable_ty.display(self.db) ), ); } From 53cc0a2f23f334a7753de0be4b9ed9bb6ae94148 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Wed, 4 Sep 2024 15:14:17 +0100 Subject: [PATCH 5/5] cleanups --- crates/red_knot_python_semantic/src/types.rs | 4 ++-- crates/red_knot_python_semantic/src/types/infer.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index c8ddde305139bb..68f665637b869e 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -1,6 +1,6 @@ use infer::TypeInferenceBuilder; use ruff_db::files::File; -use ruff_python_ast::{self as ast, AnyNodeRef}; +use ruff_python_ast as ast; use crate::semantic_index::ast_ids::HasScopedAstId; use crate::semantic_index::definition::{Definition, DefinitionKind}; @@ -487,7 +487,7 @@ enum IterationOutcome<'db> { impl<'db> IterationOutcome<'db> { fn unwrap_with_diagnostic( self, - iterable_node: AnyNodeRef, + iterable_node: ast::AnyNodeRef, inference_builder: &mut TypeInferenceBuilder<'db>, ) -> Type<'db> { match self { diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index c323984fa5c162..472a171579d244 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -1842,7 +1842,7 @@ impl<'db> TypeInferenceBuilder<'db> { .iterate(self.db) .unwrap_with_diagnostic(value.as_ref().into(), self); - // TODO get type from `SendType` of generator/awaitable + // TODO get type from `ReturnType` of generator Type::Unknown }