From 5231dff8df534f0850624816aa80a2c7c570ac47 Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Sun, 28 May 2023 00:49:32 +0200 Subject: [PATCH 01/10] Add boilerplate and fixtures --- .../test/fixtures/flake8_pyi/PYI032.py | 24 ++++++++++++++++ .../test/fixtures/flake8_pyi/PYI032.pyi | 24 ++++++++++++++++ crates/ruff/src/codes.rs | 1 + crates/ruff/src/rules/flake8_pyi/mod.rs | 2 ++ .../flake8_pyi/rules/any_eq_ne_annotation.rs | 28 +++++++++++++++++++ crates/ruff/src/rules/flake8_pyi/rules/mod.rs | 2 ++ 6 files changed, 81 insertions(+) create mode 100644 crates/ruff/resources/test/fixtures/flake8_pyi/PYI032.py create mode 100644 crates/ruff/resources/test/fixtures/flake8_pyi/PYI032.pyi create mode 100644 crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs diff --git a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI032.py b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI032.py new file mode 100644 index 00000000000000..2d226ebe975f8c --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI032.py @@ -0,0 +1,24 @@ +from typing import Any +import typing + + +class Bad: + def __eq__(self, other: Any) -> bool: ... # Fine because not a stub file + def __ne__(self, other: typing.Any) -> typing.Any: ... # Fine because not a stub file + + +class Good: + def __eq__(self, other: object) -> bool: ... + + def __ne__(self, obj: object) -> int: ... + + +class WeirdButFine: + def __eq__(self, other: Any, strange_extra_arg: list[str]) -> Any: ... + def __ne__(self, *, kw_only_other: Any) -> bool: ... + + +class Unannotated: + def __eq__(self) -> Any: ... + def __ne__(self) -> bool: ... + diff --git a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI032.pyi b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI032.pyi new file mode 100644 index 00000000000000..82cb899e3b3231 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI032.pyi @@ -0,0 +1,24 @@ +from typing import Any +import typing + + +class Bad: + def __eq__(self, other: Any) -> bool: ... # Y032 + def __ne__(self, other: typing.Any) -> typing.Any: ... # Y032 + + +class Good: + def __eq__(self, other: object) -> bool: ... + + def __ne__(self, obj: object) -> int: ... + + +class WeirdButFine: + def __eq__(self, other: Any, strange_extra_arg: list[str]) -> Any: ... + def __ne__(self, *, kw_only_other: Any) -> bool: ... + + +class Unannotated: + def __eq__(self) -> Any: ... + def __ne__(self) -> bool: ... + diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 83cf2125728853..8fc12850a23ec7 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -590,6 +590,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8Pyi, "016") => (RuleGroup::Unspecified, Rule::DuplicateUnionMember), (Flake8Pyi, "020") => (RuleGroup::Unspecified, Rule::QuotedAnnotationInStub), (Flake8Pyi, "021") => (RuleGroup::Unspecified, Rule::DocstringInStub), + (Flake8Pyi, "032") => (RuleGroup::Unspecified, Rule::AnyEqNeAnnotation), (Flake8Pyi, "033") => (RuleGroup::Unspecified, Rule::TypeCommentInStub), (Flake8Pyi, "042") => (RuleGroup::Unspecified, Rule::SnakeCaseTypeAlias), (Flake8Pyi, "043") => (RuleGroup::Unspecified, Rule::TSuffixedTypeAlias), diff --git a/crates/ruff/src/rules/flake8_pyi/mod.rs b/crates/ruff/src/rules/flake8_pyi/mod.rs index ad62a00f8659c1..afa06d0089890d 100644 --- a/crates/ruff/src/rules/flake8_pyi/mod.rs +++ b/crates/ruff/src/rules/flake8_pyi/mod.rs @@ -12,6 +12,8 @@ mod tests { use crate::test::test_path; use crate::{assert_messages, settings}; + #[test_case(Rule::AnyEqNeNotation, Path::new("PYI032.py"))] + #[test_case(Rule::AnyEqNeNotation, Path::new("PYI032.py"))] #[test_case(Rule::ArgumentDefaultInStub, Path::new("PYI014.py"))] #[test_case(Rule::ArgumentDefaultInStub, Path::new("PYI014.pyi"))] #[test_case(Rule::AssignmentDefaultInStub, Path::new("PYI015.py"))] diff --git a/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs b/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs new file mode 100644 index 00000000000000..d3f488d412e126 --- /dev/null +++ b/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs @@ -0,0 +1,28 @@ +use rustpython_parser::ast::{self, Constant, Expr, Ranged, Stmt}; + +use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; +use ruff_macros::{derive_message_formats, violation}; + +use crate::checkers::ast::Checker; +use crate::registry::Rule; + +#[violation] +pub struct AnyEqNeNotation { + method: String, +} + +impl AlwaysAutofixableViolation for AnyEqNeNotation { + #[derive_message_formats] + fn message(&self) -> String { + format!("Prefer `object` to `Any` for the second parameter in {method}") + } + + fn autofix_title(&self) -> String { + format!("Replace `object` with `Any`") + } +} + +/// PYI032 +pub(crate) fn any_eq_ne_annotation(checker: &mut Checker, body: &[Stmt]) { + // TODO: Implement +} diff --git a/crates/ruff/src/rules/flake8_pyi/rules/mod.rs b/crates/ruff/src/rules/flake8_pyi/rules/mod.rs index 0c6b55d63451ca..029d6c71814fbe 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/mod.rs +++ b/crates/ruff/src/rules/flake8_pyi/rules/mod.rs @@ -1,3 +1,4 @@ +pub(crate) use any_eq_ne_annotation::{any_eq_ne_annotation, AnyEqNeNotation}; pub(crate) use bad_version_info_comparison::{ bad_version_info_comparison, BadVersionInfoComparison, }; @@ -27,6 +28,7 @@ pub(crate) use unrecognized_platform::{ unrecognized_platform, UnrecognizedPlatformCheck, UnrecognizedPlatformName, }; +mod any_eq_ne_annotation; mod bad_version_info_comparison; mod docstring_in_stubs; mod duplicate_union_member; From 591d82cfa830a47a182984b34bd1bcdbbcd1699b Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Sun, 28 May 2023 03:07:24 +0200 Subject: [PATCH 02/10] Add docs, add registry, add first steps to functionality --- crates/ruff/src/checkers/ast/mod.rs | 3 + crates/ruff/src/registry.rs | 1 + crates/ruff/src/rules/flake8_pyi/mod.rs | 4 +- .../flake8_pyi/rules/any_eq_ne_annotation.rs | 72 ++++++++++++++++--- crates/ruff/src/rules/flake8_pyi/rules/mod.rs | 2 +- 5 files changed, 70 insertions(+), 12 deletions(-) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 6bca01c629170c..6606e06e9f7f9f 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -429,6 +429,9 @@ where if self.enabled(Rule::StubBodyMultipleStatements) { flake8_pyi::rules::stub_body_multiple_statements(self, stmt, body); } + if self.enabled(Rule::AnyEqNeAnnotation) { + flake8_pyi::rules::any_eq_ne_annotation(self, stmt, name, args); + } } if self.enabled(Rule::DunderFunctionName) { diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index 3fb67075629fc6..53458771c463b6 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -509,6 +509,7 @@ ruff_macros::register_rules!( rules::flake8_errmsg::rules::FStringInException, rules::flake8_errmsg::rules::DotFormatInException, // flake8-pyi + rules::flake8_pyi::rules::AnyEqNeAnnotation, rules::flake8_pyi::rules::ArgumentDefaultInStub, rules::flake8_pyi::rules::AssignmentDefaultInStub, rules::flake8_pyi::rules::BadVersionInfoComparison, diff --git a/crates/ruff/src/rules/flake8_pyi/mod.rs b/crates/ruff/src/rules/flake8_pyi/mod.rs index afa06d0089890d..6cc38f9200f707 100644 --- a/crates/ruff/src/rules/flake8_pyi/mod.rs +++ b/crates/ruff/src/rules/flake8_pyi/mod.rs @@ -12,8 +12,8 @@ mod tests { use crate::test::test_path; use crate::{assert_messages, settings}; - #[test_case(Rule::AnyEqNeNotation, Path::new("PYI032.py"))] - #[test_case(Rule::AnyEqNeNotation, Path::new("PYI032.py"))] + #[test_case(Rule::AnyEqNeAnnotation, Path::new("PYI032.py"))] + #[test_case(Rule::AnyEqNeAnnotation, Path::new("PYI032.pyi"))] #[test_case(Rule::ArgumentDefaultInStub, Path::new("PYI014.py"))] #[test_case(Rule::ArgumentDefaultInStub, Path::new("PYI014.pyi"))] #[test_case(Rule::AssignmentDefaultInStub, Path::new("PYI015.py"))] diff --git a/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs b/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs index d3f488d412e126..76423da9a76ef4 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs +++ b/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs @@ -1,20 +1,45 @@ -use rustpython_parser::ast::{self, Constant, Expr, Ranged, Stmt}; +use rustpython_parser::ast::{Arguments, Stmt}; -use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; +use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::helpers::identifier_range; use crate::checkers::ast::Checker; -use crate::registry::Rule; +/// ## What it does +/// Checks for `Any` type annotations for the second parameter in `__ne__` and `__eq__` methods +/// +/// ## Why is this bad? +/// From the Python docs: `Use object to indicate that a value could be any type in a typesafe +/// manner. Use Any to indicate that a value is dynamically typed.` For `__eq__` and `__ne__` method +/// we want to do the former +/// +/// ## Example +/// ```python +/// class Foo: +/// def __eq__(self, obj: Any): +/// def __ne__(self, obj: typing.Any): +/// ``` +/// +/// Use instead: +/// ```python +/// class Foo: +/// def __eq__(self, obj: object): +/// def __ne__(self, obj: object): +/// ``` +/// ## References +/// - [Python Docs](https://docs.python.org/3/library/typing.html#the-any-type) +/// - [Mypy Docs](https://mypy.readthedocs.io/en/latest/dynamic_typing.html#any-vs-object) #[violation] -pub struct AnyEqNeNotation { - method: String, +pub struct AnyEqNeAnnotation { + method_name: String, } -impl AlwaysAutofixableViolation for AnyEqNeNotation { +impl AlwaysAutofixableViolation for AnyEqNeAnnotation { #[derive_message_formats] fn message(&self) -> String { - format!("Prefer `object` to `Any` for the second parameter in {method}") + let AnyEqNeAnnotation { method_name } = self; + format!("Prefer `object` to `Any` for the second parameter in {method_name}") } fn autofix_title(&self) -> String { @@ -23,6 +48,35 @@ impl AlwaysAutofixableViolation for AnyEqNeNotation { } /// PYI032 -pub(crate) fn any_eq_ne_annotation(checker: &mut Checker, body: &[Stmt]) { - // TODO: Implement +pub(crate) fn any_eq_ne_annotation( + checker: &mut Checker, + stmt: &Stmt, + name: &str, + args: &Arguments, +) { + if !checker.semantic_model().scope().kind.is_class() { + return; + } + + // Ignore non `__eq__` and `__ne__` methods + if name != "__eq__" && name != "__ne__" { + return; + } + + // Different numbers of arguments than 2 are handled by other rules + if args.args.len() == 2 { + if let Some(annotation) = &args.args[1].annotation { + if checker + .semantic_model() + .match_typing_expr(annotation, "Any") + { + checker.diagnostics.push(Diagnostic::new( + AnyEqNeAnnotation { + method_name: name.to_string(), + }, + identifier_range(stmt, checker.locator), + )); + } + } + } } diff --git a/crates/ruff/src/rules/flake8_pyi/rules/mod.rs b/crates/ruff/src/rules/flake8_pyi/rules/mod.rs index 029d6c71814fbe..cb14a41f2bf98d 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/mod.rs +++ b/crates/ruff/src/rules/flake8_pyi/rules/mod.rs @@ -1,4 +1,4 @@ -pub(crate) use any_eq_ne_annotation::{any_eq_ne_annotation, AnyEqNeNotation}; +pub(crate) use any_eq_ne_annotation::{any_eq_ne_annotation, AnyEqNeAnnotation}; pub(crate) use bad_version_info_comparison::{ bad_version_info_comparison, BadVersionInfoComparison, }; From 5d234dd3b54920d34d73fc3c79f031949ccbd9dd Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Sun, 28 May 2023 18:31:48 +0200 Subject: [PATCH 03/10] Fix violation logic --- .../flake8_pyi/rules/any_eq_ne_annotation.rs | 12 +++++------ ...__flake8_pyi__tests__PYI032_PYI032.py.snap | 4 ++++ ..._flake8_pyi__tests__PYI032_PYI032.pyi.snap | 20 +++++++++++++++++++ 3 files changed, 30 insertions(+), 6 deletions(-) create mode 100644 crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI032_PYI032.py.snap create mode 100644 crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI032_PYI032.pyi.snap diff --git a/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs b/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs index 76423da9a76ef4..a1ae2210549d09 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs +++ b/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs @@ -1,6 +1,6 @@ use rustpython_parser::ast::{Arguments, Stmt}; -use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic}; +use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::helpers::identifier_range; @@ -35,16 +35,16 @@ pub struct AnyEqNeAnnotation { method_name: String, } -impl AlwaysAutofixableViolation for AnyEqNeAnnotation { +impl Violation for AnyEqNeAnnotation { #[derive_message_formats] fn message(&self) -> String { let AnyEqNeAnnotation { method_name } = self; format!("Prefer `object` to `Any` for the second parameter in {method_name}") } - fn autofix_title(&self) -> String { - format!("Replace `object` with `Any`") - } + // fn autofix_title(&self) -> String { + // format!("Replace `object` with `Any`") + // } } /// PYI032 @@ -74,7 +74,7 @@ pub(crate) fn any_eq_ne_annotation( AnyEqNeAnnotation { method_name: name.to_string(), }, - identifier_range(stmt, checker.locator), + args.args[1].range, )); } } diff --git a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI032_PYI032.py.snap b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI032_PYI032.py.snap new file mode 100644 index 00000000000000..d1aa2e91165585 --- /dev/null +++ b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI032_PYI032.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__PYI032_PYI032.pyi.snap b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI032_PYI032.pyi.snap new file mode 100644 index 00000000000000..b9fbb0fc837f52 --- /dev/null +++ b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI032_PYI032.pyi.snap @@ -0,0 +1,20 @@ +--- +source: crates/ruff/src/rules/flake8_pyi/mod.rs +--- +PYI032.pyi:6:22: PYI032 Prefer `object` to `Any` for the second parameter in __eq__ + | +6 | class Bad: +7 | def __eq__(self, other: Any) -> bool: ... # Y032 + | ^^^^^^^^^^ PYI032 +8 | def __ne__(self, other: typing.Any) -> typing.Any: ... # Y032 + | + +PYI032.pyi:7:22: PYI032 Prefer `object` to `Any` for the second parameter in __ne__ + | +7 | class Bad: +8 | def __eq__(self, other: Any) -> bool: ... # Y032 +9 | def __ne__(self, other: typing.Any) -> typing.Any: ... # Y032 + | ^^^^^^^^^^^^^^^^^ PYI032 + | + + From b7e4585cfc3f9b076e88b4c017f4e1a1ba9aef6f Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Sun, 28 May 2023 19:07:59 +0200 Subject: [PATCH 04/10] Add autofix logic --- crates/ruff/src/checkers/ast/mod.rs | 2 +- .../flake8_pyi/rules/any_eq_ne_annotation.rs | 48 ++++++++++++------- ...ke8_pyi__tests__PYI032_PYI032.pyi.snap.new | 43 +++++++++++++++++ 3 files changed, 74 insertions(+), 19 deletions(-) create mode 100644 crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI032_PYI032.pyi.snap.new diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 6606e06e9f7f9f..4bacc82baee245 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -430,7 +430,7 @@ where flake8_pyi::rules::stub_body_multiple_statements(self, stmt, body); } if self.enabled(Rule::AnyEqNeAnnotation) { - flake8_pyi::rules::any_eq_ne_annotation(self, stmt, name, args); + flake8_pyi::rules::any_eq_ne_annotation(self, name, args); } } diff --git a/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs b/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs index a1ae2210549d09..7e3e980995a8b3 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs +++ b/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs @@ -1,10 +1,10 @@ -use rustpython_parser::ast::{Arguments, Stmt}; +use rustpython_parser::ast::Arguments; -use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Violation}; +use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::helpers::identifier_range; use crate::checkers::ast::Checker; +use crate::registry::AsRule; /// ## What it does /// Checks for `Any` type annotations for the second parameter in `__ne__` and `__eq__` methods @@ -12,7 +12,7 @@ use crate::checkers::ast::Checker; /// ## Why is this bad? /// From the Python docs: `Use object to indicate that a value could be any type in a typesafe /// manner. Use Any to indicate that a value is dynamically typed.` For `__eq__` and `__ne__` method -/// we want to do the former +/// we want to do the former. /// /// ## Example /// ```python @@ -35,47 +35,59 @@ pub struct AnyEqNeAnnotation { method_name: String, } -impl Violation for AnyEqNeAnnotation { +impl AlwaysAutofixableViolation for AnyEqNeAnnotation { #[derive_message_formats] fn message(&self) -> String { let AnyEqNeAnnotation { method_name } = self; format!("Prefer `object` to `Any` for the second parameter in {method_name}") } - // fn autofix_title(&self) -> String { - // format!("Replace `object` with `Any`") - // } + fn autofix_title(&self) -> String { + format!("Replace `object` with `Any`") + } } /// PYI032 -pub(crate) fn any_eq_ne_annotation( - checker: &mut Checker, - stmt: &Stmt, - name: &str, - args: &Arguments, -) { +pub(crate) fn any_eq_ne_annotation(checker: &mut Checker, name: &str, args: &Arguments) { if !checker.semantic_model().scope().kind.is_class() { return; } - // Ignore non `__eq__` and `__ne__` methods + // Ignore non `__eq__` and non `__ne__` methods if name != "__eq__" && name != "__ne__" { return; } - // Different numbers of arguments than 2 are handled by other rules + // Different numbers of arguments than 2 are handled by other rules if args.args.len() == 2 { if let Some(annotation) = &args.args[1].annotation { if checker .semantic_model() .match_typing_expr(annotation, "Any") { - checker.diagnostics.push(Diagnostic::new( + let mut diagnostic = Diagnostic::new( AnyEqNeAnnotation { method_name: name.to_string(), }, args.args[1].range, - )); + ); + if checker.patch(diagnostic.kind.rule()) { + // def __eq__(self, arg2: Any): ... + if let Some(name) = annotation.as_name_expr() { + diagnostic.set_fix(Fix::automatic(Edit::range_replacement( + format!("object"), + name.range, + ))); + } + // def __eq__(self, arg2: typing.Any): ... + if let Some(attr) = annotation.as_attribute_expr() { + diagnostic.set_fix(Fix::automatic(Edit::range_replacement( + format!("object"), + attr.range, + ))); + } + } + checker.diagnostics.push(diagnostic); } } } diff --git a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI032_PYI032.pyi.snap.new b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI032_PYI032.pyi.snap.new new file mode 100644 index 00000000000000..8c05170b61c083 --- /dev/null +++ b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI032_PYI032.pyi.snap.new @@ -0,0 +1,43 @@ +--- +source: crates/ruff/src/rules/flake8_pyi/mod.rs +assertion_line: 61 +--- +PYI032.pyi:6:22: PYI032 [*] Prefer `object` to `Any` for the second parameter in __eq__ + | +6 | class Bad: +7 | def __eq__(self, other: Any) -> bool: ... # Y032 + | ^^^^^^^^^^ PYI032 +8 | def __ne__(self, other: typing.Any) -> typing.Any: ... # Y032 + | + = help: Replace `object` with `Any` + +ℹ Fix +3 3 | +4 4 | +5 5 | class Bad: +6 |- def __eq__(self, other: Any) -> bool: ... # Y032 + 6 |+ def __eq__(self, other: object) -> bool: ... # Y032 +7 7 | def __ne__(self, other: typing.Any) -> typing.Any: ... # Y032 +8 8 | +9 9 | + +PYI032.pyi:7:22: PYI032 [*] Prefer `object` to `Any` for the second parameter in __ne__ + | +7 | class Bad: +8 | def __eq__(self, other: Any) -> bool: ... # Y032 +9 | def __ne__(self, other: typing.Any) -> typing.Any: ... # Y032 + | ^^^^^^^^^^^^^^^^^ PYI032 + | + = help: Replace `object` with `Any` + +ℹ Fix +4 4 | +5 5 | class Bad: +6 6 | def __eq__(self, other: Any) -> bool: ... # Y032 +7 |- def __ne__(self, other: typing.Any) -> typing.Any: ... # Y032 + 7 |+ def __ne__(self, other: object) -> typing.Any: ... # Y032 +8 8 | +9 9 | +10 10 | class Good: + + From 4049c305ea8c05694262aa4cb6b1a4fb92c7105f Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Sun, 28 May 2023 19:08:22 +0200 Subject: [PATCH 05/10] cargo insta review --- ..._flake8_pyi__tests__PYI032_PYI032.pyi.snap | 26 ++++++++++- ...ke8_pyi__tests__PYI032_PYI032.pyi.snap.new | 43 ------------------- 2 files changed, 24 insertions(+), 45 deletions(-) delete mode 100644 crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI032_PYI032.pyi.snap.new diff --git a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI032_PYI032.pyi.snap b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI032_PYI032.pyi.snap index b9fbb0fc837f52..6353f775a6c77d 100644 --- a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI032_PYI032.pyi.snap +++ b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI032_PYI032.pyi.snap @@ -1,20 +1,42 @@ --- source: crates/ruff/src/rules/flake8_pyi/mod.rs --- -PYI032.pyi:6:22: PYI032 Prefer `object` to `Any` for the second parameter in __eq__ +PYI032.pyi:6:22: PYI032 [*] Prefer `object` to `Any` for the second parameter in __eq__ | 6 | class Bad: 7 | def __eq__(self, other: Any) -> bool: ... # Y032 | ^^^^^^^^^^ PYI032 8 | def __ne__(self, other: typing.Any) -> typing.Any: ... # Y032 | + = help: Replace `object` with `Any` -PYI032.pyi:7:22: PYI032 Prefer `object` to `Any` for the second parameter in __ne__ +ℹ Fix +3 3 | +4 4 | +5 5 | class Bad: +6 |- def __eq__(self, other: Any) -> bool: ... # Y032 + 6 |+ def __eq__(self, other: object) -> bool: ... # Y032 +7 7 | def __ne__(self, other: typing.Any) -> typing.Any: ... # Y032 +8 8 | +9 9 | + +PYI032.pyi:7:22: PYI032 [*] Prefer `object` to `Any` for the second parameter in __ne__ | 7 | class Bad: 8 | def __eq__(self, other: Any) -> bool: ... # Y032 9 | def __ne__(self, other: typing.Any) -> typing.Any: ... # Y032 | ^^^^^^^^^^^^^^^^^ PYI032 | + = help: Replace `object` with `Any` + +ℹ Fix +4 4 | +5 5 | class Bad: +6 6 | def __eq__(self, other: Any) -> bool: ... # Y032 +7 |- def __ne__(self, other: typing.Any) -> typing.Any: ... # Y032 + 7 |+ def __ne__(self, other: object) -> typing.Any: ... # Y032 +8 8 | +9 9 | +10 10 | class Good: diff --git a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI032_PYI032.pyi.snap.new b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI032_PYI032.pyi.snap.new deleted file mode 100644 index 8c05170b61c083..00000000000000 --- a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI032_PYI032.pyi.snap.new +++ /dev/null @@ -1,43 +0,0 @@ ---- -source: crates/ruff/src/rules/flake8_pyi/mod.rs -assertion_line: 61 ---- -PYI032.pyi:6:22: PYI032 [*] Prefer `object` to `Any` for the second parameter in __eq__ - | -6 | class Bad: -7 | def __eq__(self, other: Any) -> bool: ... # Y032 - | ^^^^^^^^^^ PYI032 -8 | def __ne__(self, other: typing.Any) -> typing.Any: ... # Y032 - | - = help: Replace `object` with `Any` - -ℹ Fix -3 3 | -4 4 | -5 5 | class Bad: -6 |- def __eq__(self, other: Any) -> bool: ... # Y032 - 6 |+ def __eq__(self, other: object) -> bool: ... # Y032 -7 7 | def __ne__(self, other: typing.Any) -> typing.Any: ... # Y032 -8 8 | -9 9 | - -PYI032.pyi:7:22: PYI032 [*] Prefer `object` to `Any` for the second parameter in __ne__ - | -7 | class Bad: -8 | def __eq__(self, other: Any) -> bool: ... # Y032 -9 | def __ne__(self, other: typing.Any) -> typing.Any: ... # Y032 - | ^^^^^^^^^^^^^^^^^ PYI032 - | - = help: Replace `object` with `Any` - -ℹ Fix -4 4 | -5 5 | class Bad: -6 6 | def __eq__(self, other: Any) -> bool: ... # Y032 -7 |- def __ne__(self, other: typing.Any) -> typing.Any: ... # Y032 - 7 |+ def __ne__(self, other: object) -> typing.Any: ... # Y032 -8 8 | -9 9 | -10 10 | class Good: - - From 20d39ba7b36f8109ded572a14b2952496bee2d2f Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Sun, 28 May 2023 19:08:44 +0200 Subject: [PATCH 06/10] Update ruff.schema.json --- ruff.schema.json | 1 + 1 file changed, 1 insertion(+) diff --git a/ruff.schema.json b/ruff.schema.json index d3547d8c391fb4..ffc81fb0689905 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2205,6 +2205,7 @@ "PYI020", "PYI021", "PYI03", + "PYI032", "PYI033", "PYI04", "PYI042", From 6b00fc090e606d8d8da190833ad27febfb477d6c Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Sun, 28 May 2023 19:18:28 +0200 Subject: [PATCH 07/10] Add `...` to docs --- .../src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs b/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs index 7e3e980995a8b3..9ae2f7c2597c42 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs +++ b/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs @@ -17,15 +17,15 @@ use crate::registry::AsRule; /// ## Example /// ```python /// class Foo: -/// def __eq__(self, obj: Any): -/// def __ne__(self, obj: typing.Any): +/// def __eq__(self, obj: Any): ... +/// def __ne__(self, obj: typing.Any): ... /// ``` /// /// Use instead: /// ```python /// class Foo: -/// def __eq__(self, obj: object): -/// def __ne__(self, obj: object): +/// def __eq__(self, obj: object): ... +/// def __ne__(self, obj: object): ... /// ``` /// ## References /// - [Python Docs](https://docs.python.org/3/library/typing.html#the-any-type) From 7f69c5b06a2a74f5555eda6da62fb7c33c785727 Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Sun, 28 May 2023 19:21:57 +0200 Subject: [PATCH 08/10] Add indent to `...` --- .../rules/flake8_pyi/rules/any_eq_ne_annotation.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs b/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs index 9ae2f7c2597c42..ebaed081712303 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs +++ b/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs @@ -17,15 +17,19 @@ use crate::registry::AsRule; /// ## Example /// ```python /// class Foo: -/// def __eq__(self, obj: Any): ... -/// def __ne__(self, obj: typing.Any): ... +/// def __eq__(self, obj: Any): +/// ... +/// def __ne__(self, obj: typing.Any): +/// ... /// ``` /// /// Use instead: /// ```python /// class Foo: -/// def __eq__(self, obj: object): ... -/// def __ne__(self, obj: object): ... +/// def __eq__(self, obj: object): +/// ... +/// def __ne__(self, obj: object): +/// ... /// ``` /// ## References /// - [Python Docs](https://docs.python.org/3/library/typing.html#the-any-type) From 236fd55779d118b65176714aac5c3efe690d320f Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Sun, 28 May 2023 19:43:32 +0200 Subject: [PATCH 09/10] Add \n to docs --- crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs b/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs index ebaed081712303..211bb19cefae5e 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs +++ b/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs @@ -19,6 +19,7 @@ use crate::registry::AsRule; /// class Foo: /// def __eq__(self, obj: Any): /// ... +/// /// def __ne__(self, obj: typing.Any): /// ... /// ``` @@ -28,6 +29,7 @@ use crate::registry::AsRule; /// class Foo: /// def __eq__(self, obj: object): /// ... +/// /// def __ne__(self, obj: object): /// ... /// ``` From 3e0e6c86e392a02d2698c6ef9ce6713f3e23303a Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 28 May 2023 18:31:59 -0400 Subject: [PATCH 10/10] Tweak docs; shuffle around some calls --- .../flake8_pyi/rules/any_eq_ne_annotation.rs | 92 +++++++++---------- ..._flake8_pyi__tests__PYI032_PYI032.pyi.snap | 12 +-- 2 files changed, 49 insertions(+), 55 deletions(-) diff --git a/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs b/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs index 211bb19cefae5e..03235b31bee8c9 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs +++ b/crates/ruff/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs @@ -1,4 +1,4 @@ -use rustpython_parser::ast::Arguments; +use rustpython_parser::ast::{Arguments, Ranged}; use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; @@ -7,20 +7,22 @@ use crate::checkers::ast::Checker; use crate::registry::AsRule; /// ## What it does -/// Checks for `Any` type annotations for the second parameter in `__ne__` and `__eq__` methods +/// Checks for `__eq__` and `__ne__` implementations that use `typing.Any` as +/// the type annotation for the `obj` parameter. /// /// ## Why is this bad? -/// From the Python docs: `Use object to indicate that a value could be any type in a typesafe -/// manner. Use Any to indicate that a value is dynamically typed.` For `__eq__` and `__ne__` method -/// we want to do the former. +/// The Python documentation recommends the use of `object` to "indicate that a +/// value could be any type in a typesafe manner", while `Any` should be used to +/// "indicate that a value is dynamically typed." +/// +/// The semantics of `__eq__` and `__ne__` are such that the `obj` parameter +/// should be any type, as opposed to a dynamically typed value. Therefore, the +/// `object` type annotation is more appropriate. /// /// ## Example /// ```python /// class Foo: -/// def __eq__(self, obj: Any): -/// ... -/// -/// def __ne__(self, obj: typing.Any): +/// def __eq__(self, obj: typing.Any): /// ... /// ``` /// @@ -29,13 +31,10 @@ use crate::registry::AsRule; /// class Foo: /// def __eq__(self, obj: object): /// ... -/// -/// def __ne__(self, obj: object): -/// ... /// ``` /// ## References -/// - [Python Docs](https://docs.python.org/3/library/typing.html#the-any-type) -/// - [Mypy Docs](https://mypy.readthedocs.io/en/latest/dynamic_typing.html#any-vs-object) +/// - [Python documentation](https://docs.python.org/3/library/typing.html#the-any-type) +/// - [Mypy documentation](https://mypy.readthedocs.io/en/latest/dynamic_typing.html#any-vs-object) #[violation] pub struct AnyEqNeAnnotation { method_name: String, @@ -45,56 +44,51 @@ impl AlwaysAutofixableViolation for AnyEqNeAnnotation { #[derive_message_formats] fn message(&self) -> String { let AnyEqNeAnnotation { method_name } = self; - format!("Prefer `object` to `Any` for the second parameter in {method_name}") + format!("Prefer `object` to `Any` for the second parameter to `{method_name}`") } fn autofix_title(&self) -> String { - format!("Replace `object` with `Any`") + format!("Replace with `object`") } } /// PYI032 pub(crate) fn any_eq_ne_annotation(checker: &mut Checker, name: &str, args: &Arguments) { - if !checker.semantic_model().scope().kind.is_class() { + if !matches!(name, "__eq__" | "__ne__") { return; } - // Ignore non `__eq__` and non `__ne__` methods - if name != "__eq__" && name != "__ne__" { + if args.args.len() != 2 { + return; + } + + let Some(annotation) = &args.args[1].annotation else { + return; + }; + + if !checker.semantic_model().scope().kind.is_class() { return; } - // Different numbers of arguments than 2 are handled by other rules - if args.args.len() == 2 { - if let Some(annotation) = &args.args[1].annotation { - if checker - .semantic_model() - .match_typing_expr(annotation, "Any") - { - let mut diagnostic = Diagnostic::new( - AnyEqNeAnnotation { - method_name: name.to_string(), - }, - args.args[1].range, - ); - if checker.patch(diagnostic.kind.rule()) { - // def __eq__(self, arg2: Any): ... - if let Some(name) = annotation.as_name_expr() { - diagnostic.set_fix(Fix::automatic(Edit::range_replacement( - format!("object"), - name.range, - ))); - } - // def __eq__(self, arg2: typing.Any): ... - if let Some(attr) = annotation.as_attribute_expr() { - diagnostic.set_fix(Fix::automatic(Edit::range_replacement( - format!("object"), - attr.range, - ))); - } - } - checker.diagnostics.push(diagnostic); + if checker + .semantic_model() + .match_typing_expr(annotation, "Any") + { + let mut diagnostic = Diagnostic::new( + AnyEqNeAnnotation { + method_name: name.to_string(), + }, + annotation.range(), + ); + if checker.patch(diagnostic.kind.rule()) { + // Ex) `def __eq__(self, obj: Any): ...` + if checker.semantic_model().is_builtin("object") { + diagnostic.set_fix(Fix::automatic(Edit::range_replacement( + "object".to_string(), + annotation.range(), + ))); } } + checker.diagnostics.push(diagnostic); } } diff --git a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI032_PYI032.pyi.snap b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI032_PYI032.pyi.snap index 6353f775a6c77d..9f4ffac084ec3d 100644 --- a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI032_PYI032.pyi.snap +++ b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI032_PYI032.pyi.snap @@ -1,14 +1,14 @@ --- source: crates/ruff/src/rules/flake8_pyi/mod.rs --- -PYI032.pyi:6:22: PYI032 [*] Prefer `object` to `Any` for the second parameter in __eq__ +PYI032.pyi:6:29: PYI032 [*] Prefer `object` to `Any` for the second parameter to `__eq__` | 6 | class Bad: 7 | def __eq__(self, other: Any) -> bool: ... # Y032 - | ^^^^^^^^^^ PYI032 + | ^^^ PYI032 8 | def __ne__(self, other: typing.Any) -> typing.Any: ... # Y032 | - = help: Replace `object` with `Any` + = help: Replace with `object` ℹ Fix 3 3 | @@ -20,14 +20,14 @@ PYI032.pyi:6:22: PYI032 [*] Prefer `object` to `Any` for the second parameter in 8 8 | 9 9 | -PYI032.pyi:7:22: PYI032 [*] Prefer `object` to `Any` for the second parameter in __ne__ +PYI032.pyi:7:29: PYI032 [*] Prefer `object` to `Any` for the second parameter to `__ne__` | 7 | class Bad: 8 | def __eq__(self, other: Any) -> bool: ... # Y032 9 | def __ne__(self, other: typing.Any) -> typing.Any: ... # Y032 - | ^^^^^^^^^^^^^^^^^ PYI032 + | ^^^^^^^^^^ PYI032 | - = help: Replace `object` with `Any` + = help: Replace with `object` ℹ Fix 4 4 |