[flake8-pyi] Implement PYI029 (#4851)

crates/ruff/resources/test/fixtures/flake8_pyi/PYI029.py

@@ -0,0 +1,57 @@
+import builtins
+from abc import abstractmethod
+
+
+def __repr__(self) -> str:
+    ...
+
+
+def __str__(self) -> builtins.str:
+    ...
+
+
+def __repr__(self, /, foo) -> str:
+    ...
+
+
+def __repr__(self, *, foo) -> str:
+    ...
+
+
+class ShouldRemoveSingle:
+    def __str__(self) -> builtins.str:
+        ...
+
+
+class ShouldRemove:
+    def __repr__(self) -> str:
+        ...
+
+    def __str__(self) -> builtins.str:
+        ...
+
+
+class NoReturnSpecified:
+    def __str__(self):
+        ...
+
+    def __repr__(self):
+        ...
+
+
+class NonMatchingArgs:
+    def __str__(self, *, extra) -> builtins.str:
+        ...
+
+    def __repr__(self, /, extra) -> str:
+        ...
+
+
+class MatchingArgsButAbstract:
+    @abstractmethod
+    def __str__(self) -> builtins.str:
+        ...
+
+    @abstractmethod
+    def __repr__(self) -> str:
+        ...

crates/ruff/resources/test/fixtures/flake8_pyi/PYI029.pyi

@@ -0,0 +1,28 @@
+import builtins
+from abc import abstractmethod
+
+def __repr__(self) -> str: ...
+def __str__(self) -> builtins.str: ...
+def __repr__(self, /, foo) -> str: ...
+def __repr__(self, *, foo) -> str: ...
+
+class ShouldRemoveSingle:
+    def __str__(self) -> builtins.str: ...  # Error: PYI029
+
+class ShouldRemove:
+    def __repr__(self) -> str: ...  # Error: PYI029
+    def __str__(self) -> builtins.str: ...  # Error: PYI029
+
+class NoReturnSpecified:
+    def __str__(self): ...
+    def __repr__(self): ...
+
+class NonMatchingArgs:
+    def __str__(self, *, extra) -> builtins.str: ...
+    def __repr__(self, /, extra) -> str: ...
+
+class MatchingArgsButAbstract:
+    @abstractmethod
+    def __str__(self) -> builtins.str: ...
+    @abstractmethod
+    def __repr__(self) -> str: ...

crates/ruff/src/checkers/ast/mod.rs

@@ -451,6 +451,9 @@ where
                             stmt.is_async_function_def_stmt(),
                         );
                     }
+                    if self.enabled(Rule::StrOrReprDefinedInStub) {
+                        flake8_pyi::rules::str_or_repr_defined_in_stub(self, stmt);
+                    }
                 }
 
                 if self.enabled(Rule::DunderFunctionName) {

crates/ruff/src/codes.rs

@@ -605,6 +605,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
         (Flake8Pyi, "021") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::DocstringInStub),
         (Flake8Pyi, "024") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::CollectionsNamedTuple),
         (Flake8Pyi, "025") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::UnaliasedCollectionsAbcSetImport),
+        (Flake8Pyi, "029") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::StrOrReprDefinedInStub),
         (Flake8Pyi, "032") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::AnyEqNeAnnotation),
         (Flake8Pyi, "033") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::TypeCommentInStub),
         (Flake8Pyi, "034") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::NonSelfReturnType),

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

