Skip to content

Commit

Permalink
[pylint] Implement eq-without-hash rule (PLW1641)
Browse files Browse the repository at this point in the history
  • Loading branch information
jelly committed Jul 24, 2023
1 parent 8a7dcb7 commit b669439
Show file tree
Hide file tree
Showing 8 changed files with 113 additions and 0 deletions.
16 changes: 16 additions & 0 deletions crates/ruff/resources/test/fixtures/pylint/eq_without_hash.py
Original file line number Diff line number Diff line change
@@ -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)
3 changes: 3 additions & 0 deletions crates/ruff/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -694,6 +694,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);
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Pylint, "W0711") => (RuleGroup::Unspecified, rules::pylint::rules::BinaryOpException),
(Pylint, "W1508") => (RuleGroup::Unspecified, rules::pylint::rules::InvalidEnvvarDefault),
(Pylint, "W1509") => (RuleGroup::Unspecified, rules::pylint::rules::SubprocessPopenPreexecFn),
(Pylint, "W1641") => (RuleGroup::Unspecified, rules::pylint::rules::EqWithoutHash),
(Pylint, "W2901") => (RuleGroup::Unspecified, rules::pylint::rules::RedefinedLoopName),
(Pylint, "W3301") => (RuleGroup::Unspecified, rules::pylint::rules::NestedMinMax),

Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"))]
Expand Down
75 changes: 75 additions & 0 deletions crates/ruff/src/rules/pylint/rules/eq_without_hash.rs
Original file line number Diff line number Diff line change
@@ -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?
/// A class that overrides eq() and does not define hash() will have its hash()
/// implicitly set to None.
///
/// ## 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 !has_eq_without_hash(body) {
return;
}
checker
.diagnostics
.push(Diagnostic::new(EqWithoutHash, name.range()));
}

fn has_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;
} else if name == "__eq__" {
has_eq = true;
}
}
has_eq && !has_hash
}
2 changes: 2 additions & 0 deletions crates/ruff/src/rules/pylint/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;
Expand Down Expand Up @@ -63,6 +64,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;
Expand Down
Original file line number Diff line number Diff line change
@@ -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"
|


3 changes: 3 additions & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit b669439

Please sign in to comment.