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

Code before:
    }
}

impl<'db> IntoIterator for Hover<'db> {
    type Item = HoverContent<'db>;
    type IntoIter = std::vec::IntoIter<Self::Item>;

    fn into_iter(self) -> Self::IntoIter {
        self.contents.into_iter()
    }
}

impl<'a, 'db> IntoIterator for &'a Hover<'db> {
    type Item = &'a HoverContent<'db>;
    type IntoIter = std::slice::Iter<'a, HoverContent<'db>>;

    fn into_iter(self) -> Self::IntoIter {
        self.iter()
    }
}

pub struct DisplayHover<'a> {
    db: &'a dyn Db,
    hover: &'a Hover<'a>,
    kind: MarkupKind,
}

impl fmt::Display for DisplayHover<'_> {
    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
        let mut first = true;
        for content in &self.hover.contents {
            if !first {
                self.kind.horizontal_line().fmt(f)?;
            }

            content.display(self.db, self.kind).fmt(f)?;
            first = false;
        }

        Ok(())
    }
}

#[derive(Debug, Clone, Eq, PartialEq)]
pub enum HoverContent<'db> {
    Type(Type<'db>, Option<TypeVarVariance>),
    Docstring(Docstring),
}

impl<'db> HoverContent<'db> {
    fn display(&self, db: &'db dyn Db, kind: MarkupKind) -> DisplayHoverContent<'_, 'db> {

Review comment (suggestion):
This is debris from versions where this was being asked to store `CallSignatureDetails`, which is actually invariant over `'db`. Leaving it like this just simplifies any potential future work.

Code after:
    }
}

impl<'db> IntoIterator for Hover<'db> {
    type Item = HoverContent<'db>;
    type IntoIter = std::vec::IntoIter<Self::Item>;

    fn into_iter(self) -> Self::IntoIter {
        self.contents.into_iter()
    }
}

impl<'a, 'db> IntoIterator for &'a Hover<'db> {
    type Item = &'a HoverContent<'db>;
    type IntoIter = std::slice::Iter<'a, HoverContent<'db>>;

    fn into_iter(self) -> Self::IntoIter {
        self.iter()
    }
}

pub struct DisplayHover<'db, 'a> {
    db: &'db dyn Db,
    hover: &'a Hover<'db>,
    kind: MarkupKind,
}

impl fmt::Display for DisplayHover<'_, '_> {
    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
        let mut first = true;
        for content in &self.hover.contents {
            if !first {
                self.kind.horizontal_line().fmt(f)?;
            }

            content.display(self.db, self.kind).fmt(f)?;
            first = false;
        }

        Ok(())
    }
}

#[derive(Debug, Clone)]
pub enum HoverContent<'db> {
    Signature(String),
    Type(Type<'db>, Option<TypeVarVariance>),
    Docstring(Docstring),
}

impl<'db> HoverContent<'db> {
Repository: astral-sh/ruff
File: crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_getcwd.rs (Rust)

Code before:
    }

    let range = call.range();
    let mut diagnostic = checker.report_diagnostic(OsGetcwd, call.func.range());

    if !call.arguments.is_empty() {
        return;
    }

    if is_fix_os_getcwd_enabled(checker.settings()) {
        diagnostic.try_set_fix(|| {
            let (import_edit, binding) = checker.importer().get_or_import_symbol(
                &ImportRequest::import("pathlib", "Path"),
                call.start(),
                checker.semantic(),
            )?;

            let applicability = if checker.comment_ranges().intersects(range) {
                Applicability::Unsafe
            } else if is_top_level_expression_call(checker, call) {
                // Safe when the call is a top-level expression (return value not used)
                Applicability::Safe
            } else {
                // Unsafe because the return type changes (str/bytes -> Path)
                Applicability::Unsafe
            };

            let replacement = format!("{binding}.cwd()");

            Ok(Fix::applicable_edits(
                Edit::range_replacement(replacement, range),
                [import_edit],
                applicability,
            ))
        });
    }
}


Review comment (suggestion):
I think I would slightly prefer something like:


```suggestion
// Unsafe when the fix would delete comments or change a used return value
let applicability = if checker.comment_ranges().intersects(range) 
  || !is_top_level_expression_call(checker, call) {
                Applicability::Unsafe
            } else {
                Applicability::Safe
};
```

