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

Code before:
            },
        }
    }
}

/// A pair of vector / matrix delimiters.
#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)]
pub struct DelimiterPair {
    open: Delimiter,
    close: Delimiter,
}

cast! {
    DelimiterPair,

    self => array![self.open, self.close].into_value(),

    v: Array => {
        let v = v.as_slice();
        if v.len() != 2 {
            bail!("expected 2 delimiters, found {}", v.len());
        }
        let open = v[0].clone().cast()?;
        let close = v[1].clone().cast()?;
        Self { open, close }
    },
    v: Delimiter => {
        Self {
            open: v,
            close: v.find_matching(),
        }
    }
}

impl DelimiterPair {
    const PAREN: Self = Self {
        open: Delimiter(Some('(')),
        close: Delimiter(Some(')')),
    };
    const BRACE: Self = Self {
        open: Delimiter(Some('{')),
        close: Delimiter(Some('}')),
    };

    /// The delimiter's opening character.
    fn open(self) -> Option<char> {
        self.open.get()
    }

    /// The delimiter's closing character.
    fn close(self) -> Option<char> {

Review comment (suggestion):
```suggestion
    v: Array => match v.as_slice() {
        [open, close] => Self {
            open: open.clone().cast()?,
            close: close.clone().cast()?,
        },
        _ => bail!("expected 2 delimiters, found {}", v.len())
    },
```

Code after:
    }
}

/// A pair of delimiters (one closing, one opening) used for matrices, vectors
/// and cases.
#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)]
pub struct DelimiterPair {
    open: Delimiter,
    close: Delimiter,
}

cast! {
    DelimiterPair,

    self => array![self.open, self.close].into_value(),

    v: Array => match v.as_slice() {
        [open, close] => Self {
            open: open.clone().cast()?,
            close: close.clone().cast()?,
        },
        _ => bail!("expected 2 delimiters, found {}", v.len())
    },
    v: Delimiter => Self { open: v, close: v.find_matching() }
}

impl DelimiterPair {
    const PAREN: Self = Self {
        open: Delimiter(Some('(')),
        close: Delimiter(Some(')')),
    };
    const BRACE: Self = Self {
        open: Delimiter(Some('{')),
        close: Delimiter(Some('}')),
    };

    /// The delimiter's opening character.
    fn open(self) -> Option<char> {
        self.open.get()
    }

    /// The delimiter's closing character.
    fn close(self) -> Option<char> {
        self.close.get()
    }
}

/// Layout the inner contents of a vector.
fn layout_vec_body(
    ctx: &mut MathContext,
    styles: StyleChain,
Repository: typst/typst
File: crates/typst/src/math/matrix.rs (Rust)

Code before:
#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)]
pub struct DelimiterPair {
    open: Delimiter,
    close: Delimiter,
}

cast! {
    DelimiterPair,

    self => array![self.open, self.close].into_value(),

    v: Array => {
        let v = v.as_slice();
        if v.len() != 2 {
            bail!("expected 2 delimiters, found {}", v.len());
        }
        let open = v[0].clone().cast()?;
        let close = v[1].clone().cast()?;
        Self { open, close }
    },
    v: Delimiter => {
        Self {
            open: v,
            close: v.find_matching(),
        }
    }
}

impl DelimiterPair {
    const PAREN: Self = Self {
        open: Delimiter(Some('(')),
        close: Delimiter(Some(')')),
    };
    const BRACE: Self = Self {
        open: Delimiter(Some('{')),
        close: Delimiter(Some('}')),
    };

    /// The delimiter's opening character.
    fn open(self) -> Option<char> {
        self.open.get()
    }

    /// The delimiter's closing character.
    fn close(self) -> Option<char> {
        self.close.get()
    }
}

/// Layout the inner contents of a vector.
fn layout_vec_body(

Review comment (suggestion):
```suggestion
    v: Delimiter => Self { open: v, close: v.find_matching() },
```
macros prevent rustfmt, but I think it would probably be this

Code after:
pub struct DelimiterPair {
    open: Delimiter,
    close: Delimiter,
}

cast! {
    DelimiterPair,

    self => array![self.open, self.close].into_value(),

    v: Array => match v.as_slice() {
        [open, close] => Self {
            open: open.clone().cast()?,
            close: close.clone().cast()?,
        },
        _ => bail!("expected 2 delimiters, found {}", v.len())
    },
    v: Delimiter => Self { open: v, close: v.find_matching() }
}

impl DelimiterPair {
    const PAREN: Self = Self {
        open: Delimiter(Some('(')),
        close: Delimiter(Some(')')),
    };
    const BRACE: Self = Self {
        open: Delimiter(Some('{')),
        close: Delimiter(Some('}')),
    };

    /// The delimiter's opening character.
    fn open(self) -> Option<char> {
        self.open.get()
    }

    /// The delimiter's closing character.
    fn close(self) -> Option<char> {
        self.close.get()
    }
}

/// Layout the inner contents of a vector.
fn layout_vec_body(
    ctx: &mut MathContext,
    styles: StyleChain,
    column: &[Content],
    align: FixedAlignment,
    row_gap: Rel<Abs>,
    alternator: LeftRightAlternator,
) -> SourceResult<Frame> {
    let gap = row_gap.relative_to(ctx.regions.base().y);
Repository: typst/typst
File: crates/typst-cli/src/world.rs (Rust)

Code before:
            with_offset.day().try_into().ok()?,
        )
    }
}

