From f602f185d8d097794313e92a34a72ccb77f31d93 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 23 Jul 2024 12:22:18 -0300 Subject: [PATCH 1/3] fix: error on duplicate struct field --- .../src/hir/def_collector/dc_mod.rs | 20 ++++++++++++- .../src/hir/def_collector/errors.rs | 19 +++++++++++++ compiler/noirc_frontend/src/tests.rs | 28 +++++++++++++++++++ 3 files changed, 66 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 762c08b9205..9dc63233e85 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -260,7 +260,8 @@ impl<'a> ModCollector<'a> { } /// Collect any struct definitions declared within the ast. - /// Returns a vector of errors if any structs were already defined. + /// Returns a vector of errors if any structs were already defined, + /// or if a struct has duplicate fields in it. fn collect_structs( &mut self, context: &mut Context, @@ -271,6 +272,23 @@ impl<'a> ModCollector<'a> { for struct_definition in types { let name = struct_definition.name.clone(); + let mut seen_field_names = std::collections::HashSet::new(); + for (field_name, _) in &struct_definition.fields { + if seen_field_names.insert(field_name) { + continue; + } + + let previous_field_name = *seen_field_names.get(field_name).unwrap(); + definition_errors.push(( + DefCollectorErrorKind::DuplicateField { + first_def: previous_field_name.clone(), + second_def: field_name.clone(), + } + .into(), + self.file_id, + )) + } + let unresolved = UnresolvedStruct { file_id: self.file_id, module_id: self.module_id, diff --git a/compiler/noirc_frontend/src/hir/def_collector/errors.rs b/compiler/noirc_frontend/src/hir/def_collector/errors.rs index 1ccf8dd4792..e525c9b2b8e 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/errors.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/errors.rs @@ -26,6 +26,8 @@ pub enum DuplicateType { pub enum DefCollectorErrorKind { #[error("duplicate {typ} found in namespace")] Duplicate { typ: DuplicateType, first_def: Ident, second_def: Ident }, + #[error("duplicate struct field {first_def}")] + DuplicateField { first_def: Ident, second_def: Ident }, #[error("unresolved import")] UnresolvedModuleDecl { mod_name: Ident, expected_path: String, alternative_path: String }, #[error("overlapping imports")] @@ -132,6 +134,23 @@ impl<'a> From<&'a DefCollectorErrorKind> for Diagnostic { diag } } + DefCollectorErrorKind::DuplicateField { first_def, second_def } => { + let primary_message = format!( + "Duplicate definitions of struct field with name {} found", + &first_def.0.contents + ); + { + let first_span = first_def.0.span(); + let second_span = second_def.0.span(); + let mut diag = Diagnostic::simple_error( + primary_message, + format!("First definition found here"), + first_span, + ); + diag.add_secondary(format!("Second definition found here"), second_span); + diag + } + } DefCollectorErrorKind::UnresolvedModuleDecl { mod_name, expected_path, alternative_path } => { let span = mod_name.0.span(); let mod_name = &mod_name.0.contents; diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index b4f17489ff7..3910c3c1e48 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -2459,3 +2459,31 @@ fn no_super() { assert_eq!(span.start(), 4); assert_eq!(span.end(), 9); } + +#[test] +fn duplicate_struct_field() { + let src = r#" + struct Foo { + x: i32, + x: i32, + } + + fn main() {} + "#; + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::DefinitionError(DefCollectorErrorKind::DuplicateField { + first_def, + second_def, + }) = &errors[0].0 + else { + panic!("Expected a duplicate field error, got {:?}", errors[0].0); + }; + + assert_eq!(first_def.to_string(), "x"); + assert_eq!(second_def.to_string(), "x"); + + assert_eq!(first_def.span().start(), 26); + assert_eq!(second_def.span().start(), 42); +} From a22027841f932e962aa97ca057150d5f1d447bc0 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 23 Jul 2024 12:24:55 -0300 Subject: [PATCH 2/3] clippy --- compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs | 2 +- compiler/noirc_frontend/src/hir/def_collector/errors.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 9dc63233e85..838c5d3a6c1 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -286,7 +286,7 @@ impl<'a> ModCollector<'a> { } .into(), self.file_id, - )) + )); } let unresolved = UnresolvedStruct { diff --git a/compiler/noirc_frontend/src/hir/def_collector/errors.rs b/compiler/noirc_frontend/src/hir/def_collector/errors.rs index e525c9b2b8e..9e9471c0cb3 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/errors.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/errors.rs @@ -144,10 +144,10 @@ impl<'a> From<&'a DefCollectorErrorKind> for Diagnostic { let second_span = second_def.0.span(); let mut diag = Diagnostic::simple_error( primary_message, - format!("First definition found here"), + "First definition found here".to_string(), first_span, ); - diag.add_secondary(format!("Second definition found here"), second_span); + diag.add_secondary("Second definition found here".to_string(), second_span); diag } } From dd04e45de83ba887b54431f908e83dc71833dbbf Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 23 Jul 2024 12:42:07 -0300 Subject: [PATCH 3/3] Extract `check_duplicate_field_names` function --- .../src/hir/def_collector/dc_mod.rs | 42 +++++++++++-------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 838c5d3a6c1..3591e53a801 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -270,24 +270,9 @@ impl<'a> ModCollector<'a> { ) -> Vec<(CompilationError, FileId)> { let mut definition_errors = vec![]; for struct_definition in types { - let name = struct_definition.name.clone(); + self.check_duplicate_field_names(&struct_definition, &mut definition_errors); - let mut seen_field_names = std::collections::HashSet::new(); - for (field_name, _) in &struct_definition.fields { - if seen_field_names.insert(field_name) { - continue; - } - - let previous_field_name = *seen_field_names.get(field_name).unwrap(); - definition_errors.push(( - DefCollectorErrorKind::DuplicateField { - first_def: previous_field_name.clone(), - second_def: field_name.clone(), - } - .into(), - self.file_id, - )); - } + let name = struct_definition.name.clone(); let unresolved = UnresolvedStruct { file_id: self.file_id, @@ -346,6 +331,29 @@ impl<'a> ModCollector<'a> { definition_errors } + fn check_duplicate_field_names( + &self, + struct_definition: &NoirStruct, + definition_errors: &mut Vec<(CompilationError, FileId)>, + ) { + let mut seen_field_names = std::collections::HashSet::new(); + for (field_name, _) in &struct_definition.fields { + if seen_field_names.insert(field_name) { + continue; + } + + let previous_field_name = *seen_field_names.get(field_name).unwrap(); + definition_errors.push(( + DefCollectorErrorKind::DuplicateField { + first_def: previous_field_name.clone(), + second_def: field_name.clone(), + } + .into(), + self.file_id, + )); + } + } + /// Collect any type aliases definitions declared within the ast. /// Returns a vector of errors if any type aliases were already defined. fn collect_type_aliases(