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

Code before:
}

/// RUF008
pub(crate) fn mutable_dataclass_default(checker: &Checker, class_def: &ast::StmtClassDef) {
    let semantic = checker.semantic();

    let Some((dataclass_kind, _)) = dataclass_kind(class_def, semantic) else {
        return;
    };

    for statement in &class_def.body {
        let Stmt::AnnAssign(ast::StmtAnnAssign {
            annotation,
            value: Some(value),
            ..
        }) = statement
        else {
            continue;
        };

        let value_to_check = if let Expr::Call(ast::ExprCall {
            func, arguments, ..
        }) = value.as_ref()
        {
            if is_dataclass_field(func, checker.semantic(), dataclass_kind) {
                arguments.find_keyword("default").map(|kw| &kw.value)
            } else {
                Some(value.as_ref())
            }
        } else {
            Some(value.as_ref())
        };

        let Some(value_to_check) = value_to_check else {
            continue;
        };

        if is_mutable_expr(value_to_check, checker.semantic())
            && !is_class_var_annotation(annotation, checker.semantic())
            && !is_immutable_annotation(annotation, checker.semantic(), &[])
        {
            checker.report_diagnostic(MutableDataclassDefault, value_to_check.range());
        }
    }
}


Review comment (suggestion):
I think it might make sense to check for the `default` keyword and also the first positional argument with `find_argument_value`. That seems like the main selling point of `attr.ib` compared to `attr.field`, and the other calls are keyword-only, so it seems okay to "accept" invalid calls to them.


```suggestion
                arguments.find_argument_value("default", 0)
```

That will actually handle the `map` call for you as well.

Code after:
    }
}

/// RUF008
pub(crate) fn mutable_dataclass_default(checker: &Checker, class_def: &ast::StmtClassDef) {
    let semantic = checker.semantic();

    let Some((dataclass_kind, _)) = dataclass_kind(class_def, semantic) else {
        return;
    };

    for statement in &class_def.body {
        let Stmt::AnnAssign(ast::StmtAnnAssign {
            annotation,
            value: Some(value),
            ..
        }) = statement
        else {
            continue;
        };

        let value = match &**value {
            Expr::Call(ast::ExprCall {
                func, arguments, ..
            }) if is_mutable_default_in_dataclass_field_enabled(checker.settings())
                && is_dataclass_field(func, checker.semantic(), dataclass_kind) =>
            {
                arguments.find_argument_value("default", 0)
            }
            value => Some(value),
        };

        let Some(value) = value else {
            continue;
        };

        if is_mutable_expr(value, checker.semantic())
            && !is_class_var_annotation(annotation, checker.semantic())
            && !is_immutable_annotation(annotation, checker.semantic(), &[])
        {
            checker.report_diagnostic(MutableDataclassDefault, value.range());
        }
    }
}
Repository: astral-sh/ruff
File: crates/ruff_linter/src/rules/ruff/rules/mutable_dataclass_default.rs (Rust)

Code before:
    let Some((dataclass_kind, _)) = dataclass_kind(class_def, semantic) else {
        return;
    };

    for statement in &class_def.body {
        let Stmt::AnnAssign(ast::StmtAnnAssign {
            annotation,
            value: Some(value),
            ..
        }) = statement
        else {
            continue;
        };

        let value_to_check = if let Expr::Call(ast::ExprCall {
            func, arguments, ..
        }) = value.as_ref()
        {
            if is_dataclass_field(func, checker.semantic(), dataclass_kind) {
                arguments.find_keyword("default").map(|kw| &kw.value)
            } else {
                Some(value.as_ref())
            }
        } else {
            Some(value.as_ref())
        };

        let Some(value_to_check) = value_to_check else {
            continue;
        };

        if is_mutable_expr(value_to_check, checker.semantic())
            && !is_class_var_annotation(annotation, checker.semantic())
            && !is_immutable_annotation(annotation, checker.semantic(), &[])
        {
            checker.report_diagnostic(MutableDataclassDefault, value_to_check.range());
        }
    }
}


Review comment (suggestion):
I think this might be slightly nicer as a `match` to avoid these dangling elses:

```rust
        let value_to_check = match &**value {
            Expr::Call(ast::ExprCall {
                func, arguments, ..
            }) if is_dataclass_field(func, checker.semantic(), dataclass_kind) => {
                arguments.find_argument_value("default", 0)
            }
            value => Some(value),
        };
```

