[pylint] Detect nested methods correctly (PLW1641) (#15032)

Co-authored-by: Micha Reiser <micha@reiser.io>

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"

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,
+}
+
+impl EqHash {
+    fn from_class(class: &StmtClassDef) -> Self {
+        let (mut has_eq, mut has_hash) = (HasMethod::No, HasMethod::No);
+
+        any_member_declaration(class, &mut |declaration| {
+            let id = match declaration.kind() {
+                ClassMemberKind::Assign(StmtAssign { targets, .. }) => {
+                    let [Expr::Name(ExprName { id, .. })] = &targets[..] else {
+                        return false;
                     };
 
-                // Check if `__hash__` was explicitly set, as in:
-                // ```python
-                // class Class(SuperClass):
-                //     def __eq__(self, other):
-                //         return True
-                //
-                //     __hash__ = SuperClass.__hash__
-                // ```
+                    id
+                }
+                ClassMemberKind::AnnAssign(StmtAnnAssign { target, .. }) => {
+                    let Expr::Name(ExprName { id, .. }) = target.as_ref() else {
+                        return false;
+                    };
 
-                if id == "__hash__" {
-                    has_hash = true;
+                    id
                 }
-            }
-            Stmt::FunctionDef(ast::StmtFunctionDef { name, .. }) => match name.as_str() {
-                "__hash__" => has_hash = true,
-                "__eq__" => has_eq = true,
-                _ => {}
-            },
+                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<ClassMemberBoundness> for HasMethod {
+    fn from(value: ClassMemberBoundness) -> Self {
+        match value {
+            ClassMemberBoundness::PossiblyUnbound => Self::Maybe,
+            ClassMemberBoundness::Bound => Self::Yes,
+        }
+    }
+}
+
+impl BitOr<HasMethod> for HasMethod {
+    type Output = Self;
+
+    fn bitor(self, rhs: Self) -> Self::Output {
+        match (self, rhs) {
+            (HasMethod::No, _) => rhs,
+            (_, _) => self,
+        }
     }
-    has_eq && !has_hash
 }

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 |         ...
     |

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| {