From e33d1f54c6f0035546e78665fa560c383a00b1f1 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Tue, 30 Jul 2024 14:37:33 +0100 Subject: [PATCH] Improve consistency in determining whether a function is property --- .../test/fixtures/pydoclint/DOC201_google.py | 15 +++++++++ .../pylint/property_with_parameters.py | 9 ++++++ .../rules/pydoclint/rules/check_docstring.rs | 32 ++++++------------- .../pylint/rules/property_with_parameters.rs | 12 +++++-- ...__PLR0206_property_with_parameters.py.snap | 9 ++++++ crates/ruff_python_semantic/src/definition.rs | 18 +++++++++-- 6 files changed, 67 insertions(+), 28 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC201_google.py b/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC201_google.py index 800ed3ed9c503..ccb9a76560088 100644 --- a/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC201_google.py +++ b/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC201_google.py @@ -71,3 +71,18 @@ def nested(): return 5 print("I never return") + + +from functools import cached_property + +class Baz: + # OK + @cached_property + def baz(self) -> str: + """ + Do something + + Args: + num (int): A number + """ + return 'test' diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/property_with_parameters.py b/crates/ruff_linter/resources/test/fixtures/pylint/property_with_parameters.py index dba24c4a1e388..210f02981ff35 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/property_with_parameters.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/property_with_parameters.py @@ -38,3 +38,12 @@ def attribute_var_args(self, *args): # [property-with-parameters] @property def attribute_var_kwargs(self, **kwargs): #[property-with-parameters] return {key: value * 2 for key, value in kwargs.items()} + + +from functools import cached_property + + +class Cached: + @cached_property + def cached_prop(self, value): # [property-with-parameters] + ... diff --git a/crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs b/crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs index 480759ee393b9..0fdb0d09d9f78 100644 --- a/crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs +++ b/crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs @@ -439,27 +439,6 @@ fn extract_raised_exception<'a>( None } -// 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"] - ) - }) -} - /// DOC201, DOC202, DOC501, DOC502 pub(crate) fn check_docstring( checker: &mut Checker, @@ -498,8 +477,15 @@ pub(crate) fn check_docstring( }; // DOC201 - if checker.enabled(Rule::DocstringMissingReturns) { - if !is_property(definition, checker) && docstring_sections.returns.is_none() { + if checker.enabled(Rule::DocstringMissingReturns) && docstring_sections.returns.is_none() { + let extra_property_decorators = checker + .settings + .pydocstyle + .property_decorators + .iter() + .map(|decorator| QualifiedName::from_dotted_name(decorator)) + .collect::>(); + if !definition.is_property(&extra_property_decorators, checker.semantic()) { if let Some(body_return) = body_entries.returns.first() { let diagnostic = Diagnostic::new(DocstringMissingReturns, body_return.range()); diagnostics.push(diagnostic); diff --git a/crates/ruff_linter/src/rules/pylint/rules/property_with_parameters.rs b/crates/ruff_linter/src/rules/pylint/rules/property_with_parameters.rs index 835f19c1a38b4..32b54c5f3f976 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/property_with_parameters.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/property_with_parameters.rs @@ -1,6 +1,8 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::name::QualifiedName; use ruff_python_ast::{identifier::Identifier, Decorator, Parameters, Stmt}; +use ruff_python_semantic::analyze::visibility::is_property; use crate::checkers::ast::Checker; @@ -55,10 +57,14 @@ pub(crate) fn property_with_parameters( return; } let semantic = checker.semantic(); - if decorator_list + let extra_property_decorators = checker + .settings + .pydocstyle + .property_decorators .iter() - .any(|decorator| semantic.match_builtin_expr(&decorator.expression, "property")) - { + .map(|decorator| QualifiedName::from_dotted_name(decorator)) + .collect::>(); + if is_property(decorator_list, &extra_property_decorators, semantic) { checker .diagnostics .push(Diagnostic::new(PropertyWithParameters, stmt.identifier())); diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0206_property_with_parameters.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0206_property_with_parameters.py.snap index 50ffeaf9636b8..cf968be9da783 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0206_property_with_parameters.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0206_property_with_parameters.py.snap @@ -42,3 +42,12 @@ property_with_parameters.py:39:9: PLR0206 Cannot have defined parameters for pro | ^^^^^^^^^^^^^^^^^^^^ PLR0206 40 | return {key: value * 2 for key, value in kwargs.items()} | + +property_with_parameters.py:48:9: PLR0206 Cannot have defined parameters for properties + | +46 | class Cached: +47 | @cached_property +48 | def cached_prop(self, value): # [property-with-parameters] + | ^^^^^^^^^^^ PLR0206 +49 | ... + | diff --git a/crates/ruff_python_semantic/src/definition.rs b/crates/ruff_python_semantic/src/definition.rs index 069e3c36ebfd6..667fc8109c7a4 100644 --- a/crates/ruff_python_semantic/src/definition.rs +++ b/crates/ruff_python_semantic/src/definition.rs @@ -6,13 +6,16 @@ use std::ops::Deref; use std::path::Path; use ruff_index::{newtype_index, IndexSlice, IndexVec}; -use ruff_python_ast::{self as ast, Stmt}; +use ruff_python_ast::name::QualifiedName; +use ruff_python_ast::{self as ast, Stmt, StmtFunctionDef}; use ruff_text_size::{Ranged, TextRange}; use crate::analyze::visibility::{ - class_visibility, function_visibility, method_visibility, module_visibility, Visibility, + class_visibility, function_visibility, is_property, method_visibility, module_visibility, + Visibility, }; use crate::model::all::DunderAllName; +use crate::SemanticModel; /// Id uniquely identifying a definition in a program. #[newtype_index] @@ -148,6 +151,17 @@ impl<'a> Definition<'a> { ) } + 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) + }) + } + /// Return the name of the definition. pub fn name(&self) -> Option<&'a str> { match self {