ronantakizawa/codereview-bench huggingface dataset:
Repository: HigherOrderCO/Bend
File: src/term/transform/encode_pattern_matching.rs (Rust)

Code before:
  let term = add_arg_calls(term, &match_path);

  // Lambda for pattern matched value
  let term = Term::named_lam(arg_name, term);

  let term = add_arg_lams(term, &match_path);

  add_case_to_book(book, crnt_name.clone(), term);
}

impl Term {
  fn named_lam(nam: Name, bod: Term) -> Term {
    Term::Lam { tag: Tag::Static, nam: Some(nam), bod: Box::new(bod) }
  }

  fn arg_call(term: Term, arg: Name) -> Term {
    Term::App { tag: Tag::Static, fun: Box::new(term), arg: Box::new(Term::Var { nam: arg }) }
  }

  fn optional_arg_call(term: Term, arg: Option<Name>) -> Term {
    let arg = Box::new(arg.map_or(Term::Era, |nam| Term::Var { nam }));
    Term::App { tag: Tag::Static, fun: Box::new(term), arg }
  }

  // Returns a sequence representing whether the lambda arg is binded or ignored
  fn lambdas_usage(&self) -> Vec<Option<()>> {
    fn go(term: &Term, vec: &mut Vec<Option<()>>) {
      match term {
        Term::Lam { nam: None, bod, .. } => {
          vec.push(None);
          go(bod, vec);
        }
        Term::Lam { nam: Some(_), bod, .. } => {
          vec.push(Some(()));
          go(bod, vec);
        }
        Term::Let { nxt, .. } | Term::Dup { nxt, .. } => {
          go(nxt, vec);
        }
        _ => {}
      }
    }

    let mut term = self.clone();
    term.make_var_names_unique();
    term.linearize_vars();

    let mut res = Vec::new();
    go(&term, &mut res);
    res
  }

Review comment (suggestion):
This function name is a bit hard to understand. Something like `arg_vars_are_used` maybe? Not sure, this one is a bit hard.

Code after:
  let term = Term::named_lam(arg_name, term);

  let term = add_arg_lams(term, &match_path);

  add_case_to_book(book, crnt_name.clone(), term);
}

impl Term {
  fn named_lam(nam: Name, bod: Term) -> Term {
    Term::Lam { tag: Tag::Static, nam: Some(nam), bod: Box::new(bod) }
  }

  fn arg_call(term: Term, arg: Name) -> Term {
    Term::App { tag: Tag::Static, fun: Box::new(term), arg: Box::new(Term::Var { nam: arg }) }
  }

  fn optional_arg_call(term: Term, arg: Option<Name>) -> Term {
    let arg = Box::new(arg.map_or(Term::Era, |nam| Term::Var { nam }));
    Term::App { tag: Tag::Static, fun: Box::new(term), arg }
  }

