From 60f18bd0ef7a0c469343b93a96414caba556076e Mon Sep 17 00:00:00 2001 From: epenet <6771947+epenet@users.noreply.github.com> Date: Mon, 8 Jul 2024 13:43:41 +0000 Subject: [PATCH 1/5] [flake8-return] Exempt properties (RET501) --- .../test/fixtures/flake8_return/RET501.py | 5 ++++ .../src/checkers/ast/analyze/statement.rs | 7 ++++- .../src/rules/flake8_return/rules/function.rs | 30 ++++++++++++++++--- 3 files changed, 37 insertions(+), 5 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_return/RET501.py b/crates/ruff_linter/resources/test/fixtures/flake8_return/RET501.py index 9bde6810cd7a1..57e814d70d6bd 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_return/RET501.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_return/RET501.py @@ -12,3 +12,8 @@ def get(self, key: str) -> str | None: def get(self, key: str) -> None: print(f"{key} not found") return None + + @property + def prop(self) -> None: + print("Property not found") + return None diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index e92fee7e8d45e..433653c92ecba 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -223,7 +223,12 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { Rule::SuperfluousElseContinue, Rule::SuperfluousElseBreak, ]) { - flake8_return::rules::function(checker, body, returns.as_ref().map(AsRef::as_ref)); + flake8_return::rules::function( + checker, + body, + decorator_list, + returns.as_ref().map(AsRef::as_ref), + ); } if checker.enabled(Rule::UselessReturn) { pylint::rules::useless_return( diff --git a/crates/ruff_linter/src/rules/flake8_return/rules/function.rs b/crates/ruff_linter/src/rules/flake8_return/rules/function.rs index 124abdfee953e..a1f0f400fe30e 100644 --- a/crates/ruff_linter/src/rules/flake8_return/rules/function.rs +++ b/crates/ruff_linter/src/rules/flake8_return/rules/function.rs @@ -9,7 +9,7 @@ use ruff_python_ast::helpers::{is_const_false, is_const_true}; use ruff_python_ast::stmt_if::elif_else_range; use ruff_python_ast::visitor::Visitor; use ruff_python_ast::whitespace::indentation; -use ruff_python_ast::{self as ast, ElifElseClause, Expr, Stmt}; +use ruff_python_ast::{self as ast, Decorator, ElifElseClause, Expr, Stmt}; use ruff_python_codegen::Stylist; use ruff_python_index::Indexer; use ruff_python_semantic::SemanticModel; @@ -364,7 +364,16 @@ impl Violation for SuperfluousElseBreak { } /// RET501 -fn unnecessary_return_none(checker: &mut Checker, stack: &Stack) { +fn unnecessary_return_none(checker: &mut Checker, decorator_list: &[Decorator], stack: &Stack) { + // Skip properties. + let semantic = checker.semantic(); + if decorator_list + .iter() + .any(|decorator| semantic.match_builtin_expr(&decorator.expression, "property")) + { + return; + } + for stmt in &stack.returns { let Some(expr) = stmt.value.as_deref() else { continue; @@ -731,7 +740,12 @@ fn superfluous_elif_else(checker: &mut Checker, stack: &Stack) { } /// Run all checks from the `flake8-return` plugin. -pub(crate) fn function(checker: &mut Checker, body: &[Stmt], returns: Option<&Expr>) { +pub(crate) fn function( + checker: &mut Checker, + body: &[Stmt], + decorator_list: &[Decorator], + returns: Option<&Expr>, +) { // Find the last statement in the function. let Some(last_stmt) = body.last() else { // Skip empty functions. @@ -785,9 +799,17 @@ pub(crate) fn function(checker: &mut Checker, body: &[Stmt], returns: Option<&Ex } } else { if checker.enabled(Rule::UnnecessaryReturnNone) { + // Skip properties. + let semantic = checker.semantic(); + if decorator_list + .iter() + .any(|decorator| semantic.match_builtin_expr(&decorator.expression, "property")) + { + return; + } // Skip functions that have a return annotation that is not `None`. if returns.map_or(true, Expr::is_none_literal_expr) { - unnecessary_return_none(checker, &stack); + unnecessary_return_none(checker, decorator_list, &stack); } } } From dc3773b8789c4a68d772d82dd0314a4f7ecc3d5b Mon Sep 17 00:00:00 2001 From: epenet <6771947+epenet@users.noreply.github.com> Date: Mon, 8 Jul 2024 15:57:49 +0200 Subject: [PATCH 2/5] Update snap --- ...ter__rules__flake8_return__tests__RET501_RET501.py.snap | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_return/snapshots/ruff_linter__rules__flake8_return__tests__RET501_RET501.py.snap b/crates/ruff_linter/src/rules/flake8_return/snapshots/ruff_linter__rules__flake8_return__tests__RET501_RET501.py.snap index 9705d137b6dd1..c8bb1d4aa1e69 100644 --- a/crates/ruff_linter/src/rules/flake8_return/snapshots/ruff_linter__rules__flake8_return__tests__RET501_RET501.py.snap +++ b/crates/ruff_linter/src/rules/flake8_return/snapshots/ruff_linter__rules__flake8_return__tests__RET501_RET501.py.snap @@ -26,6 +26,8 @@ RET501.py:14:9: RET501 [*] Do not explicitly `return None` in function if it is 13 | print(f"{key} not found") 14 | return None | ^^^^^^^^^^^ RET501 +15 | +16 | @property | = help: Remove explicit `return None` @@ -35,5 +37,6 @@ RET501.py:14:9: RET501 [*] Do not explicitly `return None` in function if it is 13 13 | print(f"{key} not found") 14 |- return None 14 |+ return - - +15 | +16 | @property +17 | def prop(self) -> None: From 225a4169c43ef633c65822d56d1e3f78b619442e Mon Sep 17 00:00:00 2001 From: epenet <6771947+epenet@users.noreply.github.com> Date: Mon, 8 Jul 2024 16:10:08 +0200 Subject: [PATCH 3/5] Remove duplicate code --- .../ruff_linter/src/rules/flake8_return/rules/function.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_return/rules/function.rs b/crates/ruff_linter/src/rules/flake8_return/rules/function.rs index a1f0f400fe30e..47607e014c484 100644 --- a/crates/ruff_linter/src/rules/flake8_return/rules/function.rs +++ b/crates/ruff_linter/src/rules/flake8_return/rules/function.rs @@ -799,14 +799,6 @@ pub(crate) fn function( } } else { if checker.enabled(Rule::UnnecessaryReturnNone) { - // Skip properties. - let semantic = checker.semantic(); - if decorator_list - .iter() - .any(|decorator| semantic.match_builtin_expr(&decorator.expression, "property")) - { - return; - } // Skip functions that have a return annotation that is not `None`. if returns.map_or(true, Expr::is_none_literal_expr) { unnecessary_return_none(checker, decorator_list, &stack); From 75e65254e5b1e5aacc5f21b085355ae91ff2a272 Mon Sep 17 00:00:00 2001 From: epenet <6771947+epenet@users.noreply.github.com> Date: Mon, 8 Jul 2024 16:10:14 +0200 Subject: [PATCH 4/5] Update snap --- ...er__rules__flake8_return__tests__RET501_RET501.py.snap | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_return/snapshots/ruff_linter__rules__flake8_return__tests__RET501_RET501.py.snap b/crates/ruff_linter/src/rules/flake8_return/snapshots/ruff_linter__rules__flake8_return__tests__RET501_RET501.py.snap index c8bb1d4aa1e69..889492cab6623 100644 --- a/crates/ruff_linter/src/rules/flake8_return/snapshots/ruff_linter__rules__flake8_return__tests__RET501_RET501.py.snap +++ b/crates/ruff_linter/src/rules/flake8_return/snapshots/ruff_linter__rules__flake8_return__tests__RET501_RET501.py.snap @@ -26,7 +26,7 @@ RET501.py:14:9: RET501 [*] Do not explicitly `return None` in function if it is 13 | print(f"{key} not found") 14 | return None | ^^^^^^^^^^^ RET501 -15 | +15 | 16 | @property | = help: Remove explicit `return None` @@ -37,6 +37,6 @@ RET501.py:14:9: RET501 [*] Do not explicitly `return None` in function if it is 13 13 | print(f"{key} not found") 14 |- return None 14 |+ return -15 | -16 | @property -17 | def prop(self) -> None: +15 15 | +16 16 | @property +17 17 | def prop(self) -> None: From 9e0853851c053bce2d3ac0ec50348a98ef7f23c0 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 8 Jul 2024 19:23:56 -0700 Subject: [PATCH 5/5] Move gate --- .../src/rules/flake8_return/rules/function.rs | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_return/rules/function.rs b/crates/ruff_linter/src/rules/flake8_return/rules/function.rs index 47607e014c484..0085f6bfe4b2b 100644 --- a/crates/ruff_linter/src/rules/flake8_return/rules/function.rs +++ b/crates/ruff_linter/src/rules/flake8_return/rules/function.rs @@ -365,15 +365,6 @@ impl Violation for SuperfluousElseBreak { /// RET501 fn unnecessary_return_none(checker: &mut Checker, decorator_list: &[Decorator], stack: &Stack) { - // Skip properties. - let semantic = checker.semantic(); - if decorator_list - .iter() - .any(|decorator| semantic.match_builtin_expr(&decorator.expression, "property")) - { - return; - } - for stmt in &stack.returns { let Some(expr) = stmt.value.as_deref() else { continue; @@ -381,7 +372,17 @@ fn unnecessary_return_none(checker: &mut Checker, decorator_list: &[Decorator], if !expr.is_none_literal_expr() { continue; } - let mut diagnostic = Diagnostic::new(UnnecessaryReturnNone, stmt.range); + + // Skip properties. + if decorator_list.iter().any(|decorator| { + checker + .semantic() + .match_builtin_expr(&decorator.expression, "property") + }) { + return; + } + + let mut diagnostic = Diagnostic::new(UnnecessaryReturnNone, stmt.range()); diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( "return".to_string(), stmt.range(), @@ -396,10 +397,10 @@ fn implicit_return_value(checker: &mut Checker, stack: &Stack) { if stmt.value.is_some() { continue; } - let mut diagnostic = Diagnostic::new(ImplicitReturnValue, stmt.range); + let mut diagnostic = Diagnostic::new(ImplicitReturnValue, stmt.range()); diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( "return None".to_string(), - stmt.range, + stmt.range(), ))); checker.diagnostics.push(diagnostic); }