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

Code before:
        let mut label = InlayHintLabel::simple(name.to_string(), None);

        label.append_str("=");

        Self {
            position,
            kind: InlayHintKind::CallArgumentName,
            label,
        }
    }

    pub fn display(&self) -> InlayHintDisplay<'_> {
        InlayHintDisplay { inlay_hint: self }
    }
}

#[derive(Debug, Clone)]
pub enum InlayHintKind {
    Type,
    CallArgumentName,
}

#[derive(Debug, Clone)]
pub struct InlayHintLabel {
    pub parts: Vec<InlayHintLabelPart>,
}

impl InlayHintLabel {
    pub fn simple(s: impl Into<String>, target: Option<crate::NavigationTarget>) -> InlayHintLabel {
        InlayHintLabel {
            parts: vec![InlayHintLabelPart {
                text: s.into(),
                target,
            }],
        }
    }

    pub fn prepend_str(&mut self, s: &str) {
        match &mut *self.parts {
            [InlayHintLabelPart { text, target: None }, ..] => text.insert_str(0, s),
            _ => self.parts.insert(
                0,
                InlayHintLabelPart {
                    text: s.into(),
                    target: None,
                },
            ),
        }
    }

    pub fn append_str(&mut self, s: &str) {

Review comment (suggestion):
Are you thinking of expanding `InlayHintLabel` with additional fields in the future?

Regardless, I think the `parts` field should be private and we should expose this information via a method that returns `&[InlayHintLabelPart]` instead.

Code after:
    fn call_argument_name(position: TextSize, name: &str) -> Self {
        let label_parts = vec![InlayHintLabelPart::new(name), "=".into()];

        Self {
            position,
            kind: InlayHintKind::CallArgumentName,
            label: InlayHintLabel { parts: label_parts },
        }
    }

    pub fn display(&self) -> InlayHintDisplay<'_> {
        InlayHintDisplay { inlay_hint: self }
    }
}

#[derive(Debug, Clone)]
pub enum InlayHintKind {
    Type,
    CallArgumentName,
}

#[derive(Debug, Clone)]
pub struct InlayHintLabel {
    parts: Vec<InlayHintLabelPart>,
}

impl InlayHintLabel {
    pub fn parts(&self) -> &[InlayHintLabelPart] {
        &self.parts
    }
}

pub struct InlayHintDisplay<'a> {
    inlay_hint: &'a InlayHint,
}

impl fmt::Display for InlayHintDisplay<'_> {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> std::fmt::Result {
        for part in &self.inlay_hint.label.parts {
            write!(f, "{}", part.text)?;
        }
        Ok(())
    }
}

#[derive(Default, Debug, Clone)]
pub struct InlayHintLabelPart {
    text: String,

    target: Option<crate::NavigationTarget>,
}
Repository: astral-sh/ruff
File: crates/ty_ide/src/inlay_hints.rs (Rust)

Code before:

#[derive(Debug, Clone)]
pub enum InlayHintKind {
    Type,
    CallArgumentName,
}

#[derive(Debug, Clone)]
pub struct InlayHintLabel {
    pub parts: Vec<InlayHintLabelPart>,
}

impl InlayHintLabel {
    pub fn simple(s: impl Into<String>, target: Option<crate::NavigationTarget>) -> InlayHintLabel {
        InlayHintLabel {
            parts: vec![InlayHintLabelPart {
                text: s.into(),
                target,
            }],
        }
    }

    pub fn prepend_str(&mut self, s: &str) {
        match &mut *self.parts {
            [InlayHintLabelPart { text, target: None }, ..] => text.insert_str(0, s),
            _ => self.parts.insert(
                0,
                InlayHintLabelPart {
                    text: s.into(),
                    target: None,
                },
            ),
        }
    }

    pub fn append_str(&mut self, s: &str) {
        match &mut *self.parts {
            [.., InlayHintLabelPart { text, target: None }] => text.push_str(s),
            _ => self.parts.push(InlayHintLabelPart {
                text: s.into(),
                target: None,
            }),
        }
    }
}

