diff --git a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI029.py b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI029.py new file mode 100644 index 0000000000..20cc7d6ae5 --- /dev/null +++ b/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: + ... diff --git a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI029.pyi b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI029.pyi new file mode 100644 index 0000000000..a082a733a2 --- /dev/null +++ b/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: ... diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 80066088ee..f5e130f11a 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/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) { diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 211a9b215d..0f5effd416 100644 --- a/crates/ruff/src/codes.rs +++ b/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), diff --git a/crates/ruff/src/rules/flake8_pyi/mod.rs b/crates/ruff/src/rules/flake8_pyi/mod.rs index e92e12e003..7e27c598a5 100644 --- a/crates/ruff/src/rules/flake8_pyi/mod.rs +++ b/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"))] diff --git a/crates/ruff/src/rules/flake8_pyi/rules/mod.rs b/crates/ruff/src/rules/flake8_pyi/rules/mod.rs index 4f20a04c76..8b897e7ad5 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/mod.rs +++ b/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; diff --git a/crates/ruff/src/rules/flake8_pyi/rules/str_or_repr_defined_in_stub.rs b/crates/ruff/src/rules/flake8_pyi/rules/str_or_repr_defined_in_stub.rs new file mode 100644 index 0000000000..88f204fcc3 --- /dev/null +++ b/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); +} diff --git a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI029_PYI029.py.snap b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI029_PYI029.py.snap new file mode 100644 index 0000000000..d1aa2e9116 --- /dev/null +++ b/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 +--- + diff --git a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI029_PYI029.pyi.snap b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI029_PYI029.pyi.snap new file mode 100644 index 0000000000..b1c616ccb4 --- /dev/null +++ b/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): ... + + diff --git a/ruff.schema.json b/ruff.schema.json index d5b7fa3b25..f99ffe9dee 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2242,6 +2242,7 @@ "PYI021", "PYI024", "PYI025", + "PYI029", "PYI03", "PYI032", "PYI033",