feat(experimental): Issue errors for unreachable match branches

compiler/noirc_frontend/src/elaborator/enums.rs

@@ -27,6 +27,78 @@
 
 use super::Elaborator;
 
+struct MatchCompiler<'elab, 'ctx> {
+    elaborator: &'elab mut Elaborator<'ctx>,
+    has_missing_cases: bool,
+    // We iterate on this to issue errors later so it needs to be a BTreeMap (versus HashMap) to be
+    // deterministic.
+    unreachable_cases: BTreeMap<RowBody, Location>,
+}
+
+/// A Pattern is anything that can appear before the `=>` in a match rule.
+#[derive(Debug, Clone)]
+enum Pattern {
+    /// A pattern checking for a tag and possibly binding variables such as `Some(42)`
+    Constructor(Constructor, Vec<Pattern>),
+    /// An integer literal pattern such as `4`, `12345`, or `-56`
+    Int(SignedField),
+    /// A pattern binding a variable such as `a` or `_`
+    Binding(DefinitionId),
+
+    /// Multiple patterns combined with `|` where we should match this pattern if any
+    /// constituent pattern matches. e.g. `Some(3) | None` or `Some(1) | Some(2) | None`
+    #[allow(unused)]
+    Or(Vec<Pattern>),
+
+    /// An integer range pattern such as `1..20` which will match any integer n such that
+    /// 1 <= n < 20.
+    #[allow(unused)]
+    Range(SignedField, SignedField),
+
+    /// An error occurred while translating this pattern. This Pattern kind always translates
+    /// to a Fail branch in the decision tree, although the compiler is expected to halt
+    /// with errors before execution.
+    Error,
+}
+
+#[derive(Clone)]
+struct Column {
+    variable_to_match: DefinitionId,
+    pattern: Pattern,
+}
+
+impl Column {
+    fn new(variable_to_match: DefinitionId, pattern: Pattern) -> Self {
+        Column { variable_to_match, pattern }
+    }
+}
+
+#[derive(Clone)]
+pub(super) struct Row {
+    columns: Vec<Column>,
+    guard: Option<RowBody>,
+    body: RowBody,
+    original_body: RowBody,
+    location: Location,
+}
+
+type RowBody = ExprId;
+
+impl Row {
+    fn new(columns: Vec<Column>, guard: Option<RowBody>, body: RowBody, location: Location) -> Row {
+        Row { columns, guard, body, original_body: body, location }
+    }
+}
+
+impl Row {
+    fn remove_column(&mut self, variable: DefinitionId) -> Option<Column> {
+        self.columns
+            .iter()
+            .position(|c| c.variable_to_match == variable)
+            .map(|idx| self.columns.remove(idx))
+    }
+}
+
 impl Elaborator<'_> {
     /// Defines the value of an enum variant that we resolve an enum
     /// variant expression to. E.g. `Foo::Bar` in `Foo::Bar(baz)`.
@@ -273,6 +345,7 @@
 
         let rows = vecmap(rules, |(pattern, branch)| {
             self.push_scope();
+            let pattern_location = pattern.location;
             let pattern =
                 self.expression_to_pattern(pattern, &expected_pattern_type, &mut Vec::new());
             let columns = vec![Column::new(variable_to_match, pattern)];
@@ -288,7 +361,7 @@
             });
 
             self.pop_scope();
-            Row::new(columns, guard, body)
+            Row::new(columns, guard, body, pattern_location)
         });
         (rows, result_type)
     }
@@ -524,8 +597,7 @@
                     ),
                     Err(error) => {
                         self.push_err(error);
-                        let id = self.fresh_match_variable(expected_type.clone(), location);
-                        Pattern::Binding(id)
+                        Pattern::Error
                     }
                 }
             }
