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

Code before:
/// ```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 = "NEXT_RUFF_VERSION")]
pub(crate) struct SwapWithTemporaryVariable<'a> {
    first_var: &'a Name,
    second_var: &'a Name,
}

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,
        } = 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,
        } = self;

        Some(format!(
            "Use `{first_var}, {second_var} = {second_var}, {first_var}` instead"
        ))
    }
}

Review comment (suggestion):
nit: I don't think the `_var` suffix is that helpful, so I'd probably just drop it:


```suggestion
    first: &'a Name,
    second: &'a Name,
```

Code after:
/// ```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)
    });

    for (stmt_a, stmt_b, stmt_c) in consecutive_assignments {
        // Detect patterns like:
Repository: astral-sh/ruff
File: crates/ruff_linter/resources/test/fixtures/pylint/repeated_equality_comparison.py (Python)

Code before:
# Errors.
foo == "a" or foo == "b"

foo != "a" and foo != "b"

foo == "a" or foo == "b" or foo == "c"

foo != "a" and foo != "b" and foo != "c"

foo == a or foo == "b" or foo == 3  # Mixed types.

"a" == foo or "b" == foo or "c" == foo

"a" != foo and "b" != foo and "c" != foo

"a" == foo or foo == "b" or "c" == foo

foo == bar or baz == foo or qux == foo

foo == "a" or "b" == foo or foo == "c"

foo != "a" and "b" != foo and foo != "c"

foo == "a" or foo == "b" or "c" == bar or "d" == bar  # Multiple targets

foo.bar == "a" or foo.bar == "b"  # Attributes.

# OK
foo == "a" and foo == "b" and foo == "c"  # `and` mixed with `==`.

foo != "a" or foo != "b" or foo != "c"  # `or` mixed with `!=`.

foo == a or foo == b() or foo == c  # Call expression.

foo != a or foo() != b or foo != c  # Call expression.

foo in {"a", "b", "c"}  # Uses membership test already.

foo not in {"a", "b", "c"}  # Uses membership test already.

foo == "a"  # Single comparison.

foo != "a"  # Single comparison.

foo == "a" == "b" or foo == "c"  # Multiple comparisons.

foo == bar == "b" or foo == "c"  # Multiple comparisons.

foo == foo or foo == bar  # Self-comparison.

foo[0] == "a" or foo[0] == "b"  # Subscripts.

foo() == "a" or foo() == "b"  # Calls.

import sys

sys.platform == "win32" or sys.platform == "emscripten"  # sys attributes

foo == "a" or "c" == bar or foo == "b" or "d" == bar  # Multiple targets

foo == "a" or ("c" == bar or "d" == bar) or foo == "b"  # Multiple targets

foo == "a" or foo == "b" or "c" != bar and "d" != bar  # Multiple targets

foo == "a" or ("c" != bar and "d" != bar) or foo == "b"  # Multiple targets

foo == "a" and "c" != bar or foo == "b" and "d" != bar  # Multiple targets

foo == 1 or foo == True # Different types, same hashed value

foo == 1 or foo == 1.0 # Different types, same hashed value

foo == False or foo == 0 # Different types, same hashed value

foo == 0.0 or foo == 0j # Different types, same hashed value


Review comment (suggestion):
I think we should consider a rule to get this to reduce to ```foo == "bar"```. Opening a separate issue would probably be best.

Code after:

foo() == "a" or foo() == "b"  # Calls.

import sys

sys.platform == "win32" or sys.platform == "emscripten"  # sys attributes

foo == "a" or "c" == bar or foo == "b" or "d" == bar  # Multiple targets

foo == "a" or ("c" == bar or "d" == bar) or foo == "b"  # Multiple targets

foo == "a" or foo == "b" or "c" != bar and "d" != bar  # Multiple targets

foo == "a" or ("c" != bar and "d" != bar) or foo == "b"  # Multiple targets

foo == "a" and "c" != bar or foo == "b" and "d" != bar  # Multiple targets

foo == 1 or foo == True # Different types, same hashed value

foo == 1 or foo == 1.0 # Different types, same hashed value

foo == False or foo == 0 # Different types, same hashed value

foo == 0.0 or foo == 0j # Different types, same hashed value

foo == "bar" or foo == "bar" # All members identical

foo == "bar" or foo == "bar" or foo == "buzz" # All but one members identical
Repository: astral-sh/ruff
File: crates/ty_ide/src/completion.rs (Rust)

Code before:
        let builder = completion_test_builder(
            "\
3.14.<CURSOR>
",
        );

        builder.build().contains("conjugate");
    }

    #[test]
    fn nested_attribute_access1() {
        let builder = completion_test_builder(
            "\
class A:
    x: str

class B:
    a: A

b = B()
b.a.<CURSOR>
",
        );

        assert_snapshot!(builder.skip_dunders().build().snapshot(),
        @r"
        x
        ");
    }

    #[test]
    fn nested_attribute_access2() {
        let builder = completion_test_builder(
            "\
class B:
    c: int

class A:
    b: B

a = A()
([1] + [a.b.<CURSOR>] + [3]).pop()
",
        );

        builder
            .build()
            .contains("c")
            .not_contains("b")
            .not_contains("pop");
    }

Review comment (suggestion):
Essentially an arbitrary test that I changed (that can make use of this) to not add dead code.

There a lot of other tests that could make nice use of this that I intend to change if this lands, but I didn't want to overdo it ahead of time in case this functionality is not desirable.

Code after:
    fn attribute_on_float() {
        let builder = completion_test_builder(
            "\
3.14.<CURSOR>
",
        );

        builder.build().contains("conjugate");
    }

    #[test]
    fn nested_attribute_access1() {
        let builder = completion_test_builder(
            "\
class A:
    x: str

class B:
    a: A

b = B()
b.a.<CURSOR>
",
        );

        assert_snapshot!(builder.skip_dunders().build().snapshot(),
        @r"
        x
        ");
    }

    #[test]
    fn nested_attribute_access2() {
        let builder = completion_test_builder(
            "\
class B:
    c: int

class A:
    b: B

a = A()
([1] + [a.b.<CURSOR>] + [3]).pop()
",
        );

        builder
            .build()
            .contains("c")
            .not_contains("b")
            .not_contains("pop");
Repository: astral-sh/ruff
File: crates/ruff_linter/resources/test/fixtures/pyupgrade/UP045_py39.py (Python)

Code before:
"""
Regression test for https://github.com/astral-sh/ruff/issues/20096
"""

from __future__ import annotations

from typing import Optional, cast, TypeAlias


x: Optional[str]             # UP045
x: "Optional[str]"           # UP045
cast("Optional[str]", None)  # UP045
cast(Optional[str], None)    # okay, str | None is a runtime error
x: TypeAlias = "Optional[str]"  # UP045
x: TypeAlias = Optional[str]  # okay


Review comment (suggestion):
Can we add some tests for "Complex" string annotations (an annotation that uses implicit string concatenation). 

Code after:
"""
Regression test for https://github.com/astral-sh/ruff/issues/20096
"""

from __future__ import annotations

from typing import Optional, cast, TypeAlias


x: Optional[str]             # UP045
x: "Optional[str]"           # UP045
cast("Optional[str]", None)  # UP045
cast(Optional[str], None)    # okay, str | None is a runtime error
x: TypeAlias = "Optional[str]"  # UP045
x: TypeAlias = Optional[str]  # okay

# complex (implicitly concatenated) annotations
#
# TODO(brent) these aren't currently flagged by the rule but probably should
# be. From what I can tell, this isn't an issue in the rule itself but rather
# with our parsing of such annotations.
x: (
    "Optional"
    "["
    "str"
    "]"
)
cast((
    "Optional"
    "["
    "str"
    "]"
), None)
x: TypeAlias = (
    "Optional"
    "["
    "str"
    "]"
)
Repository: astral-sh/ruff
File: crates/ty_ide/src/docstring.rs (Rust)

Code before:
        wow it did the thing
        >>> thing.do_other_thing()
        it sure did the thing
        ```
        "#;

        let docstring = Docstring::new(docstring.to_owned());

        assert_snapshot!(docstring.render_markdown(), @r"
        My cool func:  
          
        ```python  
        ```````````python
        >>> thing.do_thing()
        wow it did the thing
        >>> thing.do_other_thing()
        it sure did the thing
        ```
        ```````````
        ");
    }

    // `.. code::` is a literal block and the `.. code::` should be deleted
    #[test]
    fn code_block() {
        let docstring = r#"
        Here's some code!

        .. code::
            def main() {
                print("hello world!")
            }
        "#;

        let docstring = Docstring::new(docstring.to_owned());

        assert_snapshot!(docstring.render_markdown(), @r#"
        Here's some code!  
          
          
        ```````````python
            def main() {
                print("hello world!")
            }
        ```````````
        "#);
    }

    // `.. code:: rust` is a literal block with rust syntax highlighting
    #[test]
    fn code_block_lang() {

Review comment (suggestion):
```suggestion
    // TODO: We should not parse the contents of a markdown codefence
```

Code after:
        &nbsp;&nbsp;&nbsp;The `spam` argument is considered evil now.  
          
        &nbsp;&nbsp;&nbsp;You really shouldnt use it  
          
        And that's the docs
        ");
    }

    // I don't know if this is valid syntax but we preserve stuff before non-code blocks like
    // `..deprecated ::`
    #[test]
    fn deprecated_prefix_gunk() {
        let docstring = r#"
        wow this is some changes .. deprecated:: 1.2.3
            x = 2
        "#;

        let docstring = Docstring::new(docstring.to_owned());

        assert_snapshot!(docstring.render_markdown(), @r"
        **wow this is some changes deprecated:** *1.2.3*  
        &nbsp;&nbsp;&nbsp;&nbsp;x = 2
        ");
    }

    // We should not parse the contents of a markdown codefence
    #[test]
    fn explicit_markdown_block_with_ps1_contents() {
        let docstring = r#"
        My cool func:
        
        ```python
        >>> thing.do_thing()
        wow it did the thing
        >>> thing.do_other_thing()
        it sure did the thing
        ```
        "#;

        let docstring = Docstring::new(docstring.to_owned());

        assert_snapshot!(docstring.render_markdown(), @r"
        My cool func:  
          
        ```python
        >>> thing.do_thing()
        wow it did the thing
        >>> thing.do_other_thing()
        it sure did the thing
        ```
        ");
Repository: astral-sh/ruff
File: crates/ruff_python_ast/src/python_version.rs (Rust)

Code before:
            PythonVersion::PY313,
            PythonVersion::PY314,
            PythonVersion::PY315,
        ]
        .into_iter()
    }

    /// The minimum supported Python version.
    pub const fn lowest() -> Self {
        Self::PY37
    }

    pub const fn latest() -> Self {
        Self::PY315
    }

    /// The latest Python version supported in preview
    pub fn latest_preview() -> Self {
        let latest_preview = Self::PY315;
        debug_assert!(latest_preview >= Self::latest());
        latest_preview
    }

    pub const fn latest_ty() -> Self {
        // Make sure to update the default value for `EnvironmentOptions::python_version` when bumping this version.
        Self::PY315
    }

    pub const fn as_tuple(self) -> (u8, u8) {
        (self.major, self.minor)
    }

    pub fn free_threaded_build_available(self) -> bool {
        self >= PythonVersion::PY313
    }

    /// Return `true` if the current version supports [PEP 701].
    ///
    /// [PEP 701]: https://peps.python.org/pep-0701/
    pub fn supports_pep_701(self) -> bool {
        self >= Self::PY312
    }

    pub fn defers_annotations(self) -> bool {
        self >= Self::PY314
    }
}

impl Default for PythonVersion {
    fn default() -> Self {
        Self::PY310

Review comment (suggestion):
We should not change `latest` or `latest_ty` before we have stable Python 3.15 support

Code after:
            PythonVersion::PY313,
            PythonVersion::PY314,
            PythonVersion::PY315,
        ]
        .into_iter()
    }

    /// The minimum supported Python version.
    pub const fn lowest() -> Self {
        Self::PY37
    }

    pub const fn latest() -> Self {
        Self::PY314
    }

    /// The latest Python version supported in preview
    pub fn latest_preview() -> Self {
        let latest_preview = Self::PY315;
        debug_assert!(latest_preview >= Self::latest());
        latest_preview
    }

    pub const fn latest_ty() -> Self {
        // Make sure to update the default value for `EnvironmentOptions::python_version` when bumping this version.
        Self::PY314
    }

    pub const fn as_tuple(self) -> (u8, u8) {
        (self.major, self.minor)
    }

    pub fn free_threaded_build_available(self) -> bool {
        self >= PythonVersion::PY313
    }

    /// Return `true` if the current version supports [PEP 701].
    ///
    /// [PEP 701]: https://peps.python.org/pep-0701/
    pub fn supports_pep_701(self) -> bool {
        self >= Self::PY312
    }

    pub fn defers_annotations(self) -> bool {
        self >= Self::PY314
    }
}

impl Default for PythonVersion {
    fn default() -> Self {
        Self::PY310
Repository: astral-sh/ruff
File: crates/ty_ide/src/completion.rs (Rust)

Code before:
        let builder = completion_test_builder(
            r#"
from typing import Any

def f(x: Any | str):
    x.remove<CURSOR>
"#,
        );
        assert_snapshot!(
            builder.build().snapshot(),
            @r"
        removeprefix
        removesuffix
        ",
        );
    }

    /// Tests that when we have `(U & Any) | T` that we offer
    /// completions for `T`.
    #[test]
    fn dynamic_type_with_intersection_type_annotation() {
        let builder = completion_test_builder(
            r#"
from typing import Any

def f(x: (int & Any) | str):
    x.remove<CURSOR>
"#,
        );
        assert_snapshot!(
            builder.build().snapshot(),
            @r"
        removeprefix
        removesuffix
        ",
        );
    }

    /// A way to create a simple single-file (named `main.py`) completion test
    /// builder.
    ///
    /// Use cases that require multiple files with a `<CURSOR>` marker
    /// in a file other than `main.py` can use `CursorTest::builder()`
    /// and then `CursorTestBuilder::completion_test_builder()`.
    fn completion_test_builder(source: &str) -> CompletionTestBuilder {
        CursorTest::builder()
            .source("main.py", source)
            .completion_test_builder()
    }

    /// A builder for executing a completion test.

Review comment (suggestion):
we display an intersection as `int & Any`, but that syntax isn't standardised yet and we don't actually support it in user annotations -- you want

```suggestion
from typing import Any
from ty_extensions import Intersection

def f(x: Intersection[int, Any] | str):
```

Code after:
        let builder = completion_test_builder(
            r#"
from typing import Any

def f(x: Any | str):
    x.remove<CURSOR>
"#,
        );
        assert_snapshot!(
            builder.build().snapshot(),
            @r"
        removeprefix
        removesuffix
        ",
        );
    }

    /// Tests that when we have `(U & Any) | T` that we offer
    /// completions for `T`.
    #[test]
    fn dynamic_type_with_intersection_type_annotation() {
        let builder = completion_test_builder(
            r#"
from typing import Any
from ty_extensions import Intersection

def f(x: Intersection[int, Any] | str):
    x.remove<CURSOR>
"#,
        );
        assert_snapshot!(
            builder.build().snapshot(),
            @r"
        removeprefix
        removesuffix
        ",
        );
    }

    /// A way to create a simple single-file (named `main.py`) completion test
    /// builder.
    ///
    /// Use cases that require multiple files with a `<CURSOR>` marker
    /// in a file other than `main.py` can use `CursorTest::builder()`
    /// and then `CursorTestBuilder::completion_test_builder()`.
    fn completion_test_builder(source: &str) -> CompletionTestBuilder {
        CursorTest::builder()
            .source("main.py", source)
            .completion_test_builder()
    }
Repository: astral-sh/ruff
File: crates/ruff_linter/src/rules/flake8_executable/rules/shebang_missing_python.rs (Rust)

Code before:
/// ```python
/// #!/usr/bin/env python3
/// ```
///
/// ## References
/// - [Python documentation: Executable Python Scripts](https://docs.python.org/3/tutorial/appendix.html#executable-python-scripts)
#[derive(ViolationMetadata)]
#[violation_metadata(stable_since = "v0.0.229")]
pub(crate) struct ShebangMissingPython;

impl Violation for ShebangMissingPython {
    #[derive_message_formats]
    fn message(&self) -> String {
        "Shebang should contain `python`, `pytest`, or `uv run`".to_string()
    }
}

/// EXE003
pub(crate) fn shebang_missing_python(
    range: TextRange,
    shebang: &ShebangDirective,
    context: &LintContext,
) {
    if shebang.contains("python")
        || shebang.contains("pytest")
        || (shebang.contains("uv") && shebang.contains("run"))
        || shebang.contains("uvx")
        || shebang.contains("uv tool run")
    {
        return;
    }

    context.report_diagnostic_if_enabled(ShebangMissingPython, range);
}


Review comment (suggestion):
I'd prefer a stricter regex as mentioned in the original issue. We should also make the same change for `uvx` and `uv tool run`.

Code after:
/// ## What it does
/// Checks for a shebang directive in `.py` files that does not contain `python`,
/// `pytest`, or `uv run`.
///
/// ## Why is this bad?
/// In Python, a shebang (also known as a hashbang) is the first line of a
/// script, which specifies the command that should be used to run the
/// script.
///
/// For Python scripts, if the shebang does not include a command that explicitly
/// or implicitly specifies an interpreter, then the file will be executed with
/// the default interpreter, which is likely a mistake.
///
/// ## Example
/// ```python
/// #!/usr/bin/env bash
/// ```
///
/// Use instead:
/// ```python
/// #!/usr/bin/env python3
/// ```
///
/// ## References
/// - [Python documentation: Executable Python Scripts](https://docs.python.org/3/tutorial/appendix.html#executable-python-scripts)
#[derive(ViolationMetadata)]
#[violation_metadata(stable_since = "v0.0.229")]
pub(crate) struct ShebangMissingPython;

impl Violation for ShebangMissingPython {
    #[derive_message_formats]
    fn message(&self) -> String {
        "Shebang should contain `python`, `pytest`, or `uv run`".to_string()
    }
}

/// EXE003
pub(crate) fn shebang_missing_python(
    range: TextRange,
    shebang: &ShebangDirective,
    context: &LintContext,
) {
    if shebang.contains("python") || shebang.contains("pytest") || UV_RUN_REGEX.is_match(shebang) {
        return;
    }

    context.report_diagnostic_if_enabled(ShebangMissingPython, range);
}
Repository: astral-sh/ruff
File: crates/ruff_linter/src/rules/flake8_executable/rules/shebang_missing_python.rs (Rust)

Code before:
use std::sync::LazyLock;

use regex::Regex;
use ruff_macros::{ViolationMetadata, derive_message_formats};
use ruff_text_size::TextRange;

use crate::Violation;
use crate::checkers::ast::LintContext;
use crate::comments::shebang::ShebangDirective;

static UV_RUN_REGEX: LazyLock<Regex> =
    LazyLock::new(|| Regex::new(r"(?x) \b(?:uv|uvx|uv\s+tool)\b .* \s run \b").unwrap());

/// ## What it does
/// Checks for a shebang directive in `.py` files that does not contain `python`,
/// `pytest`, or `uv run`.
///
/// ## Why is this bad?
/// In Python, a shebang (also known as a hashbang) is the first line of a
/// script, which specifies the command that should be used to run the
/// script.
///
/// For Python scripts, if the shebang does not include a command that explicitly
/// or implicitly specifies an interpreter, then the file will be executed with
/// the default interpreter, which is likely a mistake.
///
/// ## Example
/// ```python
/// #!/usr/bin/env bash
/// ```
///
/// Use instead:
/// ```python
/// #!/usr/bin/env python3
/// ```
///
/// ## References

Review comment (suggestion):
`uvx` doesn't require `run`. So I don't thinkw e need to change anything for `uvx`.

For uv and `uv tool`, would a regex like this work `uv\s+(?:--?[a-zA-Z][\w-]*(?:[=\s]\S+)?\s+)*run(?:\s+.*)?`

Code after:
use std::sync::LazyLock;

use regex::Regex;
use ruff_macros::{ViolationMetadata, derive_message_formats};
use ruff_text_size::TextRange;

use crate::Violation;
use crate::checkers::ast::LintContext;
use crate::comments::shebang::ShebangDirective;

static UV_RUN_REGEX: LazyLock<Regex> = LazyLock::new(|| {
    Regex::new(
        r"(?x)
        \b
        (?:
            # Part A: uv or uv tool (these MUST be followed by run)
            (?:uv|uv\s+tool) \s+ (?:--?[a-zA-Z][\w-]*(?:[=\s]\S+)?\s+)* run
            |
            # Part B: uvx (stands alone, run is optional/redundant)
            uvx (?: \s+ .* )?
        )
        \b
    ",
    )
    .unwrap()
});

/// ## What it does
/// Checks for a shebang directive in `.py` files that does not contain `python`,
/// `pytest`, or `uv run`.
///
/// ## Why is this bad?
/// In Python, a shebang (also known as a hashbang) is the first line of a
/// script, which specifies the command that should be used to run the
/// script.
///
/// For Python scripts, if the shebang does not include a command that explicitly