impl SystemWorld {
    /// Access the canonical slot for the given file id.
    fn slot<F, T>(&self, id: FileId, f: F) -> T
    where
        F: FnOnce(&mut FileSlot) -> T,
    {
        let mut map = self.slots.lock();
        f(map
            .entry(id)
            .or_insert_with(|| FileSlot::new(id, Arc::clone(&self.package_storage))))
    }
}

/// Holds the processed data for a file ID.
///
/// Both fields can be populated if the file is both imported and read().
struct FileSlot {
    /// The slot's file id.
    id: FileId,
    /// Package storage information, defining where packages are located.
    package_storage: Arc<PackageStorage>,
    /// The lazily loaded and incrementally updated source file.
    source: SlotCell<Source>,
    /// The lazily loaded raw byte buffer.
    file: SlotCell<Bytes>,
}

impl FileSlot {
    /// Create a new file slot.
    fn new(id: FileId, package_storage: Arc<PackageStorage>) -> Self {
        Self {
            id,
            file: SlotCell::new(),
            package_storage,
            source: SlotCell::new(),
        }
    }

    /// Whether the file was accessed in the ongoing compilation.
    fn accessed(&self) -> bool {
        self.source.accessed() || self.file.accessed()
    }

    /// Marks the file as not yet accessed in preparation of the next
    /// compilation.
    fn reset(&mut self) {

Review comment (suggestion):
Instead of storing the `PackageStorage` in an `Arc` and cloning it here, we could pass it as a reference like `project_root`. It's a little verbose, but this `Arc` feels a little unnecessary for something that is essentially a global variable.

Code after:
            with_offset.day().try_into().ok()?,
        )
    }
}

impl SystemWorld {
    /// Access the canonical slot for the given file id.
    fn slot<F, T>(&self, id: FileId, f: F) -> T
    where
        F: FnOnce(&mut FileSlot) -> T,
    {
        let mut map = self.slots.lock();
        f(map.entry(id).or_insert_with(|| FileSlot::new(id)))
    }
}

/// Holds the processed data for a file ID.
///
/// Both fields can be populated if the file is both imported and read().
struct FileSlot {
    /// The slot's file id.
    id: FileId,
    /// The lazily loaded and incrementally updated source file.
    source: SlotCell<Source>,
    /// The lazily loaded raw byte buffer.
    file: SlotCell<Bytes>,
}

impl FileSlot {
    /// Create a new file slot.
    fn new(id: FileId) -> Self {
        Self { id, file: SlotCell::new(), source: SlotCell::new() }
    }

