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

Code before:
        /// An optional message to display on error instead of the representations
        /// of the compared values.
        #[named]
        message: Option<EcoString>,
    ) -> StrResult<NoneValue> {
        if left == right {
            if let Some(message) = message {
                bail!("inequality assertion failed: {message}");
            } else {
                bail!(
                    "inequality assertion failed: value {} was equal to {}",
                    left.repr(),
                    right.repr()
                );
            }
        }
        Ok(NoneValue)
    }
}

cast! {
    SyntaxMode,
    self => match self {
        SyntaxMode::Markup => "markup".into_value(),
        SyntaxMode::Math => "math".into_value(),
        SyntaxMode::Code => "code".into_value(),
    },
    "markup" => SyntaxMode::Markup,
    "math" => SyntaxMode::Math,
    "code" => SyntaxMode::Code,
}

/// Evaluates a string as Typst code.
///
/// This function should only be used as a last resort.
///
/// # Example
/// ```example
/// #eval("1 + 1") \
/// #eval("(1, 2, 3, 4)").len() \
/// #eval("*Markup!*", mode: "markup") \
/// ```
#[func(title = "Evaluate")]
pub fn eval(
    engine: &mut Engine,
    /// A string of Typst code to evaluate.
    source: Spanned<String>,
    /// The [syntactical mode]($reference/syntax/#modes) in which the string is
    /// parsed.
    ///
    /// ```example

Review comment (refactor):
These into_value calls could be deduplicated similarly as in the cast for MathClass.

Code after:
        /// An optional message to display on error instead of the representations
        /// of the compared values.
        #[named]
        message: Option<EcoString>,
    ) -> StrResult<NoneValue> {
        if left == right {
            if let Some(message) = message {
                bail!("inequality assertion failed: {message}");
            } else {
                bail!(
                    "inequality assertion failed: value {} was equal to {}",
                    left.repr(),
                    right.repr()
                );
            }
        }
        Ok(NoneValue)
    }
}

/// Evaluates a string as Typst code.
///
/// This function should only be used as a last resort.
///
/// # Example
/// ```example
/// #eval("1 + 1") \
/// #eval("(1, 2, 3, 4)").len() \
/// #eval("*Markup!*", mode: "markup") \
/// ```
#[func(title = "Evaluate")]
pub fn eval(
    engine: &mut Engine,
    /// A string of Typst code to evaluate.
    source: Spanned<String>,
    /// The [syntactical mode]($reference/syntax/#modes) in which the string is
    /// parsed.
    ///
    /// ```example
    /// #eval("= Heading", mode: "markup")
    /// #eval("1_2^3", mode: "math")
    /// ```
    #[named]
    #[default(SyntaxMode::Code)]
    mode: SyntaxMode,
    /// A scope of definitions that are made available.
    ///
    /// ```example
    /// #eval("x + 1", scope: (x: 2)) \
    /// #eval(
    ///   "abc/xyz",
Repository: typst/typst
File: crates/typst-library/src/math/accent.rs (Rust)

Code before:
    '\u{0302}' | '^' | 'ˆ' => hat,
    '\u{0303}' | '~' | '∼' | '˜' => tilde,
    '\u{0304}' | '¯' => macron,
    '\u{0305}' | '-' | '‾' | '−' => dash,
    '\u{0306}' | '˘' => breve,
    '\u{0307}' | '.' | '˙' | '⋅' => dot,
    '\u{0308}' | '¨' => dot_double,
    '\u{20db}' => dot_triple,
    '\u{20dc}' => dot_quad,
    '\u{030a}' | '∘' | '○' => circle,
    '\u{030b}' | '˝' => acute_double,
    '\u{030c}' | 'ˇ' => caron,
    '\u{20d6}' | '←' => arrow_l,
    '\u{20d7}' | '→' | '⟶' => arrow,
    '\u{20e1}' | '↔' | '⟷' => arrow_l_r,
    '\u{20d0}' | '↼' => harpoon_lt,
    '\u{20d1}' | '⇀' => harpoon,
}

