Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pylint] Implement invalid-bytes-returned (E0308) #10959

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
# 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, name, body);
}
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
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
103 changes: 103 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,103 @@
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, 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, name: &str, body: &[Stmt]) {
if name != "__bytes__" {
return;
}

if !checker.semantic().current_scope().kind.is_class() {
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::<Vec<_>>();
if body_without_comments.is_empty() {
return;
}
if body_without_comments.len() == 1 && body_without_comments[0].is_raise_stmt() {
return;
}

let returns = {
let mut visitor = ReturnStatementVisitor::default();
visitor.visit_body(body);
visitor.returns
};

if returns.is_empty() {
checker.diagnostics.push(Diagnostic::new(
InvalidBytesReturnType,
body.last().unwrap().range(),
));
}

for stmt in returns {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this rule and the other invalid-*-returned rules also flag the case where the method has no return statement

class Test:
	def __bytes__(self):
		print("nope")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this case as well. If this becomes the consensus, I can also add it to the already included bool (E0304) and str (E0307) cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MichaReiser As it turned out, the no-return case is not that simple: if there is just a pass/ellipsis/raise statement, then this rule should not raise. However, if there is e.g. a print statement and a raise statement, then it should raise - see the tests for examples.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this to use is_stub for detecting the other cases.

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,52 @@
---
source: crates/ruff_linter/src/rules/pylint/mod.rs
---
invalid_return_type_bytes.py:5:16: PLE0308 `__bytes__` does not return `bytes`
|
3 | class Float:
4 | def __bytes__(self):
5 | return 3.05 # [invalid-bytes-return]
| ^^^^ PLE0308
6 |
7 | class Int:
|

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

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

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

invalid_return_type_bytes.py:22:9: PLE0308 `__bytes__` does not return `bytes`
|
20 | def __bytes__(self):
21 | print("raise some error")
22 | raise NotImplementedError # [invalid-bytes-return]
| ^^^^^^^^^^^^^^^^^^^^^^^^^ PLE0308
23 |
24 | # TODO: Once Ruff has better type checking
|
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.