Improve consistency between linter rules in determining whether a function is property#12581
Improve consistency between linter rules in determining whether a function is property#12581AlexWaygood merged 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
The changes to this file increase the scope of this rule. We could therefore make the changes to this rule a preview-only change. However, I don't see it as a significant increase in scope: I doubt there will be many new hits on user code as a result of this change. The error that the rule is trying to catch is pretty uncommon, anyway.
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| DOC201 | 14 | 0 | 14 | 0 | 0 |
|
The ecosystem check has a lot of hits going away for |
MichaReiser
left a comment
There was a problem hiding this comment.
There's also
ruff/crates/ruff_python_semantic/src/analyze/visibility.rs
Lines 66 to 83 in 7a4419a
and they both seem slightly different...
| let extra_property_decorators = checker | ||
| .settings | ||
| .pydocstyle | ||
| .property_decorators | ||
| .iter() | ||
| .map(|decorator| QualifiedName::from_dotted_name(decorator)) | ||
| .collect::<Vec<QualifiedName>>(); |
There was a problem hiding this comment.
I think I'm reviewing the PRs in the wrong order. This is now behind your new iterator and no longer requires collecting?
There was a problem hiding this comment.
I think I'm reviewing the PRs in the wrong order.
You are, yes :-) This is the first PR in the stack, and the other two PRs are based on top of this branch.
But as I mentioned in #12582 (comment), I think we still need to collect ~always even with my new iterator, because the common pattern seems to be to do something like this:
for decorator in decorators {
if extra_properties.any(|property| property == decorator) {
return true;
}
}If extra_properties is an iterator there, it could be exhausted after the first iteration of the outer for loop
There was a problem hiding this comment.
We can Clone the iterator to avoid this
| pub fn is_property( | ||
| &self, | ||
| extra_properties: &[QualifiedName], | ||
| semantic: &SemanticModel, | ||
| ) -> bool { | ||
| self.as_function_def() | ||
| .is_some_and(|StmtFunctionDef { decorator_list, .. }| { | ||
| is_property(decorator_list, extra_properties, semantic) | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
I wonder if it's worth having this method in definition if it only ever is called from pylint rules. I would move it upwards next to the lint rule.
There was a problem hiding this comment.
I'd prefer to have it here, so that it can be easily reused by other rules that we add in the future
That's the function I'm switching the rules over to using in this PR! |
Summary
We have a function in
ruff_python_semanticfor detecting whether a function is a property or not. However, there are various linter rules where we've forgotten to use it. That means that our linter rules are internally inconsistent about whether they consider methods decorated with@functools.cached_propertyto be a property method, for example. This PR fixes that.Test Plan
cargo test -p ruff_linter --lib