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

Code before:
        impl Accent {
            /// Normalize an accent to a combining one.
            pub fn combine(c: char) -> Option<char> {
                Some(match c {
                    $(
                        $($ch)|+ => accents!(@__private first $($ch),+),
                    )+
                    _ => return None,
                })
            }
        }

        $(
            /// The accent function for callable symbol definitions.
            #[func]
            pub fn $name(
                /// The base to which the accent is applied.
                base: Content,
                /// The size of the accent, relative to the width of the base.
                #[named]
                size: Option<Smart<Rel<Length>>>,
            ) -> Content {
                let mut accent = AccentElem::new(
                    base,
                    Accent::new(accents!(@__private first $($ch),+)),
                );
                if let Some(size) = size {
                    accent = accent.with_size(size);
                }
                accent.pack()
            }
        )+
    };

    (@__private first $ch:literal $(, $rest:literal)*) => { $ch };
}

/// Attaches an accent to a base.
///
/// # Example
/// ```example
/// $grave(a) = accent(a, `)$ \
/// $arrow(a) = accent(a, arrow)$ \
/// $tilde(a) = accent(a, \u{0303})$
/// ```
#[elem(LayoutMath)]
pub struct AccentElem {
    /// The base to which the accent is applied.
    /// May consist of multiple letters.
    ///
    /// ```example

Review comment (suggestion):
```suggestion
                let mut accent = AccentElem::new(base, Accent::new($primary));
```

Code after:
        impl Accent {
            /// Normalize an accent to a combining one.
            pub fn combine(c: char) -> Option<char> {
                Some(match c {
                    $($primary $(| $alt)* => $primary,)*
                    _ => return None,
                })
            }
        }

        $(
            /// The accent function for callable symbol definitions.
            #[func]
            pub fn $name(
                /// The base to which the accent is applied.
                base: Content,
                /// The size of the accent, relative to the width of the base.
                #[named]
                size: Option<Smart<Rel<Length>>>,
            ) -> Content {
                let mut accent = AccentElem::new(base, Accent::new($primary));
                if let Some(size) = size {
                    accent = accent.with_size(size);
                }
                accent.pack()
            }
        )+
    };
}

/// Attaches an accent to a base.
///
/// # Example
/// ```example
/// $grave(a) = accent(a, `)$ \
/// $arrow(a) = accent(a, arrow)$ \
/// $tilde(a) = accent(a, \u{0303})$
/// ```
#[elem(LayoutMath)]
pub struct AccentElem {
    /// The base to which the accent is applied.
    /// May consist of multiple letters.
    ///
    /// ```example
    /// $arrow(A B C)$
    /// ```
    #[required]
    pub base: Content,

    /// The accent to apply to the base.
    ///
Repository: typst/typst
File: crates/typst-macros/src/symbols.rs (Rust)

Code before:
                <#path as ::typst::foundations::NativeFunc>::func,
            ),
        },
    }
}

struct Symbol {
    name: syn::Ident,
    kind: Kind,
}

enum Kind {
    Single(syn::LitChar, Handler),
    Multiple(Punctuated<Variant, Token![,]>),
}

struct Variant {
    name: String,
    c: syn::LitChar,
    handler: Handler,
}

enum Handler {
    None,
    Path(Path),
}

impl Parse for Symbol {
    fn parse(input: ParseStream) -> Result<Self> {
        let name = input.call(Ident::parse_any)?;
        input.parse::<Token![:]>()?;
        let kind = input.parse()?;
        Ok(Self { name, kind })
    }
}

impl Parse for Kind {
    fn parse(input: ParseStream) -> Result<Self> {
        let handler = input.parse::<Handler>()?;
        if input.peek(syn::LitChar) {
            Ok(Self::Single(input.parse()?, handler))
        } else {
            if !handler.is_none() {
                return Err(input.error("unexpected handler"));
            }
            let content;
            syn::bracketed!(content in input);
            Ok(Self::Multiple(Punctuated::parse_terminated(&content)?))
        }
    }
}

