Skip to content

Commit

Permalink
Account for other property-like decorators as well
Browse files Browse the repository at this point in the history
  • Loading branch information
AlexWaygood committed Jul 30, 2024
1 parent ac94d4a commit d5fb0cb
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 42 deletions.
23 changes: 20 additions & 3 deletions crates/ruff_linter/resources/test/fixtures/flake8_return/RET501.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
from functools import cached_property


def x(y):
if not y:
return
Expand All @@ -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
38 changes: 25 additions & 13 deletions crates/ruff_linter/src/rules/flake8_return/rules/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}

Expand All @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
@@ -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:

0 comments on commit d5fb0cb

Please sign in to comment.