Skip to content

Commit

Permalink
fix(experimental): Replace most remaining match panics with errors (#…
Browse files Browse the repository at this point in the history 
…7536)
  • Loading branch information
@jfecher
jfecher authored Feb 26, 2025
1 parent 45e7a7d commit d118e17
Show file tree
Hide file tree
Showing 5 changed files with 429 additions and 219 deletions.
219 changes: 160 additions & 59 deletions compiler/noirc_frontend/src/elaborator/enums.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -334,44 +334,30 @@ impl Elaborator<'_> {
// - Possible diagnostics improvement: warn if `a` is defined as a variable
// when there is a matching enum variant with name `Foo::a` which can
// be imported. The user likely intended to reference the enum variant.
let path_len = path.segments.len();
let location = path.location;
let last_ident = path.last_ident();

// Setting this to `Some` allows us to shadow globals with the same name.
// We should avoid this if there is a `::` in the path since that means the
// user is trying to resolve to a non-local item.
let shadow_existing = path.is_ident().then_some(last_ident);

match self.resolve_path_or_error(path) {
Ok(resolution) => self.path_resolution_to_constructor(
resolution,
shadow_existing,
Vec::new(),
expected_type,
location,
variables_defined,
),
Err(_) if path_len == 1 => {
// Define the variable
let kind = DefinitionKind::Local(None);

if let Some(existing) =
variables_defined.iter().find(|elem| *elem == &last_ident)
{
// Allow redefinition of `_` only, to ignore variables
if last_ident.0.contents != "_" {
let error = ResolverError::VariableAlreadyDefinedInPattern {
existing: existing.clone(),
new_location: last_ident.location(),
};
self.push_err(error);
}
Err(error) => {
if let Some(name) = shadow_existing {
self.define_pattern_variable(name, expected_type, variables_defined)
} else {
variables_defined.push(last_ident.clone());
self.push_err(error);
Pattern::Error
}

let id = self.add_variable_decl(last_ident, false, true, true, kind).id;
self.interner.push_definition_type(id, expected_type.clone());
Pattern::Binding(id)
}
Err(error) => {
self.push_err(error);
Pattern::Error
}
}
}
Expand Down Expand Up @@ -436,6 +422,32 @@ impl Elaborator<'_> {
}
}

fn define_pattern_variable(
&mut self,
name: Ident,
expected_type: &Type,
variables_defined: &mut Vec<Ident>,
) -> Pattern {
// Define the variable
let kind = DefinitionKind::Local(None);

if let Some(existing) = variables_defined.iter().find(|elem| *elem == &name) {
// Allow redefinition of `_` only, to ignore variables
if name.0.contents != "_" {
self.push_err(ResolverError::VariableAlreadyDefinedInPattern {
existing: existing.clone(),
new_location: name.location(),
});
}
} else {
variables_defined.push(name.clone());
}

let id = self.add_variable_decl(name, false, true, true, kind).id;
self.interner.push_definition_type(id, expected_type.clone());
Pattern::Binding(id)
}

fn constructor_to_pattern(
&mut self,
constructor: ConstructorExpression,
Expand Down Expand Up @@ -490,13 +502,21 @@ impl Elaborator<'_> {
expected_type: &Type,
variables_defined: &mut Vec<Ident>,
) -> Pattern {
let syntax_error = |this: &mut Self| {
this.push_err(ResolverError::InvalidSyntaxInPattern { location: name.location });
Pattern::Error
};

match name.kind {
ExpressionKind::Variable(path) => {
let location = path.location;

match self.resolve_path_or_error(path) {
// Use None for `name` here - we don't want to define a variable if this
// resolves to an existing item.
Ok(resolution) => self.path_resolution_to_constructor(
resolution,
None,
args,
expected_type,
location,
Expand Down Expand Up @@ -526,28 +546,47 @@ impl Elaborator<'_> {
variables_defined,
)
} else {
panic!("Invalid expr kind {name}")
syntax_error(self)
}
}
other => todo!("invalid constructor `{other}`"),
_ => syntax_error(self),
}
}

