From cb491ea159a7d6e125c14a0bfe21486966e3d10d Mon Sep 17 00:00:00 2001 From: epenet <6771947+epenet@users.noreply.github.com> Date: Mon, 29 Jul 2024 06:22:54 +0000 Subject: [PATCH 1/6] [flake8-return] Exempt cached properties (RET501) --- .../resources/test/fixtures/flake8_return/RET501.py | 8 ++++++++ 1 file changed, 8 insertions(+) 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 57e814d70d6bd..5728ebcbfe269 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_return/RET501.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_return/RET501.py @@ -1,3 +1,6 @@ +from functools import cached_property + + def x(y): if not y: return @@ -17,3 +20,8 @@ def get(self, key: str) -> None: def prop(self) -> None: print("Property not found") return None + + @cached_property + def prop(self) -> None: + print("Property not found") + return None From 2f30854ac825a6591b5d3fc38d93ad2b96014a76 Mon Sep 17 00:00:00 2001 From: epenet <6771947+epenet@users.noreply.github.com> Date: Mon, 29 Jul 2024 06:35:54 +0000 Subject: [PATCH 2/6] Update snap --- ...lake8_return__tests__RET501_RET501.py.snap | 52 +++++++++---------- 1 file changed, 26 insertions(+), 26 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 889492cab6623..2cece7ec25311 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 @@ -1,42 +1,42 @@ --- source: crates/ruff_linter/src/rules/flake8_return/mod.rs --- -RET501.py:4:5: RET501 [*] Do not explicitly `return None` in function if it is the only possible return value +RET501.py:7:5: RET501 [*] Do not explicitly `return None` in function if it is the only possible return value | -2 | if not y: -3 | return -4 | return None # error +5 | if not y: +6 | return +7 | return None # error | ^^^^^^^^^^^ RET501 | = help: Remove explicit `return None` ℹ Safe fix -1 1 | def x(y): -2 2 | if not y: -3 3 | return -4 |- return None # error - 4 |+ return # error -5 5 | -6 6 | -7 7 | class BaseCache: +4 4 | def x(y): +5 5 | if not y: +6 6 | return +7 |- return None # error + 7 |+ return # error +8 8 | +9 9 | +10 10 | class BaseCache: -RET501.py:14:9: RET501 [*] Do not explicitly `return None` in function if it is the only possible return value +RET501.py:17:9: RET501 [*] Do not explicitly `return None` in function if it is the only possible return value | -12 | def get(self, key: str) -> None: -13 | print(f"{key} not found") -14 | return None +15 | def get(self, key: str) -> None: +16 | print(f"{key} not found") +17 | return None | ^^^^^^^^^^^ RET501 -15 | -16 | @property +18 | +19 | @property | = help: Remove explicit `return None` ℹ Safe fix -11 11 | -12 12 | def get(self, key: str) -> None: -13 13 | print(f"{key} not found") -14 |- return None - 14 |+ return -15 15 | -16 16 | @property -17 17 | def prop(self) -> None: +14 14 | +15 15 | def get(self, key: str) -> None: +16 16 | print(f"{key} not found") +17 |- return None + 17 |+ return +18 18 | +19 19 | @property +20 20 | def prop(self) -> None: From ac94d4aa26a01fa4da869f46df899fa9a03015bb Mon Sep 17 00:00:00 2001 From: epenet <6771947+epenet@users.noreply.github.com> Date: Mon, 29 Jul 2024 07:02:06 +0000 Subject: [PATCH 3/6] Implement rule --- .../src/rules/flake8_return/rules/function.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 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 0085f6bfe4b2b..ddfe2cf1afe3e 100644 --- a/crates/ruff_linter/src/rules/flake8_return/rules/function.rs +++ b/crates/ruff_linter/src/rules/flake8_return/rules/function.rs @@ -5,7 +5,7 @@ use anyhow::Result; use ruff_diagnostics::{AlwaysFixableViolation, FixAvailability, Violation}; use ruff_diagnostics::{Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::helpers::{is_const_false, is_const_true}; +use ruff_python_ast::helpers::{is_const_false, is_const_true, map_callable}; use ruff_python_ast::stmt_if::elif_else_range; use ruff_python_ast::visitor::Visitor; use ruff_python_ast::whitespace::indentation; @@ -373,11 +373,17 @@ fn unnecessary_return_none(checker: &mut Checker, decorator_list: &[Decorator], continue; } - // Skip properties. + // Skip properties and cached properties. if decorator_list.iter().any(|decorator| { checker .semantic() - .match_builtin_expr(&decorator.expression, "property") + .resolve_qualified_name(map_callable(&decorator.expression)) + .is_some_and(|qualified_name| { + matches!( + qualified_name.segments(), + ["" | "builtins", "property"] | ["functools", "cached_property"] + ) + }) }) { return; } From d5fb0cb6a463ac709a77b546586a8f71a0cd3874 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Tue, 30 Jul 2024 11:29:14 +0100 Subject: [PATCH 4/6] Account for other property-like decorators as well --- .../test/fixtures/flake8_return/RET501.py | 23 ++++++-- .../src/rules/flake8_return/rules/function.rs | 38 +++++++++----- ...lake8_return__tests__RET501_RET501.py.snap | 52 +++++++++---------- 3 files changed, 71 insertions(+), 42 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 5728ebcbfe269..ddf43c179534a 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_return/RET501.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_return/RET501.py @@ -1,6 +1,3 @@ -from functools import cached_property - - def x(y): if not y: return @@ -21,7 +18,27 @@ def prop(self) -> None: print("Property not found") return None + +import abc +import enum +import types +from functools import cached_property + + +class BaseCache2: @cached_property def prop(self) -> None: print("Property not found") return None + + @abc.abstractproperty + def prop2(self) -> None: + return None + + @types.DynamicClassAttribute + def prop3(self) -> None: + return None + + @enum.property + def prop4(self) -> None: + return None 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 ddfe2cf1afe3e..69cc90c882e01 100644 --- a/crates/ruff_linter/src/rules/flake8_return/rules/function.rs +++ b/crates/ruff_linter/src/rules/flake8_return/rules/function.rs @@ -5,7 +5,7 @@ use anyhow::Result; use ruff_diagnostics::{AlwaysFixableViolation, FixAvailability, Violation}; use ruff_diagnostics::{Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::helpers::{is_const_false, is_const_true, map_callable}; +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; @@ -373,18 +373,11 @@ fn unnecessary_return_none(checker: &mut Checker, decorator_list: &[Decorator], continue; } - // Skip properties and cached properties. - if decorator_list.iter().any(|decorator| { - checker - .semantic() - .resolve_qualified_name(map_callable(&decorator.expression)) - .is_some_and(|qualified_name| { - matches!( - qualified_name.segments(), - ["" | "builtins", "property"] | ["functools", "cached_property"] - ) - }) - }) { + // Skip properties and other stdlib property-like decorators. + if decorator_list + .iter() + .any(|decorator| is_property_like_decorator(decorator, checker.semantic())) + { return; } @@ -397,6 +390,25 @@ fn unnecessary_return_none(checker: &mut Checker, decorator_list: &[Decorator], } } +/// Determine whether `decorator` is `@property`, +/// or another stdlib decorator similar to `@property. +/// +/// TODO: be more principled here once we have type inference; +/// hardcoding these is a little hacky. +fn is_property_like_decorator(decorator: &Decorator, semantic: &SemanticModel) -> bool { + semantic + .resolve_qualified_name(&decorator.expression) + .is_some_and(|qualified_name| { + matches!( + qualified_name.segments(), + ["" | "builtins" | "enum", "property"] + | ["functools", "cached_property"] + | ["abc", "abstractproperty"] + | ["types", "DynamicClassAttribute"] + ) + }) +} + /// RET502 fn implicit_return_value(checker: &mut Checker, stack: &Stack) { for stmt in &stack.returns { 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 2cece7ec25311..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 @@ -1,42 +1,42 @@ --- source: crates/ruff_linter/src/rules/flake8_return/mod.rs --- -RET501.py:7:5: RET501 [*] Do not explicitly `return None` in function if it is the only possible return value +RET501.py:4:5: RET501 [*] Do not explicitly `return None` in function if it is the only possible return value | -5 | if not y: -6 | return -7 | return None # error +2 | if not y: +3 | return +4 | return None # error | ^^^^^^^^^^^ RET501 | = help: Remove explicit `return None` ℹ Safe fix -4 4 | def x(y): -5 5 | if not y: -6 6 | return -7 |- return None # error - 7 |+ return # error -8 8 | -9 9 | -10 10 | class BaseCache: +1 1 | def x(y): +2 2 | if not y: +3 3 | return +4 |- return None # error + 4 |+ return # error +5 5 | +6 6 | +7 7 | class BaseCache: -RET501.py:17:9: RET501 [*] Do not explicitly `return None` in function if it is the only possible return value +RET501.py:14:9: RET501 [*] Do not explicitly `return None` in function if it is the only possible return value | -15 | def get(self, key: str) -> None: -16 | print(f"{key} not found") -17 | return None +12 | def get(self, key: str) -> None: +13 | print(f"{key} not found") +14 | return None | ^^^^^^^^^^^ RET501 -18 | -19 | @property +15 | +16 | @property | = help: Remove explicit `return None` ℹ Safe fix -14 14 | -15 15 | def get(self, key: str) -> None: -16 16 | print(f"{key} not found") -17 |- return None - 17 |+ return -18 18 | -19 19 | @property -20 20 | def prop(self) -> None: +11 11 | +12 12 | def get(self, key: str) -> None: +13 13 | print(f"{key} not found") +14 |- return None + 14 |+ return +15 15 | +16 16 | @property +17 17 | def prop(self) -> None: From f45245384d5f1a9e8592dd867ea414714da1598d Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Tue, 30 Jul 2024 11:38:18 +0100 Subject: [PATCH 5/6] fix clippy and make tests meaningful --- .../resources/test/fixtures/flake8_return/RET501.py | 3 +++ crates/ruff_linter/src/rules/flake8_return/rules/function.rs | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) 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 ddf43c179534a..ea441e248636c 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_return/RET501.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_return/RET501.py @@ -33,12 +33,15 @@ def prop(self) -> None: @abc.abstractproperty def prop2(self) -> None: + print("Override me") return None @types.DynamicClassAttribute def prop3(self) -> None: + print("Gotta make this a multiline function for it to be a meaningful test") return None @enum.property def prop4(self) -> None: + print("I've run out of things to say") return None 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 69cc90c882e01..18dce86437b5d 100644 --- a/crates/ruff_linter/src/rules/flake8_return/rules/function.rs +++ b/crates/ruff_linter/src/rules/flake8_return/rules/function.rs @@ -391,7 +391,7 @@ fn unnecessary_return_none(checker: &mut Checker, decorator_list: &[Decorator], } /// Determine whether `decorator` is `@property`, -/// or another stdlib decorator similar to `@property. +/// or another stdlib decorator similar to `@property`. /// /// TODO: be more principled here once we have type inference; /// hardcoding these is a little hacky. From c2c96bccffb7d76d8de5a4e4fdfbc0bd967a399d Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Tue, 30 Jul 2024 12:02:21 +0100 Subject: [PATCH 6/6] Use the function in `ruff_python_semantic` --- .../test/fixtures/flake8_return/RET501.py | 18 --------- .../src/rules/flake8_return/rules/function.rs | 38 ++++++++----------- 2 files changed, 15 insertions(+), 41 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 ea441e248636c..972b686ac9cbd 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_return/RET501.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_return/RET501.py @@ -19,9 +19,6 @@ def prop(self) -> None: return None -import abc -import enum -import types from functools import cached_property @@ -30,18 +27,3 @@ class BaseCache2: def prop(self) -> None: print("Property not found") return None - - @abc.abstractproperty - def prop2(self) -> None: - print("Override me") - return None - - @types.DynamicClassAttribute - def prop3(self) -> None: - print("Gotta make this a multiline function for it to be a meaningful test") - return None - - @enum.property - def prop4(self) -> None: - print("I've run out of things to say") - return None 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 18dce86437b5d..96780d9cedf0a 100644 --- a/crates/ruff_linter/src/rules/flake8_return/rules/function.rs +++ b/crates/ruff_linter/src/rules/flake8_return/rules/function.rs @@ -6,12 +6,14 @@ use ruff_diagnostics::{AlwaysFixableViolation, FixAvailability, Violation}; use ruff_diagnostics::{Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::helpers::{is_const_false, is_const_true}; +use ruff_python_ast::name::QualifiedName; 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, Decorator, ElifElseClause, Expr, Stmt}; use ruff_python_codegen::Stylist; use ruff_python_index::Indexer; +use ruff_python_semantic::analyze::visibility::is_property; use ruff_python_semantic::SemanticModel; use ruff_python_trivia::{is_python_whitespace, SimpleTokenKind, SimpleTokenizer}; use ruff_source_file::Locator; @@ -373,11 +375,20 @@ fn unnecessary_return_none(checker: &mut Checker, decorator_list: &[Decorator], continue; } - // Skip properties and other stdlib property-like decorators. - if decorator_list + let extra_property_decorators = checker + .settings + .pydocstyle + .property_decorators .iter() - .any(|decorator| is_property_like_decorator(decorator, checker.semantic())) - { + .map(|decorator| QualifiedName::from_dotted_name(decorator)) + .collect::>(); + + // Skip property functions + if is_property( + decorator_list, + &extra_property_decorators, + checker.semantic(), + ) { return; } @@ -390,25 +401,6 @@ fn unnecessary_return_none(checker: &mut Checker, decorator_list: &[Decorator], } } -/// Determine whether `decorator` is `@property`, -/// or another stdlib decorator similar to `@property`. -/// -/// TODO: be more principled here once we have type inference; -/// hardcoding these is a little hacky. -fn is_property_like_decorator(decorator: &Decorator, semantic: &SemanticModel) -> bool { - semantic - .resolve_qualified_name(&decorator.expression) - .is_some_and(|qualified_name| { - matches!( - qualified_name.segments(), - ["" | "builtins" | "enum", "property"] - | ["functools", "cached_property"] - | ["abc", "abstractproperty"] - | ["types", "DynamicClassAttribute"] - ) - }) -} - /// RET502 fn implicit_return_value(checker: &mut Checker, stack: &Stack) { for stmt in &stack.returns {