ronantakizawa/codereview-bench huggingface dataset:
Repository: typst/typst
File: tests/src/args.rs (Rust)

Code before:
use regex::Regex;
use std::path::PathBuf;

use clap::{Parser, Subcommand};

/// Typst's test runner.
#[derive(Debug, Clone, Parser)]
#[command(bin_name = "cargo test --workspace --test tests --")]
#[clap(name = "typst-test", author)]
pub struct CliArguments {
    /// The command to run.
    #[command(subcommand)]
    pub command: Option<Command>,
    /// All the tests whose names match the test name pattern will be run.
    #[arg(value_parser = Regex::new)]
    pub pattern: Vec<Regex>,
    /// Restricts test selection within the given path.
    #[arg(short, long, value_parser = |s: &str| PathBuf::from(s).canonicalize())]
    pub path: Vec<PathBuf>,
    /// Only selects the test that matches with the test name verbatim.
    #[arg(short, long)]
    pub exact: bool,
    /// Lists what tests will be run, without actually running them.
    #[arg(long, group = "action")]
    pub list: bool,
    /// Updates the reference images of non-passing tests.
    #[arg(short, long, group = "action")]
    pub update: bool,
    /// The scaling factor to render the output image with.

Review comment (suggestion):
please move the `regex` import from the std group into the third-party group

Code after:
use std::path::PathBuf;

use clap::{Parser, Subcommand};
use regex::Regex;

/// Typst's test runner.
#[derive(Debug, Clone, Parser)]
#[command(bin_name = "cargo test --workspace --test tests --")]
#[clap(name = "typst-test", author)]
pub struct CliArguments {
    /// The command to run.
    #[command(subcommand)]
    pub command: Option<Command>,
    /// All the tests whose names match the test name pattern will be run.
    #[arg(value_parser = Regex::new)]
    pub pattern: Vec<Regex>,
    /// Restricts test selection within the given path.
    #[arg(short, long, value_parser = |s: &str| PathBuf::from(s).canonicalize())]
    pub path: Vec<PathBuf>,
    /// Only selects the test that matches with the test name verbatim.
    #[arg(short, long)]
    pub exact: bool,
    /// Lists what tests will be run, without actually running them.
    #[arg(long, group = "action")]
    pub list: bool,
    /// Updates the reference images of non-passing tests.
    #[arg(short, long, group = "action")]
    pub update: bool,
    /// The scaling factor to render the output image with.
Repository: typst/typst
File: tests/src/tests.rs (Rust)

Code before:
    // Set up the thread pool.
    if let Some(num_threads) = ARGS.num_threads {
        rayon::ThreadPoolBuilder::new()
            .num_threads(num_threads)
            .build_global()
            .unwrap();
    }
}

fn test() {
    let (tests, skipped) = match crate::collect::collect() {
        Ok(output) => output,
        Err(errors) => {
            eprintln!("failed to collect tests");
            for error in errors {
                eprintln!("❌ {error}");
            }
            std::process::exit(1);
        }
    };

    if ARGS.list {
        for test in tests.iter() {
            println!("{test}");
        }
        println!("{} selected, {skipped} skipped", tests.len());
        return;
    }

    let selected = tests.len();
    if selected == 0 {
        eprintln!("no test selected");
        return;
    }

    // Run the tests.
    let logger = Mutex::new(Logger::new(selected, skipped));
    std::thread::scope(|scope| {
        let logger = &logger;
        let (sender, receiver) = std::sync::mpsc::channel();

        // Regularly refresh the logger in case we make no progress.
        scope.spawn(move || {
            while receiver.recv_timeout(Duration::from_millis(500)).is_err() {
                logger.lock().refresh();
            }
        });

        // Run the tests.
        //
        // We use `par_bridge` instead of `par_iter` because the former

Review comment (suggestion):
all other output is to stderr, so should this be eprintln? or maybe only the final line stderr, so that you could pipe stdout to get the test names.

Code after:
    // Set up the thread pool.
    if let Some(num_threads) = ARGS.num_threads {
        rayon::ThreadPoolBuilder::new()
            .num_threads(num_threads)
            .build_global()
            .unwrap();
    }
}

fn test() {
    let (tests, skipped) = match crate::collect::collect() {
        Ok(output) => output,
        Err(errors) => {
            eprintln!("failed to collect tests");
            for error in errors {
                eprintln!("❌ {error}");
            }
            std::process::exit(1);
        }
    };

    let selected = tests.len();
    if ARGS.list {
        for test in tests.iter() {
            println!("{test}");
        }
        eprintln!("{selected} selected, {skipped} skipped");
        return;
    } else if selected == 0 {
        eprintln!("no test selected");
        return;
    }

    // Run the tests.
    let logger = Mutex::new(Logger::new(selected, skipped));
    std::thread::scope(|scope| {
        let logger = &logger;
        let (sender, receiver) = std::sync::mpsc::channel();

        // Regularly refresh the logger in case we make no progress.
        scope.spawn(move || {
            while receiver.recv_timeout(Duration::from_millis(500)).is_err() {
                logger.lock().refresh();
            }
        });

        // Run the tests.
        //
        // We use `par_bridge` instead of `par_iter` because the former
        // results in a stack overflow during PDF export. Probably related
        // to `typst::util::Deferred` yielding.
Repository: typst/typst
File: crates/typst/src/foundations/array.rs (Rust)

Code before:
use std::cmp::Ordering;
use std::fmt::{Debug, Formatter};
use std::num::{NonZeroI64, NonZeroUsize};
use std::ops::{Add, AddAssign};

use comemo::Tracked;
use ecow::{eco_format, EcoString, EcoVec};
use serde::{Deserialize, Serialize};
use smallvec::SmallVec;
use typst_syntax::Spanned;

use crate::diag::{bail, At, SourceResult, StrResult};
use crate::engine::Engine;
use crate::eval::ops;
use crate::foundations::{
    cast, func, repr, scope, ty, Args, Bytes, CastInfo, Context, Dict, FromValue, Func,
    IntoValue, Reflect, Repr, Str, Value, Version,
};
use crate::syntax::Span;

/// Create a new [`Array`] from values.
#[macro_export]
#[doc(hidden)]
macro_rules! __array {
    ($value:expr; $count:expr) => {
        $crate::foundations::Array::from($crate::foundations::eco_vec![
            $crate::foundations::IntoValue::into_value($value);
            $count
        ])
    };

    ($($value:expr),* $(,)?) => {
        $crate::foundations::Array::from($crate::foundations::eco_vec![$(
            $crate::foundations::IntoValue::into_value($value)
        ),*])

Review comment (suggestion):
Please import as `crate::syntax::Spanned` instead.

Code after:
use std::cmp::Ordering;
use std::fmt::{Debug, Formatter};
use std::num::{NonZeroI64, NonZeroUsize};
use std::ops::{Add, AddAssign};

use comemo::Tracked;
use ecow::{eco_format, EcoString, EcoVec};
use serde::{Deserialize, Serialize};
use smallvec::SmallVec;

use crate::diag::{bail, At, SourceDiagnostic, SourceResult, StrResult};
use crate::engine::Engine;
use crate::eval::ops;
use crate::foundations::{
    cast, func, repr, scope, ty, Args, Bytes, CastInfo, Context, Dict, FromValue, Func,
    IntoValue, Reflect, Repr, Str, Value, Version,
};
use crate::syntax::{Span, Spanned};

/// Create a new [`Array`] from values.
#[macro_export]
#[doc(hidden)]
macro_rules! __array {
    ($value:expr; $count:expr) => {
        $crate::foundations::Array::from($crate::foundations::eco_vec![
            $crate::foundations::IntoValue::into_value($value);
            $count
        ])
    };

    ($($value:expr),* $(,)?) => {
        $crate::foundations::Array::from($crate::foundations::eco_vec![$(
            $crate::foundations::IntoValue::into_value($value)
        ),*])
    };
Repository: typst/typst
File: crates/typst/src/layout/place.rs (Rust)

Code before:
use typst_macros::scope;

use crate::diag::{bail, At, Hint, SourceResult};
use crate::engine::Engine;
use crate::foundations::{elem, Content, Packed, Smart, StyleChain, Unlabellable};
use crate::layout::{
    Alignment, Axes, Em, Fragment, LayoutMultiple, Length, Regions, Rel, Size, VAlignment,
};
use crate::realize::{Behave, Behaviour};

/// Places content at an absolute position.
///
/// Placed content will not affect the position of other content. Place is
/// always relative to its parent container and will be in the foreground of all
/// other content in the container. Page margins will be respected.
///
///
/// # Example
/// ```example
/// #set page(height: 60pt)
/// Hello, world!
///
/// #place(
///   top + right,
///   square(
///     width: 20pt,
///     stroke: 2pt + blue
///   ),
/// )
/// ```

Review comment (suggestion):
rust-analyzer always imports form `typst_macros`, but `scope` is supposed to be imported from `foundations` (where it is reexported)

Code after:
use crate::diag::{bail, At, Hint, SourceResult};
use crate::engine::Engine;
use crate::foundations::{elem, scope, Content, Packed, Smart, StyleChain, Unlabellable};
use crate::layout::{
    Alignment, Axes, Em, Fragment, LayoutMultiple, Length, Regions, Rel, Size, VAlignment,
};
use crate::realize::{Behave, Behaviour};

/// Places content at an absolute position.
///
/// Placed content will not affect the position of other content. Place is
/// always relative to its parent container and will be in the foreground of all
/// other content in the container. Page margins will be respected.
///
///
/// # Example
/// ```example
/// #set page(height: 60pt)
/// Hello, world!
///
/// #place(
///   top + right,
///   square(
///     width: 20pt,
///     stroke: 2pt + blue
///   ),
/// )
/// ```
#[elem(scope, Behave)]
pub struct PlaceElem {
Repository: typst/typst
File: crates/typst/src/layout/place.rs (Rust)

Code before:
impl Behave for Packed<PlaceElem> {
    fn behaviour(&self) -> Behaviour {
        Behaviour::Ignorant
    }
}

/// Asks the layout algorithm to place pending figures before
/// continuing with the content.
///
/// This is useful for preventing floating figures from spilling
/// into the next section.
///
/// ```example
/// #set page(height: 150pt, width: 150pt)
///
/// #let floater = place(auto, float: true, rect(width: 100%, height: 90pt, text(size: 24pt)[I float!]))
///
/// Some introductory text.
///
/// #floater #floater #floater #floater
///
/// Some additional text.
///
/// #place.flush()
///
/// Some conclusive text.
/// ```
#[elem(Behave, Unlabellable)]
pub struct FlushElem {}

impl Behave for Packed<FlushElem> {
    fn behaviour(&self) -> Behaviour {
        Behaviour::Invisible
    }
}

impl Unlabellable for Packed<FlushElem> {}


Review comment (suggestion):
The example is a bit long (and also wide, as in the number of columns) for the documentation. Could you try to find something shorter and nicer looking that demonstrates the purpose? Maybe also without a `floater` variable since that complicates things a bit.

Code after:
        Behaviour::Ignorant
    }
}

/// Asks the layout algorithm to place pending floating elements before
/// continuing with the content.
///
/// This is useful for preventing floating figures from spilling
/// into the next section.
///
/// ```example
/// #set page(height: 165pt, width: 150pt)
///
/// Some introductory text: #lorem(15)
///
/// #figure(
///   rect(
///     width: 100%,
///     height: 64pt,
///     [I float with a caption!],
///   ),
///   placement: auto,
///   caption: [A self-describing figure],
/// )
///
/// #place.flush()
///
/// Some conclusive text that must occur
/// after the figure.
/// ```
#[elem(Behave, Unlabellable)]
pub struct FlushElem {}

impl Behave for Packed<FlushElem> {
    fn behaviour(&self) -> Behaviour {
        Behaviour::Invisible
    }
}

impl Unlabellable for Packed<FlushElem> {}
Repository: typst/typst
File: crates/typst/src/layout/place.rs (Rust)

Code before:
        Ok(Fragment::frame(frame))
    }
}

impl Behave for Packed<PlaceElem> {
    fn behaviour(&self) -> Behaviour {
        Behaviour::Ignorant
    }
}

/// Asks the layout algorithm to place pending floating elements before
/// continuing with the content.
///
/// This is useful for preventing floating figures from spilling
/// into the next section.
///
/// ```example
/// #set page(height: 165pt, width: 150pt)
///
/// Some introductory text: #lorem(15)
///
/// #figure(placement: auto, caption: [A self-describing figure], rect(width: 100%, height: 64pt, [I float with a caption!]))
///
/// #place.flush()
///
/// Some conclusive text that must occur after the figure.
/// ```
#[elem(Behave, Unlabellable)]
pub struct FlushElem {}

impl Behave for Packed<FlushElem> {
    fn behaviour(&self) -> Behaviour {
        Behaviour::Invisible
    }
}

impl Unlabellable for Packed<FlushElem> {}


Review comment (suggestion):
The content is better, the number of columns is still very high. There will likely be a bad linebreak when rendered. I suggest something like this:
```suggestion
/// #set page(height: 165pt, width: 150pt)
///
/// Some introductory text: #lorem(15)
///
/// #figure(
///   rect(
///     width: 100%, 
///     height: 64pt, 
///     [I float with a caption!],
///   ),
///   placement: auto, 
///   caption: [A self-describing figure], 
/// )
///
/// #place.flush()
///
/// Some conclusive text that must occur 
/// after the figure.
```

Code after:
        Ok(Fragment::frame(frame))
    }
}

impl Behave for Packed<PlaceElem> {
    fn behaviour(&self) -> Behaviour {
        Behaviour::Ignorant
    }
}

/// Asks the layout algorithm to place pending floating elements before
/// continuing with the content.
///
/// This is useful for preventing floating figures from spilling
/// into the next section.
///
/// ```example
/// #set page(height: 165pt, width: 150pt)
///
/// Some introductory text: #lorem(15)
///
/// #figure(
///   rect(
///     width: 100%,
///     height: 64pt,
///     [I float with a caption!],
///   ),
///   placement: auto,
///   caption: [A self-describing figure],
/// )
///
/// #place.flush()
///
/// Some conclusive text that must occur
/// after the figure.
/// ```
#[elem(Behave, Unlabellable)]
pub struct FlushElem {}

impl Behave for Packed<FlushElem> {
    fn behaviour(&self) -> Behaviour {
        Behaviour::Invisible
    }
}

impl Unlabellable for Packed<FlushElem> {}
Repository: typst/typst
File: crates/typst-cli/src/args.rs (Rust)

Code before:
    pub dir: Option<String>,
}

/// Processes an input file to extract provided metadata
#[derive(Debug, Clone, Parser)]
pub struct QueryCommand {
    /// Shared arguments
    #[clap(flatten)]
    pub common: SharedArgs,

    /// Defines which elements to retrieve
    pub selector: String,

    /// Extracts just one field from all retrieved elements
    #[clap(long = "field")]
    pub field: Option<String>,

    /// Expects and retrieves exactly one element
    #[clap(long = "one", default_value = "false")]
    pub one: bool,

    /// The format to serialize in
    #[clap(long = "format", default_value = "json")]
    pub format: SerializationFormat,

    /// Pretty-print JSON
    #[clap(long)]
    pub pretty: bool,
}

// Output file format for query command
#[derive(Debug, Copy, Clone, Eq, PartialEq, ValueEnum)]
pub enum SerializationFormat {
    Json,
    Yaml,
}

/// Common arguments of compile, watch, and query.
#[derive(Debug, Clone, Args)]
pub struct SharedArgs {
    /// Path to input Typst file, use `-` to read input from stdin
    #[clap(value_parser = input_value_parser)]
    pub input: Input,

    /// Configures the project root (for absolute paths)
    #[clap(long = "root", env = "TYPST_ROOT", value_name = "DIR")]
    pub root: Option<PathBuf>,

    /// Add a string key-value pair visible through `sys.inputs`
    #[clap(
        long = "input",

Review comment (suggestion):
```suggestion
    /// Whether to pretty-print the serialized output
```
It could apply to potential future formats

Code after:
    pub dir: Option<String>,
}

/// Processes an input file to extract provided metadata
#[derive(Debug, Clone, Parser)]
pub struct QueryCommand {
    /// Shared arguments
    #[clap(flatten)]
    pub common: SharedArgs,

    /// Defines which elements to retrieve
    pub selector: String,

    /// Extracts just one field from all retrieved elements
    #[clap(long = "field")]
    pub field: Option<String>,

    /// Expects and retrieves exactly one element
    #[clap(long = "one", default_value = "false")]
    pub one: bool,

    /// The format to serialize in
    #[clap(long = "format", default_value = "json")]
    pub format: SerializationFormat,

    /// Whether to pretty-print the serialized output
    #[clap(long)]
    pub pretty: bool,
}

// Output file format for query command
#[derive(Debug, Copy, Clone, Eq, PartialEq, ValueEnum)]
pub enum SerializationFormat {
    Json,
    Yaml,
}

/// Common arguments of compile, watch, and query.
#[derive(Debug, Clone, Args)]
pub struct SharedArgs {
    /// Path to input Typst file, use `-` to read input from stdin
    #[clap(value_parser = input_value_parser)]
    pub input: Input,

    /// Configures the project root (for absolute paths)
    #[clap(long = "root", env = "TYPST_ROOT", value_name = "DIR")]
    pub root: Option<PathBuf>,

    /// Add a string key-value pair visible through `sys.inputs`
    #[clap(
        long = "input",
Repository: typst/typst
File: crates/typst/src/text/mod.rs (Rust)

Code before:
    ].into_value(),
    mut v: Dict => {
        let ret = Self {
            hyphenation: v.take("hyphenation").ok().map(|v| v.cast()).transpose()?,
            runt: v.take("runt").ok().map(|v| v.cast()).transpose()?,
            widow: v.take("widow").ok().map(|v| v.cast()).transpose()?,
            orphan: v.take("orphan").ok().map(|v| v.cast()).transpose()?,
        };
        v.finish(&["hyphenation", "runt", "widow", "orphan"])?;
        ret
    },
}

/// Function to parse the language argument.
/// Provides a hint if a region is used in the language field.
fn parse_lang(args: &mut Args) -> SourceResult<Option<Lang>> {
    let Some(Spanned { v: iso, span }) = args.named::<Spanned<EcoString>>("lang")? else {
        return Ok(None);
    };

    let result = Lang::from_str(&iso);
    if result.is_err() {
        if let Some((lang, region)) = iso.split_once('-') {
            if Lang::from_str(lang).is_ok() && Region::from_str(region).is_ok() {
                return result
                    .hint(eco_format!("you should use the `region` field for '{region}'"))
                    .at(span)
                    .map(Some);
            }
        }
    }
    result.at(span).map(Some)
}


Review comment (suggestion):
```suggestion
                    .hint(eco_format!("you should use the `region` parameter for '{region}'"))
```
The documentation typically refers to those as "parameters" I think.

Code after:
        let ret = Self {
            hyphenation: v.take("hyphenation").ok().map(|v| v.cast()).transpose()?,
            runt: v.take("runt").ok().map(|v| v.cast()).transpose()?,
            widow: v.take("widow").ok().map(|v| v.cast()).transpose()?,
            orphan: v.take("orphan").ok().map(|v| v.cast()).transpose()?,
        };
        v.finish(&["hyphenation", "runt", "widow", "orphan"])?;
        ret
    },
}