  /// Returns a sequence representing whether consecutive lambda args are binded or discarded
  ///
  /// # Example
  ///
  /// `λa λ* (a λx x) -> vec![true, false]`
  /// 
  fn arg_vars_are_used(&self) -> Vec<bool> {
    fn go(term: &Term, vec: &mut Vec<bool>) {
      match term {
        Term::Lam { nam: None, bod, .. } => {
          vec.push(false);
          go(bod, vec);
        }
        Term::Lam { nam: Some(_), bod, .. } => {
          vec.push(true);
          go(bod, vec);
        }
        Term::Let { nxt, .. } | Term::Dup { nxt, .. } => {
          go(nxt, vec);
        }
        _ => {}
      }
    }

    let mut term = self.clone();
    term.make_var_names_unique();
    term.linearize_vars();

    let mut res = Vec::new();
    go(&term, &mut res);
Repository: HigherOrderCO/Bend
File: src/term/parser/lexer.rs (Rust)

Code before:

  #[regex("@|λ")]
  Lambda,

  #[token("$")]
  Dollar,

  #[token("let")]
  Let,

  #[token("match")]
  Match,

  #[token("=")]
  Equals,

  #[regex("[0-9_]+", |lex| from_radix(10, lex).ok(), priority = 2)]
  #[regex("0[xX][0-9a-fA-F_]+", |lex| from_radix(16, lex).ok())]
  #[regex("0[bB][0-1_]+", |lex| from_radix(2, lex).ok())]
  Num(u64),

  #[regex(r#""([^"\\]|\\t|\\u|\\n|\\")*""#, |lex| normalized_string(lex).ok())]
  Str(String),

  #[regex(r#"'\\U[0-9a-fA-F]{1,8}'"#, normalized_char, priority = 2)]
  #[regex(r#"'(\p{L}|\p{M}|\p{N}|\p{P}|\p{S}|\p{Z}|\p{C}|\p{Emoji}|\\t|\\u[0-9a-fA-F]{1,4}|\\n|\\')'"#, normalized_char)]
  Char(u64),

  #[token("#")]
  Hash,

  #[token("+")]
  Add,

  #[token("-")]
  Sub,

  #[token("*")]
  Asterisk,

  #[token("/")]
  Div,

  #[token("%")]
  Mod,

  #[token("~")]
  Tilde,

  #[token("&")]
  And,

Review comment (security):
Can you add a comment explaining that this covers every possible unicode character?.

Also, you could merge the different escape codes into a single regex rule for simplicity: `\\[tn']`. Then if we want to add more in the future it becomes easier.

Code after:

  #[regex("@|λ")]
  Lambda,

  #[token("$")]
  Dollar,

  #[token("let")]
  Let,

  #[token("match")]
  Match,

  #[token("=")]
  Equals,

  #[regex("[0-9_]+", |lex| from_radix(10, lex).ok(), priority = 2)]
  #[regex("0[xX][0-9a-fA-F_]+", |lex| from_radix(16, lex).ok())]
  #[regex("0[bB][0-1_]+", |lex| from_radix(2, lex).ok())]
  Num(u64),

  #[regex(r#""([^"\\]|\\[tun"])*""#, |lex| normalized_string(lex).ok())]
  Str(String),

  #[regex(r#"'\\U[0-9a-fA-F]{1,8}'"#, normalized_char, priority = 2)]
  // Since '.' is just covering any ascii char, we need to make the
  // regex match any possible character of the unicode general category
  #[regex(r#"'(\p{L}|\p{M}|\p{N}|\p{P}|\p{S}|\p{Z}|\p{C}|\p{Emoji}|\\u[0-9a-fA-F]{1,4}|\\[tn'])'"#, normalized_char)]
  Char(u64),

  #[token("#")]
  Hash,

  #[token("+")]
  Add,

  #[token("-")]
  Sub,

  #[token("*")]
  Asterisk,

  #[token("/")]
  Div,

  #[token("%")]
  Mod,

  #[token("~")]
  Tilde,
Repository: HigherOrderCO/Bend
File: src/term/transform/eta_reduction.rs (Rust)

Code before:
use super::encode_pattern_matching::RULE_PREFIX;
use crate::term::*;

impl Book {
  /// Applies eta-reduction to all generated definitions, converting occurrences of `@x (f x)` into just `f`.
  /// Assumes that variables are linear (used exactly once).
  pub fn eta_reduction(&mut self) {
    for def in self.defs.values_mut() {
      def.assert_no_pattern_matching_rules();

      if self
        .def_names
        .name(&def.def_id)
        // Definitions containing `RULE_PREFIX` are match arms branches written by the user, and shouldn't be reduced
        .map_or(false, |Name(name)| name.contains('$') && !name.contains(RULE_PREFIX))
      {
        let rule = &mut def.rules[0].body;
        rule.eta_reduction();
      }
    }
  }
}

impl Term {
  /// Eta-reduces a term and any subterms.
  /// Expects variables to be linear.
  pub fn eta_reduction(&mut self) {
    match self {
      Term::Lam { tag: lam_tag, nam: Some(lam_var), bod } => {
        bod.eta_reduction();
        match bod.as_mut() {
          Term::App { tag: arg_tag, fun, arg: box Term::Var { nam: var_nam } }
            if lam_var == var_nam && lam_tag == arg_tag =>
          {
            *self = std::mem::take(fun.as_mut());
          }
          _ => (),
        }
      }
      Term::Lam { bod, .. } | Term::Chn { bod, .. } => bod.eta_reduction(),

Review comment (refactor):
This way, the extracted supercombinators will still be eta-reduced (They have $ but not $R).

I think for each definition we should keep track of whether their body was auto-generated or user -provided, by adding an extra field to the Definition type

Code after:
use crate::term::*;

impl Book {
  /// Applies eta-reduction to all generated definitions, converting occurrences of `@x (f x)` into just `f`.
  /// Assumes that variables are linear (used exactly once).
  pub fn eta_reduction(&mut self) {
    for def in self.defs.values_mut() {
      def.assert_no_pattern_matching_rules();
      if def.generated {
        let rule = &mut def.rules[0].body;
        rule.eta_reduction();
      }
    }
  }
}

impl Term {
  /// Eta-reduces a term and any subterms.
  /// Expects variables to be linear.
  pub fn eta_reduction(&mut self) {
    match self {
      Term::Lam { tag: lam_tag, nam: Some(lam_var), bod } => {
        bod.eta_reduction();
        match bod.as_mut() {
          Term::App { tag: arg_tag, fun, arg: box Term::Var { nam: var_nam } }
            if lam_var == var_nam && lam_tag == arg_tag =>
          {
            *self = std::mem::take(fun.as_mut());
          }
          _ => (),
        }
      }
      Term::Lam { bod, .. } | Term::Chn { bod, .. } => bod.eta_reduction(),
      Term::Let { pat: _, val, nxt } | Term::Dup { tag: _, fst: _, snd: _, val, nxt } => {
        val.eta_reduction();
        nxt.eta_reduction();
      }
      Term::App { fun: fst, arg: snd, .. }
      | Term::Tup { fst, snd }
      | Term::Sup { fst, snd, .. }
Repository: HigherOrderCO/Bend
File: src/term/transform/encode_lists.rs (Rust)

Code before:
          used |= arm.encode_lists();
        }
        used
      }
      Term::Var { nam: Name(nam) } => nam == LCONS || nam == LNIL,
      Term::Lnk { .. } | Term::Ref { .. } | Term::Num { .. } | Term::Str { .. } | Term::Era => false,
    }
  }
}

impl Pattern {
  pub fn encode_lists(&mut self) -> bool {
    match self {
      Pattern::Var(Some(Name(nam))) => nam == LNIL,
      Pattern::List(pats) => {
        let lnil = Pattern::Var(Some(Name::new(LNIL)));

        *self = pats.iter_mut().rfold(lnil, |acc, nxt| {
          nxt.encode_lists();
          Pattern::Ctr(Name::new(LCONS), vec![nxt.clone(), acc])
        });

        true
      }
      Pattern::Ctr(Name(nam), pats) => {
        let mut uses = nam == LCONS;
        for pat in pats {
          uses |= pat.encode_lists();
        }
        uses
      }
      Pattern::Tup(fst, snd) => {
        let fst_uses = fst.encode_lists();
        let snd_uses = snd.encode_lists();
        fst_uses || snd_uses
      }
      Pattern::Var(..) | Pattern::Num(..) => false,
    }
  }
}


Review comment (suggestion):
nam == LCONS || nam == LNIL

If we write the pattern like this `(Head (LNil)) = LNil` then it will be understood as a constructor by the parser without needing the delayed name resolving

Code after:
      Term::Var { nam: Name(nam) } => nam == LCONS || nam == LNIL,
      Term::Lnk { .. } | Term::Ref { .. } | Term::Num { .. } | Term::Str { .. } | Term::Era => false,
    }
  }
}

