diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_bytes.py b/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_bytes.py new file mode 100644 index 0000000000000..ea53d51e9b2e8 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_bytes.py @@ -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 diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 8eaf670f227fc..f6f6321fd4702 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::InvalidBytesReturnType) { + pylint::rules::invalid_bytes_return(checker, function_def); + } if checker.enabled(Rule::InvalidStrReturnType) { pylint::rules::invalid_str_return(checker, name, body); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index ec5ae9e792ae4..780abad541c85 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -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), diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/mutable_argument_default.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/mutable_argument_default.rs index ae17d50b423ae..e028eb02c2084 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/mutable_argument_default.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/mutable_argument_default.rs @@ -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}; @@ -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, - }) -} diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index bcd050c0d40f7..2ee4e43b8f8fb 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::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"))] diff --git a/crates/ruff_linter/src/rules/pylint/rules/invalid_bytes_return.rs b/crates/ruff_linter/src/rules/pylint/rules/invalid_bytes_return.rs new file mode 100644 index 0000000000000..fdfe78227d89c --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_bytes_return.rs @@ -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())); + } + } +} diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 2eb23f352fd97..77c4da5c69664 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -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::*; @@ -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; diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0308_invalid_return_type_bytes.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0308_invalid_return_type_bytes.py.snap new file mode 100644 index 0000000000000..462121c23098a --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0308_invalid_return_type_bytes.py.snap @@ -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] + | diff --git a/crates/ruff_python_semantic/src/analyze/function_type.rs b/crates/ruff_python_semantic/src/analyze/function_type.rs index e249c45a64956..a13881df15108 100644 --- a/crates/ruff_python_semantic/src/analyze/function_type.rs +++ b/crates/ruff_python_semantic/src/analyze/function_type.rs @@ -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}; @@ -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, + }) +} diff --git a/ruff.schema.json b/ruff.schema.json index 90582fc6750e8..16287b238ec7f 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3269,6 +3269,7 @@ "PLE0302", "PLE0304", "PLE0307", + "PLE0308", "PLE06", "PLE060", "PLE0604",