Skip to content

Commit

Permalink
fix(experimental elaborator): Fix frontend tests when `--use-elaborat…
Browse files Browse the repository at this point in the history
…or` flag is specified (noir-lang#5145)

# Description

## Problem\*

Resolves noir-lang#5146

Fixes all failing frontend tests with the elaborator

## Summary\*

This PR includes a few fixes:
- Use `check_trait_impl_method_matches_declaration` to issue errors when
a trait impl method is declared with an incorrect signature.
- `define_function_meta` for default trait functions on impls when
they're copied over. Also make sure to set `self.current_trait_impl` and
`self.self_type` for them as well.
- Use `try_get_trait_implementation` when getting a trait impl may fail
(which may happen if there was a previous error resolving the trait for
that impl). This was triggering on the test where we tried to declare a
trait impl implementing a struct rather than a trait.
- `check_trait_wrong_parameter_type` has a duplicate error on master but
not in the elaborator so I changed the expected error count there.

## Additional Context

After this PR all existing code _should_ work with the elaborator and we
can get the word out to get others testing the `--use-elaborator` flag
for any errors/panics.

## 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 30, 2024
1 parent 05df0cc commit d6122eb
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 10 deletions.
16 changes: 13 additions & 3 deletions compiler/noirc_frontend/src/elaborator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use crate::{
},
resolution::{errors::ResolverError, path_resolver::PathResolver, resolver::LambdaContext},
scope::ScopeForest as GenericScopeForest,
type_check::TypeCheckError,
type_check::{check_trait_impl_method_matches_declaration, TypeCheckError},
},
hir_def::{expr::HirIdent, function::Parameters, traits::TraitConstraint},
macros_api::{
Expand Down Expand Up @@ -851,6 +851,12 @@ impl<'context> Elaborator<'context> {
self.generics = trait_impl.resolved_generics;
self.current_trait_impl = trait_impl.impl_id;

for (module, function, _) in &trait_impl.methods.functions {
self.local_module = *module;
let errors = check_trait_impl_method_matches_declaration(self.interner, *function);
self.errors.extend(errors.into_iter().map(|error| (error.into(), self.file)));
}

self.elaborate_functions(trait_impl.methods);

self.self_type = None;
Expand All @@ -877,25 +883,26 @@ impl<'context> Elaborator<'context> {
fn collect_trait_impl(&mut self, trait_impl: &mut UnresolvedTraitImpl) {
self.local_module = trait_impl.module_id;
self.file = trait_impl.file_id;
self.current_trait_impl = trait_impl.impl_id;
trait_impl.trait_id = self.resolve_trait_by_path(trait_impl.trait_path.clone());

let self_type = trait_impl.methods.self_type.clone();
let self_type =
self_type.expect("Expected struct type to be set before collect_trait_impl");

self.self_type = Some(self_type.clone());
let self_type_span = trait_impl.object_type.span;

if matches!(self_type, Type::MutableReference(_)) {
let span = self_type_span.unwrap_or_else(|| trait_impl.trait_path.span());
self.push_err(DefCollectorErrorKind::MutableReferenceInTraitImpl { span });
}

assert!(trait_impl.trait_id.is_some());
if let Some(trait_id) = trait_impl.trait_id {
self.generics = trait_impl.resolved_generics.clone();
self.collect_trait_impl_methods(trait_id, trait_impl);

let span = trait_impl.object_type.span.expect("All trait self types should have spans");
self.generics = trait_impl.resolved_generics.clone();
self.declare_methods_on_struct(true, &mut trait_impl.methods, span);

let methods = trait_impl.methods.function_ids();
Expand Down Expand Up @@ -945,6 +952,8 @@ impl<'context> Elaborator<'context> {
}

self.generics.clear();
self.current_trait_impl = None;
self.self_type = None;
}

fn get_module_mut(&mut self, module: ModuleId) -> &mut ModuleData {
Expand Down Expand Up @@ -1059,6 +1068,7 @@ impl<'context> Elaborator<'context> {
let module = self.module_id();
let location = Location::new(default_impl.def.span, trait_impl.file_id);
self.interner.push_function(func_id, &default_impl.def, module, location);
self.define_function_meta(&mut default_impl_clone, func_id, false);
func_ids_in_trait.insert(func_id);
ordered_methods.push((
method.default_impl_module_id,
Expand Down
8 changes: 7 additions & 1 deletion compiler/noirc_frontend/src/hir/type_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,13 @@ pub(crate) fn check_trait_impl_method_matches_declaration(

let impl_ =
meta.trait_impl.expect("Trait impl function should have a corresponding trait impl");
let impl_ = interner.get_trait_implementation(impl_);

// If the trait implementation is not defined in the interner then there was a previous
// error in resolving the trait path and there is likely no trait for this impl.
let Some(impl_) = interner.try_get_trait_implementation(impl_) else {
return errors;
};

let impl_ = impl_.borrow();
let trait_info = interner.get_trait(impl_.trait_id);

Expand Down
4 changes: 4 additions & 0 deletions compiler/noirc_frontend/src/node_interner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1145,6 +1145,10 @@ impl NodeInterner {
}
}

pub fn try_get_trait_implementation(&self, id: TraitImplId) -> Option<Shared<TraitImpl>> {
self.trait_implementations.get(&id).cloned()
}

pub fn get_trait_implementation(&self, id: TraitImplId) -> Shared<TraitImpl> {
self.trait_implementations[&id].clone()
}
Expand Down
13 changes: 7 additions & 6 deletions compiler/noirc_frontend/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,10 @@ fn check_trait_wrong_parameter_type() {
}";
let errors = get_program_errors(src);
assert!(!has_parser_error(&errors));
assert!(errors.len() == 2, "Expected 2 errors, got: {:?}", errors);

// This is a duplicate error in the name resolver & type checker.
// In the elaborator there is no duplicate and only 1 error is issued
assert!(errors.len() <= 2, "Expected 1 or 2 errors, got: {:?}", errors);

for (err, _file_id) in errors {
match &err {
Expand Down Expand Up @@ -593,22 +596,20 @@ fn check_impl_struct_not_trait() {
bar: Field,
array: [Field; 2],
}
struct Default {
x: Field,
z: Field,
}
// Default is struct not a trait
// Default is a struct not a trait
impl Default for Foo {
fn default(x: Field, y: Field) -> Self {
Self { bar: x, array: [x,y] }
}
}
fn main() {
}
fn main() {}
";
let errors = get_program_errors(src);
assert!(!has_parser_error(&errors));
Expand Down

0 comments on commit d6122eb

Please sign in to comment.