[`flake8-return`] Exempt cached properties and other property-like decorators from explicit return rule (`RET501`) by epenet · Pull Request #12563 · astral-sh/ruff

epenet · 2024-07-29T06:24:39Z

Summary

Follow-up to #12243, in which regular properties were exempted but not cached properties.

Still linked to home-assistant/core#115031

Test Plan

codspeed-hq · 2024-07-29T06:41:17Z

CodSpeed Performance Report

Merging #12563 will not alter performance

_{Comparing epenet:20240729-0822 (c2c96bc) with epenet:20240729-0822 (f452453)}

Summary

✅ 33 untouched benchmarks

github-actions · 2024-07-29T07:15:45Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

MichaReiser

LGTM. Thank you. I let @AlexWaygood merge to get the blessing of someone who actually understands Python ;)

AlexWaygood

LGTM. I added support for other stdlib property-like decorators as well, such as @abc.abstractproperty, @enum.property, and @types.DynamicClassAttribute. I think the same rationale applies for special-casing these decorators as it does for special-casing @functools.cached_property.

crates/ruff_linter/resources/test/fixtures/flake8_return/RET501.py

AlexWaygood · 2024-07-30T10:35:24Z

crates/ruff_linter/src/rules/flake8_return/rules/function.rs

+/// hardcoding these is a little hacky.
+fn is_property_like_decorator(decorator: &Decorator, semantic: &SemanticModel) -> bool {
+    semantic
+        .resolve_qualified_name(&decorator.expression)


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

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?

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

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.

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"]

)

})

}

nice, thanks

[flake8-return] Exempt cached properties (RET501)

cb491ea

epenet marked this pull request as draft July 29, 2024 06:24

Update snap

2f30854

epenet force-pushed the 20240729-0822 branch from a8f0516 to ac94d4a Compare July 29, 2024 07:01

Implement rule

ac94d4a

epenet marked this pull request as ready for review July 29, 2024 07:08

epenet mentioned this pull request Jul 29, 2024

Enable Ruff RET501 home-assistant/core#115031

Merged

20 tasks

MichaReiser approved these changes Jul 30, 2024

View reviewed changes

MichaReiser added the rule Implementing or modifying a lint rule label Jul 30, 2024

Account for other property-like decorators as well

d5fb0cb

AlexWaygood force-pushed the 20240729-0822 branch from 20b13fd to d5fb0cb Compare July 30, 2024 10:30

AlexWaygood approved these changes Jul 30, 2024

View reviewed changes

epenet commented Jul 30, 2024

View reviewed changes

crates/ruff_linter/resources/test/fixtures/flake8_return/RET501.py Outdated Show resolved Hide resolved

AlexWaygood reviewed Jul 30, 2024

View reviewed changes

fix clippy and make tests meaningful

f452453

AlexWaygood changed the title ~~[flake8-return] Exempt cached properties from explicit return rule (RET501)~~ [flake8-return] Exempt cached properties and other property-like decorators from explicit return rule (RET501) Jul 30, 2024

Use the function in ruff_python_semantic

c2c96bc

AlexWaygood enabled auto-merge (squash) July 30, 2024 11:03

AlexWaygood merged commit 459c85b into astral-sh:main Jul 30, 2024

epenet deleted the 20240729-0822 branch July 30, 2024 11:12

BrewTestBot mentioned this pull request Aug 2, 2024

ruff 0.5.6 Homebrew/homebrew-core#179432

Merged

epenet mentioned this pull request Oct 3, 2024

Restore __slots__ to core objects home-assistant/core#127441

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

[`flake8-return`] Exempt cached properties and other property-like decorators from explicit return rule (`RET501`)#12563
AlexWaygood merged 6 commits intoastral-sh:mainfrom
epenet:20240729-0822

epenet commented Jul 29, 2024 •

edited

Loading

Uh oh!

codspeed-hq bot commented Jul 29, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Jul 29, 2024 •

edited

Loading

Uh oh!

MichaReiser left a comment

Uh oh!

AlexWaygood left a comment

Uh oh!

Uh oh!

AlexWaygood Jul 30, 2024

Uh oh!

epenet Jul 30, 2024

Uh oh!

AlexWaygood Jul 30, 2024

Uh oh!

AlexWaygood Jul 30, 2024 •

edited

Loading

Uh oh!

epenet Jul 30, 2024

Uh oh!

AlexWaygood Jul 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	/// 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())
	})
	})
	}

	// 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"]
	)
	})
	}

Conversation

epenet commented Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

codspeed-hq bot commented Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodSpeed Performance Report

Merging #12563 will not alter performance

Summary

Uh oh!

github-actions bot commented Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlexWaygood Jul 30, 2024

Choose a reason for hiding this comment

Uh oh!

epenet Jul 30, 2024

Choose a reason for hiding this comment

Uh oh!

AlexWaygood Jul 30, 2024

Choose a reason for hiding this comment

Uh oh!

AlexWaygood Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epenet Jul 30, 2024

Choose a reason for hiding this comment

Uh oh!

AlexWaygood Jul 30, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

epenet commented Jul 29, 2024 •

edited

Loading

codspeed-hq bot commented Jul 29, 2024 •

edited

Loading

github-actions bot commented Jul 29, 2024 •

edited

Loading

`ruff-ecosystem` results

AlexWaygood Jul 30, 2024 •

edited

Loading