cast! {
    Accent,
    self => self.0.into_value(),
    v: char => Self::new(v),
    v: Content => match v.to_packed::<SymbolElem>() {
        Some(elem) if elem.text.chars().count() == 1 => Self::new(elem.text.chars().next().unwrap()),
        _ => bail!("expected a single-codepoint symbol"),
    },
}


Review comment (suggestion):
this could use the same `parse` approach as in `Delimiter`, right? I didn't know about `FromStr for char`, that's neat!

Code after:
    '\u{0302}' | '^' | 'ˆ' => hat,
    '\u{0303}' | '~' | '∼' | '˜' => tilde,
    '\u{0304}' | '¯' => macron,
    '\u{0305}' | '-' | '‾' | '−' => dash,
    '\u{0306}' | '˘' => breve,
    '\u{0307}' | '.' | '˙' | '⋅' => dot,
    '\u{0308}' | '¨' => dot_double,
    '\u{20db}' => dot_triple,
    '\u{20dc}' => dot_quad,
    '\u{030a}' | '∘' | '○' => circle,
    '\u{030b}' | '˝' => acute_double,
    '\u{030c}' | 'ˇ' => caron,
    '\u{20d6}' | '←' => arrow_l,
    '\u{20d7}' | '→' | '⟶' => arrow,
    '\u{20e1}' | '↔' | '⟷' => arrow_l_r,
    '\u{20d0}' | '↼' => harpoon_lt,
    '\u{20d1}' | '⇀' => harpoon,
}

cast! {
    Accent,
    self => self.0.into_value(),
    v: char => Self::new(v),
    v: Content => match v.to_packed::<SymbolElem>().and_then(|elem| elem.text.parse::<char>().ok()) {
        Some(c) => Self::new(c),
        _ => bail!("expected a single-codepoint symbol"),
    },
}
Repository: typst/typst
File: crates/typst-layout/src/math/fragment.rs (Rust)

Code before:
    assembly: GlyphAssembly<'_>,
    repeat: usize,
) -> impl Iterator<Item = GlyphPart> + '_ {
    assembly.parts.into_iter().flat_map(move |part| {
        let count = if part.part_flags.extender() { repeat } else { 1 };
        std::iter::repeat_n(part, count)
    })
}

pub fn has_dtls_feat(font: &Font) -> bool {
    font.ttf()
        .tables()
        .gsub
        .and_then(|gsub| gsub.features.index(ttf_parser::Tag::from_bytes(b"dtls")))
        .is_some()
}

#[comemo::memoize]
fn shape(
    world: Tracked<dyn World + '_>,
    variant: FontVariant,
    features: Vec<rustybuzz::Feature>,
    language: rustybuzz::Language,
    fallback: bool,
    text: &str,
    span: Span,
    families: Vec<&FontFamily>,
) -> SourceResult<Option<(char, Font, Glyph)>> {
    let mut used = vec![];
    let buffer = UnicodeBuffer::new();
    shape_glyph(
        world,
        &mut used,
        buffer,
        variant,
        features,
        language,
        fallback,
        text,
        span,
        families.into_iter(),
    )
}

#[allow(clippy::too_many_arguments)]
fn shape_glyph<'a>(
    world: Tracked<'a, dyn World + 'a>,
    used: &mut Vec<Font>,
    mut buffer: rustybuzz::UnicodeBuffer,
    variant: FontVariant,
    features: Vec<rustybuzz::Feature>,

Review comment (suggestion):
IIRC there was intentionally no span here because it hurts memoization. It would be better to return a StrResult and attach the span outside of the memoization boundary with `at`

Code after:
    assembly: GlyphAssembly<'_>,
    repeat: usize,
) -> impl Iterator<Item = GlyphPart> + '_ {
    assembly.parts.into_iter().flat_map(move |part| {
        let count = if part.part_flags.extender() { repeat } else { 1 };
        std::iter::repeat_n(part, count)
    })
}

