Update how warns are displayed, add repeated bind warning

src/diagnostics.rs

@@ -1,7 +1,7 @@
 use crate::term::{
   check::{
-    set_entrypoint::EntryErr, shared_names::TopLevelErr, unbound_pats::UnboundCtrErr,
-    unbound_vars::UnboundVarErr,
+    repeated_bind::RepeatedBindWarn, set_entrypoint::EntryErr, shared_names::TopLevelErr,
+    unbound_pats::UnboundCtrErr, unbound_vars::UnboundVarErr,
   },
   display::DisplayFn,
   transform::{
@@ -23,7 +23,7 @@ pub struct Info {
   err_counter: usize,
   errs: Vec<Error>,
   errs_with_def: BTreeMap<Name, Vec<Error>>,
-  pub warns: Vec<Warning>,
+  pub warns: Warnings,
 }
 
 impl Info {
@@ -67,12 +67,16 @@ impl Info {
     if self.err_counter == 0 { Ok(t) } else { Err(std::mem::take(self)) }
   }
 
+  pub fn warning<W: Into<WarningType>>(&mut self, def_name: Name, warning: W) {
+    self.warns.0.entry(def_name).or_default().push(warning.into());
+  }
+
   pub fn display(&self, verbose: bool) -> impl Display + '_ {
     DisplayFn(move |f| {
       writeln!(f, "{}", self.errs.iter().map(|err| err.display(verbose)).join("\n"))?;
 
       for (def_name, errs) in &self.errs_with_def {
-        writeln!(f, "In definition '{def_name}':")?;
+        in_definition(def_name, f)?;
         for err in errs {
           writeln!(f, "{:ERR_INDENT_SIZE$}{}", "", err.display(verbose))?;
         }
@@ -83,6 +87,10 @@ impl Info {
   }
 }
 
+fn in_definition(def_name: &Name, f: &mut Formatter<'_>) -> Result<(), std::fmt::Error> {
+  writeln!(f, "In definition '{def_name}':")
+}
+
 impl Display for Info {
   fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
     write!(f, "{}", self.display(false))
@@ -112,6 +120,7 @@ pub enum Error {
   TopLevel(TopLevelErr),
   Custom(String),
   ArgError(ArgError),
+  RepeatedBind(RepeatedBindWarn),
 }
 
 impl Display for Error {
@@ -132,6 +141,7 @@ impl Error {
       Error::TopLevel(err) => write!(f, "{err}"),
       Error::Custom(err) => write!(f, "{err}"),
       Error::ArgError(err) => write!(f, "{err}"),
+      Error::RepeatedBind(err) => write!(f, "{err}"),
     })
   }
 }
@@ -184,19 +194,41 @@ impl From<ArgError> for Error {
   }
 }
 
+#[derive(Debug, Clone, Default)]
+pub struct Warnings(pub BTreeMap<Name, Vec<WarningType>>);
+
 #[derive(Debug, Clone)]
-pub enum Warning {
-  MatchOnlyVars(Name),
-  UnusedDefinition(Name),
+pub enum WarningType {
+  MatchOnlyVars,
+  UnusedDefinition,
+  RepeatedBind(RepeatedBindWarn),
 }
 
-impl Display for Warning {
+impl Display for WarningType {
   fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
     match self {
-      Warning::MatchOnlyVars(def_name) => {
-        write!(f, "Match expression at definition '{def_name}' only uses var patterns.")
+      WarningType::MatchOnlyVars => write!(f, "Match expression at definition only uses var patterns."),
+      WarningType::UnusedDefinition => write!(f, "Definition is unusued."),
+      WarningType::RepeatedBind(warn) => write!(f, "{warn}"),
+    }
+  }
+}
+
+impl Display for Warnings {
+  fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+    for (def_name, warns) in &self.0 {
+      in_definition(def_name, f)?;
+      for warn in warns {
+        writeln!(f, "{:ERR_INDENT_SIZE$}{}", "", warn)?;
       }
-      Warning::UnusedDefinition(def_name) => write!(f, "Unused definition '{def_name}'."),
     }
+
+    Ok(())
+  }
+}
+
+impl From<RepeatedBindWarn> for WarningType {
+  fn from(value: RepeatedBindWarn) -> Self {
+    Self::RepeatedBind(value)
   }
 }

src/lib.rs

@@ -1,7 +1,7 @@
 #![feature(box_patterns)]
 #![feature(let_chains)]
 
-use diagnostics::{Info, Warning};
+use diagnostics::{Info, WarningType, Warnings};
 use hvmc::{
   ast::{self, Net},
   dispatch_dyn_net,
@@ -17,11 +17,8 @@ use std::{
   time::Instant,
 };
 use term::{
-  book_to_nets,
-  display::{display_readback_errors, DisplayJoin},
-  net_to_term::net_to_term,
-  term_to_net::Labels,
-  AdtEncoding, Book, Ctx, ReadbackError, Term,
+  book_to_nets, display::display_readback_errors, net_to_term::net_to_term, term_to_net::Labels, AdtEncoding,
+  Book, Ctx, ReadbackError, Term,
 };
 
 pub mod diagnostics;
@@ -107,11 +104,7 @@ pub fn compile_book(
   Ok(CompileResult { core_book, labels, warns })
 }
 
-pub fn desugar_book(
-  book: &mut Book,
-  opts: CompileOpts,
-  args: Option<Vec<Term>>,
-) -> Result<Vec<Warning>, Info> {
+pub fn desugar_book(book: &mut Book, opts: CompileOpts, args: Option<Vec<Term>>) -> Result<Warnings, Info> {
   let mut ctx = Ctx::new(book);
 
   ctx.check_shared_names();
@@ -123,6 +116,7 @@ pub fn desugar_book(
 
   ctx.book.resolve_ctrs_in_pats();
   ctx.resolve_refs()?;
+  ctx.check_repeated_binds();
 
   ctx.check_match_arity()?;
   ctx.check_unbound_pats()?;
@@ -194,7 +188,7 @@ pub fn run_book(
   let book = Arc::new(book);
   let labels = Arc::new(labels);
 
-  display_warnings(&warns, warning_opts)?;
+  display_warnings(warns, warning_opts)?;
 
   // Run
   let debug_hook = run_opts.debug_hook(&book, &labels);
@@ -461,6 +455,7 @@ impl CompileOpts {
 pub struct WarningOpts {
   pub match_only_vars: WarnState,
   pub unused_defs: WarnState,
+  pub repeated_bind: WarnState,
 }
 
 #[derive(Default, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
@@ -473,65 +468,77 @@ pub enum WarnState {
 
 impl WarningOpts {
   pub fn allow_all() -> Self {
-    Self { match_only_vars: WarnState::Allow, unused_defs: WarnState::Allow }
+    Self { match_only_vars: WarnState::Allow, unused_defs: WarnState::Allow, repeated_bind: WarnState::Allow }
   }
 
   pub fn deny_all() -> Self {
-    Self { match_only_vars: WarnState::Deny, unused_defs: WarnState::Deny }
+    Self { match_only_vars: WarnState::Deny, unused_defs: WarnState::Deny, repeated_bind: WarnState::Deny }
   }
 
   pub fn warn_all() -> Self {
-    Self { match_only_vars: WarnState::Warn, unused_defs: WarnState::Warn }
+    Self { match_only_vars: WarnState::Warn, unused_defs: WarnState::Warn, repeated_bind: WarnState::Warn }
   }
 
-  /// Filters warnings based on the enabled flags.
-  pub fn filter<'a>(&'a self, warns: &'a [Warning], ws: WarnState) -> Vec<&Warning> {
+  /// Split warnings into two based on its Warn or Deny WarnState.
+  pub fn split(&self, warns: Warnings) -> (Warnings, Warnings) {
+    let mut warn = Warnings::default();
+    let mut deny = Warnings::default();
+
     warns
-      .iter()
-      .filter(|w| {
-        (match w {
-          Warning::MatchOnlyVars(_) => self.match_only_vars,
-          Warning::UnusedDefinition(_) => self.unused_defs,
-        }) == ws
-      })
-      .collect()
+      .0
+      .into_iter()
+      .flat_map(|(def, warns)| warns.into_iter().map(move |warn| (def.clone(), warn)))
+      .for_each(|(def, w)| {
+        let ws = match w {
+          WarningType::MatchOnlyVars => self.match_only_vars,
+          WarningType::UnusedDefinition => self.unused_defs,
+          WarningType::RepeatedBind(_) => self.repeated_bind,
+        };
+
+        match ws {
+          WarnState::Allow => {}
+          WarnState::Warn => warn.0.entry(def).or_default().push(w),
+          WarnState::Deny => deny.0.entry(def).or_default().push(w),
+        };
+      });
+
+    (warn, deny)
   }
 }
 
 /// Either just prints warnings or returns Err when any denied was produced.
-pub fn display_warnings(warnings: &[Warning], warning_opts: WarningOpts) -> Result<(), String> {
-  let warns = warning_opts.filter(warnings, WarnState::Warn);
-  if !warns.is_empty() {
-    eprintln!("Warnings:\n{}", DisplayJoin(|| warns.iter(), "\n"));
+pub fn display_warnings(warnings: Warnings, warning_opts: WarningOpts) -> Result<(), String> {
+  let (warns, denies) = warning_opts.split(warnings);
+
+  if !warns.0.is_empty() {
+    eprintln!("Warnings:\n{}", warns);
   }
-  let denies = warning_opts.filter(warnings, WarnState::Deny);
-  if !denies.is_empty() {
-    return Err(format!(
-      "{}\nCould not run the code because of the previous warnings",
-      DisplayJoin(|| denies.iter(), "\n")
-    ));
+
+  if !denies.0.is_empty() {
+    return Err(format!("{denies}\nCould not run the code because of the previous warnings."));
   }
+
   Ok(())
 }
 
 pub struct CompileResult {
-  pub warns: Vec<Warning>,
+  pub warns: Warnings,
   pub core_book: hvmc::ast::Book,
   pub labels: Labels,
 }
 
 impl std::fmt::Debug for CompileResult {
   fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-    for warn in &self.warns {
-      writeln!(f, "// WARNING: {}", warn)?;
+    if !self.warns.0.is_empty() {
+      writeln!(f, "// WARNING:\n{}", self.warns)?;
     }
     write!(f, "{}", self.core_book)
   }
 }
 
 impl CompileResult {
-  pub fn display_with_warns(&self, opts: WarningOpts) -> Result<String, String> {
-    display_warnings(&self.warns, opts)?;
+  pub fn display_with_warns(self, opts: WarningOpts) -> Result<String, String> {
+    display_warnings(self.warns, opts)?;
     Ok(self.core_book.to_string())
   }
 }

src/term/check/mod.rs

@@ -1,5 +1,6 @@
 pub mod ctrs_arities;
 pub mod match_arity;
+pub mod repeated_bind;
 pub mod set_entrypoint;
 pub mod shared_names;
 pub mod type_check;

src/term/check/repeated_bind.rs

@@ -0,0 +1,70 @@
+use crate::term::{Ctx, Name, Term};
+use std::{collections::HashSet, fmt::Display};
+
+#[derive(Debug, Clone)]
+pub enum RepeatedBindWarn {
+  Rule(Name),
+  Match(Name),
+}
+
+impl Display for RepeatedBindWarn {
+  fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+    match self {
+      RepeatedBindWarn::Rule(bind) => write!(f, "Repeated bind inside rule pattern: '{bind}'."),
+      RepeatedBindWarn::Match(bind) => write!(f, "Repeated bind inside match arm: '{bind}'."),
+    }
+  }
+}
+
+impl Ctx<'_> {
+  /// Checks that there are no unbound variables in all definitions.
+  pub fn check_repeated_binds(&mut self) {
+    for (def_name, def) in &self.book.defs {
+      for rule in &def.rules {
+        let mut binds = HashSet::new();
+        for pat in &rule.pats {
+          for nam in pat.named_binds() {
+            if !binds.insert(nam) {
+              self.info.warning(def_name.clone(), RepeatedBindWarn::Rule(nam.clone()));
+            }
+          }
+        }
+
+        let mut repeated_in_matches = Vec::new();
+        rule.body.check_repeated_binds(&mut repeated_in_matches);
+
+        for repeated in repeated_in_matches {
+          self.info.warning(def_name.clone(), repeated);
+        }
+      }
+    }
+  }
+}
+
+impl Term {
+  pub fn check_repeated_binds(&self, repeated: &mut Vec<RepeatedBindWarn>) {
+    Term::recursive_call(|| match self {
+      Term::Mat { args, rules } => {
+        for arg in args {
+          arg.check_repeated_binds(repeated);
+        }
+
+        for rule in rules {
+          let mut binds = HashSet::new();
+          for pat in &rule.pats {
+            for nam in pat.named_binds() {
+              if !binds.insert(nam) {
+                repeated.push(RepeatedBindWarn::Match(nam.clone()));
+              }
+            }
+          }
+        }
+      }
+      _ => {
+        for child in self.children() {
+          child.check_repeated_binds(repeated);
+        }
+      }
+    })
+  }
+}

src/term/transform/definition_pruning.rs

@@ -1,7 +1,7 @@
 use std::collections::{hash_map::Entry, HashMap};
 
 use crate::{
-  diagnostics::Warning,
+  diagnostics::WarningType,
   term::{Adt, AdtEncoding, Book, Ctx, Name, Tag, Term, LIST, STRING},
   CORE_BUILTINS,
 };
@@ -72,7 +72,7 @@ impl Ctx<'_> {
       if prune_all || def.builtin {
         self.book.defs.swap_remove(&def_name);
       } else if !def_name.is_generated() {
-        self.info.warns.push(Warning::UnusedDefinition(def_name.clone()));
+        self.info.warning(def_name.clone(), WarningType::UnusedDefinition);
       }
     }
   }

tests/golden_tests/compile_file/repeated_bind_match.hvm

@@ -0,0 +1,4 @@
+main = @a match a {
+ (List.cons x x): x
+ (List.nil)     : *
+}

tests/golden_tests/compile_file/repeated_bind_rule.hvm

@@ -0,0 +1,3 @@
+Foo a a = a
+
+main = (Foo 1 2)

tests/snapshots/compile_file__crlf.hvm.snap

@@ -2,6 +2,9 @@
 source: tests/golden_tests.rs
 input_file: tests/golden_tests/compile_file/crlf.hvm
 ---
-// WARNING: Unused definition 'a'.
+// WARNING:
+In definition 'a':
+  Definition is unusued.
+
 @a = #1
 @main = #2