ronantakizawa/codereview-bench huggingface dataset:
Repository: typst/typst
File: crates/typst/src/foundations/ty.rs (Rust)

Code before:

    /// Search keywords for the type.
    pub fn keywords(&self) -> &'static [&'static str] {
        self.0.keywords
    }

    /// This type's constructor function.
    pub fn constructor(&self) -> StrResult<Func> {
        self.0
            .constructor
            .as_ref()
            .map(|lazy| Func::from(*lazy))
            .ok_or_else(|| {
                eco_format!("type `{}` does not have a constructor", self.short_name())
            })
    }

    /// The type's associated scope that holds sub-definitions.
    pub fn scope(&self) -> &'static Scope {
        &(self.0).0.scope
    }

    /// Get a field from this type's scope, if possible.
    pub fn field(&self, field: &str) -> StrResult<&'static Value> {
        self.scope().get(field).ok_or_else(|| {
            eco_format!("type `{}` does not contain field `{}`", self.short_name(), field)
        })
    }
}

// Type compatibility.
impl Type {
    /// The type's backward-compatible name.
    pub fn compat_name(&self) -> &str {
        self.long_name()
    }
}

#[scope]
impl Type {
    /// Determines a value's type.
    ///
    /// ```example
    /// #type(12) \
    /// #type(14.7) \
    /// #type("hello") \
    /// #type(<glacier>) \
    /// #type([Hi]) \
    /// #type(x => x + 1) \
    /// #type(type)
    /// ```

Review comment (suggestion):
```suggestion
            eco_format!("type {self} does not contain field `{field}`")
```

Code after:

    /// Search keywords for the type.
    pub fn keywords(&self) -> &'static [&'static str] {
        self.0.keywords
    }

    /// This type's constructor function.
    pub fn constructor(&self) -> StrResult<Func> {
        self.0
            .constructor
            .as_ref()
            .map(|lazy| Func::from(*lazy))
            .ok_or_else(|| eco_format!("type {self} does not have a constructor"))
    }

    /// The type's associated scope that holds sub-definitions.
    pub fn scope(&self) -> &'static Scope {
        &(self.0).0.scope
    }

    /// Get a field from this type's scope, if possible.
    pub fn field(&self, field: &str) -> StrResult<&'static Value> {
        self.scope()
            .get(field)
            .ok_or_else(|| eco_format!("type {self} does not contain field `{field}`"))
    }
}

// Type compatibility.
impl Type {
    /// The type's backward-compatible name.
    pub fn compat_name(&self) -> &str {
        self.long_name()
    }
}

#[scope]
impl Type {
    /// Determines a value's type.
    ///
    /// ```example
    /// #type(12) \
    /// #type(14.7) \
    /// #type("hello") \
    /// #type(<glacier>) \
    /// #type([Hi]) \
    /// #type(x => x + 1) \
    /// #type(type)
    /// ```
    #[func(constructor)]
    pub fn construct(
Repository: typst/typst
File: crates/typst/src/text/font/exceptions.rs (Rust)

Code before:
use serde::Deserialize;

use super::{FontStretch, FontStyle, FontWeight};

#[derive(Debug, Default, Deserialize)]
pub struct Exception {
    pub family: Option<&'static str>,
    pub style: Option<FontStyle>,
    pub weight: Option<FontWeight>,
    pub stretch: Option<FontStretch>,
}

pub fn find_exception(postscript_name: &str) -> Option<&'static Exception> {
    EXCEPTION_MAP.get(postscript_name)
}

/// A map which keys are PostScript name and values are override entries.
static EXCEPTION_MAP: phf::Map<&'static str, Exception> = phf::phf_map! {
    "NewCM08-Book" => Exception {
        family: Some("New Computer Modern 08"),
        weight: Some(FontWeight(450)),
        style: None,
        stretch: None,
    },
    "NewCM08-BookItalic" => Exception {
        family: Some("New Computer Modern 08"),
        weight: Some(FontWeight(450)),
        style: None,
        stretch: None,
    },
    "NewCM08-Italic" => Exception {
        family: Some("New Computer Modern 08"),
        weight: None,
        style: None,
        stretch: None,
    },
    "NewCM08-Regular" => Exception {
        family: Some("New Computer Modern 08"),
        weight: None,
        style: None,
        stretch: None,
    },
    "NewCM10-Bold" => Exception {
        family: Some("New Computer Modern"),
        weight: None,
        style: None,
        stretch: None,
    },

Review comment (suggestion):
What do you think about using something like

```suggestion
    "NewCM08-Book" => Exception::new()
        .family("New Computer Modern 08")
        .weight(FontWeight(450))
```


Code after:
use serde::Deserialize;

use super::{FontStretch, FontStyle, FontWeight};

#[derive(Debug, Default, Deserialize)]
pub struct Exception {
    pub family: Option<&'static str>,
    pub style: Option<FontStyle>,
    pub weight: Option<FontWeight>,
    pub stretch: Option<FontStretch>,
}

impl Exception {
    pub const fn new() -> Self {
        Self {
            family: None,
            style: None,
            weight: None,
            stretch: None,
        }
    }

