[ruff] Abstract method defined in normal class (RUF044)

crates/ruff_linter/resources/test/fixtures/ruff/RUF044.py

@@ -0,0 +1,43 @@
+from abc import abstractmethod, abstractproperty, abstractstaticmethod, abstractclassmethod
+
+
+class NoBase:
+    @abstractmethod
+    def abstract_instance_method(self): ...
+
+    @abstractclassmethod
+    def abstract_class_method(cls): ...
+
+    @abstractstaticmethod
+    def abstract_static_method(): ...
+
+    @abstractproperty
+    def abstract_property(self): ...
+
+
+class HasBase(NoBase):
+    @abstractmethod
+    def abstract_instance_method(self): ...
+
+    @abstractclassmethod
+    def abstract_class_method(cls): ...
+
+    @abstractstaticmethod
+    def abstract_static_method(): ...
+
+    @abstractproperty
+    def abstract_property(self): ...
+
+
+class HasMetaclass(metaclass=HasBase):
+    @abstractmethod
+    def abstract_instance_method(self): ...
+
+    @abstractclassmethod
+    def abstract_class_method(cls): ...
+
+    @abstractstaticmethod
+    def abstract_static_method(): ...
+
+    @abstractproperty
+    def abstract_property(self): ...

crates/ruff_linter/resources/test/fixtures/ruff/RUF044_abc.py

@@ -0,0 +1,32 @@
+from abc import ABC, abstractmethod, abstractproperty, abstractstaticmethod, abstractclassmethod
+
+
+class A(ABC):
+    @abstractmethod
+    def abstract_instance_method(self): ...
+
+    @abstractclassmethod
+    def abstract_class_method(cls): ...
+
+    @abstractstaticmethod
+    def abstract_static_method(): ...
+
+    @abstractproperty
+    def abstract_property(self): ...
+
+
+class B: ...
+
+
+class C(B, A):
+    @abstractmethod
+    def abstract_instance_method(self): ...
+
+    @abstractclassmethod
+    def abstract_class_method(cls): ...
+
+    @abstractstaticmethod
+    def abstract_static_method(): ...
+
+    @abstractproperty
+    def abstract_property(self): ...

crates/ruff_linter/resources/test/fixtures/ruff/RUF044_abcmeta.py

@@ -0,0 +1,32 @@
+from abc import ABCMeta, abstractmethod, abstractproperty, abstractstaticmethod, abstractclassmethod
+
+
+class M(ABCMeta): ...
+
+
+class C(metaclass=M):
+    @abstractmethod
+    def abstract_instance_method(self): ...
+
+    @abstractclassmethod
+    def abstract_class_method(cls): ...
+
+    @abstractstaticmethod
+    def abstract_static_method(): ...
+
+    @abstractproperty
+    def abstract_property(self): ...
+
+
+class D(C):
+    @abstractmethod
+    def abstract_instance_method(self): ...
+
+    @abstractclassmethod
+    def abstract_class_method(cls): ...
+
+    @abstractstaticmethod
+    def abstract_static_method(): ...
+
+    @abstractproperty
+    def abstract_property(self): ...

crates/ruff_linter/src/checkers/ast/analyze/statement.rs

@@ -550,6 +550,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
             if checker.enabled(Rule::SubclassBuiltin) {
                 refurb::rules::subclass_builtin(checker, class_def);
             }
+            if checker.enabled(Rule::AbstractMethodInNormalClass) {
+                ruff::rules::abstract_method_in_normal_class(checker, class_def);
+            }
         }
         Stmt::Import(ast::StmtImport { names, range: _ }) => {
             if checker.enabled(Rule::MultipleImportsOnOneLine) {

crates/ruff_linter/src/codes.rs

@@ -986,6 +986,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
         (Ruff, "040") => (RuleGroup::Preview, rules::ruff::rules::InvalidAssertMessageLiteralArgument),
         (Ruff, "041") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryNestedLiteral),
         (Ruff, "043") => (RuleGroup::Preview, rules::ruff::rules::PytestRaisesAmbiguousPattern),
+        (Ruff, "044") => (RuleGroup::Preview, rules::ruff::rules::AbstractMethodInNormalClass),
         (Ruff, "046") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryCastToInt),
         (Ruff, "048") => (RuleGroup::Preview, rules::ruff::rules::MapIntVersionParsing),
         (Ruff, "051") => (RuleGroup::Preview, rules::ruff::rules::IfKeyInDictDel),

crates/ruff_linter/src/rules/ruff/mod.rs

