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

Code before:
}

impl Add<HAlignment> for VAlignment {
    type Output = Alignment;

    fn add(self, rhs: HAlignment) -> Self::Output {
        Alignment::Both(rhs, self)
    }
}

impl From<VAlignment> for Alignment {
    fn from(align: VAlignment) -> Self {
        Self::V(align)
    }
}

cast! {
    VAlignment,
    self => Alignment::V(self).into_value(),
    align: Alignment => match align {
        Alignment::V(y) => y,
        v => bail!("expected `top`, `horizon`, or `bottom`, found {}", v.repr()),
    }
}

/// A vertical alignment which only allows either ends, thus excluding `horizon`.
#[derive(Debug, Default, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
pub enum OuterVAlignment {
    #[default]
    Top,
    Bottom,
}

impl OuterVAlignment {
    /// Resolve the axis alignment based on the vertical direction.
    pub const fn fix(self) -> FixedAlignment {
        match self {
            Self::Top => FixedAlignment::Start,
            Self::Bottom => FixedAlignment::End,
        }
    }
}

impl From<OuterVAlignment> for VAlignment {
    fn from(value: OuterVAlignment) -> Self {
        match value {
            OuterVAlignment::Top => Self::Top,
            OuterVAlignment::Bottom => Self::Bottom,
        }
    }
}

Review comment (suggestion):
I personally think this change was unnecessary (and made it less clear). The type name already suggests we only allow outer values; the doc comment lists them for further clarity.

Code after:
    }

    /// Turns into a fixed alignment.
    pub const fn fix(self) -> FixedAlignment {
        match self {
            Self::Top => FixedAlignment::Start,
            Self::Horizon => FixedAlignment::Center,
            Self::Bottom => FixedAlignment::End,
        }
    }
}

impl Repr for VAlignment {
    fn repr(&self) -> EcoString {
        match self {
            Self::Top => "top".into(),
            Self::Horizon => "horizon".into(),
            Self::Bottom => "bottom".into(),
        }
    }
}

impl Add<HAlignment> for VAlignment {
    type Output = Alignment;

    fn add(self, rhs: HAlignment) -> Self::Output {
        Alignment::Both(rhs, self)
    }
}

impl From<VAlignment> for Alignment {
    fn from(align: VAlignment) -> Self {
        Self::V(align)
    }
}

impl TryFrom<Alignment> for VAlignment {
    type Error = EcoString;

    fn try_from(value: Alignment) -> StrResult<Self> {
        match value {
            Alignment::V(v) => Ok(v),
            v => bail!("expected `top`, `horizon`, or `bottom`, found {}", v.repr()),
        }
    }
}

