From be811c64ffc6a329e15e162bc70f9cebcc5b3aa0 Mon Sep 17 00:00:00 2001 From: Tibor Reiss Date: Mon, 15 Apr 2024 21:07:59 +0200 Subject: [PATCH 1/6] Implement E0308 - invalid-bytes-returned --- .../pylint/invalid_return_type_bytes.py | 35 +++++++++ .../src/checkers/ast/analyze/statement.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/pylint/mod.rs | 4 + .../pylint/rules/invalid_bytes_return.rs | 77 +++++++++++++++++++ .../ruff_linter/src/rules/pylint/rules/mod.rs | 2 + ..._PLE0308_invalid_return_type_bytes.py.snap | 32 ++++++++ ruff.schema.json | 1 + 8 files changed, 155 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_bytes.py create mode 100644 crates/ruff_linter/src/rules/pylint/rules/invalid_bytes_return.rs create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0308_invalid_return_type_bytes.py.snap 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..cab11da110e6f --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_bytes.py @@ -0,0 +1,35 @@ +# 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] + +# 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 diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 8eaf670f227fc..980003bfc57e1 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, name, body); + } 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/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..28e2e1f0d021d --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_bytes_return.rs @@ -0,0 +1,77 @@ +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; + } + + let returns = { + let mut visitor = ReturnStatementVisitor::default(); + visitor.visit_body(body); + visitor.returns + }; + + 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..7da53c8db9c6f --- /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,32 @@ +--- +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 | # TODO: Once Ruff has better type checking + | 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", From 197b5fcf2e829dd2cc18eae272c5d7c77d6e918a Mon Sep 17 00:00:00 2001 From: Tibor Reiss Date: Tue, 16 Apr 2024 19:56:17 +0200 Subject: [PATCH 2/6] Code review --- .../fixtures/pylint/invalid_return_type_bytes.py | 4 ++++ .../src/rules/pylint/rules/invalid_bytes_return.rs | 7 +++++++ ..._tests__PLE0308_invalid_return_type_bytes.py.snap | 12 +++++++++++- 3 files changed, 22 insertions(+), 1 deletion(-) 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 index cab11da110e6f..8f12af9346bac 100644 --- 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 @@ -12,6 +12,10 @@ class Str: def __bytes__(self): return "some bytes" # [invalid-bytes-return] +class BytesNoReturn: + def __bytes__(self): + print("ruff") # [invalid-bytes-return] + # TODO: Once Ruff has better type checking def return_bytes(): return "some string" 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 index 28e2e1f0d021d..3040572663efb 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/invalid_bytes_return.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_bytes_return.rs @@ -57,6 +57,13 @@ pub(crate) fn invalid_bytes_return(checker: &mut Checker, name: &str, body: &[St visitor.returns }; + if returns.is_empty() { + checker.diagnostics.push(Diagnostic::new( + InvalidBytesReturnType, + body.last().unwrap().range(), + )); + } + for stmt in returns { if let Some(value) = stmt.value.as_deref() { if !matches!( 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 index 7da53c8db9c6f..333aa49fabfa6 100644 --- 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 @@ -28,5 +28,15 @@ invalid_return_type_bytes.py:13:16: PLE0308 `__bytes__` does not return `bytes` 13 | return "some bytes" # [invalid-bytes-return] | ^^^^^^^^^^^^ PLE0308 14 | -15 | # TODO: Once Ruff has better type checking +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 | # TODO: Once Ruff has better type checking | From c5d6194645c3c535a59cc9d59b38f70563a32af8 Mon Sep 17 00:00:00 2001 From: Tibor Reiss Date: Wed, 17 Apr 2024 19:56:25 +0200 Subject: [PATCH 3/6] ellipsis, raise --- .../pylint/invalid_return_type_bytes.py | 19 +++++++++++++++++-- .../pylint/rules/invalid_bytes_return.rs | 8 ++++++++ ..._PLE0308_invalid_return_type_bytes.py.snap | 12 +++++++++++- 3 files changed, 36 insertions(+), 3 deletions(-) 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 index 8f12af9346bac..db55c3d835c61 100644 --- 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 @@ -16,6 +16,11 @@ 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" @@ -25,15 +30,25 @@ 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/rules/pylint/rules/invalid_bytes_return.rs b/crates/ruff_linter/src/rules/pylint/rules/invalid_bytes_return.rs index 3040572663efb..727d63570d9fd 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/invalid_bytes_return.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_bytes_return.rs @@ -51,6 +51,14 @@ pub(crate) fn invalid_bytes_return(checker: &mut Checker, name: &str, body: &[St 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 returns = { let mut visitor = ReturnStatementVisitor::default(); visitor.visit_body(body); 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 index 333aa49fabfa6..ac13e0243e6da 100644 --- 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 @@ -38,5 +38,15 @@ invalid_return_type_bytes.py:17:9: PLE0308 `__bytes__` does not return `bytes` 17 | print("ruff") # [invalid-bytes-return] | ^^^^^^^^^^^^^ PLE0308 18 | -19 | # TODO: Once Ruff has better type checking +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 | From 4faf31912c43827b1d03874c2d4e7bb34f0d02db Mon Sep 17 00:00:00 2001 From: Tibor Reiss Date: Wed, 17 Apr 2024 21:46:35 +0200 Subject: [PATCH 4/6] special raise --- .../src/rules/pylint/rules/invalid_bytes_return.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) 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 index 727d63570d9fd..05992f1770bfa 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/invalid_bytes_return.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_bytes_return.rs @@ -59,6 +59,17 @@ pub(crate) fn invalid_bytes_return(checker: &mut Checker, name: &str, body: &[St return; } + let body_without_comments = body + .iter() + .filter(|stmt| !matches!(stmt, Stmt::Expr(expr) if expr.value.is_string_literal_expr())) + .collect::>(); + 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); From e218cc1e4ddc14bd2cd9994870793fc95fc8913a Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 17 Apr 2024 21:21:15 -0400 Subject: [PATCH 5/6] Format fixture --- .../pylint/invalid_return_type_bytes.py | 15 +++++- ..._PLE0308_invalid_return_type_bytes.py.snap | 50 ++++++++----------- 2 files changed, 33 insertions(+), 32 deletions(-) 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 index db55c3d835c61..ea53d51e9b2e8 100644 --- 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 @@ -1,30 +1,37 @@ # 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] @@ -32,23 +39,27 @@ def __bytes__(self): # 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): - ... + def __bytes__(self): ... + class Bytes4: def __bytes__(self): pass + class Bytes5: def __bytes__(self): raise NotImplementedError 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 index ac13e0243e6da..a9772b0c94240 100644 --- 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 @@ -1,52 +1,42 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -invalid_return_type_bytes.py:5:16: PLE0308 `__bytes__` does not return `bytes` +invalid_return_type_bytes.py:6:16: PLE0308 `__bytes__` does not return `bytes` | -3 | class Float: -4 | def __bytes__(self): -5 | return 3.05 # [invalid-bytes-return] +4 | class Float: +5 | def __bytes__(self): +6 | return 3.05 # [invalid-bytes-return] | ^^^^ PLE0308 -6 | -7 | class Int: | -invalid_return_type_bytes.py:9:16: PLE0308 `__bytes__` does not return `bytes` +invalid_return_type_bytes.py:11:16: PLE0308 `__bytes__` does not return `bytes` | - 7 | class Int: - 8 | def __bytes__(self): - 9 | return 0 # [invalid-bytes-return] + 9 | class Int: +10 | def __bytes__(self): +11 | return 0 # [invalid-bytes-return] | ^ PLE0308 -10 | -11 | class Str: | -invalid_return_type_bytes.py:13:16: PLE0308 `__bytes__` does not return `bytes` +invalid_return_type_bytes.py:16:16: PLE0308 `__bytes__` does not return `bytes` | -11 | class Str: -12 | def __bytes__(self): -13 | return "some bytes" # [invalid-bytes-return] +14 | class Str: +15 | def __bytes__(self): +16 | return "some bytes" # [invalid-bytes-return] | ^^^^^^^^^^^^ PLE0308 -14 | -15 | class BytesNoReturn: | -invalid_return_type_bytes.py:17:9: PLE0308 `__bytes__` does not return `bytes` +invalid_return_type_bytes.py:21:9: PLE0308 `__bytes__` does not return `bytes` | -15 | class BytesNoReturn: -16 | def __bytes__(self): -17 | print("ruff") # [invalid-bytes-return] +19 | class BytesNoReturn: +20 | def __bytes__(self): +21 | print("ruff") # [invalid-bytes-return] | ^^^^^^^^^^^^^ PLE0308 -18 | -19 | class BytesWrongRaise: | -invalid_return_type_bytes.py:22:9: PLE0308 `__bytes__` does not return `bytes` +invalid_return_type_bytes.py:27:9: PLE0308 `__bytes__` does not return `bytes` | -20 | def __bytes__(self): -21 | print("raise some error") -22 | raise NotImplementedError # [invalid-bytes-return] +25 | def __bytes__(self): +26 | print("raise some error") +27 | raise NotImplementedError # [invalid-bytes-return] | ^^^^^^^^^^^^^^^^^^^^^^^^^ PLE0308 -23 | -24 | # TODO: Once Ruff has better type checking | From ced76b98afd87ab4af76a89e6418e705bc82c256 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 17 Apr 2024 21:24:28 -0400 Subject: [PATCH 6/6] Use is_stub --- .../src/checkers/ast/analyze/statement.rs | 2 +- .../rules/mutable_argument_default.rs | 31 ++----------------- .../pylint/rules/invalid_bytes_return.rs | 29 +++++------------ ..._PLE0308_invalid_return_type_bytes.py.snap | 9 +++--- .../src/analyze/function_type.rs | 28 ++++++++++++++++- 5 files changed, 44 insertions(+), 55 deletions(-) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 980003bfc57e1..f6f6321fd4702 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -98,7 +98,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { pylint::rules::invalid_bool_return(checker, name, body); } if checker.enabled(Rule::InvalidBytesReturnType) { - pylint::rules::invalid_bytes_return(checker, name, body); + 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/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/rules/invalid_bytes_return.rs b/crates/ruff_linter/src/rules/pylint/rules/invalid_bytes_return.rs index 05992f1770bfa..fdfe78227d89c 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/invalid_bytes_return.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_bytes_return.rs @@ -1,8 +1,10 @@ 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::Stmt; +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; @@ -42,8 +44,8 @@ impl Violation for InvalidBytesReturnType { } /// E0308 -pub(crate) fn invalid_bytes_return(checker: &mut Checker, name: &str, body: &[Stmt]) { - if name != "__bytes__" { +pub(crate) fn invalid_bytes_return(checker: &mut Checker, function_def: &ast::StmtFunctionDef) { + if function_def.name.as_str() != "__bytes__" { return; } @@ -51,35 +53,20 @@ pub(crate) fn invalid_bytes_return(checker: &mut Checker, name: &str, body: &[St 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::>(); - if body_without_comments.is_empty() { - return; - } - if body_without_comments.len() == 1 && body_without_comments[0].is_raise_stmt() { + if is_stub(function_def, checker.semantic()) { return; } let returns = { let mut visitor = ReturnStatementVisitor::default(); - visitor.visit_body(body); + visitor.visit_body(&function_def.body); visitor.returns }; if returns.is_empty() { checker.diagnostics.push(Diagnostic::new( InvalidBytesReturnType, - body.last().unwrap().range(), + function_def.identifier(), )); } 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 index a9772b0c94240..462121c23098a 100644 --- 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 @@ -25,18 +25,19 @@ invalid_return_type_bytes.py:16:16: PLE0308 `__bytes__` does not return `bytes` | ^^^^^^^^^^^^ PLE0308 | -invalid_return_type_bytes.py:21:9: PLE0308 `__bytes__` does not return `bytes` +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] - | ^^^^^^^^^^^^^ PLE0308 | -invalid_return_type_bytes.py:27:9: PLE0308 `__bytes__` does not return `bytes` +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] - | ^^^^^^^^^^^^^^^^^^^^^^^^^ PLE0308 | 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, + }) +}