From 9361e22fe919f5ddc70dfd7ad35c24e0b89166f5 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Wed, 6 Dec 2023 20:47:36 -0600 Subject: [PATCH] Avoid `ANN2xx` autofix for abstract methods with empty body (#9034) ## Summary This PR updates the `ANN201`, `ANN202`, `ANN205`, and `ANN206` rules to not create a fix for the return type when it's an abstract method and the function body is empty i.e., it only contains either a pass statement, docstring or an ellipsis literal. fixes: #9004 ## Test Plan Add the following test cases: - Abstract method with pass statement - Abstract method with docstring - Abstract method with ellipsis literal - Abstract method with possible return type --- .../flake8_annotations/auto_return_type.py | 35 +++++ .../flake8_annotations/rules/definition.rs | 127 ++++++++++++------ ..._annotations__tests__auto_return_type.snap | 68 ++++++++++ ...tations__tests__auto_return_type_py38.snap | 68 ++++++++++ 4 files changed, 254 insertions(+), 44 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_annotations/auto_return_type.py b/crates/ruff_linter/resources/test/fixtures/flake8_annotations/auto_return_type.py index ab75a50da7..60d89cbec5 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_annotations/auto_return_type.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_annotations/auto_return_type.py @@ -147,3 +147,38 @@ def func(x: int): while x > 0: break return 1 + + +import abc +from abc import abstractmethod + + +class Foo(abc.ABC): + @abstractmethod + def method(self): + pass + + @abc.abstractmethod + def method(self): + """Docstring.""" + + @abc.abstractmethod + def method(self): + ... + + @staticmethod + @abstractmethod + def method(): + pass + + @classmethod + @abstractmethod + def method(cls): + pass + + @abstractmethod + def method(self): + if self.x > 0: + return 1 + else: + return 1.5 diff --git a/crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs b/crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs index 9360d0dfdd..dee00d6667 100644 --- a/crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs +++ b/crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs @@ -537,6 +537,19 @@ fn check_dynamically_typed( } } +fn is_empty_body(body: &[Stmt]) -> bool { + body.iter().all(|stmt| match stmt { + Stmt::Pass(_) => true, + Stmt::Expr(ast::StmtExpr { value, range: _ }) => { + matches!( + value.as_ref(), + Expr::StringLiteral(_) | Expr::EllipsisLiteral(_) + ) + } + _ => false, + }) +} + /// Generate flake8-annotation checks for a given `Definition`. pub(crate) fn definition( checker: &Checker, @@ -725,16 +738,22 @@ pub(crate) fn definition( ) { if is_method && visibility::is_classmethod(decorator_list, checker.semantic()) { if checker.enabled(Rule::MissingReturnTypeClassMethod) { - let return_type = auto_return_type(function) - .and_then(|return_type| { - return_type.into_expression( - checker.importer(), - function.parameters.start(), - checker.semantic(), - checker.settings.target_version, - ) - }) - .map(|(return_type, edits)| (checker.generator().expr(&return_type), edits)); + let return_type = if visibility::is_abstract(decorator_list, checker.semantic()) + && is_empty_body(body) + { + None + } else { + auto_return_type(function) + .and_then(|return_type| { + return_type.into_expression( + checker.importer(), + function.parameters.start(), + checker.semantic(), + checker.settings.target_version, + ) + }) + .map(|(return_type, edits)| (checker.generator().expr(&return_type), edits)) + }; let mut diagnostic = Diagnostic::new( MissingReturnTypeClassMethod { name: name.to_string(), @@ -752,16 +771,22 @@ pub(crate) fn definition( } } else if is_method && visibility::is_staticmethod(decorator_list, checker.semantic()) { if checker.enabled(Rule::MissingReturnTypeStaticMethod) { - let return_type = auto_return_type(function) - .and_then(|return_type| { - return_type.into_expression( - checker.importer(), - function.parameters.start(), - checker.semantic(), - checker.settings.target_version, - ) - }) - .map(|(return_type, edits)| (checker.generator().expr(&return_type), edits)); + let return_type = if visibility::is_abstract(decorator_list, checker.semantic()) + && is_empty_body(body) + { + None + } else { + auto_return_type(function) + .and_then(|return_type| { + return_type.into_expression( + checker.importer(), + function.parameters.start(), + checker.semantic(), + checker.settings.target_version, + ) + }) + .map(|(return_type, edits)| (checker.generator().expr(&return_type), edits)) + }; let mut diagnostic = Diagnostic::new( MissingReturnTypeStaticMethod { name: name.to_string(), @@ -818,18 +843,25 @@ pub(crate) fn definition( match visibility { visibility::Visibility::Public => { if checker.enabled(Rule::MissingReturnTypeUndocumentedPublicFunction) { - let return_type = auto_return_type(function) - .and_then(|return_type| { - return_type.into_expression( - checker.importer(), - function.parameters.start(), - checker.semantic(), - checker.settings.target_version, - ) - }) - .map(|(return_type, edits)| { - (checker.generator().expr(&return_type), edits) - }); + let return_type = + if visibility::is_abstract(decorator_list, checker.semantic()) + && is_empty_body(body) + { + None + } else { + auto_return_type(function) + .and_then(|return_type| { + return_type.into_expression( + checker.importer(), + function.parameters.start(), + checker.semantic(), + checker.settings.target_version, + ) + }) + .map(|(return_type, edits)| { + (checker.generator().expr(&return_type), edits) + }) + }; let mut diagnostic = Diagnostic::new( MissingReturnTypeUndocumentedPublicFunction { name: name.to_string(), @@ -853,18 +885,25 @@ pub(crate) fn definition( } visibility::Visibility::Private => { if checker.enabled(Rule::MissingReturnTypePrivateFunction) { - let return_type = auto_return_type(function) - .and_then(|return_type| { - return_type.into_expression( - checker.importer(), - function.parameters.start(), - checker.semantic(), - checker.settings.target_version, - ) - }) - .map(|(return_type, edits)| { - (checker.generator().expr(&return_type), edits) - }); + let return_type = + if visibility::is_abstract(decorator_list, checker.semantic()) + && is_empty_body(body) + { + None + } else { + auto_return_type(function) + .and_then(|return_type| { + return_type.into_expression( + checker.importer(), + function.parameters.start(), + checker.semantic(), + checker.settings.target_version, + ) + }) + .map(|(return_type, edits)| { + (checker.generator().expr(&return_type), edits) + }) + }; let mut diagnostic = Diagnostic::new( MissingReturnTypePrivateFunction { name: name.to_string(), diff --git a/crates/ruff_linter/src/rules/flake8_annotations/snapshots/ruff_linter__rules__flake8_annotations__tests__auto_return_type.snap b/crates/ruff_linter/src/rules/flake8_annotations/snapshots/ruff_linter__rules__flake8_annotations__tests__auto_return_type.snap index 6fcbb95d02..72b8e8b9a6 100644 --- a/crates/ruff_linter/src/rules/flake8_annotations/snapshots/ruff_linter__rules__flake8_annotations__tests__auto_return_type.snap +++ b/crates/ruff_linter/src/rules/flake8_annotations/snapshots/ruff_linter__rules__flake8_annotations__tests__auto_return_type.snap @@ -427,4 +427,72 @@ auto_return_type.py:146:5: ANN201 [*] Missing return type annotation for public 148 148 | break 149 149 | return 1 +auto_return_type.py:158:9: ANN201 Missing return type annotation for public function `method` + | +156 | class Foo(abc.ABC): +157 | @abstractmethod +158 | def method(self): + | ^^^^^^ ANN201 +159 | pass + | + = help: Add return type annotation + +auto_return_type.py:162:9: ANN201 Missing return type annotation for public function `method` + | +161 | @abc.abstractmethod +162 | def method(self): + | ^^^^^^ ANN201 +163 | """Docstring.""" + | + = help: Add return type annotation + +auto_return_type.py:166:9: ANN201 Missing return type annotation for public function `method` + | +165 | @abc.abstractmethod +166 | def method(self): + | ^^^^^^ ANN201 +167 | ... + | + = help: Add return type annotation + +auto_return_type.py:171:9: ANN205 Missing return type annotation for staticmethod `method` + | +169 | @staticmethod +170 | @abstractmethod +171 | def method(): + | ^^^^^^ ANN205 +172 | pass + | + = help: Add return type annotation + +auto_return_type.py:176:9: ANN206 Missing return type annotation for classmethod `method` + | +174 | @classmethod +175 | @abstractmethod +176 | def method(cls): + | ^^^^^^ ANN206 +177 | pass + | + = help: Add return type annotation + +auto_return_type.py:180:9: ANN201 [*] Missing return type annotation for public function `method` + | +179 | @abstractmethod +180 | def method(self): + | ^^^^^^ ANN201 +181 | if self.x > 0: +182 | return 1 + | + = help: Add return type annotation: `float` + +ℹ Unsafe fix +177 177 | pass +178 178 | +179 179 | @abstractmethod +180 |- def method(self): + 180 |+ def method(self) -> float: +181 181 | if self.x > 0: +182 182 | return 1 +183 183 | else: + diff --git a/crates/ruff_linter/src/rules/flake8_annotations/snapshots/ruff_linter__rules__flake8_annotations__tests__auto_return_type_py38.snap b/crates/ruff_linter/src/rules/flake8_annotations/snapshots/ruff_linter__rules__flake8_annotations__tests__auto_return_type_py38.snap index d91484ca3f..7667c2299d 100644 --- a/crates/ruff_linter/src/rules/flake8_annotations/snapshots/ruff_linter__rules__flake8_annotations__tests__auto_return_type_py38.snap +++ b/crates/ruff_linter/src/rules/flake8_annotations/snapshots/ruff_linter__rules__flake8_annotations__tests__auto_return_type_py38.snap @@ -482,4 +482,72 @@ auto_return_type.py:146:5: ANN201 [*] Missing return type annotation for public 148 149 | break 149 150 | return 1 +auto_return_type.py:158:9: ANN201 Missing return type annotation for public function `method` + | +156 | class Foo(abc.ABC): +157 | @abstractmethod +158 | def method(self): + | ^^^^^^ ANN201 +159 | pass + | + = help: Add return type annotation + +auto_return_type.py:162:9: ANN201 Missing return type annotation for public function `method` + | +161 | @abc.abstractmethod +162 | def method(self): + | ^^^^^^ ANN201 +163 | """Docstring.""" + | + = help: Add return type annotation + +auto_return_type.py:166:9: ANN201 Missing return type annotation for public function `method` + | +165 | @abc.abstractmethod +166 | def method(self): + | ^^^^^^ ANN201 +167 | ... + | + = help: Add return type annotation + +auto_return_type.py:171:9: ANN205 Missing return type annotation for staticmethod `method` + | +169 | @staticmethod +170 | @abstractmethod +171 | def method(): + | ^^^^^^ ANN205 +172 | pass + | + = help: Add return type annotation + +auto_return_type.py:176:9: ANN206 Missing return type annotation for classmethod `method` + | +174 | @classmethod +175 | @abstractmethod +176 | def method(cls): + | ^^^^^^ ANN206 +177 | pass + | + = help: Add return type annotation + +auto_return_type.py:180:9: ANN201 [*] Missing return type annotation for public function `method` + | +179 | @abstractmethod +180 | def method(self): + | ^^^^^^ ANN201 +181 | if self.x > 0: +182 | return 1 + | + = help: Add return type annotation: `float` + +ℹ Unsafe fix +177 177 | pass +178 178 | +179 179 | @abstractmethod +180 |- def method(self): + 180 |+ def method(self) -> float: +181 181 | if self.x > 0: +182 182 | return 1 +183 183 | else: +