    const fn family(self, family: &'static str) -> Self {
        Self { family: Some(family), ..self }
    }

    const fn style(self, style: FontStyle) -> Self {
        Self { style: Some(style), ..self }
    }

    const fn weight(self, weight: u16) -> Self {
        Self { weight: Some(FontWeight(weight)), ..self }
    }

    #[allow(unused)] // left for future use
    const fn stretch(self, stretch: u16) -> Self {
        Self { stretch: Some(FontStretch(stretch)), ..self }
    }
}

pub fn find_exception(postscript_name: &str) -> Option<&'static Exception> {
    EXCEPTION_MAP.get(postscript_name)
}

/// A map which keys are PostScript name and values are override entries.
static EXCEPTION_MAP: phf::Map<&'static str, Exception> = phf::phf_map! {
    "NewCM08-Book" => Exception::new()
        .family("New Computer Modern 08")
Repository: typst/typst
File: crates/typst/src/text/font/book.rs (Rust)

Code before:
use std::cmp::Reverse;
use std::collections::BTreeMap;
use std::fmt::{self, Debug, Formatter};

use serde::{Deserialize, Serialize};
use ttf_parser::{name_id, PlatformId, Tag};
use unicode_segmentation::UnicodeSegmentation;

use crate::text::{Font, FontStretch, FontStyle, FontVariant, FontWeight};

use super::exceptions::find_exception;

/// Metadata about a collection of fonts.
#[derive(Debug, Default, Clone, Hash)]
pub struct FontBook {
    /// Maps from lowercased family names to font indices.
    families: BTreeMap<String, Vec<usize>>,
    /// Metadata about each font in the collection.
    infos: Vec<FontInfo>,
}

impl FontBook {
    /// Create a new, empty font book.
    pub fn new() -> Self {
        Self { families: BTreeMap::new(), infos: vec![] }
    }

    /// Create a font book from a collection of font infos.
    pub fn from_infos(infos: impl IntoIterator<Item = FontInfo>) -> Self {
        let mut book = Self::new();
        for info in infos {
            book.push(info);
        }
        book
    }


Review comment (suggestion):
```suggestion
use super::exceptions::find_exception;
use crate::text::{Font, FontStretch, FontStyle, FontVariant, FontWeight};
```

Code after:
use std::cmp::Reverse;
use std::collections::BTreeMap;
use std::fmt::{self, Debug, Formatter};

use serde::{Deserialize, Serialize};
use ttf_parser::{name_id, PlatformId, Tag};
use unicode_segmentation::UnicodeSegmentation;

use super::exceptions::find_exception;
use crate::text::{Font, FontStretch, FontStyle, FontVariant, FontWeight};

/// Metadata about a collection of fonts.
#[derive(Debug, Default, Clone, Hash)]
pub struct FontBook {
    /// Maps from lowercased family names to font indices.
    families: BTreeMap<String, Vec<usize>>,
    /// Metadata about each font in the collection.
    infos: Vec<FontInfo>,
}

impl FontBook {
    /// Create a new, empty font book.
    pub fn new() -> Self {
        Self { families: BTreeMap::new(), infos: vec![] }
    }

