Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flake8-pyi] Implement PYI045 #4700

Merged
merged 5 commits into from
May 29, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Include async
  • Loading branch information
charliermarsh committed May 29, 2023
commit 190f98f7dd09c4612b3df7be8601638ed5b9d492
12 changes: 10 additions & 2 deletions crates/ruff/resources/test/fixtures/flake8_pyi/PYI045.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
import typing
from collections.abc import Iterator, Iterable

# All OK for PYI045 because this isn't a stub.


class NoReturn:
def __iter__(self):
Expand Down Expand Up @@ -75,3 +73,13 @@ def __iter__(self) -> collections.abc.Iterator:
class CollectionsIteratorTReturn:
def __iter__(self) -> collections.abc.Iterator[int]:
...


class TypingAsyncIterableTReturn:
def __aiter__(self) -> typing.AsyncIterable[int]:
...


class TypingAsyncIterableReturn:
def __aiter__(self) -> typing.AsyncIterable:
...
6 changes: 6 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_pyi/PYI045.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,9 @@ class CollectionsIteratorReturn:

class CollectionsIteratorTReturn:
def __iter__(self) -> collections.abc.Iterator[int]: ...

class TypingAsyncIterableTReturn:
def __aiter__(self) -> typing.AsyncIterable[int]: ... # Error: PYI045

class TypingAsyncIterableReturn:
def __aiter__(self) -> typing.AsyncIterable: ... # Error: PYI045
102 changes: 69 additions & 33 deletions crates/ruff/src/rules/flake8_pyi/rules/iter_method_return_iterable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,80 +9,116 @@ use ruff_python_semantic::definition::{Definition, Member, MemberKind};
use crate::checkers::ast::Checker;

/// ## What it does
/// Checks for `__iter__` methods in stubs with the wrong return type annotation.
/// Checks for `__iter__` methods in stubs that return `Iterable[T]` instead
/// of an `Iterator[T]`.
///
/// ## Why is this bad?
/// `__iter__` should return an `Iterator`, not an `Iterable`.
/// `__iter__` methods should always should return an `Iterator` of some kind,
/// not an `Iterable`.
///
/// In Python, an `Iterator` is an object that has a `__next__` method, which
/// provides a consistent interface for sequentially processing elements from
/// a sequence or other iterable object. Meanwhile, an `Iterable` is an object
/// with an `__iter__` method, which itself returns an `Iterator`.
///
/// Every `Iterator` is an `Iterable`, but not every `Iterable` is an `Iterator`.
/// By returning an `Iterable` from `__iter__`, you may end up returning an
/// object that doesn't implement `__next__`, which will cause a `TypeError`
/// at runtime. For example, returning a `list` from `__iter__` will cause
/// a `TypeError` when you call `__next__` on it, as a `list` is an `Iterable`,
/// but not an `Iterator`.
///
/// ## Example
/// ```python
/// class Foo:
/// def __iter__(self) -> collections.abc.Iterable:
/// import collections.abc
///
///
/// class Class:
/// def __iter__(self) -> collections.abc.Iterable[str]:
/// ...
/// ```
///
/// Use instead:
/// ```python
/// class Foo:
/// def __iter__(self) -> collections.abc.Iterator:
/// import collections.abc
///
/// class Class:
/// def __iter__(self) -> collections.abc.Iterator[str]:
/// ...
/// ```
#[violation]
pub struct IterMethodReturnIterable;
pub struct IterMethodReturnIterable {
async_: bool,
}

impl Violation for IterMethodReturnIterable {
#[derive_message_formats]
fn message(&self) -> String {
format!("__iter__ methods should never return ` Iterable[T]`, as they should always return some kind of `Iterator`.")
let IterMethodReturnIterable { async_ } = self;
if *async_ {
format!("`__aiter__` methods should return an `AsyncIterator`, not an `AsyncIterable`")
} else {
format!("`__iter__` methods should return an `Iterator`, not an `Iterable`")
}
}
}

/// PYI045
pub(crate) fn iter_method_return_iterable(checker: &mut Checker, definition: &Definition) {
let Definition::Member(Member {
kind: MemberKind::Method,
stmt,
..
}) = definition else {
kind: MemberKind::Method,
stmt,
..
}) = definition else {
return;
};

let Stmt::FunctionDef(ast::StmtFunctionDef {
name,
returns,
..
}) = stmt else {
name,
returns,
..
}) = stmt else {
return;
};

if name != "__iter__" {
return;
}

let Some(returns) = returns else {
return;
};

