ronantakizawa/codereview-bench huggingface dataset:
Repository: typst/typst
File: crates/typst-layout/src/inline/line.rs (Rust)

Code before:
    }
}

impl<'a> Deref for Items<'a> {
    type Target = Vec<IndexedItemEntry<'a>>;

    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

impl DerefMut for Items<'_> {
    fn deref_mut(&mut self) -> &mut Self::Target {
        &mut self.0
    }
}

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

/// An item accompanied by its position within a line.
#[derive(Debug)]
pub struct IndexedItemEntry<'a> {
    pub item: ItemEntry<'a>,
    pub idx: usize,
}

/// A reference to or a boxed item.
///
/// This is conceptually similar to a [`Cow<'a, Item<'a>>`][std::borrow::Cow],
/// but we box owned items since an [`Item`] is much bigger than
/// a box.
pub enum ItemEntry<'a> {
    Ref(&'a Item<'a>),
    Box(Box<Item<'a>>),
}

impl<'a> ItemEntry<'a> {
    fn text_mut(&mut self) -> Option<&mut ShapedText<'a>> {
        match self {
            Self::Ref(item) => {
                let text = item.text()?;
                *self = Self::Box(Box::new(Item::Text(text.clone())));
                match self {
                    Self::Box(item) => item.text_mut(),
                    _ => unreachable!(),
                }
            }

Review comment (suggestion):
I think this isn't quite pulling its weight. I would prefer just having `Vec<(usize, ItemEntry<'a>)>` in `struct Items`.

Code after:
impl<'a> Deref for Items<'a> {
    type Target = Vec<(usize, ItemEntry<'a>)>;

    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

impl DerefMut for Items<'_> {
    fn deref_mut(&mut self) -> &mut Self::Target {
        &mut self.0
    }
}

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

/// A reference to or a boxed item.
pub enum ItemEntry<'a> {
    Ref(&'a Item<'a>),
    Box(Box<Item<'a>>),
}

impl<'a> ItemEntry<'a> {
    fn text_mut(&mut self) -> Option<&mut ShapedText<'a>> {
        match self {
            Self::Ref(item) => {
                let text = item.text()?;
                *self = Self::Box(Box::new(Item::Text(text.clone())));
                match self {
                    Self::Box(item) => item.text_mut(),
                    _ => unreachable!(),
                }
            }
            Self::Box(item) => item.text_mut(),
        }
    }
}

impl<'a> Deref for ItemEntry<'a> {
    type Target = Item<'a>;

    fn deref(&self) -> &Self::Target {
        match self {
            Self::Ref(item) => item,
            Self::Box(item) => item,
        }
    }
Repository: typst/typst
File: crates/typst-layout/src/inline/prepare.rs (Rust)

Code before:
use typst_library::layout::{Dir, Em};
use unicode_bidi::{BidiInfo, Level as BidiLevel};

use super::*;

#[derive(Debug)]
pub struct Run<'a> {
    pub item: Item<'a>,
    pub range: Range,
}

/// A representation in which children are already layouted and text is already
/// preshaped.
///
/// In many cases, we can directly reuse these results when constructing a line.
/// Only when a line break falls onto a text index that is not safe-to-break per
/// rustybuzz, we have to reshape that portion.
pub struct Preparation<'a> {
    /// The full text.
    pub text: &'a str,
    /// Configuration for inline layout.
    pub config: &'a Config,
    /// Bidirectional text embedding levels.
    ///
    /// This is `None` if all text directions are uniform (all the base
    /// direction).
    pub bidi: Option<BidiInfo<'a>>,
    /// Text runs, spacing and layouted elements.
    pub items: Vec<Run<'a>>,
    /// Maps from byte indices to item indices.
    pub indices: Vec<usize>,

Review comment (suggestion):
The `Run` change seems quite unrelated. We can discuss whether this is worthwhile, but should do so in another PR.

Code after:
use typst_library::layout::{Dir, Em};
use unicode_bidi::{BidiInfo, Level as BidiLevel};

use super::*;

/// A representation in which children are already layouted and text is already
/// preshaped.
///
/// In many cases, we can directly reuse these results when constructing a line.
/// Only when a line break falls onto a text index that is not safe-to-break per
/// rustybuzz, we have to reshape that portion.
pub struct Preparation<'a> {
    /// The full text.
    pub text: &'a str,
    /// Configuration for inline layout.
    pub config: &'a Config,
    /// Bidirectional text embedding levels.
    ///
    /// This is `None` if all text directions are uniform (all the base
    /// direction).
    pub bidi: Option<BidiInfo<'a>>,
    /// Text runs, spacing and layouted elements.
    pub items: Vec<(Range, Item<'a>)>,
    /// Maps from byte indices to item indices.
    pub indices: Vec<usize>,
    /// The span mapper.
    pub spans: SpanMapper,
}

impl<'a> Preparation<'a> {
    /// Get the item that contains the given `text_offset`.
Repository: typst/typst
File: crates/typst-kit/src/package.rs (Rust)

Code before:
        }
    }
}

/// Minimal information required about a package to determine its latest
/// version.
#[derive(Deserialize)]
struct MinimalPackageInfo {
    name: String,
    version: PackageVersion,
}

/// A temporary directory that is a automatically cleaned up.
struct Tempdir(PathBuf);

impl Tempdir {
    /// Creates a directory at the path and auto-cleans it.
    fn create(path: PathBuf) -> io::Result<Self> {
        std::fs::create_dir_all(&path)?;
        Ok(Self(path))
    }
}

impl Drop for Tempdir {
    fn drop(&mut self) {
        fs::remove_dir_all(&self.0).ok();
    }
}

impl AsRef<Path> for Tempdir {
    fn as_ref(&self) -> &Path {
        self
    }
}

impl Deref for Tempdir {
    type Target = Path;

    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

/// Enriches an I/O error with a message and turns it into a
/// `PackageError::Other`.
#[cold]
fn error(message: &str, err: io::Error) -> PackageError {
    PackageError::Other(Some(eco_format!("{message} ({err})")))
}

#[cfg(test)]

Review comment (suggestion):
```suggestion
        _ = fs::remove_dir_all(&self.0);
```

I think this is more idiomatic, it makes it clear that this is an explicitly ignored return value and is easier to grep.

Code after:

/// Minimal information required about a package to determine its latest
/// version.
#[derive(Deserialize)]
struct MinimalPackageInfo {
    name: String,
    version: PackageVersion,
}

/// A temporary directory that is a automatically cleaned up.
struct Tempdir(PathBuf);

impl Tempdir {
    /// Creates a directory at the path and auto-cleans it.
    fn create(path: PathBuf) -> io::Result<Self> {
        std::fs::create_dir_all(&path)?;
        Ok(Self(path))
    }
}

impl Drop for Tempdir {
    fn drop(&mut self) {
        _ = fs::remove_dir_all(&self.0);
    }
}

impl AsRef<Path> for Tempdir {
    fn as_ref(&self) -> &Path {
        &self.0
    }
}

/// Enriches an I/O error with a message and turns it into a
/// `PackageError::Other`.
#[cold]
fn error(message: &str, err: io::Error) -> PackageError {
    PackageError::Other(Some(eco_format!("{message}: {err}")))
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn lazy_deser_index() {
        let storage = PackageStorage::with_index(
            None,
            None,
            Downloader::new("typst/test"),
            OnceCell::with_value(vec![
                serde_json::json!({
Repository: typst/typst
File: crates/typst-library/src/foundations/ty.rs (Rust)

Code before:
/// elements like headings and shapes, and style information.
///
/// # Example
/// ```example
/// #let x = 10
/// #if type(x) == int [
///   #x is an integer!
/// ] else [
///   #x is another value...
/// ]
///
/// An image is of type
/// #type(image("glacier.jpg")).
/// ```
///
/// The type of `10` is `int`. Now, what is the type of `int` or even `type`?
/// ```example
/// #type(int) \
/// #type(type)
/// ```
///
/// [none]($none) and [auto]($auto) are more special. Because they are the only
/// values of their type, their type isn't actually bound to any name.
/// ```example
/// type(none) == none: #{ type(none) == none } \
/// type(auto) == auto: #{ type(auto) == auto }
/// ```
/// Instead, to test for these _values_, compare to them directly:
/// ```example
/// #let empty = none
/// empty is none: #{ empty == none }
/// ```
///
/// Note that `type` is used for "high level" comparisons, such as whether a variable is `content`
/// vs `int`. See [func]($content.func) to programmatically determine which _element_ a variable
/// is.
#[ty(scope, cast)]
#[derive(Copy, Clone, Eq, PartialEq, Hash)]
pub struct Type(Static<NativeTypeData>);

impl Type {
    /// Get the type for `T`.
    pub fn of<T: NativeType>() -> Self {
        T::ty()
    }