    /// Create a font book from a collection of font infos.
    pub fn from_infos(infos: impl IntoIterator<Item = FontInfo>) -> Self {
        let mut book = Self::new();
        for info in infos {
            book.push(info);
        }
        book
    }

    /// Create a font book for a collection of fonts.
Repository: typst/typst
File: crates/typst/src/math/class.rs (Rust)

Code before:
use unicode_math_class::MathClass;

use crate::diag::SourceResult;
use crate::foundations::{elem, Content, Packed};
use crate::math::{LayoutMath, Limits, MathContext};

/// Forced use of a certain math class.
///
/// This is useful to treat certain symbols as if they were of a different
/// class, e.g. to make a symbol behave like a relation. The class of a symbol
/// defines spacing around the symbol, and how scripts are attached to it by
/// default. Note that the latter can always be overridden using
/// [`{limits}`](math.limits) and [`{scripts}`](math.scripts).
///
/// # Example
/// ```example
/// #let loves = math.class(
///   "relation",
///   sym.suit.heart,
/// )
///
/// $x loves y and y loves 5$
/// ```
#[elem(LayoutMath)]
pub struct ClassElem {
    /// The class to apply to the content.
    #[required]
    pub class: MathClass,

    /// The content to which the class is applied.
    #[required]
    pub body: Content,
}

impl LayoutMath for Packed<ClassElem> {
    #[typst_macros::time(name = "math.class", span = self.span())]

Review comment (suggestion):
If I understand it right, it isn't only the spacing. The symbol also may be moved vertically, and I think there should be more consequences. I think it's important this is not just the spacing.

See this example, where `b` letter moves down when set as `opening` class:

https://sitandr.github.io/typst-examples-book/book/basics/math/classes.html

So I have no idea how to write it in a good way. I haven't look thoroughly in the source, though.

Code after:
use unicode_math_class::MathClass;

use crate::diag::SourceResult;
use crate::foundations::{elem, Content, Packed};
use crate::math::{LayoutMath, Limits, MathContext};

/// Forced use of a certain math class.
///
/// This is useful to treat certain symbols as if they were of a different
/// class, e.g. to make a symbol behave like a relation. The class of a symbol
/// defines the way it is laid out, including spacing around it, and how its
/// scripts are attached by default. Note that the latter can always be
/// overridden using [`{limits}`](math.limits) and [`{scripts}`](math.scripts).
///
/// # Example
/// ```example
/// #let loves = math.class(
///   "relation",
///   sym.suit.heart,
/// )
///
/// $x loves y and y loves 5$
/// ```
#[elem(LayoutMath)]
pub struct ClassElem {
    /// The class to apply to the content.
    #[required]
    pub class: MathClass,

    /// The content to which the class is applied.
    #[required]
    pub body: Content,
}

impl LayoutMath for Packed<ClassElem> {
    #[typst_macros::time(name = "math.class", span = self.span())]
Repository: typst/typst
File: crates/typst/src/model/cite.rs (Rust)

Code before:
use crate::introspection::Locatable;
use crate::model::bibliography::Works;
use crate::model::CslStyle;
use crate::text::{Lang, Region, TextElem};

/// Cite a work from the bibliography.
///
/// Before you starting citing, you need to add a [bibliography]($bibliography)
/// somewhere in your document.
///
/// # Example
/// ```example
/// This was already noted by
/// pirates long ago. @arrgh
///
/// Multiple sources say ...
/// @arrgh @netwok.
///
/// You can also call `cite`
/// explicitly. #cite(<arrgh>)
///
/// If your source name contains
/// certain characters such as slashes
/// which are not recognized by the `<>` syntax,
/// you can use `label` instead.
/// #cite(label("DBLP:books/lib/Knuth86a"))
///
/// #bibliography("works.bib")
/// ```
///
/// # Syntax
/// This function indirectly has dedicated syntax. [References]($ref) can be
/// used to cite works from the bibliography. The label then corresponds to the
/// citation key.
#[elem(Synthesize)]
pub struct CiteElem {
    /// The citation key that identifies the entry in the bibliography that
    /// shall be cited, as a label.
    ///
    /// ```example
    /// // All the same
    /// @netwok \
    /// #cite(<netwok>) \
    /// #cite(label("netwok"))
    /// >>> #set text(0pt)
    /// >>> #bibliography("works.bib", style: "apa")
    /// ```
    #[required]
    pub key: Label,