/// Convert a PathResolutionItem - usually an enum variant or global - to a Constructor.
/// If `name` is `Some`, we'll define a Pattern::Binding instead of erroring if the
/// item doesn't resolve to a variant or global. This would shadow an existing
/// value such as a free function. Generally this is desired unless the variable was
/// a path with multiple components such as `foo::bar` which should always be treated as
/// a path to an existing item.
fn path_resolution_to_constructor(
&mut self,
name: PathResolutionItem,
resolution: PathResolutionItem,
name: Option<Ident>,
args: Vec<Expression>,
expected_type: &Type,
location: Location,
variables_defined: &mut Vec<Ident>,
) -> Pattern {
let (actual_type, expected_arg_types, variant_index) = match name {
let (actual_type, expected_arg_types, variant_index) = match &resolution {
PathResolutionItem::Global(id) => {
// variant constant
let global = self.interner.get_global(id);
let variant_index = match global.value {
GlobalValue::Resolved(Value::Enum(tag, ..)) => tag,
_ => todo!("Value is not an enum constant"),
self.elaborate_global_if_unresolved(id);
let global = self.interner.get_global(*id);
let variant_index = match &global.value {
GlobalValue::Resolved(Value::Enum(tag, ..)) => *tag,
// This may be a global constant. Treat it like a normal constant
GlobalValue::Resolved(value) => {
let value = value.clone();
return self.global_constant_to_integer_constructor(
value,
expected_type,
location,
);
}
// We tried to resolve this value above so there must have been an error
// in doing so. Avoid reporting an additional error.
_ => return Pattern::Error,
};

let global_type = self.interner.definition_type(global.definition_id);
Expand All @@ -556,8 +595,12 @@ impl Elaborator<'_> {
}
PathResolutionItem::Method(_type_id, _type_turbofish, func_id) => {
// TODO(#7430): Take type_turbofish into account when instantiating the function's type
let meta = self.interner.function_meta(&func_id);
let Some(variant_index) = meta.enum_variant_index else { todo!("not a variant") };
let meta = self.interner.function_meta(func_id);
let Some(variant_index) = meta.enum_variant_index else {
let item = resolution.description();
self.push_err(ResolverError::UnexpectedItemInPattern { location, item });
return Pattern::Error;
};

let (actual_type, expected_arg_types) = match meta.typ.instantiate(self.interner).0
{
Expand All @@ -567,18 +610,22 @@ impl Elaborator<'_> {

(actual_type, expected_arg_types, variant_index)
}
PathResolutionItem::Module(_) => todo!("path_resolution_to_constructor {name:?}"),
PathResolutionItem::Type(_) => todo!("path_resolution_to_constructor {name:?}"),
PathResolutionItem::TypeAlias(_) => todo!("path_resolution_to_constructor {name:?}"),
PathResolutionItem::Trait(_) => todo!("path_resolution_to_constructor {name:?}"),
PathResolutionItem::ModuleFunction(_) => {
todo!("path_resolution_to_constructor {name:?}")
}
PathResolutionItem::TypeAliasFunction(_, _, _) => {
todo!("path_resolution_to_constructor {name:?}")
}
PathResolutionItem::TraitFunction(_, _, _) => {
todo!("path_resolution_to_constructor {name:?}")
PathResolutionItem::Module(_)
| PathResolutionItem::Type(_)
| PathResolutionItem::TypeAlias(_)
| PathResolutionItem::Trait(_)
| PathResolutionItem::ModuleFunction(_)
| PathResolutionItem::TypeAliasFunction(_, _, _)
| PathResolutionItem::TraitFunction(_, _, _) => {
// This variable refers to an existing item
if let Some(name) = name {
// If name is set, shadow the existing item
return self.define_pattern_variable(name, expected_type, variables_defined);
} else {
let item = resolution.description();
self.push_err(ResolverError::UnexpectedItemInPattern { location, item });
return Pattern::Error;
}
}
};

Expand All @@ -591,7 +638,10 @@ impl Elaborator<'_> {
});

if args.len() != expected_arg_types.len() {
// error expected N args, found M?
let expected = expected_arg_types.len();
let found = args.len();
self.push_err(TypeCheckError::ArityMisMatch { expected, found, location });
return Pattern::Error;
}

let args = args.into_iter().zip(expected_arg_types);
Expand All @@ -602,6 +652,55 @@ impl Elaborator<'_> {
Pattern::Constructor(constructor, args)
}

fn global_constant_to_integer_constructor(
&mut self,
constant: Value,
expected_type: &Type,
location: Location,
) -> Pattern {
let actual_type = constant.get_type();
self.unify(&actual_type, expected_type, || TypeCheckError::TypeMismatch {
expected_typ: expected_type.to_string(),
expr_typ: actual_type.to_string(),
expr_location: location,
});

// Convert a signed integer type like i32 to SignedField
macro_rules! signed_to_signed_field {
($value:expr) => {{
let negative = $value < 0;
// Widen the value so that SignedType::MIN does not wrap to 0 when negated below
let mut widened = $value as i128;
if negative {
widened = -widened;
}
SignedField::new(widened.into(), negative)
}};
}

let value = match constant {
Value::Bool(value) => SignedField::positive(value),
Value::Field(value) => SignedField::positive(value),
Value::I8(value) => signed_to_signed_field!(value),
Value::I16(value) => signed_to_signed_field!(value),
Value::I32(value) => signed_to_signed_field!(value),
Value::I64(value) => signed_to_signed_field!(value),
Value::U1(value) => SignedField::positive(value),
Value::U8(value) => SignedField::positive(value as u128),
Value::U16(value) => SignedField::positive(value as u128),
Value::U32(value) => SignedField::positive(value),
Value::U64(value) => SignedField::positive(value),
Value::U128(value) => SignedField::positive(value),
Value::Zeroed(_) => SignedField::positive(0u32),
_ => {
self.push_err(ResolverError::NonIntegerGlobalUsedInPattern { location });
return Pattern::Error;
}
};

Pattern::Int(value)
}

fn struct_name_and_field_types(
&mut self,
typ: &Type,
Expand Down Expand Up @@ -663,8 +762,6 @@ impl Elaborator<'_> {
Ok(HirMatch::Switch(branch_var, cases, Some(fallback)))
}

Type::Array(_, _) => todo!(),
Type::Slice(_) => todo!(),
Type::Bool => {
let cases = vec![
(Constructor::False, Vec::new(), Vec::new()),
Expand Down Expand Up @@ -715,12 +812,16 @@ impl Elaborator<'_> {
} else {
drop(def);
let typ = Type::DataType(type_def, generics);
todo!("Cannot match on type {typ}")
Err(ResolverError::TypeUnsupportedInMatch { typ, location })
}
}
typ @ (Type::Alias(_, _)
| Type::TypeVariable(_)
// We could match on these types in the future
typ @ (Type::Array(_, _)
| Type::Slice(_)
| Type::String(_)
// But we'll never be able to match on these
| Type::Alias(_, _)
| Type::TypeVariable(_)
| Type::FmtString(_, _)
| Type::TraitAsType(_, _, _)
| Type::NamedGeneric(_, _)
Expand All @@ -731,7 +832,9 @@ impl Elaborator<'_> {
| Type::Constant(_, _)
| Type::Quoted(_)
| Type::InfixExpr(_, _, _, _)
| Type::Error) => todo!("Cannot match on type {typ:?}"),
| Type::Error) => {
Err(ResolverError::TypeUnsupportedInMatch { typ, location })
},
}
}