I would probably just shadow `value` instead of using `value_to_check` too.

Code after:

    let Some((dataclass_kind, _)) = dataclass_kind(class_def, semantic) else {
        return;
    };

    for statement in &class_def.body {
        let Stmt::AnnAssign(ast::StmtAnnAssign {
            annotation,
            value: Some(value),
            ..
        }) = statement
        else {
            continue;
        };

        let value = match &**value {
            Expr::Call(ast::ExprCall {
                func, arguments, ..
            }) if is_mutable_default_in_dataclass_field_enabled(checker.settings())
                && is_dataclass_field(func, checker.semantic(), dataclass_kind) =>
            {
                arguments.find_argument_value("default", 0)
            }
            value => Some(value),
        };

        let Some(value) = value else {
            continue;
        };

        if is_mutable_expr(value, checker.semantic())
            && !is_class_var_annotation(annotation, checker.semantic())
            && !is_immutable_annotation(annotation, checker.semantic(), &[])
        {
            checker.report_diagnostic(MutableDataclassDefault, value.range());
        }
    }
}
Repository: astral-sh/ruff
File: crates/ruff_linter/resources/test/fixtures/ruff/RUF008_attrs.py (Python)

Code before:
import typing
from typing import ClassVar, Sequence

import attr
import attrs
from attr import s
from attrs import define, frozen

KNOWINGLY_MUTABLE_DEFAULT = []


@define
class A:
    mutable_default: list[int] = []
    immutable_annotation: typing.Sequence[int] = []
    without_annotation = []
    correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT
    perfectly_fine: list[int] = field(default_factory=list)
    class_variable: typing.ClassVar[list[int]] = []


@frozen
class B:
    mutable_default: list[int] = []
    immutable_annotation: Sequence[int] = []
    without_annotation = []
    correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT
    perfectly_fine: list[int] = field(default_factory=list)
    class_variable: ClassVar[list[int]] = []


Review comment (suggestion):
I wouldn't recommend this for real code, but for the sake of the test, let's import it like this:

```suggestion
import attr, attrs
```

That avoids shifting all of the snapshot lines down one and makes the diff easier to review.

Code after:
import typing
from typing import ClassVar, Sequence

import attr, attrs
from attr import s
from attrs import define, frozen

KNOWINGLY_MUTABLE_DEFAULT = []


@define
class A:
    mutable_default: list[int] = []
    immutable_annotation: typing.Sequence[int] = []
    without_annotation = []
    correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT
    perfectly_fine: list[int] = field(default_factory=list)
    class_variable: typing.ClassVar[list[int]] = []


@frozen
class B:
    mutable_default: list[int] = []
    immutable_annotation: Sequence[int] = []
    without_annotation = []
    correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT
    perfectly_fine: list[int] = field(default_factory=list)
    class_variable: ClassVar[list[int]] = []
Repository: astral-sh/ruff
File: crates/ruff_server/src/resolve.rs (Rust)

Code before:
/// 4. If none of the above conditions are met, the document is excluded.
fn is_document_excluded(
    path: &Path,
    resolver_settings: &FileResolverSettings,
    linter_settings: Option<&LinterSettings>,
    formatter_settings: Option<&FormatterSettings>,
    language_id: Option<LanguageId>,
) -> bool {
    if let Some(exclusion) = match_any_exclusion(
        path,
        resolver_settings,
        linter_settings.map(|s| &*s.exclude),
        formatter_settings.map(|s| &*s.exclude),
    ) {
        tracing::debug!("Ignored path via `{}`: {}", exclusion, path.display());
        return true;
    }

    if let Some(inclusion) = match_any_inclusion(path, resolver_settings) {
        tracing::debug!("Included path via `{}`: {}", inclusion, path.display());
        false
    } else if let Some(LanguageId::Python) = language_id {
        tracing::debug!("Included path via Python language ID: {}", path.display());
        false
    } else if let Some(LanguageId::Markdown) = language_id
        && let Some(_) = formatter_settings
    {
        tracing::debug!("Included path via Markdown language ID: {}", path.display());
        false
    } else {
        tracing::debug!(
            "Ignored path as it's not in the inclusion set: {}",
            path.display()
        );
        true
    }
}


Review comment (suggestion):
nit: I'd probably do this if I weren't using the binding in the `let`:


```suggestion
        && formatter_settings.is_some()
```

