Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(experimental): Issue missing case error for match expressions #7560

Open
wants to merge 2 commits into
base: jf/unreachable-case-error
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 100 additions & 12 deletions compiler/noirc_frontend/src/elaborator/enums.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
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 crate::{
Expand Down Expand Up @@ -793,13 +793,17 @@
///
/// 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 {
MatchCompiler::run(self, rows)
pub(super) fn elaborate_match_rows(&mut self, rows: Vec<Row>, location: Location) -> HirMatch {
MatchCompiler::run(self, rows, location)
}
}

impl<'elab, 'ctx> MatchCompiler<'elab, 'ctx> {
fn run(elaborator: &'elab mut Elaborator<'ctx>, rows: Vec<Row>) -> HirMatch {
fn run(
elaborator: &'elab mut Elaborator<'ctx>,
rows: Vec<Row>,
location: Location,
) -> HirMatch {
let mut compiler = Self {
elaborator,
has_missing_cases: false,
Expand All @@ -812,7 +816,7 @@
});

if compiler.has_missing_cases {
compiler.issue_missing_cases_error(&hir_match);
compiler.issue_missing_cases_error(&hir_match, location);
}

if !compiler.unreachable_cases.is_empty() {
Expand Down Expand Up @@ -954,7 +958,7 @@

/// Compiles the cases and fallback cases for integer and range patterns.
///
/// Integers have an infinite number of constructors, so we specialise the

Check warning on line 961 in compiler/noirc_frontend/src/elaborator/enums.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (specialise)
/// compilation of integer and range patterns.
fn compile_int_cases(
&mut self,
Expand Down Expand Up @@ -1177,17 +1181,101 @@
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) {
for location in self.unreachable_cases.values().copied() {
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, location: Location) {
let starting_id = match tree {
HirMatch::Switch(id, ..) => Some(*id),
_ => None,
};

let mut cases = BTreeSet::new();
self.find_missing_values(tree, &mut Default::default(), &mut cases, starting_id);

self.elaborator.push_err(TypeCheckError::MissingCases { cases, location });
}

fn find_missing_values(
&self,
tree: &HirMatch,
env: &mut HashMap<DefinitionId, Case>,
missing_cases: &mut BTreeSet<String>,
starting_id: Option<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 {
env.insert(*definition_id, case.clone());
self.find_missing_values(&case.body, env, missing_cases, starting_id);
}

if let Some(else_case) = else_case {
for case in self.missing_cases(cases) {
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]) -> Vec<Case> {
// 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];

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());
Case::new(constructor, args, HirMatch::Failure { missing_case: true })
})
}

fn construct_missing_case(
starting_id: Option<DefinitionId>,
env: &HashMap<DefinitionId, Case>,
) -> String {
let Some(id) = starting_id else {
return "_".to_string();
};

let Some(case) = env.get(&id) else {
return "_".to_string();
};

let constructor = case.constructor.to_string();
let no_arguments = case.arguments.is_empty();

let args =
vecmap(&case.arguments, |arg| Self::construct_missing_case(Some(*arg), env)).join(", ");

if no_arguments { constructor } else { format!("{constructor}({args})") }
}
}
3 changes: 2 additions & 1 deletion compiler/noirc_frontend/src/elaborator/expressions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1055,6 +1055,7 @@ 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);

Expand All @@ -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, expr_location)
} else {
HirMatch::Failure { missing_case: false }
});
Expand Down
25 changes: 25 additions & 0 deletions compiler/noirc_frontend/src/hir/type_check/errors.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use std::collections::BTreeSet;
use std::rc::Rc;

use acvm::FieldElement;
Expand All @@ -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")]
Expand Down Expand Up @@ -237,6 +241,8 @@ pub enum TypeCheckError {
NestedUnsafeBlock { location: Location },
#[error("Unreachable match case")]
UnreachableCase { location: Location },
#[error("Missing cases")]
MissingCases { cases: BTreeSet<String>, location: Location },
}

#[derive(Debug, Clone, PartialEq, Eq)]
Expand Down Expand Up @@ -324,6 +330,7 @@ impl TypeCheckError {
| TypeCheckError::TypeAnnotationsNeededForIndex { location }
| TypeCheckError::UnnecessaryUnsafeBlock { location }
| TypeCheckError::UnreachableCase { location }
| TypeCheckError::MissingCases { location, .. }
| TypeCheckError::NestedUnsafeBlock { location } => *location,
TypeCheckError::DuplicateNamedTypeArg { name: ident, .. }
| TypeCheckError::NoSuchNamedTypeArg { name: ident, .. } => ident.location(),
Expand Down Expand Up @@ -651,6 +658,24 @@ 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::<Vec<_>>();

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)
},
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_frontend/src/hir/type_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
38 changes: 37 additions & 1 deletion compiler/noirc_frontend/src/hir_def/expr.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use fm::FileId;
use iter_extended::vecmap;
use noirc_errors::Location;

use crate::Shared;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down
38 changes: 38 additions & 0 deletions compiler/noirc_frontend/src/tests/enums.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
let src = r#"
enum Foo { Bar }
^^^ This requires the unstable feature 'enums' which is not enabled
~~~ Pass -Zenums to nargo to enable this feature at your own risk.

Check warning on line 32 in compiler/noirc_frontend/src/tests/enums.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (Zenums)

fn main() {
let _x = Foo::Bar;
Expand Down Expand Up @@ -250,3 +250,41 @@
",
);
}

#[test]
fn missing_single_case() {
check_errors(
"
fn main() {
match Opt::Some(3) {
^^^^^^^^^^^^ Missing case: `Some(_)`
Opt::None => (),
}
}

enum Opt<T> {
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
}
",
);
}
Loading