[flake8-pyi] Implement PYI029

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: ...
+
+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/src/checkers/ast/mod.rs

@@ -460,6 +460,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

@@ -604,6 +604,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,113 @@
+use rustpython_parser::ast;
+use rustpython_parser::ast::{Ranged, Stmt};
+
+use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix};
+use ruff_macros::{derive_message_formats, violation};
+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?
+/// These definitions are redundant with `object.__str__` or `object.__repr__`.
+///
+/// ## 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
+    };
+
+    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;
+    }
+
+    let Some(returns) = returns else {
+        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(),
+        },
+        stmt.range(),
+    );
+
+    if checker.patch(diagnostic.kind.rule()) {
+        let mut edit = delete_stmt(
+            stmt,
+            checker.semantic_model().stmt_parent(),
+            checker.locator,
+            checker.indexer,
+            checker.stylist,
+        );
+
+        // If we removed the last statement, replace it with `...` instead of `pass` since we're
+        // editing a stub.
+        if edit.content() == Some("pass") {
+            edit = Edit::range_replacement("...".to_string(), edit.range());
+        }
+
+        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,61 @@
+---
+source: crates/ruff/src/rules/flake8_pyi/mod.rs
+---
+PYI029.pyi:17:5: PYI029 [*] Defining `__str__` in a stub is almost always redundant
+   |
+17 | class ShouldRemoveSingle:
+18 |     def __str__(self) -> builtins.str: ...
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI029
+19 | 
+20 | class ShouldRemove:
+   |
+   = help: Remove definition of `str`
+
+ℹ Fix
+14 14 | def __repr__(self, *, foo) -> str: ...
+15 15 | 
+16 16 | class ShouldRemoveSingle:
+17    |-    def __str__(self) -> builtins.str: ...
+   17 |+    ...
+18 18 | 
+19 19 | class ShouldRemove:
+20 20 |     def __repr__(self) -> str: ...
+
+PYI029.pyi:20:5: PYI029 [*] Defining `__repr__` in a stub is almost always redundant
+   |
+20 | class ShouldRemove:
+21 |     def __repr__(self) -> str: ...
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI029
+22 | 
+23 |     def __str__(self) -> builtins.str: ...
+   |
+   = help: Remove definition of `repr`
+
+ℹ Fix
+17 17 |     def __str__(self) -> builtins.str: ...
+18 18 | 
+19 19 | class ShouldRemove:
+20    |-    def __repr__(self) -> str: ...
+21 20 | 
+22 21 |     def __str__(self) -> builtins.str: ...
+23 22 | 
+
+PYI029.pyi:22:5: PYI029 [*] Defining `__str__` in a stub is almost always redundant
+   |
+22 |     def __repr__(self) -> str: ...
+23 | 
+24 |     def __str__(self) -> builtins.str: ...
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI029
+   |
+   = help: Remove definition of `str`
+
+ℹ Fix
+19 19 | class ShouldRemove:
+20 20 |     def __repr__(self) -> str: ...
+21 21 | 
+22    |-    def __str__(self) -> builtins.str: ...
+23 22 | 
+24 23 | 
+25 24 | class NoReturnSpecified:
+
+