ronantakizawa/codereview-bench huggingface dataset:
Repository: astral-sh/ruff
File: crates/ruff_linter/src/rules/flake8_blind_except/rules/blind_except.rs (Rust)

Code before:
) {
    let Some(type_) = type_ else {
        return;
    };

    let semantic = checker.semantic();
    let Some((builtin_exception_type, range)) = contains_blind_exception(semantic, type_) else {
        return;
    };

    // If the exception is re-raised, don't flag an error.
    let mut visitor = ReraiseVisitor::new(name);
    visitor.visit_body(body);
    if visitor.seen() {
        return;
    }

    // If the exception is logged, don't flag an error.
    let mut visitor = LogExceptionVisitor::new(
        semantic,
        &checker.settings().logger_objects,
        checker.settings(),
    );
    visitor.visit_body(body);
    if visitor.seen() {
        return;
    }

    checker.report_diagnostic(
        BlindExcept {
            name: builtin_exception_type.into(),
        },
        range,
    );
}

/// A visitor to detect whether the exception with the given name was re-raised.
struct ReraiseVisitor<'a> {
    name: Option<&'a str>,
    seen: bool,
}

impl<'a> ReraiseVisitor<'a> {
    /// Create a new [`ReraiseVisitor`] with the given exception name.
    fn new(name: Option<&'a str>) -> Self {
        Self { name, seen: false }
    }

    /// Returns `true` if the exception was re-raised.
    fn seen(&self) -> bool {
        self.seen

Review comment (refactor):
This looks a bit redundant right now, but I'm leaning toward leaving it like this because it should make it slightly easier to remove the preview gating later. Otherwise I would suggest removing the separate `logger_objects` argument and just extracting that from the full settings in `new`.

Code after:
    checker: &Checker,
    type_: Option<&Expr>,
    name: Option<&str>,
    body: &[Stmt],
) {
    let Some(type_) = type_ else {
        return;
    };

    let semantic = checker.semantic();
    let Some((builtin_exception_type, range)) = contains_blind_exception(semantic, type_) else {
        return;
    };

    // If the exception is re-raised, don't flag an error.
    let mut visitor = ReraiseVisitor::new(name);
    visitor.visit_body(body);
    if visitor.seen() {
        return;
    }

    // If the exception is logged, don't flag an error.
    let mut visitor = LogExceptionVisitor::new(
        semantic,
        &checker.settings().logger_objects,
        checker.settings(),
    );
    visitor.visit_body(body);
    if visitor.seen() {
        return;
    }

    checker.report_diagnostic(
        BlindExcept {
            name: builtin_exception_type.into(),
        },
        range,
    );
}

/// A visitor to detect whether the exception with the given name was re-raised.
struct ReraiseVisitor<'a> {
    name: Option<&'a str>,
    seen: bool,
}

impl<'a> ReraiseVisitor<'a> {
    /// Create a new [`ReraiseVisitor`] with the given exception name.
    fn new(name: Option<&'a str>) -> Self {
        Self { name, seen: false }
    }
Repository: astral-sh/ruff
File: crates/ruff_linter/resources/test/fixtures/flake8_blind_except/BLE.py (Python)

Code before:
except Exception as e:
    raise bad
except BaseException:
    pass

import logging

try:
    pass
except Exception:
    logging.error("...")


try:
    pass
except Exception:
    logging.error("...", exc_info=False)


try:
    pass
except Exception:
    logging.error("...", exc_info=None)


try:
    pass
except Exception:
    logging.error("...", exc_info=True)

try:
    pass
except Exception as e:
    logging.error("...", exc_info=e)


try:
    pass
except Exception:
    logging.exception("...")


try:
    pass
except Exception as e:
    logging.debug("...", exc_info=e)


try:
    pass
except Exception:

Review comment (suggestion):
Sorry for another test nit, but could you move the new cases to the bottom of the file? That avoids the churn in the snapshot line numbers. And annotating the logging lines with either `# ok` or `# BLE001` depending on if we expect a diagnostic would also be a big help (although I think these are all okay?)

Code after:
except Exception as e:
    raise bad
except BaseException:
    pass

import logging

try:
    pass
except Exception:
    logging.error("...")  # BLE001


try:
    pass
except Exception:
    logging.error("...", exc_info=False)  # BLE001


try:
    pass
except Exception:
    logging.error("...", exc_info=None)  # BLE001


try:
    pass
except Exception:
    logging.exception("...")  # ok


try:
    pass
except Exception:
    logging.error("...", exc_info=True)  # ok


from logging import critical, error, exception

try:
    pass
except Exception:
    error("...")  # BLE001


try:
    pass
except Exception:
    error("...", exc_info=False)  # BLE001
Repository: astral-sh/ruff
File: crates/ruff_linter/src/rules/ruff/rules/invalid_rule_code.rs (Rust)

Code before:
///
/// ## Options
/// - `lint.external`
#[derive(ViolationMetadata)]
#[violation_metadata(preview_since = "0.11.4")]
pub(crate) struct InvalidRuleCode {
    pub(crate) rule_code: String,
    pub(crate) kind: InvalidRuleCodeKind,
    pub(crate) whole_comment: bool,
}

impl AlwaysFixableViolation for InvalidRuleCode {
    #[derive_message_formats]
    fn message(&self) -> String {
        format!(
            "Invalid rule code in {}: {}",
            self.kind.as_str(),
            self.rule_code
        )
    }