pub fn has_dtls_feat(font: &Font) -> bool {
    font.ttf()
        .tables()
        .gsub
        .and_then(|gsub| gsub.features.index(ttf_parser::Tag::from_bytes(b"dtls")))
        .is_some()
}

#[comemo::memoize]
fn shape(
    world: Tracked<dyn World + '_>,
    variant: FontVariant,
    features: Vec<rustybuzz::Feature>,
    language: rustybuzz::Language,
    fallback: bool,
    text: &str,
    families: Vec<&FontFamily>,
) -> HintedStrResult<Option<(char, Font, Glyph)>> {
    let mut used = vec![];
    let buffer = UnicodeBuffer::new();
    shape_glyph(
        world,
        &mut used,
        buffer,
        variant,
        features,
        language,
        fallback,
        text,
        families.into_iter(),
    )
}

#[allow(clippy::too_many_arguments)]
fn shape_glyph<'a>(
    world: Tracked<'a, dyn World + 'a>,
    used: &mut Vec<Font>,
    mut buffer: rustybuzz::UnicodeBuffer,
    variant: FontVariant,
    features: Vec<rustybuzz::Feature>,
    language: rustybuzz::Language,
    fallback: bool,
Repository: typst/typst
File: crates/typst-ide/src/complete.rs (Rust)

Code before:

    #[test]
    fn test_autocomplete_in_dict_literal() {
        let document = "#let first = 0\n#(first: 1, second: one)";

        // After opening paren
        test(document, 17).must_be_empty();
        test_implicit(document, 17).must_be_empty();

        // After first key
        test(document, 22).must_be_empty();
        test_implicit(document, 22).must_be_empty();

        // After colon
        test(document, 23).must_include(["align", "first"]);
        test_implicit(document, 23).must_be_empty();

        test(document, 24).must_include(["align", "first"]);
        test_implicit(document, 24).must_be_empty();

        // After first value
        test(document, 25).must_be_empty();
        test_implicit(document, 25).must_be_empty();

        // After comma
        test(document, 26).must_be_empty();
        test_implicit(document, 26).must_be_empty();

        test(document, 27).must_be_empty();
        test_implicit(document, 27).must_be_empty();
    }
}


Review comment (suggestion):
Let's also add tests for destructuring: `#let value = 20\n#let (va|: value) = (va: 10)`, as this has also changed.

Code after:
        test_implicit(document, 18).must_include(["font"]);
    }

    #[test]
    fn test_autocomplete_in_list_literal() {
        let document = "#let val = 0\n#(1, \"one\")";

        // After opening paren
        test(document, 15).must_include(["color", "val"]);
        test_implicit(document, 15).must_be_empty();

        // After first element
        test(document, 16).must_be_empty();
        test_implicit(document, 16).must_be_empty();

        // After comma
        test(document, 17).must_include(["color", "val"]);
        test_implicit(document, 17).must_be_empty();

        test(document, 18).must_include(["color", "val"]);
        test_implicit(document, 18).must_be_empty();
    }

    #[test]
    fn test_autocomplete_in_dict_literal() {
        let document = "#let first = 0\n#(first: 1, second: one)";

        // After opening paren
        test(document, 17).must_be_empty();
        test_implicit(document, 17).must_be_empty();

        // After first key
        test(document, 22).must_be_empty();
        test_implicit(document, 22).must_be_empty();

        // After colon
        test(document, 23).must_include(["align", "first"]);
        test_implicit(document, 23).must_be_empty();

        test(document, 24).must_include(["align", "first"]);
        test_implicit(document, 24).must_be_empty();

        // After first value
        test(document, 25).must_be_empty();
        test_implicit(document, 25).must_be_empty();

        // After comma
        test(document, 26).must_be_empty();
        test_implicit(document, 26).must_be_empty();

        test(document, 27).must_be_empty();
Repository: typst/typst
File: crates/typst-syntax/src/package.rs (Rust)

Code before:
            ));
        }

        if self.package.version != spec.version {
            return Err(eco_format!(
                "package manifest contains mismatched version {}",
                self.package.version
            ));
        }

        if let Some(required) = self.package.compiler {
            let current = PackageVersion::compiler();
            if !current.matches_ge(&required) {
                return Err(eco_format!(
                    "package requires typst {required} or newer \
                     (current version is {current})"
                ));
            }
        }

        Ok(())
    }
}

