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

Code before:
use crate::foundations::{elem, scope, Cast, Content, Smart};
use crate::layout::{Alignment, Em, Length, Rel};

/// Places content relatively to its parent container.
///
/// Placed content can be either overlaid (the default) or floating. Overlaid
/// content is aligned with the parent container according to the given
/// [`alignment`]($place.alignment), and shown over any other content added so
/// far in the container. Floating content is placed at the top or bottom of
/// the container, displacing other content down or up respectively. In both
/// cases, the content position can be adjusted with [`dx`]($place.dx) and
/// [`dy`]($place.dy) offsets without affecting the layout.
///
/// # Effect on the position of other elements
///
/// Overlaid elements don't take space in the flow of content, but a `place`
/// call inserts an invisible block-level element in the flow. This can
/// affect the layout by breaking the current paragraph. To avoid this,
/// you can wrap the `place` call in a [`box`]($box) when the call is made
/// in the middle of a paragraph.
///
///
/// # Example
/// ```example
/// #set page(height: 60pt)
/// Hello, world!
///
/// #place(
///   top + right,

Review comment (suggestion):
It'd be nice to mention the "absolute position" terminology somewhere below. I agree it's not fully correct, but it's what people are used to read, so it's usually easier to associate with behavior from elsewhere. Maybe a short paragraph or sentence somewhere mentioning that "in the absence of a parent container, the content will be placed absolutely, that is, relative to the current page" could help.

Code after:
use crate::foundations::{elem, scope, Cast, Content, Smart};
use crate::layout::{Alignment, Em, Length, Rel};

/// Places content relatively to its parent container.
///
/// Placed content can be either overlaid (the default) or floating. Overlaid
/// content is aligned with the parent container according to the given
/// [`alignment`]($place.alignment), and shown over any other content added so
/// far in the container. Floating content is placed at the top or bottom of
/// the container, displacing other content down or up respectively. In both
/// cases, the content position can be adjusted with [`dx`]($place.dx) and
/// [`dy`]($place.dy) offsets without affecting the layout.
///
/// The parent can be any container such as a [`block`], [`box`],
/// [`rect`], etc. A top level `place` call will place content directly
/// in the text area of the current page. This can be used for absolute
/// positioning on the page: with a `top + left`
/// [`alignment`]($place.alignment), the offsets `dx` and `dy` will set the
/// position of the element's top left corner relatively to the top left corner
/// of the text area. For absolute positioning on the full page including
/// margins, you can use `place` in [`page.foreground`]($page.foreground) or
/// [`page.background`]($page.background).
///
/// # Examples
/// ```example
/// #set page(height: 60pt)
/// Hello, world!
///
/// #rect(
Repository: typst/typst
File: crates/typst/src/foundations/scope.rs (Rust)

Code before:
            if var.matches('-').count() > 1 { "s" } else { "" },
            var.replace('-', " - ")
        ));
    }

    res
}

#[cold]
fn unknown_variable_math(var: &str, in_global: bool) -> HintedString {
    let mut res = HintedString::new(eco_format!("unknown variable: {}", var));

    if matches!(var, "none" | "auto" | "false" | "true") {
        res.hint(eco_format!(
            "if you meant to use a literal, try adding a hash before it: `#{var}`",
        ));
    } else if in_global {
        res.hint(eco_format!(
            "`{var}` is not available directly in math, try adding a hash before it: `#{var}`",
        ));
    } else {
        res.hint(eco_format!(
            "if you meant to display multiple letters as is, try adding spaces between each letter: `{}`",
            var.chars()
                .flat_map(|c| std::iter::once(' ').chain(std::iter::once(c)))
                .skip(1)
                .collect::<EcoString>()
        ));
        res.hint(eco_format!(
            "or if you meant to display this as text, try placing it in quotes: `\"{var}\"`"
        ));
    }

    res
}

/// A map from binding names to values.
#[derive(Default, Clone)]
pub struct Scope {
    map: IndexMap<EcoString, Slot>,
    deduplicate: bool,
    category: Option<Category>,
}

impl Scope {
    /// Create a new empty scope.
    pub fn new() -> Self {
        Default::default()
    }

    /// Create a new scope with duplication prevention.

Review comment (suggestion):
intersperse stable when :/
```suggestion
                .flat_map(|c| [' ', c]))
                .skip(1)
```

Code after:
            if var.matches('-').count() > 1 { "s" } else { "" },
            var.replace('-', " - ")
        ));
    }

    res
}