impl From<String> for InlayHintLabel {
    fn from(s: String) -> Self {
        Self {
            parts: vec![InlayHintLabelPart {
                text: s,

Review comment (suggestion):
At first, I thought that the catch-all branch is for the empty vector (`[]`) but I don't think that's true, the branch that isn't covered here is an `InlayHintLabelPart` where `target` is `Some`. Can you expand on why does the string needs to be added only when navigation target is not present? Based on the method name, I would expect this to add the string regardless of whether target is present or not.

Code after:
#[derive(Debug, Clone)]
pub enum InlayHintKind {
    Type,
    CallArgumentName,
}

#[derive(Debug, Clone)]
pub struct InlayHintLabel {
    parts: Vec<InlayHintLabelPart>,
}

impl InlayHintLabel {
    pub fn parts(&self) -> &[InlayHintLabelPart] {
        &self.parts
    }
}

pub struct InlayHintDisplay<'a> {
    inlay_hint: &'a InlayHint,
}

impl fmt::Display for InlayHintDisplay<'_> {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> std::fmt::Result {
        for part in &self.inlay_hint.label.parts {
            write!(f, "{}", part.text)?;
        }
        Ok(())
    }
}

#[derive(Default, Debug, Clone)]
pub struct InlayHintLabelPart {
    text: String,

    target: Option<crate::NavigationTarget>,
}

impl InlayHintLabelPart {
    pub fn new(text: impl Into<String>) -> Self {
        Self {
            text: text.into(),
            target: None,
        }
    }

    pub fn text(&self) -> &str {
        &self.text
    }

    pub fn target(&self) -> Option<&crate::NavigationTarget> {
        self.target.as_ref()
Repository: astral-sh/ruff
File: crates/ty_ide/src/inlay_hints.rs (Rust)

Code before:

pub struct InlayHintDisplay<'a> {
    pub inlay_hint: &'a InlayHint,
}

impl<'a> InlayHintDisplay<'a> {
    pub fn new(label: &'a InlayHint) -> Self {
        Self { inlay_hint: label }
    }
}

impl fmt::Display for InlayHintDisplay<'_> {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> std::fmt::Result {
        for part in &self.inlay_hint.label.parts {
            write!(f, "{}", part.text)?;
        }
        Ok(())
    }
}

#[derive(Default, Debug, Clone)]
pub struct InlayHintLabelPart {
    pub text: String,

    pub target: Option<crate::NavigationTarget>,
}

pub fn inlay_hints(
    db: &dyn Db,
    file: File,
    range: TextRange,
    settings: &InlayHintSettings,
) -> Vec<InlayHint> {
    let mut visitor = InlayHintVisitor::new(db, file, range, settings);

    let ast = parsed_module(db, file).load(db);

    visitor.visit_body(ast.suite());

    visitor.hints
}

/// Settings to control the behavior of inlay hints.
#[derive(Clone, Debug)]
pub struct InlayHintSettings {
    /// Whether to show variable type hints.
    ///
    /// For example, this would enable / disable hints like the ones quoted below:
    /// ```python
    /// x": Literal[1]" = 1
    /// ```

Review comment (suggestion):
I would recommend to keep the field private and expose methods to provide the information.

Additionally, I think we should have a `From<&str>` / `From<String>` / `new(text: String)` implementation for `InlayHintLabelPart` and use builder pattern for the target like `with_target` because it's optional.

Code after:
        Self {
            text: s,
            target: None,
        }
    }
}

impl From<&str> for InlayHintLabelPart {
    fn from(s: &str) -> Self {
        Self {
            text: s.to_string(),
            target: None,
        }
    }
}

pub fn inlay_hints(
    db: &dyn Db,
    file: File,
    range: TextRange,
    settings: &InlayHintSettings,
) -> Vec<InlayHint> {
    let mut visitor = InlayHintVisitor::new(db, file, range, settings);

    let ast = parsed_module(db, file).load(db);

    visitor.visit_body(ast.suite());

    visitor.hints
}

/// Settings to control the behavior of inlay hints.
#[derive(Clone, Debug)]
pub struct InlayHintSettings {
    /// Whether to show variable type hints.
    ///
    /// For example, this would enable / disable hints like the ones quoted below:
    /// ```python
    /// x": Literal[1]" = 1
    /// ```
    pub variable_types: bool,

    /// Whether to show call argument names.
    ///
    /// For example, this would enable / disable hints like the ones quoted below:
    /// ```python
    /// def foo(x: int): pass
    /// foo("x="1)
    /// ```
    pub call_argument_names: bool,
    // Add any new setting that enables additional inlays to `any_enabled`.
Repository: astral-sh/ruff
File: crates/ty_python_semantic/src/site_packages.rs (Rust)

Code before:

        let parent_component = site_packages_ancestor_components.next()?;

        if site_packages_ancestor_components.next()? != UnixLibDir::Lib {
            return None;
        }

        let version = parent_component
            .strip_prefix("python")
            .or_else(|| parent_component.strip_prefix("pypy"))?
            .trim_end_matches('t');

        let version = PythonVersion::from_str(version).ok()?;
        let source = PythonVersionSource::InstallationDirectoryLayout {
            site_packages_parent_dir: Box::from(parent_component),
        };