impl TemplateInfo {
    /// Create a new template info with only required content.
    pub fn new(path: impl Into<EcoString>, entrypoint: impl Into<EcoString>) -> Self {
        TemplateInfo {
            path: path.into(),
            entrypoint: entrypoint.into(),
            thumbnail: None,
            unknown_fields: UnknownFields::new(),
        }
    }
}

impl PackageInfo {
    /// Create a new package info with only required content.
    pub fn new(
        name: impl Into<EcoString>,
        version: PackageVersion,
        entrypoint: impl Into<EcoString>,
    ) -> Self {
        PackageInfo {
            name: name.into(),
            version,
            entrypoint: entrypoint.into(),
            authors: vec![],
            categories: vec![],
            compiler: None,
            description: None,

Review comment (suggestion):
nit: perhaps that should say required "fields" instead of "content", due to the word's association within Typst.

Code after:
            ));
        }

        if self.package.version != spec.version {
            return Err(eco_format!(
                "package manifest contains mismatched version {}",
                self.package.version
            ));
        }

        if let Some(required) = self.package.compiler {
            let current = PackageVersion::compiler();
            if !current.matches_ge(&required) {
                return Err(eco_format!(
                    "package requires typst {required} or newer \
                     (current version is {current})"
                ));
            }
        }

        Ok(())
    }
}

impl TemplateInfo {
    /// Create a new template info with only required fields.
    pub fn new(path: impl Into<EcoString>, entrypoint: impl Into<EcoString>) -> Self {
        TemplateInfo {
            path: path.into(),
            entrypoint: entrypoint.into(),
            thumbnail: None,
            unknown_fields: UnknownFields::new(),
        }
    }
}