cast! {
    VAlignment,
    self => Alignment::V(self).into_value(),
    align: Alignment => Self::try_from(align)?,
Repository: typst/typst
File: crates/typst/src/layout/align.rs (Rust)

Code before:
/// An internal representation that combines horizontal or vertical alignments. The
/// allowed alignment positions are designated by the type parameter `H` and `V`.
///
/// This is not user-visible, but an internal type to impose type safety. For example,
/// `SpecificAlignment<HAlignment, OuterVAlignment>` does not allow vertical alignment
/// position "center", because `V = OuterVAlignment` doesn't have it.
#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)]
pub enum SpecificAlignment<H, V>
where
    H: Into<HAlignment> + Copy,
    V: Into<VAlignment> + Copy,
    HAlignment: Into<H>,
    VAlignment: Into<V>,
{
    H(H),
    V(V),
    Both(H, V),
}

impl<H, V> SpecificAlignment<H, V>
where
    H: Into<HAlignment> + Copy,
    V: Into<VAlignment> + Copy,
    HAlignment: Into<H>,
    VAlignment: Into<V>,
{
    /// The horizontal component.
    pub const fn x(self) -> Option<H> {
        match self {
            Self::H(x) | Self::Both(x, _) => Some(x),
            Self::V(_) => None,
        }
    }

    /// The vertical component.
    pub const fn y(self) -> Option<V> {
        match self {
            Self::V(y) | Self::Both(_, y) => Some(y),
            Self::H(_) => None,
        }
    }
}

impl Add<OuterVAlignment> for HAlignment {
    type Output = SpecificAlignment<HAlignment, OuterVAlignment>;

    fn add(self, rhs: OuterVAlignment) -> Self::Output {
        SpecificAlignment::Both(self, rhs)
    }
}


Review comment (suggestion):
As a consequence of my previous comment:

```suggestion
impl<H, V> SpecificAlignment<H, V> {
```


Code after:
impl From<OuterVAlignment> for VAlignment {
    fn from(value: OuterVAlignment) -> Self {
        match value {
            OuterVAlignment::Top => Self::Top,
            OuterVAlignment::Bottom => Self::Bottom,
        }
    }
}

impl TryFrom<Alignment> for OuterVAlignment {
    type Error = EcoString;

    fn try_from(value: Alignment) -> StrResult<Self> {
        match value {
            Alignment::V(VAlignment::Top) => Ok(Self::Top),
            Alignment::V(VAlignment::Bottom) => Ok(Self::Bottom),
            v => bail!("expected `top` or `bottom`, found {}", v.repr()),
        }
    }
}

cast! {
    OuterVAlignment,
    self => VAlignment::from(self).into_value(),
    align: Alignment => Self::try_from(align)?,
}

/// An internal representation that combines horizontal or vertical alignments. The
/// allowed alignment positions are designated by the type parameter `H` and `V`.
///
/// This is not user-visible, but an internal type to impose type safety. For example,
/// `SpecificAlignment<HAlignment, OuterVAlignment>` does not allow vertical alignment
/// position "center", because `V = OuterVAlignment` doesn't have it.
#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)]
pub enum SpecificAlignment<H, V> {
    H(H),
    V(V),
    Both(H, V),
}

impl<H, V> SpecificAlignment<H, V>
where
    H: Copy,
    V: Copy,
{
    /// The horizontal component.
    pub const fn x(self) -> Option<H> {
        match self {
            Self::H(h) | Self::Both(h, _) => Some(h),
            Self::V(_) => None,
        }
Repository: typst/typst
File: crates/typst/src/layout/align.rs (Rust)

Code before:

    /// The vertical component.
    pub const fn y(self) -> Option<V> {
        match self {
            Self::V(y) | Self::Both(_, y) => Some(y),
            Self::H(_) => None,
        }
    }
}

impl Add<OuterVAlignment> for HAlignment {
    type Output = SpecificAlignment<HAlignment, OuterVAlignment>;

    fn add(self, rhs: OuterVAlignment) -> Self::Output {
        SpecificAlignment::Both(self, rhs)
    }
}

impl Add<HAlignment> for OuterVAlignment {
    type Output = SpecificAlignment<HAlignment, OuterVAlignment>;

    fn add(self, rhs: HAlignment) -> Self::Output {
        SpecificAlignment::Both(rhs, self)
    }
}

impl<H, V> From<SpecificAlignment<H, V>> for Alignment
where
    H: Into<HAlignment> + Copy,
    V: Into<VAlignment> + Copy,
    HAlignment: Into<H>,
    VAlignment: Into<V>,
{
    fn from(value: SpecificAlignment<H, V>) -> Alignment {
        type FromType<H, V> = SpecificAlignment<H, V>;
        match value {
            FromType::H(x) => Alignment::H(x.into()),
            FromType::V(y) => Alignment::V(y.into()),
            FromType::Both(x, y) => Alignment::Both(x.into(), y.into()),
        }
    }
}

impl<H, V> Reflect for SpecificAlignment<H, V>
where
    H: Into<HAlignment> + Copy + Reflect,
    V: Into<VAlignment> + Copy + Reflect,
    HAlignment: Into<H>,
    VAlignment: Into<V>,
{
    fn input() -> CastInfo {

Review comment (suggestion):
I don't think we will use this anywhere in the Rust code, since you can just write `SpecificAlignment::Both(first, second)` instead (which is much clearer). Users won't use this either, since variables in Typst can't have type OuterHAlignment nor OuterVAlignment, given that a user can only have a variable of type Alignment (which may correspond to HAlignment or VAlignment, or both, under the hood).

```suggestion
```

Code after:
#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)]
pub enum SpecificAlignment<H, V> {
    H(H),
    V(V),
    Both(H, V),
}

impl<H, V> SpecificAlignment<H, V>
where
    H: Copy,
    V: Copy,
{
    /// The horizontal component.
    pub const fn x(self) -> Option<H> {
        match self {
            Self::H(h) | Self::Both(h, _) => Some(h),
            Self::V(_) => None,
        }
    }

    /// The vertical component.
    pub const fn y(self) -> Option<V> {
        match self {
            Self::V(v) | Self::Both(_, v) => Some(v),
            Self::H(_) => None,
        }
    }
}

impl<H, V> From<SpecificAlignment<H, V>> for Alignment
where
    HAlignment: From<H>,
    VAlignment: From<V>,
{
    fn from(value: SpecificAlignment<H, V>) -> Self {
        type FromType<H, V> = SpecificAlignment<H, V>;
        match value {
            FromType::H(h) => Self::H(HAlignment::from(h)),
            FromType::V(v) => Self::V(VAlignment::from(v)),
            FromType::Both(h, v) => Self::Both(HAlignment::from(h), VAlignment::from(v)),
        }
    }
}

impl<H, V> Reflect for SpecificAlignment<H, V>
where
    H: Reflect,
    V: Reflect,
{
    fn input() -> CastInfo {
        H::input() + V::input()
Repository: typst/typst
File: crates/typst/src/layout/align.rs (Rust)

Code before:

impl Add<HAlignment> for OuterVAlignment {
    type Output = SpecificAlignment<HAlignment, OuterVAlignment>;

    fn add(self, rhs: HAlignment) -> Self::Output {
        SpecificAlignment::Both(rhs, self)
    }
}

impl<H, V> From<SpecificAlignment<H, V>> for Alignment
where
    H: Into<HAlignment> + Copy,
    V: Into<VAlignment> + Copy,
    HAlignment: Into<H>,
    VAlignment: Into<V>,
{
    fn from(value: SpecificAlignment<H, V>) -> Alignment {
        type FromType<H, V> = SpecificAlignment<H, V>;
        match value {
            FromType::H(x) => Alignment::H(x.into()),
            FromType::V(y) => Alignment::V(y.into()),
            FromType::Both(x, y) => Alignment::Both(x.into(), y.into()),
        }
    }
}

impl<H, V> Reflect for SpecificAlignment<H, V>
where
    H: Into<HAlignment> + Copy + Reflect,
    V: Into<VAlignment> + Copy + Reflect,
    HAlignment: Into<H>,
    VAlignment: Into<V>,
{
    fn input() -> CastInfo {
        H::input() + V::input()
    }

    fn output() -> CastInfo {
        H::output() + V::output()
    }

    fn castable(value: &Value) -> bool {
        H::castable(value) || V::castable(value)
    }
}

impl<H, V> IntoValue for SpecificAlignment<H, V>
where
    H: Into<HAlignment> + Copy + Reflect,
    V: Into<VAlignment> + Copy + Reflect,
    HAlignment: Into<H>,

Review comment (suggestion):
(might need a little help from rustfmt here)

```suggestion
impl<H, V> From<SpecificAlignment<H, V>> for Alignment
where
    HAlignment: From<H>,
    VAlignment: From<V>,
{
    fn from(value: SpecificAlignment<H, V>) -> Self {
        match value {
            SpecificAlignment::H(x) => Self::H(HAlignment::from(x)),
            SpecificAlignment::V(y) => Self::V(VAlignment::from(y)),
            SpecificAlignment::Both(x, y) => Self::Both(HAlignment::from(x), VAlignment::from(y)),
        }
    }
}

```


Code after:
        }
    }

    /// The vertical component.
    pub const fn y(self) -> Option<V> {
        match self {
            Self::V(v) | Self::Both(_, v) => Some(v),
            Self::H(_) => None,
        }
    }
}

impl<H, V> From<SpecificAlignment<H, V>> for Alignment
where
    HAlignment: From<H>,
    VAlignment: From<V>,
{
    fn from(value: SpecificAlignment<H, V>) -> Self {
        type FromType<H, V> = SpecificAlignment<H, V>;
        match value {
            FromType::H(h) => Self::H(HAlignment::from(h)),
            FromType::V(v) => Self::V(VAlignment::from(v)),
            FromType::Both(h, v) => Self::Both(HAlignment::from(h), VAlignment::from(v)),
        }
    }
}

impl<H, V> Reflect for SpecificAlignment<H, V>
where
    H: Reflect,
    V: Reflect,
{
    fn input() -> CastInfo {
        H::input() + V::input()
    }

    fn output() -> CastInfo {
        H::output() + V::output()
    }

    fn castable(value: &Value) -> bool {
        H::castable(value) || V::castable(value)
    }
}

impl<H, V> IntoValue for SpecificAlignment<H, V>
where
    HAlignment: From<H>,
    VAlignment: From<V>,
{
    fn into_value(self) -> Value {
Repository: typst/typst
File: crates/typst/src/layout/align.rs (Rust)

Code before:
    V: Into<VAlignment> + Copy + Reflect + FromValue,
    HAlignment: Into<H>,
    VAlignment: Into<V>,
{
    fn from_value(value: Value) -> StrResult<Self> {
        if Alignment::castable(&value) {
            let align = Alignment::from_value(value)?;
            let result = match align {
                Alignment::H(x) => {
                    let h_value = x.into_value();
                    Self::H(H::from_value(h_value)?)
                }
                Alignment::V(y) => {
                    let v_value = y.into_value();
                    Self::V(V::from_value(v_value)?)
                }
                Alignment::Both(x, y) => {
                    let h_value = x.into_value();
                    let v_value = y.into_value();
                    Self::Both(H::from_value(h_value)?, V::from_value(v_value)?)
                }
            };
            return Ok(result);
        }
        Err(Self::error(&value))
    }
}

/// A fixed alignment in the global coordinate space.
///
/// For horizontal alignment, start is globally left and for vertical alignment
/// it is globally top.
#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
pub enum FixedAlignment {
    Start,
    Center,
    End,
}

impl FixedAlignment {
    /// Returns the position of this alignment in a container with the given
    /// extent.
    pub fn position(self, extent: Abs) -> Abs {
        match self {
            Self::Start => Abs::zero(),
            Self::Center => extent / 2.0,
            Self::End => extent,
        }
    }
}


Review comment (suggestion):
I was going to suggest to remove the bounds here, but thinking further, I think you might be able to replace the last three trait implementations by a single `cast!` invocation (take a look at how other types do it).

Code after:
                Alignment::Both(h, v) => {
                    Self::Both(H::try_from(h.into())?, V::try_from(v.into())?)
                }
            };
            return Ok(result);
        }
        Err(Self::error(&value))
    }
}

/// A fixed alignment in the global coordinate space.
///
/// For horizontal alignment, start is globally left and for vertical alignment
/// it is globally top.
#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
pub enum FixedAlignment {
    Start,
    Center,
    End,
}

impl FixedAlignment {
    /// Returns the position of this alignment in a container with the given
    /// extent.
    pub fn position(self, extent: Abs) -> Abs {
        match self {
            Self::Start => Abs::zero(),
            Self::Center => extent / 2.0,
            Self::End => extent,
        }
    }
}

impl From<Side> for FixedAlignment {
    fn from(side: Side) -> Self {
        match side {
            Side::Left => Self::Start,
            Side::Top => Self::Start,
            Side::Right => Self::End,
            Side::Bottom => Self::End,
        }
    }
}
Repository: typst/typst
File: crates/typst/src/layout/align.rs (Rust)

Code before:
    }
}