#[cold]
fn unknown_variable_math(var: &str, in_global: bool) -> HintedString {
    let mut res = HintedString::new(eco_format!("unknown variable: {}", var));

    if matches!(var, "none" | "auto" | "false" | "true") {
        res.hint(eco_format!(
            "if you meant to use a literal, try adding a hash before it: `#{var}`",
        ));
    } else if in_global {
        res.hint(eco_format!(
            "`{var}` is not available directly in math, try adding a hash before it: `#{var}`",
        ));
    } else {
        res.hint(eco_format!(
            "if you meant to display multiple letters as is, try adding spaces between each letter: `{}`",
            var.chars()
                .flat_map(|c| [' ', c])
                .skip(1)
                .collect::<EcoString>()
        ));
        res.hint(eco_format!(
            "or if you meant to display this as text, try placing it in quotes: `\"{var}\"`"
        ));
    }

    res
}

/// A map from binding names to values.
#[derive(Default, Clone)]
pub struct Scope {
    map: IndexMap<EcoString, Slot>,
    deduplicate: bool,
    category: Option<Category>,
}

impl Scope {
    /// Create a new empty scope.
    pub fn new() -> Self {
        Default::default()
    }

    /// Create a new scope with duplication prevention.
Repository: typst/typst
File: crates/typst-syntax/src/set.rs (Rust)

Code before:
    /// Combine two syntax sets.
    pub const fn union(self, other: Self) -> Self {
        Self(self.0 | other.0)
    }

    /// Whether the set contains the given syntax kind.
    pub const fn contains(&self, kind: SyntaxKind) -> bool {
        (kind as u8) < BITS && (self.0 & bit(kind)) != 0
    }
}

const BITS: u8 = 128;

const fn bit(kind: SyntaxKind) -> u128 {
    1 << (kind as usize)
}

/// Generate an inline const syntax set of the given kinds.
///
/// Example: `if syntax_set!(Plus, Minus).contains(SyntaxKind::Plus) { ... }`
/// Becomes:
/// ```rust
/// if {const SET:SyntaxSet = SyntaxSet::new()
///     .add(SyntaxKind::Plus)
///     .add(SyntaxKind::Minus)
/// }.contains(SyntaxKind::Plus) { ... }
/// ```
macro_rules! syntax_set {
    ( $($kind:ident),* $(,)? ) => {
        { // 'SET' never escapes this inner block
            const SET: crate::set::SyntaxSet = crate::set::SyntaxSet::new()
                $(.add(crate::SyntaxKind:: $kind))*;
            SET
        }
    }
}

// Export so other modules can import as: `use set::syntax_set`
pub(crate) use syntax_set;

/// Just build a syntax set of the given kinds. Only used in this file.
///
/// Example: `const SET: SyntaxSet = make_set!(Plus, Minus);`
/// Becomes:
/// ```rust
/// SyntaxSet::new()
///     .add(SyntaxKind::Plus)
///     .add(SyntaxKind::Minus)
/// ```
macro_rules! make_set {
    ( $($kind:ident),* $(,)? ) => {

Review comment (suggestion):
I believe `SET` should be returned here within the block expression.

Code after:
    /// Combine two syntax sets.
    pub const fn union(self, other: Self) -> Self {
        Self(self.0 | other.0)
    }

    /// Whether the set contains the given syntax kind.
    pub const fn contains(&self, kind: SyntaxKind) -> bool {
        (kind as u8) < BITS && (self.0 & bit(kind)) != 0
    }
}

const BITS: u8 = 128;

const fn bit(kind: SyntaxKind) -> u128 {
    1 << (kind as usize)
}

/// Generate a compile-time constant `SyntaxSet` of the given kinds.
macro_rules! syntax_set {
    ($($kind:ident),* $(,)?) => {{
        const SET: crate::set::SyntaxSet = crate::set::SyntaxSet::new()
            $(.add(crate::SyntaxKind:: $kind))*;
        SET
    }}
}

// Export so other modules can import as: `use set::syntax_set`
pub(crate) use syntax_set;

/// Syntax kinds that can start a statement.
pub const STMT: SyntaxSet = syntax_set!(Let, Set, Show, Import, Include, Return);

/// Syntax kinds that can start a math expression.
pub const MATH_EXPR: SyntaxSet = syntax_set!(
    Hash,
    MathIdent,
    Text,
    MathShorthand,
    Linebreak,
    MathAlignPoint,
    Escape,
    Str,
    Root,
    Prime,
);

/// Syntax kinds that can start a code expression.
pub const CODE_EXPR: SyntaxSet = CODE_PRIMARY.union(UNARY_OP);

/// Syntax kinds that can start an atomic code expression.
pub const ATOMIC_CODE_EXPR: SyntaxSet = ATOMIC_CODE_PRIMARY;
Repository: typst/typst
File: crates/typst-library/src/pdf/embed.rs (Rust)

Code before:
use crate::diag::{At, SourceResult, StrResult};
use crate::engine::Engine;
use crate::foundations::{func, Bytes, Content, NativeElement, Packed, Show, StyleChain};
use crate::introspection::Locatable;
use crate::loading::Readable;
use crate::text::LocalName;
use crate::{Span, World};
use ecow::EcoString;
use std::sync::Arc;
use typst_macros::{elem, scope, Cast};
use typst_syntax::Spanned;
use typst_utils::LazyHash;

#[elem(scope, Show, LocalName, Locatable)]
pub struct EmbedElem {
    /// Path to a file to be embedded
    ///
    /// For more details, see the [Paths section]($syntax/#paths).
    #[required]
    #[parse(
        let Spanned { v: path, span } =
        args.expect::<Spanned<EcoString>>("path to the file to be embedded")?;
        let id = span.resolve_path(&path).at(span)?;
        let data = engine.world.file(id).at(span)?;
        path
    )]
    #[borrowed]
    pub path: EcoString,

    /// The raw file data.
    #[internal]
    #[required]
    #[parse(Readable::Bytes(data))]
    pub data: Readable,

    /// The name of the attached file
    ///
    /// If no name is given, the path is used instead
    #[borrowed]
    pub name: Option<EcoString>,


