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

Code before:
        let num = counter.display_at_loc(engine, loc, styles, numbering)?;
        let alt = num.plain_text();
        let sup = SuperElem::new(num).pack().spanned(span);
        let prefix = DirectLinkElem::new(loc, sup, Some(alt)).pack().spanned(span);
        let body = self.note.body_content().unwrap().clone();

        Ok((prefix, body))
    }
}

impl ShowSet for Packed<FootnoteEntry> {
    fn show_set(&self, _: StyleChain) -> Styles {
        let mut out = Styles::new();
        out.set(ParElem::leading, Em::new(0.5).into());
        out.set(TextElem::size, TextSize(Em::new(0.85).into()));
        out
    }
}

cast! {
    FootnoteElem,
    v: Content => v.unpack::<Self>().unwrap_or_else(Self::with_content)
}

/// This is an empty element inserted by the HTML footnote rule to indicate the
/// presence of the default footnote rule. It's only used by the warning in
/// `FootnoteContainer::unsupported_with_custom_dom` and could be removed if
/// that's not needed anymore.
#[elem(Locatable)]
pub struct FootnoteMarker {}


Review comment (suggestion):
```suggestion
/// presence of the default footnote rule. It's only used by the error in
```

Code after:
        let num = counter.display_at_loc(engine, loc, styles, numbering)?;
        let alt = num.plain_text();
        let sup = SuperElem::new(num).pack().spanned(span);
        let prefix = DirectLinkElem::new(loc, sup, Some(alt)).pack().spanned(span);
        let body = self.note.body_content().unwrap().clone();

        Ok((prefix, body))
    }
}

impl ShowSet for Packed<FootnoteEntry> {
    fn show_set(&self, _: StyleChain) -> Styles {
        let mut out = Styles::new();
        out.set(ParElem::leading, Em::new(0.5).into());
        out.set(TextElem::size, TextSize(Em::new(0.85).into()));
        out
    }
}

cast! {
    FootnoteElem,
    v: Content => v.unpack::<Self>().unwrap_or_else(Self::with_content)
}

/// This is an empty element inserted by the HTML footnote rule to indicate the
/// presence of the default footnote rule. It's only used by the error in
/// `FootnoteContainer::unsupported_with_custom_dom` and could be removed if
/// that's not needed anymore.
#[elem(Locatable)]
pub struct FootnoteMarker {}
Repository: typst/typst
File: crates/typst-library/src/foundations/version.rs (Rust)

Code before:
    pub fn values(&self) -> &[u32] {
        &self.0
    }
}

#[scope]
impl Version {
    /// Creates a new version.
    ///
    /// It can have any number of components (even zero).
    ///
    /// ```example
    /// #version() \
    /// #version(1) \
    /// #version(1, 2, 3, 4) \
    /// #version((1, 2, 3, 4)) \
    /// #version((1, 2), 3)
    /// ```
    ///
    /// As a practical use case, it allows comparing current version
    /// ([`{sys.version}`]($version)) to a specific one.
    ///
    /// // This will have to be updated in the future to keep true and false.
    /// ```example
    /// #sys.version > version(0, 13, 0)
    /// #sys.version > version(1, 13, 0)
    /// ```
    #[func(constructor)]
    pub fn construct(
        /// The components of the version (array arguments are flattened)
        #[variadic]
        components: Vec<VersionComponents>,
    ) -> Version {
        let mut version = Version::new();
        for c in components {
            match c {
                VersionComponents::Single(v) => version.push(v),
                VersionComponents::Multiple(values) => {
                    for v in values {
                        version.push(v);
                    }
                }
            }
        }
        version
    }

    /// Retrieves a component of a version.
    ///
    /// The returned integer is always non-negative. Returns `0` if the version
    /// isn't specified to the necessary length.

Review comment (suggestion):
```suggestion
    /// #(sys.version > version(0, 13, 0))
    /// #(sys.version > version(1, 13, 0))
```

Code after:
    pub fn values(&self) -> &[u32] {
        &self.0
    }
}