impl Pattern {
  pub fn encode_lists(&mut self) -> bool {
    match self {
      // TODO: This should be removed once when we separate adding
      // constructors to desugaring list terms.
      Pattern::Var(Some(Name(nam))) => nam == LCONS || nam == LNIL,
      Pattern::List(pats) => {
        let lnil = Pattern::Var(Some(Name::new(LNIL)));

        *self = pats.iter_mut().rfold(lnil, |acc, nxt| {
          nxt.encode_lists();
          Pattern::Ctr(Name::new(LCONS), vec![nxt.clone(), acc])
        });

        true
      }
      Pattern::Ctr(Name(nam), pats) => {
        let mut uses = nam == LCONS || nam == LNIL;
        for pat in pats {
          uses |= pat.encode_lists();
        }
        uses
      }
      Pattern::Tup(fst, snd) => {
        let fst_uses = fst.encode_lists();
        let snd_uses = snd.encode_lists();
        fst_uses || snd_uses
      }
      Pattern::Var(..) | Pattern::Num(..) => false,
    }
  }
}
Repository: HigherOrderCO/Bend
File: src/lib.rs (Rust)

Code before:
pub struct WarningOpts {
  pub all: bool,
  pub match_only_vars: bool,
  pub unused_defs: bool,
}

impl WarningOpts {
  pub fn all() -> Self {
    Self { all: true, match_only_vars: true, unused_defs: true }
  }

