Skip to content

Commit

Permalink
[pylint] Implement invalid-length-returned (E0303) (#10963)
Browse files Browse the repository at this point in the history
Add pylint rule invalid-length-returned (PLE0303)

See #970 for rules

Test Plan: `cargo test`

TBD: from the description: "Strictly speaking `bool` is a subclass of
`int`, thus returning `True`/`False` is valid. To be consistent with
other rules (e.g.
[PLE0305](#10962)
invalid-index-returned), ruff will raise, compared to pylint which will
not raise."
  • Loading branch information
tibor-reiss authored Apr 18, 2024
1 parent b23414e commit 9f01ac3
Show file tree
Hide file tree
Showing 8 changed files with 242 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -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
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
4 changes: 4 additions & 0 deletions crates/ruff_linter/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
110 changes: 110 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/invalid_length_return.rs
Original file line number Diff line number Diff line change
@@ -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,
..
})
)
}
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
@@ -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]
|
1 change: 1 addition & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 9f01ac3

Please sign in to comment.