#[scope]
impl Version {
    /// Creates a new version.
    ///
    /// It can have any number of components (even zero).
    ///
    /// ```example:"Constructing versions"
    /// #version() \
    /// #version(1) \
    /// #version(1, 2, 3, 4) \
    /// #version((1, 2, 3, 4)) \
    /// #version((1, 2), 3)
    /// ```
    ///
    /// As a practical use case, this allows comparing the current version
    /// ([`{sys.version}`]($version)) to a specific one.
    ///
    /// ```example:"Comparing with the current version"
    /// Current version: #sys.version \
    /// #(sys.version >= version(0, 14, 0)) \
    /// #(version(3, 2, 0) > version(4, 1, 0))
    /// ```
    #[func(constructor)]
    pub fn construct(
        /// The components of the version (array arguments are flattened)
        #[variadic]
        components: Vec<VersionComponents>,
    ) -> Version {
        let mut version = Version::new();
        for c in components {
            match c {
                VersionComponents::Single(v) => version.push(v),
                VersionComponents::Multiple(values) => {
                    for v in values {
                        version.push(v);
                    }
                }
            }
        }
        version
    }

    /// Retrieves a component of a version.
    ///
    /// The returned integer is always non-negative. Returns `0` if the version
    /// isn't specified to the necessary length.
Repository: typst/typst
File: crates/typst-library/src/foundations/version.rs (Rust)

Code before:
    }

    /// The values of the version
    pub fn values(&self) -> &[u32] {
        &self.0
    }
}

#[scope]
impl Version {
    /// Creates a new version.
    ///
    /// It can have any number of components (even zero).
    ///
    /// ```example
    /// #version() \
    /// #version(1) \
    /// #version(1, 2, 3, 4) \
    /// #version((1, 2, 3, 4)) \
    /// #version((1, 2), 3)
    /// ```
    ///
    /// As a practical use case, it allows comparing current version
    /// ([`{sys.version}`]($version)) to a specific one.
    ///
    /// // This will have to be updated in the future to keep true and false.
    /// ```example
    /// #(sys.version > version(0, 13, 0))
    /// #(sys.version > version(1, 13, 0))
    /// ```
    #[func(constructor)]
    pub fn construct(
        /// The components of the version (array arguments are flattened)
        #[variadic]
        components: Vec<VersionComponents>,
    ) -> Version {
        let mut version = Version::new();
        for c in components {
            match c {
                VersionComponents::Single(v) => version.push(v),
                VersionComponents::Multiple(values) => {
                    for v in values {
                        version.push(v);
                    }
                }
            }
        }
        version
    }

    /// Retrieves a component of a version.

Review comment (suggestion):
Will this `/// // ...` here actually prevent this comment from appearing in the documentation? (if that is the intention)

Code after:
    }

    /// The values of the version
    pub fn values(&self) -> &[u32] {
        &self.0
    }
}

#[scope]
impl Version {
    /// Creates a new version.
    ///
    /// It can have any number of components (even zero).
    ///
    /// ```example:"Constructing versions"
    /// #version() \
    /// #version(1) \
    /// #version(1, 2, 3, 4) \
    /// #version((1, 2, 3, 4)) \
    /// #version((1, 2), 3)
    /// ```
    ///
    /// As a practical use case, this allows comparing the current version
    /// ([`{sys.version}`]($version)) to a specific one.
    ///
    /// ```example:"Comparing with the current version"
    /// Current version: #sys.version \
    /// #(sys.version >= version(0, 14, 0)) \
    /// #(version(3, 2, 0) > version(4, 1, 0))
    /// ```
    #[func(constructor)]
    pub fn construct(
        /// The components of the version (array arguments are flattened)
        #[variadic]
        components: Vec<VersionComponents>,
    ) -> Version {
        let mut version = Version::new();
        for c in components {
            match c {
                VersionComponents::Single(v) => version.push(v),
                VersionComponents::Multiple(values) => {
                    for v in values {
                        version.push(v);
                    }
                }
            }
        }
        version
    }

    /// Retrieves a component of a version.
Repository: typst/typst
File: crates/typst-library/src/layout/hide.rs (Rust)

Code before:
use crate::foundations::{Content, elem};
use crate::introspection::Tagged;

/// Hides content without affecting layout.
///
/// The `hide` function allows you to hide content while the layout still 'sees'
/// it. This is useful to create whitespace that is exactly as large as some
/// content. It may also be useful to redact content because its arguments are
/// not included in the output.
///
/// # Example
/// ```example
/// Hello Jane \
/// #hide[Hello] Joe
/// ```
#[elem(Tagged)]
pub struct HideElem {
    /// The content to hide.
    #[required]
    pub body: Content,

