ronantakizawa/codereview-bench huggingface dataset:
Repository: astral-sh/ruff
File: crates/ruff_python_formatter/src/expression/expr_unary_op.rs (Rust)

Code before:
            space().fmt(f)?;
        }

        if operand
            .as_bin_op_expr()
            .is_some_and(|bin_op| bin_op.op.is_pow())
        {
            operand.format().with_options(Parentheses::Always).fmt(f)
        } else {
            operand.format().fmt(f)
        }
    }
}

impl NeedsParentheses for ExprUnaryOp {
    fn needs_parentheses(
        &self,
        parent: AnyNodeRef,
        context: &PyFormatContext,
    ) -> OptionalParentheses {
        if parent.is_expr_await() {
            return OptionalParentheses::Always;
        }

        if needs_line_break(self, context) {
            return OptionalParentheses::Multiline;
        }

        if is_expression_parenthesized(
            self.operand.as_ref().into(),
            context.comments().ranges(),
            context.source(),
        ) {
            return OptionalParentheses::Never;
        }

        if context.comments().has(self.operand.as_ref()) {
            return OptionalParentheses::Always;
        }

        self.operand.needs_parentheses(self.into(), context)
    }
}

/// Returns `true` if the unary operator will have a hard line break between the operator and its
/// operand and thus requires parentheses.
fn needs_line_break(item: &ExprUnaryOp, context: &PyFormatContext) -> bool {
    let comments = context.comments();
    let parenthesized_operand_range = parenthesized_range(
        item.operand.as_ref().into(),
        item.into(),

Review comment (suggestion):
I think we can even return `Always` here because we know it breaks over multiple lines and will need parentheses

Code after:
            space().fmt(f)?;
        }

        if operand
            .as_bin_op_expr()
            .is_some_and(|bin_op| bin_op.op.is_pow())
        {
            operand.format().with_options(Parentheses::Always).fmt(f)
        } else {
            operand.format().fmt(f)
        }
    }
}

impl NeedsParentheses for ExprUnaryOp {
    fn needs_parentheses(
        &self,
        parent: AnyNodeRef,
        context: &PyFormatContext,
    ) -> OptionalParentheses {
        if parent.is_expr_await() {
            return OptionalParentheses::Always;
        }

        if needs_line_break(self, context) {
            return OptionalParentheses::Always;
        }

        if is_expression_parenthesized(
            self.operand.as_ref().into(),
            context.comments().ranges(),
            context.source(),
        ) {
            return OptionalParentheses::Never;
        }

        if context.comments().has(self.operand.as_ref()) {
            return OptionalParentheses::Always;
        }

        self.operand.needs_parentheses(self.into(), context)
    }
}

/// Returns `true` if the unary operator will have a hard line break between the operator and its
/// operand and thus requires parentheses.
fn needs_line_break(item: &ExprUnaryOp, context: &PyFormatContext) -> bool {
    let comments = context.comments();
    let parenthesized_operand_range = parenthesized_range(
        item.operand.as_ref().into(),
        item.into(),
Repository: astral-sh/ruff
File: crates/ruff_linter/resources/test/fixtures/ruff/RUF066.py (Python)

Code before:
"""
Should emit:
RUF066 - on lines 12, 23, 48
"""

import abc
import typing


class User:
    @property
    def name(self):  # No return
        f"{self.first_name} {self.last_name}"

    @property
    def age(self):
        return 100

    def method(self):
        x = 1

    @property
    def nested(self):  # No return
        def inner():
            return 0

    @property
    def stub(self): ...

Review comment (suggestion):
Would you mind adding inline comments marking each case as okay or an error instead of one comment at the top? That way it will appear in each of the snapshots. That makes it a bit easier to review.

And could you renumber the rule to RUF069? I think we have PRs in flight for [66](https://github.com/astral-sh/ruff/pull/9911), [67](https://github.com/astral-sh/ruff/pull/20585), and [68](https://github.com/astral-sh/ruff/pull/21079) already.

Code after:
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
Repository: astral-sh/ruff
File: crates/ruff_linter/src/rules/ruff/rules/property_without_return.rs (Rust)

Code before:

    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.range(),
    );
}

#[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 {

Review comment (suggestion):
Sorry to add one more change needed. We should pick a smaller range for the diagnostic, rather than throwing a yellow squiggle on an entire function in an editor, as changing the diagnostic range later will potentially be a [breaking change](https://github.com/astral-sh/ruff/issues/14900#issuecomment-2543923625).

I think I'd suggest just marking the `@property` decorator itself, since that's the part that's unfulfilled by the body of the function.

Code after:

    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 {
Repository: astral-sh/ruff
File: crates/ruff_linter/src/rules/ruff/rules/property_without_return.rs (Rust)

Code before:
///         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")
    }
}

/// RUF069
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;
    }