    /// Whether the file was accessed in the ongoing compilation.
    fn accessed(&self) -> bool {
        self.source.accessed() || self.file.accessed()
    }

    /// Marks the file as not yet accessed in preparation of the next
    /// compilation.
    fn reset(&mut self) {
        self.source.reset();
        self.file.reset();
    }

    /// Retrieve the source for this file.
    fn source(
        &mut self,
        project_root: &Path,
        package_storage: &PackageStorage,
Repository: typst/typst
File: crates/typst-cli/src/main.rs (Rust)

Code before:
use std::cell::Cell;
use std::io::{self, Write};
use std::process::ExitCode;

use clap::Parser;
use codespan_reporting::term;
use codespan_reporting::term::termcolor::WriteColor;
use once_cell::sync::Lazy;
use typst::diag::HintedString;

use crate::args::{CliArguments, Command};
use crate::timings::Timer;

thread_local! {
    /// The CLI's exit code.
    static EXIT: Cell<ExitCode> = const { Cell::new(ExitCode::SUCCESS) };
}

/// The parsed commandline arguments.
static ARGS: Lazy<CliArguments> = Lazy::new(CliArguments::parse);

/// Entry point.
fn main() -> ExitCode {
    let timer = Timer::new(&ARGS);

    let res = match &ARGS.command {
        Command::Compile(command) => {
            crate::compile::compile(timer, command.clone()).map_err(HintedString::from)
        }
        Command::Watch(command) => {
            crate::watch::watch(timer, command.clone()).map_err(HintedString::from)
        }
        Command::Init(command) => crate::init::init(command).map_err(HintedString::from),
        Command::Query(command) => crate::query::query(command),
        Command::Fonts(command) => {
            crate::fonts::fonts(command).map_err(HintedString::from)
        }
        Command::Update(command) => {
            crate::update::update(command).map_err(HintedString::from)
        }
    };

    if let Err(msg) = res {
        set_failed();
        print_error(msg.message()).expect("failed to print error");
    }

    EXIT.with(|cell| cell.get())
}

/// Ensure a failure exit code.

Review comment (suggestion):
maybe we could move the match into a separate function, so that we can use `?` instead of all the `map_err`? could be called `run` or `dispatch`.

Code after:
use std::cell::Cell;
use std::io::{self, Write};
use std::process::ExitCode;

use clap::Parser;
use codespan_reporting::term;
use codespan_reporting::term::termcolor::WriteColor;
use once_cell::sync::Lazy;
use typst::diag::HintedStrResult;

use crate::args::{CliArguments, Command};
use crate::timings::Timer;

thread_local! {
    /// The CLI's exit code.
    static EXIT: Cell<ExitCode> = const { Cell::new(ExitCode::SUCCESS) };
}

/// The parsed commandline arguments.
static ARGS: Lazy<CliArguments> = Lazy::new(CliArguments::parse);

/// Entry point.
fn main() -> ExitCode {
    let res = dispatch();

    if let Err(msg) = res {
        set_failed();
        print_error(msg.message()).expect("failed to print error");
    }

    EXIT.with(|cell| cell.get())
}

/// Execute the requested command.
fn dispatch() -> HintedStrResult<()> {
    let timer = Timer::new(&ARGS);

    match &ARGS.command {
        Command::Compile(command) => crate::compile::compile(timer, command.clone())?,
        Command::Watch(command) => crate::watch::watch(timer, command.clone())?,
        Command::Init(command) => crate::init::init(command)?,
        Command::Query(command) => crate::query::query(command)?,
        Command::Fonts(command) => crate::fonts::fonts(command)?,
        Command::Update(command) => crate::update::update(command)?,
    }

    Ok(())
}

/// Ensure a failure exit code.
fn set_failed() {
Repository: typst/typst
File: crates/typst/src/diag.rs (Rust)

Code before:
    pub hints: EcoVec<EcoString>,
}

/// The severity of a [`SourceDiagnostic`].
#[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)]
pub enum Severity {
    /// A fatal error.
    Error,
    /// A non-fatal warning.
    Warning,
}

impl SourceDiagnostic {
    /// Create a new, bare error.
    pub fn error(span: Span, message: impl Into<EcoString>) -> Self {
        Self {
            severity: Severity::Error,
            span,
            trace: eco_vec![],
            message: message.into(),
            hints: eco_vec![],
        }
    }

    /// Create a new error with hints.
    pub fn hinted_error(span: Span, hinted_message: HintedString) -> Self {
        let mut components = hinted_message.components.into_iter();
        Self {
            severity: Severity::Error,
            span,
            trace: eco_vec![],
            message: components.next().unwrap(),
            hints: components.collect(),
        }
    }

    /// Create a new, bare warning.
    pub fn warning(span: Span, message: impl Into<EcoString>) -> Self {
        Self {
            severity: Severity::Warning,
            span,
            trace: eco_vec![],
            message: message.into(),
            hints: eco_vec![],
        }
    }

    /// Adds a single hint to the diagnostic.
    pub fn hint(&mut self, hint: impl Into<EcoString>) {
        self.hints.push(hint.into());
    }

Review comment (suggestion):
I think it's nicer if we keep this in the `At` impl, so that there is only one way to do this for API consumers.

Code after:

/// An error or warning in a source file.
///
/// The contained spans will only be detached if any of the input source files
/// were detached.
#[derive(Debug, Clone, Eq, PartialEq, Hash)]
pub struct SourceDiagnostic {
    /// Whether the diagnostic is an error or a warning.
    pub severity: Severity,
    /// The span of the relevant node in the source code.
    pub span: Span,
    /// A diagnostic message describing the problem.
    pub message: EcoString,
    /// The trace of function calls leading to the problem.
    pub trace: EcoVec<Spanned<Tracepoint>>,
    /// Additional hints to the user, indicating how this problem could be avoided
    /// or worked around.
    pub hints: EcoVec<EcoString>,
}

/// The severity of a [`SourceDiagnostic`].
#[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)]
pub enum Severity {
    /// A fatal error.
    Error,
    /// A non-fatal warning.
    Warning,
}

impl SourceDiagnostic {
    /// Create a new, bare error.
    pub fn error(span: Span, message: impl Into<EcoString>) -> Self {
        Self {
            severity: Severity::Error,
            span,
            trace: eco_vec![],
            message: message.into(),
            hints: eco_vec![],
        }
    }

    /// Create a new, bare warning.
    pub fn warning(span: Span, message: impl Into<EcoString>) -> Self {
        Self {
            severity: Severity::Warning,
            span,
            trace: eco_vec![],
            message: message.into(),
            hints: eco_vec![],
        }
    }
Repository: typst/typst
File: crates/typst/src/symbols/symbol.rs (Rust)

Code before:
/// function.
///
/// ```example
/// #sym.arrow.r \
/// #sym.gt.eq.not \
/// $gt.eq.not$ \
/// #emoji.face.halo
/// ```
///
/// Many symbols have different variants, which can be selected by appending the
/// modifiers with dot notation. The order of the modifiers is not relevant.
/// Visit the documentation pages of the symbol modules and click on a symbol to
/// see its available variants.
///
/// ```example
/// $arrow.l$ \
/// $arrow.r$ \
/// $arrow.t.quad$
/// ```
#[ty(scope, cast)]
#[derive(Debug, Clone, Eq, PartialEq, Hash)]
pub struct Symbol(Repr);

/// The character of a symbol, possibly with a function.
#[derive(Debug, Clone, Eq, PartialEq, Hash)]
pub enum SymChar {
    Pure(char),
    WithFunc(char, fn() -> Func),
}

/// The internal representation.
#[derive(Clone, Eq, PartialEq, Hash)]
enum Repr {
    Single(SymChar),
    Const(&'static [(&'static str, SymChar)]),
    Multi(Arc<(List, EcoString)>),
}

/// A collection of symbols.
#[derive(Clone, Eq, PartialEq, Hash)]
enum List {
    Static(&'static [(&'static str, SymChar)]),
    Runtime(Box<[(EcoString, SymChar)]>),
}

