Skip to content

Commit

Permalink
fix(experimental elaborator): Fix panic during monomorphization (#5126)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

Resolves a panic in the elaborator during monomorphization related to
parameters being resolved twice and having a different DefinitionId the
second time.

## Summary\*

I've stored the original ids from the first resolution and switched to
using only those.

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
jfecher authored May 28, 2024
1 parent 67add01 commit 13173e8
Show file tree
Hide file tree
Showing 5 changed files with 75 additions and 12 deletions.
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 @@ -295,9 +295,11 @@ impl<'context> Elaborator<'context> {
.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 @@ -560,8 +562,9 @@ impl<'context> Elaborator<'context> {
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 @@ -579,7 +582,12 @@ impl<'context> Elaborator<'context> {
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,
);

parameters.push((pattern, typ.clone(), visibility));
parameter_types.push(typ);
Expand Down Expand Up @@ -648,6 +656,7 @@ impl<'context> Elaborator<'context> {
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 };

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 @@ -552,6 +552,7 @@ pub mod test {
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

0 comments on commit 13173e8

Please sign in to comment.