Review comment (suggestion):
I think we prefer to put the name in backticks for clarity:

```suggestion
        format!("`{name}` is a property without a `return` statement")
```

Code after:
///         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;
    }
Repository: astral-sh/ruff
File: crates/ty_ide/src/code_action.rs (Rust)

Code before:
use ruff_db::{files::File, parsed::parsed_module};
use ruff_diagnostics::Edit;
use ruff_text_size::TextRange;
use ty_project::Db;
use ty_python_semantic::types::{UNDEFINED_REVEAL, UNRESOLVED_REFERENCE};

/// A `QuickFix` Code Action
#[derive(Debug, Clone)]
pub struct QuickFix {
    pub title: String,
    pub edits: Vec<Edit>,
    pub preferred: bool,
}

pub fn code_actions(
    db: &dyn Db,
    file: File,
    diagnostic_range: TextRange,
    diagnostic_id: &str,
) -> Option<Vec<QuickFix>> {
    let registry = db.lint_registry();
    let Ok(lint_id) = registry.get(diagnostic_id) else {
        return None;
    };
    let lint_name = lint_id.name();
    if lint_name == UNRESOLVED_REFERENCE.name() || lint_name == UNDEFINED_REVEAL.name() {
        let parsed = parsed_module(db, file).load(db);
        let node = covering_node(parsed.syntax().into(), diagnostic_range).node();
        let symbol = &node.expr_name()?.id;

        let fixes = completion::missing_imports(db, file, &parsed, symbol, node)
            .into_iter()
            .map(|import| QuickFix {
                title: import.label,
                edits: vec![import.edit],
                preferred: true,
            })
            .collect();
        Some(fixes)
    } else {
        None
    }
}


Review comment (suggestion):
```suggestion
    if lint_id == LintId::of(&UNRESOLVED_REFERENCE) || lint_id == LintId::of(&UNDEFINED_REVEAL){
```

Code after:

use ruff_db::{files::File, parsed::parsed_module};
use ruff_diagnostics::Edit;
use ruff_text_size::TextRange;
use ty_project::Db;
use ty_python_semantic::create_suppression_fix;
use ty_python_semantic::lint::LintId;
use ty_python_semantic::types::{UNDEFINED_REVEAL, UNRESOLVED_REFERENCE};

/// A `QuickFix` Code Action
#[derive(Debug, Clone)]
pub struct QuickFix {
    pub title: String,
    pub edits: Vec<Edit>,
    pub preferred: bool,
}

pub fn code_actions(
    db: &dyn Db,
    file: File,
    diagnostic_range: TextRange,
    diagnostic_id: &str,
) -> Vec<QuickFix> {
    let registry = db.lint_registry();
    let Ok(lint_id) = registry.get(diagnostic_id) else {
        return Vec::new();
    };

    let mut actions = Vec::new();

    // Suggest imports for unresolved references (often ideal)
    // TODO: suggest qualifying with an already imported symbol
    let is_unresolved_reference =
        lint_id == LintId::of(&UNRESOLVED_REFERENCE) || lint_id == LintId::of(&UNDEFINED_REVEAL);
    if is_unresolved_reference
        && let Some(import_quick_fix) = create_import_symbol_quick_fix(db, file, diagnostic_range)
    {
        actions.extend(import_quick_fix);
    }

    // Suggest just suppressing the lint (always a valid option, but never ideal)
    actions.push(QuickFix {
        title: format!("Ignore '{}' for this line", lint_id.name()),
        edits: create_suppression_fix(db, file, lint_id, diagnostic_range).into_edits(),
        preferred: false,
    });

    actions
}