impl Repr for VAlignment {
    fn repr(&self) -> EcoString {
        match self {
            Self::Top => "top".into(),
            Self::Horizon => "horizon".into(),
            Self::Bottom => "bottom".into(),
        }
    }
}

impl Add<HAlignment> for VAlignment {
    type Output = Alignment;

    fn add(self, rhs: HAlignment) -> Self::Output {
        Alignment::Both(rhs, self)
    }
}

impl Resolve for VAlignment {
    type Output = FixedAlignment;

    fn resolve(self, styles: StyleChain) -> Self::Output {
        self.fix(TextElem::dir_in(styles))
    }
}

impl From<VAlignment> for Alignment {
    fn from(align: VAlignment) -> Self {
        Self::V(align)
    }
}

impl TryFrom<Alignment> for VAlignment {
    type Error = EcoString;

    fn try_from(value: Alignment) -> StrResult<Self> {
        match value {
            Alignment::V(v) => Ok(v),
            v => bail!("expected `top`, `horizon`, or `bottom`, found {}", v.repr()),
        }
    }
}

cast! {
    VAlignment,
    self => Alignment::V(self).into_value(),
    align: Alignment => Self::try_from(align)?,
}

Review comment (suggestion):
No need to retrieve the text direction if it isn't used. If `VAlignment` really needs to implement `FixAlignment`, we could just pass `Dir::TTB`.

