From 936e761ee687dd7aead9feadbf9cb296a7191304 Mon Sep 17 00:00:00 2001 From: Yordan Madzhunkov Date: Wed, 9 Aug 2023 13:42:37 +0300 Subject: [PATCH 1/3] chore: Refactor DefCollector duplicate errors --- .../src/hir/def_collector/dc_crate.rs | 13 ++- .../src/hir/def_collector/dc_mod.rs | 30 +++++-- .../src/hir/def_collector/errors.rs | 79 +++++-------------- 3 files changed, 55 insertions(+), 67 deletions(-) diff --git a/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs b/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs index 631f0cf86e4..d1f790ffea5 100644 --- a/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -151,7 +151,11 @@ impl DefCollector { .import(name.clone(), ns); if let Err((first_def, second_def)) = result { - let err = DefCollectorErrorKind::DuplicateImport { first_def, second_def }; + let err = DefCollectorErrorKind::Duplicate { + typ: "import".to_string(), + first_def, + second_def, + }; errors.push(err.into_file_diagnostic(root_file_id)); } } @@ -256,8 +260,11 @@ fn collect_impls( let result = module.declare_function(method.name_ident().clone(), *method_id); if let Err((first_def, second_def)) = result { - let err = - DefCollectorErrorKind::DuplicateFunction { first_def, second_def }; + let err = DefCollectorErrorKind::Duplicate { + typ: "function".to_string(), + first_def, + second_def, + }; errors.push(err.into_file_diagnostic(unresolved.file_id)); } } diff --git a/crates/noirc_frontend/src/hir/def_collector/dc_mod.rs b/crates/noirc_frontend/src/hir/def_collector/dc_mod.rs index 9d05539750c..583dcc27b61 100644 --- a/crates/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/crates/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -82,7 +82,11 @@ impl<'a> ModCollector<'a> { self.def_collector.def_map.modules[self.module_id.0].declare_global(name, stmt_id); if let Err((first_def, second_def)) = result { - let err = DefCollectorErrorKind::DuplicateGlobal { first_def, second_def }; + let err = DefCollectorErrorKind::Duplicate { + typ: "global".to_string(), + first_def, + second_def, + }; errors.push(err.into_file_diagnostic(self.file_id)); } @@ -142,7 +146,11 @@ impl<'a> ModCollector<'a> { .declare_function(name, func_id); if let Err((first_def, second_def)) = result { - let error = DefCollectorErrorKind::DuplicateFunction { first_def, second_def }; + let error = DefCollectorErrorKind::Duplicate { + typ: "function".to_string(), + first_def, + second_def, + }; errors.push(error.into_file_diagnostic(self.file_id)); } } @@ -172,7 +180,11 @@ impl<'a> ModCollector<'a> { self.def_collector.def_map.modules[self.module_id.0].declare_struct(name, id); if let Err((first_def, second_def)) = result { - let err = DefCollectorErrorKind::DuplicateFunction { first_def, second_def }; + let err = DefCollectorErrorKind::Duplicate { + typ: "type definition".to_string(), + first_def, + second_def, + }; errors.push(err.into_file_diagnostic(self.file_id)); } @@ -211,7 +223,11 @@ impl<'a> ModCollector<'a> { .declare_type_alias(name, type_alias_id); if let Err((first_def, second_def)) = result { - let err = DefCollectorErrorKind::DuplicateFunction { first_def, second_def }; + let err = DefCollectorErrorKind::Duplicate { + typ: "function".to_string(), + first_def, + second_def, + }; errors.push(err.into_file_diagnostic(self.file_id)); } @@ -323,7 +339,11 @@ impl<'a> ModCollector<'a> { if let Err((first_def, second_def)) = modules[self.module_id.0].declare_child_module(mod_name.to_owned(), mod_id) { - let err = DefCollectorErrorKind::DuplicateModuleDecl { first_def, second_def }; + let err = DefCollectorErrorKind::Duplicate { + typ: "module".to_string(), + first_def, + second_def, + }; errors.push(err.into_file_diagnostic(self.file_id)); return None; } diff --git a/crates/noirc_frontend/src/hir/def_collector/errors.rs b/crates/noirc_frontend/src/hir/def_collector/errors.rs index 03f528020f8..60cfacd627c 100644 --- a/crates/noirc_frontend/src/hir/def_collector/errors.rs +++ b/crates/noirc_frontend/src/hir/def_collector/errors.rs @@ -8,14 +8,8 @@ use thiserror::Error; #[derive(Error, Debug)] pub enum DefCollectorErrorKind { - #[error("duplicate function found in namespace")] - DuplicateFunction { first_def: Ident, second_def: Ident }, - #[error("duplicate function found in namespace")] - DuplicateModuleDecl { first_def: Ident, second_def: Ident }, - #[error("duplicate import")] - DuplicateImport { first_def: Ident, second_def: Ident }, - #[error("duplicate global found in namespace")] - DuplicateGlobal { first_def: Ident, second_def: Ident }, + #[error("duplicate {typ} found in namespace")] + Duplicate { typ: String, first_def: Ident, second_def: Ident }, #[error("unresolved import")] UnresolvedModuleDecl { mod_name: Ident }, #[error("path resolution error")] @@ -35,57 +29,24 @@ impl DefCollectorErrorKind { impl From for Diagnostic { fn from(error: DefCollectorErrorKind) -> Diagnostic { match error { - DefCollectorErrorKind::DuplicateFunction { first_def, second_def } => { - let first_span = first_def.0.span(); - let second_span = second_def.0.span(); - let func_name = &first_def.0.contents; - - let mut diag = Diagnostic::simple_error( - format!("duplicate definitions of {func_name} function found"), - "first definition found here".to_string(), - first_span, - ); - diag.add_secondary("second definition found here".to_string(), second_span); - diag - } - DefCollectorErrorKind::DuplicateModuleDecl { first_def, second_def } => { - let first_span = first_def.0.span(); - let second_span = second_def.0.span(); - let mod_name = &first_def.0.contents; - - let mut diag = Diagnostic::simple_error( - format!("module {mod_name} has been declared twice"), - "first declaration found here".to_string(), - first_span, - ); - diag.add_secondary("second declaration found here".to_string(), second_span); - diag - } - DefCollectorErrorKind::DuplicateImport { first_def, second_def } => { - let first_span = first_def.0.span(); - let second_span = second_def.0.span(); - let import_name = &first_def.0.contents; - - let mut diag = Diagnostic::simple_error( - format!("the name `{import_name}` is defined multiple times"), - "first import found here".to_string(), - first_span, - ); - diag.add_secondary("second import found here".to_string(), second_span); - diag - } - DefCollectorErrorKind::DuplicateGlobal { first_def, second_def } => { - let first_span = first_def.0.span(); - let second_span = second_def.0.span(); - let import_name = &first_def.0.contents; - - let mut diag = Diagnostic::simple_error( - format!("the name `{import_name}` is defined multiple times"), - "first global declaration found here".to_string(), - first_span, - ); - diag.add_secondary("second global declaration found here".to_string(), second_span); - diag + DefCollectorErrorKind::Duplicate { typ, first_def, second_def } => { + let primary_message = + format!("duplicate definitions of {} function found", &first_def.0.contents); + { + let duplicate_type: &str = &typ; + let first_span = first_def.0.span(); + let second_span = second_def.0.span(); + let mut diag = Diagnostic::simple_error( + primary_message, + format!("first {} found here", duplicate_type), + first_span, + ); + diag.add_secondary( + format!("second {} found here", duplicate_type), + second_span, + ); + diag + } } DefCollectorErrorKind::UnresolvedModuleDecl { mod_name } => { let span = mod_name.0.span(); From ca531e6067bcef7f56d53e05eab3df9ebc9e9917 Mon Sep 17 00:00:00 2001 From: Yordan Madzhunkov Date: Wed, 9 Aug 2023 14:16:00 +0300 Subject: [PATCH 2/3] Better message --- crates/noirc_frontend/src/hir/def_collector/errors.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_frontend/src/hir/def_collector/errors.rs b/crates/noirc_frontend/src/hir/def_collector/errors.rs index 60cfacd627c..524a6810b04 100644 --- a/crates/noirc_frontend/src/hir/def_collector/errors.rs +++ b/crates/noirc_frontend/src/hir/def_collector/errors.rs @@ -31,7 +31,7 @@ impl From for Diagnostic { match error { DefCollectorErrorKind::Duplicate { typ, first_def, second_def } => { let primary_message = - format!("duplicate definitions of {} function found", &first_def.0.contents); + format!("duplicate definitions of {} with name {} found", &typ, &first_def.0.contents); { let duplicate_type: &str = &typ; let first_span = first_def.0.span(); From 0edf9789e0d32735b674f64948758b7a28274e89 Mon Sep 17 00:00:00 2001 From: Yordan Madzhunkov Date: Thu, 10 Aug 2023 10:29:14 +0300 Subject: [PATCH 3/3] Convert duplicate type to enum --- .../src/hir/def_collector/dc_crate.rs | 6 +-- .../src/hir/def_collector/dc_mod.rs | 12 +++--- .../src/hir/def_collector/errors.rs | 40 ++++++++++++++----- 3 files changed, 39 insertions(+), 19 deletions(-) diff --git a/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs b/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs index d1f790ffea5..e256bbd8cb8 100644 --- a/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -1,5 +1,5 @@ use super::dc_mod::collect_defs; -use super::errors::DefCollectorErrorKind; +use super::errors::{DefCollectorErrorKind, DuplicateType}; use crate::graph::CrateId; use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleId}; use crate::hir::resolution::errors::ResolverError; @@ -152,7 +152,7 @@ impl DefCollector { if let Err((first_def, second_def)) = result { let err = DefCollectorErrorKind::Duplicate { - typ: "import".to_string(), + typ: DuplicateType::Import, first_def, second_def, }; @@ -261,7 +261,7 @@ fn collect_impls( if let Err((first_def, second_def)) = result { let err = DefCollectorErrorKind::Duplicate { - typ: "function".to_string(), + typ: DuplicateType::Function, first_def, second_def, }; diff --git a/crates/noirc_frontend/src/hir/def_collector/dc_mod.rs b/crates/noirc_frontend/src/hir/def_collector/dc_mod.rs index 583dcc27b61..545375dc64a 100644 --- a/crates/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/crates/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -9,7 +9,7 @@ use crate::{ use super::{ dc_crate::{DefCollector, UnresolvedFunctions, UnresolvedGlobal, UnresolvedTypeAlias}, - errors::DefCollectorErrorKind, + errors::{DefCollectorErrorKind, DuplicateType}, }; use crate::hir::def_map::{parse_file, LocalModuleId, ModuleData, ModuleId, ModuleOrigin}; use crate::hir::resolution::import::ImportDirective; @@ -83,7 +83,7 @@ impl<'a> ModCollector<'a> { if let Err((first_def, second_def)) = result { let err = DefCollectorErrorKind::Duplicate { - typ: "global".to_string(), + typ: DuplicateType::Global, first_def, second_def, }; @@ -147,7 +147,7 @@ impl<'a> ModCollector<'a> { if let Err((first_def, second_def)) = result { let error = DefCollectorErrorKind::Duplicate { - typ: "function".to_string(), + typ: DuplicateType::Function, first_def, second_def, }; @@ -181,7 +181,7 @@ impl<'a> ModCollector<'a> { if let Err((first_def, second_def)) = result { let err = DefCollectorErrorKind::Duplicate { - typ: "type definition".to_string(), + typ: DuplicateType::TypeDefinition, first_def, second_def, }; @@ -224,7 +224,7 @@ impl<'a> ModCollector<'a> { if let Err((first_def, second_def)) = result { let err = DefCollectorErrorKind::Duplicate { - typ: "function".to_string(), + typ: DuplicateType::Function, first_def, second_def, }; @@ -340,7 +340,7 @@ impl<'a> ModCollector<'a> { modules[self.module_id.0].declare_child_module(mod_name.to_owned(), mod_id) { let err = DefCollectorErrorKind::Duplicate { - typ: "module".to_string(), + typ: DuplicateType::Module, first_def, second_def, }; diff --git a/crates/noirc_frontend/src/hir/def_collector/errors.rs b/crates/noirc_frontend/src/hir/def_collector/errors.rs index 524a6810b04..e218c1dbd8a 100644 --- a/crates/noirc_frontend/src/hir/def_collector/errors.rs +++ b/crates/noirc_frontend/src/hir/def_collector/errors.rs @@ -6,10 +6,20 @@ use noirc_errors::FileDiagnostic; use noirc_errors::Span; use thiserror::Error; +use std::fmt; + +pub enum DuplicateType { + Function, + Module, + Global, + TypeDefinition, + Import, +} + #[derive(Error, Debug)] pub enum DefCollectorErrorKind { - #[error("duplicate {typ} found in namespace")] - Duplicate { typ: String, first_def: Ident, second_def: Ident }, + #[error("duplicate {typ:?} found in namespace")] + Duplicate { typ: DuplicateType, first_def: Ident, second_def: Ident }, #[error("unresolved import")] UnresolvedModuleDecl { mod_name: Ident }, #[error("path resolution error")] @@ -26,25 +36,35 @@ impl DefCollectorErrorKind { } } +impl fmt::Debug for DuplicateType { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match *self { + DuplicateType::Function => write!(f, "function"), + DuplicateType::Module => write!(f, "module"), + DuplicateType::Global => write!(f, "global"), + DuplicateType::TypeDefinition => write!(f, "type definition"), + DuplicateType::Import => write!(f, "import"), + } + } +} + impl From for Diagnostic { fn from(error: DefCollectorErrorKind) -> Diagnostic { match error { DefCollectorErrorKind::Duplicate { typ, first_def, second_def } => { - let primary_message = - format!("duplicate definitions of {} with name {} found", &typ, &first_def.0.contents); + let primary_message = format!( + "duplicate definitions of {:?} with name {} found", + &typ, &first_def.0.contents + ); { - let duplicate_type: &str = &typ; let first_span = first_def.0.span(); let second_span = second_def.0.span(); let mut diag = Diagnostic::simple_error( primary_message, - format!("first {} found here", duplicate_type), + format!("first {:?} found here", &typ), first_span, ); - diag.add_secondary( - format!("second {} found here", duplicate_type), - second_span, - ); + diag.add_secondary(format!("second {:?} found here", &typ), second_span); diag } }