  // TODO(refactor): this kind of code does not look good
  pub fn from_vec(&mut self, values: Vec<String>) -> Result<(), String> {
    for value in values {
      match value.as_ref() {
        "all" => self.all = true,
        "unused-defs" => self.unused_defs = true,
        "match-only-vars" => self.match_only_vars = true,
        other => return Err(format!("Unknown option '{other}'.")),
      }
    }
    Ok(())
  }

  /// Removes warnings based on the enabled flags.
  pub fn filter(&self, wrns: Vec<Warning>) -> Vec<Warning> {
    if self.all {
      // return the vec in case of the --errors flag be enabled
      return wrns;
    };
    wrns
      .into_iter()
      .filter(|w| {
        if self.match_only_vars {
          !matches!(w, Warning::MatchOnlyVars { .. })
        } else if self.unused_defs {
          !matches!(w, Warning::UnusedDefinition { .. })
        } else {
          true
        }
      })
      .collect_vec()
  }
}

pub struct CompileResult {
  pub core_book: hvmc::ast::Book,
  pub hvmc_names: HvmcNames,
  pub labels: Labels,
  pub warnings: Vec<Warning>,
}

Review comment (suggestion):
I think it would be better to have this be: `pub fn is_warning_enabled(&self, warn: &Warning) -> bool`.
Then you can do `wrns.iter().filter(|w| opt.is_warning_enabled(w))`.

Code after:
#[derive(Default, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
pub enum WarnState {
  #[default]
  Warn,
  Allow,
  Deny,
}

#[derive(clap::ValueEnum, Clone, Debug)]
pub enum WarningArgs {
  All,
  UnusedDefs,
  MatchOnlyVars,
}

impl WarningOpts {
  pub fn from_cli_opts(
    &mut self,
    wopts_id_seq: Vec<&clap::Id>,
    allows: &mut IntoIter<WarningArgs>,
    denies: &mut IntoIter<WarningArgs>,
    warns: &mut IntoIter<WarningArgs>,
  ) {
    for id in wopts_id_seq {
      match id.as_ref() {
        "allows" => self.set(allows.next().unwrap(), Self::allow_all(), WarnState::Allow),
        "denies" => self.set(denies.next().unwrap(), Self::deny_all(), WarnState::Deny),
        "warns" => self.set(warns.next().unwrap(), Self::default(), WarnState::Warn),
        _ => {}
      }
    }
  }

  pub fn allow_all() -> Self {
    Self { match_only_vars: WarnState::Allow, unused_defs: WarnState::Allow }
  }

  fn set(&mut self, val: WarningArgs, all: Self, switch: WarnState) {
    match val {
      WarningArgs::All => *self = all,
      WarningArgs::UnusedDefs => self.unused_defs = switch,
      WarningArgs::MatchOnlyVars => self.match_only_vars = switch,
    }
  }

  pub fn deny_all() -> Self {
    Self { match_only_vars: WarnState::Deny, unused_defs: WarnState::Deny }
  }

  /// Filters warnings based on the enabled flags.
  pub fn filter<'a>(&'a self, wrns: &'a [Warning], ws: WarnState) -> Vec<&Warning> {
Repository: HigherOrderCO/Bend
File: src/lib.rs (Rust)

Code before:
  /// Enables dereferences in `pre_reduce` pass.
  pub pre_reduce_refs: bool,
}

impl Opts {
  /// All optimizations enabled.
  pub fn heavy() -> Self {
    Self {
      eta: true,
      ref_to_ref: true,
      prune: true,
      pre_reduce: true,
      supercombinators: true,
      simplify_main: true,
      pre_reduce_refs: true,
    }
  }

  /// All optimizations disabled, except detach supercombinators.
  pub fn light() -> Self {
    Self { supercombinators: true, ..Self::default() }
  }
}