Expand Down Expand Up @@ -769,11 +872,9 @@ impl Elaborator<'_> {
let (key, cons) = match col.pattern {
Pattern::Int(val) => ((val, val), Constructor::Int(val)),
Pattern::Range(start, stop) => ((start, stop), Constructor::Range(start, stop)),
Pattern::Error => continue,
pattern => {
eprintln!("Unexpected pattern for integer type: {pattern:?}");
continue;
}
// Any other pattern shouldn't have an integer type and we expect a type
// check error to already have been issued.
_ => continue,
};

if let Some(index) = tested.get(&key) {
Expand Down
28 changes: 28 additions & 0 deletions compiler/noirc_frontend/src/hir/resolution/errors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,12 @@ pub enum ResolverError {
InvalidSyntaxInPattern { location: Location },
#[error("Variable '{existing}' was already defined in the same match pattern")]
VariableAlreadyDefinedInPattern { existing: Ident, new_location: Location },
#[error("Only integer globals can be used in match patterns")]
NonIntegerGlobalUsedInPattern { location: Location },
#[error("Cannot match on values of type `{typ}`")]
TypeUnsupportedInMatch { typ: Type, location: Location },
#[error("Expected a struct, enum, or literal value in pattern, but found a {item}")]
UnexpectedItemInPattern { location: Location, item: &'static str },
}

impl ResolverError {
Expand Down Expand Up @@ -254,6 +260,9 @@ impl ResolverError {
| ResolverError::MutatingComptimeInNonComptimeContext { location, .. }
| ResolverError::InvalidInternedStatementInExpr { location, .. }
| ResolverError::InvalidSyntaxInPattern { location }
| ResolverError::NonIntegerGlobalUsedInPattern { location, .. }
| ResolverError::TypeUnsupportedInMatch { location, .. }
| ResolverError::UnexpectedItemInPattern { location, .. }
| ResolverError::VariableAlreadyDefinedInPattern { new_location: location, .. } => {
*location
}
Expand Down Expand Up @@ -796,6 +805,25 @@ impl<'a> From<&'a ResolverError> for Diagnostic {
error.add_secondary(format!("`{existing}` was previously defined here"), existing.location());
error
},
ResolverError::NonIntegerGlobalUsedInPattern { location } => {
let message = "Only integer or boolean globals can be used in match patterns".to_string();
let secondary = "This global is not an integer or boolean".to_string();
Diagnostic::simple_error(message, secondary, *location)
},
ResolverError::TypeUnsupportedInMatch { typ, location } => {
Diagnostic::simple_error(
format!("Cannot match on values of type `{typ}`"),
String::new(),
*location,
)
},
ResolverError::UnexpectedItemInPattern { item, location } => {
Diagnostic::simple_error(
format!("Expected a struct, enum, or literal pattern, but found a {item}"),
String::new(),
*location,
)
},
}
}
}
Loading

0 comments on commit d118e17

Please sign in to comment.