let annotation = match returns.as_ref() {
// e.g., Iterable[T]
Expr::Subscript(ast::ExprSubscript { value, .. }) => value.as_ref(),
// e.g., typing.Iterable, Iterable
ann_expr @ (Expr::Name(_) | Expr::Attribute(_)) => ann_expr,
let annotation = if let Expr::Subscript(ast::ExprSubscript { value, .. }) = returns.as_ref() {
// Ex) `Iterable[T]`
value
} else {
// Ex) `Iterable`, `typing.Iterable`
returns
};

let async_ = match name.as_str() {
"__iter__" => false,
"__aiter__" => true,
_ => return,
};

if checker
.semantic_model()
.resolve_call_path(annotation)
.map_or(false, |cp| {
matches!(
cp.as_slice(),
&["typing", "Iterable"] | &["collections", "abc", "Iterable"]
)
.map_or(false, |call_path| {
if async_ {
matches!(
call_path.as_slice(),
["typing", "AsyncIterable"] | ["collections", "abc", "AsyncIterable"]
)
} else {
matches!(
call_path.as_slice(),
["typing", "Iterable"] | ["collections", "abc", "Iterable"]
)
}
})
{
checker
.diagnostics
.push(Diagnostic::new(IterMethodReturnIterable, returns.range()));
checker.diagnostics.push(Diagnostic::new(
IterMethodReturnIterable { async_ },
returns.range(),
));
}
}
Original file line number Diff line number Diff line change
@@ -1,39 +1,39 @@
---
source: crates/ruff/src/rules/flake8_pyi/mod.rs
---
PYI045.pyi:9:27: PYI045 __iter__ methods should never return ` Iterable[T]`, as they should always return some kind of `Iterator`.
PYI045.pyi:9:27: PYI045 `__iter__` methods should return an `Iterator`, not an `Iterable`
|
9 | class TypingIterableTReturn:
10 | def __iter__(self) -> typing.Iterable[int]: ... # Error: PYI045
| ^^^^^^^^^^^^^^^^^^^^ PYI045
11 | def not_iter(self) -> typing.Iterable[int]: ...
|

PYI045.pyi:13:27: PYI045 __iter__ methods should never return ` Iterable[T]`, as they should always return some kind of `Iterator`.
PYI045.pyi:13:27: PYI045 `__iter__` methods should return an `Iterator`, not an `Iterable`
|
13 | class TypingIterableReturn:
14 | def __iter__(self) -> typing.Iterable: ... # Error: PYI045
| ^^^^^^^^^^^^^^^ PYI045
15 | def not_iter(self) -> typing.Iterable: ...
|

PYI045.pyi:17:27: PYI045 __iter__ methods should never return ` Iterable[T]`, as they should always return some kind of `Iterator`.
PYI045.pyi:17:27: PYI045 `__iter__` methods should return an `Iterator`, not an `Iterable`
|
17 | class CollectionsIterableTReturn:
18 | def __iter__(self) -> collections.abc.Iterable[int]: ... # Error: PYI045
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI045
19 | def not_iter(self) -> collections.abc.Iterable[int]: ...
|

PYI045.pyi:21:27: PYI045 __iter__ methods should never return ` Iterable[T]`, as they should always return some kind of `Iterator`.
PYI045.pyi:21:27: PYI045 `__iter__` methods should return an `Iterator`, not an `Iterable`
|
21 | class CollectionsIterableReturn:
22 | def __iter__(self) -> collections.abc.Iterable: ... # Error: PYI045
| ^^^^^^^^^^^^^^^^^^^^^^^^ PYI045
23 | def not_iter(self) -> collections.abc.Iterable: ...
|

PYI045.pyi:25:27: PYI045 __iter__ methods should never return ` Iterable[T]`, as they should always return some kind of `Iterator`.
PYI045.pyi:25:27: PYI045 `__iter__` methods should return an `Iterator`, not an `Iterable`
|
25 | class IterableReturn:
26 | def __iter__(self) -> Iterable: ... # Error: PYI045
Expand All @@ -42,4 +42,20 @@ PYI045.pyi:25:27: PYI045 __iter__ methods should never return ` Iterable[T]`, as
28 | class IteratorReturn:
|

PYI045.pyi:46:28: PYI045 `__aiter__` methods should return an `AsyncIterator`, not an `AsyncIterable`
|
46 | class TypingAsyncIterableTReturn:
47 | def __aiter__(self) -> typing.AsyncIterable[int]: ... # Error: PYI045
| ^^^^^^^^^^^^^^^^^^^^^^^^^ PYI045
48 |
49 | class TypingAsyncIterableReturn:
|

PYI045.pyi:49:28: PYI045 `__aiter__` methods should return an `AsyncIterator`, not an `AsyncIterable`
|
49 | class TypingAsyncIterableReturn:
50 | def __aiter__(self) -> typing.AsyncIterable: ... # Error: PYI045
| ^^^^^^^^^^^^^^^^^^^^ PYI045
|


Loading