From 70b707b0a8349a029ef4140ffe9ff9bb284f9049 Mon Sep 17 00:00:00 2001 From: boolean-light Date: Wed, 13 Mar 2024 14:02:59 +0900 Subject: [PATCH 1/4] implement E304 for pylint invalid bool return type --- .../pylint/invalid_return_type_bool.py | 21 +++++++ .../src/checkers/ast/analyze/statement.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/pylint/mod.rs | 1 + .../rules/pylint/rules/invalid_bool_return.rs | 60 +++++++++++++++++++ .../ruff_linter/src/rules/pylint/rules/mod.rs | 2 + ...__PLE0304_invalid_return_type_bool.py.snap | 20 +++++++ ruff.schema.json | 1 + 8 files changed, 109 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_bool.py create mode 100644 crates/ruff_linter/src/rules/pylint/rules/invalid_bool_return.rs create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0304_invalid_return_type_bool.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_bool.py b/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_bool.py new file mode 100644 index 0000000000000..564080b831543 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_bool.py @@ -0,0 +1,21 @@ +class Float: + def __bool__(self): + return 3.05 # [invalid-bool-return] + +class Int: + def __bool__(self): + return 0 # [invalid-bool-return] + + +class Str: + def __bool__(self): + x = "ruff" + return x # [invalid-bool-return] + +# TODO fixme once Ruff has better type checking +def return_int(): + return 3 + +class ComplexReturn: + def __bool__(self): + return return_int() # [invalid-bool-return] \ No newline at end of file diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index ea1335bbc0cd1..191456bf0766b 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -91,6 +91,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { checker.diagnostics.push(diagnostic); } } + if checker.enabled(Rule::InvalidBoolReturnType) { + pylint::rules::invalid_bool_return(checker, name, body); + } if checker.enabled(Rule::InvalidStrReturnType) { pylint::rules::invalid_str_return(checker, name, body); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index ef05dcb053128..c471c890a0f1a 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -240,6 +240,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "E0237") => (RuleGroup::Stable, rules::pylint::rules::NonSlotAssignment), (Pylint, "E0241") => (RuleGroup::Stable, rules::pylint::rules::DuplicateBases), (Pylint, "E0302") => (RuleGroup::Stable, rules::pylint::rules::UnexpectedSpecialMethodSignature), + (Pylint, "E0304") => (RuleGroup::Preview, rules::pylint::rules::InvalidBoolReturnType), (Pylint, "E0307") => (RuleGroup::Stable, rules::pylint::rules::InvalidStrReturnType), (Pylint, "E0604") => (RuleGroup::Stable, rules::pylint::rules::InvalidAllObject), (Pylint, "E0605") => (RuleGroup::Stable, rules::pylint::rules::InvalidAllFormat), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index e03e9303b3f26..0cbd284ddbc8f 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -71,6 +71,7 @@ mod tests { #[test_case(Rule::ImportSelf, Path::new("import_self/module.py"))] #[test_case(Rule::InvalidAllFormat, Path::new("invalid_all_format.py"))] #[test_case(Rule::InvalidAllObject, Path::new("invalid_all_object.py"))] + #[test_case(Rule::InvalidBoolReturnType, Path::new("invalid_return_type_bool.py"))] #[test_case(Rule::InvalidStrReturnType, Path::new("invalid_return_type_str.py"))] #[test_case(Rule::DuplicateBases, Path::new("duplicate_bases.py"))] #[test_case(Rule::InvalidCharacterBackspace, Path::new("invalid_characters.py"))] diff --git a/crates/ruff_linter/src/rules/pylint/rules/invalid_bool_return.rs b/crates/ruff_linter/src/rules/pylint/rules/invalid_bool_return.rs new file mode 100644 index 0000000000000..f67054fe448fd --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_bool_return.rs @@ -0,0 +1,60 @@ +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::helpers::ReturnStatementVisitor; +use ruff_python_ast::visitor::Visitor; +use ruff_python_ast::Stmt; +use ruff_python_semantic::analyze::type_inference::{PythonType, NumberLike, ResolvedPythonType}; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for `__bool__` implementations that return a type other than `bool`. +/// +/// ## Why is this bad? +/// The `__bool__` method should return a `bool` object. Returning a different +/// type may cause unexpected behavior. +#[violation] +pub struct InvalidBoolReturnType; + +impl Violation for InvalidBoolReturnType { + #[derive_message_formats] + fn message(&self) -> String { + format!("`__bool__` does not return `bool`") + } +} + +/// E0307 +pub(crate) fn invalid_bool_return(checker: &mut Checker, name: &str, body: &[Stmt]) { + if name != "__bool__" { + return; + } + + if !checker.semantic().current_scope().kind.is_class() { + return; + } + + let returns = { + let mut visitor = ReturnStatementVisitor::default(); + visitor.visit_body(body); + visitor.returns + }; + + for stmt in returns { + if let Some(value) = stmt.value.as_deref() { + if !matches!( + ResolvedPythonType::from(value), + ResolvedPythonType::Unknown | ResolvedPythonType::Atom(PythonType::Number(NumberLike::Bool)) + ) { + checker + .diagnostics + .push(Diagnostic::new(InvalidBoolReturnType, value.range())); + } + } else { + // Disallow implicit `None`. + checker + .diagnostics + .push(Diagnostic::new(InvalidBoolReturnType, stmt.range())); + } + } +} diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 34289564eed02..8f6d40554d2ad 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -25,6 +25,7 @@ pub(crate) use import_private_name::*; pub(crate) use import_self::*; pub(crate) use invalid_all_format::*; pub(crate) use invalid_all_object::*; +pub(crate) use invalid_bool_return::*; pub(crate) use invalid_envvar_default::*; pub(crate) use invalid_envvar_value::*; pub(crate) use invalid_str_return::*; @@ -113,6 +114,7 @@ mod import_private_name; mod import_self; mod invalid_all_format; mod invalid_all_object; +mod invalid_bool_return; mod invalid_envvar_default; mod invalid_envvar_value; mod invalid_str_return; diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0304_invalid_return_type_bool.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0304_invalid_return_type_bool.py.snap new file mode 100644 index 0000000000000..bdad758e0eca2 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0304_invalid_return_type_bool.py.snap @@ -0,0 +1,20 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +invalid_return_type_bool.py:3:16: PLE0304 `__bool__` does not return `bool` + | +1 | class Float: +2 | def __bool__(self): +3 | return 3.05 # [invalid-bool-return] + | ^^^^ PLE0304 +4 | +5 | class Int: + | + +invalid_return_type_bool.py:7:16: PLE0304 `__bool__` does not return `bool` + | +5 | class Int: +6 | def __bool__(self): +7 | return 0 # [invalid-bool-return] + | ^ PLE0304 + | diff --git a/ruff.schema.json b/ruff.schema.json index 5d44debca2f2f..d0f0f9a6394e0 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3219,6 +3219,7 @@ "PLE03", "PLE030", "PLE0302", + "PLE0304", "PLE0307", "PLE06", "PLE060", From f35e34e3a8dfb75a71a241ba4b8043cd90f8ee41 Mon Sep 17 00:00:00 2001 From: boolean-light Date: Wed, 13 Mar 2024 14:32:46 +0900 Subject: [PATCH 2/4] Code formatting --- .../src/rules/pylint/rules/invalid_bool_return.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/invalid_bool_return.rs b/crates/ruff_linter/src/rules/pylint/rules/invalid_bool_return.rs index f67054fe448fd..35f410ee75345 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/invalid_bool_return.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_bool_return.rs @@ -3,7 +3,7 @@ use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::helpers::ReturnStatementVisitor; use ruff_python_ast::visitor::Visitor; use ruff_python_ast::Stmt; -use ruff_python_semantic::analyze::type_inference::{PythonType, NumberLike, ResolvedPythonType}; +use ruff_python_semantic::analyze::type_inference::{NumberLike, PythonType, ResolvedPythonType}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -44,7 +44,8 @@ pub(crate) fn invalid_bool_return(checker: &mut Checker, name: &str, body: &[Stm if let Some(value) = stmt.value.as_deref() { if !matches!( ResolvedPythonType::from(value), - ResolvedPythonType::Unknown | ResolvedPythonType::Atom(PythonType::Number(NumberLike::Bool)) + ResolvedPythonType::Unknown + | ResolvedPythonType::Atom(PythonType::Number(NumberLike::Bool)) ) { checker .diagnostics From 6d6885b776e5a7f67aa2ef23b9d4e0e2f47c0d63 Mon Sep 17 00:00:00 2001 From: boolean-light Date: Wed, 13 Mar 2024 22:19:25 +0900 Subject: [PATCH 3/4] Extended documentation and testcase --- .../pylint/invalid_return_type_bool.py | 20 +++++++++++++++++-- .../rules/pylint/rules/invalid_bool_return.rs | 17 ++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_bool.py b/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_bool.py index 564080b831543..31ab90b738a2e 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_bool.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_bool.py @@ -1,3 +1,5 @@ +# These testcases should raise errors + class Float: def __bool__(self): return 3.05 # [invalid-bool-return] @@ -12,10 +14,24 @@ def __bool__(self): x = "ruff" return x # [invalid-bool-return] -# TODO fixme once Ruff has better type checking +# TODO: Once Ruff has better type checking def return_int(): return 3 class ComplexReturn: def __bool__(self): - return return_int() # [invalid-bool-return] \ No newline at end of file + return return_int() # [invalid-bool-return] + + + +# These testcases should NOT raise errors + +class Bool: + def __bool__(self): + return True + + +class Bool2: + def __bool__(self): + x = True + return x \ No newline at end of file diff --git a/crates/ruff_linter/src/rules/pylint/rules/invalid_bool_return.rs b/crates/ruff_linter/src/rules/pylint/rules/invalid_bool_return.rs index 35f410ee75345..d677b86e51832 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/invalid_bool_return.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_bool_return.rs @@ -14,6 +14,23 @@ use crate::checkers::ast::Checker; /// ## Why is this bad? /// The `__bool__` method should return a `bool` object. Returning a different /// type may cause unexpected behavior. +/// +/// ## Example +/// ```python +/// class Foo: +/// def __bool__(self): +/// return 2 +/// ``` +/// +/// Use instead: +/// ```python +/// class Foo: +/// def __bool__(self): +/// return True +/// ``` +/// +/// ## References +/// - [Python documentation: The `__bool__` method](https://docs.python.org/3/reference/datamodel.html#object.__bool__) #[violation] pub struct InvalidBoolReturnType; From c0fe3af5b52a9abfce9fd95a5c7599db80492ac4 Mon Sep 17 00:00:00 2001 From: boolean-light Date: Wed, 13 Mar 2024 22:24:30 +0900 Subject: [PATCH 4/4] Updating snapshots --- ...__PLE0304_invalid_return_type_bool.py.snap | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0304_invalid_return_type_bool.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0304_invalid_return_type_bool.py.snap index bdad758e0eca2..b28107c0851d8 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0304_invalid_return_type_bool.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0304_invalid_return_type_bool.py.snap @@ -1,20 +1,20 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -invalid_return_type_bool.py:3:16: PLE0304 `__bool__` does not return `bool` +invalid_return_type_bool.py:5:16: PLE0304 `__bool__` does not return `bool` | -1 | class Float: -2 | def __bool__(self): -3 | return 3.05 # [invalid-bool-return] +3 | class Float: +4 | def __bool__(self): +5 | return 3.05 # [invalid-bool-return] | ^^^^ PLE0304 -4 | -5 | class Int: +6 | +7 | class Int: | -invalid_return_type_bool.py:7:16: PLE0304 `__bool__` does not return `bool` +invalid_return_type_bool.py:9:16: PLE0304 `__bool__` does not return `bool` | -5 | class Int: -6 | def __bool__(self): -7 | return 0 # [invalid-bool-return] +7 | class Int: +8 | def __bool__(self): +9 | return 0 # [invalid-bool-return] | ^ PLE0304 |