    /// This style is set on the content contained in the `hide` element.
    #[internal]
    #[ghost]
    pub hidden: bool,
}


Review comment (suggestion):
```suggestion
/// be reverse-engineered based on its size in the layout. We thus do not
```

Code after:
use crate::foundations::{Content, elem};
use crate::introspection::Tagged;

/// Hides content without affecting layout.
///
/// The `hide` function allows you to hide content while the layout still "sees"
/// it. This is useful for creating blank space that is exactly as large as some
/// content.
///
/// # Example
/// ```example
/// Hello Jane \
/// #hide[Hello] Joe
/// ```
///
/// # Redaction
/// This function may also be useful for redacting content as its arguments are
/// neither present visually nor accessible to Assistive Technology. That said,
/// there can be _some_ traces of the hidden content (such as a bookmarked
/// heading in the PDF's Document Outline).
///
/// Note that, depending on the circumstances, it may be possible for content to
/// be reverse engineered based on its size in the layout. We thus do not
/// recommend using this function to hide highly sensitive information.
#[elem(Tagged)]
pub struct HideElem {
    /// The content to hide.
    #[required]
    pub body: Content,

    /// This style is set on the content contained in the `hide` element.
    #[internal]
    #[ghost]
    pub hidden: bool,
}
Repository: typst/typst
File: crates/typst-library/src/foundations/str.rs (Rust)

Code before:
#[ty(scope)]
#[derive(Debug, Clone)]
pub struct Regex(regex::Regex);

impl Regex {
    /// Create a new regular expression.
    pub fn new(re: &str) -> StrResult<Self> {
        regex::Regex::new(re).map(Self).map_err(|err| eco_format!("{err}"))
    }
}

#[scope]
impl Regex {
    /// Create a regular expression from a string.
    #[func(constructor)]
    pub fn construct(
        /// The regular expression as a string.
        ///
        /// Both Typst strings and regular expressions use backslashes for
        /// escaping. To produce a regex escape sequence that is also valid in
        /// Typst, you need to escape the backslash itself (e.g., writing
        /// `{regex("\\\\")}` for the regex `\\`). Regex escape sequences that
        /// are not valid Typst escape sequences (e.g., `\d` and `\b`) can be
        /// entered into strings directly, but it's a good practice to still
        /// escape them to avoid ambiguity (i.e., `{regex("\\b\\d")}`). See the
        /// [list of valid string escape sequences]($str.escapes).
        ///
        /// If you need many escape sequences, you can also create a raw element
        /// and extract its text to use it for your regular expressions:
        /// ``{regex(`\d+\.\d+\.\d+`.text)}``.
        regex: Spanned<Str>,
    ) -> SourceResult<Regex> {
        Self::new(&regex.v).at(regex.span)
    }
}

impl Deref for Regex {
    type Target = regex::Regex;

    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

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

impl PartialEq for Regex {

Review comment (security):
```suggestion
        /// [list of valid string escape sequences]($str/#escapes).
```

Code after:
    )
}

/// The char boundary access error message.
#[cold]
fn not_a_char_boundary(index: i64) -> EcoString {
    eco_format!("string index {} is not a character boundary", index)
}

/// The error message when the string is empty.
#[cold]
fn string_is_empty() -> EcoString {
    "string is empty".into()
}

/// A regular expression.
///
/// Can be used as a [show rule selector]($styling/#show-rules) and with
/// [string methods]($str) like `find`, `split`, and `replace`.
///
/// [See here](https://docs.rs/regex/latest/regex/#syntax) for a specification
/// of the supported syntax.
///
/// # Example
/// ```example
/// // Works with string methods.
/// #"a,b;c".split(regex("[,;]"))
///
/// // Works with show rules.
/// #show regex("\\d+"): set text(red)
///
/// The numbers 1 to 10.
/// ```
#[ty(scope)]
#[derive(Debug, Clone)]
pub struct Regex(regex::Regex);

impl Regex {
    /// Create a new regular expression.
    pub fn new(re: &str) -> StrResult<Self> {
        regex::Regex::new(re).map(Self).map_err(|err| eco_format!("{err}"))
    }
}

#[scope]
impl Regex {
    /// Create a regular expression from a string.
    #[func(constructor)]
    pub fn construct(
        /// The regular expression as a string.
        ///
Repository: typst/typst
File: crates/typst-library/src/foundations/sys.rs (Rust)

Code before:
//! System-related things.

use crate::foundations::{Dict, Module, Scope, Version};

/// A module with system-related things.
///
/// # Panics
///
/// If any version component of the Typst version overflows the numeric range of the respective
/// [`Version`] component here.
pub fn module(inputs: Dict) -> Module {
    let typst_version = typst_utils::TypstVersion::new();
    let version = Version::from_iter([
        u32::try_from(typst_version.major()).unwrap(),
        u32::try_from(typst_version.minor()).unwrap(),
        u32::try_from(typst_version.patch()).unwrap(),
    ]);

    let mut scope = Scope::deduplicating();
    scope.define("version", version);
    scope.define("inputs", inputs);
    Module::new("sys", scope)
}


Review comment (suggestion):
Since a Typst build cannot properly work with version numbers > u32, let's codify that in `TypstVersion` and have `major()`, `minor()` and `patch()` directly return u32.

Code after:
//! System-related things.

use crate::foundations::{Dict, Module, Scope, Version};

/// A module with system-related things.
pub fn module(inputs: Dict) -> Module {
    let typst_version = typst_utils::version();
    let version = Version::from_iter([
        typst_version.major(),
        typst_version.minor(),
        typst_version.patch(),
    ]);

    let mut scope = Scope::deduplicating();
    scope.define("version", version);
    scope.define("inputs", inputs);
    Module::new("sys", scope)
}
Repository: typst/typst
File: crates/typst-utils/src/version.rs (Rust)

Code before:
            }
        }
    })
}

