diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/eq_without_hash.py b/crates/ruff_linter/resources/test/fixtures/pylint/eq_without_hash.py index 35c2fab7b5..d4fc15e5c6 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/eq_without_hash.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/eq_without_hash.py @@ -1,4 +1,6 @@ -class Person: # [eq-without-hash] +### Errors + +class Person: def __init__(self): self.name = "monty" @@ -6,7 +8,108 @@ class Person: # [eq-without-hash] return isinstance(other, Person) and other.name == self.name -# OK +class MaybeEqIf: + if ...: + def __eq__(self, other): ... + + +class MaybeEqElif: + if ...: + ... + elif ...: + def __eq__(self, other): ... + + +class MaybeEqElse: + if ...: + ... + else: + def __eq__(self, other): ... + + +class MaybeEqWith: + with ...: + def __eq__(self, other): ... + + +class MaybeEqFor: + for _ in ...: + def __eq__(self, other): ... + + +class MaybeEqForElse: + for _ in ...: + ... + else: + def __eq__(self, other): ... + + +class MaybeEqWhile: + while ...: + def __eq__(self, other): ... + + +class MaybeEqWhileElse: + while ...: + ... + else: + def __eq__(self, other): ... + + +class MaybeEqTry: + try: + def __eq__(self, other): ... + except Exception: + ... + + +class MaybeEqTryExcept: + try: + ... + except Exception: + def __eq__(self, other): ... + + +class MaybeEqTryExceptElse: + try: + ... + except Exception: + ... + else: + def __eq__(self, other): ... + + +class MaybeEqTryFinally: + try: + ... + finally: + def __eq__(self, other): ... + + +class MaybeEqMatchCase: + match ...: + case int(): + def __eq__(self, other): ... + + +class MaybeEqMatchCaseWildcard: + match ...: + case int(): ... + case _: + def __eq__(self, other): ... + + +class MaybeEqDeeplyNested: + if ...: + ... + else: + with ...: + for _ in ...: + def __eq__(self, other): ... + + +### OK + class Language: def __init__(self): self.name = "python" diff --git a/crates/ruff_linter/src/rules/pylint/rules/eq_without_hash.rs b/crates/ruff_linter/src/rules/pylint/rules/eq_without_hash.rs index e87a1b9e30..616da20958 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/eq_without_hash.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/eq_without_hash.rs @@ -1,8 +1,13 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, ViolationMetadata}; - -use ruff_python_ast::{self as ast, Expr, Stmt}; +use ruff_python_ast::{ + Expr, ExprName, Identifier, StmtAnnAssign, StmtAssign, StmtClassDef, StmtFunctionDef, +}; +use ruff_python_semantic::analyze::class::{ + any_member_declaration, ClassMemberBoundness, ClassMemberKind, +}; use ruff_text_size::Ranged; +use std::ops::BitOr; use crate::checkers::ast::Checker; @@ -105,47 +110,96 @@ impl Violation for EqWithoutHash { } /// W1641 -pub(crate) fn object_without_hash_method( - checker: &mut Checker, - ast::StmtClassDef { name, body, .. }: &ast::StmtClassDef, -) { - if has_eq_without_hash(body) { - checker - .diagnostics - .push(Diagnostic::new(EqWithoutHash, name.range())); +pub(crate) fn object_without_hash_method(checker: &mut Checker, class: &StmtClassDef) { + let eq_hash = EqHash::from_class(class); + if matches!( + eq_hash, + EqHash { + eq: HasMethod::Yes | HasMethod::Maybe, + hash: HasMethod::No + } + ) { + let diagnostic = Diagnostic::new(EqWithoutHash, class.name.range()); + checker.diagnostics.push(diagnostic); } } -fn has_eq_without_hash(body: &[Stmt]) -> bool { - let mut has_hash = false; - let mut has_eq = false; - for statement in body { - match statement { - Stmt::Assign(ast::StmtAssign { targets, .. }) => { - let [Expr::Name(ast::ExprName { id, .. })] = targets.as_slice() else { - continue; - }; +#[derive(Debug)] +struct EqHash { + hash: HasMethod, + eq: HasMethod, +} - // Check if `__hash__` was explicitly set, as in: - // ```python - // class Class(SuperClass): - // def __eq__(self, other): - // return True - // - // __hash__ = SuperClass.__hash__ - // ``` +impl EqHash { + fn from_class(class: &StmtClassDef) -> Self { + let (mut has_eq, mut has_hash) = (HasMethod::No, HasMethod::No); - if id == "__hash__" { - has_hash = true; + any_member_declaration(class, &mut |declaration| { + let id = match declaration.kind() { + ClassMemberKind::Assign(StmtAssign { targets, .. }) => { + let [Expr::Name(ExprName { id, .. })] = &targets[..] else { + return false; + }; + + id } - } - Stmt::FunctionDef(ast::StmtFunctionDef { name, .. }) => match name.as_str() { - "__hash__" => has_hash = true, - "__eq__" => has_eq = true, + ClassMemberKind::AnnAssign(StmtAnnAssign { target, .. }) => { + let Expr::Name(ExprName { id, .. }) = target.as_ref() else { + return false; + }; + + id + } + ClassMemberKind::FunctionDef(StmtFunctionDef { + name: Identifier { id, .. }, + .. + }) => id.as_str(), + }; + + match id { + "__eq__" => has_eq = has_eq | declaration.boundness().into(), + "__hash__" => has_hash = has_hash | declaration.boundness().into(), _ => {} - }, - _ => {} + } + + !has_eq.is_no() && !has_hash.is_no() + }); + + Self { + eq: has_eq, + hash: has_hash, + } + } +} + +#[derive(Debug, Copy, Clone, Eq, PartialEq, is_macro::Is, Default)] +enum HasMethod { + /// There is no assignment or declaration. + #[default] + No, + /// The assignment or declaration is placed directly within the class body. + Yes, + /// The assignment or declaration is placed within an intermediate block + /// (`if`/`elif`/`else`, `for`/`else`, `while`/`else`, `with`, `case`, `try`/`except`). + Maybe, +} + +impl From for HasMethod { + fn from(value: ClassMemberBoundness) -> Self { + match value { + ClassMemberBoundness::PossiblyUnbound => Self::Maybe, + ClassMemberBoundness::Bound => Self::Yes, + } + } +} + +impl BitOr for HasMethod { + type Output = Self; + + fn bitor(self, rhs: Self) -> Self::Output { + match (self, rhs) { + (HasMethod::No, _) => rhs, + (_, _) => self, } } - has_eq && !has_hash } diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW1641_eq_without_hash.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW1641_eq_without_hash.py.snap index 94cf3bf44f..33ba7d3796 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW1641_eq_without_hash.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW1641_eq_without_hash.py.snap @@ -2,10 +2,132 @@ source: crates/ruff_linter/src/rules/pylint/mod.rs snapshot_kind: text --- -eq_without_hash.py:1:7: PLW1641 Object does not implement `__hash__` method +eq_without_hash.py:3:7: PLW1641 Object does not implement `__hash__` method | -1 | class Person: # [eq-without-hash] +1 | ### Errors +2 | +3 | class Person: | ^^^^^^ PLW1641 -2 | def __init__(self): -3 | self.name = "monty" +4 | def __init__(self): +5 | self.name = "monty" | + +eq_without_hash.py:11:7: PLW1641 Object does not implement `__hash__` method + | +11 | class MaybeEqIf: + | ^^^^^^^^^ PLW1641 +12 | if ...: +13 | def __eq__(self, other): ... + | + +eq_without_hash.py:16:7: PLW1641 Object does not implement `__hash__` method + | +16 | class MaybeEqElif: + | ^^^^^^^^^^^ PLW1641 +17 | if ...: +18 | ... + | + +eq_without_hash.py:23:7: PLW1641 Object does not implement `__hash__` method + | +23 | class MaybeEqElse: + | ^^^^^^^^^^^ PLW1641 +24 | if ...: +25 | ... + | + +eq_without_hash.py:30:7: PLW1641 Object does not implement `__hash__` method + | +30 | class MaybeEqWith: + | ^^^^^^^^^^^ PLW1641 +31 | with ...: +32 | def __eq__(self, other): ... + | + +eq_without_hash.py:35:7: PLW1641 Object does not implement `__hash__` method + | +35 | class MaybeEqFor: + | ^^^^^^^^^^ PLW1641 +36 | for _ in ...: +37 | def __eq__(self, other): ... + | + +eq_without_hash.py:40:7: PLW1641 Object does not implement `__hash__` method + | +40 | class MaybeEqForElse: + | ^^^^^^^^^^^^^^ PLW1641 +41 | for _ in ...: +42 | ... + | + +eq_without_hash.py:47:7: PLW1641 Object does not implement `__hash__` method + | +47 | class MaybeEqWhile: + | ^^^^^^^^^^^^ PLW1641 +48 | while ...: +49 | def __eq__(self, other): ... + | + +eq_without_hash.py:52:7: PLW1641 Object does not implement `__hash__` method + | +52 | class MaybeEqWhileElse: + | ^^^^^^^^^^^^^^^^ PLW1641 +53 | while ...: +54 | ... + | + +eq_without_hash.py:59:7: PLW1641 Object does not implement `__hash__` method + | +59 | class MaybeEqTry: + | ^^^^^^^^^^ PLW1641 +60 | try: +61 | def __eq__(self, other): ... + | + +eq_without_hash.py:66:7: PLW1641 Object does not implement `__hash__` method + | +66 | class MaybeEqTryExcept: + | ^^^^^^^^^^^^^^^^ PLW1641 +67 | try: +68 | ... + | + +eq_without_hash.py:73:7: PLW1641 Object does not implement `__hash__` method + | +73 | class MaybeEqTryExceptElse: + | ^^^^^^^^^^^^^^^^^^^^ PLW1641 +74 | try: +75 | ... + | + +eq_without_hash.py:82:7: PLW1641 Object does not implement `__hash__` method + | +82 | class MaybeEqTryFinally: + | ^^^^^^^^^^^^^^^^^ PLW1641 +83 | try: +84 | ... + | + +eq_without_hash.py:89:7: PLW1641 Object does not implement `__hash__` method + | +89 | class MaybeEqMatchCase: + | ^^^^^^^^^^^^^^^^ PLW1641 +90 | match ...: +91 | case int(): + | + +eq_without_hash.py:95:7: PLW1641 Object does not implement `__hash__` method + | +95 | class MaybeEqMatchCaseWildcard: + | ^^^^^^^^^^^^^^^^^^^^^^^^ PLW1641 +96 | match ...: +97 | case int(): ... + | + +eq_without_hash.py:102:7: PLW1641 Object does not implement `__hash__` method + | +102 | class MaybeEqDeeplyNested: + | ^^^^^^^^^^^^^^^^^^^ PLW1641 +103 | if ...: +104 | ... + | diff --git a/crates/ruff_python_semantic/src/analyze/class.rs b/crates/ruff_python_semantic/src/analyze/class.rs index bcdaf5cbe0..34f1f058c1 100644 --- a/crates/ruff_python_semantic/src/analyze/class.rs +++ b/crates/ruff_python_semantic/src/analyze/class.rs @@ -5,7 +5,10 @@ use crate::{BindingId, SemanticModel}; use ruff_python_ast as ast; use ruff_python_ast::helpers::map_subscript; use ruff_python_ast::name::QualifiedName; -use ruff_python_ast::{Expr, ExprName, ExprStarred, ExprSubscript, ExprTuple}; +use ruff_python_ast::{ + ExceptHandler, Expr, ExprName, ExprStarred, ExprSubscript, ExprTuple, Stmt, StmtFor, StmtIf, + StmtMatch, StmtTry, StmtWhile, StmtWith, +}; /// Return `true` if any base class matches a [`QualifiedName`] predicate. pub fn any_qualified_base_class( @@ -109,6 +112,166 @@ pub fn any_super_class( inner(class_def, semantic, func, &mut FxHashSet::default()) } +#[derive(Clone, Debug)] +pub struct ClassMemberDeclaration<'a> { + kind: ClassMemberKind<'a>, + boundness: ClassMemberBoundness, +} + +impl<'a> ClassMemberDeclaration<'a> { + pub fn kind(&self) -> &ClassMemberKind<'a> { + &self.kind + } + + pub fn boundness(&self) -> ClassMemberBoundness { + self.boundness + } +} + +#[derive(Copy, Clone, Debug, Eq, PartialEq)] +pub enum ClassMemberBoundness { + PossiblyUnbound, + Bound, +} + +impl ClassMemberBoundness { + pub const fn is_bound(self) -> bool { + matches!(self, Self::Bound) + } + + pub const fn is_possibly_unbound(self) -> bool { + matches!(self, Self::PossiblyUnbound) + } +} + +#[derive(Copy, Clone, Debug)] +pub enum ClassMemberKind<'a> { + Assign(&'a ast::StmtAssign), + AnnAssign(&'a ast::StmtAnnAssign), + FunctionDef(&'a ast::StmtFunctionDef), +} + +pub fn any_member_declaration( + class: &ast::StmtClassDef, + func: &mut dyn FnMut(ClassMemberDeclaration) -> bool, +) -> bool { + fn any_stmt_in_body( + body: &[Stmt], + func: &mut dyn FnMut(ClassMemberDeclaration) -> bool, + boundness: ClassMemberBoundness, + ) -> bool { + body.iter().any(|stmt| { + let kind = match stmt { + Stmt::FunctionDef(function_def) => Some(ClassMemberKind::FunctionDef(function_def)), + Stmt::Assign(assign) => Some(ClassMemberKind::Assign(assign)), + Stmt::AnnAssign(assign) => Some(ClassMemberKind::AnnAssign(assign)), + Stmt::With(StmtWith { body, .. }) => { + if any_stmt_in_body(body, func, ClassMemberBoundness::PossiblyUnbound) { + return true; + } + + None + } + + Stmt::For(StmtFor { body, orelse, .. }) + | Stmt::While(StmtWhile { body, orelse, .. }) => { + if any_stmt_in_body(body, func, ClassMemberBoundness::PossiblyUnbound) + || any_stmt_in_body(orelse, func, ClassMemberBoundness::PossiblyUnbound) + { + return true; + } + + None + } + + Stmt::If(StmtIf { + body, + elif_else_clauses, + .. + }) => { + if any_stmt_in_body(body, func, ClassMemberBoundness::PossiblyUnbound) + || elif_else_clauses.iter().any(|it| { + any_stmt_in_body(&it.body, func, ClassMemberBoundness::PossiblyUnbound) + }) + { + return true; + } + None + } + + Stmt::Match(StmtMatch { cases, .. }) => { + if cases.iter().any(|it| { + any_stmt_in_body(&it.body, func, ClassMemberBoundness::PossiblyUnbound) + }) { + return true; + } + + None + } + + Stmt::Try(StmtTry { + body, + handlers, + orelse, + finalbody, + .. + }) => { + if any_stmt_in_body(body, func, ClassMemberBoundness::PossiblyUnbound) + || any_stmt_in_body(orelse, func, ClassMemberBoundness::PossiblyUnbound) + || any_stmt_in_body(finalbody, func, ClassMemberBoundness::PossiblyUnbound) + || handlers.iter().any(|ExceptHandler::ExceptHandler(it)| { + any_stmt_in_body(&it.body, func, ClassMemberBoundness::PossiblyUnbound) + }) + { + return true; + } + + None + } + // Technically, a method can be defined using a few more methods: + // + // ```python + // class C1: + // # Import + // import __eq__ # Callable module + // # ImportFrom + // from module import __eq__ # Top level callable + // # ExprNamed + // (__eq__ := lambda self, other: True) + // ``` + // + // Those cases are not yet supported because they're rare. + Stmt::ClassDef(_) + | Stmt::Return(_) + | Stmt::Delete(_) + | Stmt::AugAssign(_) + | Stmt::TypeAlias(_) + | Stmt::Raise(_) + | Stmt::Assert(_) + | Stmt::Import(_) + | Stmt::ImportFrom(_) + | Stmt::Global(_) + | Stmt::Nonlocal(_) + | Stmt::Expr(_) + | Stmt::Pass(_) + | Stmt::Break(_) + | Stmt::Continue(_) + | Stmt::IpyEscapeCommand(_) => None, + }; + + if let Some(kind) = kind { + if func(ClassMemberDeclaration { kind, boundness }) { + return true; + } + } + + false + }) + } + + any_stmt_in_body(&class.body, func, ClassMemberBoundness::Bound) +} + /// Return `true` if `class_def` is a class that has one or more enum classes in its mro pub fn is_enumeration(class_def: &ast::StmtClassDef, semantic: &SemanticModel) -> bool { any_qualified_base_class(class_def, semantic, &|qualified_name| {