Skip to content

Commit

Permalink
fix(experimental elaborator): Fix duplicate resolve_type on self ty…
Browse files Browse the repository at this point in the history 
…pe and don't leak a trait impl's generics (#5102)

# Description

## Problem\*

Resolves <!-- Link to GitHub Issue -->

## Summary\*

Fixes two issues:
- Use cached `resolved_object_type` instead of re-resolving the self
type in `declare_methods_on_struct` and `check_trait_impl_coherence`.
This is needed to prevent duplicate errors, and in the later case, since
generics are not added to scope to be able to re-resolve the object type
at that point.
- Fix a case when resolving trait impls that `self.generics` was not
cleared afterward.

## Additional Context

Down to 112 errors in the standard library now. The remaining errors are
variants of:
- "Duplicate definitions of \<global> found"
- "Duplicate definitions of \<generic> found" - looks to be only one
case where there is a generic directly on a trait method. `fn
hash<H>(self, state: &mut H) where H: Hasher;`
- "Expression type is ambiguous" - from the two turbofish cases that
haven't been merged in this branch yet
- "Expected type &mut _, found type H" - issues with auto-deref and
trait methods possibly
- "No matching impl found for ..." - trait solver issues

## 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.

---------

Co-authored-by: Tom French <[email protected]>
Co-authored-by: Maxim Vezenov <[email protected]>
Co-authored-by: Álvaro Rodríguez <[email protected]>
Co-authored-by: guipublic <[email protected]>
  • Loading branch information
@jfecher @TomAFrench
@vezenovm @sirasistant @guipublic
5 people authored May 28, 2024
1 parent 11e98f3 commit db561e2
Show file tree
Hide file tree
Showing 4 changed files with 135 additions and 163 deletions.
153 changes: 63 additions & 90 deletions compiler/noirc_frontend/src/elaborator/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,8 +223,8 @@ impl<'context> Elaborator<'context> {
//
// These are resolved after trait impls so that struct methods are chosen
// over trait methods if there are name conflicts.
for ((typ, module), impls) in &mut items.impls {
this.collect_impls(typ, *module, impls);
for ((_self_type, module), impls) in &mut items.impls {
this.collect_impls(*module, impls);
}

// We must wait to resolve non-literal globals until after we resolve structs since struct
Expand Down Expand Up @@ -284,8 +284,6 @@ impl<'context> Elaborator<'context> {
self.scopes.start_function();
self.current_item = Some(DependencyId::Function(id));

// Check whether the function has globals in the local module and add them to the scope
self.resolve_local_globals();
self.trait_bounds = function.def.where_clause.clone();

if function.def.is_unconstrained {
Expand Down Expand Up @@ -474,18 +472,6 @@ impl<'context> Elaborator<'context> {
None
}

fn resolve_local_globals(&mut self) {
let globals = vecmap(self.interner.get_all_globals(), |global| {
(global.id, global.local_id, global.ident.clone())
});
for (id, local_module_id, name) in globals {
if local_module_id == self.local_module {
let definition = DefinitionKind::Global(id);
self.add_global_variable_decl(name, definition);
}
}
}

/// TODO: This is currently only respected for generic free functions
/// there's a bunch of other places where trait constraints can pop up
fn resolve_trait_constraints(
Expand All @@ -494,21 +480,20 @@ impl<'context> Elaborator<'context> {
) -> Vec<TraitConstraint> {
where_clause
.iter()
.cloned()
.filter_map(|constraint| self.resolve_trait_constraint(constraint))
.collect()
}

pub fn resolve_trait_constraint(
&mut self,
constraint: UnresolvedTraitConstraint,
constraint: &UnresolvedTraitConstraint,
) -> Option<TraitConstraint> {
let typ = self.resolve_type(constraint.typ);
let typ = self.resolve_type(constraint.typ.clone());
let trait_generics =
vecmap(constraint.trait_bound.trait_generics, |typ| self.resolve_type(typ));
vecmap(&constraint.trait_bound.trait_generics, |typ| self.resolve_type(typ.clone()));

let span = constraint.trait_bound.trait_path.span();
let the_trait = self.lookup_trait_or_error(constraint.trait_bound.trait_path)?;
let the_trait = self.lookup_trait_or_error(constraint.trait_bound.trait_path.clone())?;
let trait_id = the_trait.id;

let expected_generics = the_trait.generics.len();
Expand Down Expand Up @@ -548,9 +533,6 @@ impl<'context> Elaborator<'context> {
self.scopes.start_function();
self.current_item = Some(DependencyId::Function(func_id));

// Check whether the function has globals in the local module and add them to the scope
self.resolve_local_globals();

let location = Location::new(func.name_ident().span(), self.file);
let id = self.interner.function_definition_id(func_id);
let name_ident = HirIdent::non_trait_method(id, location);
Expand Down Expand Up @@ -864,40 +846,69 @@ impl<'context> Elaborator<'context> {
self.file = trait_impl.file_id;
self.local_module = trait_impl.module_id;

let unresolved_type = trait_impl.object_type;
let self_type_span = unresolved_type.span;
let old_generics_length = self.generics.len();
self.generics = trait_impl.resolved_generics;
self.current_trait_impl = trait_impl.impl_id;

let trait_generics =
vecmap(&trait_impl.trait_generics, |generic| self.resolve_type(generic.clone()));
self.elaborate_functions(trait_impl.methods);

let self_type = trait_impl.resolved_object_type.unwrap_or(Type::Error);
let impl_id =
trait_impl.impl_id.expect("An impls' id should be set during define_function_metas");
self.self_type = None;
self.current_trait_impl = None;
self.generics.clear();
}

self.current_trait_impl = trait_impl.impl_id;
fn collect_impls(
&mut self,
module: LocalModuleId,
impls: &mut [(Vec<Ident>, Span, UnresolvedFunctions)],
) {
self.local_module = module;

let methods = trait_impl.methods.function_ids();
for (generics, span, unresolved) in impls {
self.file = unresolved.file_id;
let old_generic_count = self.generics.len();
self.add_generics(generics);
self.declare_methods_on_struct(false, unresolved, *span);
self.generics.truncate(old_generic_count);
}
}

self.elaborate_functions(trait_impl.methods);
fn collect_trait_impl(&mut self, trait_impl: &mut UnresolvedTraitImpl) {
self.local_module = trait_impl.module_id;
self.file = trait_impl.file_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");

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.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();
for func_id in &methods {
self.interner.set_function_trait(*func_id, self_type.clone(), trait_id);
}

let where_clause = trait_impl
.where_clause
.into_iter()
.iter()
.flat_map(|item| self.resolve_trait_constraint(item))
.collect();

let trait_generics = trait_impl.resolved_trait_generics.clone();

let resolved_trait_impl = Shared::new(TraitImpl {
ident: trait_impl.trait_path.last_segment().clone(),
typ: self_type.clone(),
Expand All @@ -914,7 +925,7 @@ impl<'context> Elaborator<'context> {
self_type.clone(),
trait_id,
trait_generics,
impl_id,
trait_impl.impl_id.expect("impl_id should be set in define_function_metas"),
generics,
resolved_trait_impl,
) {
Expand All @@ -931,45 +942,7 @@ impl<'context> Elaborator<'context> {
}
}

self.self_type = None;
self.current_trait_impl = None;
self.generics.truncate(old_generics_length);
}

fn collect_impls(
&mut self,
self_type: &UnresolvedType,
module: LocalModuleId,
impls: &mut [(Vec<Ident>, Span, UnresolvedFunctions)],
) {
self.local_module = module;

for (generics, span, unresolved) in impls {
self.file = unresolved.file_id;
self.recover_generics(|this| {
this.add_generics(generics);
this.declare_methods_on_struct(self_type, false, unresolved, *span);
});
}
}

fn collect_trait_impl(&mut self, trait_impl: &mut UnresolvedTraitImpl) {
self.local_module = trait_impl.module_id;
self.file = trait_impl.file_id;
trait_impl.trait_id = self.resolve_trait_by_path(trait_impl.trait_path.clone());

if let Some(trait_id) = trait_impl.trait_id {
self.collect_trait_impl_methods(trait_id, trait_impl);

let span = trait_impl.object_type.span.expect("All trait self types should have spans");
let object_type = &trait_impl.object_type;

self.recover_generics(|this| {
this.add_generics(&trait_impl.generics);
trait_impl.resolved_generics = this.generics.clone();
this.declare_methods_on_struct(object_type, true, &mut trait_impl.methods, span);
});
}
self.generics.clear();
}

fn get_module_mut(&mut self, module: ModuleId) -> &mut ModuleData {
Expand All @@ -979,14 +952,13 @@ impl<'context> Elaborator<'context> {

fn declare_methods_on_struct(
&mut self,
self_type: &UnresolvedType,
is_trait_impl: bool,
functions: &mut UnresolvedFunctions,
span: Span,
) {
let self_type = self.resolve_type(self_type.clone());

functions.self_type = Some(self_type.clone());
let self_type = functions.self_type.as_ref();
let self_type =
self_type.expect("Expected struct type to be set before declare_methods_on_struct");

let function_ids = functions.function_ids();

Expand Down Expand Up @@ -1016,11 +988,11 @@ impl<'context> Elaborator<'context> {
}
}

self.declare_struct_methods(&self_type, &function_ids);
self.declare_struct_methods(self_type, &function_ids);
// We can define methods on primitive types only if we're in the stdlib
} else if !is_trait_impl && self_type != Type::Error {
} else if !is_trait_impl && *self_type != Type::Error {
if self.crate_id.is_stdlib() {
self.declare_struct_methods(&self_type, &function_ids);
self.declare_struct_methods(self_type, &function_ids);
} else {
self.push_err(DefCollectorErrorKind::NonStructTypeInImpl { span });
}
Expand Down Expand Up @@ -1145,8 +1117,8 @@ impl<'context> Elaborator<'context> {
self.local_module = trait_impl.module_id;
self.file = trait_impl.file_id;

let object_crate = match self.resolve_type(trait_impl.object_type.clone()) {
Type::Struct(struct_type, _) => struct_type.borrow().id.krate(),
let object_crate = match &trait_impl.resolved_object_type {
Some(Type::Struct(struct_type, _)) => struct_type.borrow().id.krate(),
_ => CrateId::Dummy,
};

Expand All @@ -1163,7 +1135,6 @@ impl<'context> Elaborator<'context> {
self.local_module = alias.module_id;

let generics = self.add_generics(&alias.type_alias_def.generics);
self.resolve_local_globals();
self.current_item = Some(DependencyId::Alias(alias_id));
let typ = self.resolve_type(alias.type_alias_def.typ);
self.interner.set_type_alias(alias_id, typ, generics);
Expand Down Expand Up @@ -1215,9 +1186,6 @@ impl<'context> Elaborator<'context> {
self.recover_generics(|this| {
let generics = this.add_generics(&unresolved.generics);

// Check whether the struct definition has globals in the local module and add them to the scope
this.resolve_local_globals();

this.current_item = Some(DependencyId::Struct(struct_id));

this.resolving_ids.insert(struct_id);
Expand Down Expand Up @@ -1253,7 +1221,7 @@ impl<'context> Elaborator<'context> {
let (let_statement, _typ) = self.elaborate_let(let_stmt);

let statement_id = self.interner.get_global(global_id).let_statement;
self.interner.get_global_definition_mut(global_id).kind = definition_kind;
self.interner.get_global_definition_mut(global_id).kind = definition_kind.clone();
self.interner.replace_statement(statement_id, let_statement);
}

Expand Down Expand Up @@ -1286,6 +1254,11 @@ impl<'context> Elaborator<'context> {

let unresolved_type = &trait_impl.object_type;
self.add_generics(&trait_impl.generics);
trait_impl.resolved_generics = self.generics.clone();

let trait_generics =
vecmap(&trait_impl.trait_generics, |generic| self.resolve_type(generic.clone()));
trait_impl.resolved_trait_generics = trait_generics;

let self_type = self.resolve_type(unresolved_type.clone());

Expand Down
Loading

0 comments on commit db561e2

Please sign in to comment.