diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_length.py b/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_length.py index 67b6f196a6f31..d55f655f04512 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_length.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_length.py @@ -1,34 +1,42 @@ # These testcases should raise errors + class Bool: def __len__(self): return True # [invalid-length-return] + class Float: def __len__(self): return 3.05 # [invalid-length-return] + class Str: def __len__(self): return "ruff" # [invalid-length-return] + class LengthNoReturn: def __len__(self): print("ruff") # [invalid-length-return] + class LengthNegative: def __len__(self): return -42 # [invalid-length-return] + class LengthWrongRaise: def __len__(self): print("raise some error") raise NotImplementedError # [invalid-length-return] + # TODO: Once Ruff has better type checking def return_int(): return "3" + class ComplexReturn: def __len__(self): return return_int() # [invalid-length-return] @@ -36,23 +44,27 @@ def __len__(self): # These testcases should NOT raise errors + class Length: def __len__(self): return 42 + class Length2: def __len__(self): x = 42 return x + class Length3: - def __len__(self): - ... + def __len__(self): ... + class Length4: def __len__(self): pass + class Length5: def __len__(self): - raise NotImplementedError \ No newline at end of file + raise NotImplementedError diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 3dbddd137b981..8e497746f6249 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -98,7 +98,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { pylint::rules::invalid_bool_return(checker, name, body); } if checker.enabled(Rule::InvalidLengthReturnType) { - pylint::rules::invalid_length_return(checker, name, body); + pylint::rules::invalid_length_return(checker, function_def); } if checker.enabled(Rule::InvalidBytesReturnType) { pylint::rules::invalid_bytes_return(checker, function_def); diff --git a/crates/ruff_linter/src/rules/pylint/rules/invalid_length_return.rs b/crates/ruff_linter/src/rules/pylint/rules/invalid_length_return.rs index eb184153a8663..f3ecf1546ed1a 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/invalid_length_return.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_length_return.rs @@ -1,20 +1,27 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::helpers::ReturnStatementVisitor; +use ruff_python_ast::identifier::Identifier; use ruff_python_ast::visitor::Visitor; -use ruff_python_ast::{self as ast, Expr, Stmt}; +use ruff_python_ast::{self as ast, Expr}; +use ruff_python_semantic::analyze::function_type::is_stub; use ruff_python_semantic::analyze::type_inference::{NumberLike, PythonType, ResolvedPythonType}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; /// ## What it does -/// Checks for `__len__` implementations that return other than a non-negative `integer`. +/// Checks for `__len__` implementations that return values other than a non-negative +/// `integer`. /// /// ## Why is this bad? /// The `__len__` method should return a non-negative `integer`. Returning a different /// value may cause unexpected behavior. /// +/// Note: `bool` is a subclass of `int`, so it's technically valid for `__len__` to +/// return `True` or `False`. However, for consistency with other rules, Ruff will +/// still raise when `__len__` returns a `bool`. +/// /// ## Example /// ```python /// class Foo: @@ -29,9 +36,7 @@ use crate::checkers::ast::Checker; /// return 2 /// ``` /// -/// Note: Strictly speaking `bool` is a subclass of `int`, thus returning `True`/`False` is valid. -/// To be consistent with other rules (e.g. PLE0305 invalid-index-returned), ruff will raise, compared -/// to pylint which will not raise. +/// /// ## References /// - [Python documentation: The `__len__` method](https://docs.python.org/3/reference/datamodel.html#object.__len__) #[violation] @@ -40,13 +45,13 @@ pub struct InvalidLengthReturnType; impl Violation for InvalidLengthReturnType { #[derive_message_formats] fn message(&self) -> String { - format!("`__len__` does not return a non-negative `integer`") + format!("`__len__` does not return a non-negative integer") } } /// E0303 -pub(crate) fn invalid_length_return(checker: &mut Checker, name: &str, body: &[Stmt]) { - if name != "__len__" { +pub(crate) fn invalid_length_return(checker: &mut Checker, function_def: &ast::StmtFunctionDef) { + if function_def.name.as_str() != "__len__" { return; } @@ -54,35 +59,20 @@ pub(crate) fn invalid_length_return(checker: &mut Checker, name: &str, body: &[S return; } - if body.len() == 1 - && (matches!(&body[0], Stmt::Expr(expr) if expr.value.is_ellipsis_literal_expr()) - || body[0].is_pass_stmt() - || body[0].is_raise_stmt()) - { - return; - } - - let body_without_comments = body - .iter() - .filter(|stmt| !matches!(stmt, Stmt::Expr(expr) if expr.value.is_string_literal_expr())) - .collect::>(); - if body_without_comments.is_empty() { - return; - } - if body_without_comments.len() == 1 && body_without_comments[0].is_raise_stmt() { + if is_stub(function_def, checker.semantic()) { return; } let returns = { let mut visitor = ReturnStatementVisitor::default(); - visitor.visit_body(body); + visitor.visit_body(&function_def.body); visitor.returns }; if returns.is_empty() { checker.diagnostics.push(Diagnostic::new( InvalidLengthReturnType, - body.last().unwrap().range(), + function_def.identifier(), )); } @@ -108,6 +98,7 @@ pub(crate) fn invalid_length_return(checker: &mut Checker, name: &str, body: &[S } } +/// Returns `true` if the given expression is a negative integer. fn is_negative_integer(value: &Expr) -> bool { matches!( value, diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0303_invalid_return_type_length.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0303_invalid_return_type_length.py.snap index 87d34da877b89..75c33552b1f85 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0303_invalid_return_type_length.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0303_invalid_return_type_length.py.snap @@ -1,62 +1,51 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -invalid_return_type_length.py:5:16: PLE0303 `__len__` does not return a non-negative `integer` +invalid_return_type_length.py:6:16: PLE0303 `__len__` does not return a non-negative integer | -3 | class Bool: -4 | def __len__(self): -5 | return True # [invalid-length-return] +4 | class Bool: +5 | def __len__(self): +6 | return True # [invalid-length-return] | ^^^^ PLE0303 -6 | -7 | class Float: | -invalid_return_type_length.py:9:16: PLE0303 `__len__` does not return a non-negative `integer` +invalid_return_type_length.py:11:16: PLE0303 `__len__` does not return a non-negative integer | - 7 | class Float: - 8 | def __len__(self): - 9 | return 3.05 # [invalid-length-return] + 9 | class Float: +10 | def __len__(self): +11 | return 3.05 # [invalid-length-return] | ^^^^ PLE0303 -10 | -11 | class Str: | -invalid_return_type_length.py:13:16: PLE0303 `__len__` does not return a non-negative `integer` +invalid_return_type_length.py:16:16: PLE0303 `__len__` does not return a non-negative integer | -11 | class Str: -12 | def __len__(self): -13 | return "ruff" # [invalid-length-return] +14 | class Str: +15 | def __len__(self): +16 | return "ruff" # [invalid-length-return] | ^^^^^^ PLE0303 -14 | -15 | class LengthNoReturn: | -invalid_return_type_length.py:17:9: PLE0303 `__len__` does not return a non-negative `integer` +invalid_return_type_length.py:20:9: PLE0303 `__len__` does not return a non-negative integer | -15 | class LengthNoReturn: -16 | def __len__(self): -17 | print("ruff") # [invalid-length-return] - | ^^^^^^^^^^^^^ PLE0303 -18 | -19 | class LengthNegative: +19 | class LengthNoReturn: +20 | def __len__(self): + | ^^^^^^^ PLE0303 +21 | print("ruff") # [invalid-length-return] | -invalid_return_type_length.py:21:16: PLE0303 `__len__` does not return a non-negative `integer` +invalid_return_type_length.py:26:16: PLE0303 `__len__` does not return a non-negative integer | -19 | class LengthNegative: -20 | def __len__(self): -21 | return -42 # [invalid-length-return] +24 | class LengthNegative: +25 | def __len__(self): +26 | return -42 # [invalid-length-return] | ^^^ PLE0303 -22 | -23 | class LengthWrongRaise: | -invalid_return_type_length.py:26:9: PLE0303 `__len__` does not return a non-negative `integer` - | -24 | def __len__(self): -25 | print("raise some error") -26 | raise NotImplementedError # [invalid-length-return] - | ^^^^^^^^^^^^^^^^^^^^^^^^^ PLE0303 -27 | -28 | # TODO: Once Ruff has better type checking +invalid_return_type_length.py:30:9: PLE0303 `__len__` does not return a non-negative integer + | +29 | class LengthWrongRaise: +30 | def __len__(self): + | ^^^^^^^ PLE0303 +31 | print("raise some error") +32 | raise NotImplementedError # [invalid-length-return] |