From 426e227d1439ecbaf62cffa7c54b760260b234f9 Mon Sep 17 00:00:00 2001 From: Jelle van der Waa Date: Fri, 21 Jul 2023 22:58:27 +0200 Subject: [PATCH] [pylint] Implement `eq-without-hash` rule (PLW1641) --- .../test/fixtures/pylint/eq_without_hash.py | 16 ++++ crates/ruff/src/checkers/ast/mod.rs | 3 + crates/ruff/src/codes.rs | 1 + crates/ruff/src/rules/pylint/mod.rs | 1 + .../src/rules/pylint/rules/eq_without_hash.rs | 75 +++++++++++++++++++ crates/ruff/src/rules/pylint/rules/mod.rs | 2 + ...nt__tests__PLW1641_eq_without_hash.py.snap | 12 +++ ruff.schema.json | 3 + 8 files changed, 113 insertions(+) create mode 100644 crates/ruff/resources/test/fixtures/pylint/eq_without_hash.py create mode 100644 crates/ruff/src/rules/pylint/rules/eq_without_hash.rs create mode 100644 crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW1641_eq_without_hash.py.snap diff --git a/crates/ruff/resources/test/fixtures/pylint/eq_without_hash.py b/crates/ruff/resources/test/fixtures/pylint/eq_without_hash.py new file mode 100644 index 00000000000000..44a62f3b7aa6a4 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/pylint/eq_without_hash.py @@ -0,0 +1,16 @@ +class Person: + def __init__(self): + self.name = "monty" + + def __eq__(self, other): + return isinstance(other, Person) and other.name == self.name + +class Language: + def __init__(self): + self.name = "python" + + def __eq__(self, other): + return isinstance(other, Language) and other.name == self.name + + def __hash__(self): + return hash(self.name) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index c48dcb59f4b3b7..e906323c27a427 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -654,6 +654,9 @@ where if self.enabled(Rule::DjangoUnorderedBodyContentInModel) { flake8_django::rules::unordered_body_content_in_model(self, bases, body); } + if self.enabled(Rule::EqWithoutHash) { + pylint::rules::object_without_hash_method(self, class_def); + } if !self.is_stub { if self.enabled(Rule::DjangoModelWithoutDunderStr) { flake8_django::rules::model_without_dunder_str(self, class_def); diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 38f97cf14857f7..a520e749414ee6 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -221,6 +221,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "W0603") => (RuleGroup::Unspecified, rules::pylint::rules::GlobalStatement), (Pylint, "W0711") => (RuleGroup::Unspecified, rules::pylint::rules::BinaryOpException), (Pylint, "W1508") => (RuleGroup::Unspecified, rules::pylint::rules::InvalidEnvvarDefault), + (Pylint, "W1641") => (RuleGroup::Unspecified, rules::pylint::rules::EqWithoutHash), (Pylint, "W2901") => (RuleGroup::Unspecified, rules::pylint::rules::RedefinedLoopName), (Pylint, "W3301") => (RuleGroup::Unspecified, rules::pylint::rules::NestedMinMax), diff --git a/crates/ruff/src/rules/pylint/mod.rs b/crates/ruff/src/rules/pylint/mod.rs index c182a14b23906d..1558c9227b0dcd 100644 --- a/crates/ruff/src/rules/pylint/mod.rs +++ b/crates/ruff/src/rules/pylint/mod.rs @@ -32,6 +32,7 @@ mod tests { Path::new("repeated_isinstance_calls.py") )] #[test_case(Rule::ComparisonWithItself, Path::new("comparison_with_itself.py"))] + #[test_case(Rule::EqWithoutHash, Path::new("eq_without_hash.py"))] #[test_case(Rule::ManualFromImport, Path::new("import_aliasing.py"))] #[test_case(Rule::SingleStringSlots, Path::new("single_string_slots.py"))] #[test_case(Rule::SysExitAlias, Path::new("sys_exit_alias_0.py"))] diff --git a/crates/ruff/src/rules/pylint/rules/eq_without_hash.rs b/crates/ruff/src/rules/pylint/rules/eq_without_hash.rs new file mode 100644 index 00000000000000..3db5b26feb6f01 --- /dev/null +++ b/crates/ruff/src/rules/pylint/rules/eq_without_hash.rs @@ -0,0 +1,75 @@ +use rustpython_parser::ast::{self, Ranged, Stmt}; + +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks that a class that implements a `__eq__` also implements `__hash__`. +/// +/// ## Why is this bad? +/// +/// +/// ## Example +/// ```python +/// class Person: +/// def __init__(self): +/// self.name = "monty" +/// +/// def __eq__(self, other): +/// return isinstance(other, Person) and other.name == self.name +/// ``` +/// +/// Use instead: +/// ```python +/// class Person: +/// def __init__(self): +/// self.name = "monty" +/// +/// def __eq__(self, other): +/// return isinstance(other, Person) and other.name == self.name +/// +/// def __hash__(self): +/// return hash(self.name) +/// ``` +#[violation] +pub struct EqWithoutHash; + +impl Violation for EqWithoutHash { + #[derive_message_formats] + fn message(&self) -> String { + format!("Object does not implement `__hash__` method") + } +} + +/// W1641 +pub(crate) fn object_without_hash_method( + checker: &mut Checker, + ast::StmtClassDef { name, body, .. }: &ast::StmtClassDef, +) { + if !hash_eq_without_hash(body) { + return; + } + checker + .diagnostics + .push(Diagnostic::new(EqWithoutHash, name.range())); +} + +fn hash_eq_without_hash(body: &[Stmt]) -> bool { + let mut has_hash = false; + let mut has_eq = false; + + for statement in body { + let Stmt::FunctionDef(ast::StmtFunctionDef { name, .. }) = statement else { + continue; + }; + if name == "__hash__" { + has_hash = true; + } + if name == "__eq__" { + has_eq = true; + } + } + has_eq && !has_hash +} diff --git a/crates/ruff/src/rules/pylint/rules/mod.rs b/crates/ruff/src/rules/pylint/rules/mod.rs index 829c9e6916a9b2..62fadf7b27ffe8 100644 --- a/crates/ruff/src/rules/pylint/rules/mod.rs +++ b/crates/ruff/src/rules/pylint/rules/mod.rs @@ -10,6 +10,7 @@ pub(crate) use comparison_of_constant::*; pub(crate) use comparison_with_itself::*; pub(crate) use continue_in_finally::*; pub(crate) use duplicate_bases::*; +pub(crate) use eq_without_hash::*; pub(crate) use global_statement::*; pub(crate) use global_variable_not_assigned::*; pub(crate) use import_self::*; @@ -61,6 +62,7 @@ mod comparison_of_constant; mod comparison_with_itself; mod continue_in_finally; mod duplicate_bases; +mod eq_without_hash; mod global_statement; mod global_variable_not_assigned; mod import_self; diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW1641_eq_without_hash.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW1641_eq_without_hash.py.snap new file mode 100644 index 00000000000000..83af186c0079b0 --- /dev/null +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW1641_eq_without_hash.py.snap @@ -0,0 +1,12 @@ +--- +source: crates/ruff/src/rules/pylint/mod.rs +--- +eq_without_hash.py:1:7: PLW1641 Object does not implement `__hash__` method + | +1 | class Person: + | ^^^^^^ PLW1641 +2 | def __init__(self): +3 | self.name = "monty" + | + + diff --git a/ruff.schema.json b/ruff.schema.json index 66a99e5a568027..e4b0843d27b001 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2258,6 +2258,9 @@ "PLW15", "PLW150", "PLW1508", + "PLW16", + "PLW164", + "PLW1641", "PLW2", "PLW29", "PLW290",