Skip to content

[flake8-return] Exempt cached properties and other property-like decorators from explicit return rule (RET501) #12563

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jul 30, 2024
Merged

[flake8-return] Exempt cached properties and other property-like decorators from explicit return rule (RET501) #12563

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cb491ea
[flake8-return] Exempt cached properties (RET501)
epenet Jul 29, 2024
2f30854
Update snap
epenet Jul 29, 2024
ac94d4a
Implement rule
epenet Jul 29, 2024
d5fb0cb
Account for other property-like decorators as well
AlexWaygood Jul 30, 2024
f452453
fix clippy and make tests meaningful
AlexWaygood Jul 30, 2024
c2c96bc
Use the function in `ruff_python_semantic`
AlexWaygood Jul 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_return/RET501.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,28 @@ def get(self, key: str) -> None:
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
30 changes: 24 additions & 6 deletions crates/ruff_linter/src/rules/flake8_return/rules/function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,12 +373,11 @@ fn unnecessary_return_none(checker: &mut Checker, decorator_list: &[Decorator],
continue;
}

// Skip properties.
if decorator_list.iter().any(|decorator| {
checker
.semantic()
.match_builtin_expr(&decorator.expression, "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 @@ -391,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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@epenet -- you had semantic.resolve_qualified_name(map_callable(&decorator.expression)) here. I think using map_callable would be incorrect in this instance. map_callable() is a convenience function for when we don't care whether e.g. a function is decorated with @functools.lru_cache or @functools.lru_cache() (whether the decorator is a call-expression or not is irrelevant). But for all of these property-like decorators, you can only use them as @property; it's invalid to use them as @property(). So for these decorators, we just want to call resolve_qualified_name directly on the expression, instead of passing it through map_callable first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied it from elsewhere in the code base.
I wonder if this applies there as well...
And would it make sense to move this function to a shared location?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied it from elsewhere in the code base.
I wonder if this applies there as well...

It's hard to say in general. For a lot of decorators (like @lru_cache), you really don't care about whether it's been decorated with @lru_cache or @lru_cache(). So while there may be other places in the codebase where it's being used incorrectly, I'm pretty confident they're not all incorrect ;-)

And would it make sense to move this function to a shared location?

Possibly...

Copy link
Member

@AlexWaygood AlexWaygood Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good that you asked. It seems we already have a general-purpose function for this:

ruff/crates/ruff_python_semantic/src/analyze/visibility.rs

Lines 63 to 83 in aaa56eb

/// Returns `true` if a function definition is a `@property`.
/// `extra_properties` can be used to check additional non-standard
/// `@property`-like decorators.
pub fn is_property(
decorator_list: &[Decorator],
extra_properties: &[QualifiedName],
semantic: &SemanticModel,
) -> bool {
decorator_list.iter().any(|decorator| {
semantic
.resolve_qualified_name(map_callable(&decorator.expression))
.is_some_and(|qualified_name| {
matches!(
qualified_name.segments(),
["" | "builtins", "property"] | ["functools", "cached_property"]
) || extra_properties
.iter()
.any(|extra_property| extra_property.segments() == qualified_name.segments())
})
})
}

We should just use that for now. I'll apply some fixes to it in a followup PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, I wonder if it might make sense to look at rules/pydoclint/rules/check_docstring.rs also:

ruff/crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs

Lines 442 to 461 in 459c85b

// Checks if a function has a `@property` decorator
fn is_property(definition: &Definition, checker: &Checker) -> bool {
let Some(function) = definition.as_function_def() else {
return false;
};
let Some(last_decorator) = function.decorator_list.last() else {
return false;
};
checker
.semantic()
.resolve_qualified_name(&last_decorator.expression)
.is_some_and(|qualified_name| {
matches!(
qualified_name.segments(),
["", "property"] | ["functools", "cached_property"]
)
})
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, thanks

.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
Loading