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 new file mode 100644 index 0000000000000..d55f655f04512 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_length.py @@ -0,0 +1,70 @@ +# 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] + + +# 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): ... + + +class Length4: + def __len__(self): + pass + + +class Length5: + def __len__(self): + 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 712296030ab6f..6f314ec5b1437 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -97,6 +97,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::InvalidBoolReturnType) { pylint::rules::invalid_bool_return(checker, name, body); } + if checker.enabled(Rule::InvalidLengthReturnType) { + 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/codes.rs b/crates/ruff_linter/src/codes.rs index 780abad541c85..36ee42e65b37b 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -241,6 +241,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, "E0303") => (RuleGroup::Preview, rules::pylint::rules::InvalidLengthReturnType), (Pylint, "E0304") => (RuleGroup::Preview, rules::pylint::rules::InvalidBoolReturnType), (Pylint, "E0307") => (RuleGroup::Stable, rules::pylint::rules::InvalidStrReturnType), (Pylint, "E0308") => (RuleGroup::Preview, rules::pylint::rules::InvalidBytesReturnType), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 2ee4e43b8f8fb..c6c16ae36ea74 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -77,6 +77,10 @@ mod tests { #[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::InvalidLengthReturnType, + Path::new("invalid_return_type_length.py") + )] #[test_case( Rule::InvalidBytesReturnType, Path::new("invalid_return_type_bytes.py") 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 new file mode 100644 index 0000000000000..f3ecf1546ed1a --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_length_return.rs @@ -0,0 +1,110 @@ +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}; +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 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: +/// def __len__(self): +/// return "2" +/// ``` +/// +/// Use instead: +/// ```python +/// class Foo: +/// def __len__(self): +/// return 2 +/// ``` +/// +/// +/// ## References +/// - [Python documentation: The `__len__` method](https://docs.python.org/3/reference/datamodel.html#object.__len__) +#[violation] +pub struct InvalidLengthReturnType; + +impl Violation for InvalidLengthReturnType { + #[derive_message_formats] + fn message(&self) -> String { + format!("`__len__` does not return a non-negative integer") + } +} + +/// E0303 +pub(crate) fn invalid_length_return(checker: &mut Checker, function_def: &ast::StmtFunctionDef) { + if function_def.name.as_str() != "__len__" { + return; + } + + if !checker.semantic().current_scope().kind.is_class() { + return; + } + + if is_stub(function_def, checker.semantic()) { + return; + } + + let returns = { + let mut visitor = ReturnStatementVisitor::default(); + visitor.visit_body(&function_def.body); + visitor.returns + }; + + if returns.is_empty() { + checker.diagnostics.push(Diagnostic::new( + InvalidLengthReturnType, + function_def.identifier(), + )); + } + + for stmt in returns { + if let Some(value) = stmt.value.as_deref() { + if is_negative_integer(value) + || !matches!( + ResolvedPythonType::from(value), + ResolvedPythonType::Unknown + | ResolvedPythonType::Atom(PythonType::Number(NumberLike::Integer)) + ) + { + checker + .diagnostics + .push(Diagnostic::new(InvalidLengthReturnType, value.range())); + } + } else { + // Disallow implicit `None`. + checker + .diagnostics + .push(Diagnostic::new(InvalidLengthReturnType, stmt.range())); + } + } +} + +/// Returns `true` if the given expression is a negative integer. +fn is_negative_integer(value: &Expr) -> bool { + matches!( + value, + Expr::UnaryOp(ast::ExprUnaryOp { + op: ast::UnaryOp::USub, + .. + }) + ) +} diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 77c4da5c69664..e0c51c55b758d 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -31,6 +31,7 @@ pub(crate) use invalid_bool_return::*; pub(crate) use invalid_bytes_return::*; pub(crate) use invalid_envvar_default::*; pub(crate) use invalid_envvar_value::*; +pub(crate) use invalid_length_return::*; pub(crate) use invalid_str_return::*; pub(crate) use invalid_string_characters::*; pub(crate) use iteration_over_set::*; @@ -130,6 +131,7 @@ mod invalid_bool_return; mod invalid_bytes_return; mod invalid_envvar_default; mod invalid_envvar_value; +mod invalid_length_return; mod invalid_str_return; mod invalid_string_characters; mod iteration_over_set; 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 new file mode 100644 index 0000000000000..75c33552b1f85 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0303_invalid_return_type_length.py.snap @@ -0,0 +1,51 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +invalid_return_type_length.py:6:16: PLE0303 `__len__` does not return a non-negative integer + | +4 | class Bool: +5 | def __len__(self): +6 | return True # [invalid-length-return] + | ^^^^ PLE0303 + | + +invalid_return_type_length.py:11:16: PLE0303 `__len__` does not return a non-negative integer + | + 9 | class Float: +10 | def __len__(self): +11 | return 3.05 # [invalid-length-return] + | ^^^^ PLE0303 + | + +invalid_return_type_length.py:16:16: PLE0303 `__len__` does not return a non-negative integer + | +14 | class Str: +15 | def __len__(self): +16 | return "ruff" # [invalid-length-return] + | ^^^^^^ PLE0303 + | + +invalid_return_type_length.py:20:9: PLE0303 `__len__` does not return a non-negative integer + | +19 | class LengthNoReturn: +20 | def __len__(self): + | ^^^^^^^ PLE0303 +21 | print("ruff") # [invalid-length-return] + | + +invalid_return_type_length.py:26:16: PLE0303 `__len__` does not return a non-negative integer + | +24 | class LengthNegative: +25 | def __len__(self): +26 | return -42 # [invalid-length-return] + | ^^^ PLE0303 + | + +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] + | diff --git a/ruff.schema.json b/ruff.schema.json index 16287b238ec7f..0d6827effe842 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3267,6 +3267,7 @@ "PLE03", "PLE030", "PLE0302", + "PLE0303", "PLE0304", "PLE0307", "PLE0308",