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..cd43330f2c887 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_hash.py @@ -0,0 +1,65 @@ +# 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] + + +# 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 + + +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 + + +class HashWrong6: + def __hash__(self): + print("raise some error") + 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 d7d189e849afd..db30aeb3f7a83 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, function_def); + } 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..fe585ccf5d287 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs @@ -0,0 +1,107 @@ +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::terminal::Terminal; +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. +/// +/// 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: +/// def __hash__(self): +/// return "2" +/// ``` +/// +/// Use instead: +/// ```python +/// class Foo: +/// def __hash__(self): +/// return 2 +/// ``` +/// +/// +/// ## 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 an integer") + } +} + +/// E0309 +pub(crate) fn invalid_hash_return(checker: &mut Checker, function_def: &ast::StmtFunctionDef) { + if function_def.name.as_str() != "__hash__" { + return; + } + + if !checker.semantic().current_scope().kind.is_class() { + return; + } + + if is_stub(function_def, checker.semantic()) { + return; + } + + // 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 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!( + 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..cffa2bcfc47d9 --- /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,34 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +invalid_return_type_hash.py:6:16: PLE0309 `__hash__` does not return an integer + | +4 | class Bool: +5 | def __hash__(self): +6 | return True # [invalid-hash-return] + | ^^^^ PLE0309 + | + +invalid_return_type_hash.py:11:16: PLE0309 `__hash__` does not return an integer + | + 9 | class Float: +10 | def __hash__(self): +11 | return 3.05 # [invalid-hash-return] + | ^^^^ PLE0309 + | + +invalid_return_type_hash.py:16:16: PLE0309 `__hash__` does not return an integer + | +14 | class Str: +15 | def __hash__(self): +16 | return "ruff" # [invalid-hash-return] + | ^^^^^^ PLE0309 + | + +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] + | 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",