impl Symbol {
    /// Create a new symbol from a single character.
    pub const fn single(c: SymChar) -> Self {
        Self(Repr::Single(c))
    }


Review comment (suggestion):
I think a struct with fields of type `char` and `Option<fn() -> Func>` would be a bit simpler.

Code after:
/// function.
///
/// ```example
/// #sym.arrow.r \
/// #sym.gt.eq.not \
/// $gt.eq.not$ \
/// #emoji.face.halo
/// ```
///
/// Many symbols have different variants, which can be selected by appending the
/// modifiers with dot notation. The order of the modifiers is not relevant.
/// Visit the documentation pages of the symbol modules and click on a symbol to
/// see its available variants.
///
/// ```example
/// $arrow.l$ \
/// $arrow.r$ \
/// $arrow.t.quad$
/// ```
#[ty(scope, cast)]
#[derive(Debug, Clone, Eq, PartialEq, Hash)]
pub struct Symbol(Repr);

/// The character of a symbol, possibly with a function.
#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)]
pub struct SymChar(char, Option<fn() -> Func>);

/// The internal representation.
#[derive(Clone, Eq, PartialEq, Hash)]
enum Repr {
    Single(SymChar),
    Const(&'static [(&'static str, SymChar)]),
    Multi(Arc<(List, EcoString)>),
}

