From 97442dd6210eeeadfa67b29948013ffc2b729971 Mon Sep 17 00:00:00 2001 From: Tibor Reiss Date: Mon, 15 Apr 2024 21:16:17 +0200 Subject: [PATCH 1/6] Implement E0309 - invalid-hash-returned --- .../pylint/invalid_return_type_hash.py | 29 +++++++ .../src/checkers/ast/analyze/statement.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/pylint/mod.rs | 9 ++- .../rules/pylint/rules/invalid_hash_return.rs | 81 +++++++++++++++++++ .../ruff_linter/src/rules/pylint/rules/mod.rs | 2 + ...__PLE0309_invalid_return_type_hash.py.snap | 32 ++++++++ ruff.schema.json | 1 + 8 files changed, 154 insertions(+), 4 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_hash.py create mode 100644 crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0309_invalid_return_type_hash.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_hash.py b/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_hash.py new file mode 100644 index 0000000000000..ddd03c01142df --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_hash.py @@ -0,0 +1,29 @@ +# These testcases should raise errors + +class Bool: + def __hash__(self): + return True # [invalid-hash-return] + +class Float: + def __hash__(self): + return 3.05 # [invalid-hash-return] + +class Str: + def __hash__(self): + return "ruff" # [invalid-hash-return] + +# TODO: Once Ruff has better type checking +def return_int(): + return "3" + +class ComplexReturn: + def __hash__(self): + return return_int() # [invalid-hash-return] + + + +# These testcases should NOT raise errors + +class Hash: + def __hash__(self): + return 7741 diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index d7d189e849afd..e01bb803e697a 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -103,6 +103,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::InvalidBytesReturnType) { pylint::rules::invalid_bytes_return(checker, function_def); } + if checker.enabled(Rule::InvalidHashReturnType) { + pylint::rules::invalid_hash_return(checker, name, body); + } if checker.enabled(Rule::InvalidStrReturnType) { pylint::rules::invalid_str_return(checker, function_def); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 36ee42e65b37b..f5c30294eba4f 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -245,6 +245,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (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, "E0309") => (RuleGroup::Preview, rules::pylint::rules::InvalidHashReturnType), (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 c6c16ae36ea74..def31a22c258a 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -77,14 +77,15 @@ 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") )] + #[test_case(Rule::InvalidHashReturnType, Path::new("invalid_return_type_hash.py"))] + #[test_case( + Rule::InvalidLengthReturnType, + Path::new("invalid_return_type_length.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_hash_return.rs b/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs new file mode 100644 index 0000000000000..6420fb3d5d5b5 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs @@ -0,0 +1,81 @@ +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::{NumberLike, PythonType, ResolvedPythonType}; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for `__hash__` implementations that return a type other than `integer`. +/// +/// ## Why is this bad? +/// The `__hash__` method should return an `integer`. Returning a different +/// type may cause unexpected behavior. +/// +/// ## Example +/// ```python +/// class Foo: +/// def __hash__(self): +/// return "2" +/// ``` +/// +/// Use instead: +/// ```python +/// class Foo: +/// def __hash__(self): +/// return 2 +/// ``` +/// +/// Note: Strictly speaking `bool` is a subclass of `int`, thus returning `True`/`False` is valid. +/// To be consistent with other rules (e.g. PLE0305 invalid-index-returned), ruff will raise, compared +/// to pylint which will not raise. +/// ## References +/// - [Python documentation: The `__hash__` method](https://docs.python.org/3/reference/datamodel.html#object.__hash__) +#[violation] +pub struct InvalidHashReturnType; + +impl Violation for InvalidHashReturnType { + #[derive_message_formats] + fn message(&self) -> String { + format!("`__hash__` does not return `integer`") + } +} + +/// E0309 +pub(crate) fn invalid_hash_return(checker: &mut Checker, name: &str, body: &[Stmt]) { + if name != "__hash__" { + 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::Number(NumberLike::Integer)) + ) { + checker + .diagnostics + .push(Diagnostic::new(InvalidHashReturnType, value.range())); + } + } else { + // Disallow implicit `None`. + checker + .diagnostics + .push(Diagnostic::new(InvalidHashReturnType, 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 e0c51c55b758d..362a00f8390ce 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -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_hash_return::*; pub(crate) use invalid_length_return::*; pub(crate) use invalid_str_return::*; pub(crate) use invalid_string_characters::*; @@ -131,6 +132,7 @@ mod invalid_bool_return; mod invalid_bytes_return; mod invalid_envvar_default; mod invalid_envvar_value; +mod invalid_hash_return; mod invalid_length_return; mod invalid_str_return; mod invalid_string_characters; diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0309_invalid_return_type_hash.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0309_invalid_return_type_hash.py.snap new file mode 100644 index 0000000000000..8e6479a474665 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0309_invalid_return_type_hash.py.snap @@ -0,0 +1,32 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +invalid_return_type_hash.py:5:16: PLE0309 `__hash__` does not return `integer` + | +3 | class Bool: +4 | def __hash__(self): +5 | return True # [invalid-hash-return] + | ^^^^ PLE0309 +6 | +7 | class Float: + | + +invalid_return_type_hash.py:9:16: PLE0309 `__hash__` does not return `integer` + | + 7 | class Float: + 8 | def __hash__(self): + 9 | return 3.05 # [invalid-hash-return] + | ^^^^ PLE0309 +10 | +11 | class Str: + | + +invalid_return_type_hash.py:13:16: PLE0309 `__hash__` does not return `integer` + | +11 | class Str: +12 | def __hash__(self): +13 | return "ruff" # [invalid-hash-return] + | ^^^^^^ PLE0309 +14 | +15 | # TODO: Once Ruff has better type checking + | diff --git a/ruff.schema.json b/ruff.schema.json index 0d6827effe842..cb3bdae4539f6 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3271,6 +3271,7 @@ "PLE0304", "PLE0307", "PLE0308", + "PLE0309", "PLE06", "PLE060", "PLE0604", From 73a219bdc7a8f5e7627b4a0929ccbdc947a7d6c5 Mon Sep 17 00:00:00 2001 From: Tibor Reiss Date: Tue, 16 Apr 2024 20:07:14 +0200 Subject: [PATCH 2/6] Handle no return case --- .../test/fixtures/pylint/invalid_return_type_hash.py | 4 ++++ .../src/rules/pylint/rules/invalid_hash_return.rs | 7 +++++++ ...__tests__PLE0309_invalid_return_type_hash.py.snap | 12 +++++++++++- 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_hash.py b/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_hash.py index ddd03c01142df..46e0a0370c64e 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_hash.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_hash.py @@ -12,6 +12,10 @@ class Str: def __hash__(self): return "ruff" # [invalid-hash-return] +class HashNoReturn: + def __hash__(self): + print("ruff") # [invalid-hash-return] + # TODO: Once Ruff has better type checking def return_int(): return "3" diff --git a/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs b/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs index 6420fb3d5d5b5..40e4d041d7973 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs @@ -60,6 +60,13 @@ pub(crate) fn invalid_hash_return(checker: &mut Checker, name: &str, body: &[Stm visitor.returns }; + if returns.is_empty() { + checker.diagnostics.push(Diagnostic::new( + InvalidHashReturnType, + 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__PLE0309_invalid_return_type_hash.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0309_invalid_return_type_hash.py.snap index 8e6479a474665..9a728d4d31b26 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0309_invalid_return_type_hash.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0309_invalid_return_type_hash.py.snap @@ -28,5 +28,15 @@ invalid_return_type_hash.py:13:16: PLE0309 `__hash__` does not return `integer` 13 | return "ruff" # [invalid-hash-return] | ^^^^^^ PLE0309 14 | -15 | # TODO: Once Ruff has better type checking +15 | class HashNoReturn: + | + +invalid_return_type_hash.py:17:9: PLE0309 `__hash__` does not return `integer` + | +15 | class HashNoReturn: +16 | def __hash__(self): +17 | print("ruff") # [invalid-hash-return] + | ^^^^^^^^^^^^^ PLE0309 +18 | +19 | # TODO: Once Ruff has better type checking | From b36dcee1b17c590183d845423fd6bc20e9c9ac48 Mon Sep 17 00:00:00 2001 From: Tibor Reiss Date: Wed, 17 Apr 2024 19:57:30 +0200 Subject: [PATCH 3/6] ellipsis, raise --- .../pylint/invalid_return_type_hash.py | 22 +++++++++++++++++++ .../rules/pylint/rules/invalid_hash_return.rs | 8 +++++++ ...__PLE0309_invalid_return_type_hash.py.snap | 12 +++++++++- 3 files changed, 41 insertions(+), 1 deletion(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_hash.py b/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_hash.py index 46e0a0370c64e..937891150d30c 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_hash.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_hash.py @@ -16,6 +16,11 @@ class HashNoReturn: def __hash__(self): print("ruff") # [invalid-hash-return] +class HashWrongRaise: + def __hash__(self): + print("raise some error") + raise NotImplementedError # [invalid-hash-return] + # TODO: Once Ruff has better type checking def return_int(): return "3" @@ -31,3 +36,20 @@ def __hash__(self): class Hash: def __hash__(self): return 7741 + +class Hash2: + def __hash__(self): + x = 7741 + return x + +class Hash3: + def __hash__(self): + ... + +class Has4: + def __hash__(self): + pass + +class Hash5: + def __hash__(self): + raise NotImplementedError \ No newline at end of file diff --git a/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs b/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs index 40e4d041d7973..614a6997474f7 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs @@ -54,6 +54,14 @@ pub(crate) fn invalid_hash_return(checker: &mut Checker, name: &str, body: &[Stm 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__PLE0309_invalid_return_type_hash.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0309_invalid_return_type_hash.py.snap index 9a728d4d31b26..4ee1e34302d0a 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0309_invalid_return_type_hash.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0309_invalid_return_type_hash.py.snap @@ -38,5 +38,15 @@ invalid_return_type_hash.py:17:9: PLE0309 `__hash__` does not return `integer` 17 | print("ruff") # [invalid-hash-return] | ^^^^^^^^^^^^^ PLE0309 18 | -19 | # TODO: Once Ruff has better type checking +19 | class HashWrongRaise: + | + +invalid_return_type_hash.py:22:9: PLE0309 `__hash__` does not return `integer` + | +20 | def __hash__(self): +21 | print("raise some error") +22 | raise NotImplementedError # [invalid-hash-return] + | ^^^^^^^^^^^^^^^^^^^^^^^^^ PLE0309 +23 | +24 | # TODO: Once Ruff has better type checking | From 74f7ee5e9cd94b79b45c61cf5b38befc2e28f798 Mon Sep 17 00:00:00 2001 From: Tibor Reiss Date: Wed, 17 Apr 2024 21:44:55 +0200 Subject: [PATCH 4/6] special raise --- .../src/rules/pylint/rules/invalid_hash_return.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs b/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs index 614a6997474f7..a1f9d35392bd2 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs @@ -62,6 +62,17 @@ pub(crate) fn invalid_hash_return(checker: &mut Checker, name: &str, body: &[Stm 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 4875d683cd9025358041834804159028759667df Mon Sep 17 00:00:00 2001 From: Tibor Reiss Date: Thu, 18 Apr 2024 20:07:40 +0200 Subject: [PATCH 5/6] Small updates --- .../pylint/invalid_return_type_hash.py | 17 +++++- .../src/checkers/ast/analyze/statement.rs | 2 +- .../rules/pylint/rules/invalid_hash_return.rs | 37 +++++-------- ...__PLE0309_invalid_return_type_hash.py.snap | 55 ++++++++----------- 4 files changed, 51 insertions(+), 60 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_hash.py b/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_hash.py index 937891150d30c..166053833ed25 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_hash.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_hash.py @@ -1,30 +1,37 @@ # These testcases should raise errors + class Bool: def __hash__(self): return True # [invalid-hash-return] + class Float: def __hash__(self): return 3.05 # [invalid-hash-return] + class Str: def __hash__(self): return "ruff" # [invalid-hash-return] + class HashNoReturn: def __hash__(self): print("ruff") # [invalid-hash-return] + class HashWrongRaise: def __hash__(self): print("raise some error") raise NotImplementedError # [invalid-hash-return] + # TODO: Once Ruff has better type checking def return_int(): return "3" + class ComplexReturn: def __hash__(self): return return_int() # [invalid-hash-return] @@ -33,23 +40,27 @@ def __hash__(self): # These testcases should NOT raise errors + class Hash: def __hash__(self): return 7741 + class Hash2: def __hash__(self): x = 7741 return x + class Hash3: - def __hash__(self): - ... + def __hash__(self): ... + class Has4: def __hash__(self): pass + class Hash5: def __hash__(self): - raise NotImplementedError \ No newline at end of file + 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 e01bb803e697a..db30aeb3f7a83 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -104,7 +104,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { pylint::rules::invalid_bytes_return(checker, function_def); } if checker.enabled(Rule::InvalidHashReturnType) { - pylint::rules::invalid_hash_return(checker, name, body); + pylint::rules::invalid_hash_return(checker, function_def); } if checker.enabled(Rule::InvalidStrReturnType) { pylint::rules::invalid_str_return(checker, function_def); diff --git a/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs b/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs index a1f9d35392bd2..568cc88fb97ac 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_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::{NumberLike, PythonType, ResolvedPythonType}; use ruff_text_size::Ranged; @@ -15,6 +17,10 @@ use crate::checkers::ast::Checker; /// The `__hash__` method should return an `integer`. Returning a different /// type may cause unexpected behavior. /// +/// Note: `bool` is a subclass of `int`, so it's technically valid for `__hash__` to +/// return `True` or `False`. However, for consistency with other rules, Ruff will +/// still raise when `__hash__` returns a `bool`. +/// /// ## Example /// ```python /// class Foo: @@ -29,9 +35,7 @@ use crate::checkers::ast::Checker; /// return 2 /// ``` /// -/// Note: Strictly speaking `bool` is a subclass of `int`, thus returning `True`/`False` is valid. -/// To be consistent with other rules (e.g. PLE0305 invalid-index-returned), ruff will raise, compared -/// to pylint which will not raise. +/// /// ## References /// - [Python documentation: The `__hash__` method](https://docs.python.org/3/reference/datamodel.html#object.__hash__) #[violation] @@ -45,8 +49,8 @@ impl Violation for InvalidHashReturnType { } /// E0309 -pub(crate) fn invalid_hash_return(checker: &mut Checker, name: &str, body: &[Stmt]) { - if name != "__hash__" { +pub(crate) fn invalid_hash_return(checker: &mut Checker, function_def: &ast::StmtFunctionDef) { + if function_def.name.as_str() != "__hash__" { return; } @@ -54,35 +58,20 @@ pub(crate) fn invalid_hash_return(checker: &mut Checker, name: &str, body: &[Stm 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( InvalidHashReturnType, - body.last().unwrap().range(), + function_def.identifier(), )); } diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0309_invalid_return_type_hash.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0309_invalid_return_type_hash.py.snap index 4ee1e34302d0a..209b82ae60293 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0309_invalid_return_type_hash.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0309_invalid_return_type_hash.py.snap @@ -1,52 +1,43 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -invalid_return_type_hash.py:5:16: PLE0309 `__hash__` does not return `integer` +invalid_return_type_hash.py:6:16: PLE0309 `__hash__` does not return `integer` | -3 | class Bool: -4 | def __hash__(self): -5 | return True # [invalid-hash-return] +4 | class Bool: +5 | def __hash__(self): +6 | return True # [invalid-hash-return] | ^^^^ PLE0309 -6 | -7 | class Float: | -invalid_return_type_hash.py:9:16: PLE0309 `__hash__` does not return `integer` +invalid_return_type_hash.py:11:16: PLE0309 `__hash__` does not return `integer` | - 7 | class Float: - 8 | def __hash__(self): - 9 | return 3.05 # [invalid-hash-return] + 9 | class Float: +10 | def __hash__(self): +11 | return 3.05 # [invalid-hash-return] | ^^^^ PLE0309 -10 | -11 | class Str: | -invalid_return_type_hash.py:13:16: PLE0309 `__hash__` does not return `integer` +invalid_return_type_hash.py:16:16: PLE0309 `__hash__` does not return `integer` | -11 | class Str: -12 | def __hash__(self): -13 | return "ruff" # [invalid-hash-return] +14 | class Str: +15 | def __hash__(self): +16 | return "ruff" # [invalid-hash-return] | ^^^^^^ PLE0309 -14 | -15 | class HashNoReturn: | -invalid_return_type_hash.py:17:9: PLE0309 `__hash__` does not return `integer` +invalid_return_type_hash.py:20:9: PLE0309 `__hash__` does not return `integer` | -15 | class HashNoReturn: -16 | def __hash__(self): -17 | print("ruff") # [invalid-hash-return] - | ^^^^^^^^^^^^^ PLE0309 -18 | -19 | class HashWrongRaise: +19 | class HashNoReturn: +20 | def __hash__(self): + | ^^^^^^^^ PLE0309 +21 | print("ruff") # [invalid-hash-return] | -invalid_return_type_hash.py:22:9: PLE0309 `__hash__` does not return `integer` +invalid_return_type_hash.py:25:9: PLE0309 `__hash__` does not return `integer` | -20 | def __hash__(self): -21 | print("raise some error") -22 | raise NotImplementedError # [invalid-hash-return] - | ^^^^^^^^^^^^^^^^^^^^^^^^^ PLE0309 -23 | -24 | # TODO: Once Ruff has better type checking +24 | class HashWrongRaise: +25 | def __hash__(self): + | ^^^^^^^^ PLE0309 +26 | print("raise some error") +27 | raise NotImplementedError # [invalid-hash-return] | From b5d22aca0349f6cf32717dd13fa4535f2c7abecd Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 18 Apr 2024 23:26:37 -0400 Subject: [PATCH 6/6] Avoid raise --- .../pylint/invalid_return_type_hash.py | 13 +++++----- .../rules/pylint/rules/invalid_hash_return.rs | 25 +++++++++++++------ ...__PLE0309_invalid_return_type_hash.py.snap | 17 +++---------- 3 files changed, 28 insertions(+), 27 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_hash.py b/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_hash.py index 166053833ed25..cd43330f2c887 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_hash.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_hash.py @@ -21,12 +21,6 @@ def __hash__(self): print("ruff") # [invalid-hash-return] -class HashWrongRaise: - def __hash__(self): - print("raise some error") - raise NotImplementedError # [invalid-hash-return] - - # TODO: Once Ruff has better type checking def return_int(): return "3" @@ -37,7 +31,6 @@ def __hash__(self): return return_int() # [invalid-hash-return] - # These testcases should NOT raise errors @@ -64,3 +57,9 @@ def __hash__(self): class Hash5: def __hash__(self): raise NotImplementedError + + +class HashWrong6: + def __hash__(self): + print("raise some error") + raise NotImplementedError diff --git a/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs b/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs index 568cc88fb97ac..fe585ccf5d287 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs @@ -5,6 +5,7 @@ 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::terminal::Terminal; use ruff_python_semantic::analyze::type_inference::{NumberLike, PythonType, ResolvedPythonType}; use ruff_text_size::Ranged; @@ -44,7 +45,7 @@ pub struct InvalidHashReturnType; impl Violation for InvalidHashReturnType { #[derive_message_formats] fn message(&self) -> String { - format!("`__hash__` does not return `integer`") + format!("`__hash__` does not return an integer") } } @@ -62,19 +63,29 @@ pub(crate) fn invalid_hash_return(checker: &mut Checker, function_def: &ast::Stm return; } - let returns = { - let mut visitor = ReturnStatementVisitor::default(); - visitor.visit_body(&function_def.body); - visitor.returns - }; + // Determine the terminal behavior (i.e., implicit return, no return, etc.). + let terminal = Terminal::from_function(function_def); + + // If every control flow path raises an exception, ignore the function. + if terminal == Terminal::Raise { + return; + } - if returns.is_empty() { + // If there are no return statements, add a diagnostic. + if terminal == Terminal::Implicit { checker.diagnostics.push(Diagnostic::new( InvalidHashReturnType, function_def.identifier(), )); + return; } + let returns = { + let mut visitor = ReturnStatementVisitor::default(); + visitor.visit_body(&function_def.body); + visitor.returns + }; + 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__PLE0309_invalid_return_type_hash.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0309_invalid_return_type_hash.py.snap index 209b82ae60293..cffa2bcfc47d9 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0309_invalid_return_type_hash.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0309_invalid_return_type_hash.py.snap @@ -1,7 +1,7 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -invalid_return_type_hash.py:6:16: PLE0309 `__hash__` does not return `integer` +invalid_return_type_hash.py:6:16: PLE0309 `__hash__` does not return an integer | 4 | class Bool: 5 | def __hash__(self): @@ -9,7 +9,7 @@ invalid_return_type_hash.py:6:16: PLE0309 `__hash__` does not return `integer` | ^^^^ PLE0309 | -invalid_return_type_hash.py:11:16: PLE0309 `__hash__` does not return `integer` +invalid_return_type_hash.py:11:16: PLE0309 `__hash__` does not return an integer | 9 | class Float: 10 | def __hash__(self): @@ -17,7 +17,7 @@ invalid_return_type_hash.py:11:16: PLE0309 `__hash__` does not return `integer` | ^^^^ PLE0309 | -invalid_return_type_hash.py:16:16: PLE0309 `__hash__` does not return `integer` +invalid_return_type_hash.py:16:16: PLE0309 `__hash__` does not return an integer | 14 | class Str: 15 | def __hash__(self): @@ -25,19 +25,10 @@ invalid_return_type_hash.py:16:16: PLE0309 `__hash__` does not return `integer` | ^^^^^^ PLE0309 | -invalid_return_type_hash.py:20:9: PLE0309 `__hash__` does not return `integer` +invalid_return_type_hash.py:20:9: PLE0309 `__hash__` does not return an integer | 19 | class HashNoReturn: 20 | def __hash__(self): | ^^^^^^^^ PLE0309 21 | print("ruff") # [invalid-hash-return] | - -invalid_return_type_hash.py:25:9: PLE0309 `__hash__` does not return `integer` - | -24 | class HashWrongRaise: -25 | def __hash__(self): - | ^^^^^^^^ PLE0309 -26 | print("raise some error") -27 | raise NotImplementedError # [invalid-hash-return] - |