    /// The type's short name, how it is used in code (e.g. `str`).
    pub fn short_name(&self) -> &'static str {
        self.0.name
    }


Review comment (suggestion):
This example might cause more confusion that clarity because it shows precisely the comparison one _shouldn't perform._

Code after:
/// elements like headings and shapes, and style information.
///
/// # Example
/// ```example
/// #let x = 10
/// #if type(x) == int [
///   #x is an integer!
/// ] else [
///   #x is another value...
/// ]
///
/// An image is of type
/// #type(image("glacier.jpg")).
/// ```
///
/// The type of `{10}` is `int`. Now, what is the type of `int` or even `type`?
/// ```example
/// #type(int) \
/// #type(type)
/// ```
///
/// Unlike other types like `int`, [none] and [auto] do not have a name
/// representing them. To test if a value is one of these, compare your value to
/// them directly, e.g:
/// ```example
/// #let val = none
/// #if val == none [
///   Yep, it's none.
/// ]
/// ```
///
/// Note that `type` will return [`content`] for all document elements. To
/// programmatically determine which kind of content you are dealing with, see
/// [`content.func`].
#[ty(scope, cast)]
#[derive(Copy, Clone, Eq, PartialEq, Hash)]
pub struct Type(Static<NativeTypeData>);

impl Type {
    /// Get the type for `T`.
    pub fn of<T: NativeType>() -> Self {
        T::ty()
    }