#[derive(clap::ValueEnum, Clone, Debug)]
pub enum OptArgs {
  All,
  NoAll,
  Eta,
  NoEta,
  Prune,
  NoPrune,
  RefToRef,
  NoRefToRef,
  PreReduce,
  NoPrereduce,
  Supercombinators,
  NoSupercombinators,
  SimplifyMain,
  NoSimplifyMain,
  PreReduceRefs,
  NoPreReduceRefs,
}

impl Opts {
  pub fn from_cli_opts(&mut self, values: Vec<OptArgs>) -> Result<(), String> {
    use OptArgs::*;
    for value in values {
      match value {
        All => *self = Opts::heavy(),
        NoAll => *self = Opts::default(),

Review comment (suggestion):
All this CLI interface code should be moved to `main.rs`.
hvml as a library shouldn't be aware of CLI

Code after:
  /// Enables dereferences in [hvmc_net::pre_reduce] pass.
  pub pre_reduce_refs: bool,

  /// Enables [term::transform::definition_merge]
  pub merge_definitions: bool,
}

impl Opts {
  /// All optimizations enabled.
  pub fn heavy() -> Self {
    Self {
      eta: true,
      ref_to_ref: true,
      prune: true,
      pre_reduce: true,
      supercombinators: true,
      simplify_main: true,
      pre_reduce_refs: true,
      merge_definitions: true,
    }
  }

  /// All optimizations disabled, except detach supercombinators.
  pub fn light() -> Self {
    Self { supercombinators: true, ..Self::default() }
  }
}

impl Opts {
  pub fn check(&self) {
    if !self.supercombinators {
      println!("Warning: Running in strict mode without enabling the supercombinators pass can lead to some functions expanding infinitely.");
    }
  }
}

#[derive(Default, Clone, Copy)]
pub struct WarningOpts {
  pub match_only_vars: WarnState,
  pub unused_defs: WarnState,
}

#[derive(Default, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
pub enum WarnState {
  #[default]
  Warn,
  Allow,
  Deny,
}

impl WarningOpts {
Repository: HigherOrderCO/Bend
File: src/main.rs (Rust)

Code before:

    #[arg(short = '1', help = "Single-core mode (no parallelism)")]
    single_core: bool,

    #[arg(short = 'l', help = "Linear readback (show explicit dups)")]
    linear: bool,

    #[arg(short, long = "stats", help = "Shows runtime stats and rewrite counts")]
    arg_stats: bool,

    #[arg(help = "Path to the input file")]
    path: PathBuf,

    #[arg(
      short = 'O',
      value_delimiter = ' ',
      action = clap::ArgAction::Append,
      long_help = r#"Enables or disables the given optimizations
      supercombinators is enabled by default."#,
    )]
    cli_opts: Vec<hvml::OptArgs>,

    #[command(flatten)]
    wopts: WOpts,
  },
  /// Runs the lambda-term level optimization passes.
  Desugar {
    #[arg(help = "Path to the input file")]
    path: PathBuf,
  },
}

#[derive(Args, Debug, Clone)]
#[group(multiple = true)]
struct WOpts {
  #[arg(
    short = 'W',
    long = "warn",
    value_delimiter = ' ',
    action = clap::ArgAction::Append,
    long_help = "Show compilation warnings",
  )]
  pub warns: Vec<hvml::WarningArgs>,

  #[arg(
    short = 'D',
    long = "deny",
    value_delimiter = ' ',
    action = clap::ArgAction::Append,
    long_help = "Deny compilation warnings",
  )]

Review comment (suggestion):
```suggestion
  /// Runs the lambda-term level desugaring passes.
```
Since we don't perform any optimizations here anymore

Code after:

    #[arg(short = '1', help = "Single-core mode (no parallelism)")]
    single_core: bool,

    #[arg(short = 'l', help = "Linear readback (show explicit dups)")]
    linear: bool,

    #[arg(short, long = "stats", help = "Shows runtime stats and rewrite counts")]
    arg_stats: bool,

    #[arg(help = "Path to the input file")]
    path: PathBuf,

    #[arg(
      short = 'O',
      value_delimiter = ' ',
      action = clap::ArgAction::Append,
      long_help = r#"Enables or disables the given optimizations
      supercombinators is enabled by default."#,
    )]
    cli_opts: Vec<OptArgs>,

    #[command(flatten)]
    wopts: WOpts,
  },
  /// Runs the lambda-term level desugaring passes.
  Desugar {
    #[arg(help = "Path to the input file")]
    path: PathBuf,
  },
}