/// A collection of symbols.
#[derive(Clone, Eq, PartialEq, Hash)]
enum List {
    Static(&'static [(&'static str, SymChar)]),
    Runtime(Box<[(EcoString, SymChar)]>),
}

impl Symbol {
    /// Create a new symbol from a single character.
    pub const fn single(c: SymChar) -> Self {
        Self(Repr::Single(c))
    }

    /// Create a symbol with a static variant list.
    #[track_caller]
    pub const fn list(list: &'static [(&'static str, SymChar)]) -> Self {
Repository: typst/typst
File: crates/typst/src/symbols/symbol.rs (Rust)

Code before:
    fn fmt(&self, f: &mut Formatter) -> fmt::Result {
        f.write_char(self.get())
    }
}

impl SymChar {
    /// Create a symbol character without a function.
    pub const fn pure(c: char) -> Self {
        Self::Pure(c)
    }

    /// Create a symbol character with a function.
    pub const fn with_func(c: char, func: fn() -> Func) -> Self {
        Self::WithFunc(c, func)
    }

    /// Get the function associated with the symbol.
    pub fn func(&self) -> Option<Func> {
        match self {
            Self::Pure(_) => None,
            Self::WithFunc(_, func) => Some((func)()),
        }
    }
}

impl From<&SymChar> for char {
    fn from(sym: &SymChar) -> Self {
        *match sym {
            SymChar::Pure(c) => c,
            SymChar::WithFunc(c, _) => c,
        }
    }
}

impl From<char> for SymChar {
    fn from(c: char) -> Self {
        SymChar::Pure(c)
    }
}

impl Debug for Repr {
    fn fmt(&self, f: &mut Formatter) -> fmt::Result {
        match self {
            Self::Single(c) => Debug::fmt(c, f),
            Self::Const(list) => list.fmt(f),
            Self::Multi(lists) => lists.fmt(f),
        }
    }
}

impl Debug for List {

Review comment (suggestion):
I think an accessor would be clearer for this. The `char -> SymChar` from impl is fine, but this one feels a bit off.

Code after:
    fn fmt(&self, f: &mut Formatter) -> fmt::Result {
        f.write_char(self.get())
    }
}

impl SymChar {
    /// Create a symbol character without a function.
    pub const fn pure(c: char) -> Self {
        Self(c, None)
    }

    /// Create a symbol character with a function.
    pub const fn with_func(c: char, func: fn() -> Func) -> Self {
        Self(c, Some(func))
    }

    /// Get the character of the symbol.
    pub const fn char(&self) -> char {
        self.0
    }

    /// Get the function associated with the symbol.
    pub fn func(&self) -> Option<Func> {
        self.1.map(|f| f())
    }
}