/// Function to parse the language argument.
/// Provides a hint if a region is used in the language parameter.
fn parse_lang(args: &mut Args) -> SourceResult<Option<Lang>> {
    let Some(Spanned { v: iso, span }) = args.named::<Spanned<EcoString>>("lang")? else {
        return Ok(None);
    };

    let result = Lang::from_str(&iso);
    if result.is_err() {
        if let Some((lang, region)) = iso.split_once('-') {
            if Lang::from_str(lang).is_ok() && Region::from_str(region).is_ok() {
                return result
                    .hint(eco_format!(
                        "you should leave only \"{}\" in the `lang` parameter and specify \"{}\" in the `region` parameter",
                        lang, region,
                    ))
                    .at(span)
                    .map(Some);
            }
        }
    }
    result.at(span).map(Some)
}
Repository: typst/typst
File: crates/typst/src/math/matrix.rs (Rust)

Code before:
            FixedAlignment::Start,
            self.gap(styles),
            LeftRightAlternator::None,
        )?;

        let (open, close) = if self.reverse(styles) {
            (None, delim.close())
        } else {
            (delim.open(), None)
        };

        layout_delimiters(ctx, styles, frame, open, close, self.span())
    }
}

#[derive(Debug, Default, Copy, Clone, Eq, PartialEq, Hash)]
struct Delimiter(Option<char>);