fn create_import_symbol_quick_fix(
Repository: astral-sh/ruff
File: crates/ty_ide/src/completion.rs (Rust)

Code before:
                is_type_check_only: false,
                is_definitively_raisable: false,
                documentation,
            }
        })
        .collect();
    Some(completions)
}

// Returns function arguments that have already been set.
//
// If `offset` is inside an arguments node, this returns
// the list of argument names that are already set.
//
// For example, given:
// ```python
// def abc(foo, bar, baz): ...
// abc(foo=1, bar=2, b<CURSOR>)
// ```
// the resulting value is `["foo", "bar"]`
//
// This is useful to be able to exclude autocomplete suggestions
// for arguments that have already been set to some value.
//
// If the parent node is not an arguments node, the return value
// is an empty Vec.

fn detect_set_function_args(parsed: &ParsedModuleRef, offset: TextSize) -> Vec<&str> {
    let range = TextRange::empty(offset);
    covering_node(parsed.syntax().into(), range)
        .parent()
        .and_then(|node| match node {
            ast::AnyNodeRef::Arguments(args) => Some(args),
            _ => None,
        })
        .map(|args| {
            args.keywords
                .iter()
                .filter_map(|kw| kw.arg.as_ref().map(|ident| ident.id.as_str()))
                .collect()
        })
        .unwrap_or_default()
}

pub(crate) struct ImportEdit {
    pub label: String,
    pub edit: Edit,
}

pub(crate) fn missing_imports(
    db: &dyn Db,

Review comment (suggestion):
Same as above. This should also be a `///` comment.

Code after:
                is_type_check_only: false,
                is_definitively_raisable: false,
                documentation,
            }
        })
        .collect();
    Some(completions)
}

/// Returns function arguments that have already been set.
///
/// If `offset` is inside an arguments node, this returns
/// the list of argument names that are already set.
///
/// For example, given:
///
/// ```python
/// def abc(foo, bar, baz): ...
/// abc(foo=1, bar=2, b<CURSOR>)
/// ```
///
/// the resulting value is `["foo", "bar"]`
///
/// This is useful to be able to exclude autocomplete suggestions
/// for arguments that have already been set to some value.
///
/// If the parent node is not an arguments node, the return value
/// is an empty Vec.
fn detect_set_function_args(parsed: &ParsedModuleRef, offset: TextSize) -> FxHashSet<&str> {
    let range = TextRange::empty(offset);
    covering_node(parsed.syntax().into(), range)
        .parent()
        .and_then(|node| match node {
            ast::AnyNodeRef::Arguments(args) => Some(args),
            _ => None,
        })
        .map(|args| {
            args.keywords
                .iter()
                .filter_map(|kw| kw.arg.as_ref().map(|ident| ident.id.as_str()))
                .collect()
        })
        .unwrap_or_default()
}

pub(crate) struct ImportEdit {
    pub label: String,
    pub edit: Edit,
}

pub(crate) fn missing_imports(
Repository: astral-sh/ruff
File: crates/ty_ide/src/completion.rs (Rust)

Code before:
                documentation,
            }
        })
        .collect();
    Some(completions)
}

// Returns function arguments that have already been set.
//
// If `offset` is inside an arguments node, this returns
// the list of argument names that are already set.
//
// For example, given:
// ```python
// def abc(foo, bar, baz): ...
// abc(foo=1, bar=2, b<CURSOR>)
// ```
// the resulting value is `["foo", "bar"]`
//
// This is useful to be able to exclude autocomplete suggestions
// for arguments that have already been set to some value.
//
// If the parent node is not an arguments node, the return value
// is an empty Vec.

fn detect_set_function_args(parsed: &ParsedModuleRef, offset: TextSize) -> Vec<&str> {
    let range = TextRange::empty(offset);
    covering_node(parsed.syntax().into(), range)
        .parent()
        .and_then(|node| match node {
            ast::AnyNodeRef::Arguments(args) => Some(args),
            _ => None,
        })
        .map(|args| {
            args.keywords
                .iter()
                .filter_map(|kw| kw.arg.as_ref().map(|ident| ident.id.as_str()))
                .collect()
        })
        .unwrap_or_default()
}