Code after:
    }

    let range = call.range();
    let mut diagnostic = checker.report_diagnostic(OsGetcwd, call.func.range());

    if !call.arguments.is_empty() {
        return;
    }

    if is_fix_os_getcwd_enabled(checker.settings()) {
        diagnostic.try_set_fix(|| {
            let (import_edit, binding) = checker.importer().get_or_import_symbol(
                &ImportRequest::import("pathlib", "Path"),
                call.start(),
                checker.semantic(),
            )?;

            // Unsafe when the fix would delete comments or change a used return value
            let applicability = if checker.comment_ranges().intersects(range)
                || !is_top_level_expression_call(checker)
            {
                Applicability::Unsafe
            } else {
                Applicability::Safe
            };

            let replacement = format!("{binding}.cwd()");

            Ok(Fix::applicable_edits(
                Edit::range_replacement(replacement, range),
                [import_edit],
                applicability,
            ))
        });
    }
}
Repository: astral-sh/ruff
File: crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_path_islink.rs (Rust)

Code before:
impl Violation for OsPathIslink {
    const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
    #[derive_message_formats]
    fn message(&self) -> String {
        "`os.path.islink()` should be replaced by `Path.is_symlink()`".to_string()
    }

    fn fix_title(&self) -> Option<String> {
        Some("Replace with `Path(...).is_symlink()`".to_string())
    }
}

/// PTH114
pub(crate) fn os_path_islink(checker: &Checker, call: &ExprCall, segments: &[&str]) {
    if segments != ["os", "path", "islink"] {
        return;
    }

    check_os_pathlib_single_arg_calls(
        checker,
        call,
        "is_symlink()",
        "path",
        is_fix_os_path_islink_enabled(checker.settings()),
        OsPathIslink,
        Some(Applicability::Safe),
    );
}


Review comment (suggestion):
Hmm, are there any cases left where we pass `None`? I think we could just make this a required argument. `None` and `Safe` were being treated as the same anyway, I think, so I don't see any benefit to using the Option.

Code after:
impl Violation for OsPathIslink {
    const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
    #[derive_message_formats]
    fn message(&self) -> String {
        "`os.path.islink()` should be replaced by `Path.is_symlink()`".to_string()
    }

    fn fix_title(&self) -> Option<String> {
        Some("Replace with `Path(...).is_symlink()`".to_string())
    }
}

/// PTH114
pub(crate) fn os_path_islink(checker: &Checker, call: &ExprCall, segments: &[&str]) {
    if segments != ["os", "path", "islink"] {
        return;
    }

    check_os_pathlib_single_arg_calls(
        checker,
        call,
        "is_symlink()",
        "path",
        is_fix_os_path_islink_enabled(checker.settings()),
        OsPathIslink,
        Applicability::Safe,
    );
}
Repository: astral-sh/ruff
File: crates/ruff_linter/src/rules/flake8_use_pathlib/helpers.rs (Rust)

Code before:
}

/// Returns `true` if the given expression looks like a file descriptor, i.e., if it is an integer.
pub(crate) fn is_file_descriptor(expr: &Expr, semantic: &SemanticModel) -> bool {
    if matches!(
        expr,
        Expr::NumberLiteral(ast::ExprNumberLiteral {
            value: ast::Number::Int(_),
            ..
        })
    ) {
        return true;
    }

    let Some(name) = get_name_expr(expr) else {
        return false;
    };

    let Some(binding) = semantic.only_binding(name).map(|id| semantic.binding(id)) else {
        return false;
    };

    typing::is_int(binding, semantic)
}