    fn fix_title(&self) -> String {
        if self.whole_comment {
            format!("Remove the {} comment", self.kind.as_str())
        } else {
            format!("Remove rule code {}", self.rule_code)
        }
    }
}

/// RUF102 for invalid noqa codes
pub(crate) fn invalid_noqa_code(
    context: &LintContext,
    noqa_directives: &NoqaDirectives,
    locator: &Locator,
    external: &[String],
) {
    for line in noqa_directives.lines() {
        let Directive::Codes(directive) = &line.directive else {
            continue;
        };

        let all_valid = directive
            .iter()
            .all(|code| code_is_valid(code.as_str(), external));

        if all_valid {
            continue;
        }

        let (valid_codes, invalid_codes): (Vec<_>, Vec<_>) = directive

Review comment (suggestion):
```suggestion
            format!("Remove the rule code `{}`", self.rule_code)
```

Code after:
///
/// ## Options
/// - `lint.external`
#[derive(ViolationMetadata)]
#[violation_metadata(preview_since = "0.11.4")]
pub(crate) struct InvalidRuleCode {
    pub(crate) rule_code: String,
    pub(crate) kind: InvalidRuleCodeKind,
    pub(crate) whole_comment: bool,
}

impl AlwaysFixableViolation for InvalidRuleCode {
    #[derive_message_formats]
    fn message(&self) -> String {
        format!(
            "Invalid rule code in {}: {}",
            self.kind.as_str(),
            self.rule_code
        )
    }

    fn fix_title(&self) -> String {
        if self.whole_comment {
            format!("Remove the {} comment", self.kind.as_str())
        } else {
            format!("Remove the rule code `{}`", self.rule_code)
        }
    }
}

/// RUF102 for invalid noqa codes
pub(crate) fn invalid_noqa_code(
    context: &LintContext,
    noqa_directives: &NoqaDirectives,
    locator: &Locator,
    external: &[String],
) {
    for line in noqa_directives.lines() {
        let Directive::Codes(directive) = &line.directive else {
            continue;
        };

        let all_valid = directive
            .iter()
            .all(|code| code_is_valid(code.as_str(), external));

        if all_valid {
            continue;
        }

        let (valid_codes, invalid_codes): (Vec<_>, Vec<_>) = directive
Repository: astral-sh/ruff
File: crates/ty_server/tests/e2e/commands.rs (Rust)

Code before:
    let foo = SystemPath::new("src/foo.py");
    let foo_content = "\
def foo() -> str:
return 42
";

    let mut server = TestServerBuilder::new()?
        .with_workspace(workspace_root, None)?
        .with_file(foo, foo_content)?
        .enable_pull_diagnostics(false)
        .build()
        .wait_until_workspaces_are_initialized();

    let response = execute_command(&mut server, "ty.printDebugInformation".to_string(), vec![]);
    let response = response.expect("expect server response");

    let response = response
        .as_str()
        .expect("debug command to return a string response");

    insta::with_settings!({
        filters => vec![
            (r"\b[0-9]+.[0-9]+MB\b","[X.XXMB]"),
            (r"Workspace .+\)","Workspace XXX"),
            (r"Project at .+","Project at XXX"),
            (r"rules: \{(.|\n)+?\}\,","rules: {<RULES>},"),
    ]}, {
        insta::assert_snapshot!(response);
    });

    Ok(())
}


Review comment (suggestion):
```suggestion
            (r"rules: \{(.|\n)+?\}\,","rules: <RULES>,"),
```

Code after:
    let foo = SystemPath::new("src/foo.py");
    let foo_content = "\
def foo() -> str:
return 42
";

    let mut server = TestServerBuilder::new()?
        .with_workspace(workspace_root, None)?
        .with_file(foo, foo_content)?
        .enable_pull_diagnostics(false)
        .build()
        .wait_until_workspaces_are_initialized();

    let response = execute_command(&mut server, "ty.printDebugInformation".to_string(), vec![]);
    let response = response.expect("expect server response");

    let response = response
        .as_str()
        .expect("debug command to return a string response");

    insta::with_settings!({
        filters => vec![
            (r"\b[0-9]+.[0-9]+MB\b", "[X.XXMB]"),
            (r"Workspace .+\)", "Workspace XXX"),
            (r"Project at .+", "Project at XXX"),
            (r"rules: \{(.|\n)+?\}\,", "rules: <RULES>,"),
    ]}, {
        insta::assert_snapshot!(response);
    });

    Ok(())
}
Repository: astral-sh/ruff
File: crates/ty/tests/cli/main.rs (Rust)

Code before:

    assert_cmd_snapshot!(case.command(), @r###"
    success: true
    exit_code: 0
    ----- stdout -----
    info[revealed-type]: Revealed type
     --> test.py:4:13
      |
    2 | from typing_extensions import reveal_type
    3 | total = 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 +…
    4 | reveal_type(total)
      |             ^^^^^ `Literal[2000]`
      |

    Found 1 diagnostic

    ----- stderr -----
    "###);

    Ok(())
}

#[test_case::test_case("concise")]
#[test_case::test_case("full")]
#[test_case::test_case("github")]
#[test_case::test_case("gitlab")]
fn output_format(output_format: &str) -> anyhow::Result<()> {
    let case = CliTest::with_file(
        "test.py",
        "
        from typing_extensions import reveal_type
        reveal_type('str'.lower())  # revealed-type
        match 42:  # invalid-syntax
            case _: ...
        ",
    )?;

    let snapshot = format!("output_format_{output_format}");

    assert_cmd_snapshot!(
        snapshot,
        case.command()
            .args(["--output-format", output_format, "--target-version", "3.9"])
    );

    Ok(())
}

pub(crate) struct CliTest {
    _temp_dir: TempDir,
    settings: Settings,

Review comment (refactor):
Nit: I would probably create a helper function and call that from separate `concise_output`, `full_output` etc. functions over using test case, given that the test cases are very limited (has better IDE support)

Code after:
    ----- stdout -----
    test.py:5:13: info[revealed-type] Revealed type: `Literal["hello"]`
    Found 1 diagnostic

    ----- stderr -----
    "###);

    Ok(())
}

#[test]
fn can_handle_large_binop_expressions() -> anyhow::Result<()> {
    let mut content = String::new();
    writeln!(
        &mut content,
        "
        from typing_extensions import reveal_type
        total = 1{plus_one_repeated}
        reveal_type(total)
        ",
        plus_one_repeated = " + 1".repeat(2000 - 1)
    )?;

    let case = CliTest::with_file("test.py", &ruff_python_trivia::textwrap::dedent(&content))?;

    assert_cmd_snapshot!(case.command(), @r###"
    success: true
    exit_code: 0
    ----- stdout -----
    info[revealed-type]: Revealed type
     --> test.py:4:13
      |
    2 | from typing_extensions import reveal_type
    3 | total = 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 +…
    4 | reveal_type(total)
      |             ^^^^^ `Literal[2000]`
      |

    Found 1 diagnostic

    ----- stderr -----
    "###);

    Ok(())
}

pub(crate) struct CliTest {
    _temp_dir: TempDir,
    settings: Settings,
    settings_scope: Option<SettingsBindDropGuard>,
    project_dir: PathBuf,
Repository: astral-sh/ruff
File: crates/ty_python_semantic/src/types/function.rs (Rust)

Code before:
    /// `typing(_extensions).override`
    Override,
    /// `typing(_extensions).is_protocol`
    IsProtocol,
    /// `typing(_extensions).get_protocol_members`
    GetProtocolMembers,
    /// `typing(_extensions).runtime_checkable`
    RuntimeCheckable,
    /// `typing(_extensions).dataclass_transform`
    DataclassTransform,

    /// `abc.abstractmethod`
    #[strum(serialize = "abstractmethod")]
    AbstractMethod,

    /// `contextlib.asynccontextmanager`
    #[strum(serialize = "asynccontextmanager")]
    AsyncContextManager,

    /// `dataclasses.dataclass`
    Dataclass,
    /// `dataclasses.field`
    Field,

    /// `functools.total_ordering`
    #[strum(serialize = "total_ordering")]
    TotalOrdering,

    /// `inspect.getattr_static`
    GetattrStatic,

    /// `ty_extensions.static_assert`
    StaticAssert,
    /// `ty_extensions.is_equivalent_to`
    IsEquivalentTo,
    /// `ty_extensions.is_subtype_of`
    IsSubtypeOf,
    /// `ty_extensions.is_assignable_to`
    IsAssignableTo,
    /// `ty_extensions.is_disjoint_from`
    IsDisjointFrom,
    /// `ty_extensions.is_singleton`
    IsSingleton,
    /// `ty_extensions.is_single_valued`
    IsSingleValued,
    /// `ty_extensions.generic_context`
    GenericContext,
    /// `ty_extensions.into_callable`
    IntoCallable,
    /// `ty_extensions.dunder_all_names`
    DunderAllNames,

Review comment (bug):
this doesn't seem necessary, I think strum's default is fine here. No tests fail with it removed:

```suggestion
```

Code after:
    Cast,
    /// `typing(_extensions).overload`
    Overload,
    /// `typing(_extensions).override`
    Override,
    /// `typing(_extensions).is_protocol`
    IsProtocol,
    /// `typing(_extensions).get_protocol_members`
    GetProtocolMembers,
    /// `typing(_extensions).runtime_checkable`
    RuntimeCheckable,
    /// `typing(_extensions).dataclass_transform`
    DataclassTransform,

    /// `abc.abstractmethod`
    #[strum(serialize = "abstractmethod")]
    AbstractMethod,

    /// `contextlib.asynccontextmanager`
    #[strum(serialize = "asynccontextmanager")]
    AsyncContextManager,

    /// `dataclasses.dataclass`
    Dataclass,
    /// `dataclasses.field`
    Field,

    /// `functools.total_ordering`
    TotalOrdering,

    /// `inspect.getattr_static`
    GetattrStatic,

    /// `ty_extensions.static_assert`
    StaticAssert,
    /// `ty_extensions.is_equivalent_to`
    IsEquivalentTo,
    /// `ty_extensions.is_subtype_of`
    IsSubtypeOf,
    /// `ty_extensions.is_assignable_to`
    IsAssignableTo,
    /// `ty_extensions.is_disjoint_from`
    IsDisjointFrom,
    /// `ty_extensions.is_singleton`
    IsSingleton,
    /// `ty_extensions.is_single_valued`
    IsSingleValued,
    /// `ty_extensions.generic_context`
    GenericContext,
    /// `ty_extensions.into_callable`
    IntoCallable,
Repository: astral-sh/ruff
File: crates/ruff_linter/src/rules/pylint/rules/swap_with_temporary_variable.rs (Rust)

Code before:
                return None;
            };

            (target_variable, &stmt_assign.value)
        }
        Stmt::AnnAssign(stmt_ann_assign) => {
            // only assignments that actually assign a value are relevant here
            let Some(value) = &stmt_ann_assign.value else {
                return None;
            };

            (&*stmt_ann_assign.target, value)
        }
        // Stmt::AugAssign is not relevant because it modifies the content
        // of a variable based on its existing value, so it can't swap variables
        _ => return None,
    };

    // assignment value is more complex than just a simple variable, skip such cases.
    if let (Some(target_expr), Some(value_expr)) =
        (target.clone().name_expr(), value.clone().name_expr())
    {
        Some(VarToVarAssignment {
            target_var_name: target_expr.id,
            value_var_name: value_expr.id,
            range: stmt.range(),
        })
    } else {
        None
    }
}


Review comment (suggestion):
Could we avoid cloning here too? `VarToVarAssignment` may need a lifetime bound, but I think it should be doable.

Code after:
)> {
    let binding = semantic.binding(binding_id);

    // Only consider simple assignments (no imports, function defs, etc.)
    if !binding.kind.is_assignment() {
        return None;
    }

    let node_id = binding.source?;

    let stmt = binding.statement(semantic)?;
    let stmt_a = VarToVarAssignment::from_stmt(stmt)?;

    // Find the enclosing suite so we can look at the next siblings.
    // For the global scope, use the module body; otherwise, find the parent statement.
    let suite = if scope_id.is_global() {
        traversal::EnclosingSuite::new(semantic.definitions.python_ast()?, stmt.into())
    } else {
        traversal::suite(stmt, semantic.parent_statement(node_id)?)
    }?;

    let stmt_b = VarToVarAssignment::from_stmt(suite.next_sibling()?)?;
    let stmt_c = VarToVarAssignment::from_stmt(suite.next_siblings().get(1)?)?;

    Some((stmt_a, stmt_b, stmt_c))
}

