From 33713a7e2a153155a6ecefbd505635449ff18e0a Mon Sep 17 00:00:00 2001 From: Shahar Naveh <50263213+ShaharNaveh@users.noreply.github.com> Date: Wed, 26 Nov 2025 10:31:22 +0200 Subject: [PATCH] Add rule to detect unnecessary class properties (#21535) Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com> Co-authored-by: Amethyst Reese Co-authored-by: Micha Reiser --- .../resources/test/fixtures/ruff/RUF066.py | 70 +++++++++++ .../src/checkers/ast/analyze/statement.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + .../rules/unnecessary_placeholder.rs | 22 +--- crates/ruff_linter/src/rules/ruff/mod.rs | 1 + .../ruff_linter/src/rules/ruff/rules/mod.rs | 2 + .../ruff/rules/property_without_return.rs | 119 ++++++++++++++++++ ..._rules__ruff__tests__RUF066_RUF066.py.snap | 31 +++++ crates/ruff_python_semantic/src/model.rs | 18 ++- ruff.schema.json | 1 + 10 files changed, 246 insertions(+), 22 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/ruff/RUF066.py create mode 100644 crates/ruff_linter/src/rules/ruff/rules/property_without_return.rs create mode 100644 crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF066_RUF066.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF066.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF066.py new file mode 100644 index 0000000000..ed08d51ef6 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF066.py @@ -0,0 +1,70 @@ +import abc +import typing + + +class User: # Test normal class properties + @property + def name(self): # ERROR: No return + f"{self.first_name} {self.last_name}" + + @property + def age(self): # OK: Returning something + return 100 + + def method(self): # OK: Not a property + x = 1 + + @property + def nested(self): # ERROR: Property itself doesn't return + def inner(): + return 0 + + @property + def stub(self): ... # OK: A stub; doesn't return anything + + +class UserMeta(metaclass=abc.ABCMeta): # Test properies inside of an ABC class + @property + @abc.abstractmethod + def abstr_prop1(self): ... # OK: Abstract methods doesn't need to return anything + + @property + @abc.abstractmethod + def abstr_prop2(self): # OK: Abstract methods doesn't need to return anything + """ + A cool docstring + """ + + @property + def prop1(self): # OK: Returning a value + return 1 + + @property + def prop2(self): # ERROR: Not returning something (even when we are inside an ABC) + 50 + + def method(self): # OK: Not a property + x = 1 + + +def func(): # OK: Not a property + x = 1 + + +class Proto(typing.Protocol): # Tests for a Protocol class + @property + def prop1(self) -> int: ... # OK: A stub property + + +class File: # Extra tests for things like yield/yield from/raise + @property + def stream1(self): # OK: Yields something + yield + + @property + def stream2(self): # OK: Yields from something + yield from self.stream1 + + @property + def children(self): # OK: Raises + raise ValueError("File does not have children") diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index a01194d5be..e1b4da60f2 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -347,6 +347,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.is_rule_enabled(Rule::InvalidArgumentName) { pep8_naming::rules::invalid_argument_name_function(checker, function_def); } + if checker.is_rule_enabled(Rule::PropertyWithoutReturn) { + ruff::rules::property_without_return(checker, function_def); + } } Stmt::Return(_) => { if checker.is_rule_enabled(Rule::ReturnInInit) { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 172841dc7c..ebec5f4acc 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -1058,6 +1058,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Ruff, "063") => rules::ruff::rules::AccessAnnotationsFromClassDict, (Ruff, "064") => rules::ruff::rules::NonOctalPermissions, (Ruff, "065") => rules::ruff::rules::LoggingEagerConversion, + (Ruff, "066") => rules::ruff::rules::PropertyWithoutReturn, (Ruff, "100") => rules::ruff::rules::UnusedNOQA, (Ruff, "101") => rules::ruff::rules::RedirectedNOQA, diff --git a/crates/ruff_linter/src/rules/flake8_pie/rules/unnecessary_placeholder.rs b/crates/ruff_linter/src/rules/flake8_pie/rules/unnecessary_placeholder.rs index 3d446f68b0..6ae6f0fc6d 100644 --- a/crates/ruff_linter/src/rules/flake8_pie/rules/unnecessary_placeholder.rs +++ b/crates/ruff_linter/src/rules/flake8_pie/rules/unnecessary_placeholder.rs @@ -1,8 +1,6 @@ use ruff_macros::{ViolationMetadata, derive_message_formats}; -use ruff_python_ast::helpers::map_subscript; use ruff_python_ast::whitespace::trailing_comment_start_offset; use ruff_python_ast::{Expr, ExprStringLiteral, Stmt, StmtExpr}; -use ruff_python_semantic::{ScopeKind, SemanticModel}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -101,7 +99,7 @@ pub(crate) fn unnecessary_placeholder(checker: &Checker, body: &[Stmt]) { // Ellipses are significant in protocol methods and abstract methods. // Specifically, Pyright uses the presence of an ellipsis to indicate that // a method is a stub, rather than a default implementation. - if in_protocol_or_abstract_method(checker.semantic()) { + if checker.semantic().in_protocol_or_abstract_method() { return; } Placeholder::Ellipsis @@ -163,21 +161,3 @@ impl std::fmt::Display for Placeholder { } } } - -/// Return `true` if the [`SemanticModel`] is in a `typing.Protocol` subclass or an abstract -/// method. -fn in_protocol_or_abstract_method(semantic: &SemanticModel) -> bool { - semantic.current_scopes().any(|scope| match scope.kind { - ScopeKind::Class(class_def) => class_def - .bases() - .iter() - .any(|base| semantic.match_typing_expr(map_subscript(base), "Protocol")), - ScopeKind::Function(function_def) => { - ruff_python_semantic::analyze::visibility::is_abstract( - &function_def.decorator_list, - semantic, - ) - } - _ => false, - }) -} diff --git a/crates/ruff_linter/src/rules/ruff/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index ecba00ea9d..5f06ffdb9f 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/mod.rs @@ -115,6 +115,7 @@ mod tests { #[test_case(Rule::NonOctalPermissions, Path::new("RUF064.py"))] #[test_case(Rule::LoggingEagerConversion, Path::new("RUF065_0.py"))] #[test_case(Rule::LoggingEagerConversion, Path::new("RUF065_1.py"))] + #[test_case(Rule::PropertyWithoutReturn, Path::new("RUF066.py"))] #[test_case(Rule::RedirectedNOQA, Path::new("RUF101_0.py"))] #[test_case(Rule::RedirectedNOQA, Path::new("RUF101_1.py"))] #[test_case(Rule::InvalidRuleCode, Path::new("RUF102.py"))] diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index 9d5ade77f3..206a504e74 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mod.rs @@ -35,6 +35,7 @@ pub(crate) use non_octal_permissions::*; pub(crate) use none_not_at_end_of_union::*; pub(crate) use parenthesize_chained_operators::*; pub(crate) use post_init_default::*; +pub(crate) use property_without_return::*; pub(crate) use pytest_raises_ambiguous_pattern::*; pub(crate) use quadratic_list_summation::*; pub(crate) use redirected_noqa::*; @@ -99,6 +100,7 @@ mod non_octal_permissions; mod none_not_at_end_of_union; mod parenthesize_chained_operators; mod post_init_default; +mod property_without_return; mod pytest_raises_ambiguous_pattern; mod quadratic_list_summation; mod redirected_noqa; diff --git a/crates/ruff_linter/src/rules/ruff/rules/property_without_return.rs b/crates/ruff_linter/src/rules/ruff/rules/property_without_return.rs new file mode 100644 index 0000000000..3027eb876b --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/rules/property_without_return.rs @@ -0,0 +1,119 @@ +use ruff_macros::{ViolationMetadata, derive_message_formats}; +use ruff_python_ast::identifier::Identifier; +use ruff_python_ast::visitor::{Visitor, walk_expr, walk_stmt}; +use ruff_python_ast::{Expr, Stmt, StmtFunctionDef}; +use ruff_python_semantic::analyze::{function_type, visibility}; + +use crate::checkers::ast::Checker; +use crate::{FixAvailability, Violation}; + +/// ## What it does +/// Detects class `@property` methods that does not have a `return` statement. +/// +/// ## Why is this bad? +/// Property methods are expected to return a computed value, a missing return in a property usually indicates an implementation mistake. +/// +/// ## Example +/// ```python +/// class User: +/// @property +/// def full_name(self): +/// f"{self.first_name} {self.last_name}" +/// ``` +/// +/// Use instead: +/// ```python +/// class User: +/// @property +/// def full_name(self): +/// return f"{self.first_name} {self.last_name}" +/// ``` +/// +/// ## References +/// - [Python documentation: The property class](https://docs.python.org/3/library/functions.html#property) +#[derive(ViolationMetadata)] +#[violation_metadata(preview_since = "0.14.7")] +pub(crate) struct PropertyWithoutReturn { + name: String, +} + +impl Violation for PropertyWithoutReturn { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::None; + + #[derive_message_formats] + fn message(&self) -> String { + let Self { name } = self; + format!("`{name}` is a property without a `return` statement") + } +} + +/// RUF066 +pub(crate) fn property_without_return(checker: &Checker, function_def: &StmtFunctionDef) { + let semantic = checker.semantic(); + + if checker.source_type.is_stub() || semantic.in_protocol_or_abstract_method() { + return; + } + + let StmtFunctionDef { + decorator_list, + body, + name, + .. + } = function_def; + + if !visibility::is_property(decorator_list, [], semantic) + || visibility::is_overload(decorator_list, semantic) + || function_type::is_stub(function_def, semantic) + { + return; + } + + let mut visitor = PropertyVisitor::default(); + visitor.visit_body(body); + if visitor.found { + return; + } + + checker.report_diagnostic( + PropertyWithoutReturn { + name: name.to_string(), + }, + function_def.identifier(), + ); +} + +#[derive(Default)] +struct PropertyVisitor { + found: bool, +} + +// NOTE: We are actually searching for the presence of +// `yield`/`yield from`/`raise`/`return` statement/expression, +// as having one of those indicates that there's likely no implementation mistake +impl Visitor<'_> for PropertyVisitor { + fn visit_expr(&mut self, expr: &Expr) { + if self.found { + return; + } + + match expr { + Expr::Yield(_) | Expr::YieldFrom(_) => self.found = true, + _ => walk_expr(self, expr), + } + } + + fn visit_stmt(&mut self, stmt: &Stmt) { + if self.found { + return; + } + + match stmt { + Stmt::Return(_) | Stmt::Raise(_) => self.found = true, + Stmt::FunctionDef(_) => { + // Do not recurse into nested functions; they're evaluated separately. + } + _ => walk_stmt(self, stmt), + } + } +} diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF066_RUF066.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF066_RUF066.py.snap new file mode 100644 index 0000000000..62dcefc843 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF066_RUF066.py.snap @@ -0,0 +1,31 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +--- +RUF066 `name` is a property without a `return` statement + --> RUF066.py:7:9 + | +5 | class User: # Test normal class properties +6 | @property +7 | def name(self): # ERROR: No return + | ^^^^ +8 | f"{self.first_name} {self.last_name}" + | + +RUF066 `nested` is a property without a `return` statement + --> RUF066.py:18:9 + | +17 | @property +18 | def nested(self): # ERROR: Property itself doesn't return + | ^^^^^^ +19 | def inner(): +20 | return 0 + | + +RUF066 `prop2` is a property without a `return` statement + --> RUF066.py:43:9 + | +42 | @property +43 | def prop2(self): # ERROR: Not returning something (even when we are inside an ABC) + | ^^^^^ +44 | 50 + | diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index 40ddadc4f8..78536071d7 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -3,12 +3,13 @@ use std::path::Path; use bitflags::bitflags; use rustc_hash::FxHashMap; -use ruff_python_ast::helpers::from_relative_import; +use ruff_python_ast::helpers::{from_relative_import, map_subscript}; use ruff_python_ast::name::{QualifiedName, UnqualifiedName}; use ruff_python_ast::{self as ast, Expr, ExprContext, PySourceType, Stmt}; use ruff_text_size::{Ranged, TextRange, TextSize}; use crate::Imported; +use crate::analyze::visibility; use crate::binding::{ Binding, BindingFlags, BindingId, BindingKind, Bindings, Exceptions, FromImport, Import, SubmoduleImport, @@ -2153,6 +2154,21 @@ impl<'a> SemanticModel<'a> { function.range() == function_def.range() }) } + + /// Return `true` if the model is in a `typing.Protocol` subclass or an abstract + /// method. + pub fn in_protocol_or_abstract_method(&self) -> bool { + self.current_scopes().any(|scope| match scope.kind { + ScopeKind::Class(class_def) => class_def + .bases() + .iter() + .any(|base| self.match_typing_expr(map_subscript(base), "Protocol")), + ScopeKind::Function(function_def) => { + visibility::is_abstract(&function_def.decorator_list, self) + } + _ => false, + }) + } } pub struct ShadowedBinding { diff --git a/ruff.schema.json b/ruff.schema.json index 5ef059e122..1c8a092042 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -4044,6 +4044,7 @@ "RUF063", "RUF064", "RUF065", + "RUF066", "RUF1", "RUF10", "RUF100",