/// Typst version definition.
///
/// This structure contains the current Typst version. To query the precise
/// version number, refer to the [`TypstVersion::major()`],
/// [`TypstVersion::minor()`] and [`TypstVersion::patch()`] functions. You can
/// read the underlying, raw version string (e.g. for CLI output) with
/// [`TypstVersion::raw`].
///
/// Optionally, this may also contain the Git commit from which Typst was built.
/// However, this field may be unpopulated.
#[derive(Debug, Clone, Copy)]
pub struct TypstVersion {
    /// Typst major version number.
    major: u32,
    /// Typst minor version number.
    minor: u32,
    /// Typst patch version number.
    patch: u32,
    /// Raw, unmodified version string.
    raw: &'static str,
    /// The commit SHA.
    commit: Option<&'static str>,
}

impl TypstVersion {
    /// Returns the Typst major version.
    pub fn major(&self) -> u32 {
        self.major
    }

    /// Returns the Typst minor version.
    pub fn minor(&self) -> u32 {
        self.minor
    }

    /// Returns the Typst patch version.
    pub fn patch(&self) -> u32 {
        self.patch
    }

    /// Returns the raw, unparsed version string.
    ///
    /// Guaranteed to conform to SemVer.
    pub fn raw(&self) -> &'static str {
        self.raw
    }

Review comment (suggestion):
I would say that it's "commit's SHA-1 value", but it looks like some web results also use "commit SHA", though I don't think it's a proper way.

Code after:
            }
        }
    })
}

/// Typst version definition.
///
/// This structure contains the current Typst version. To query the precise
/// version number, refer to the [`TypstVersion::major()`],
/// [`TypstVersion::minor()`] and [`TypstVersion::patch()`] functions. You can
/// read the underlying, raw version string (e.g., for CLI output) with
/// [`TypstVersion::raw`].
///
/// Optionally, this may also contain the hash value of the Git commit from
/// which Typst was built. However, this field may be unpopulated.
#[derive(Debug, Clone, Copy)]
pub struct TypstVersion {
    /// Typst major version number.
    major: u32,
    /// Typst minor version number.
    minor: u32,
    /// Typst patch version number.
    patch: u32,
    /// Raw, unmodified version string.
    raw: &'static str,
    /// The raw commit hash.
    commit: Option<&'static str>,
}

impl TypstVersion {
    /// Returns the Typst major version.
    pub fn major(&self) -> u32 {
        self.major
    }

    /// Returns the Typst minor version.
    pub fn minor(&self) -> u32 {
        self.minor
    }

    /// Returns the Typst patch version.
    pub fn patch(&self) -> u32 {
        self.patch
    }

    /// Returns the raw, unparsed version string.
    ///
    /// Guaranteed to conform to SemVer.
    pub fn raw(&self) -> &'static str {
        self.raw
    }
Repository: typst/typst
File: crates/typst-utils/build.rs (Rust)

Code before:
use std::process::Command;