Code after:
    }
}

impl Repr for VAlignment {
    fn repr(&self) -> EcoString {
        match self {
            Self::Top => "top".into(),
            Self::Horizon => "horizon".into(),
            Self::Bottom => "bottom".into(),
        }
    }
}

impl Add<HAlignment> for VAlignment {
    type Output = Alignment;

    fn add(self, rhs: HAlignment) -> Self::Output {
        Alignment::Both(rhs, self)
    }
}

impl Resolve for VAlignment {
    type Output = FixedAlignment;

    fn resolve(self, _: StyleChain) -> Self::Output {
        self.fix(Dir::TTB)
    }
}

impl From<VAlignment> for Alignment {
    fn from(align: VAlignment) -> Self {
        Self::V(align)
    }
}

impl TryFrom<Alignment> for VAlignment {
    type Error = EcoString;

    fn try_from(value: Alignment) -> StrResult<Self> {
        match value {
            Alignment::V(v) => Ok(v),
            v => bail!("expected `top`, `horizon`, or `bottom`, found {}", v.repr()),
        }
    }
}

cast! {
    VAlignment,
    self => Alignment::V(self).into_value(),
    align: Alignment => Self::try_from(align)?,
}
Repository: typst/typst
File: crates/typst/src/symbols/sym.rs (Rust)

