Skip to content

Commit

Permalink
[pylint] Implement invalid-bytes-returned (E0308) (#10959)
Browse files Browse the repository at this point in the history
Add pylint rule invalid-bytes-returned (PLE0308)

See #970 for rules

Test Plan: `cargo test`
  • Loading branch information
tibor-reiss authored Apr 18, 2024
1 parent 06b3e37 commit 1480d72
Show file tree
Hide file tree
Showing 10 changed files with 239 additions and 29 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
# These testcases should raise errors


class Float:
def __bytes__(self):
return 3.05 # [invalid-bytes-return]


class Int:
def __bytes__(self):
return 0 # [invalid-bytes-return]


class Str:
def __bytes__(self):
return "some bytes" # [invalid-bytes-return]


class BytesNoReturn:
def __bytes__(self):
print("ruff") # [invalid-bytes-return]


class BytesWrongRaise:
def __bytes__(self):
print("raise some error")
raise NotImplementedError # [invalid-bytes-return]


# TODO: Once Ruff has better type checking
def return_bytes():
return "some string"


class ComplexReturn:
def __bytes__(self):
return return_bytes() # [invalid-bytes-return]


# These testcases should NOT raise errors


class Bytes:
def __bytes__(self):
return b"some bytes"


class Bytes2:
def __bytes__(self):
x = b"some bytes"
return x


class Bytes3:
def __bytes__(self): ...


class Bytes4:
def __bytes__(self):
pass


class Bytes5:
def __bytes__(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::InvalidBytesReturnType) {
pylint::rules::invalid_bytes_return(checker, function_def);
}
if checker.enabled(Rule::InvalidStrReturnType) {
pylint::rules::invalid_str_return(checker, name, body);
}
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 @@ -243,6 +243,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(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, "E0308") => (RuleGroup::Preview, rules::pylint::rules::InvalidBytesReturnType),
(Pylint, "E0604") => (RuleGroup::Stable, rules::pylint::rules::InvalidAllObject),
(Pylint, "E0605") => (RuleGroup::Stable, rules::pylint::rules::InvalidAllFormat),
(Pylint, "E0643") => (RuleGroup::Preview, rules::pylint::rules::PotentialIndexError),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::{is_docstring_stmt, map_callable};
use ruff_python_ast::helpers::is_docstring_stmt;
use ruff_python_ast::name::QualifiedName;
use ruff_python_ast::{self as ast, Expr, Parameter, ParameterWithDefault, Stmt, StmtRaise};
use ruff_python_ast::{self as ast, Expr, Parameter, ParameterWithDefault};
use ruff_python_codegen::{Generator, Stylist};
use ruff_python_index::Indexer;
use ruff_python_semantic::analyze::function_type::is_stub;
use ruff_python_semantic::analyze::typing::{is_immutable_annotation, is_mutable_expr};
use ruff_python_semantic::SemanticModel;
use ruff_python_trivia::{indentation_at_offset, textwrap};
Expand Down Expand Up @@ -213,29 +214,3 @@ fn move_initialization(
let initialization_edit = Edit::insertion(content, pos);
Some(Fix::unsafe_edits(default_edit, [initialization_edit]))
}

/// Returns `true` if a function has an empty body, and is therefore a stub.
///
/// A function body is considered to be empty if it contains only `pass` statements, `...` literals,
/// `NotImplementedError` raises, or string literal statements (docstrings).
fn is_stub(function_def: &ast::StmtFunctionDef, semantic: &SemanticModel) -> bool {
function_def.body.iter().all(|stmt| match stmt {
Stmt::Pass(_) => true,
Stmt::Expr(ast::StmtExpr { value, range: _ }) => {
matches!(
value.as_ref(),
Expr::StringLiteral(_) | Expr::EllipsisLiteral(_)
)
}
Stmt::Raise(StmtRaise {
range: _,
exc: exception,
cause: _,
}) => exception.as_ref().is_some_and(|exc| {
semantic
.resolve_builtin_symbol(map_callable(exc))
.is_some_and(|name| matches!(name, "NotImplementedError" | "NotImplemented"))
}),
_ => false,
})
}
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::InvalidBytesReturnType,
Path::new("invalid_return_type_bytes.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"))]
Expand Down
90 changes: 90 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/invalid_bytes_return.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
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};
use ruff_python_semantic::analyze::function_type::is_stub;
use ruff_python_semantic::analyze::type_inference::{PythonType, ResolvedPythonType};
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;

/// ## What it does
/// Checks for `__bytes__` implementations that return a type other than `bytes`.
///
/// ## Why is this bad?
/// The `__bytes__` method should return a `bytes` object. Returning a different
/// type may cause unexpected behavior.
///
/// ## Example
/// ```python
/// class Foo:
/// def __bytes__(self):
/// return 2
/// ```
///
/// Use instead:
/// ```python
/// class Foo:
/// def __bytes__(self):
/// return b"2"
/// ```
///
/// ## References
/// - [Python documentation: The `__bytes__` method](https://docs.python.org/3/reference/datamodel.html#object.__bytes__)
#[violation]
pub struct InvalidBytesReturnType;