Code after:
/// 4. If none of the above conditions are met, the document is excluded.
fn is_document_excluded(
    path: &Path,
    resolver_settings: &FileResolverSettings,
    linter_settings: Option<&LinterSettings>,
    formatter_settings: Option<&FormatterSettings>,
    language_id: Option<LanguageId>,
) -> bool {
    if let Some(exclusion) = match_any_exclusion(
        path,
        resolver_settings,
        linter_settings.map(|s| &*s.exclude),
        formatter_settings.map(|s| &*s.exclude),
    ) {
        tracing::debug!("Ignored path via `{}`: {}", exclusion, path.display());
        return true;
    }

    if let Some(inclusion) = match_any_inclusion(path, resolver_settings) {
        tracing::debug!("Included path via `{}`: {}", inclusion, path.display());
        false
    } else if let Some(LanguageId::Python) = language_id {
        tracing::debug!("Included path via Python language ID: {}", path.display());
        false
    } else if let Some(LanguageId::Markdown) = language_id
        && formatter_settings.is_some()
    {
        tracing::debug!("Included path via Markdown language ID: {}", path.display());
        false
    } else {
        tracing::debug!(
            "Ignored path as it's not in the inclusion set: {}",
            path.display()
        );
        true
    }
}
Repository: astral-sh/ruff
File: crates/ty_ide/src/goto_definition.rs (Rust)