Review comment (suggestion):
In my experience, redefined `Option` is a bit of an anti-pattern because you don't get the option methods with it (e.g. `is_none` which you had to redefine). You could try having a newtype `struct Handler(Option<Path>)` instead and see whether it ends up nicer or not.

Code after:
                <#path as ::typst::foundations::NativeFunc>::func,
            ),
        },
    }
}

struct Symbol {
    name: syn::Ident,
    kind: Kind,
}

enum Kind {
    Single(syn::LitChar, Handler),
    Multiple(Punctuated<Variant, Token![,]>),
}

struct Variant {
    name: String,
    c: syn::LitChar,
    handler: Handler,
}

struct Handler(Option<Path>);

impl Parse for Symbol {
    fn parse(input: ParseStream) -> Result<Self> {
        let name = input.call(Ident::parse_any)?;
        input.parse::<Token![:]>()?;
        let kind = input.parse()?;
        Ok(Self { name, kind })
    }
}

impl Parse for Kind {
    fn parse(input: ParseStream) -> Result<Self> {
        let handler = input.parse::<Handler>()?;
        if input.peek(syn::LitChar) {
            Ok(Self::Single(input.parse()?, handler))
        } else {
            if handler.0.is_some() {
                return Err(input.error("unexpected handler"));
            }
            let content;
            syn::bracketed!(content in input);
            Ok(Self::Multiple(Punctuated::parse_terminated(&content)?))
        }
    }
}

impl Parse for Variant {
    fn parse(input: ParseStream) -> Result<Self> {
Repository: typst/typst
File: crates/typst-syntax/src/parser.rs (Rust)

Code before:
    }

    if let Some(m) = named {
        p.wrap(m, SyntaxKind::Named);
    }
}

/// Parses the contents of a code block.
fn code(p: &mut Parser, stop: impl FnMut(&Parser) -> bool) {
    let m = p.marker();
    code_exprs(p, stop);
    p.wrap(m, SyntaxKind::Code);
}

/// Parses a sequence of code expressions.
fn code_exprs(p: &mut Parser, mut stop: impl FnMut(&Parser) -> bool) {
    while !p.end() && !stop(p) {
        p.enter_newline_mode(NewlineMode::Contextual);

        let at_expr = p.at_set(set::CODE_EXPR);
        if at_expr {
            code_expr(p);
            if !p.end() && !stop(p) && !p.eat_if(SyntaxKind::Semicolon) {
                p.expected("semicolon or line break");
                if p.at(SyntaxKind::Label) {
                    p.hint("you can use labels only in markup mode");
                }
            }
        }

        p.exit_newline_mode();
        if !at_expr && !p.end() {
            p.unexpected();
        }
    }
}

/// Parses a single code expression.
fn code_expr(p: &mut Parser) {
    code_expr_prec(p, false, 0)
}

/// Parses a code expression embedded in markup or math.
fn embedded_code_expr(p: &mut Parser) {
    p.enter_newline_mode(NewlineMode::Stop);
    p.enter(LexMode::Code);
    p.assert(SyntaxKind::Hash);
    p.unskip();

    let stmt = p.at_set(set::STMT);
    let at = p.at_set(set::ATOMIC_CODE_EXPR);

Review comment (suggestion):
Maybe we can improve this message a bit? Something like "labels can only be applied in markup mode", followed by a second hint "try wrapping your code in a markup block (\`[ ]\`)"

(I'm open to further suggestions)

Code after:
    }

    if let Some(m) = named {
        p.wrap(m, SyntaxKind::Named);
    }
}

/// Parses the contents of a code block.
fn code(p: &mut Parser, stop: impl FnMut(&Parser) -> bool) {
    let m = p.marker();
    code_exprs(p, stop);
    p.wrap(m, SyntaxKind::Code);
}

