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

fix(experimental elaborator): Fix panic during monomorphization #5126

Merged
merged 2 commits into from
May 28, 2024
Merged
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
21 changes: 15 additions & 6 deletions compiler/noirc_frontend/src/elaborator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -325,9 +325,11 @@
.expect("FuncMetas should be declared before a function is elaborated")
.clone();

for (parameter, param2) in function.def.parameters.iter().zip(&func_meta.parameters.0) {
let definition_kind = DefinitionKind::Local(None);
self.elaborate_pattern(parameter.pattern.clone(), param2.1.clone(), definition_kind);
// The DefinitionIds for each parameter were already created in define_function_meta
// so we need to reintroduce the same IDs into scope here.
for parameter in &func_meta.parameter_idents {
let name = self.interner.definition_name(parameter.id).to_owned();
self.add_existing_variable_to_scope(name, parameter.clone());
}

self.generics = func_meta.all_generics.clone();
Expand Down Expand Up @@ -358,7 +360,7 @@
// when multiple impls are available. Instead we default first to choose the Field or u64 impl.
for typ in &self.type_variables {
if let Type::TypeVariable(variable, kind) = typ.follow_bindings() {
let msg = "TypeChecker should only track defaultable type vars";

Check warning on line 363 in compiler/noirc_frontend/src/elaborator/mod.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (defaultable)
variable.bind(kind.default_type().expect(msg));
}
}
Expand Down Expand Up @@ -590,8 +592,9 @@
self.add_generics(&func.def.generics);

let mut generics = vecmap(&self.generics, |(_, typevar, _)| typevar.clone());
let mut parameters = vec![];
let mut parameter_types = vec![];
let mut parameters = Vec::new();
let mut parameter_types = Vec::new();
let mut parameter_idents = Vec::new();

for Param { visibility, pattern, typ, span: _ } in func.parameters().iter().cloned() {
if visibility == Visibility::Public && !self.pub_allowed(func) {
Expand All @@ -609,7 +612,12 @@
has_inline_attribute,
type_span,
);
let pattern = self.elaborate_pattern(pattern, typ.clone(), DefinitionKind::Local(None));
let pattern = self.elaborate_pattern_and_store_ids(
pattern,
typ.clone(),
DefinitionKind::Local(None),
&mut parameter_idents,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like it may be cleaner to just save the hir idents on self then it can cleared after we are done elaborating a function meta. We can then remove the #[allow(clippy::too_many_arguments)] and the threading of the mutable reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to avoid cluttering self with too many fields that are only used temporarily in a couple functions. Otherwise the object grows even larger, and it's already quite large since it has all of the name resolver's and type checker's fields.

);

parameters.push((pattern, typ.clone(), visibility));
parameter_types.push(typ);
Expand Down Expand Up @@ -678,6 +686,7 @@
all_generics: self.generics.clone(),
trait_impl: self.current_trait_impl,
parameters: parameters.into(),
parameter_idents,
return_type: func.def.return_type.clone(),
return_visibility: func.def.return_visibility,
has_body: !func.def.body.is_empty(),
Expand Down
60 changes: 54 additions & 6 deletions compiler/noirc_frontend/src/elaborator/patterns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,19 @@ impl<'context> Elaborator<'context> {
expected_type: Type,
definition_kind: DefinitionKind,
) -> HirPattern {
self.elaborate_pattern_mut(pattern, expected_type, definition_kind, None)
self.elaborate_pattern_mut(pattern, expected_type, definition_kind, None, &mut Vec::new())
}

/// Equivalent to `elaborate_pattern`, this version just also
/// adds any new DefinitionIds that were created to the given Vec.
pub(super) fn elaborate_pattern_and_store_ids(
&mut self,
pattern: Pattern,
expected_type: Type,
definition_kind: DefinitionKind,
created_ids: &mut Vec<HirIdent>,
) -> HirPattern {
self.elaborate_pattern_mut(pattern, expected_type, definition_kind, None, created_ids)
}

fn elaborate_pattern_mut(
Expand All @@ -36,6 +48,7 @@ impl<'context> Elaborator<'context> {
expected_type: Type,
definition: DefinitionKind,
mutable: Option<Span>,
new_definitions: &mut Vec<HirIdent>,
) -> HirPattern {
match pattern {
Pattern::Identifier(name) => {
Expand All @@ -47,15 +60,21 @@ impl<'context> Elaborator<'context> {
};
let ident = self.add_variable_decl(name, mutable.is_some(), true, definition);
self.interner.push_definition_type(ident.id, expected_type);
new_definitions.push(ident.clone());
HirPattern::Identifier(ident)
}
Pattern::Mutable(pattern, span, _) => {
if let Some(first_mut) = mutable {
self.push_err(ResolverError::UnnecessaryMut { first_mut, second_mut: span });
}

let pattern =
self.elaborate_pattern_mut(*pattern, expected_type, definition, Some(span));
let pattern = self.elaborate_pattern_mut(
*pattern,
expected_type,
definition,
Some(span),
new_definitions,
);
let location = Location::new(span, self.file);
HirPattern::Mutable(Box::new(pattern), location)
}
Expand All @@ -79,7 +98,13 @@ impl<'context> Elaborator<'context> {

let fields = vecmap(fields.into_iter().enumerate(), |(i, field)| {
let field_type = field_types.get(i).cloned().unwrap_or(Type::Error);
self.elaborate_pattern_mut(field, field_type, definition.clone(), mutable)
self.elaborate_pattern_mut(
field,
field_type,
definition.clone(),
mutable,
new_definitions,
)
});
let location = Location::new(span, self.file);
HirPattern::Tuple(fields, location)
Expand All @@ -91,10 +116,12 @@ impl<'context> Elaborator<'context> {
expected_type,
definition,
mutable,
new_definitions,
),
}
}

#[allow(clippy::too_many_arguments)]
fn elaborate_struct_pattern(
&mut self,
name: Path,
Expand All @@ -103,6 +130,7 @@ impl<'context> Elaborator<'context> {
expected_type: Type,
definition: DefinitionKind,
mutable: Option<Span>,
new_definitions: &mut Vec<HirIdent>,
) -> HirPattern {
let error_identifier = |this: &mut Self| {
// Must create a name here to return a HirPattern::Identifier. Allowing
Expand Down Expand Up @@ -140,6 +168,7 @@ impl<'context> Elaborator<'context> {
expected_type.clone(),
definition,
mutable,
new_definitions,
);

HirPattern::Struct(expected_type, fields, location)
Expand All @@ -148,6 +177,7 @@ impl<'context> Elaborator<'context> {
/// Resolve all the fields of a struct constructor expression.
/// Ensures all fields are present, none are repeated, and all
/// are part of the struct.
#[allow(clippy::too_many_arguments)]
fn resolve_constructor_pattern_fields(
&mut self,
struct_type: Shared<StructType>,
Expand All @@ -156,15 +186,21 @@ impl<'context> Elaborator<'context> {
expected_type: Type,
definition: DefinitionKind,
mutable: Option<Span>,
new_definitions: &mut Vec<HirIdent>,
) -> Vec<(Ident, HirPattern)> {
let mut ret = Vec::with_capacity(fields.len());
let mut seen_fields = HashSet::default();
let mut unseen_fields = struct_type.borrow().field_names();

for (field, pattern) in fields {
let field_type = expected_type.get_field_type(&field.0.contents).unwrap_or(Type::Error);
let resolved =
self.elaborate_pattern_mut(pattern, field_type, definition.clone(), mutable);
let resolved = self.elaborate_pattern_mut(
pattern,
field_type,
definition.clone(),
mutable,
new_definitions,
);

if unseen_fields.contains(&field) {
unseen_fields.remove(&field);
Expand Down Expand Up @@ -239,6 +275,18 @@ impl<'context> Elaborator<'context> {
ident
}

pub fn add_existing_variable_to_scope(&mut self, name: String, ident: HirIdent) {
let second_span = ident.location.span;
let resolver_meta = ResolverMeta { num_times_used: 0, ident, warn_if_unused: true };
michaeljklein marked this conversation as resolved.
Show resolved Hide resolved

let old_value = self.scopes.get_mut_scope().add_key_value(name.clone(), resolver_meta);

if let Some(old_value) = old_value {
let first_span = old_value.ident.location.span;
self.push_err(ResolverError::DuplicateDefinition { name, first_span, second_span });
}
}

pub fn add_global_variable_decl(
&mut self,
name: Ident,
Expand Down
1 change: 1 addition & 0 deletions compiler/noirc_frontend/src/hir/resolution/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1110,6 +1110,7 @@ impl<'a> Resolver<'a> {
// This is only used by the elaborator
all_generics: Vec::new(),
is_trait_function: false,
parameter_idents: Vec::new(),
}
}

Expand Down
1 change: 1 addition & 0 deletions compiler/noirc_frontend/src/hir/type_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@
// when multiple impls are available. Instead we default first to choose the Field or u64 impl.
for typ in &type_checker.type_variables {
if let Type::TypeVariable(variable, kind) = typ.follow_bindings() {
let msg = "TypeChecker should only track defaultable type vars";

Check warning on line 138 in compiler/noirc_frontend/src/hir/type_check/mod.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (defaultable)
variable.bind(kind.default_type().expect(msg));
}
}
Expand Down Expand Up @@ -552,6 +552,7 @@
is_trait_function: false,
has_inline_attribute: false,
all_generics: Vec::new(),
parameter_idents: Vec::new(),
};
interner.push_fn_meta(func_meta, func_id);

Expand Down
4 changes: 4 additions & 0 deletions compiler/noirc_frontend/src/hir_def/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,10 @@ pub struct FuncMeta {

pub parameters: Parameters,

/// The HirIdent of each identifier within the parameter list.
/// Note that this includes separate entries for each identifier in e.g. tuple patterns.
pub parameter_idents: Vec<HirIdent>,

pub return_type: FunctionReturnType,

pub return_visibility: Visibility,
Expand Down
Loading