Review comment (suggestion):
```suggestion
    /// Path to a file to be embedded.
```
Please end all comments with a full stop.

Code after:
use ecow::EcoString;
use typst_syntax::{Span, Spanned};

use crate::diag::{At, SourceResult, StrResult};
use crate::engine::Engine;
use crate::foundations::{
    elem, func, scope, Cast, Content, NativeElement, Packed, Show, StyleChain,
};
use crate::introspection::Locatable;
use crate::loading::Readable;
use crate::World;

/// A file that will be embedded into the output PDF.
///
/// This can be used to distribute additional files that are related to the PDF
/// within it. PDF readers will display the files in a file listing.
///
/// Some international standards use this mechanism to embed machine-readable
/// data (e.g., ZUGFeRD/Factur-X for invoices) that mirrors the visual content
/// of the PDF.
///
/// # Example
/// ```typ
/// #pdf.embed(
///   "experiment.csv",
///   relationship: "supplement",
///   mime-type: "text/csv",
///   description: "Raw Oxygen readings from the Arctic experiment",
/// )
/// ```
///
/// # Notes
/// - This element is ignored if exporting to a format other than PDF.
/// - File embeddings are not currently supported for PDF/A-2, even if the
///   embedded file conforms to PDF/A-1 or PDF/A-2.
#[elem(scope, Show, Locatable)]
pub struct EmbedElem {
    /// Path to a file to be embedded.
    ///
    /// For more details, see the [Paths section]($syntax/#paths).
    #[required]
Repository: typst/typst
File: crates/typst/src/model/par.rs (Rust)

Code before:
    /// )
    ///
    /// First line \
    /// Second line
    /// #pagebreak()
    /// First line again \
    /// Second line again
    /// ```
    #[ghost]
    #[default(LineNumberingScope::Document)]
    pub numbering_scope: LineNumberingScope,
}

impl Construct for ParLine {
    fn construct(_: &mut Engine, args: &mut Args) -> SourceResult<Content> {
        bail!(args.span, "cannot be constructed manually");
    }
}

/// Possible line numbering scope options, indicating how often the line number
/// counter should be reset.
#[derive(Debug, Cast, Clone, Copy, PartialEq, Eq, Hash)]
pub enum LineNumberingScope {
    /// Indicates that the line number counter spans the whole document, i.e.,
    /// it's never automatically reset. Currently, the manual reset of
    /// the counter is not supported.
    Document,
    /// Indicates that the line number counter should be reset at the start of
    /// every new page.
    Page,
}

/// A marker used to indicate the presence of a line.
///
/// This element is added to each line in a paragraph and later searched to
/// find out where to add line numbers.
#[elem(Construct, Locatable, Count)]
pub struct ParLineMarker {
    #[internal]
    #[required]
    pub numbering: Numbering,

    #[internal]
    #[required]
    pub number_align: Smart<HAlignment>,

    #[internal]
    #[required]
    pub number_margin: OuterHAlignment,

    #[internal]

Review comment (suggestion):
I feel like the last sentence should go on the type's own documentation.

Code after:
    /// )
    ///
    /// First line \
    /// Second line
    /// #pagebreak()
    /// First line again \
    /// Second line again
    /// ```
    #[ghost]
    #[default(LineNumberingScope::Document)]
    pub numbering_scope: LineNumberingScope,
}