#[derive(Args, Debug, Clone)]
#[group(multiple = true)]
struct WOpts {
  #[arg(
    short = 'W',
    long = "warn",
    value_delimiter = ' ',
    action = clap::ArgAction::Append,
    help = "Show the specified compilation warning",
  )]
  pub warns: Vec<WarningArgs>,

  #[arg(
    short = 'D',
    long = "deny",
    value_delimiter = ' ',
    action = clap::ArgAction::Append,
    help = "Deny the specified compilation warning",
  )]
Repository: HigherOrderCO/Bend
File: src/main.rs (Rust)

Code before:
      value_delimiter = ' ',
      action = clap::ArgAction::Append,
      long_help = r#"Enables or disables the given optimizations
      supercombinators is enabled by default."#,
    )]
    cli_opts: Vec<hvml::OptArgs>,

    #[command(flatten)]
    wopts: WOpts,
  },
  /// Runs the lambda-term level optimization passes.
  Desugar {
    #[arg(help = "Path to the input file")]
    path: PathBuf,
  },
}

#[derive(Args, Debug, Clone)]
#[group(multiple = true)]
struct WOpts {
  #[arg(
    short = 'W',
    long = "warn",
    value_delimiter = ' ',
    action = clap::ArgAction::Append,
    long_help = "Show compilation warnings",
  )]
  pub warns: Vec<hvml::WarningArgs>,

  #[arg(
    short = 'D',
    long = "deny",
    value_delimiter = ' ',
    action = clap::ArgAction::Append,
    long_help = "Deny compilation warnings",
  )]
  pub denies: Vec<hvml::WarningArgs>,

  #[arg(
    short = 'A',
    long = "allow",
    value_delimiter = ' ',
    action = clap::ArgAction::Append,
    long_help = "Allow compilation warnings",
  )]
  pub allows: Vec<hvml::WarningArgs>,
}

fn mem_parser(arg: &str) -> Result<usize, String> {
  let (base, mult) = match arg.to_lowercase().chars().last() {
    None => return Err("Mem size argument is empty".to_string()),

Review comment (suggestion):
From just the help this is a bit ambiguous and seems more like it will show/allow/deny all warnings.

It would be better along the lines of "Show the specified compilation warning".

It would also be nice if the help command could show all the possible values somewhere and group these three options together somehow, eg:
"-W/D/A --warn/deny/allow : Show/Deny/Allow compilation warnings".
But I don't know how feasible that would be with clap

Code after:
      value_delimiter = ' ',
      action = clap::ArgAction::Append,
      long_help = r#"Enables or disables the given optimizations
      supercombinators is enabled by default."#,
    )]
    cli_opts: Vec<OptArgs>,

    #[command(flatten)]
    wopts: WOpts,
  },
  /// Runs the lambda-term level desugaring passes.
  Desugar {
    #[arg(help = "Path to the input file")]
    path: PathBuf,
  },
}

#[derive(Args, Debug, Clone)]
#[group(multiple = true)]
struct WOpts {
  #[arg(
    short = 'W',
    long = "warn",
    value_delimiter = ' ',
    action = clap::ArgAction::Append,
    help = "Show the specified compilation warning",
  )]
  pub warns: Vec<WarningArgs>,

  #[arg(
    short = 'D',
    long = "deny",
    value_delimiter = ' ',
    action = clap::ArgAction::Append,
    help = "Deny the specified compilation warning",
  )]
  pub denies: Vec<WarningArgs>,

  #[arg(
    short = 'A',
    long = "allow",
    value_delimiter = ' ',
    action = clap::ArgAction::Append,
    help = "Allow the specified compilation warning",
  )]
  pub allows: Vec<WarningArgs>,
}

fn mem_parser(arg: &str) -> Result<usize, String> {
  let (base, mult) = match arg.to_lowercase().chars().last() {
    None => return Err("Mem size argument is empty".to_string()),