/// Parses a sequence of code expressions.
fn code_exprs(p: &mut Parser, mut stop: impl FnMut(&Parser) -> bool) {
    while !p.end() && !stop(p) {
        p.enter_newline_mode(NewlineMode::Contextual);

        let at_expr = p.at_set(set::CODE_EXPR);
        if at_expr {
            code_expr(p);
            if !p.end() && !stop(p) && !p.eat_if(SyntaxKind::Semicolon) {
                p.expected("semicolon or line break");
                if p.at(SyntaxKind::Label) {
                    p.hint("labels can only be applied in markup mode");
                    p.hint("try wrapping your code in a markup block (`[ ]`)");
                }
            }
        }

        p.exit_newline_mode();
        if !at_expr && !p.end() {
            p.unexpected();
        }
    }
}

/// Parses a single code expression.
fn code_expr(p: &mut Parser) {
    code_expr_prec(p, false, 0)
}

/// Parses a code expression embedded in markup or math.
fn embedded_code_expr(p: &mut Parser) {
    p.enter_newline_mode(NewlineMode::Stop);
    p.enter(LexMode::Code);
    p.assert(SyntaxKind::Hash);
    p.unskip();

    let stmt = p.at_set(set::STMT);
Repository: typst/typst
File: crates/typst/src/foundations/label.rs (Rust)

Code before:
    pub fn construct(
        /// The name of the label.
        name: PicoStr,
    ) -> Label {
        Self(name)
    }
}

impl Repr for Label {
    fn repr(&self) -> EcoString {
        eco_format!("<{}>", self.as_str())
    }
}

impl From<Label> for PicoStr {
    fn from(value: Label) -> Self {
        value.into_inner()
    }
}

/// Indicates that an element cannot be labelled.
pub trait Unlabellable {}

pub(crate) fn is_valid_in_label_literal(id: &str) -> bool {
    !id.is_empty() && id.chars().all(typst_syntax::is_valid_in_label)
}


Review comment (suggestion):
It seems that this function is simple and only used once in `CastInfo::error`.

I'm not sure if this is over-encapsulation.

Code after:
use ecow::{eco_format, EcoString};

use crate::foundations::{func, scope, ty, Repr};
use crate::utils::PicoStr;

/// A label for an element.
///
/// Inserting a label into content attaches it to the closest preceding element
/// that is not a space. The preceding element must be in the same scope as the
/// label, which means that `[Hello #[<label>]]`, for instance, wouldn't work.
///
/// A labelled element can be [referenced]($ref), [queried]($query) for, and
/// [styled]($styling) through its label.
///
/// Once constructed, you can get the name of a label using
/// [`str`]($str/#constructor).
///
/// # Example
/// ```example
/// #show <a>: set text(blue)
/// #show label("b"): set text(red)
///
/// = Heading <a>
/// *Strong* #label("b")
/// ```
///
/// # Syntax
/// This function also has dedicated syntax: You can create a label by enclosing
/// its name in angle brackets. This works both in markup and code. A label's
/// name can contain letters, numbers, `_`, `-`, `:`, and `.`.
///
/// Currently, labels can only be attached to elements in markup mode, not in
/// code mode. This might change in the future.
#[ty(scope, cast)]
#[derive(Debug, Clone, Copy, Eq, PartialEq, Ord, PartialOrd, Hash)]
pub struct Label(PicoStr);

impl Label {
    /// Creates a label from a string, interning it.
    pub fn new(name: impl Into<PicoStr>) -> Self {
        Self(name.into())
    }

    /// Resolves the label to a string.
    pub fn as_str(&self) -> &'static str {
        self.0.resolve()
    }

    /// Turns this label into its inner interned string.
    pub fn into_inner(self) -> PicoStr {
        self.0
    }
}

#[scope]
impl Label {
    /// Creates a label from a string.
    #[func(constructor)]
    pub fn construct(
        /// The name of the label.
        name: PicoStr,
    ) -> Label {
        Self(name)
    }
}