impl Construct for ParLine {
    fn construct(_: &mut Engine, args: &mut Args) -> SourceResult<Content> {
        bail!(args.span, "cannot be constructed manually");
    }
}

/// Possible line numbering scope options, indicating how often the line number
/// counter should be reset.
///
/// Note that, currently, manually resetting the line number counter is not
/// supported.
#[derive(Debug, Cast, Clone, Copy, PartialEq, Eq, Hash)]
pub enum LineNumberingScope {
    /// Indicates that the line number counter spans the whole document, i.e.,
    /// it's never automatically reset.
    Document,
    /// Indicates that the line number counter should be reset at the start of
    /// every new page.
    Page,
}

/// A marker used to indicate the presence of a line.
///
/// This element is added to each line in a paragraph and later searched to
/// find out where to add line numbers.
#[elem(Construct, Locatable, Count)]
pub struct ParLineMarker {
    #[internal]
    #[required]
    pub numbering: Numbering,

    #[internal]
    #[required]
    pub number_align: Smart<HAlignment>,

    #[internal]
    #[required]
    pub number_margin: OuterHAlignment,
Repository: typst/typst
File: crates/typst/src/model/par.rs (Rust)

Code before:
    ///   numbering: "1",
    ///   numbering-scope: "page",
    /// )
    ///
    /// First line \
    /// Second line
    /// #pagebreak()
    /// First line again \
    /// Second line again
    /// ```
    #[ghost]
    #[default(LineNumberingScope::Document)]
    pub numbering_scope: LineNumberingScope,
}

impl Construct for ParLine {
    fn construct(_: &mut Engine, args: &mut Args) -> SourceResult<Content> {
        bail!(args.span, "cannot be constructed manually");
    }
}

/// Possible line numbering scope options, indicating how often the line number
/// counter should be reset.
///
/// Note that currently the manual reset of the line number counter is not
/// supported.
#[derive(Debug, Cast, Clone, Copy, PartialEq, Eq, Hash)]
pub enum LineNumberingScope {
    /// Indicates that the line number counter spans the whole document, i.e.,
    /// it's never automatically reset.
    Document,
    /// Indicates that the line number counter should be reset at the start of
    /// every new page.
    Page,
}

/// A marker used to indicate the presence of a line.
///
/// This element is added to each line in a paragraph and later searched to
/// find out where to add line numbers.
#[elem(Construct, Locatable, Count)]
pub struct ParLineMarker {
    #[internal]
    #[required]
    pub numbering: Numbering,

    #[internal]
    #[required]
    pub number_align: Smart<HAlignment>,

    #[internal]

Review comment (suggestion):
Perhaps a bit clearer:

```suggestion
///
/// Note that, currently, manually resetting the line number counter at
/// arbitrary points of your document is not supported.
```

Code after:
    ///   numbering: "1",
    ///   numbering-scope: "page",
    /// )
    ///
    /// First line \
    /// Second line
    /// #pagebreak()
    /// First line again \
    /// Second line again
    /// ```
    #[ghost]
    #[default(LineNumberingScope::Document)]
    pub numbering_scope: LineNumberingScope,
}

impl Construct for ParLine {
    fn construct(_: &mut Engine, args: &mut Args) -> SourceResult<Content> {
        bail!(args.span, "cannot be constructed manually");
    }
}

/// Possible line numbering scope options, indicating how often the line number
/// counter should be reset.
///
/// Note that, currently, manually resetting the line number counter is not
/// supported.
#[derive(Debug, Cast, Clone, Copy, PartialEq, Eq, Hash)]
pub enum LineNumberingScope {
    /// Indicates that the line number counter spans the whole document, i.e.,
    /// it's never automatically reset.
    Document,
    /// Indicates that the line number counter should be reset at the start of
    /// every new page.
    Page,
}

/// A marker used to indicate the presence of a line.
///
/// This element is added to each line in a paragraph and later searched to
/// find out where to add line numbers.
#[elem(Construct, Locatable, Count)]
pub struct ParLineMarker {
    #[internal]
    #[required]
    pub numbering: Numbering,

    #[internal]
    #[required]
    pub number_align: Smart<HAlignment>,

    #[internal]
Repository: typst/typst
File: crates/typst-library/src/text/mod.rs (Rust)

Code before:
        eco_format!("[{}]", self.text)
    }
}