    /// The type's short name, how it is used in code (e.g. `str`).
    pub fn short_name(&self) -> &'static str {
        self.0.name
    }

    /// The type's long name, for use in diagnostics (e.g. `string`).
    pub fn long_name(&self) -> &'static str {
Repository: typst/typst
File: crates/typst-library/src/foundations/ty.rs (Rust)

Code before:
///   #x is another value...
/// ]
///
/// An image is of type
/// #type(image("glacier.jpg")).
/// ```
///
/// The type of `10` is `int`. Now, what is the type of `int` or even `type`?
/// ```example
/// #type(int) \
/// #type(type)
/// ```
///
/// [none]($none) and [auto]($auto) are more special. Because they are the only
/// values of their type, their type isn't actually bound to any name.
/// ```example
/// type(none) == none: #{ type(none) == none } \
/// type(auto) == auto: #{ type(auto) == auto }
/// ```
/// Instead, to test for these _values_, compare to them directly:
/// ```example
/// #let empty = none
/// empty is none: #{ empty == none }
/// ```
///
/// Note that `type` is used for "high level" comparisons, such as whether a variable is `content`
/// vs `int`. See [func]($content.func) to programmatically determine which _element_ a variable
/// is.
#[ty(scope, cast)]
#[derive(Copy, Clone, Eq, PartialEq, Hash)]
pub struct Type(Static<NativeTypeData>);

impl Type {
    /// Get the type for `T`.
    pub fn of<T: NativeType>() -> Self {
        T::ty()
    }

    /// The type's short name, how it is used in code (e.g. `str`).
    pub fn short_name(&self) -> &'static str {
        self.0.name
    }

    /// The type's long name, for use in diagnostics (e.g. `string`).
    pub fn long_name(&self) -> &'static str {
        self.0.long_name
    }

    /// The type's title case name, for use in documentation (e.g. `String`).
    pub fn title(&self) -> &'static str {
        self.0.title

Review comment (suggestion):
The need for this paragraph shows once again that elements are really just types and I'll be very happy once we've realized that in the compiler.

Code after:
///   #x is another value...
/// ]
///
/// An image is of type
/// #type(image("glacier.jpg")).
/// ```
///
/// The type of `{10}` is `int`. Now, what is the type of `int` or even `type`?
/// ```example
/// #type(int) \
/// #type(type)
/// ```
///
/// Unlike other types like `int`, [none] and [auto] do not have a name
/// representing them. To test if a value is one of these, compare your value to
/// them directly, e.g:
/// ```example
/// #let val = none
/// #if val == none [
///   Yep, it's none.
/// ]
/// ```
///
/// Note that `type` will return [`content`] for all document elements. To
/// programmatically determine which kind of content you are dealing with, see
/// [`content.func`].
#[ty(scope, cast)]
#[derive(Copy, Clone, Eq, PartialEq, Hash)]
pub struct Type(Static<NativeTypeData>);

impl Type {
    /// Get the type for `T`.
    pub fn of<T: NativeType>() -> Self {
        T::ty()
    }

    /// The type's short name, how it is used in code (e.g. `str`).
    pub fn short_name(&self) -> &'static str {
        self.0.name
    }

    /// The type's long name, for use in diagnostics (e.g. `string`).
    pub fn long_name(&self) -> &'static str {
        self.0.long_name
    }

    /// The type's title case name, for use in documentation (e.g. `String`).
    pub fn title(&self) -> &'static str {
        self.0.title
    }
