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

Use function_type::classify for yield-in-init #2742

Merged
merged 1 commit into from
Feb 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -1360,7 +1360,7 @@ For more, see [Pylint](https://pypi.org/project/pylint/) on PyPI.

| Code | Name | Message | Fix |
| ---- | ---- | ------- | --- |
| PLE0100 | yield-in-init | `__init__` method is a generator | |
| PLE0100 | [yield-in-init](https://github.com/charliermarsh/ruff/blob/main/docs/rules/yield-in-init.md) | `__init__` method is a generator | |
| PLE0117 | nonlocal-without-binding | Nonlocal name `{name}` found without binding | |
| PLE0118 | used-prior-global-declaration | Name `{name}` is used prior to global declaration on line {line} | |
| PLE0604 | invalid-all-object | Invalid object in `__all__`, must contain only strings | |
Expand Down
8 changes: 8 additions & 0 deletions crates/ruff/src/checkers/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3884,6 +3884,14 @@ impl<'a> Checker<'a> {
&self.scopes[*(self.scope_stack.last().expect("No current scope found"))]
}

pub fn current_scope_parent(&self) -> Option<&Scope> {
self.scope_stack
.iter()
.rev()
.nth(1)
.map(|index| &self.scopes[*index])
}

pub fn current_scopes(&self) -> impl Iterator<Item = &Scope> {
self.scope_stack
.iter()
Expand Down
65 changes: 48 additions & 17 deletions crates/ruff/src/rules/pylint/rules/yield_in_init.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
use ruff_macros::{define_violation, derive_message_formats};
use rustpython_parser::ast::Expr;

use ruff_macros::{define_violation, derive_message_formats};

use crate::ast::function_type;
use crate::ast::function_type::FunctionType;
use crate::{
ast::types::{FunctionDef, Range, ScopeKind},
checkers::ast::Checker,
Expand All @@ -9,6 +12,19 @@ use crate::{
};

define_violation!(
/// ### What it does
/// Checks for `__init__.py` methods that turned into generators
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's __init__ methods ... not __init__.py methods (you made the mistake twice).

/// via the presence of `yield` or `yield from` statements.
///
/// ### Why is this bad?
/// Generators are not allowed in `__init__.py` methods.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even without that mistake I find that description to be very bad.

Firstly it is misleading because generators can be used in __init__ methods ... it's just that the __init__ method cannot be a generator.

Secondly "is not allowed" is just completely nondescript ... what does that mean? Not allowed by whom?

I think GitHub's CodeQL documentation does a better job:

The __init__ method of a class is used to initialize new objects, not create them. As such, it should not return any value. By including a yield expression in the method turns it into a generator method. On calling it will return a generator resulting in a runtime error.

The last sentence here is the important part.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I'll fix it up! Though, in my opinion, calling something "very bad" is not a productive way to deliver feedback and is a good way to turn a discussion into an argument.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation is written in a voice of authority ... when we use such a voice I think we should take great care that what we are writing is correct, clear and informative because otherwise we are very much bound to cause confusion or waste time of users who take our documentation at face value.

I think accurately and clearly answering Why is this bad? can be quite work-intensive for many of our lints. I'd still like us to put in that work rather than adding vague descriptions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree! Nowhere did I disagree that the documentation here could be improved.

Copy link
Contributor

@not-my-profile not-my-profile Feb 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if it's ok for us to copy the documentationfrom GitHub CodeQL in verbatim ... while you did add a reference in e5f5142 it's not clear that the section is from there. I guess this could be clarified with a Markdown footnote ... but I guess it also raises some licensing questions ... is the licensing of our documentation more lax than of our code?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought a reference was sufficient but I don't want there to be any ambiguity so I'll just rewrite it entirely.

///
/// ### Example
/// ```python
/// class Foo:
/// def __init__(self):
/// yield 1
/// ```
pub struct YieldInInit;
);

Expand All @@ -21,23 +37,38 @@ impl Violation for YieldInInit {

/// PLE0100
pub fn yield_in_init(checker: &mut Checker, expr: &Expr) {
let parent_scope_is_class: Option<bool> =
checker
.current_scopes()
.nth(1)
.and_then(|scope| match scope.kind {
ScopeKind::Class(..) => Some(true),
_ => None,
});

let current_scope_is_init = match checker.current_scope().kind {
ScopeKind::Function(FunctionDef { name, .. }) => Some(name == "__init__"),
_ => None,
let scope = checker.current_scope();
let ScopeKind::Function(FunctionDef {
name,
decorator_list,
..
}) = &scope.kind else {
return;
};

if *name != "__init__" {
return;
}

let Some(parent) = checker.current_scope_parent() else {
return;
};

if parent_scope_is_class == Some(true) && current_scope_is_init == Some(true) {
checker
.diagnostics
.push(Diagnostic::new(YieldInInit, Range::from_located(expr)));
if !matches!(
function_type::classify(
checker,
parent,
name,
decorator_list,
&checker.settings.pep8_naming.classmethod_decorators,
&checker.settings.pep8_naming.staticmethod_decorators,
),
FunctionType::Method
) {
return;
}

checker
.diagnostics
.push(Diagnostic::new(YieldInInit, Range::from_located(expr)));
}
17 changes: 17 additions & 0 deletions docs/rules/yield-in-init.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# yield-in-init (PLE0100)

Derived from the **Pylint** linter.

### What it does
Checks for `__init__.py` methods that turned into generators
via the presence of `yield` or `yield from` statements.

### Why is this bad?
Generators are not allowed in `__init__.py` methods.

### Example
```python
class Foo:
def __init__(self):
yield 1
```