impl PackageInfo {
    /// Create a new package info with only required fields.
    pub fn new(
        name: impl Into<EcoString>,
        version: PackageVersion,
        entrypoint: impl Into<EcoString>,
    ) -> Self {
        PackageInfo {
            name: name.into(),
            version,
            entrypoint: entrypoint.into(),
            authors: vec![],
            categories: vec![],
            compiler: None,
            description: None,
Repository: typst/typst
File: crates/typst-library/src/math/frac.rs (Rust)

Code before:
/// $ ((x+1)) / 2 = frac(a, b) $
/// ```
///
/// # Syntax
/// This function also has dedicated syntax: Use a slash to turn neighbouring
/// expressions into a fraction. Multiple atoms can be grouped into a single
/// expression using round grouping parentheses. Such parentheses are removed
/// from the output, but you can nest multiple to force them.
#[elem(title = "Fraction", Mathy)]
pub struct FracElem {
    /// The fraction's numerator.
    #[required]
    pub num: Content,

    /// The fraction's denominator.
    #[required]
    pub denom: Content,

    /// The style of the fraction.
    /// - `"vertical"`: stacked numerator and denominator with a bar.
    /// - `"diagonal"`: numerator and denominator separated by a slash.
    /// - `"horizontal"`: numerator and denominator placed inline and
    ///   parentheses are not absorbed
    ///
    /// The default style is "vertical"
    pub style: Option<FracStyle>,

    /// Whether the numerator was originally surrounded by parentheses
    /// that were stripped by the parser.
    #[internal]
    #[synthesized]
    pub num_deparenthesized: bool,

    /// Whether the denominator was originally surrounded by parentheses
    /// that were stripped by the parser.
    #[internal]
    #[synthesized]
    pub denom_deparenthesized: bool,
}

/// A binomial expression.
///
/// # Example
/// ```example
/// $ binom(n, k) $
/// $ binom(n, k_1, k_2, k_3, ..., k_m) $
/// ```
#[elem(title = "Binomial", Mathy)]
pub struct BinomElem {
    /// The binomial's upper index.
    #[required]

Review comment (suggestion):
```suggestion
    #[default(FracStyle::Vertical)]
    pub style: FracStyle,
```

Code after:
#[elem(title = "Fraction", Mathy)]
pub struct FracElem {
    /// The fraction's numerator.
    #[required]
    pub num: Content,

    /// The fraction's denominator.
    #[required]
    pub denom: Content,

    /// How the fraction shoud be laid out.
    ///
    /// ```example
    /// #set math.frac(style: "skewed")
    /// $ a / b $
    /// $ frac(x, y, style: "vertical") $
    /// ```
    #[default(FracStyle::Vertical)]
    pub style: FracStyle,

    /// Whether the numerator was originally surrounded by parentheses
    /// that were stripped by the parser.
    #[internal]
    #[parse(None)]
    #[default(false)]
    pub num_deparenthesized: bool,

    /// Whether the denominator was originally surrounded by parentheses
    /// that were stripped by the parser.
    #[internal]
    #[parse(None)]
    #[default(false)]
    pub denom_deparenthesized: bool,
}

/// Fraction style
#[derive(Debug, Default, Copy, Clone, Eq, PartialEq, Hash, Cast)]
pub enum FracStyle {
    /// Stacked numerator and denominator with a bar.
    #[default]
    Vertical,
    /// Numerator and denominator separated by a slash.
    Skewed,
    /// Numerator and denominator placed inline and parentheses are not
    /// absorbed.
    Horizontal,
}

/// A binomial expression.
///
/// # Example
Repository: typst/typst
File: crates/typst-layout/src/math/frac.rs (Rust)

Code before:
                span,
            ),
        );
        ctx.push(FrameFragment::new(styles, frame));
    }

    Ok(())
}

// Lays out a horizontal fraction
fn layout_horizontal_frac(
    ctx: &mut MathContext,
    styles: StyleChain,
    num: &Content,
    denom: &Content,
    span: Span,
    num_deparen: bool,
    denom_deparen: bool,
) -> SourceResult<()> {
    let num_frame = if num_deparen {
        ctx.layout_into_frame(
            &Content::new(LrElem::new(Content::sequence(vec![
                SymbolElem::packed('('),
                num.clone(),
                SymbolElem::packed(')'),
            ]))),
            styles,
        )?
    } else {
        ctx.layout_into_frame(num, styles)?
    };
    ctx.push(FrameFragment::new(styles, num_frame));

    let mut slash = GlyphFragment::new_char(ctx.font, styles, '/', span)?;
    slash.center_on_axis();
    ctx.push(slash);

    let denom_frame = if denom_deparen {
        ctx.layout_into_frame(
            &Content::new(LrElem::new(Content::sequence(vec![
                SymbolElem::packed('('),
                denom.clone(),
                SymbolElem::packed(')'),
            ]))),
            styles,
        )?
    } else {
        ctx.layout_into_frame(denom, styles)?
    };
    ctx.push(FrameFragment::new(styles, denom_frame));


Review comment (suggestion):
```suggestion
            &LrElem::new(Content::sequence(vec![
                SymbolElem::packed('('),
                num.clone(),
                SymbolElem::packed(')'),
            ])).pack(),
```
This is a little nicer, I think. And similar for `denom_frame` below.

Code after:
        ctx.push(right);
    } else {
        frame.push(
            line_pos,
            FrameItem::Shape(
                Geometry::Line(Point::with_x(line_width)).stroked(
                    FixedStroke::from_pair(
                        styles.get_ref(TextElem::fill).as_decoration(),
                        thickness,
                    ),
                ),
                span,
            ),
        );
        ctx.push(FrameFragment::new(styles, frame));
    }

    Ok(())
}