Repository: typst/typst
File: crates/typst-library/src/text/mod.rs (Rust)

Code before:
            Ok(
                regex_syntax::ast::Ast::ClassBracketed(..)
                | regex_syntax::ast::Ast::ClassUnicode(..)
                | regex_syntax::ast::Ast::ClassPerl(..)
                | regex_syntax::ast::Ast::Dot(..)
                | regex_syntax::ast::Ast::Literal(..),
            ) => {}
            _ => bail!(
                "coverage regex may only use dot, letters, and character classes";
                hint: "the regex is applied to each letter individually"
            ),
        }
        Covers::Regex(regex)
    },
}

/// Font family fallback list.
///
/// Must contain at least one font.
#[derive(Debug, Default, Clone, PartialEq, Hash)]
pub struct FontList(pub Vec<FontFamily>);

impl FontList {
    pub fn new(fonts: Vec<FontFamily>) -> StrResult<Self> {
        if fonts.is_empty() {
            bail!("font fallback list should not be empty")
        } else {
            Ok(Self(fonts))
        }
    }
}

impl<'a> IntoIterator for &'a FontList {
    type IntoIter = std::slice::Iter<'a, FontFamily>;
    type Item = &'a FontFamily;

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

cast! {
    FontList,
    self => if self.0.len() == 1 {
        self.0.into_iter().next().unwrap().name.into_value()
    } else {
        self.0.into_value()
    },
    family: FontFamily => Self(vec![family]),
    values: Array => Self::new(values.into_iter().map(|v| v.cast()).collect::<HintedStrResult<_>>()?)?,
}

Review comment (suggestion):
if it's an error, I think "should" should be "must". the current pharsing would be more apprioriate for a warning.

Code after:
            Ok(
                regex_syntax::ast::Ast::ClassBracketed(..)
                | regex_syntax::ast::Ast::ClassUnicode(..)
                | regex_syntax::ast::Ast::ClassPerl(..)
                | regex_syntax::ast::Ast::Dot(..)
                | regex_syntax::ast::Ast::Literal(..),
            ) => {}
            _ => bail!(
                "coverage regex may only use dot, letters, and character classes";
                hint: "the regex is applied to each letter individually"
            ),
        }
        Covers::Regex(regex)
    },
}

/// Font family fallback list.
///
/// Must contain at least one font.
#[derive(Debug, Default, Clone, PartialEq, Hash)]
pub struct FontList(pub Vec<FontFamily>);

impl FontList {
    pub fn new(fonts: Vec<FontFamily>) -> StrResult<Self> {
        if fonts.is_empty() {
            bail!("font fallback list must not be empty")
        } else {
            Ok(Self(fonts))
        }
    }
}

impl<'a> IntoIterator for &'a FontList {
    type IntoIter = std::slice::Iter<'a, FontFamily>;
    type Item = &'a FontFamily;

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

cast! {
    FontList,
    self => if self.0.len() == 1 {
        self.0.into_iter().next().unwrap().name.into_value()
    } else {
        self.0.into_value()
    },
    family: FontFamily => Self(vec![family]),
    values: Array => Self::new(values.into_iter().map(|v| v.cast()).collect::<HintedStrResult<_>>()?)?,
}
Repository: typst/typst
File: crates/typst-library/src/model/par.rs (Rust)

Code before:
/// can disable line numbers for text inside certain elements by setting the
/// numbering to `{none}` using show-set rules.
///
/// ```example
/// >>> #set page(margin: (left: 3em))
/// // Styled red line numbers.
/// #set par.line(
///   numbering: n => text(red)[#n]
/// )
///
/// // Disable numbers inside figures.
/// #show figure: set par.line(
///   numbering: none
/// )
///
/// Roses are red. \
/// Violets are blue.
///
/// #figure(
///   caption: [Without line numbers.]
/// )[
///   Lorem ipsum \
///   dolor sit amet
/// ]
///
/// The text above is a sample \
/// originating from distant times.
/// ```
///
/// This element exposes further options which may be used to control other
/// aspects of line numbering, such as its [alignment]($par.line.number-align)
/// or [margin]($par.line.number-margin). In addition, you can control whether
/// the numbering is reset on each page through the
/// [`numbering-scope`]($par.line.numbering-scope) option.
#[elem(name = "line", title = "Paragraph Line", keywords = ["line numbering"], Construct, Locatable)]
pub struct ParLine {
    /// How to number each line. Accepts a
    /// [numbering pattern or function]($numbering) taking a single number.
    ///
    /// ```example
    /// >>> #set page(margin: (left: 3em))
    /// #set par.line(numbering: "I")
    ///
    /// Roses are red. \
    /// Violets are blue. \
    /// Typst is there for you.
    /// ```
    #[ghost]
    pub numbering: Option<Numbering>,