        Some(PythonVersionWithSource { version, source })
    }

    pub fn into_vec(self) -> Vec<SystemPathBuf> {
        self.0.into_iter().collect()
    }
}

impl fmt::Debug for SitePackagesPaths {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        f.debug_list().entries(self.0.iter()).finish()
    }
}

impl<const N: usize> From<[SystemPathBuf; N]> for SitePackagesPaths {
    fn from(paths: [SystemPathBuf; N]) -> Self {
        Self(IndexSet::from(paths))
    }
}

impl IntoIterator for SitePackagesPaths {
    type Item = SystemPathBuf;
    type IntoIter = indexmap::set::IntoIter<SystemPathBuf>;

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

impl PartialEq<&[SystemPathBuf]> for SitePackagesPaths {
    fn eq(&self, other: &&[SystemPathBuf]) -> bool {
        self.0.as_slice() == *other
    }
}

Review comment (bug):
I think we should implement `Display` instead of `Debug` if we care about how this is formatted. It more clearly communicates that the formatting is user-facing.

Code after:

        let parent_component = site_packages_ancestor_components.next()?;

        if site_packages_ancestor_components.next()? != UnixLibDir::Lib {
            return None;
        }

        let version = parent_component
            .strip_prefix("python")
            .or_else(|| parent_component.strip_prefix("pypy"))?
            .trim_end_matches('t');

        let version = PythonVersion::from_str(version).ok()?;
        let source = PythonVersionSource::InstallationDirectoryLayout {
            site_packages_parent_dir: Box::from(parent_component),
        };

        Some(PythonVersionWithSource { version, source })
    }

    pub fn into_vec(self) -> Vec<SystemPathBuf> {
        self.0.into_iter().collect()
    }
}

impl fmt::Display for SitePackagesPaths {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        f.debug_list().entries(self.0.iter()).finish()
    }
}

impl<const N: usize> From<[SystemPathBuf; N]> for SitePackagesPaths {
    fn from(paths: [SystemPathBuf; N]) -> Self {
        Self(IndexSet::from(paths))
    }
}