// Lays out a horizontal fraction
fn layout_horizontal_frac(
    ctx: &mut MathContext,
    styles: StyleChain,
    num: &Content,
    denom: &Content,
    span: Span,
    num_deparen: bool,
    denom_deparen: bool,
) -> SourceResult<()> {
    let num = if num_deparen {
        &LrElem::new(Content::sequence(vec![
            SymbolElem::packed('('),
            num.clone(),
            SymbolElem::packed(')'),
        ]))
        .pack()
    } else {
        num
    };
    let num_frame = ctx.layout_into_fragment(num, styles)?;
    ctx.push(num_frame);

    let mut slash =
        ctx.layout_into_fragment(&SymbolElem::packed('/').spanned(span), styles)?;
    slash.center_on_axis();
    ctx.push(slash);

    let denom = if denom_deparen {
        &LrElem::new(Content::sequence(vec![
            SymbolElem::packed('('),
Repository: typst/typst
File: crates/typst-layout/src/math/frac.rs (Rust)

Code before:

    Ok(())
}

// Lays out a horizontal fraction
fn layout_horizontal_frac(
    ctx: &mut MathContext,
    styles: StyleChain,
    num: &Content,
    denom: &Content,
    span: Span,
    num_deparen: bool,
    denom_deparen: bool,
) -> SourceResult<()> {
    let num = if num_deparen {
        &LrElem::new(Content::sequence(vec![
            SymbolElem::packed('('),
            num.clone(),
            SymbolElem::packed(')'),
        ]))
        .pack()
    } else {
        num
    };
    let num_frame = ctx.layout_into_frame(num, styles)?;
    ctx.push(FrameFragment::new(styles, num_frame));

    let mut slash = GlyphFragment::new_char(ctx.font, styles, '/', span)?;
    slash.center_on_axis();
    ctx.push(slash);

    let denom = if denom_deparen {
        &LrElem::new(Content::sequence(vec![
            SymbolElem::packed('('),
            denom.clone(),
            SymbolElem::packed(')'),
        ]))
        .pack()
    } else {
        denom
    };
    let denom_frame = ctx.layout_into_fragment(denom, styles)?;
    ctx.push(denom_frame);

    Ok(())
}

/// Lay out a skewed fraction.
fn layout_skewed_frac(
    ctx: &mut MathContext,
    styles: StyleChain,

Review comment (suggestion):
May as well use `ctx.layout_into_fragment` here as well.

Code after:
                        styles.get_ref(TextElem::fill).as_decoration(),
                        thickness,
                    ),
                ),
                span,
            ),
        );
        ctx.push(FrameFragment::new(styles, frame));
    }

    Ok(())
}

// Lays out a horizontal fraction
fn layout_horizontal_frac(
    ctx: &mut MathContext,
    styles: StyleChain,
    num: &Content,
    denom: &Content,
    span: Span,
    num_deparen: bool,
    denom_deparen: bool,
) -> SourceResult<()> {
    let num = if num_deparen {
        &LrElem::new(Content::sequence(vec![
            SymbolElem::packed('('),
            num.clone(),
            SymbolElem::packed(')'),
        ]))
        .pack()
    } else {
        num
    };
    let num_frame = ctx.layout_into_fragment(num, styles)?;
    ctx.push(num_frame);

    let mut slash =
        ctx.layout_into_fragment(&SymbolElem::packed('/').spanned(span), styles)?;
    slash.center_on_axis();
    ctx.push(slash);

    let denom = if denom_deparen {
        &LrElem::new(Content::sequence(vec![
            SymbolElem::packed('('),
            denom.clone(),
            SymbolElem::packed(')'),
        ]))
        .pack()
    } else {
        denom
    };