impl Repr for Label {
    fn repr(&self) -> EcoString {
        eco_format!("<{}>", self.as_str())
    }
}

impl From<Label> for PicoStr {
    fn from(value: Label) -> Self {
        value.into_inner()
    }
}

/// Indicates that an element cannot be labelled.
pub trait Unlabellable {}
Repository: typst/typst
File: crates/typst-syntax/src/lexer.rs (Rust)

Code before:
    }

    /// Jump to the given index in the string.
    pub fn jump(&mut self, index: usize) {
        self.s.jump(index);
    }

    /// Whether the last token contained a newline.
    pub fn newline(&self) -> bool {
        self.newline
    }

    /// Take out the last error, if any.
    pub fn take_error(&mut self) -> Option<SyntaxError> {
        self.error.take()
    }
}

impl Lexer<'_> {
    /// Construct a full-positioned syntax error.
    fn error(&mut self, message: impl Into<EcoString>) -> SyntaxKind {
        self.error = Some(SyntaxError {
            span: Span::detached(),
            message: message.into(),
            hints: eco_vec![],
        });
        SyntaxKind::Error
    }

    /// If the current node is an error, adds a hint.
    fn hint(&mut self, message: impl Into<EcoString>) {
        if let Some(mut e) = self.take_error() {
            e.hints.push(message.into());
            self.error = Some(e);
        }
    }
}

/// Shared methods with all [`LexMode`].
impl Lexer<'_> {
    /// Proceed to the next token and return its [`SyntaxKind`]. Note the
    /// token could be a [trivia](SyntaxKind::is_trivia).
    pub fn next(&mut self) -> SyntaxKind {
        if self.mode == LexMode::Raw {
            let Some((kind, end)) = self.raw.pop() else {
                return SyntaxKind::End;
            };
            self.s.jump(end);
            return kind;
        }


Review comment (suggestion):
```suggestion
        self.error = Some(SyntaxError::new(message));
```

Code after:
    }

    /// Jump to the given index in the string.
    pub fn jump(&mut self, index: usize) {
        self.s.jump(index);
    }

    /// Whether the last token contained a newline.
    pub fn newline(&self) -> bool {
        self.newline
    }

    /// Take out the last error, if any.
    pub fn take_error(&mut self) -> Option<SyntaxError> {
        self.error.take()
    }
}

impl Lexer<'_> {
    /// Construct a full-positioned syntax error.
    fn error(&mut self, message: impl Into<EcoString>) -> SyntaxKind {
        self.error = Some(SyntaxError::new(message));
        SyntaxKind::Error
    }

    /// If the current node is an error, adds a hint.
    fn hint(&mut self, message: impl Into<EcoString>) {
        if let Some(error) = &mut self.error {
            error.hints.push(message.into());
        }
    }
}