    /// A supplement for the citation such as page or chapter number.

Review comment (suggestion):
Please move the explanation out of the example into actual prose. You can add a second example below the first one.

Code after:
use crate::introspection::Locatable;
use crate::model::bibliography::Works;
use crate::model::CslStyle;
use crate::text::{Lang, Region, TextElem};

/// Cite a work from the bibliography.
///
/// Before you starting citing, you need to add a [bibliography]($bibliography)
/// somewhere in your document.
///
/// # Example
/// ```example
/// This was already noted by
/// pirates long ago. @arrgh
///
/// Multiple sources say ...
/// @arrgh @netwok.
///
/// You can also call `cite`
/// explicitly. #cite(<arrgh>)
///
/// #bibliography("works.bib")
/// ```
///
/// If your source name contains certain characters such as slashes, which are
/// not recognized by the `<>` syntax, you can explicitly call `label` instead.
///
/// ```typ
/// Computer Modern is an example of a modernist serif typeface.
/// #cite(label("DBLP:books/lib/Knuth86a")).
/// >>> #bibliography("works.bib")
/// ```
///
/// # Syntax
/// This function indirectly has dedicated syntax. [References]($ref) can be
/// used to cite works from the bibliography. The label then corresponds to the
/// citation key.
#[elem(Synthesize)]
pub struct CiteElem {
    /// The citation key that identifies the entry in the bibliography that
    /// shall be cited, as a label.
    ///
    /// ```example
    /// // All the same
    /// @netwok \
    /// #cite(<netwok>) \
    /// #cite(label("netwok"))
    /// >>> #set text(0pt)
    /// >>> #bibliography("works.bib", style: "apa")
    /// ```
    #[required]
Repository: typst/typst
File: crates/typst/src/foundations/dict.rs (Rust)

Code before:
    }

    /// Returns the keys of the dictionary as an array in insertion order.
    #[func]
    pub fn keys(&self) -> Array {
        self.0.keys().cloned().map(Value::Str).collect()
    }

    /// Returns the values of the dictionary as an array in insertion order.
    #[func]
    pub fn values(&self) -> Array {
        self.0.values().cloned().collect()
    }