@@ -46,6 +46,8 @@ mod tests {
     #[test_case(Rule::SnakeCaseTypeAlias, Path::new("PYI042.pyi"))]
     #[test_case(Rule::UnassignedSpecialVariableInStub, Path::new("PYI035.py"))]
     #[test_case(Rule::UnassignedSpecialVariableInStub, Path::new("PYI035.pyi"))]
+    #[test_case(Rule::StrOrReprDefinedInStub, Path::new("PYI029.py"))]
+    #[test_case(Rule::StrOrReprDefinedInStub, Path::new("PYI029.pyi"))]
     #[test_case(Rule::StubBodyMultipleStatements, Path::new("PYI048.py"))]
     #[test_case(Rule::StubBodyMultipleStatements, Path::new("PYI048.pyi"))]
     #[test_case(Rule::TSuffixedTypeAlias, Path::new("PYI043.py"))]

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

@@ -24,6 +24,7 @@ pub(crate) use simple_defaults::{
     unassigned_special_variable_in_stub, ArgumentDefaultInStub, AssignmentDefaultInStub,
     TypedArgumentDefaultInStub, UnannotatedAssignmentInStub, UnassignedSpecialVariableInStub,
 };
+pub(crate) use str_or_repr_defined_in_stub::{str_or_repr_defined_in_stub, StrOrReprDefinedInStub};
 pub(crate) use string_or_bytes_too_long::{string_or_bytes_too_long, StringOrBytesTooLong};
 pub(crate) use stub_body_multiple_statements::{
     stub_body_multiple_statements, StubBodyMultipleStatements,
@@ -54,6 +55,7 @@ mod pass_statement_stub_body;
 mod prefix_type_params;
 mod quoted_annotation_in_stub;
 mod simple_defaults;
+mod str_or_repr_defined_in_stub;
 mod string_or_bytes_too_long;
 mod stub_body_multiple_statements;
 mod type_alias_naming;

crates/ruff/src/rules/flake8_pyi/rules/str_or_repr_defined_in_stub.rs

@@ -0,0 +1,110 @@
+use rustpython_parser::ast;
+use rustpython_parser::ast::Stmt;
+
+use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Fix};
+use ruff_macros::{derive_message_formats, violation};
+use ruff_python_ast::helpers::identifier_range;
+use ruff_python_semantic::analyze::visibility::is_abstract;
+
+use crate::autofix::edits::delete_stmt;
+use crate::checkers::ast::Checker;
+use crate::registry::AsRule;
+
+/// ## What it does
+/// Checks for redundant definitions of `__str__` or `__repr__` in stubs.
+///
+/// ## Why is this bad?
+/// Defining `__str__` or `__repr__` in a stub is almost always redundant,
+/// as the signatures are almost always identical to those of the default
+/// equivalent, `object.__str__` and `object.__repr__`, respectively.
+///
+/// ## Example
+/// ```python
+/// class Foo:
+///     def __repr__(self) -> str:
+///         ...
+/// ```
+#[violation]
+pub struct StrOrReprDefinedInStub {
+    name: String,
+}
+
+impl AlwaysAutofixableViolation for StrOrReprDefinedInStub {
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        let StrOrReprDefinedInStub { name } = self;
+        format!("Defining `{name}` in a stub is almost always redundant")
+    }
+
+    fn autofix_title(&self) -> String {
+        let StrOrReprDefinedInStub { name } = self;
+        format!("Remove definition of `{name}`")
+    }
+}
+
+/// PYI029
+pub(crate) fn str_or_repr_defined_in_stub(checker: &mut Checker, stmt: &Stmt) {
+    let Stmt::FunctionDef(ast::StmtFunctionDef {
+        name,
+        decorator_list,
+        returns,
+        args,
+        ..
+    }) = stmt else {
+        return
+    };
+
+    let Some(returns) = returns else {
+        return;
+    };
+
+    if !matches!(name.as_str(), "__str__" | "__repr__") {
+        return;
+    }
+
+    if !checker.semantic_model().scope().kind.is_class() {
+        return;
+    }
+
+    // It is a violation only if the method signature matches that of `object.__str__`
+    // or `object.__repr__` exactly and the method is not decorated as abstract.
+    if !args.kwonlyargs.is_empty() || (args.args.len() + args.posonlyargs.len()) > 1 {
+        return;
+    }
+
+    if is_abstract(checker.semantic_model(), decorator_list) {
+        return;
+    }
+
+    if checker
+        .semantic_model()
+        .resolve_call_path(returns)
+        .map_or(true, |call_path| {
+            !matches!(call_path.as_slice(), ["" | "builtins", "str"])
+        })
+    {
+        return;
+    }
+
+    let mut diagnostic = Diagnostic::new(
+        StrOrReprDefinedInStub {
+            name: name.to_string(),
+        },
+        identifier_range(stmt, checker.locator),
+    );
+    if checker.patch(diagnostic.kind.rule()) {
+        let stmt = checker.semantic_model().stmt();
+        let parent = checker.semantic_model().stmt_parent();
+        let edit = delete_stmt(
+            stmt,
+            parent,
+            checker.locator,
+            checker.indexer,
+            checker.stylist,
+        );
+        diagnostic.set_fix(
+            Fix::automatic(edit).isolate(checker.isolation(checker.semantic_model().stmt_parent())),
+        );
+    }
+    checker.diagnostics.push(diagnostic);
+}

crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI029_PYI029.py.snap

@@ -0,0 +1,4 @@
+---
+source: crates/ruff/src/rules/flake8_pyi/mod.rs
+---
+

crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI029_PYI029.pyi.snap

@@ -0,0 +1,62 @@
+---
+source: crates/ruff/src/rules/flake8_pyi/mod.rs
+---
+PYI029.pyi:10:9: PYI029 [*] Defining `__str__` in a stub is almost always redundant
+   |
+10 | class ShouldRemoveSingle:
+11 |     def __str__(self) -> builtins.str: ...  # Error: PYI029
+   |         ^^^^^^^ PYI029
+12 | 
+13 | class ShouldRemove:
+   |
+   = help: Remove definition of `str`
+
+ℹ Fix
+7  7  | def __repr__(self, *, foo) -> str: ...
+8  8  | 
+9  9  | class ShouldRemoveSingle:
+10    |-    def __str__(self) -> builtins.str: ...  # Error: PYI029
+   10 |+    pass  # Error: PYI029
+11 11 | 
+12 12 | class ShouldRemove:
+13 13 |     def __repr__(self) -> str: ...  # Error: PYI029
+
+PYI029.pyi:13:9: PYI029 [*] Defining `__repr__` in a stub is almost always redundant
+   |
+13 | class ShouldRemove:
+14 |     def __repr__(self) -> str: ...  # Error: PYI029
+   |         ^^^^^^^^ PYI029
+15 |     def __str__(self) -> builtins.str: ...  # Error: PYI029
+   |
+   = help: Remove definition of `repr`
+
+ℹ Fix
+10 10 |     def __str__(self) -> builtins.str: ...  # Error: PYI029
+11 11 | 
+12 12 | class ShouldRemove:
+13    |-    def __repr__(self) -> str: ...  # Error: PYI029
+14 13 |     def __str__(self) -> builtins.str: ...  # Error: PYI029
+15 14 | 
+16 15 | class NoReturnSpecified:
+
+PYI029.pyi:14:9: PYI029 [*] Defining `__str__` in a stub is almost always redundant
+   |
+14 | class ShouldRemove:
+15 |     def __repr__(self) -> str: ...  # Error: PYI029
+16 |     def __str__(self) -> builtins.str: ...  # Error: PYI029
+   |         ^^^^^^^ PYI029
+17 | 
+18 | class NoReturnSpecified:
+   |
+   = help: Remove definition of `str`
+
+ℹ Fix
+11 11 | 
+12 12 | class ShouldRemove:
+13 13 |     def __repr__(self) -> str: ...  # Error: PYI029
+14    |-    def __str__(self) -> builtins.str: ...  # Error: PYI029
+15 14 | 
+16 15 | class NoReturnSpecified:
+17 16 |     def __str__(self): ...
+
+