pub(crate) struct ImportEdit {
    pub label: String,
    pub edit: Edit,
}

pub(crate) fn missing_imports(
    db: &dyn Db,
    file: File,
    parsed: &ParsedModuleRef,

Review comment (performance):
I think this should return a `FxHashSet` so that membership testing is constant time. Otherwise the loop in the caller is quadratic. (Which probably doesn't matter much in most real world cases, and perhaps even faster, but it'd be better to avoid quadratic complexity and this is a simple way to do it.)

Code after:
                documentation,
            }
        })
        .collect();
    Some(completions)
}

/// Returns function arguments that have already been set.
///
/// If `offset` is inside an arguments node, this returns
/// the list of argument names that are already set.
///
/// For example, given:
///
/// ```python
/// def abc(foo, bar, baz): ...
/// abc(foo=1, bar=2, b<CURSOR>)
/// ```
///
/// the resulting value is `["foo", "bar"]`
///
/// This is useful to be able to exclude autocomplete suggestions
/// for arguments that have already been set to some value.
///
/// If the parent node is not an arguments node, the return value
/// is an empty Vec.
fn detect_set_function_args(parsed: &ParsedModuleRef, offset: TextSize) -> FxHashSet<&str> {
    let range = TextRange::empty(offset);
    covering_node(parsed.syntax().into(), range)
        .parent()
        .and_then(|node| match node {
            ast::AnyNodeRef::Arguments(args) => Some(args),
            _ => None,
        })
        .map(|args| {
            args.keywords
                .iter()
                .filter_map(|kw| kw.arg.as_ref().map(|ident| ident.id.as_str()))
                .collect()
        })
        .unwrap_or_default()
}

pub(crate) struct ImportEdit {
    pub label: String,
    pub edit: Edit,
}

pub(crate) fn missing_imports(
    db: &dyn Db,
    file: File,
Repository: astral-sh/ruff
File: crates/ty_ide/src/rename.rs (Rust)

Code before:
          |
        4 | @overload
        5 | def test() -> None: ...
          |     ^^^^
        6 | @overload
        7 | def test(a: str) -> str: ...
          |
        ");
    }

    #[test]
    fn rename_property() {
        let test = CursorTest::builder()
            .source(
                "mypackage/__init__.py",
                r#"
                class Foo:
                    @property
                    def my_property<CURSOR>(self) -> int:
                        return 42
                "#,
            )
            .source(
                "mypackage/subpkg/__init__.py",
                r#"
                from mypackage import Foo

                print(Foo().my_property)
                "#,
            )
            .build();

        assert_snapshot!(test.rename("better_name"), @r"
        info[rename]: Rename symbol (found 2 locations)
         --> mypackage/__init__.py:4:9
          |
        2 | class Foo:
        3 |     @property
        4 |     def my_property(self) -> int:
          |         ^^^^^^^^^^^
        5 |         return 42
          |
         ::: mypackage/subpkg/__init__.py:4:13
          |
        2 | from mypackage import Foo
        3 |
        4 | print(Foo().my_property)
          |             -----------
          |
        ");
    }

Review comment (performance):
You probably copy pasted my tests where I messed this up. Do we need the subpackage complexity here or could we just go with `main.py` and `lib.py`?

Code after:
          |
        4 | @overload
        5 | def test() -> None: ...
          |     ^^^^
        6 | @overload
        7 | def test(a: str) -> str: ...
          |
        ");
    }

    #[test]
    fn rename_property() {
        let test = CursorTest::builder()
            .source(
                "lib.py",
                r#"
                class Foo:
                    @property
                    def my_property<CURSOR>(self) -> int:
                        return 42
                "#,
            )
            .source(
                "main.py",
                r#"
                from lib import Foo

                print(Foo().my_property)
                "#,
            )
            .build();

        assert_snapshot!(test.rename("better_name"), @r"
        info[rename]: Rename symbol (found 2 locations)
         --> lib.py:4:9
          |
        2 | class Foo:
        3 |     @property
        4 |     def my_property(self) -> int:
          |         ^^^^^^^^^^^
        5 |         return 42
          |
         ::: main.py:4:13
          |
        2 | from lib import Foo
        3 |
        4 | print(Foo().my_property)
          |             -----------
          |
        ");
    }