Code before:
          |
        info: Found 1 definition
         --> main.py:3:9
          |
        2 | class MyClass:
        3 |     def __init__(self, val):
          |         --------
        4 |         self.val = val
        5 | x = MyClass()
          |
        ");
    }

    /// goto-definition on a class init closing parenthesis
    /// when there is an argument is somewhat ambiguous, and
    /// so doesn't find any defs.
    #[test]
    fn goto_definition_class_init_parenthesis_ambiguous_closing() {
        let test = CursorTest::builder()
            .source(
                "main.py",
                "
class MyClass:
    def __init__(self, val):
        self.val = val
x = MyClass(0<CURSOR>)
",
            )
            .build();

        assert_snapshot!(test.goto_definition(), @"No goto target found");
    }

    /// goto-definition on a class init parenthesis includes `__new__`
    #[test]
    fn goto_definition_class_init_parenthesis_includes_new() {
        let test = CursorTest::builder()
            .source(
                "main.py",
                "
class MyClass:
    def __init__(self, val):
        self.val = val
    def __new__(self, val):
        self.val = val
x = MyClass<CURSOR>()
",
            )
            .build();

        assert_snapshot!(test.goto_definition(), @r"

Review comment (suggestion):
Let's add a test case where it's some variable instead of a literal `0` and I think that should then go to the definition of that variable.

Code after:
          |
        info: Found 1 definition
         --> main.py:3:9
          |
        2 | class MyClass:
        3 |     def __init__(self, val):
          |         --------
        4 |         self.val = val
        5 | x = MyClass()
          |
        ");
    }

    /// goto-definition on a class init closing parenthesis
    /// when there is an argument is somewhat ambiguous, and
    /// so doesn't find any defs.
    #[test]
    fn goto_definition_class_init_parenthesis_ambiguous_closing() {
        let test = CursorTest::builder()
            .source(
                "main.py",
                "
class MyClass:
    def __init__(self, val):
        self.val = val
x = MyClass(0<CURSOR>)
",
            )
            .build();

        assert_snapshot!(test.goto_definition(), @"No goto target found");
    }

    /// goto-definition on a class init closing parenthesis when there
    /// is an argument with its own definition is somewhat ambiguous,
    /// and but we currently go to the definition of the argument.
    #[test]
    fn goto_definition_class_init_parenthesis_ambiguous_argument_closing() {
        let test = CursorTest::builder()
            .source(
                "main.py",
                "
foo = 1

class MyClass:
    def __init__(self, val):
        self.val = val
x = MyClass(foo<CURSOR>)
",
            )
            .build();
Repository: astral-sh/ruff
File: crates/ty_server/src/server/api/notifications/did_open.rs (Rust)

Code before:

impl NotificationHandler for DidOpenTextDocumentHandler {
    type NotificationType = DidOpenTextDocument;
}

impl SyncNotificationHandler for DidOpenTextDocumentHandler {
    fn run(
        session: &mut Session,
        client: &Client,
        params: DidOpenTextDocumentParams,
    ) -> Result<()> {
        let DidOpenTextDocumentParams {
            text_document:
                TextDocumentItem {
                    uri,
                    text,
                    version,
                    language_id,
                },
        } = params;

        let document = session.open_text_document(
            TextDocument::new(uri, text, version).with_language_id(&language_id),
        );

        publish_diagnostics_if_needed(&document, session, client);

        Ok(())
    }
}


Review comment (suggestion):
This check does seem to be causing some confusion because if this is true then the document won't be opened and the following notifications for this document would result in "Document ... is not open in the session" logs (https://github.com/astral-sh/ty/issues/2824#issuecomment-3940447785, https://github.com/astral-sh/ty/issues/2937).

Code after:
pub(crate) struct DidOpenTextDocumentHandler;

impl NotificationHandler for DidOpenTextDocumentHandler {
    type NotificationType = DidOpenTextDocument;
}

impl SyncNotificationHandler for DidOpenTextDocumentHandler {
    fn run(
        session: &mut Session,
        client: &Client,
        params: DidOpenTextDocumentParams,
    ) -> Result<()> {
        let DidOpenTextDocumentParams {
            text_document:
                TextDocumentItem {
                    uri,
                    text,
                    version,
                    language_id,
                },
        } = params;

        let text_doc = TextDocument::new(uri, text, version).with_language_id(&language_id);
        if matches!(text_doc.language_id(), Some(LanguageId::Other)) {
            return Ok(());
        }

        let document = session.open_text_document(text_doc);
        publish_diagnostics_if_needed(&document, session, client);

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

Code before:
    ");

    // We allow `-q`
    assert_cmd_snapshot!(case.command().arg("-q"), @"
    success: false
    exit_code: 1
    ----- stdout -----
    Found 1 diagnostic

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

    // And repeated `-qq`
    assert_cmd_snapshot!(case.command().arg("-qq"), @"
    success: false
    exit_code: 1
    ----- stdout -----

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

    Ok(())
}

#[test]
fn test_output_format_env() -> anyhow::Result<()> {
    let case = CliTest::with_file(
        "test.py",
        r#"
        print(x)     # [unresolved-reference]
        print(4[1])  # [not-subscriptable]
        from typing_extensions import reveal_type
        reveal_type('str'.lower())  # [revealed-type]
        "#,
    )?;
    // Test output of cli command against output of same command with env var set.
    assert_eq!(
        case.command()
            .arg("--output-format=github")
            .arg("--warn")
            .arg("unresolved-reference")
            .output()?,
        case.command()
            .env("TY_OUTPUT_FORMAT", "github")
            .arg("--warn")
            .arg("unresolved-reference")
            .output()?
    );
    Ok(())
}


Review comment (suggestion):
Thanks! I think a snapshot test would be clearer here, though, like copilot suggested. You can see the test immediately below this one for what that generally looks like

Code after:
    ");

    // We allow `-q`
    assert_cmd_snapshot!(case.command().arg("-q"), @"
    success: false
    exit_code: 1
    ----- stdout -----
    Found 1 diagnostic

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

    // And repeated `-qq`
    assert_cmd_snapshot!(case.command().arg("-qq"), @"
    success: false
    exit_code: 1
    ----- stdout -----

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

    Ok(())
}

#[test]
fn test_output_format_env() -> anyhow::Result<()> {
    let case = CliTest::with_file(
        "test.py",
        r#"
        print(x)     # [unresolved-reference]
        print(4[1])  # [not-subscriptable]
        from typing_extensions import reveal_type
        reveal_type('str'.lower())  # [revealed-type]
        "#,
    )?;
    assert_cmd_snapshot!(case.command().env("TY_OUTPUT_FORMAT", "github").arg("--warn").arg("unresolved-reference"), @"
    success: false
    exit_code: 1
    ----- stdout -----
    ::warning title=ty (unresolved-reference),file=<temp_dir>/test.py,line=2,col=7,endLine=2,endColumn=8::test.py:2:7: unresolved-reference: Name `x` used when not defined
    ::error title=ty (not-subscriptable),file=<temp_dir>/test.py,line=3,col=7,endLine=3,endColumn=11::test.py:3:7: not-subscriptable: Cannot subscript object of type `Literal[4]` with no `__getitem__` method
    ::notice title=ty (revealed-type),file=<temp_dir>/test.py,line=5,col=13,endLine=5,endColumn=26::test.py:5:13: revealed-type: Revealed type: `LiteralString`

    ----- stderr -----
    ");
    Ok(())
}

#[test]
fn test_run_in_sub_directory() -> anyhow::Result<()> {
    let case = CliTest::with_files([("test.py", "~"), ("subdir/nothing", "")])?;
Repository: astral-sh/ruff
File: crates/ty_python_semantic/src/types/display.rs (Rust)

Code before:

            DisplayOptionalGenericContext {
                generic_context: self.generic_context,
                db: self.db,
                settings: settings.clone(),
                hide_unused_self,
            }
            .fmt_detailed(&mut f)?;
        }

        // Parameters
        self.parameters
            .display_with(self.db, settings.clone())
            .fmt_detailed(&mut f)?;

        // Return type
        f.write_str(" -> ")?;
        let return_settings = settings.singleline();

        let should_parenthesize_return_type =
            should_parenthesize_callable_type(self.return_ty, self.db);
        if should_parenthesize_return_type {
            f.write_char('(')?;
        }
        self.return_ty
            .display_with(self.db, return_settings)
            .fmt_detailed(&mut f)?;
        if should_parenthesize_return_type {
            f.write_char(')')?;
        }

        if self.parameters.is_top() {
            f.write_str("]")?;
        }

        Ok(())
    }
}

impl Display for DisplaySignature<'_, '_> {
    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
        self.fmt_detailed(&mut TypeWriter::Formatter(f))
    }
}

/// Details about signature display components, including ranges for parameters and return type
#[derive(Debug, Clone)]
pub(crate) struct SignatureDisplayDetails {
    /// The full signature string
    pub label: String,
    /// Ranges for each parameter within the label

Review comment (suggestion):
```suggestion
            .display_with(self.db, settings.singleline())
```

Code after:

            DisplayOptionalGenericContext {
                generic_context: self.generic_context,
                db: self.db,
                settings: settings.clone(),
                hide_unused_self,
            }
            .fmt_detailed(&mut f)?;
        }

        // Parameters
        self.parameters
            .display_with(self.db, settings.clone())
            .fmt_detailed(&mut f)?;

        // Return type
        f.write_str(" -> ")?;

        let should_parenthesize_return_type =
            should_parenthesize_callable_type(self.return_ty, self.db);
        if should_parenthesize_return_type {
            f.write_char('(')?;
        }
        self.return_ty
            .display_with(self.db, settings.singleline())
            .fmt_detailed(&mut f)?;
        if should_parenthesize_return_type {
            f.write_char(')')?;
        }

        if self.parameters.is_top() {
            f.write_str("]")?;
        }

        Ok(())
    }
}

impl Display for DisplaySignature<'_, '_> {
    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
        self.fmt_detailed(&mut TypeWriter::Formatter(f))
    }
}

/// Details about signature display components, including ranges for parameters and return type
#[derive(Debug, Clone)]
pub(crate) struct SignatureDisplayDetails {
    /// The full signature string
    pub label: String,
    /// Ranges for each parameter within the label
    pub parameter_ranges: Vec<TextRange>,
Repository: astral-sh/ruff
File: python/ruff/__main__.py (Python)

Code before:
from __future__ import annotations

import os
import sys

from ruff import find_ruff_bin


def _run() -> None:
    ruff = os.fsdecode(find_ruff_bin())

    if sys.platform == "win32":
        import subprocess

        # Avoid emitting a traceback on interrupt
        try:
            completed_process = subprocess.run([ruff, *sys.argv[1:]])
        except KeyboardInterrupt:
            sys.exit(2)

        sys.exit(completed_process.returncode)
    else:
        os.execvp(ruff, [ruff, *sys.argv[1:]])


if __name__ == "__main__":
    _run()


Review comment (suggestion):
I think this is fine either way, but we did have a previous PR removing the `os.fsdecode` call: #18551. 

Code after:
from __future__ import annotations

import os
import sys

from ruff import find_ruff_bin


def _run() -> None:
    ruff = find_ruff_bin()

    if sys.platform == "win32":
        import subprocess

        # Avoid emitting a traceback on interrupt
        try:
            completed_process = subprocess.run([ruff, *sys.argv[1:]])
        except KeyboardInterrupt:
            sys.exit(2)

        sys.exit(completed_process.returncode)
    else:
        os.execvp(ruff, [ruff, *sys.argv[1:]])


if __name__ == "__main__":
    _run()