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

feat(traits): added checks for duplicated trait associated items (types, consts, functions) #2927

Merged
merged 16 commits into from
Oct 5, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
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
2 changes: 1 addition & 1 deletion compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@

// Bind trait impls to their trait. Collect trait functions, that have a
// default implementation, which hasn't been overriden.
errors.extend(collect_trait_impls(

Check warning on line 306 in compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (overriden)
context,
crate_id,
&mut def_collector.collected_traits_impls,
Expand Down Expand Up @@ -535,7 +535,7 @@

if let Some(struct_type) = get_struct_type(&typ) {
errors.extend(take_errors(trait_impl.file_id, resolver));
let current_def_map = def_maps.get_mut(&crate_id).unwrap();
let current_def_map = def_maps.get_mut(&struct_type.borrow().id.krate()).unwrap();
match add_method_to_struct_namespace(
current_def_map,
struct_type,
Expand Down Expand Up @@ -1030,7 +1030,7 @@
) {
let the_trait = resolver.interner.get_trait(trait_id);

let self_type = resolver.get_self_type().expect("trait impl must have a Self type");

Check warning on line 1033 in compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (vitkov)

// Temporarily bind the trait's Self type to self_type so we can type check
let _ = the_trait.self_type_typevar.borrow_mut().bind_to(self_type.clone(), the_trait.span);
Expand All @@ -1048,7 +1048,7 @@
the_trait.methods.iter().find(|method| method.name.0.contents == func_name)
{
let function_typ = meta.typ.instantiate(resolver.interner);

Check warning on line 1051 in compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (typecheck)
if let Type::Function(params, _, _) = function_typ.0 {
if method.arguments.len() == params.len() {
// Check the parameters of the impl method against the parameters of the trait method
Expand All @@ -1064,7 +1064,7 @@
parameter_index: parameter_index + 1,
}
});
}

Check warning on line 1067 in compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (typecheck)
} else {
errors.push((
DefCollectorErrorKind::MismatchTraitImplementationNumParameters {
Expand Down Expand Up @@ -1093,7 +1093,7 @@
expr_typ: meta.return_type().to_string(),
expr_span: ret_type_span,
}
});

Check warning on line 1096 in compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (typecheck)

errors.extend(typecheck_errors.iter().cloned().map(|e| (e.into(), *file_id)));
}
Expand All @@ -1104,7 +1104,7 @@

fn resolve_free_functions(
interner: &mut NodeInterner,
crate_id: CrateId,

Check warning on line 1107 in compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (typecheck)
def_maps: &BTreeMap<CrateId, CrateDefMap>,
collected_functions: Vec<UnresolvedFunctions>,
self_type: Option<Type>,
Expand Down
127 changes: 45 additions & 82 deletions compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
use std::{collections::HashMap, vec};
use std::vec;

use fm::FileId;
use noirc_errors::Location;

use crate::{
graph::CrateId,
hir::def_collector::dc_crate::{UnresolvedStruct, UnresolvedTrait},
node_interner::TraitId,
node_interner::{FuncId, StmtId, TraitId, TypeAliasId},
parser::SubModule,
FunctionDefinition, Ident, LetStatement, NoirFunction, NoirStruct, NoirTrait, NoirTraitImpl,
NoirTypeAlias, ParsedModule, TraitImplItem, TraitItem, TypeImpl,
Expand Down Expand Up @@ -243,7 +243,7 @@
let mut definiton_errors = vec![];
for struct_definition in types {
let name = struct_definition.name.clone();

Check warning on line 246 in compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (definiton)
let unresolved = UnresolvedStruct {
file_id: self.file_id,
module_id: self.module_id,
Expand All @@ -257,7 +257,7 @@
definiton_errors.push((error.into(), self.file_id));
continue;
}
};

Check warning on line 260 in compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (definiton)

// Add the struct to scope so its path can be looked up later
let result =
Expand All @@ -272,13 +272,13 @@
definiton_errors.push((error.into(), self.file_id));
}

// And store the TypeId -> StructType mapping somewhere it is reachable

Check warning on line 275 in compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (definiton)
self.def_collector.collected_types.insert(id, unresolved);
}
definiton_errors
}

/// Collect any type aliases definitions declared within the ast.

Check warning on line 281 in compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (definiton)
/// Returns a vector of errors if any type aliases were already defined.
fn collect_type_aliases(
&mut self,
Expand Down Expand Up @@ -316,85 +316,6 @@
errors
}

fn check_for_duplicate_trait_items(
&mut self,
trait_items: &Vec<TraitItem>,
) -> Vec<(CompilationError, FileId)> {
let mut errors: Vec<(CompilationError, FileId)> = vec![];

let mut functions_and_consts: HashMap<&Ident, &TraitItem> = HashMap::new();
let mut types: HashMap<&Ident, &TraitItem> = HashMap::new();

for trait_item in trait_items {
match trait_item {
TraitItem::Function { name, .. } => {
if let Some(prev_item) = functions_and_consts.insert(name, trait_item) {
let error = match prev_item {
TraitItem::Function { name: prev_name, .. } => {
DefCollectorErrorKind::Duplicate {
typ: DuplicateType::TraitAssociatedFunction,
first_def: prev_name.clone(),
second_def: name.clone(),
}
}
TraitItem::Constant { name: prev_name, .. } => {
DefCollectorErrorKind::Duplicate {
typ: DuplicateType::TraitAssociatedItem,
first_def: prev_name.clone(),
second_def: name.clone(),
}
}
TraitItem::Type { .. } => {
unreachable!();
}
};
errors.push((error.into(), self.file_id));
}
}
TraitItem::Constant { name, .. } => {
if let Some(prev_item) = functions_and_consts.insert(name, trait_item) {
let error = match prev_item {
TraitItem::Function { name: prev_name, .. } => {
DefCollectorErrorKind::Duplicate {
typ: DuplicateType::TraitAssociatedItem,
first_def: prev_name.clone(),
second_def: name.clone(),
}
}
TraitItem::Constant { name: prev_name, .. } => {
DefCollectorErrorKind::Duplicate {
typ: DuplicateType::TraitAssociatedConst,
first_def: prev_name.clone(),
second_def: name.clone(),
}
}
TraitItem::Type { .. } => {
unreachable!();
}
};
errors.push((error.into(), self.file_id));
}
}
TraitItem::Type { name } => {
if let Some(prev_item) = types.insert(name, trait_item) {
if let TraitItem::Type { name: prev_name } = prev_item {
let error = DefCollectorErrorKind::Duplicate {
typ: DuplicateType::TraitAssociatedType,
first_def: prev_name.clone(),
second_def: name.clone(),
};
errors.push((error.into(), self.file_id));
} else {
unreachable!();
}
}
}
}
}

errors
}

/// Collect any traits definitions declared within the ast.
/// Returns a vector of errors if any traits were already defined.
fn collect_traits(
Expand Down Expand Up @@ -429,7 +350,49 @@
errors.push((error.into(), self.file_id));
}

errors.append(&mut self.check_for_duplicate_trait_items(&trait_definition.items));
for trait_item in &trait_definition.items {
match trait_item {
TraitItem::Function { name, .. } => {
if let Err((first_def, second_def)) = self.def_collector.def_map.modules
[id.0.local_id.0]
.declare_function(name.clone(), FuncId::dummy_id())
jfecher marked this conversation as resolved.
Show resolved Hide resolved
{
let error = DefCollectorErrorKind::Duplicate {
typ: DuplicateType::TraitAssociatedFunction,
first_def,
second_def,
};
errors.push((error.into(), self.file_id));
}
}
TraitItem::Constant { name, .. } => {
if let Err((first_def, second_def)) = self.def_collector.def_map.modules
[id.0.local_id.0]
.declare_global(name.clone(), StmtId::dummy_id())
{
let error = DefCollectorErrorKind::Duplicate {
typ: DuplicateType::TraitAssociatedConst,
first_def,
second_def,
};
errors.push((error.into(), self.file_id));
}
}
TraitItem::Type { name } => {
if let Err((first_def, second_def)) = self.def_collector.def_map.modules
[id.0.local_id.0]
.declare_type_alias(name.clone(), TypeAliasId::dummy_id())
{
let error = DefCollectorErrorKind::Duplicate {
typ: DuplicateType::TraitAssociatedType,
first_def,
second_def,
};
errors.push((error.into(), self.file_id));
}
}
}
}

// Add all functions that have a default implementation in the trait
let mut unresolved_functions =
Expand Down
2 changes: 0 additions & 2 deletions compiler/noirc_frontend/src/hir/def_collector/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ pub enum DuplicateType {
TraitAssociatedType,
TraitAssociatedConst,
TraitAssociatedFunction,
TraitAssociatedItem,
}

#[derive(Error, Debug, Clone)]
Expand Down Expand Up @@ -86,7 +85,6 @@ impl fmt::Display for DuplicateType {
DuplicateType::TraitAssociatedType => write!(f, "trait associated type"),
DuplicateType::TraitAssociatedConst => write!(f, "trait associated constant"),
DuplicateType::TraitAssociatedFunction => write!(f, "trait associated function"),
DuplicateType::TraitAssociatedItem => write!(f, "trait associated item"),
}
}
}
Expand Down