#[derive(Eq, PartialEq, Debug, Clone)]
struct VarToVarAssignment<'a> {
    target: &'a Name,
    value: &'a Name,
    range: TextRange,
}

impl Ranged for VarToVarAssignment<'_> {
    fn range(&self) -> TextRange {
        self.range
    }
}

impl<'a> VarToVarAssignment<'a> {
    fn from_stmt(stmt: &'a Stmt) -> Option<VarToVarAssignment<'a>> {
        let (target, value) = match stmt {
            Stmt::Assign(stmt_assign) => {
                // only one variable is expected for matching the pattern
                let [target_variable] = stmt_assign.targets.as_slice() else {
                    return None;
                };

                (target_variable, &stmt_assign.value)
            }
Repository: astral-sh/ruff
File: crates/ruff_linter/src/rules/pylint/rules/swap_with_temporary_variable.rs (Rust)

Code before:
/// more clear.
///
/// ## Example
/// ```python
/// def function(x, y):
///     if x > y:
///         temp = x
///         x = y
///         y = temp
///     assert x <= y
/// ```
///
/// Use instead:
/// ```python
/// def function(x, y):
///     if x > y:
///         x, y = y, x
///     assert x <= y
/// ```
///
/// ## Fix safety
/// The rule's fix is marked as safe, unless it contains comments. In this
/// exception case, applying the quick fix would remove comments between the
/// assignment statements.
#[derive(ViolationMetadata)]
#[violation_metadata(preview_since = "0.15.1")]
pub(crate) struct SwapWithTemporaryVariable<'a> {
    first_var: &'a Name,
    second_var: &'a Name,
    is_temp_var_reused: bool,
}

impl Violation for SwapWithTemporaryVariable<'_> {
    const FIX_AVAILABILITY: crate::FixAvailability = crate::FixAvailability::Sometimes;

    #[derive_message_formats]
    fn message(&self) -> String {
        let SwapWithTemporaryVariable {
            first_var,
            second_var,
            is_temp_var_reused: _,
        } = self;
        format!("Consider swapping `{first_var}` and `{second_var}` by using tuple unpacking")
    }

    fn fix_title(&self) -> Option<String> {
        let SwapWithTemporaryVariable {
            first_var,
            second_var,
            is_temp_var_reused,
        } = self;

Review comment (suggestion):
This is going to be outdated in a few minutes :) We recently added the special `NEXT_RUFF_VERSION` variable that we can use here:


```suggestion
#[violation_metadata(preview_since = "NEXT_RUFF_VERSION")]
```

Code after:
///
/// ## Example
/// ```python
/// def function(x, y):
///     if x > y:
///         temp = x
///         x = y
///         y = temp
///     assert x <= y
/// ```
///
/// Use instead:
/// ```python
/// def function(x, y):
///     if x > y:
///         x, y = y, x
///     assert x <= y
/// ```
///
/// ## Fix safety
/// The rule's fix is marked as safe, unless the replacement range contains comments
/// that would be removed.
#[derive(ViolationMetadata)]
#[violation_metadata(preview_since = "NEXT_RUFF_VERSION")]
pub(crate) struct SwapWithTemporaryVariable<'a> {
    first: &'a Name,
    second: &'a Name,
}

impl Violation for SwapWithTemporaryVariable<'_> {
    const FIX_AVAILABILITY: crate::FixAvailability = crate::FixAvailability::Sometimes;

    #[derive_message_formats]
    fn message(&self) -> String {
        "Unnecessary temporary variable".to_string()
    }

    fn fix_title(&self) -> Option<String> {
        let SwapWithTemporaryVariable { first, second } = self;

        Some(format!(
            "Use `{first}, {second} = {second}, {first}` instead"
        ))
    }
}

pub(crate) fn swap_with_temporary_variable(checker: &Checker, scope_id: ScopeId, scope: &Scope) {
    let consecutive_assignments = scope.binding_ids().filter_map(|binding_id| {
        match_consecutive_assignments(checker.semantic(), scope_id, binding_id)
    });