/// Shared methods with all [`LexMode`].
impl Lexer<'_> {
    /// Proceed to the next token and return its [`SyntaxKind`]. Note the
    /// token could be a [trivia](SyntaxKind::is_trivia).
    pub fn next(&mut self) -> SyntaxKind {
        if self.mode == LexMode::Raw {
            let Some((kind, end)) = self.raw.pop() else {
                return SyntaxKind::End;
            };
            self.s.jump(end);
            return kind;
        }

        self.newline = false;
        self.error = None;
        let start = self.s.cursor();
        match self.s.eat() {
            Some(c) if is_space(c, self.mode) => self.whitespace(start, c),
Repository: typst/typst
File: crates/typst/src/foundations/content.rs (Rust)

Code before:
/// An error arising when trying to access a field of content.
#[derive(Copy, Clone, Debug)]
pub enum FieldAccessError {
    MissingField,
    MissingFieldNoDefault,
    FieldNotSet,
    Internal,
}

impl FieldAccessError {
    /// Gets the error message given a field name.
    #[cold]
    pub fn get_message(self, field: &str) -> EcoString {
        match self {
            FieldAccessError::MissingField => {
                eco_format!("content does not contain field {}", field.repr())
            }
            FieldAccessError::MissingFieldNoDefault => {
                eco_format!(
                    "content does not contain field {} and \
                 no default value was specified",
                    field.repr()
                )
            }
            FieldAccessError::FieldNotSet => {
                eco_format!("field {} in content is not set at this point", field.repr())
            }
            FieldAccessError::Internal => {
                eco_format!(
                    "internal error when accessing field {} in content – this is a bug",
                    field.repr()
                )
            }
        }
    }
}


Review comment (suggestion):
the wording here could be slightly confusing if a person has written a set rule right above and thinks that this will have an effect. perhaps "is not known" at this point? we could also add a hint, but I have no idea how to explain where it is available in the length of a hint.

Code after:
/// An error arising when trying to access a field of content.
#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)]
pub enum FieldAccessError {
    Unknown,
    Unset,
    Internal,
}

impl FieldAccessError {
    /// Formats the error message given the content and the field name.
    #[cold]
    pub fn message(self, content: &Content, field: &str) -> EcoString {
        let elem_name = content.elem().name();
        match self {
            FieldAccessError::Unknown => {
                eco_format!("{elem_name} does not have field {}", field.repr())
            }
            FieldAccessError::Unset => {
                eco_format!(
                    "field {} in {elem_name} is not known at this point",
                    field.repr()
                )
            }
            FieldAccessError::Internal => {
                eco_format!(
                    "internal error when accessing field {} in {elem_name} – this is a bug",
                    field.repr()
                )
            }
        }
    }

    /// Formats the error message for an `at` calls without a default value.
    #[cold]
    pub fn message_no_default(self, content: &Content, field: &str) -> EcoString {
        let mut msg = self.message(content, field);
        msg.push_str(" and no default was specified");
        msg
    }
}
Repository: typst/typst
File: crates/typst-syntax/src/ast.rs (Rust)

Code before:

node! {
    /// A field access: `properties.age`.
    FieldAccess
}

impl<'a> FieldAccess<'a> {
    /// The expression to access the field on.
    pub fn target(self) -> Expr<'a> {
        self.0.cast_first_match().unwrap_or_default()
    }

    /// The name of the field.
    pub fn field(self) -> Ident<'a> {
        self.0.cast_last_match().unwrap_or_default()
    }
}

node! {
    /// An invocation of a function or method: `f(x, y)`.
    FuncCall
}

impl<'a> FuncCall<'a> {
    /// The function identifier.
    pub fn ident(self) -> Ident<'a> {
        self.0.cast_first_match().unwrap_or_default()
    }

    /// The function to call.
    pub fn callee(self) -> Expr<'a> {
        self.0.cast_first_match().unwrap_or_default()
    }

    /// The arguments to the function.
    pub fn args(self) -> Args<'a> {
        self.0.cast_last_match().unwrap_or_default()
    }
}

node! {
    /// A function call's argument list: `(12pt, y)`.
    Args
}

impl<'a> Args<'a> {
    /// The positional and named arguments.
    pub fn items(self) -> impl DoubleEndedIterator<Item = Arg<'a>> {
        self.0.children().filter_map(SyntaxNode::cast)
    }


Review comment (suggestion):
The callee may not be an identifier, so this operation is a bit ill-defined. You should match on `callee` instead.

Code after:

node! {
    /// A field access: `properties.age`.
    FieldAccess
}

impl<'a> FieldAccess<'a> {
    /// The expression to access the field on.
    pub fn target(self) -> Expr<'a> {
        self.0.cast_first_match().unwrap_or_default()
    }

    /// The name of the field.
    pub fn field(self) -> Ident<'a> {
        self.0.cast_last_match().unwrap_or_default()
    }
}

