diff --git a/compiler/noirc_frontend/src/elaborator/enums.rs b/compiler/noirc_frontend/src/elaborator/enums.rs index 82728cd46f0..c0bc86b51b0 100644 --- a/compiler/noirc_frontend/src/elaborator/enums.rs +++ b/compiler/noirc_frontend/src/elaborator/enums.rs @@ -1,8 +1,9 @@ -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; use fxhash::FxHashMap as HashMap; -use iter_extended::{try_vecmap, vecmap}; +use iter_extended::{btree_map, try_vecmap, vecmap}; use noirc_errors::Location; +use rangemap::StepLite; use crate::{ DataType, Kind, Shared, Type, @@ -27,6 +28,8 @@ use crate::{ use super::Elaborator; +const WILDCARD_PATTERN: &str = "_"; + struct MatchCompiler<'elab, 'ctx> { elaborator: &'elab mut Elaborator<'ctx>, has_missing_cases: bool, @@ -506,7 +509,7 @@ impl Elaborator<'_> { if let Some(existing) = variables_defined.iter().find(|elem| *elem == &name) { // Allow redefinition of `_` only, to ignore variables - if name.0.contents != "_" { + if name.0.contents != WILDCARD_PATTERN { self.push_err(ResolverError::VariableAlreadyDefinedInPattern { existing: existing.clone(), new_location: name.location(), @@ -793,13 +796,23 @@ impl Elaborator<'_> { /// /// 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) -> HirMatch { - MatchCompiler::run(self, rows) + pub(super) fn elaborate_match_rows( + &mut self, + rows: Vec, + type_matched_on: &Type, + location: Location, + ) -> HirMatch { + MatchCompiler::run(self, rows, type_matched_on, location) } } impl<'elab, 'ctx> MatchCompiler<'elab, 'ctx> { - fn run(elaborator: &'elab mut Elaborator<'ctx>, rows: Vec) -> HirMatch { + fn run( + elaborator: &'elab mut Elaborator<'ctx>, + rows: Vec, + type_matched_on: &Type, + location: Location, + ) -> HirMatch { let mut compiler = Self { elaborator, has_missing_cases: false, @@ -812,7 +825,7 @@ impl<'elab, 'ctx> MatchCompiler<'elab, 'ctx> { }); if compiler.has_missing_cases { - compiler.issue_missing_cases_error(&hir_match); + compiler.issue_missing_cases_error(&hir_match, type_matched_on, location); } if !compiler.unreachable_cases.is_empty() { @@ -1177,12 +1190,6 @@ impl<'elab, 'ctx> MatchCompiler<'elab, 'ctx> { block } - /// 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)!"); - } - /// 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) { @@ -1190,4 +1197,164 @@ impl<'elab, 'ctx> MatchCompiler<'elab, 'ctx> { self.elaborator.push_err(TypeCheckError::UnreachableCase { location }); } } + + /// Traverse the resulting HirMatch to build counter-examples of values which would + /// not be covered by the match. + fn issue_missing_cases_error( + &mut self, + tree: &HirMatch, + type_matched_on: &Type, + location: Location, + ) { + let starting_id = match tree { + HirMatch::Switch(id, ..) => *id, + _ => return self.issue_missing_cases_error_for_type(type_matched_on, location), + }; + + let mut cases = BTreeSet::new(); + self.find_missing_values(tree, &mut Default::default(), &mut cases, starting_id); + + // It's possible to trigger this matching on an empty enum like `enum Void {}` + if !cases.is_empty() { + self.elaborator.push_err(TypeCheckError::MissingCases { cases, location }); + } + } + + /// Issue a missing cases error if necessary for the given type, assuming that no + /// case of the type is covered. This is the case for empty matches `match foo {}`. + /// Note that this is expected not to error if the given type is an enum with zero variants. + fn issue_missing_cases_error_for_type(&mut self, type_matched_on: &Type, location: Location) { + let typ = type_matched_on.follow_bindings_shallow(); + if let Type::DataType(shared, generics) = typ.as_ref() { + if let Some(variants) = shared.borrow().get_variants(generics) { + let cases: BTreeSet<_> = variants.into_iter().map(|(name, _)| name).collect(); + if !cases.is_empty() { + self.elaborator.push_err(TypeCheckError::MissingCases { cases, location }); + } + return; + } + } + let typ = typ.to_string(); + self.elaborator.push_err(TypeCheckError::MissingManyCases { typ, location }); + } + + fn find_missing_values( + &self, + tree: &HirMatch, + env: &mut HashMap)>, + missing_cases: &mut BTreeSet, + starting_id: DefinitionId, + ) { + match tree { + HirMatch::Success(_) | HirMatch::Failure { missing_case: false } => (), + HirMatch::Guard { otherwise, .. } => { + self.find_missing_values(otherwise, env, missing_cases, starting_id); + } + HirMatch::Failure { missing_case: true } => { + let case = Self::construct_missing_case(starting_id, env); + missing_cases.insert(case); + } + HirMatch::Switch(definition_id, cases, else_case) => { + for case in cases { + let name = case.constructor.to_string(); + env.insert(*definition_id, (name, case.arguments.clone())); + self.find_missing_values(&case.body, env, missing_cases, starting_id); + } + + if let Some(else_case) = else_case { + let typ = self.elaborator.interner.definition_type(*definition_id); + + for case in self.missing_cases(cases, &typ) { + env.insert(*definition_id, case); + self.find_missing_values(else_case, env, missing_cases, starting_id); + } + } + + env.remove(definition_id); + } + } + } + + fn missing_cases(&self, cases: &[Case], typ: &Type) -> Vec<(String, Vec)> { + // We expect `cases` to come from a `Switch` which should always have + // at least 2 cases, otherwise it should be a Success or Failure node. + let first = &cases[0]; + + if matches!(&first.constructor, Constructor::Int(_) | Constructor::Range(..)) { + return self.missing_integer_cases(cases, typ); + } + + let all_constructors = first.constructor.all_constructors(); + let mut all_constructors = + btree_map(all_constructors, |(constructor, arg_count)| (constructor, arg_count)); + + for case in cases { + all_constructors.remove(&case.constructor); + } + + vecmap(all_constructors, |(constructor, arg_count)| { + // Safety: this id should only be used in `env` of `find_missing_values` which + // only uses it for display and defaults to "_" on unknown ids. + let args = vecmap(0..arg_count, |_| DefinitionId::dummy_id()); + (constructor.to_string(), args) + }) + } + + fn missing_integer_cases( + &self, + cases: &[Case], + typ: &Type, + ) -> Vec<(String, Vec)> { + // We could give missed cases for field ranges of `0 .. field_modulus` but since the field + // used in Noir may change we recommend a match-all pattern instead. + // If the type is a type variable, we don't know exactly which integer type this may + // resolve to so also just suggest a catch-all in that case. + if typ.is_field() || typ.is_bindable() { + return vec![(WILDCARD_PATTERN.to_string(), Vec::new())]; + } + + let mut missing_cases = rangemap::RangeInclusiveSet::new(); + + let int_max = SignedField::positive(typ.integral_maximum_size().unwrap()); + let int_min = typ.integral_minimum_size().unwrap(); + missing_cases.insert(int_min..=int_max); + + for case in cases { + match &case.constructor { + Constructor::Int(signed_field) => { + missing_cases.remove(*signed_field..=*signed_field); + } + Constructor::Range(start, end) => { + // Our ranges are exclusive, so adjust for that + missing_cases.remove(*start..=end.sub_one()); + } + _ => unreachable!( + "missing_integer_cases should only be called with Int or Range constructors" + ), + } + } + + vecmap(missing_cases, |range| { + if range.start() == range.end() { + (format!("{}", range.start()), Vec::new()) + } else { + (format!("{}..={}", range.start(), range.end()), Vec::new()) + } + }) + } + + fn construct_missing_case( + starting_id: DefinitionId, + env: &HashMap)>, + ) -> String { + let Some((constructor, arguments)) = env.get(&starting_id) else { + return WILDCARD_PATTERN.to_string(); + }; + + let no_arguments = arguments.is_empty(); + + let args = vecmap(arguments, |arg| Self::construct_missing_case(*arg, env)).join(", "); + + if no_arguments { constructor.clone() } else { format!("{constructor}({args})") } + } } diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index 6622717644d..fb098dd59ba 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -1055,8 +1055,9 @@ impl Elaborator<'_> { ) -> (HirExpression, Type) { self.use_unstable_feature(super::UnstableFeature::Enums, location); + let expr_location = match_expr.expression.location; let (expression, typ) = self.elaborate_expression(match_expr.expression); - let (let_, variable) = self.wrap_in_let(expression, typ); + let (let_, variable) = self.wrap_in_let(expression, typ.clone()); let (errored, (rows, result_type)) = self.errors_occurred_in(|this| this.elaborate_match_rules(variable, match_expr.rules)); @@ -1065,7 +1066,7 @@ impl Elaborator<'_> { // 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) + self.elaborate_match_rows(rows, &typ, expr_location) } else { HirMatch::Failure { missing_case: false } }); diff --git a/compiler/noirc_frontend/src/hir/type_check/errors.rs b/compiler/noirc_frontend/src/hir/type_check/errors.rs index b867421686d..0d36ee52e1b 100644 --- a/compiler/noirc_frontend/src/hir/type_check/errors.rs +++ b/compiler/noirc_frontend/src/hir/type_check/errors.rs @@ -1,3 +1,4 @@ +use std::collections::BTreeSet; use std::rc::Rc; use acvm::FieldElement; @@ -15,6 +16,9 @@ use crate::hir_def::types::{BinaryTypeOperator, Kind, Type}; use crate::node_interner::NodeInterner; use crate::signed_field::SignedField; +/// Rust also only shows 3 maximum, even for short patterns. +pub const MAX_MISSING_CASES: usize = 3; + #[derive(Error, Debug, Clone, PartialEq, Eq)] pub enum Source { #[error("Binary")] @@ -237,6 +241,11 @@ pub enum TypeCheckError { NestedUnsafeBlock { location: Location }, #[error("Unreachable match case")] UnreachableCase { location: Location }, + #[error("Missing cases")] + MissingCases { cases: BTreeSet, location: Location }, + /// This error is used for types like integers which have too many variants to enumerate + #[error("Missing cases: `{typ}` is non-empty")] + MissingManyCases { typ: String, location: Location }, } #[derive(Debug, Clone, PartialEq, Eq)] @@ -324,6 +333,8 @@ impl TypeCheckError { | TypeCheckError::TypeAnnotationsNeededForIndex { location } | TypeCheckError::UnnecessaryUnsafeBlock { location } | TypeCheckError::UnreachableCase { location } + | TypeCheckError::MissingCases { location, .. } + | TypeCheckError::MissingManyCases { location, .. } | TypeCheckError::NestedUnsafeBlock { location } => *location, TypeCheckError::DuplicateNamedTypeArg { name: ident, .. } | TypeCheckError::NoSuchNamedTypeArg { name: ident, .. } => ident.location(), @@ -651,6 +662,29 @@ impl<'a> From<&'a TypeCheckError> for Diagnostic { *location, ) }, + TypeCheckError::MissingCases { cases, location } => { + let s = if cases.len() == 1 { "" } else { "s" }; + + let mut not_shown = String::new(); + let mut shown_cases = cases.iter() + .map(|s| format!("`{s}`")) + .take(MAX_MISSING_CASES) + .collect::>(); + + if cases.len() > MAX_MISSING_CASES { + shown_cases.truncate(MAX_MISSING_CASES); + not_shown = format!(", and {} more not shown", cases.len() - MAX_MISSING_CASES); + } + + let shown_cases = shown_cases.join(", "); + let msg = format!("Missing case{s}: {shown_cases}{not_shown}"); + Diagnostic::simple_error(msg, String::new(), *location) + }, + TypeCheckError::MissingManyCases { typ, location } => { + let msg = format!("Missing cases: `{typ}` is non-empty"); + let secondary = "Try adding a match-all pattern: `_`".to_string(); + Diagnostic::simple_error(msg, secondary, *location) + }, } } } diff --git a/compiler/noirc_frontend/src/hir/type_check/mod.rs b/compiler/noirc_frontend/src/hir/type_check/mod.rs index f45b68dd818..c7a7890fcc0 100644 --- a/compiler/noirc_frontend/src/hir/type_check/mod.rs +++ b/compiler/noirc_frontend/src/hir/type_check/mod.rs @@ -2,4 +2,4 @@ mod errors; pub mod generics; pub use self::errors::Source; -pub use errors::{NoMatchingImplFoundError, TypeCheckError}; +pub use errors::{MAX_MISSING_CASES, NoMatchingImplFoundError, TypeCheckError}; diff --git a/compiler/noirc_frontend/src/hir_def/expr.rs b/compiler/noirc_frontend/src/hir_def/expr.rs index 6cc518f6060..25e055188c2 100644 --- a/compiler/noirc_frontend/src/hir_def/expr.rs +++ b/compiler/noirc_frontend/src/hir_def/expr.rs @@ -1,4 +1,5 @@ use fm::FileId; +use iter_extended::vecmap; use noirc_errors::Location; use crate::Shared; @@ -385,7 +386,7 @@ impl Case { } } -#[derive(Debug, Clone, Hash, PartialEq, Eq)] +#[derive(Debug, Clone, Hash, PartialEq, Eq, PartialOrd, Ord)] pub enum Constructor { True, False, @@ -435,6 +436,41 @@ impl Constructor { _ => false, } } + + /// Return all the constructors of this type from one constructor. Intended to be used + /// for error reporting in cases where there are at least 2 constructors. + pub(crate) fn all_constructors(&self) -> Vec<(Constructor, /*arg count:*/ usize)> { + match self { + Constructor::True | Constructor::False => { + vec![(Constructor::True, 0), (Constructor::False, 0)] + } + Constructor::Unit => vec![(Constructor::Unit, 0)], + Constructor::Tuple(args) => vec![(self.clone(), args.len())], + Constructor::Variant(typ, _) => { + let typ = typ.follow_bindings(); + let Type::DataType(def, generics) = &typ else { + unreachable!( + "Constructor::Variant should have a DataType type, but found {typ:?}" + ); + }; + + let def_ref = def.borrow(); + if let Some(variants) = def_ref.get_variants(generics) { + vecmap(variants.into_iter().enumerate(), |(i, (_, fields))| { + (Constructor::Variant(typ.clone(), i), fields.len()) + }) + } else + /* def is a struct */ + { + let field_count = def_ref.fields_raw().map(|fields| fields.len()).unwrap_or(0); + vec![(Constructor::Variant(typ.clone(), 0), field_count)] + } + } + + // Nothing great to return for these + Constructor::Int(_) | Constructor::Range(..) => Vec::new(), + } + } } impl std::fmt::Display for Constructor { diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index 4fd5b46657e..406759544c1 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/compiler/noirc_frontend/src/hir_def/types.rs @@ -14,6 +14,7 @@ use crate::{ ast::{IntegerBitSize, ItemVisibility}, hir::type_check::{TypeCheckError, generics::TraitGenerics}, node_interner::{ExprId, NodeInterner, TraitId, TypeAliasId}, + signed_field::{AbsU128, SignedField}, }; use iter_extended::vecmap; use noirc_errors::Location; @@ -1156,15 +1157,15 @@ impl Type { } pub fn is_field(&self) -> bool { - matches!(self.follow_bindings(), Type::FieldElement) + matches!(self.follow_bindings_shallow().as_ref(), Type::FieldElement) } pub fn is_bool(&self) -> bool { - matches!(self.follow_bindings(), Type::Bool) + matches!(self.follow_bindings_shallow().as_ref(), Type::Bool) } pub fn is_integer(&self) -> bool { - matches!(self.follow_bindings(), Type::Integer(_, _)) + matches!(self.follow_bindings_shallow().as_ref(), Type::Integer(_, _)) } /// If value_level, only check for Type::FieldElement, @@ -2761,6 +2762,36 @@ impl Type { | Type::Error => None, } } + + pub(crate) fn integral_minimum_size(&self) -> Option { + match self.follow_bindings_shallow().as_ref() { + Type::FieldElement => None, + Type::Integer(sign, num_bits) => { + if *sign == Signedness::Unsigned { + return Some(SignedField::zero()); + } + + let max_bit_size = num_bits.bit_size() - 1; + Some(if max_bit_size == 128 { + SignedField::negative(i128::MIN.abs_u128()) + } else { + SignedField::negative(1u128 << max_bit_size) + }) + } + Type::Bool => Some(SignedField::zero()), + Type::TypeVariable(var) => { + let binding = &var.1; + match &*binding.borrow() { + TypeBinding::Unbound(_, type_var_kind) => match type_var_kind { + Kind::Any | Kind::Normal | Kind::Integer | Kind::IntegerOrField => None, + Kind::Numeric(typ) => typ.integral_minimum_size(), + }, + TypeBinding::Bound(typ) => typ.integral_minimum_size(), + } + } + _ => None, + } + } } /// Wraps a given `expression` in `expression.as_slice()` diff --git a/compiler/noirc_frontend/src/signed_field.rs b/compiler/noirc_frontend/src/signed_field.rs index dcddd52daa8..925b128ea24 100644 --- a/compiler/noirc_frontend/src/signed_field.rs +++ b/compiler/noirc_frontend/src/signed_field.rs @@ -19,6 +19,14 @@ impl SignedField { Self { field: field.into(), is_negative: true } } + pub fn zero() -> SignedField { + Self { field: FieldElement::zero(), is_negative: false } + } + + pub fn one() -> SignedField { + Self { field: FieldElement::one(), is_negative: false } + } + /// Convert a signed integer to a SignedField, carefully handling /// INT_MIN in the process. Note that to convert an unsigned integer /// you can call `SignedField::positive`. @@ -125,6 +133,30 @@ impl std::fmt::Display for SignedField { } } +impl rangemap::StepLite for SignedField { + fn add_one(&self) -> Self { + if self.is_negative { + if self.field.is_one() { + Self::new(FieldElement::zero(), false) + } else { + Self::new(self.field - FieldElement::one(), self.is_negative) + } + } else { + Self::new(self.field + FieldElement::one(), self.is_negative) + } + } + + fn sub_one(&self) -> Self { + if self.is_negative { + Self::new(self.field + FieldElement::one(), self.is_negative) + } else if self.field.is_zero() { + Self::new(FieldElement::one(), true) + } else { + Self::new(self.field - FieldElement::one(), self.is_negative) + } + } +} + pub trait AbsU128 { /// Necessary to handle casting to unsigned generically without overflowing on INT_MIN. fn abs_u128(self) -> u128; diff --git a/compiler/noirc_frontend/src/tests/enums.rs b/compiler/noirc_frontend/src/tests/enums.rs index be066583365..33a54b76079 100644 --- a/compiler/noirc_frontend/src/tests/enums.rs +++ b/compiler/noirc_frontend/src/tests/enums.rs @@ -250,3 +250,120 @@ fn match_reachability_errors_ignored_when_there_is_a_type_error() { ", ); } + +#[test] +fn missing_single_case() { + check_errors( + " + fn main() { + match Opt::Some(3) { + ^^^^^^^^^^^^ Missing case: `Some(_)` + Opt::None => (), + } + } + + enum Opt { + None, + Some(T), + } + ", + ); +} + +#[test] +fn missing_many_cases() { + check_errors( + " + fn main() { + match Abc::A { + ^^^^^^ Missing cases: `C`, `D`, `E`, and 21 more not shown + Abc::A => (), + Abc::B => (), + } + } + + enum Abc { + A,B,C,D,E,F,G,H,I,J,K,L,M,N,O,P,Q,R,S,T,U,V,W,X,Y,Z + } + ", + ); +} + +#[test] +fn missing_int_ranges() { + check_errors( + " + fn main() { + let x: i8 = 3; + match Opt::Some(x) { + ^^^^^^^^^^^^ Missing cases: `None`, `Some(-128..=3)`, `Some(5)`, and 1 more not shown + Opt::Some(4) => (), + Opt::Some(6) => (), + } + } + + enum Opt { + None, + Some(T), + } + ", + ); +} + +#[test] +fn missing_int_ranges_with_negatives() { + check_errors( + " + fn main() { + let x: i32 = -4; + match x { + ^ Missing cases: `-2147483648..=-6`, `-4..=-1`, `1..=2`, and 1 more not shown + -5 => (), + 0 => (), + 3 => (), + } + } + ", + ); +} + +#[test] +fn missing_cases_with_empty_match() { + check_errors( + " + fn main() { + match Abc::A {} + ^^^^^^ Missing cases: `A`, `B`, `C`, and 23 more not shown + } + + enum Abc { + A,B,C,D,E,F,G,H,I,J,K,L,M,N,O,P,Q,R,S,T,U,V,W,X,Y,Z + } + ", + ); +} + +#[test] +fn missing_integer_cases_with_empty_match() { + check_errors( + " + fn main() { + let x: i8 = 3; + match x {} + ^ Missing cases: `i8` is non-empty + ~ Try adding a match-all pattern: `_` + } + ", + ); +} + +#[test] +fn match_on_empty_enum() { + check_errors( + " + pub fn foo(v: Void) { + match v {} + } + pub enum Void {}", + ); +}