Code before:
    smash: '⨳',

    // Currency.
    bitcoin: '₿',
    dollar: '$',
    euro: '€',
    franc: '₣',
    lira: '₺',
    peso: '₱',
    pound: '£',
    ruble: '₽',
    rupee: '₹',
    won: '₩',
    yen: '¥',

    // Miscellaneous.
    ballot: ['☐', x: '☒'],
    checkmark: ['✓', light: '🗸'],
    floral: ['❦', l: '☙', r: '❧'],
    notes: [up: '🎜', down: '🎝'],
    refmark: '※',
    servicemark: '℠',
    maltese: '✠',
    suit: [club: '♣', diamond: '♦', heart: '♥', spade: '♠'],

    // Shapes.
    bullet: '•',
    circle: [
        stroked: '○',
        stroked.tiny: '∘',
        stroked.small: '⚬',
        stroked.big: '◯',
        filled: '●',
        filled.tiny: '⦁',
        filled.small: '∙',
        filled.big: '⬤',
        dotted: '◌',
        nested: '⊚',
    ],
    ellipse: [
        stroked.h: '⬭',
        stroked.v: '⬯',
        filled.h: '⬬',
        filled.v: '⬮',
    ],
    triangle: [
        stroked.r: '▷',
        stroked.l: '◁',
        stroked.t: '△',
        stroked.b: '▽',
        stroked.bl: '◺',

Review comment (suggestion):
Maybe it's better to do something like
```suggestion
        club: ['♣', stroked: '♧'],
```

Code after:
    smash: '⨳',

    // Currency.
    bitcoin: '₿',
    dollar: '$',
    euro: '€',
    franc: '₣',
    lira: '₺',
    peso: '₱',
    pound: '£',
    ruble: '₽',
    rupee: '₹',
    won: '₩',
    yen: '¥',

    // Miscellaneous.
    ballot: ['☐', x: '☒'],
    checkmark: ['✓', light: '🗸'],
    floral: ['❦', l: '☙', r: '❧'],
    notes: [up: '🎜', down: '🎝'],
    refmark: '※',
    servicemark: '℠',
    maltese: '✠',
    suit: [
        club.filled: '♣',
        club.stroked: '♧',
        diamond.filled: '♦',
        diamond.stroked: '♢',
        heart.filled: '♥',
        heart.stroked: '♡',
        spade.filled: '♠',
        spade.stroked: '♤',
    ],

    // Shapes.
    bullet: '•',
    circle: [
        stroked: '○',
        stroked.tiny: '∘',
        stroked.small: '⚬',
        stroked.big: '◯',
        filled: '●',
        filled.tiny: '⦁',
        filled.small: '∙',
        filled.big: '⬤',
        dotted: '◌',
        nested: '⊚',
    ],
    ellipse: [
        stroked.h: '⬭',
        stroked.v: '⬯',
Repository: typst/typst
File: crates/typst-cli/src/args.rs (Rust)

Code before:

/// An input that is either stdin or a real path.
#[derive(Debug, Clone)]
pub enum Input {
    /// Stdin, represented by `-`.
    Stdin,
    /// A non-empty path.
    Path(PathBuf),
}

/// An output that is either stdout or a real path.
#[derive(Debug, Clone)]
pub enum Output {
    /// Stdout, represented by `-`.
    Stdout,
    /// A non-empty path.
    Path(PathBuf),
}

impl Display for Output {
    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
        let s = match self {
            Output::Stdout => "stdout".to_string(),
            Output::Path(path) => path.display().to_string(),
        };
        write!(f, "{s}")
    }
}

/// The clap value parser used by `SharedArgs.input`
fn input_value_parser(value: &str) -> Result<Input, clap::error::Error> {
    if value.is_empty() {
        Err(clap::Error::new(clap::error::ErrorKind::InvalidValue))
    } else if value == "-" {
        Ok(Input::Stdin)
    } else {
        Ok(Input::Path(value.into()))
    }
}

/// The clap value parser used by `CompileCommand.output`
fn output_value_parser(value: &str) -> Result<Output, clap::error::Error> {
    // Empty value also handled by clap for `Option<Output>`
    if value.is_empty() {
        Err(clap::Error::new(clap::error::ErrorKind::InvalidValue))
    } else if value == "-" {
        Ok(Output::Stdout)
    } else {
        Ok(Output::Path(value.into()))
    }
}

Review comment (suggestion):
```suggestion
        match self {
            Output::Stdout => f.pad("stdout"),
            Output::Path(path) => path.display().fmt(f),
        }
```

Code after:
}

