From 4d3d04ee6111997469354d1ddbd16f16c1b28050 Mon Sep 17 00:00:00 2001 From: Tomer Chachamu Date: Sun, 19 Feb 2023 14:54:43 +0000 Subject: [PATCH] [`PLE0101`] error when `__init__` returns a value (#3007) --- .../test/fixtures/pylint/return_in_init.py | 41 +++++++++++ crates/ruff/src/checkers/ast.rs | 3 + crates/ruff/src/codes.rs | 1 + crates/ruff/src/registry.rs | 1 + crates/ruff/src/rules/pylint/helpers.rs | 38 ++++++++++ crates/ruff/src/rules/pylint/mod.rs | 2 + crates/ruff/src/rules/pylint/rules/mod.rs | 2 + .../src/rules/pylint/rules/return_in_init.rs | 72 +++++++++++++++++++ .../src/rules/pylint/rules/yield_in_init.rs | 45 ++---------- ...int__tests__PLE0101_return_in_init.py.snap | 25 +++++++ ruff.schema.json | 1 + 11 files changed, 192 insertions(+), 39 deletions(-) create mode 100644 crates/ruff/resources/test/fixtures/pylint/return_in_init.py create mode 100644 crates/ruff/src/rules/pylint/helpers.rs create mode 100644 crates/ruff/src/rules/pylint/rules/return_in_init.rs create mode 100644 crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0101_return_in_init.py.snap diff --git a/crates/ruff/resources/test/fixtures/pylint/return_in_init.py b/crates/ruff/resources/test/fixtures/pylint/return_in_init.py new file mode 100644 index 0000000000000..3361604a65382 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/pylint/return_in_init.py @@ -0,0 +1,41 @@ +def a(): + return + +def __init__(): + return + +class A: + def __init__(self): + return + + +class B: + def __init__(self): + return 3 + + def gen(self): + return 5 + +class MyClass: + + def __init__(self): + return 1 + +class MyClass2: + """dummy class""" + + def __init__(self): + return + + +class MyClass3: + """dummy class""" + + def __init__(self): + return None + +class MyClass5: + """dummy class""" + + def __init__(self): + self.callable = lambda: (yield None) diff --git a/crates/ruff/src/checkers/ast.rs b/crates/ruff/src/checkers/ast.rs index 84f2cabd5abf9..0305ad1e49f7f 100644 --- a/crates/ruff/src/checkers/ast.rs +++ b/crates/ruff/src/checkers/ast.rs @@ -779,6 +779,9 @@ where if self.settings.rules.enabled(&Rule::ReturnOutsideFunction) { pyflakes::rules::return_outside_function(self, stmt); } + if self.settings.rules.enabled(&Rule::ReturnInInit) { + pylint::rules::return_in_init(self, stmt); + } } StmtKind::ClassDef { name, diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 5074aafbd1c43..eedaf5cc42e7a 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -123,6 +123,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option { // pylint (Pylint, "E0100") => Rule::YieldInInit, + (Pylint, "E0101") => Rule::ReturnInInit, (Pylint, "E0604") => Rule::InvalidAllObject, (Pylint, "E0605") => Rule::InvalidAllFormat, (Pylint, "E1307") => Rule::BadStringFormatType, diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index 659b05a454e48..99c5ef10f099e 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -137,6 +137,7 @@ ruff_macros::register_rules!( rules::pylint::rules::UsedPriorGlobalDeclaration, rules::pylint::rules::AwaitOutsideAsync, rules::pylint::rules::PropertyWithParameters, + rules::pylint::rules::ReturnInInit, rules::pylint::rules::ConsiderUsingFromImport, rules::pylint::rules::ComparisonOfConstant, rules::pylint::rules::ConsiderMergingIsinstance, diff --git a/crates/ruff/src/rules/pylint/helpers.rs b/crates/ruff/src/rules/pylint/helpers.rs new file mode 100644 index 0000000000000..faa9b07a84f66 --- /dev/null +++ b/crates/ruff/src/rules/pylint/helpers.rs @@ -0,0 +1,38 @@ +use crate::ast::function_type; +use crate::ast::function_type::FunctionType; +use crate::{ + ast::types::{FunctionDef, ScopeKind}, + checkers::ast::Checker, +}; + +pub fn in_dunder_init(checker: &mut Checker) -> bool { + let scope = checker.current_scope(); + let ScopeKind::Function(FunctionDef { + name, + decorator_list, + .. + }) = &scope.kind else { + return false; + }; + if *name != "__init__" { + return false; + } + let Some(parent) = checker.current_scope_parent() else { + return false; + }; + + 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 false; + } + true +} diff --git a/crates/ruff/src/rules/pylint/mod.rs b/crates/ruff/src/rules/pylint/mod.rs index 1970064105faa..bb97a7d37a946 100644 --- a/crates/ruff/src/rules/pylint/mod.rs +++ b/crates/ruff/src/rules/pylint/mod.rs @@ -1,4 +1,5 @@ //! Rules from [Pylint](https://pypi.org/project/pylint/). +mod helpers; pub(crate) mod rules; pub mod settings; @@ -16,6 +17,7 @@ mod tests { use crate::settings::Settings; use crate::test::test_path; + #[test_case(Rule::ReturnInInit, Path::new("return_in_init.py"); "PLE0101")] #[test_case(Rule::UselessImportAlias, Path::new("import_aliasing.py"); "PLC0414")] #[test_case(Rule::UnnecessaryDirectLambdaCall, Path::new("unnecessary_direct_lambda_call.py"); "PLC3002")] #[test_case(Rule::NonlocalWithoutBinding, Path::new("nonlocal_without_binding.py"); "PLE0117")] diff --git a/crates/ruff/src/rules/pylint/rules/mod.rs b/crates/ruff/src/rules/pylint/rules/mod.rs index 494e64dc7461e..e16247fae2625 100644 --- a/crates/ruff/src/rules/pylint/rules/mod.rs +++ b/crates/ruff/src/rules/pylint/rules/mod.rs @@ -11,6 +11,7 @@ pub use magic_value_comparison::{magic_value_comparison, MagicValueComparison}; pub use merge_isinstance::{merge_isinstance, ConsiderMergingIsinstance}; pub use nonlocal_without_binding::NonlocalWithoutBinding; pub use property_with_parameters::{property_with_parameters, PropertyWithParameters}; +pub use return_in_init::{return_in_init, ReturnInInit}; pub use too_many_arguments::{too_many_arguments, TooManyArguments}; pub use too_many_branches::{too_many_branches, TooManyBranches}; pub use too_many_return_statements::{too_many_return_statements, TooManyReturnStatements}; @@ -39,6 +40,7 @@ mod magic_value_comparison; mod merge_isinstance; mod nonlocal_without_binding; mod property_with_parameters; +mod return_in_init; mod too_many_arguments; mod too_many_branches; mod too_many_return_statements; diff --git a/crates/ruff/src/rules/pylint/rules/return_in_init.rs b/crates/ruff/src/rules/pylint/rules/return_in_init.rs new file mode 100644 index 0000000000000..fb8ef24648230 --- /dev/null +++ b/crates/ruff/src/rules/pylint/rules/return_in_init.rs @@ -0,0 +1,72 @@ +use rustpython_parser::ast::{Constant, ExprKind, Stmt, StmtKind}; + +use ruff_macros::{define_violation, derive_message_formats}; + +use crate::ast::types::Range; +use crate::checkers::ast::Checker; +use crate::registry::Diagnostic; +use crate::rules::pylint::helpers::in_dunder_init; +use crate::violation::Violation; + +define_violation!( + /// ## What it does + /// Checks for `__init__` methods that return values. + /// + /// ## Why is this bad? + /// The `__init__` method is the constructor for a given Python class, + /// responsible for initializing, rather than creating, new objects. + /// + /// The `__init__` method has to return `None`. Returning any value from + /// an `__init__` method will result in a runtime error. + /// + /// ## Example + /// ```python + /// class Example: + /// def __init__(self): + /// return [] + /// ``` + /// + /// Use instead: + /// ```python + /// class Example: + /// def __init__(self): + /// self.value = [] + /// ``` + /// + /// ## References + /// * [CodeQL: `py-explicit-return-in-init`](https://codeql.github.com/codeql-query-help/python/py-explicit-return-in-init/) + pub struct ReturnInInit; +); +impl Violation for ReturnInInit { + #[derive_message_formats] + fn message(&self) -> String { + format!("Explicit return in `__init__`") + } +} + +/// PLE0101 +pub fn return_in_init(checker: &mut Checker, stmt: &Stmt) { + if let StmtKind::Return { value } = &stmt.node { + if let Some(expr) = value { + if matches!( + expr.node, + ExprKind::Constant { + value: Constant::None, + .. + } + ) { + // Explicit `return None`. + return; + } + } else { + // Implicit `return`. + return; + } + } + + if in_dunder_init(checker) { + checker + .diagnostics + .push(Diagnostic::new(ReturnInInit, Range::from_located(stmt))); + } +} diff --git a/crates/ruff/src/rules/pylint/rules/yield_in_init.rs b/crates/ruff/src/rules/pylint/rules/yield_in_init.rs index be02f804b3641..6e4bf6a2326bb 100644 --- a/crates/ruff/src/rules/pylint/rules/yield_in_init.rs +++ b/crates/ruff/src/rules/pylint/rules/yield_in_init.rs @@ -2,13 +2,9 @@ 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::rules::pylint::helpers::in_dunder_init; use crate::{ - ast::types::{FunctionDef, Range, ScopeKind}, - checkers::ast::Checker, - registry::Diagnostic, - violation::Violation, + ast::types::Range, checkers::ast::Checker, registry::Diagnostic, violation::Violation, }; define_violation!( @@ -45,38 +41,9 @@ impl Violation for YieldInInit { /// PLE0100 pub fn yield_in_init(checker: &mut Checker, expr: &Expr) { - let scope = checker.current_scope(); - let ScopeKind::Function(FunctionDef { - name, - decorator_list, - .. - }) = &scope.kind else { - return; - }; - - if *name != "__init__" { - return; + if in_dunder_init(checker) { + checker + .diagnostics + .push(Diagnostic::new(YieldInInit, Range::from_located(expr))); } - - let Some(parent) = checker.current_scope_parent() else { - return; - }; - - 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))); } diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0101_return_in_init.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0101_return_in_init.py.snap new file mode 100644 index 0000000000000..ceb9f59957510 --- /dev/null +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0101_return_in_init.py.snap @@ -0,0 +1,25 @@ +--- +source: crates/ruff/src/rules/pylint/mod.rs +expression: diagnostics +--- +- kind: + ReturnInInit: ~ + location: + row: 14 + column: 8 + end_location: + row: 14 + column: 16 + fix: ~ + parent: ~ +- kind: + ReturnInInit: ~ + location: + row: 22 + column: 8 + end_location: + row: 22 + column: 16 + fix: ~ + parent: ~ + diff --git a/ruff.schema.json b/ruff.schema.json index be8624bba31c0..c9acae28af678 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1778,6 +1778,7 @@ "PLE01", "PLE010", "PLE0100", + "PLE0101", "PLE011", "PLE0117", "PLE0118",