cast! {
    Delimiter,

    self => self.0.into_value(),

    _: NoneValue => Self::none(),
    v: Symbol => Self::char(v.get())?,
    v: Str => {
        let mut chars = v.chars();
        match (chars.next(), chars.next()) {
            (Some(c), None) => Self::char(c)?,
            _ => bail!("invalid delimiter: \"{}\"", v),
        }
    },
}

impl Delimiter {
    fn none() -> Self {
        Self(None)
    }

    fn char(c: char) -> StrResult<Self> {
        if !matches!(
            unicode_math_class::class(c),
            Some(MathClass::Opening | MathClass::Closing | MathClass::Fence),
        ) {
            bail!("invalid delimiter: \"{}\"", c)
        }
        Ok(Self(Some(c)))
    }

    fn get(self) -> Option<char> {
        self.0

Review comment (suggestion):
I think you can also directly use `v: char` to get an error message for `chars().count() > 1` for free.

Code after:
            FixedAlignment::Start,
            self.gap(styles),
            LeftRightAlternator::None,
        )?;

        let (open, close) = if self.reverse(styles) {
            (None, delim.close())
        } else {
            (delim.open(), None)
        };

        layout_delimiters(ctx, styles, frame, open, close, self.span())
    }
}

/// A delimiter is a single character that is used to delimit a matrix, vector
/// or cases. The character has to be a Unicode codepoint tagged as a math
/// "opening", "closing" or "fence".
///
/// Typically, the delimiter is stretched to fit the height of whatever it
/// delimits.
#[derive(Debug, Default, Copy, Clone, Eq, PartialEq, Hash)]
struct Delimiter(Option<char>);

cast! {
    Delimiter,
    self => self.0.into_value(),
    _: NoneValue => Self::none(),
    v: Symbol => Self::char(v.get())?,
    v: char => Self::char(v)?,
}

impl Delimiter {
    fn none() -> Self {
        Self(None)
    }

    fn char(c: char) -> StrResult<Self> {
        if !matches!(
            unicode_math_class::class(c),
            Some(MathClass::Opening | MathClass::Closing | MathClass::Fence),
        ) {
            bail!("invalid delimiter: \"{}\"", c)
        }
        Ok(Self(Some(c)))
    }

    fn get(self) -> Option<char> {
        self.0
    }