node! {
    /// An invocation of a function or method: `f(x, y)`.
    FuncCall
}

impl<'a> FuncCall<'a> {
    /// The function to call.
    pub fn callee(self) -> Expr<'a> {
        self.0.cast_first_match().unwrap_or_default()
    }

    /// The arguments to the function.
    pub fn args(self) -> Args<'a> {
        self.0.cast_last_match().unwrap_or_default()
    }
}

node! {
    /// A function call's argument list: `(12pt, y)`.
    Args
}

impl<'a> Args<'a> {
    /// The positional and named arguments.
    pub fn items(self) -> impl DoubleEndedIterator<Item = Arg<'a>> {
        self.0.children().filter_map(SyntaxNode::cast)
    }

    /// Whether there is a comma at the end.
    pub fn trailing_comma(self) -> bool {
        self.0
            .children()
            .rev()
Repository: typst/typst
File: crates/typst/src/eval/call.rs (Rust)

Code before:
        if i > 0 {
            body += TextElem::packed(',');
        }
        body += arg;
    }
    if trailing_comma {
        body += TextElem::packed(',');
    }
    Ok(Value::Content(
        callee.display().spanned(callee_span)
            + LrElem::new(TextElem::packed('(') + body + TextElem::packed(')')).pack(),
    ))
}

/// Provide a hint if the callee is a shadowed standard library function.
fn hint_if_shadowed_std(
    vm: &mut Vm,
    callee: &ast::Expr,
    mut err: HintedString,
) -> HintedString {
    if let ast::Expr::Ident(ident) = callee {
        let ident = ident.get();
        if vm.scopes.check_std_shadowed(ident) {
            err.hint(eco_format!(
                "use `std.{}` to access the shadowed standard library function",
                ident,
            ));
        }
    }
    err
}

/// A visitor that determines which variables to capture for a closure.
pub struct CapturesVisitor<'a> {
    external: Option<&'a Scopes<'a>>,
    internal: Scopes<'a>,
    captures: Scope,
    capturer: Capturer,
}

impl<'a> CapturesVisitor<'a> {
    /// Create a new visitor for the given external scopes.
    pub fn new(external: Option<&'a Scopes<'a>>, capturer: Capturer) -> Self {
        Self {
            external,
            internal: Scopes::new(None),
            captures: Scope::new(),
            capturer,
        }
    }


Review comment (suggestion):
I think `ident` could be directly in the string here. for `format!` clippy even complains afaik, but not for `eco_format`.

Code after:
        body += arg;
    }
    if trailing_comma {
        body += TextElem::packed(',');
    }
    Ok(Value::Content(
        callee.display().spanned(callee_span)
            + LrElem::new(TextElem::packed('(') + body + TextElem::packed(')')).pack(),
    ))
}

/// Provide a hint if the callee is a shadowed standard library function.
fn hint_if_shadowed_std(
    vm: &mut Vm,
    callee: &ast::Expr,
    mut err: HintedString,
) -> HintedString {
    if let ast::Expr::Ident(ident) = callee {
        let ident = ident.get();
        if vm.scopes.check_std_shadowed(ident) {
            err.hint(eco_format!(
                "use `std.{ident}` to access the shadowed standard library function",
            ));
        }
    }
    err
}

/// A visitor that determines which variables to capture for a closure.
pub struct CapturesVisitor<'a> {
    external: Option<&'a Scopes<'a>>,
    internal: Scopes<'a>,
    captures: Scope,
    capturer: Capturer,
}

impl<'a> CapturesVisitor<'a> {
    /// Create a new visitor for the given external scopes.
    pub fn new(external: Option<&'a Scopes<'a>>, capturer: Capturer) -> Self {
        Self {
            external,
            internal: Scopes::new(None),
            captures: Scope::new(),
            capturer,
        }
    }

    /// Return the scope of captured variables.
    pub fn finish(self) -> Scope {
        self.captures
    }