impl From<char> for SymChar {
    fn from(c: char) -> Self {
        SymChar(c, None)
    }
}

impl Debug for Repr {
    fn fmt(&self, f: &mut Formatter) -> fmt::Result {
        match self {
            Self::Single(c) => Debug::fmt(c, f),
            Self::Const(list) => list.fmt(f),
            Self::Multi(lists) => lists.fmt(f),
        }
    }
}

impl Debug for List {
    fn fmt(&self, f: &mut Formatter) -> fmt::Result {
        match self {
            Self::Static(list) => list.fmt(f),
            Self::Runtime(list) => list.fmt(f),
        }
    }
}
Repository: typst/typst
File: crates/typst/src/symbols/symbol.rs (Rust)

Code before:
/// Moreover, you can define custom symbols with this type's constructor
/// function.
///
/// ```example
/// #sym.arrow.r \
/// #sym.gt.eq.not \
/// $gt.eq.not$ \
/// #emoji.face.halo
/// ```
///
/// Many symbols have different variants, which can be selected by appending the
/// modifiers with dot notation. The order of the modifiers is not relevant.
/// Visit the documentation pages of the symbol modules and click on a symbol to
/// see its available variants.
///
/// ```example
/// $arrow.l$ \
/// $arrow.r$ \
/// $arrow.t.quad$
/// ```
#[ty(scope, cast)]
#[derive(Debug, Clone, Eq, PartialEq, Hash)]
pub struct Symbol(Repr);

/// The character of a symbol, possibly with a function.
#[derive(Debug, Clone, Eq, PartialEq, Hash)]
pub enum SymChar {
    Pure(char),
    WithFunc(char, fn() -> Func),
}

/// The internal representation.
#[derive(Clone, Eq, PartialEq, Hash)]
enum Repr {
    Single(SymChar),
    Const(&'static [(&'static str, SymChar)]),
    Multi(Arc<(List, EcoString)>),
}

/// A collection of symbols.
#[derive(Clone, Eq, PartialEq, Hash)]
enum List {
    Static(&'static [(&'static str, SymChar)]),
    Runtime(Box<[(EcoString, SymChar)]>),
}

impl Symbol {
    /// Create a new symbol from a single character.
    pub const fn single(c: SymChar) -> Self {
        Self(Repr::Single(c))
    }

Review comment (suggestion):
This could implement `Copy` and then we can probably get rid of a few `cloned` and the `ref` in the destructuring.

Code after:
/// Moreover, you can define custom symbols with this type's constructor
/// function.
///
/// ```example
/// #sym.arrow.r \
/// #sym.gt.eq.not \
/// $gt.eq.not$ \
/// #emoji.face.halo
/// ```
///
/// Many symbols have different variants, which can be selected by appending the
/// modifiers with dot notation. The order of the modifiers is not relevant.
/// Visit the documentation pages of the symbol modules and click on a symbol to
/// see its available variants.
///
/// ```example
/// $arrow.l$ \
/// $arrow.r$ \
/// $arrow.t.quad$
/// ```
#[ty(scope, cast)]
#[derive(Debug, Clone, Eq, PartialEq, Hash)]
pub struct Symbol(Repr);

/// The character of a symbol, possibly with a function.
#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)]
pub struct SymChar(char, Option<fn() -> Func>);

/// The internal representation.
#[derive(Clone, Eq, PartialEq, Hash)]
enum Repr {
    Single(SymChar),
    Const(&'static [(&'static str, SymChar)]),
    Multi(Arc<(List, EcoString)>),
}

/// A collection of symbols.
#[derive(Clone, Eq, PartialEq, Hash)]
enum List {
    Static(&'static [(&'static str, SymChar)]),
    Runtime(Box<[(EcoString, SymChar)]>),
}

impl Symbol {
    /// Create a new symbol from a single character.
    pub const fn single(c: SymChar) -> Self {
        Self(Repr::Single(c))
    }

    /// Create a symbol with a static variant list.
    #[track_caller]