Improve consistency between linter rules in determining whether a function is property #12581

AlexWaygood · 2024-07-30T13:39:37Z

Summary

We have a function in ruff_python_semantic for 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_property to be a property method, for example. This PR fixes that.

Test Plan

cargo test -p ruff_linter --lib

AlexWaygood · 2024-07-30T13:40:54Z

crates/ruff_linter/src/rules/pylint/rules/property_with_parameters.rs

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.

github-actions · 2024-07-30T13:53:02Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+0 -14 violations, +0 -0 fixes in 2 projects; 52 projects unchanged)

apache/airflow (+0 -13 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

- airflow/models/taskinstance.py:1944:9: DOC201 `return` is not documented in docstring
- airflow/models/taskinstance.py:2008:9: DOC201 `return` is not documented in docstring
- airflow/providers/alibaba/cloud/sensors/oss_key.py:102:9: DOC201 `return` is not documented in docstring
- airflow/providers/amazon/aws/utils/mixins.py:161:9: DOC201 `return` is not documented in docstring
- airflow/providers/amazon/aws/utils/mixins.py:171:9: DOC201 `return` is not documented in docstring
- airflow/providers/apache/spark/operators/spark_sql.py:103:9: DOC201 `return` is not documented in docstring
- airflow/providers/google/cloud/operators/mlengine.py:1490:9: DOC201 `return` is not documented in docstring
- airflow/providers/google/cloud/operators/mlengine.py:1499:9: DOC201 `return` is not documented in docstring
- airflow/providers/google/cloud/operators/mlengine.py:1509:9: DOC201 `return` is not documented in docstring
- airflow/providers/google/cloud/operators/vertex_ai/auto_ml.py:651:9: DOC201 `return` is not documented in docstring
- airflow/providers/google/cloud/operators/vertex_ai/custom_job.py:1642:9: DOC201 `return` is not documented in docstring
- airflow/providers/google/cloud/operators/vertex_ai/custom_job.py:1652:9: DOC201 `return` is not documented in docstring
- airflow/providers/google/common/hooks/base_google.py:466:9: DOC201 `return` is not documented in docstring

apache/superset (+0 -1 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

- superset/viz.py:673:9: DOC201 `return` is not documented in docstring

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
DOC201	14	0	14	0	0

AlexWaygood · 2024-07-30T13:58:42Z

The ecosystem check has a lot of hits going away for DOC201, which tries to skip functions it considers to be properties. Previously it only considered a function to be a property if the last decorator on that function was @property (or similar); now it considers a function to be a property if any decorator on that function is @property (or similar). I think the new behaviour is correct.

MichaReiser

There's also

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

Lines 66 to 83 in 7a4419a

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

and they both seem slightly different...

MichaReiser · 2024-07-30T16:28:50Z

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

+        let extra_property_decorators = checker
+            .settings
+            .pydocstyle
+            .property_decorators
+            .iter()
+            .map(|decorator| QualifiedName::from_dotted_name(decorator))
+            .collect::<Vec<QualifiedName>>();


I think I'm reviewing the PRs in the wrong order. This is now behind your new iterator and no longer requires collecting?

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

We can Clone the iterator to avoid this

Oh, good point.

MichaReiser · 2024-07-30T16:29:28Z

crates/ruff_python_semantic/src/definition.rs

+    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)
+            })
+    }
+


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.

I'd prefer to have it here, so that it can be easily reused by other rules that we add in the future

AlexWaygood · 2024-07-30T16:41:58Z

There's also

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

Lines 66 to 83 in 7a4419a

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

})

})

}

and they both seem slightly different...

That's the function I'm switching the rules over to using in this PR!

Improve consistency in determining whether a function is property

e33d1f5

AlexWaygood added the linter Related to the linter label Jul 30, 2024

AlexWaygood commented Jul 30, 2024

View reviewed changes

This was referenced Jul 30, 2024

Make setting and retrieving pydocstyle settings less tedious #12582

Merged

Consider more stdlib decorators to be property-like #12583

Merged

MichaReiser approved these changes Jul 30, 2024

View reviewed changes

AlexWaygood merged commit 4738135 into main Jul 30, 2024
20 checks passed

AlexWaygood deleted the alex/is-property branch July 30, 2024 16:42

BrewTestBot mentioned this pull request Aug 2, 2024

ruff 0.5.6 Homebrew/homebrew-core#179432

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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 #12581

AlexWaygood commented Jul 30, 2024

AlexWaygood Jul 30, 2024

github-actions bot commented Jul 30, 2024

AlexWaygood commented Jul 30, 2024

MichaReiser left a comment

MichaReiser Jul 30, 2024

AlexWaygood Jul 30, 2024

MichaReiser Jul 30, 2024

AlexWaygood Jul 30, 2024

MichaReiser Jul 30, 2024

AlexWaygood Jul 30, 2024

AlexWaygood commented Jul 30, 2024

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

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 #12581

Conversation

AlexWaygood commented Jul 30, 2024

Summary

Test Plan

AlexWaygood Jul 30, 2024

Choose a reason for hiding this comment

github-actions bot commented Jul 30, 2024

ruff-ecosystem results

Linter (stable)

Linter (preview)

AlexWaygood commented Jul 30, 2024

MichaReiser left a comment

Choose a reason for hiding this comment

MichaReiser Jul 30, 2024

Choose a reason for hiding this comment

AlexWaygood Jul 30, 2024

Choose a reason for hiding this comment

MichaReiser Jul 30, 2024

Choose a reason for hiding this comment

AlexWaygood Jul 30, 2024

Choose a reason for hiding this comment

MichaReiser Jul 30, 2024

Choose a reason for hiding this comment

AlexWaygood Jul 30, 2024

Choose a reason for hiding this comment

AlexWaygood commented Jul 30, 2024

`ruff-ecosystem` results