Add rule to detect unnecessary class properties (#21535)

Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com>
Co-authored-by: Amethyst Reese <amethyst@n7.gg>
Co-authored-by: Micha Reiser <micha@reiser.io>

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

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

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,

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

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

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;

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

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

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 {

ruff.schema.json

@@ -4044,6 +4044,7 @@
         "RUF063",
         "RUF064",
         "RUF065",
+        "RUF066",
         "RUF1",
         "RUF10",
         "RUF100",