@@ -722,16 +794,38 @@
     /// This is an adaptation of https://github.com/yorickpeterse/pattern-matching-in-rust/tree/main/jacobs2021
     /// which is an implementation of https://julesjacobs.com/notes/patternmatching/patternmatching.pdf
     pub(super) fn elaborate_match_rows(&mut self, rows: Vec<Row>) -> HirMatch {
-        self.compile_rows(rows).unwrap_or_else(|error| {
-            self.push_err(error);
-            HirMatch::Failure
-        })
+        MatchCompiler::run(self, rows)
+    }
+}
+
+impl<'elab, 'ctx> MatchCompiler<'elab, 'ctx> {
+    fn run(elaborator: &'elab mut Elaborator<'ctx>, rows: Vec<Row>) -> HirMatch {
+        let mut compiler = Self {
+            elaborator,
+            has_missing_cases: false,
+            unreachable_cases: rows.iter().map(|row| (row.body, row.location)).collect(),
+        };
+
+        let hir_match = compiler.compile_rows(rows).unwrap_or_else(|error| {
+            compiler.elaborator.push_err(error);
+            HirMatch::Failure { missing_case: false }
+        });
+
+        if compiler.has_missing_cases {
+            compiler.issue_missing_cases_error(&hir_match);
+        }
+
+        if !compiler.unreachable_cases.is_empty() {
+            compiler.issue_unreachable_cases_warning();
+        }
+
+        hir_match
     }
 
     fn compile_rows(&mut self, mut rows: Vec<Row>) -> Result<HirMatch, ResolverError> {
         if rows.is_empty() {
-            eprintln!("Warning: missing case");
-            return Ok(HirMatch::Failure);
+            self.has_missing_cases = true;
+            return Ok(HirMatch::Failure { missing_case: true });
         }
 
         self.push_tests_against_bare_variables(&mut rows);
@@ -741,7 +835,10 @@
             let row = rows.remove(0);
 
             return Ok(match row.guard {
-                None => HirMatch::Success(row.body),
+                None => {
+                    self.unreachable_cases.remove(&row.original_body);
+                    HirMatch::Success(row.body)
+                }
                 Some(cond) => {
                     let remaining = self.compile_rows(rows)?;
                     HirMatch::Guard { cond, body: row.body, otherwise: Box::new(remaining) }
@@ -750,9 +847,10 @@
         }
 
         let branch_var = self.branch_variable(&rows);
-        let location = self.interner.definition(branch_var).location;
+        let location = self.elaborator.interner.definition(branch_var).location;
 
-        match self.interner.definition_type(branch_var).follow_bindings_shallow().into_owned() {
+        let definition_type = self.elaborator.interner.definition_type(branch_var);
+        match definition_type.follow_bindings_shallow().into_owned() {
             Type::FieldElement | Type::Integer(_, _) => {
                 let (cases, fallback) = self.compile_int_cases(rows, branch_var)?;
                 Ok(HirMatch::Switch(branch_var, cases, Some(fallback)))
@@ -849,14 +947,14 @@
     fn fresh_match_variable(&mut self, variable_type: Type, location: Location) -> DefinitionId {
         let name = "internal_match_variable".to_string();
         let kind = DefinitionKind::Local(None);
-        let id = self.interner.push_definition(name, false, false, kind, location);
-        self.interner.push_definition_type(id, variable_type);
+        let id = self.elaborator.interner.push_definition(name, false, false, kind, location);
+        self.elaborator.interner.push_definition_type(id, variable_type);
         id
     }
 
     /// Compiles the cases and fallback cases for integer and range patterns.
     ///
     /// Integers have an infinite number of constructors, so we specialise the
     /// compilation of integer and range patterns.
     fn compile_int_cases(
         &mut self,
@@ -946,7 +1044,7 @@
                         cols.push(Column::new(*var, pat));
                     }
 
-                    cases[idx].2.push(Row::new(cols, row.guard, row.body));
+                    cases[idx].2.push(Row::new(cols, row.guard, row.body, row.location));
                 }
             } else {
                 for (_, _, rows) in &mut cases {
@@ -1045,16 +1143,16 @@
     /// Creates:
     /// `{ let <variable> = <rhs>; <body> }`
     fn let_binding(&mut self, variable: DefinitionId, rhs: DefinitionId, body: ExprId) -> ExprId {
-        let location = self.interner.definition(rhs).location;
+        let location = self.elaborator.interner.definition(rhs).location;
 
-        let r#type = self.interner.definition_type(variable);
-        let rhs_type = self.interner.definition_type(rhs);
+        let r#type = self.elaborator.interner.definition_type(variable);
+        let rhs_type = self.elaborator.interner.definition_type(rhs);
         let variable = HirIdent::non_trait_method(variable, location);
 
         let rhs = HirExpression::Ident(HirIdent::non_trait_method(rhs, location), None);
-        let rhs = self.interner.push_expr(rhs);
-        self.interner.push_expr_type(rhs, rhs_type);
-        self.interner.push_expr_location(rhs, location);
+        let rhs = self.elaborator.interner.push_expr(rhs);
+        self.elaborator.interner.push_expr_type(rhs, rhs_type);
+        self.elaborator.interner.push_expr_location(rhs, location);
 
         let let_ = HirStatement::Let(HirLetStatement {
             pattern: HirPattern::Identifier(variable),
@@ -1065,77 +1163,31 @@
             is_global_let: false,
         });
 
-        let body_type = self.interner.id_type(body);
-        let let_ = self.interner.push_stmt(let_);
-        let body = self.interner.push_stmt(HirStatement::Expression(body));
+        let body_type = self.elaborator.interner.id_type(body);
+        let let_ = self.elaborator.interner.push_stmt(let_);
+        let body = self.elaborator.interner.push_stmt(HirStatement::Expression(body));
 
-        self.interner.push_stmt_location(let_, location);
-        self.interner.push_stmt_location(body, location);
+        self.elaborator.interner.push_stmt_location(let_, location);
+        self.elaborator.interner.push_stmt_location(body, location);
 
         let block = HirExpression::Block(HirBlockExpression { statements: vec![let_, body] });
-        let block = self.interner.push_expr(block);
-        self.interner.push_expr_type(block, body_type);
-        self.interner.push_expr_location(block, location);
+        let block = self.elaborator.interner.push_expr(block);
+        self.elaborator.interner.push_expr_type(block, body_type);
+        self.elaborator.interner.push_expr_location(block, location);
         block
     }
-}
-
-/// A Pattern is anything that can appear before the `=>` in a match rule.
-#[derive(Debug, Clone)]
-enum Pattern {
-    /// A pattern checking for a tag and possibly binding variables such as `Some(42)`
-    Constructor(Constructor, Vec<Pattern>),
-    /// An integer literal pattern such as `4`, `12345`, or `-56`
-    Int(SignedField),
-    /// A pattern binding a variable such as `a` or `_`
-    Binding(DefinitionId),
-
-    /// Multiple patterns combined with `|` where we should match this pattern if any
-    /// constituent pattern matches. e.g. `Some(3) | None` or `Some(1) | Some(2) | None`
-    #[allow(unused)]
-    Or(Vec<Pattern>),
-
-    /// An integer range pattern such as `1..20` which will match any integer n such that
-    /// 1 <= n < 20.
-    #[allow(unused)]
-    Range(SignedField, SignedField),
-
-    /// An error occurred while translating this pattern. This Pattern kind always translates
-    /// to a Fail branch in the decision tree, although the compiler is expected to halt
-    /// with errors before execution.
-    Error,
-}
-
-#[derive(Clone)]
-struct Column {
-    variable_to_match: DefinitionId,
-    pattern: Pattern,
-}
-
-impl Column {
-    fn new(variable_to_match: DefinitionId, pattern: Pattern) -> Self {
-        Column { variable_to_match, pattern }
-    }
-}
-
-#[derive(Clone)]
-pub(super) struct Row {
-    columns: Vec<Column>,
-    guard: Option<ExprId>,
-    body: ExprId,
-}
 
-impl Row {
-    fn new(columns: Vec<Column>, guard: Option<ExprId>, body: ExprId) -> Row {
-        Row { columns, guard, body }
+    /// Traverse the resulting HirMatch to build counter-examples of values which would
+    /// not be covered by the match.
+    fn issue_missing_cases_error(&self, _tree: &HirMatch) {
+        eprintln!("Missing case(s)!");
     }
-}
 
-impl Row {
-    fn remove_column(&mut self, variable: DefinitionId) -> Option<Column> {
-        self.columns
-            .iter()
-            .position(|c| c.variable_to_match == variable)
-            .map(|idx| self.columns.remove(idx))
+    /// Any case that isn't branched to when the match is finished must be covered by another
+    /// case and is thus redundant.
+    fn issue_unreachable_cases_warning(&mut self) {
+        for location in self.unreachable_cases.values().copied() {
+            self.elaborator.push_err(TypeCheckError::UnreachableCase { location });
+        }
     }
 }

compiler/noirc_frontend/src/elaborator/expressions.rs

@@ -26,7 +26,8 @@ use crate::{
             HirArrayLiteral, HirBinaryOp, HirBlockExpression, HirCallExpression, HirCastExpression,
             HirConstrainExpression, HirConstructorExpression, HirExpression, HirIdent,
             HirIfExpression, HirIndexExpression, HirInfixExpression, HirLambda, HirLiteral,
-            HirMemberAccess, HirMethodCallExpression, HirPrefixExpression, ImplKind, TraitMethod,
+            HirMatch, HirMemberAccess, HirMethodCallExpression, HirPrefixExpression, ImplKind,
+            TraitMethod,
         },
         stmt::{HirLetStatement, HirPattern, HirStatement},
         traits::{ResolvedTraitBound, TraitConstraint},
@@ -1057,8 +1058,18 @@ impl Elaborator<'_> {
         let (expression, typ) = self.elaborate_expression(match_expr.expression);
         let (let_, variable) = self.wrap_in_let(expression, typ);
 
-        let (rows, result_type) = self.elaborate_match_rules(variable, match_expr.rules);
-        let tree = HirExpression::Match(self.elaborate_match_rows(rows));
+        let (errored, (rows, result_type)) =
+            self.errors_occurred_in(|this| this.elaborate_match_rules(variable, match_expr.rules));
+
+        // Avoid calling `elaborate_match_rows` if there were errors while constructing
+        // the match rows - it'll just lead to extra errors like `unreachable pattern`
+        // warnings on branches which previously had type errors.
+        let tree = HirExpression::Match(if !errored {
+            self.elaborate_match_rows(rows)
+        } else {
+            HirMatch::Failure { missing_case: false }
+        });
+
         let tree = self.interner.push_expr(tree);
         self.interner.push_expr_type(tree, result_type.clone());
         self.interner.push_expr_location(tree, location);

compiler/noirc_frontend/src/elaborator/lints.rs

@@ -338,7 +338,7 @@ fn can_return_without_recursing_match(
 
     match match_expr {
         HirMatch::Success(expr) => check(*expr),
-        HirMatch::Failure => true,
+        HirMatch::Failure { .. } => true,
         HirMatch::Guard { cond: _, body, otherwise } => check(*body) && check_match(otherwise),
         HirMatch::Switch(_, cases, otherwise) => {
             cases.iter().all(|case| check_match(&case.body))

compiler/noirc_frontend/src/elaborator/mod.rs

@@ -2163,4 +2163,13 @@ impl<'context> Elaborator<'context> {
             self.push_err(ParserError::with_reason(reason, location));
         }
     }
+
+    /// Run the given function using the resolver and return true if any errors (not warnings)
+    /// occurred while running it.
+    pub fn errors_occurred_in<T>(&mut self, f: impl FnOnce(&mut Self) -> T) -> (bool, T) {
+        let previous_errors = self.errors.len();
+        let ret = f(self);
+        let errored = self.errors.iter().skip(previous_errors).any(|error| error.is_error());
+        (errored, ret)
+    }
 }

compiler/noirc_frontend/src/elaborator/types.rs

@@ -766,8 +766,7 @@ impl Elaborator<'_> {
         make_error: impl FnOnce() -> TypeCheckError,
     ) {
         if let Err(UnificationError) = actual.unify(expected) {
-            let error: CompilationError = make_error().into();
-            self.push_err(error);
+            self.push_err(make_error());
         }
     }
 

compiler/noirc_frontend/src/hir/comptime/hir_to_display_ast.rs

@@ -233,7 +233,7 @@ impl HirMatch {
     fn to_display_ast(&self, interner: &NodeInterner, location: Location) -> ExpressionKind {
         match self {
             HirMatch::Success(expr) => expr.to_display_ast(interner).kind,
-            HirMatch::Failure => ExpressionKind::Error,
+            HirMatch::Failure { .. } => ExpressionKind::Error,
             HirMatch::Guard { cond, body, otherwise } => {
                 let condition = cond.to_display_ast(interner);
                 let consequence = body.to_display_ast(interner);