@@ -416,6 +416,9 @@ mod tests {
     #[test_case(Rule::UnnecessaryRegularExpression, Path::new("RUF055_1.py"))]
     #[test_case(Rule::UnnecessaryCastToInt, Path::new("RUF046.py"))]
     #[test_case(Rule::PytestRaisesAmbiguousPattern, Path::new("RUF043.py"))]
+    #[test_case(Rule::AbstractMethodInNormalClass, Path::new("RUF044.py"))]
+    #[test_case(Rule::AbstractMethodInNormalClass, Path::new("RUF044_abc.py"))]
+    #[test_case(Rule::AbstractMethodInNormalClass, Path::new("RUF044_abcmeta.py"))]
     fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
         let snapshot = format!(
             "preview__{}_{}",

crates/ruff_linter/src/rules/ruff/rules/abstract_method_in_normal_class.rs

@@ -0,0 +1,239 @@
+use ruff_diagnostics::{Diagnostic, Violation};
+use ruff_macros::{derive_message_formats, ViolationMetadata};
+use ruff_python_ast::{Expr, Stmt, StmtClassDef, StmtFunctionDef};
+use ruff_python_semantic::analyze::class::any_base_class;
+use ruff_python_semantic::analyze::visibility::AbstractDecoratorKind;
+use ruff_python_semantic::{BindingKind, NodeRef, SemanticModel};
+
+use crate::checkers::ast::Checker;
+
+/// ## What it does
+/// Checks for methods decorated with at least one of `abc`'s
+/// `abstractmethod`, `abstractclassmethod`, `abstractstaticmethod` and `abstractproperty`
+/// but defined within a normal class's body.
+///
+/// ## Why is this bad?
+/// The abstract method decorators prevent users from instantiating abstract classes,
+/// or inheriting from abstract classes without implementing all abstract methods
+/// by throwing an exception. Such abstract method decorators are only effective
+/// in an abstract class.
+///
+/// For a mixin class, it is not enough that `abc.ABC` is included in the eventual MRO.
+/// The mixin class must also inherit directly from `ABC` for the decorators to take effect.
+///
+/// ```python
+/// from abc import ABC, abstractmethod
+///
+///
+/// class Base(ABC):
+///     @abstractmethod
+///     def hello(self) -> None: ...
+///
+///     def __repr__(self) -> str:
+///         return f"message={self.msg!r}"
+///
+///
+/// class Mixin:  # should be: `Mixin(ABC)`:
+///     @abstractmethod
+///     def world(self) -> None:
+///         self.msg += " goodbye"
+///
+///
+/// class FooBar(Mixin, Base):
+///     def __init__(self):
+///         self.msg = ""
+///
+///     def hello(self) -> None:
+///         self.msg += "hello"
+///
+///     # without `Mixin(ABC)`, omitting this does not raise an exception
+///     # def world(self) -> None:
+///     #     self.msg += " world"
+///
+///
+/// # `ABC` is part of the MRO
+/// print(FooBar.mro())  # [FooBar, Mixin, Base, ABC, object]
+///
+/// fb = FooBar()
+/// fb.hello()
+/// fb.world()
+/// print(str(fb))  # message='hello goodbye'
+/// ```
+///
+/// ## Known problems
+/// Does not check for base and metaclasses defined in different files.
+///
+/// ## Example
+///
+/// ```python
+/// from abc import abstractmethod
+///
+///
+/// class C:
+///     @abstractmethod
+///     def m(self) -> None:
+///         pass
+/// ```
+///
+/// Use instead:
+///
+/// ```python
+/// from abc import ABC, abstractmethod
+///
+///
+/// class C(ABC):
+///     @abstractmethod
+///     def m(self) -> None:
+///         pass
+/// ```
+#[derive(ViolationMetadata)]
+pub(crate) struct AbstractMethodInNormalClass {
+    decorator_kind: AbstractDecoratorKind,
+    class_name: String,
+}
+
+impl Violation for AbstractMethodInNormalClass {
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        "Abstract method defined in non-abstract class".to_string()
+    }
+
+    fn fix_title(&self) -> Option<String> {
+        let (decorator, class) = (&self.decorator_kind, &self.class_name);
+
+        Some(format!(
+            "Remove `@{decorator}` or make `{class}` inherit from `abc.ABC`"
+        ))
+    }
+}
+
+/// RUF044
+pub(crate) fn abstract_method_in_normal_class(checker: &mut Checker, class: &StmtClassDef) {
+    if checker.source_type.is_stub() {
+        return;
+    }
+
+    if might_be_abstract(class, checker.semantic()) {
+        return;
+    }
+
+    for stmt in &class.body {
+        check_class_body_stmt(checker, &class.name, stmt);
+    }
+}
+
+/// Returns false if the class is definitely not an abstract class.
+///
+/// A class is considered abstract when it inherits from a class
+/// created by `abc.ABCMeta` without implementing all abstract methods.
+///
+/// Thus, a class is *not* abstract when all of its bases are:
+/// * Inspectable
+/// * Do not have a metaclass that inherits from `abc.ABCMeta`
+fn might_be_abstract(class_def: &StmtClassDef, semantic: &SemanticModel) -> bool {
+    if metaclass_might_be_abcmeta(class_def, semantic) {
+        return true;
+    }
+
+    any_base_class(class_def, semantic, &mut |base| {
+        if is_abc_abc(base, semantic) {
+            return true;
+        }
+
+        let Some(base_def) = find_class_def(base, semantic) else {
+            return true;
+        };
+
+        metaclass_might_be_abcmeta(base_def, semantic)
+    })
+}
+
+/// Returns false if the class is definitely not `abc.ABCMeta` or a subclass thereof.
+///
+/// A class is *not* a subclass of `ABCMeta` when all of its bases are:
+/// * Inspectable
+/// * Do not inherit from `ABCMeta`
+fn metaclass_might_be_abcmeta(class_def: &StmtClassDef, semantic: &SemanticModel) -> bool {
+    let Some(arguments) = class_def.arguments.as_ref() else {
+        return false;
+    };
+
+    let Some(metaclass) = arguments.find_keyword("metaclass") else {
+        return false;
+    };
+    let metaclass = &metaclass.value;
+
+    if is_abc_abcmeta(metaclass, semantic) {
+        return true;
+    }
+
+    let Some(metaclass_def) = find_class_def(metaclass, semantic) else {
+        return true;
+    };
+
+    any_base_class(metaclass_def, semantic, &mut |base| {
+        is_abc_abcmeta(base, semantic) || find_class_def(base, semantic).is_none()
+    })
+}
+
+fn is_abc_abc(base: &Expr, semantic: &SemanticModel) -> bool {
+    let Some(qualified_name) = semantic.resolve_qualified_name(base) else {
+        return false;
+    };
+
+    matches!(qualified_name.segments(), ["abc", "ABC"])
+}
+
+fn is_abc_abcmeta(base: &Expr, semantic: &SemanticModel) -> bool {
+    let Some(qualified_name) = semantic.resolve_qualified_name(base) else {
+        return false;
+    };
+
+    matches!(qualified_name.segments(), ["abc", "ABCMeta"])
+}
+
+fn find_class_def<'a>(expr: &'a Expr, semantic: &'a SemanticModel) -> Option<&'a StmtClassDef> {
+    let name = expr.as_name_expr()?;
+    let binding_id = semantic.only_binding(name)?;
+
+    let binding = semantic.binding(binding_id);
+
+    if !matches!(binding.kind, BindingKind::ClassDefinition(_)) {
+        return None;
+    }
+
+    let node_id = binding.source?;
+    let node = semantic.node(node_id);
+
+    let NodeRef::Stmt(Stmt::ClassDef(base_def)) = node else {
+        return None;
+    };
+
+    Some(base_def)
+}
+
+fn check_class_body_stmt(checker: &mut Checker, class_name: &str, stmt: &Stmt) {
+    let Stmt::FunctionDef(StmtFunctionDef {
+        decorator_list,
+        name,
+        ..
+    }) = stmt
+    else {
+        return;
+    };
+
+    let Some(decorator_kind) =
+        AbstractDecoratorKind::from_decorators(decorator_list, checker.semantic())
+    else {
+        return;
+    };
+
+    let class_name = class_name.to_string();
+    let kind = AbstractMethodInNormalClass {
+        decorator_kind,
+        class_name,
+    };
+    let diagnostic = Diagnostic::new(kind, name.range);
+
+    checker.diagnostics.push(diagnostic);
+}

crates/ruff_linter/src/rules/ruff/rules/mod.rs

@@ -1,3 +1,4 @@
+pub(crate) use abstract_method_in_normal_class::*;
 pub(crate) use ambiguous_unicode_character::*;
 pub(crate) use assert_with_print_message::*;
 pub(crate) use assignment_in_assert::*;
@@ -45,6 +46,7 @@ pub(crate) use used_dummy_variable::*;
 pub(crate) use useless_if_else::*;
 pub(crate) use zip_instead_of_pairwise::*;
 
+mod abstract_method_in_normal_class;
 mod ambiguous_unicode_character;
 mod assert_with_print_message;
 mod assignment_in_assert;