impl Violation for InvalidBytesReturnType {
#[derive_message_formats]
fn message(&self) -> String {
format!("`__bytes__` does not return `bytes`")
}
}

/// E0308
pub(crate) fn invalid_bytes_return(checker: &mut Checker, function_def: &ast::StmtFunctionDef) {
if function_def.name.as_str() != "__bytes__" {
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(
InvalidBytesReturnType,
function_def.identifier(),
));
}

for stmt in returns {
if let Some(value) = stmt.value.as_deref() {
if !matches!(
ResolvedPythonType::from(value),
ResolvedPythonType::Unknown | ResolvedPythonType::Atom(PythonType::Bytes)
) {
checker
.diagnostics
.push(Diagnostic::new(InvalidBytesReturnType, value.range()));
}
} else {
// Disallow implicit `None`.
checker
.diagnostics
.push(Diagnostic::new(InvalidBytesReturnType, stmt.range()));
}
}
}
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 @@ -28,6 +28,7 @@ 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_bytes_return::*;
pub(crate) use invalid_envvar_default::*;
pub(crate) use invalid_envvar_value::*;
pub(crate) use invalid_str_return::*;
Expand Down Expand Up @@ -126,6 +127,7 @@ mod import_self;
mod invalid_all_format;
mod invalid_all_object;
mod invalid_bool_return;
mod invalid_bytes_return;
mod invalid_envvar_default;
mod invalid_envvar_value;
mod invalid_str_return;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
---
source: crates/ruff_linter/src/rules/pylint/mod.rs
---
invalid_return_type_bytes.py:6:16: PLE0308 `__bytes__` does not return `bytes`
|
4 | class Float:
5 | def __bytes__(self):
6 | return 3.05 # [invalid-bytes-return]
| ^^^^ PLE0308
|

invalid_return_type_bytes.py:11:16: PLE0308 `__bytes__` does not return `bytes`
|
9 | class Int:
10 | def __bytes__(self):
11 | return 0 # [invalid-bytes-return]
| ^ PLE0308
|

invalid_return_type_bytes.py:16:16: PLE0308 `__bytes__` does not return `bytes`
|
14 | class Str:
15 | def __bytes__(self):
16 | return "some bytes" # [invalid-bytes-return]
| ^^^^^^^^^^^^ PLE0308
|

invalid_return_type_bytes.py:20:9: PLE0308 `__bytes__` does not return `bytes`
|
19 | class BytesNoReturn:
20 | def __bytes__(self):
| ^^^^^^^^^ PLE0308
21 | print("ruff") # [invalid-bytes-return]
|

invalid_return_type_bytes.py:25:9: PLE0308 `__bytes__` does not return `bytes`
|
24 | class BytesWrongRaise:
25 | def __bytes__(self):
| ^^^^^^^^^ PLE0308
26 | print("raise some error")
27 | raise NotImplementedError # [invalid-bytes-return]
|
28 changes: 27 additions & 1 deletion crates/ruff_python_semantic/src/analyze/function_type.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use ruff_python_ast::helpers::map_callable;
use ruff_python_ast::name::{QualifiedName, UnqualifiedName};
use ruff_python_ast::Decorator;
use ruff_python_ast::{Decorator, Expr, Stmt, StmtExpr, StmtFunctionDef, StmtRaise};

use crate::model::SemanticModel;
use crate::scope::{Scope, ScopeKind};
Expand Down Expand Up @@ -128,3 +128,29 @@ fn is_class_method(

false
}

/// Returns `true` if a function has an empty body, and is therefore a stub.
///
/// A function body is considered to be empty if it contains only `pass` statements, `...` literals,
/// `NotImplementedError` raises, or string literal statements (docstrings).
pub fn is_stub(function_def: &StmtFunctionDef, semantic: &SemanticModel) -> bool {
function_def.body.iter().all(|stmt| match stmt {
Stmt::Pass(_) => true,
Stmt::Expr(StmtExpr { value, range: _ }) => {
matches!(
value.as_ref(),
Expr::StringLiteral(_) | Expr::EllipsisLiteral(_)
)
}
Stmt::Raise(StmtRaise {
range: _,
exc: exception,
cause: _,
}) => exception.as_ref().is_some_and(|exc| {
semantic
.resolve_builtin_symbol(map_callable(exc))
.is_some_and(|name| matches!(name, "NotImplementedError" | "NotImplemented"))
}),
_ => false,
})
}
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 1480d72

Please sign in to comment.