impl IntoIterator for SitePackagesPaths {
    type Item = SystemPathBuf;
    type IntoIter = indexmap::set::IntoIter<SystemPathBuf>;

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

impl PartialEq<&[SystemPathBuf]> for SitePackagesPaths {
    fn eq(&self, other: &&[SystemPathBuf]) -> bool {
        self.0.as_slice() == *other
    }
}
Repository: astral-sh/ruff
File: crates/ruff_python_parser/src/semantic_errors.rs (Rust)

Code before:
//! [`SemanticSyntaxChecker`] for AST-based syntax errors.
//!
//! This checker is not responsible for traversing the AST itself. Instead, its
//! [`SemanticSyntaxChecker::visit_stmt`] and [`SemanticSyntaxChecker::visit_expr`] methods should
//! be called in a parent `Visitor`'s `visit_stmt` and `visit_expr` methods, respectively.
use ruff_python_ast::statement_visitor;
use ruff_python_ast::statement_visitor::StatementVisitor;
use ruff_python_ast::{
    self as ast, Expr, ExprContext, IrrefutablePatternKind, Pattern, PythonVersion, Stmt, StmtExpr,
    StmtFunctionDef, StmtImportFrom,
    comparable::ComparableExpr,
    helpers,
    visitor::{Visitor, walk_expr},
};
use ruff_text_size::{Ranged, TextRange, TextSize};
use rustc_hash::{FxBuildHasher, FxHashSet};
use std::fmt::Display;
#[derive(Debug, Default)]
pub struct SemanticSyntaxChecker {
    /// The checker has traversed past the `__future__` import boundary.
    ///
    /// For example, the checker could be visiting `x` in:
    ///
    /// ```python
    /// from __future__ import annotations
    ///
    /// import os
    ///
    /// x: int = 1
    /// ```
    ///
    /// Python considers it a syntax error to import from `__future__` after any other

Review comment (suggestion):
nit:

```suggestion
use ruff_python_ast::statement_visitor::{self, StatementVisitor};
```

Code after:
//! [`SemanticSyntaxChecker`] for AST-based syntax errors.
//!
//! This checker is not responsible for traversing the AST itself. Instead, its
//! [`SemanticSyntaxChecker::visit_stmt`] and [`SemanticSyntaxChecker::visit_expr`] methods should
//! be called in a parent `Visitor`'s `visit_stmt` and `visit_expr` methods, respectively.

use ruff_python_ast::{
    self as ast, Expr, ExprContext, IrrefutablePatternKind, Pattern, PythonVersion, Stmt, StmtExpr,
    StmtFunctionDef, StmtImportFrom,
    comparable::ComparableExpr,
    helpers,
    visitor::{Visitor, walk_expr, walk_stmt},
};
use ruff_text_size::{Ranged, TextRange, TextSize};
use rustc_hash::{FxBuildHasher, FxHashSet};
use std::fmt::Display;

#[derive(Debug, Default)]
pub struct SemanticSyntaxChecker {
    /// The checker has traversed past the `__future__` import boundary.
    ///
    /// For example, the checker could be visiting `x` in:
    ///
    /// ```python
    /// from __future__ import annotations
    ///
    /// import os
    ///
    /// x: int = 1
    /// ```
    ///
    /// Python considers it a syntax error to import from `__future__` after any other
Repository: astral-sh/ruff
File: crates/ruff_linter/resources/test/fixtures/pylint/dict_iter_missing_items.py (Python)

Code before:
for k, v in d_tuple_incorrect_tuple:
    pass


# Non errors
for k, v in d.items():
    pass
for k in d.keys():
    pass
for i, v in enumerate(l):
    pass
for i, v in s1.intersection(s2):
    pass
for a, b in d_tuple: 
    pass
for a, b in d_tuple_annotated: 
    pass

# Empty dict cases
empty_dict = {}
empty_dict["x"] = 1
for k, v in empty_dict:
    pass

empty_dict_annotated_tuple_keys: dict[tuple[int, str], bool] = {}
empty_dict_annotated_tuple_keys[("x", "y")] = True
for k, v in empty_dict_annotated_tuple_keys:
    pass

empty_dict_annotated_str_keys: dict[str, int] = {}
empty_dict_annotated_str_keys["x"] = 1
for k, v in empty_dict_annotated_str_keys:
    pass


Review comment (bug):
I think it would be interesting to have these two lines in separate test cases, i.e.

```py
empty_dict_annotated_tuple_keys: dict[tuple[int, str], bool] = {}
for k, v in empty_dict_annotated_tuple_keys:
    pass

empty_dict_annotated_tuple_keys = {}
empty_dict_annotated_tuple_keys[("x", "y")] = True
for k, v in empty_dict_annotated_tuple_keys:
    pass
```

I don't think we have to skip the second case even if it's technically incorrect because it's likely to be rare, as mentioned on the issue. But it seems useful to capture our behavior in this case.

Code after:
for k, v in d_tuple_incorrect_tuple:
    pass


# Non errors
for k, v in d.items():
    pass
for k in d.keys():
    pass
for i, v in enumerate(l):
    pass
for i, v in s1.intersection(s2):
    pass
for a, b in d_tuple: 
    pass
for a, b in d_tuple_annotated: 
    pass

# Empty dict cases
empty_dict = {}
empty_dict["x"] = 1
for k, v in empty_dict:
    pass

empty_dict_annotated_tuple_keys: dict[tuple[int, str], bool] = {}
for k, v in empty_dict_annotated_tuple_keys:
    pass

empty_dict_unannotated = {}
empty_dict_unannotated[("x", "y")] = True
for k, v in empty_dict_unannotated:
    pass

empty_dict_annotated_str_keys: dict[str, int] = {}
empty_dict_annotated_str_keys["x"] = 1
for k, v in empty_dict_annotated_str_keys:
    pass
Repository: astral-sh/ruff
File: crates/ty_server/src/document/range.rs (Rust)

Code before:
        })
    }
}

/// A position in an LSP text document (cell or a regular document).
#[derive(Clone, Debug, Default)]
pub(crate) struct LspPosition {
    position: lsp_types::Position,

    /// The URI of this range's text document
    uri: Option<lsp_types::Url>,
}

impl LspPosition {
    /// Returns the position within this document.
    ///
    /// Only use [`Self::local_position`] when you already have a URI context and this position is guaranteed
    /// to be within the same document/cell
    ///
    /// Do NOT use this for standalone positions - use [`Self::to_location`] instead to ensure
    /// the URI and position are consistent.
    pub(crate) fn local_position(&self) -> lsp_types::Position {
        self.position
    }

    /// Returns the uri of the text document this position belongs.
    #[expect(unused)]
    pub(crate) fn uri(&self) -> Option<&lsp_types::Url> {
        self.uri.as_ref()
    }
}

pub(crate) trait RangeExt {
    /// Convert an LSP Range to internal [`TextRange`].
    ///
    /// Returns `None` if `file` is a notebook and the
    /// cell identified by `url` can't be looked up or if the notebook
    /// isn't open in the editor.
    fn to_text_range(
        &self,
        db: &dyn Db,
        file: File,
        url: &lsp_types::Url,
        encoding: PositionEncoding,
    ) -> Option<TextRange>;
}