/// An input that is either stdin or a real path.
#[derive(Debug, Clone)]
pub enum Input {
    /// Stdin, represented by `-`.
    Stdin,
    /// A non-empty path.
    Path(PathBuf),
}

/// An output that is either stdout or a real path.
#[derive(Debug, Clone)]
pub enum Output {
    /// Stdout, represented by `-`.
    Stdout,
    /// A non-empty path.
    Path(PathBuf),
}

impl Display for Output {
    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
        match self {
            Output::Stdout => f.pad("stdout"),
            Output::Path(path) => path.display().fmt(f),
        }
    }
}

/// The clap value parser used by `SharedArgs.input`
fn input_value_parser(value: &str) -> Result<Input, clap::error::Error> {
    if value.is_empty() {
        Err(clap::Error::new(clap::error::ErrorKind::InvalidValue))
    } else if value == "-" {
        Ok(Input::Stdin)
    } else {
        Ok(Input::Path(value.into()))
    }
}

/// The clap value parser used by `CompileCommand.output`
fn output_value_parser(value: &str) -> Result<Output, clap::error::Error> {
    // Empty value also handled by clap for `Option<Output>`
    if value.is_empty() {
        Err(clap::Error::new(clap::error::ErrorKind::InvalidValue))
    } else if value == "-" {
        Ok(Output::Stdout)
    } else {
        Ok(Output::Path(value.into()))
    }
}