impl Construct for TextElem {
    fn construct(engine: &mut Engine, args: &mut Args) -> SourceResult<Content> {
        // The text constructor is special: It doesn't create a text element.
        // Instead, it leaves the passed argument structurally unchanged, but
        // styles all text in it.
        let styles = Self::set(engine, args)?;
        let body = args.expect::<Content>("body")?;
        Ok(body.styled_with_map(styles))
    }
}

impl PlainText for Packed<TextElem> {
    fn plain_text(&self, text: &mut EcoString) {
        text.push_str(self.text());
    }
}

/// A lowercased font family like "arial".
#[derive(Debug, Clone)]
pub struct FontFamily {
    name: EcoString,
    coverage: Option<Regex>,
}

impl Hash for FontFamily {
    fn hash<H: Hasher>(&self, state: &mut H) {
        self.name.hash(state);
        self.coverage_str().hash(state);
    }
}

impl PartialEq for FontFamily {
    fn eq(&self, other: &Self) -> bool {
        self.name == other.name && self.coverage_str() == other.coverage_str()
    }
}

impl FontFamily {
    /// Create a named font family variant.
    pub fn new(string: &str) -> Self {
        Self::new_with_coverage(string, None).unwrap()
    }

    pub fn new_with_coverage(
        string: &str,
        coverage: Option<Value>,
    ) -> HintedStrResult<Self> {

Review comment (suggestion):
```suggestion
    covers: Option<Regex>,
```
Brief docs on the fields would be nice.

Code after:
    fn fmt(&self, f: &mut Formatter) -> fmt::Result {
        write!(f, "Text({})", self.text)
    }
}

impl Repr for TextElem {
    fn repr(&self) -> EcoString {
        eco_format!("[{}]", self.text)
    }
}

impl Construct for TextElem {
    fn construct(engine: &mut Engine, args: &mut Args) -> SourceResult<Content> {
        // The text constructor is special: It doesn't create a text element.
        // Instead, it leaves the passed argument structurally unchanged, but
        // styles all text in it.
        let styles = Self::set(engine, args)?;
        let body = args.expect::<Content>("body")?;
        Ok(body.styled_with_map(styles))
    }
}

impl PlainText for Packed<TextElem> {
    fn plain_text(&self, text: &mut EcoString) {
        text.push_str(self.text());
    }
}

/// A lowercased font family like "arial".
#[derive(Debug, Clone, PartialEq, Hash)]
pub struct FontFamily {
    // The name of the font family
    name: EcoString,
    // A regex that defines the Unicode codepoints supported by the font.
    covers: Option<Covers>,
}

impl FontFamily {
    /// Create a named font family variant.
    pub fn new(string: &str) -> Self {
        Self::with_coverage(string, None)
    }

    /// Create a font family by name and optional Unicode coverage.
    pub fn with_coverage(string: &str, covers: Option<Covers>) -> Self {
        Self { name: string.to_lowercase().into(), covers }
    }

    /// The lowercased family name.
    pub fn as_str(&self) -> &str {
        &self.name
Repository: typst/typst
File: crates/typst-cli/src/fonts.rs (Rust)

Code before:
use crate::args::FontsCommand;
use std::io::Write;
use typst::text::FontVariant;
use typst_kit::fonts::Fonts;
/// Execute a font listing command.
pub fn fonts(command: &FontsCommand) {
    let fonts = Fonts::searcher()
        .include_system_fonts(!command.font_args.ignore_system_fonts)
        .search_with(&command.font_args.font_paths);
    let mut out = crate::terminal::out();
    for (name, infos) in fonts.book.families() {
        writeln!(out, "{name}:").unwrap();
        if command.variants {
            for info in infos {
                let FontVariant { style, weight, stretch } = info.variant;
                writeln!(
                    out,
                    "- Style: {style:?}, Weight: {weight:?}, Stretch: {stretch:?}"
                )
                .unwrap();
            }
        }
    }
}


Review comment (bug):
I think unwrapping here might generate the same problem, but either way we should at least make `fonts` fallible and return an IO error instead of crashing.

Code after:
use typst::text::FontVariant;
use typst_kit::fonts::Fonts;

use crate::args::FontsCommand;

/// Execute a font listing command.
pub fn fonts(command: &FontsCommand) {
    let fonts = Fonts::searcher()
        .include_system_fonts(!command.font_args.ignore_system_fonts)
        .search_with(&command.font_args.font_paths);

    for (name, infos) in fonts.book.families() {
        println!("{name}");
        if command.variants {
            for info in infos {
                let FontVariant { style, weight, stretch } = info.variant;
                println!("- Style: {style:?}, Weight: {weight:?}, Stretch: {stretch:?}");
            }
        }
    }
}