impl RangeExt for lsp_types::Range {
    fn to_text_range(
        &self,
        db: &dyn Db,

Review comment (suggestion):
```suggestion
    /// Returns the uri of the text document this position belongs to.
```

Code after:
        })
    }
}

/// A position in an LSP text document (cell or a regular document).
#[derive(Clone, Debug, Default)]
pub(crate) struct LspPosition {
    position: lsp_types::Position,

    /// The URI of this range's text document
    uri: Option<lsp_types::Url>,
}

impl LspPosition {
    /// Returns the position within this document.
    ///
    /// Only use [`Self::local_position`] when you already have a URI context and this position is guaranteed
    /// to be within the same document/cell
    ///
    /// Do NOT use this for standalone positions - use [`Self::to_location`] instead to ensure
    /// the URI and position are consistent.
    pub(crate) fn local_position(&self) -> lsp_types::Position {
        self.position
    }

    /// Returns the uri of the text document this position belongs to.
    #[expect(unused)]
    pub(crate) fn uri(&self) -> Option<&lsp_types::Url> {
        self.uri.as_ref()
    }
}

pub(crate) trait RangeExt {
    /// Convert an LSP Range to internal [`TextRange`].
    ///
    /// Returns `None` if `file` is a notebook and the
    /// cell identified by `url` can't be looked up or if the notebook
    /// isn't open in the editor.
    fn to_text_range(
        &self,
        db: &dyn Db,
        file: File,
        url: &lsp_types::Url,
        encoding: PositionEncoding,
    ) -> Option<TextRange>;
}

impl RangeExt for lsp_types::Range {
    fn to_text_range(
        &self,
        db: &dyn Db,
Repository: astral-sh/ruff
File: crates/ruff_benchmark/benches/ty_walltime.rs (Rust)

Code before:
                ..EnvironmentOptions::default()
            }),
            ..Options::default()
        });

        let mut db = ProjectDatabase::new(metadata, system).unwrap();

        db.project().set_included_paths(
            &mut db,
            installed_project
                .check_paths()
                .iter()
                .map(|path| SystemPath::absolute(path, &root))
                .collect(),
        );
        db
    }
}

impl Display for Benchmark<'_> {
    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
        self.project.name.fmt(f)
    }
}

fn check_project(db: &ProjectDatabase, max_diagnostics: usize) {
    let result = db.check();
    let diagnostics = result.len();

    assert!(
        diagnostics > 1 && diagnostics <= max_diagnostics,
        "Expected between {} and {} diagnostics but got {}",
        1,
        max_diagnostics,
        diagnostics
    );
}

static ALTAIR: Benchmark = Benchmark::new(
    RealWorldProject {
        name: "altair",
        repository: "https://github.com/vega/altair",
        commit: "d1f4a1ef89006e5f6752ef1f6df4b7a509336fba",
        paths: &["altair"],
        dependencies: &[
            "jinja2",
            "narwhals",
            "numpy",
            "packaging",
            "pandas-stubs",
            "pyarrow-stubs",

Review comment (suggestion):
Was thinking about using `db.project().name(db)` instead of passing in the `project_name`...

Code after:
                ..EnvironmentOptions::default()
            }),
            ..Options::default()
        });

        let mut db = ProjectDatabase::new(metadata, system).unwrap();

        db.project().set_included_paths(
            &mut db,
            installed_project
                .check_paths()
                .iter()
                .map(|path| SystemPath::absolute(path, &root))
                .collect(),
        );
        db
    }
}

impl Display for Benchmark<'_> {
    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
        self.project.name.fmt(f)
    }
}

fn check_project(db: &ProjectDatabase, project_name: &str, max_diagnostics: usize) {
    let result = db.check();
    let diagnostics = result.len();

    assert!(
        diagnostics > 1 && diagnostics <= max_diagnostics,
        "Expected between 1 and {max_diagnostics} diagnostics on project '{project_name}' but got {diagnostics}",
    );
}

static ALTAIR: Benchmark = Benchmark::new(
    RealWorldProject {
        name: "altair",
        repository: "https://github.com/vega/altair",
        commit: "d1f4a1ef89006e5f6752ef1f6df4b7a509336fba",
        paths: &["altair"],
        dependencies: &[
            "jinja2",
            "narwhals",
            "numpy",
            "packaging",
            "pandas-stubs",
            "pyarrow-stubs",
            "pytest",
            "scipy-stubs",
            "types-jsonschema",