    /// Returns the keys and values of the dictionary as an array of pairs. Each
    /// pair is represented as an array of length two.
    #[func]
    pub fn pairs(&self) -> Array {
        self.0
            .iter()
            .map(|(k, v)| Value::Array(array![k.clone(), v.clone()]))
            .collect()
    }
}

/// A value that can be cast to bytes.
pub struct ToDict(Dict);

cast! {
    ToDict,
    v: Module => Self(v.scope().iter().map(|(k, v)| (Str::from(k.clone()), v.clone())).collect()),
}

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

impl Repr for Dict {
    fn repr(&self) -> EcoString {
        if self.is_empty() {
            return "(:)".into();
        }

        let max = 40;
        let mut pieces: Vec<_> = self
            .iter()
            .take(max)
            .map(|(key, value)| {
                if is_ident(key) {

Review comment (suggestion):
```suggestion
/// A value that can be cast to dictionary.
```


Code after:
    }

    /// Returns the keys of the dictionary as an array in insertion order.
    #[func]
    pub fn keys(&self) -> Array {
        self.0.keys().cloned().map(Value::Str).collect()
    }

    /// Returns the values of the dictionary as an array in insertion order.
    #[func]
    pub fn values(&self) -> Array {
        self.0.values().cloned().collect()
    }

    /// Returns the keys and values of the dictionary as an array of pairs. Each
    /// pair is represented as an array of length two.
    #[func]
    pub fn pairs(&self) -> Array {
        self.0
            .iter()
            .map(|(k, v)| Value::Array(array![k.clone(), v.clone()]))
            .collect()
    }
}

/// A value that can be cast to dictionary.
pub struct ToDict(Dict);

cast! {
    ToDict,
    v: Module => Self(v.scope().iter().map(|(k, v)| (Str::from(k.clone()), v.clone())).collect()),
}

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

impl Repr for Dict {
    fn repr(&self) -> EcoString {
        if self.is_empty() {
            return "(:)".into();
        }

        let max = 40;
        let mut pieces: Vec<_> = self
            .iter()
            .take(max)
            .map(|(key, value)| {
                if is_ident(key) {
Repository: typst/typst
File: crates/typst-docs/src/main.rs (Rust)

Code before:
use std::{
    fs::{create_dir_all, write},
    path::{Path, PathBuf},
};

use clap::Parser;
use typst::{model::Document, visualize::Color};
use typst_docs::{provide, Html, Resolver};
use typst_render::render;

struct MyResolver<'a> {
    out_dir: &'a Path,
    verbose: bool,
}
impl<'a> Resolver for MyResolver<'a> {
    fn commits(&self, from: &str, to: &str) -> Vec<typst_docs::Commit> {
        if self.verbose {
            eprintln!("commits({from}, {to})");
        }
        vec![]
    }
    fn example(
        &self,
        hash: u128,
        source: Option<Html>,
        document: &Document,
    ) -> typst_docs::Html {
        if self.verbose {
            eprintln!(

Review comment (suggestion):
```suggestion
use std::fs;
use std::path::{Path, PathBuf};
```
and then `fs::create_dir_all` and `fs::write` (in particular the latter is easier to understand that way in my opinion).

Code after:
use std::fs;
use std::path::{Path, PathBuf};

use clap::Parser;
use typst::model::Document;
use typst::visualize::Color;
use typst_docs::{provide, Html, Resolver};
use typst_render::render;

#[derive(Debug)]
struct CliResolver<'a> {
    assets_dir: &'a Path,
    verbose: bool,
    base: &'a str,
}

impl<'a> Resolver for CliResolver<'a> {
    fn commits(&self, from: &str, to: &str) -> Vec<typst_docs::Commit> {
        if self.verbose {
            eprintln!("commits({from}, {to})");
        }
        vec![]
    }

    fn example(
        &self,
        hash: u128,
        source: Option<Html>,
        document: &Document,
Repository: typst/typst
File: crates/typst-docs/src/main.rs (Rust)

Code before:
use std::{
    fs::{create_dir_all, write},
    path::{Path, PathBuf},
};

use clap::Parser;
use typst::{model::Document, visualize::Color};
use typst_docs::{provide, Html, Resolver};
use typst_render::render;

struct MyResolver<'a> {
    out_dir: &'a Path,
    verbose: bool,
}
impl<'a> Resolver for MyResolver<'a> {
    fn commits(&self, from: &str, to: &str) -> Vec<typst_docs::Commit> {
        if self.verbose {
            eprintln!("commits({from}, {to})");
        }
        vec![]
    }
    fn example(
        &self,
        hash: u128,
        source: Option<Html>,
        document: &Document,
    ) -> typst_docs::Html {
        if self.verbose {
            eprintln!(
                "example(0x{hash:x}, {:?} chars, Document)",
                source.as_ref().map(|s| s.as_str().len())
            );

Review comment (suggestion):
```suggestion
use typst::model::Document;
use typst::visualize::Color;
```

Code after:
use std::fs;
use std::path::{Path, PathBuf};

use clap::Parser;
use typst::model::Document;
use typst::visualize::Color;
use typst_docs::{provide, Html, Resolver};
use typst_render::render;

#[derive(Debug)]
struct CliResolver<'a> {
    assets_dir: &'a Path,
    verbose: bool,
    base: &'a str,
}

impl<'a> Resolver for CliResolver<'a> {
    fn commits(&self, from: &str, to: &str) -> Vec<typst_docs::Commit> {
        if self.verbose {
            eprintln!("commits({from}, {to})");
        }
        vec![]
    }

    fn example(
        &self,
        hash: u128,
        source: Option<Html>,
        document: &Document,
    ) -> typst_docs::Html {
        if self.verbose {
            eprintln!(
Repository: typst/typst
File: crates/typst/src/util/hash.rs (Rust)

Code before:
///
/// # Usage
/// It is best used inside of an `Arc` or `Rc` to best re-use the hash once it
/// has been computed.
pub struct LazyHash<T: ?Sized> {
    /// The hash for the value.
    hash: AtomicU128,
    /// The underlying value.
    value: T,
}

impl<T> LazyHash<T> {
    /// Wrap an item without pre-computed hash.
    #[inline]
    pub fn new(value: T) -> Self {
        Self { hash: AtomicU128::new(0), value }
    }

    /// Wrap an item with a pre-computed hash.
    ///
    /// # Safety
    /// The hash must be correct for the value. This cannot be enforced at
    /// compile time, so use with caution.
    #[inline]
    pub unsafe fn with_hash(value: T, hash: u128) -> Self {
        Self { hash: AtomicU128::new(hash), value }
    }

    /// Return the wrapped value.
    #[inline]
    pub fn into_inner(self) -> T {
        self.value
    }
}

impl<T: Hash + ?Sized + 'static> LazyHash<T> {
    /// Get the hash, returns zero if not computed yet.
    #[inline]
    pub fn hash(&self) -> u128 {
        self.hash.load(Ordering::SeqCst)
    }

    /// Reset the hash to zero.
    #[inline]
    fn reset_hash(&mut self) {
        // Because we have a mutable reference, we can skip the atomic
        *self.hash.get_mut() = 0;
    }

    /// Get the hash or compute it if not set yet.
    #[inline]

Review comment (bug):
```suggestion
    /// **Important:** The hash must be correct for the value. This cannot be
    /// enforced at compile time, so use with caution.
    #[inline]
    pub fn with_hash(value: T, hash: u128) -> Self {
        Self { hash: AtomicU128::new(hash), value }
```
I don't think this should be `unsafe`. Unsafe should be reserved for things that can actually cause memory unsafety or undefined behaviour, not logic bugs.

Code after:
/// hasher.**
///
/// # Usage
/// If the value is expected to be cloned, it is best used inside of an `Arc`
/// or `Rc` to best re-use the hash once it has been computed.
pub struct LazyHash<T: ?Sized> {
    /// The hash for the value.
    hash: AtomicU128,
    /// The underlying value.
    value: T,
}

impl<T: Default> Default for LazyHash<T> {
    #[inline]
    fn default() -> Self {
        Self::new(Default::default())
    }
}

impl<T> LazyHash<T> {
    /// Wrap an item without pre-computed hash.
    #[inline]
    pub fn new(value: T) -> Self {
        Self { hash: AtomicU128::new(0), value }
    }

    /// Wrap an item with a pre-computed hash.
    ///
    /// **Important:** The hash must be correct for the value. This cannot be
    /// enforced at compile time, so use with caution.
    #[inline]
    pub fn with_hash(value: T, hash: u128) -> Self {
        Self { hash: AtomicU128::new(hash), value }
    }

    /// Return the wrapped value.
    #[inline]
    pub fn into_inner(self) -> T {
        self.value
    }
}

impl<T: Hash + ?Sized + 'static> LazyHash<T> {
    /// Get the hash, returns zero if not computed yet.
    #[inline]
    pub fn hash(&self) -> u128 {
        self.hash.load(Ordering::SeqCst)
    }

    /// Reset the hash to zero.
    #[inline]