fn main() {
    println!("cargo:rerun-if-env-changed=TYPST_VERSION");
    println!("cargo:rerun-if-env-changed=TYPST_COMMIT_SHA");

    if option_env!("TYPST_VERSION").is_none() {
        println!("cargo:rustc-env=TYPST_VERSION={}", env!("CARGO_PKG_VERSION"));
    }

    if option_env!("TYPST_COMMIT_SHA").is_none()
        && let Some(sha) = Command::new("git")
            .args(["rev-parse", "HEAD"])
            .output()
            .ok()
            .filter(|output| output.status.success())
            .and_then(|output| String::from_utf8(output.stdout.get(..8)?.into()).ok())
    {
        println!("cargo:rustc-env=TYPST_COMMIT_SHA={sha}");
    }
}


Review comment (suggestion):
We could leave the truncation to git as well:

```rust
            .args(["rev-parse", "--short=8", "HEAD"])
            .output()
            .ok()
            .filter(|output| output.status.success())
            .and_then(|output| String::from_utf8(output.stdout).ok())
```

Code after:
use std::process::Command;

fn main() {
    println!("cargo:rerun-if-env-changed=TYPST_VERSION");
    println!("cargo:rerun-if-env-changed=TYPST_COMMIT_SHA");

    if option_env!("TYPST_VERSION").is_none() {
        println!("cargo:rustc-env=TYPST_VERSION={}", env!("CARGO_PKG_VERSION"));
    }

    if option_env!("TYPST_COMMIT_SHA").is_none()
        && let Some(sha) = Command::new("git")
            .args(["rev-parse", "HEAD"])
            .output()
            .ok()
            .filter(|output| output.status.success())
            .and_then(|output| String::from_utf8(output.stdout).ok())
    {
        println!("cargo:rustc-env=TYPST_COMMIT_SHA={sha}");
    }
}
Repository: typst/typst
File: crates/typst-utils/src/version.rs (Rust)

Code before:
//! Typst version information.

/// Returns the version of Typst.
///
/// The information is read from the following sources in order of precedence:
///
/// - For the version number:
///  - The `TYPST_VERSION` environment variable
///  - The `CARGO_PKG_VERSION` environment variable
///
/// - For the commit sha:
///   - The `TYPST_COMMIT_SHA` environment variable
///   - The output of `git rev-parse HEAD`
///
/// # Panics
/// If the `TYPST_VERSION` environment variable holds a version string that
/// doesn't conform to SemVer.
pub fn version() -> TypstVersion {
    *crate::singleton!(TypstVersion, {
        let raw = env!("TYPST_VERSION");
        let commit = option_env!("TYPST_COMMIT_SHA");
        match semver::Version::parse(raw) {
            Ok(version) => {
                return TypstVersion {
                    major: version.major.try_into().unwrap(),
                    minor: version.minor.try_into().unwrap(),
                    patch: version.patch.try_into().unwrap(),
                    raw,
                    commit,
                };
            }
            Err(err) => {
                panic!("failed to parse {raw:?} as semantic version number: {err:?}")
            }

Review comment (bug):
The indentation here is wrong:

```suggestion
///   - The `TYPST_VERSION` environment variable
///   - The `CARGO_PKG_VERSION` environment variable
```

Code after:
//! Typst version information.

/// Returns the version of Typst.
///
/// The information is read from the following sources:
///
/// - For the version number: The `TYPST_VERSION` environment variable
/// - For the commit hash: The `TYPST_COMMIT_SHA` environment variable
///
/// Build tooling can set these environment variables to configure the exposed
/// information. If the environment variables are left unset, the values are
/// populated via `build.rs` from the Cargo package manifest version and the git
/// hash in the current repository (if any).
///
/// # Panics
/// If the `TYPST_VERSION` environment variable holds a version string that
/// doesn't conform to SemVer.
pub fn version() -> TypstVersion {
    *crate::singleton!(TypstVersion, {
        let raw = env!("TYPST_VERSION");
        let commit = option_env!("TYPST_COMMIT_SHA");
        match semver::Version::parse(raw) {
            Ok(version) => {
                return TypstVersion {
                    major: version.major.try_into().unwrap(),
                    minor: version.minor.try_into().unwrap(),
                    patch: version.patch.try_into().unwrap(),
                    raw,
                    commit,
                };
            }
            Err(err) => {
                panic!("failed to parse {raw:?} as semantic version number: {err:?}")
            }