    /// The alignment of line numbers associated with each line.

Review comment (suggestion):
```suggestion
glyph.finish(&["min", "max"])?;

```

Code after:
    },
    mut dict: Dict => {
        let spacing = dict
            .take("spacing")
            .ok()
            .map(|v| Limits::cast(v, "spacing"))
            .transpose()?;
        let tracking = dict
            .take("tracking")
            .ok()
            .map(|v| Limits::cast(v, "tracking"))
            .transpose()?;
        dict.finish(&["spacing", "tracking"])?;
        Self { spacing, tracking }
    },
}

impl Fold for JustificationLimits {
    fn fold(self, outer: Self) -> Self {
        Self {
            spacing: self.spacing.fold_or(outer.spacing),
            tracking: self.tracking.fold_or(outer.tracking),
        }
    }
}

impl Default for JustificationLimits {
    fn default() -> Self {
        Self {
            spacing: Some(Limits::SPACING_DEFAULT),
            tracking: Some(Limits::TRACKING_DEFAULT),
        }
    }
}

/// Determines the minimum and maximum size by or to which spacing may be shrunk
/// and stretched.
#[derive(Debug, Copy, Clone, PartialEq, Hash)]
pub struct Limits<T> {
    /// Minimum allowable adjustment.
    pub min: T,
    /// Maximum allowable adjustment.
    pub max: T,
}

impl Limits<Rel> {
    const SPACING_DEFAULT: Self = Self {
        min: Rel::new(Ratio::new(2.0 / 3.0), Length::zero()),
        max: Rel::new(Ratio::new(1.5), Length::zero()),
    };
}
Repository: typst/typst
File: crates/typst-library/src/visualize/gradient.rs (Rust)

Code before:
            let offset = i as f64 / (stops.len() - 1) as f64;
            (stop.v.color, Ratio::new(offset))
        })
        .collect())
}

/// Sample the stops at a given position.
fn sample_stops(stops: &[(Color, Ratio)], mixing_space: ColorSpace, t: f64) -> Color {
    let t = t.clamp(0.0, 1.0);
    let mut low = 0;
    let mut high = stops.len();

    while low < high {
        let mid = (low + high) / 2;
        if stops[mid].1.get() < t {
            low = mid + 1;
        } else {
            high = mid;
        }
    }

    if low == 0 {
        low = 1;
    }
    if t == 1.0 {
        while stops[low - 1].1 >= stops[low].1 {
            low -= 1;
        }
    } else {
        while stops[low - 1].1 >= stops[low].1 {
            low += 1;
        }
    }

    let (col_0, pos_0) = stops[low - 1];
    let (col_1, pos_1) = stops[low];
    debug_assert!(pos_0 < pos_1);
    debug_assert!(pos_0.get() <= t);
    debug_assert!(t <= pos_1.get());
    let t = (t - pos_0.get()) / (pos_1.get() - pos_0.get());

    Color::mix_iter(
        [WeightedColor::new(col_0, 1.0 - t), WeightedColor::new(col_1, t)],
        mixing_space,
    )
    .unwrap()
}


Review comment (bug):
This seems like it could underflow. Overall, I'm not sure I fully understand the code. Performing an exact floating point comparison seems potentially problematic.

Unrelated to your PR, the binary search above looks like a standard binary search that could be replaced by the std version.

Code after:
            );
        }

        return Ok(out);
    }

    Ok(stops
        .iter()
        .enumerate()
        .map(|(i, stop)| {
            let offset = i as f64 / (stops.len() - 1) as f64;
            (stop.v.color, Ratio::new(offset))
        })
        .collect())
}

/// Sample the stops at a given position.
fn sample_stops(stops: &[(Color, Ratio)], mixing_space: ColorSpace, t: f64) -> Color {
    let t = t.clamp(0.0, 1.0);
    let mut j = stops.partition_point(|(_, ratio)| ratio.get() < t);

    if j == 0 {
        while stops.get(j + 1).is_some_and(|(_, r)| r.is_zero()) {
            j += 1;
        }
        return stops[j].0;
    }

    let (col_0, pos_0) = stops[j - 1];
    let (col_1, pos_1) = stops[j];
    let t = (t - pos_0.get()) / (pos_1.get() - pos_0.get());

    Color::mix_iter(
        [WeightedColor::new(col_0, 1.0 - t), WeightedColor::new(col_1, t)],
        mixing_space,
    )
    .unwrap()
}