#[allow(clippy::too_many_arguments)]
pub(crate) fn check_os_pathlib_two_arg_calls(
    checker: &Checker,
    call: &ExprCall,
    attr: &str,
    path_arg: &str,
    second_arg: &str,
    fix_enabled: bool,
    violation: impl Violation,
    applicability: Option<Applicability>,
) {
    let range = call.range();
    let mut diagnostic = checker.report_diagnostic(violation, call.func.range());

    let (Some(path_expr), Some(second_expr)) = (
        call.arguments.find_argument_value(path_arg, 0),
        call.arguments.find_argument_value(second_arg, 1),
    ) else {
        return;
    };

    let path_code = checker.locator().slice(path_expr.range());
    let second_code = checker.locator().slice(second_expr.range());

    if fix_enabled {
        diagnostic.try_set_fix(|| {

Review comment (suggestion):
Let's use `expect` instead of `allow`. That will remind us to remove it if we ever remove arguments.


```suggestion
#[expect(clippy::too_many_arguments)]
```

Code after:
        expr,
        Expr::NumberLiteral(ast::ExprNumberLiteral {
            value: ast::Number::Int(_),
            ..
        })
    ) {
        return true;
    }

    let Some(name) = get_name_expr(expr) else {
        return false;
    };

    let Some(binding) = semantic.only_binding(name).map(|id| semantic.binding(id)) else {
        return false;
    };

    typing::is_int(binding, semantic)
}

#[expect(clippy::too_many_arguments)]
pub(crate) fn check_os_pathlib_two_arg_calls(
    checker: &Checker,
    call: &ExprCall,
    attr: &str,
    path_arg: &str,
    second_arg: &str,
    fix_enabled: bool,
    violation: impl Violation,
    applicability: Applicability,
) {
    let range = call.range();
    let mut diagnostic = checker.report_diagnostic(violation, call.func.range());

    let (Some(path_expr), Some(second_expr)) = (
        call.arguments.find_argument_value(path_arg, 0),
        call.arguments.find_argument_value(second_arg, 1),
    ) else {
        return;
    };

    let path_code = checker.locator().slice(path_expr.range());
    let second_code = checker.locator().slice(second_expr.range());

    if fix_enabled {
        diagnostic.try_set_fix(|| {
            let (import_edit, binding) = checker.importer().get_or_import_symbol(
                &ImportRequest::import("pathlib", "Path"),
                call.start(),
                checker.semantic(),
            )?;
Repository: astral-sh/ruff
File: crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_path_exists.rs (Rust)

Code before:
use ruff_macros::{ViolationMetadata, derive_message_formats};
use ruff_python_ast::ExprCall;

use crate::checkers::ast::Checker;
use crate::preview::is_fix_os_path_exists_enabled;
use crate::rules::flake8_use_pathlib::helpers::check_os_pathlib_single_arg_calls;
use crate::{FixAvailability, Violation};
use ruff_diagnostics::Applicability;

/// ## What it does
/// Checks for uses of `os.path.exists`.
///
/// ## Why is this bad?
/// `pathlib` offers a high-level API for path manipulation, as compared to
/// the lower-level API offered by `os.path`. When possible, using `Path` object
/// methods such as `Path.exists()` can improve readability over the `os.path`
/// module's counterparts (e.g., `os.path.exists()`).
///
/// ## Examples
/// ```python
/// import os
///
/// os.path.exists("file.py")
/// ```
///
/// Use instead:
/// ```python
/// from pathlib import Path
///
/// Path("file.py").exists()
/// ```
///
/// ## Known issues

Review comment (refactor):
```suggestion
use ruff_diagnostics::Applicability;
use ruff_macros::{ViolationMetadata, derive_message_formats};
use ruff_python_ast::ExprCall;

use crate::checkers::ast::Checker;
use crate::preview::is_fix_os_path_exists_enabled;
use crate::rules::flake8_use_pathlib::helpers::check_os_pathlib_single_arg_calls;
use crate::{FixAvailability, Violation};
```

Code after:
use ruff_diagnostics::Applicability;
use ruff_macros::{ViolationMetadata, derive_message_formats};
use ruff_python_ast::ExprCall;

use crate::checkers::ast::Checker;
use crate::preview::is_fix_os_path_exists_enabled;
use crate::rules::flake8_use_pathlib::helpers::check_os_pathlib_single_arg_calls;
use crate::{FixAvailability, Violation};

/// ## What it does
/// Checks for uses of `os.path.exists`.
///
/// ## Why is this bad?
/// `pathlib` offers a high-level API for path manipulation, as compared to
/// the lower-level API offered by `os.path`. When possible, using `Path` object
/// methods such as `Path.exists()` can improve readability over the `os.path`
/// module's counterparts (e.g., `os.path.exists()`).
///
/// ## Examples
/// ```python
/// import os
///
/// os.path.exists("file.py")
/// ```
///
/// Use instead:
/// ```python
/// from pathlib import Path
///
/// Path("file.py").exists()
/// ```
///
/// ## Known issues
Repository: astral-sh/ruff
File: crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_readlink.rs (Rust)

Code before:
use ruff_macros::{ViolationMetadata, derive_message_formats};
use ruff_python_ast::{ExprCall, PythonVersion};

use crate::checkers::ast::Checker;
use crate::preview::is_fix_os_readlink_enabled;
use crate::rules::flake8_use_pathlib::helpers::{
    check_os_pathlib_single_arg_calls, is_keyword_only_argument_non_default,
    is_top_level_expression_call,
};
use crate::{FixAvailability, Violation};
use ruff_diagnostics::Applicability;

/// ## What it does
/// Checks for uses of `os.readlink`.
///
/// ## Why is this bad?
/// `pathlib` offers a high-level API for path manipulation, as compared to
/// the lower-level API offered by `os`. When possible, using `Path` object
/// methods such as `Path.readlink()` can improve readability over the `os`
/// module's counterparts (e.g., `os.readlink()`).
///
/// ## Examples
/// ```python
/// import os
///
/// os.readlink(file_name)
/// ```
///
/// Use instead:
/// ```python
/// from pathlib import Path
///
/// Path(file_name).readlink()
/// ```
///
/// ## Known issues

Review comment (refactor):
```suggestion
use ruff_diagnostics::Applicability;
use ruff_macros::{ViolationMetadata, derive_message_formats};
use ruff_python_ast::{ExprCall, PythonVersion};

use crate::checkers::ast::Checker;
use crate::preview::is_fix_os_readlink_enabled;
use crate::rules::flake8_use_pathlib::helpers::{
check_os_pathlib_single_arg_calls, is_keyword_only_argument_non_default,
    is_top_level_expression_call,
};
use crate::{FixAvailability, Violation};
```

Code after:
use ruff_diagnostics::Applicability;
use ruff_macros::{ViolationMetadata, derive_message_formats};
use ruff_python_ast::{ExprCall, PythonVersion};

use crate::checkers::ast::Checker;
use crate::preview::is_fix_os_readlink_enabled;
use crate::rules::flake8_use_pathlib::helpers::{
    check_os_pathlib_single_arg_calls, is_keyword_only_argument_non_default,
    is_top_level_expression_call,
};
use crate::{FixAvailability, Violation};

/// ## What it does
/// Checks for uses of `os.readlink`.
///
/// ## Why is this bad?
/// `pathlib` offers a high-level API for path manipulation, as compared to
/// the lower-level API offered by `os`. When possible, using `Path` object
/// methods such as `Path.readlink()` can improve readability over the `os`
/// module's counterparts (e.g., `os.readlink()`).
///
/// ## Examples
/// ```python
/// import os
///
/// os.readlink(file_name)
/// ```
///
/// Use instead:
/// ```python
/// from pathlib import Path
///
/// Path(file_name).readlink()
/// ```
///
/// ## Known issues
Repository: astral-sh/ruff
File: playground/ty/vite.config.ts (TypeScript)

Code before:
import { defineConfig } from "vite";
import tailwindcss from "@tailwindcss/vite";
import react from "@vitejs/plugin-react-swc";
import { dirname, join } from "path";
import { fileURLToPath } from "url";
import { viteStaticCopy } from "vite-plugin-static-copy";

const PYODIDE_EXCLUDE = [
  "!**/*.{md,html}",
  "!**/*.d.ts",
  "!**/*.whl",
  "!**/node_modules",
];

// https://vitejs.dev/config/
export default defineConfig({
  plugins: [react(), tailwindcss(), viteStaticCopyPyodide()],
  optimizeDeps: { exclude: ["pyodide", "ty_wasm"] },
});

export function viteStaticCopyPyodide() {
  const pyodideDir = dirname(fileURLToPath(import.meta.resolve("pyodide")));
  return viteStaticCopy({
    targets: [
      {
        src: [join(pyodideDir, "*").replace(/\\/g, "/"), ...PYODIDE_EXCLUDE],
        dest: "assets",
      },
    ],
  });
}


Review comment (suggestion):
We use vite for the production builds. Can you verify that the `vite build` reported sizes are the same before and after your changes

Code after:
import { defineConfig } from "vite";
import tailwindcss from "@tailwindcss/vite";
import react from "@vitejs/plugin-react-swc";
import { dirname, join } from "path";
import { fileURLToPath } from "url";
import { normalizePath } from "vite";
import { viteStaticCopy } from "vite-plugin-static-copy";

const PYODIDE_EXCLUDE = [
  "!**/*.{md,html}",
  "!**/*.d.ts",
  "!**/*.whl",
  "!**/node_modules",
];

// https://vitejs.dev/config/
export default defineConfig({
  plugins: [react(), tailwindcss(), viteStaticCopyPyodide()],
  optimizeDeps: { exclude: ["pyodide", "ty_wasm"] },
});

export function viteStaticCopyPyodide() {
  const pyodideDir = normalizePath(
    join(dirname(fileURLToPath(import.meta.resolve("pyodide"))), "*"),
  );
  return viteStaticCopy({
    targets: [
      {
        src: [pyodideDir, ...PYODIDE_EXCLUDE],
        dest: "assets",
      },
    ],
  });
}
Repository: astral-sh/ruff
File: crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM222.py (Python)

Code before:

# https://github.com/astral-sh/ruff/issues/7127
def f(a: "'b' or 'c'"): ...

# https://github.com/astral-sh/ruff/issues/20703
print(f"{b''}" or "bar")  # SIM222
x = 1
print(f"{x=}" or "bar")  # SIM222
(lambda: 1) or True  # SIM222
(i for i in range(1)) or "bar"  # SIM222

# https://github.com/astral-sh/ruff/issues/21136
def get_items():
    return tuple(item for item in Item.objects.all()) or None  # OK


def get_items_list():
    return tuple([item for item in items]) or None  # OK


def get_items_set():
    return tuple({item for item in items}) or None  # OK


# https://github.com/astral-sh/ruff/issues/21473
tuple("") or True  # OK
tuple(0) or True  # OK
tuple(1) or True  # OK
tuple(False) or True  # OK
tuple(None) or True  # OK


Review comment (suggestion):
We should add a t-string case:


```suggestion
tuple(t"") or True  # OK
```

Code after:

# https://github.com/astral-sh/ruff/issues/7127
def f(a: "'b' or 'c'"): ...

# https://github.com/astral-sh/ruff/issues/20703
print(f"{b''}" or "bar")  # SIM222
x = 1
print(f"{x=}" or "bar")  # SIM222
(lambda: 1) or True  # SIM222
(i for i in range(1)) or "bar"  # SIM222

# https://github.com/astral-sh/ruff/issues/21136
def get_items():
    return tuple(item for item in Item.objects.all()) or None  # OK


def get_items_list():
    return tuple([item for item in items]) or None  # OK


def get_items_set():
    return tuple({item for item in items}) or None  # OK


# https://github.com/astral-sh/ruff/issues/21473
tuple("") or True  # SIM222
tuple(t"") or True  # OK
tuple(0) or True  # OK
tuple(1) or True  # OK
tuple(False) or True  # OK
tuple(None) or True  # OK
tuple(...) or True  # OK
tuple(lambda x: x) or True  # OK
tuple(x for x in range(0)) or True  # OK
Repository: astral-sh/ruff
File: crates/ruff_python_formatter/src/expression/expr_unary_op.rs (Rust)

Code before:
}

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

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

        let operand_start = operand_start(self, context.source());
        if context
            .comments()
            .dangling(self)
            .iter()
            .any(|comment| comment.end() < operand_start)
        {
            return OptionalParentheses::Multiline;
        }

        if context.comments().has(self.operand.as_ref()) {
            OptionalParentheses::Always
        } else {
            self.operand.needs_parentheses(self.into(), context)
        }
    }
}

/// Returns the start of `unary_op`'s operand, or its leading parenthesis, if it has one.
pub(crate) fn operand_start(unary_op: &ExprUnaryOp, source: &str) -> TextSize {
    let mut tokenizer = SimpleTokenizer::new(
        source,
        TextRange::new(unary_op.start(), unary_op.operand.start()),
    )
    .skip_trivia();
    let op_token = tokenizer.next();
    debug_assert!(op_token.is_some_and(|token| matches!(
        token.kind,
        SimpleTokenKind::Tilde
            | SimpleTokenKind::Not
            | SimpleTokenKind::Plus

Review comment (refactor):
I think we can simplify this to returning `Multiline` when there's any dangling comment. 

Does this need to take precedence over the `Never` case when the `operand` is parenthesized?

Code after:
        ) {
            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(),
        comments.ranges(),
        context.source(),
    );
    let leading_operand_comments = comments.leading(item.operand.as_ref());
    let has_leading_comments_before_parens = parenthesized_operand_range.is_some_and(|range| {
        leading_operand_comments
            .iter()
            .any(|comment| comment.start() < range.start())
    });

    !leading_operand_comments.is_empty()
        && !is_expression_parenthesized(
            item.operand.as_ref().into(),
            context.comments().ranges(),
            context.source(),
        )
        || has_leading_comments_before_parens
}