From be0b57e9e4fbe1eeda3bd71e871acbde134247aa Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 3 Sep 2024 07:45:22 -0300 Subject: [PATCH 01/14] Change "warning: unused import" to be "warning: unused item" --- .../src/hir/def_collector/dc_crate.rs | 31 ++++++++++++++----- .../src/hir/resolution/errors.rs | 10 +++--- compiler/noirc_frontend/src/tests.rs | 8 +++-- compiler/noirc_frontend/src/usage_tracker.rs | 18 +++++------ 4 files changed, 41 insertions(+), 26 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 6a6cabe593d..fcd0ead4e68 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -12,7 +12,7 @@ use crate::{Generics, Type}; use crate::hir::resolution::import::{resolve_import, ImportDirective, PathResolution}; use crate::hir::Context; -use crate::macros_api::{Expression, MacroError, MacroProcessor}; +use crate::macros_api::{Expression, MacroError, MacroProcessor, ModuleDefId}; use crate::node_interner::{ FuncId, GlobalId, ModuleAttributes, NodeInterner, ReferenceId, StructId, TraitId, TraitImplId, TypeAliasId, @@ -397,10 +397,11 @@ impl DefCollector { local_id: resolved_import.module_scope, }; - context - .def_interner - .usage_tracker - .add_unused_import(module_id, name.clone()); + context.def_interner.usage_tracker.add_unused_item( + module_id, + name.clone(), + module_def_id, + ); } if visibility != ItemVisibility::Private { @@ -488,20 +489,34 @@ impl DefCollector { crate_id: CrateId, errors: &mut Vec<(CompilationError, FileId)>, ) { - let unused_imports = context.def_interner.usage_tracker.unused_imports().iter(); + let unused_imports = context.def_interner.usage_tracker.unused_items().iter(); let unused_imports = unused_imports.filter(|(module_id, _)| module_id.krate == crate_id); errors.extend(unused_imports.flat_map(|(module_id, usage_tracker)| { let module = &context.def_maps[&crate_id].modules()[module_id.local_id.0]; - usage_tracker.iter().map(|ident| { + usage_tracker.iter().map(|(ident, id)| { let ident = ident.clone(); - let error = CompilationError::ResolverError(ResolverError::UnusedImport { ident }); + let error = CompilationError::ResolverError(ResolverError::UnusedItem { + ident, + item_type: item_type(*id).to_string(), + }); (error, module.location.file) }) })); } } +fn item_type(id: ModuleDefId) -> &'static str { + match id { + ModuleDefId::ModuleId(_) => "module", + ModuleDefId::FunctionId(_) => "function", + ModuleDefId::TypeId(_) => "struct", + ModuleDefId::TypeAliasId(_) => "type alias", + ModuleDefId::TraitId(_) => "trait", + ModuleDefId::GlobalId(_) => "global", + } +} + fn add_import_reference( def_id: crate::macros_api::ModuleDefId, name: &Ident, diff --git a/compiler/noirc_frontend/src/hir/resolution/errors.rs b/compiler/noirc_frontend/src/hir/resolution/errors.rs index cede04dd582..d05b1d9d61b 100644 --- a/compiler/noirc_frontend/src/hir/resolution/errors.rs +++ b/compiler/noirc_frontend/src/hir/resolution/errors.rs @@ -20,8 +20,8 @@ pub enum ResolverError { DuplicateDefinition { name: String, first_span: Span, second_span: Span }, #[error("Unused variable")] UnusedVariable { ident: Ident }, - #[error("Unused import")] - UnusedImport { ident: Ident }, + #[error("Unused {item_type}")] + UnusedItem { ident: Ident, item_type: String }, #[error("Could not find variable in this scope")] VariableNotDeclared { name: String, span: Span }, #[error("path is not an identifier")] @@ -158,12 +158,12 @@ impl<'a> From<&'a ResolverError> for Diagnostic { diagnostic.unnecessary = true; diagnostic } - ResolverError::UnusedImport { ident } => { + ResolverError::UnusedItem { ident, item_type } => { let name = &ident.0.contents; let mut diagnostic = Diagnostic::simple_warning( - format!("unused import {name}"), - "unused import ".to_string(), + format!("unused {item_type} {name}"), + format!("unused {item_type}"), ident.span(), ); diagnostic.unnecessary = true; diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index a30907211a3..40222aa35c3 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -3224,12 +3224,14 @@ fn errors_on_unused_private_import() { let errors = get_program_errors(src); assert_eq!(errors.len(), 1); - let CompilationError::ResolverError(ResolverError::UnusedImport { ident }) = &errors[0].0 + let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item_type }) = + &errors[0].0 else { panic!("Expected an unused import error"); }; assert_eq!(ident.to_string(), "bar"); + assert_eq!(item_type, "import"); } #[test] @@ -3258,12 +3260,14 @@ fn errors_on_unused_pub_crate_import() { let errors = get_program_errors(src); assert_eq!(errors.len(), 1); - let CompilationError::ResolverError(ResolverError::UnusedImport { ident }) = &errors[0].0 + let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item_type }) = + &errors[0].0 else { panic!("Expected an unused import error"); }; assert_eq!(ident.to_string(), "bar"); + assert_eq!(item_type, "import"); } #[test] diff --git a/compiler/noirc_frontend/src/usage_tracker.rs b/compiler/noirc_frontend/src/usage_tracker.rs index d8b7b271734..ec2d686cebf 100644 --- a/compiler/noirc_frontend/src/usage_tracker.rs +++ b/compiler/noirc_frontend/src/usage_tracker.rs @@ -1,26 +1,22 @@ -use std::collections::HashSet; - use rustc_hash::FxHashMap as HashMap; -use crate::{ast::Ident, hir::def_map::ModuleId}; +use crate::{ast::Ident, hir::def_map::ModuleId, macros_api::ModuleDefId}; #[derive(Debug, Default)] pub struct UsageTracker { - /// List of all unused imports in each module. Each time something is imported it's added - /// to the module's set. When it's used, it's removed. At the end of the program only unused imports remain. - unused_imports: HashMap>, + unused_items: HashMap>, } impl UsageTracker { - pub(crate) fn add_unused_import(&mut self, module_id: ModuleId, name: Ident) { - self.unused_imports.entry(module_id).or_default().insert(name); + pub(crate) fn add_unused_item(&mut self, module_id: ModuleId, name: Ident, item: ModuleDefId) { + self.unused_items.entry(module_id).or_default().insert(name, item); } pub(crate) fn mark_as_used(&mut self, current_mod_id: ModuleId, name: &Ident) { - self.unused_imports.entry(current_mod_id).or_default().remove(name); + self.unused_items.entry(current_mod_id).or_default().remove(name); } - pub(crate) fn unused_imports(&self) -> &HashMap> { - &self.unused_imports + pub(crate) fn unused_items(&self) -> &HashMap> { + &self.unused_items } } From 837f53241e26aeede457e8a41dcb38977a2836e7 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 3 Sep 2024 08:31:20 -0300 Subject: [PATCH 02/14] Don't warn for tests or main function --- .../src/hir/def_collector/dc_crate.rs | 38 +++++++++---------- .../src/hir/def_collector/dc_mod.rs | 19 ++++++++-- compiler/noirc_frontend/src/usage_tracker.rs | 19 ++++++++-- 3 files changed, 50 insertions(+), 26 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index fcd0ead4e68..3c98f343a7b 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -253,7 +253,7 @@ impl DefCollector { root_file_id: FileId, debug_comptime_in_file: Option<&str>, enable_arithmetic_generics: bool, - error_on_usage_tracker: bool, + error_on_unused_items: bool, macro_processors: &[&dyn MacroProcessor], ) -> Vec<(CompilationError, FileId)> { let mut errors: Vec<(CompilationError, FileId)> = vec![]; @@ -388,21 +388,14 @@ impl DefCollector { let result = current_def_map.modules[resolved_import.module_scope.0] .import(name.clone(), visibility, module_def_id, is_prelude); - // Empty spans could come from implicitly injected imports, and we don't want to track those - if visibility != ItemVisibility::Public - && name.span().start() < name.span().end() - { - let module_id = ModuleId { - krate: crate_id, - local_id: resolved_import.module_scope, - }; - - context.def_interner.usage_tracker.add_unused_item( - module_id, - name.clone(), - module_def_id, - ); - } + let module_id = + ModuleId { krate: crate_id, local_id: resolved_import.module_scope }; + context.def_interner.usage_tracker.add_unused_item( + module_id, + name.clone(), + module_def_id, + visibility, + ); if visibility != ItemVisibility::Private { let local_id = resolved_import.module_scope; @@ -477,18 +470,23 @@ impl DefCollector { ); } - if error_on_usage_tracker { - Self::check_usage_tracker(context, crate_id, &mut errors); + if error_on_unused_items { + Self::check_unused_items(context, crate_id, &mut errors); } errors } - fn check_usage_tracker( - context: &Context, + fn check_unused_items( + context: &mut Context, crate_id: CrateId, errors: &mut Vec<(CompilationError, FileId)>, ) { + // Assume `main` is used so we don't warn on that + let root = ModuleId { krate: crate_id, local_id: context.def_maps[&crate_id].root }; + let main = Ident::new("main".to_string(), Span::default()); + context.def_interner.usage_tracker.mark_as_used(root, &main); + let unused_imports = context.def_interner.usage_tracker.unused_items().iter(); let unused_imports = unused_imports.filter(|(module_id, _)| module_id.krate == crate_id); 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 590cdc541ce..aa4f938e39c 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -15,7 +15,9 @@ use crate::ast::{ TypeImpl, }; use crate::hir::resolution::errors::ResolverError; -use crate::macros_api::{Expression, NodeInterner, UnresolvedType, UnresolvedTypeData}; +use crate::macros_api::{ + Expression, ModuleDefId, NodeInterner, UnresolvedType, UnresolvedTypeData, +}; use crate::node_interner::ModuleAttributes; use crate::{ graph::CrateId, @@ -246,6 +248,8 @@ impl<'a> ModCollector<'a> { context.def_interner.register_function(func_id, &function.def); } + let is_test = function.def.attributes.is_test_function(); + // Now link this func_id to a crate level map with the noir function and the module id // Encountering a NoirFunction, we retrieve it's module_data to get the namespace // Once we have lowered it to a HirFunction, we retrieve it's Id from the DefInterner @@ -255,8 +259,17 @@ impl<'a> ModCollector<'a> { unresolved_functions.push_fn(self.module_id, func_id, function); // Add function to scope/ns of the module - let result = self.def_collector.def_map.modules[self.module_id.0] - .declare_function(name, visibility, func_id); + let module_data = &mut self.def_collector.def_map.modules[self.module_id.0]; + let result = module_data.declare_function(name.clone(), visibility, func_id); + + if !is_test { + let module_id = ModuleId { krate, local_id: self.module_id }; + let item = ModuleDefId::FunctionId(func_id); + context + .def_interner + .usage_tracker + .add_unused_item(module_id, name, item, visibility); + } if let Err((first_def, second_def)) = result { let error = DefCollectorErrorKind::Duplicate { diff --git a/compiler/noirc_frontend/src/usage_tracker.rs b/compiler/noirc_frontend/src/usage_tracker.rs index ec2d686cebf..4ddb96c6c50 100644 --- a/compiler/noirc_frontend/src/usage_tracker.rs +++ b/compiler/noirc_frontend/src/usage_tracker.rs @@ -1,6 +1,10 @@ use rustc_hash::FxHashMap as HashMap; -use crate::{ast::Ident, hir::def_map::ModuleId, macros_api::ModuleDefId}; +use crate::{ + ast::{Ident, ItemVisibility}, + hir::def_map::ModuleId, + macros_api::ModuleDefId, +}; #[derive(Debug, Default)] pub struct UsageTracker { @@ -8,8 +12,17 @@ pub struct UsageTracker { } impl UsageTracker { - pub(crate) fn add_unused_item(&mut self, module_id: ModuleId, name: Ident, item: ModuleDefId) { - self.unused_items.entry(module_id).or_default().insert(name, item); + pub(crate) fn add_unused_item( + &mut self, + module_id: ModuleId, + name: Ident, + item: ModuleDefId, + visibility: ItemVisibility, + ) { + // Empty spans could come from implicitly injected imports, and we don't want to track those + if visibility != ItemVisibility::Public && name.span().start() < name.span().end() { + self.unused_items.entry(module_id).or_default().insert(name, item); + } } pub(crate) fn mark_as_used(&mut self, current_mod_id: ModuleId, name: &Ident) { From fe6cc5b72a4e3080c8f8a4c450a24187c7cbbd14 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 3 Sep 2024 09:40:47 -0300 Subject: [PATCH 03/14] Introduce UnusedItem to be able to track unused imports, and fix tests --- .../src/hir/def_collector/dc_crate.rs | 20 +++--------- .../src/hir/def_collector/dc_mod.rs | 7 ++--- compiler/noirc_frontend/src/node_interner.rs | 2 +- compiler/noirc_frontend/src/tests.rs | 10 +++--- compiler/noirc_frontend/src/usage_tracker.rs | 31 +++++++++++++++---- 5 files changed, 39 insertions(+), 31 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 3c98f343a7b..bae81fac136 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -7,12 +7,13 @@ use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleId}; use crate::hir::resolution::errors::ResolverError; use crate::hir::resolution::path_resolver; use crate::hir::type_check::TypeCheckError; +use crate::usage_tracker::UnusedItem; use crate::{Generics, Type}; use crate::hir::resolution::import::{resolve_import, ImportDirective, PathResolution}; use crate::hir::Context; -use crate::macros_api::{Expression, MacroError, MacroProcessor, ModuleDefId}; +use crate::macros_api::{Expression, MacroError, MacroProcessor}; use crate::node_interner::{ FuncId, GlobalId, ModuleAttributes, NodeInterner, ReferenceId, StructId, TraitId, TraitImplId, TypeAliasId, @@ -393,7 +394,7 @@ impl DefCollector { context.def_interner.usage_tracker.add_unused_item( module_id, name.clone(), - module_def_id, + UnusedItem::Import, visibility, ); @@ -492,11 +493,11 @@ impl DefCollector { errors.extend(unused_imports.flat_map(|(module_id, usage_tracker)| { let module = &context.def_maps[&crate_id].modules()[module_id.local_id.0]; - usage_tracker.iter().map(|(ident, id)| { + usage_tracker.iter().map(|(ident, unused_item)| { let ident = ident.clone(); let error = CompilationError::ResolverError(ResolverError::UnusedItem { ident, - item_type: item_type(*id).to_string(), + item_type: unused_item.item_type().to_string(), }); (error, module.location.file) }) @@ -504,17 +505,6 @@ impl DefCollector { } } -fn item_type(id: ModuleDefId) -> &'static str { - match id { - ModuleDefId::ModuleId(_) => "module", - ModuleDefId::FunctionId(_) => "function", - ModuleDefId::TypeId(_) => "struct", - ModuleDefId::TypeAliasId(_) => "type alias", - ModuleDefId::TraitId(_) => "trait", - ModuleDefId::GlobalId(_) => "global", - } -} - fn add_import_reference( def_id: crate::macros_api::ModuleDefId, name: &Ident, 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 aa4f938e39c..18bee5579e9 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -15,10 +15,9 @@ use crate::ast::{ TypeImpl, }; use crate::hir::resolution::errors::ResolverError; -use crate::macros_api::{ - Expression, ModuleDefId, NodeInterner, UnresolvedType, UnresolvedTypeData, -}; +use crate::macros_api::{Expression, NodeInterner, UnresolvedType, UnresolvedTypeData}; use crate::node_interner::ModuleAttributes; +use crate::usage_tracker::UnusedItem; use crate::{ graph::CrateId, hir::def_collector::dc_crate::{UnresolvedStruct, UnresolvedTrait}, @@ -264,7 +263,7 @@ impl<'a> ModCollector<'a> { if !is_test { let module_id = ModuleId { krate, local_id: self.module_id }; - let item = ModuleDefId::FunctionId(func_id); + let item = UnusedItem::Function(func_id); context .def_interner .usage_tracker diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 4a73df6a15f..aa51779d24b 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -655,7 +655,7 @@ impl Default for NodeInterner { auto_import_names: HashMap::default(), comptime_scopes: vec![HashMap::default()], trait_impl_associated_types: HashMap::default(), - usage_tracker: UsageTracker::default(), + usage_tracker: UsageTracker::new(), } } } diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 40222aa35c3..068f69733f2 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -1535,7 +1535,7 @@ fn struct_numeric_generic_in_function() { inner: u64 } - fn bar() { } + pub fn bar() { } "#; let errors = get_program_errors(src); assert_eq!(errors.len(), 1); @@ -1567,7 +1567,7 @@ fn struct_numeric_generic_in_struct() { #[test] fn bool_numeric_generic() { let src = r#" - fn read() -> Field { + pub fn read() -> Field { if N { 0 } else { @@ -1586,7 +1586,7 @@ fn bool_numeric_generic() { #[test] fn numeric_generic_binary_operation_type_mismatch() { let src = r#" - fn foo() -> bool { + pub fn foo() -> bool { let mut check: bool = true; check = N; check @@ -2431,7 +2431,7 @@ fn use_super() { mod foo { use super::some_func; - fn bar() { + pub fn bar() { some_func(); } } @@ -3072,7 +3072,7 @@ fn trait_impl_for_a_type_that_implements_another_trait() { } } - fn use_it(t: T) -> i32 where T: Two { + pub fn use_it(t: T) -> i32 where T: Two { Two::two(t) } diff --git a/compiler/noirc_frontend/src/usage_tracker.rs b/compiler/noirc_frontend/src/usage_tracker.rs index 4ddb96c6c50..836f9824436 100644 --- a/compiler/noirc_frontend/src/usage_tracker.rs +++ b/compiler/noirc_frontend/src/usage_tracker.rs @@ -1,22 +1,41 @@ -use rustc_hash::FxHashMap as HashMap; +use std::collections::HashMap; use crate::{ ast::{Ident, ItemVisibility}, hir::def_map::ModuleId, - macros_api::ModuleDefId, + node_interner::FuncId, }; -#[derive(Debug, Default)] +#[derive(Debug)] +pub enum UnusedItem { + Import, + Function(FuncId), +} + +impl UnusedItem { + pub fn item_type(&self) -> &'static str { + match self { + UnusedItem::Import => "import", + UnusedItem::Function(_) => "function", + } + } +} + +#[derive(Debug)] pub struct UsageTracker { - unused_items: HashMap>, + unused_items: HashMap>, } impl UsageTracker { + pub(crate) fn new() -> Self { + Self { unused_items: HashMap::new() } + } + pub(crate) fn add_unused_item( &mut self, module_id: ModuleId, name: Ident, - item: ModuleDefId, + item: UnusedItem, visibility: ItemVisibility, ) { // Empty spans could come from implicitly injected imports, and we don't want to track those @@ -29,7 +48,7 @@ impl UsageTracker { self.unused_items.entry(current_mod_id).or_default().remove(name); } - pub(crate) fn unused_items(&self) -> &HashMap> { + pub(crate) fn unused_items(&self) -> &HashMap> { &self.unused_items } } From 6691cf610a2978c738696a7dafe9e9e2d498fdcf Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 3 Sep 2024 09:42:29 -0300 Subject: [PATCH 04/14] Add a test --- compiler/noirc_frontend/src/tests.rs | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 068f69733f2..6145cdf7c73 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -3227,7 +3227,7 @@ fn errors_on_unused_private_import() { let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item_type }) = &errors[0].0 else { - panic!("Expected an unused import error"); + panic!("Expected an unused item error"); }; assert_eq!(ident.to_string(), "bar"); @@ -3263,7 +3263,7 @@ fn errors_on_unused_pub_crate_import() { let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item_type }) = &errors[0].0 else { - panic!("Expected an unused import error"); + panic!("Expected an unused item error"); }; assert_eq!(ident.to_string(), "bar"); @@ -3345,3 +3345,26 @@ fn warns_on_re_export_of_item_with_less_visibility() { ) )); } + +#[test] +fn errors_on_unused_function() { + let src = r#" + fn foo() { + bar(); + } + + fn bar() {} + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item_type }) = + &errors[0].0 + else { + panic!("Expected an unused item error"); + }; + + assert_eq!(ident.to_string(), "foo"); + assert_eq!(item_type, "function"); +} From 99c2a4fe75588b4092e049e14c5640fc59b2b6ef Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 3 Sep 2024 10:03:45 -0300 Subject: [PATCH 05/14] Fix tests --- compiler/noirc_frontend/src/tests.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 6145cdf7c73..069c0479768 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -1378,7 +1378,7 @@ fn deny_inline_attribute_on_unconstrained() { fn deny_fold_attribute_on_unconstrained() { let src = r#" #[fold] - unconstrained fn foo(x: Field, y: Field) { + unconstrained pub fn foo(x: Field, y: Field) { assert(x != y); } "#; @@ -1633,7 +1633,7 @@ fn bool_generic_as_loop_bound() { #[test] fn numeric_generic_in_function_signature() { let src = r#" - fn foo(arr: [Field; N]) -> [Field; N] { arr } + pub fn foo(arr: [Field; N]) -> [Field; N] { arr } "#; assert_no_errors(src); } @@ -1869,7 +1869,7 @@ fn normal_generic_used_when_numeric_expected_in_where_clause() { fn deserialize(fields: [Field; N]) -> Self; } - fn read() -> T where T: Deserialize { + pub fn read() -> T where T: Deserialize { T::deserialize([0, 1]) } "#; @@ -2445,7 +2445,7 @@ fn use_super_in_path() { fn some_func() {} mod foo { - fn func() { + pub fn func() { super::some_func(); } } @@ -3112,7 +3112,7 @@ fn trait_impl_for_a_type_that_implements_another_trait_with_another_impl_used() } } - fn use_it(t: u32) -> i32 { + pub fn use_it(t: u32) -> i32 { Two::two(t) } From 51753c51229d7415049baf0c3b1fd217695b41d5 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 3 Sep 2024 10:14:31 -0300 Subject: [PATCH 06/14] Another fix --- compiler/noirc_frontend/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 069c0479768..4391ddc8755 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -1885,7 +1885,7 @@ fn normal_generic_used_when_numeric_expected_in_where_clause() { fn deserialize(fields: [Field; N]) -> Self; } - fn read() -> T where T: Deserialize { + pub fn read() -> T where T: Deserialize { let mut fields: [Field; N] = [0; N]; for i in 0..N { fields[i] = i as Field + 1; From 8783c3460d7833e9544343a11a18ec49c761645c Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 3 Sep 2024 10:43:47 -0300 Subject: [PATCH 07/14] More fixes --- compiler/noirc_frontend/src/tests.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 4391ddc8755..c20ce4f4c60 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -1362,7 +1362,7 @@ fn ban_mutable_globals() { fn deny_inline_attribute_on_unconstrained() { let src = r#" #[no_predicates] - unconstrained fn foo(x: Field, y: Field) { + unconstrained pub fn foo(x: Field, y: Field) { assert(x != y); } "#; @@ -1603,7 +1603,7 @@ fn numeric_generic_binary_operation_type_mismatch() { #[test] fn bool_generic_as_loop_bound() { let src = r#" - fn read() { + pub fn read() { let mut fields = [0; N]; for i in 0..N { fields[i] = i + 1; @@ -1675,7 +1675,7 @@ fn normal_generic_as_array_length() { #[test] fn numeric_generic_as_param_type() { let src = r#" - fn foo(x: I) -> I { + pub fn foo(x: I) -> I { let _q: I = 5; x } @@ -1814,7 +1814,7 @@ fn numeric_generic_used_in_where_clause() { fn deserialize(fields: [Field; N]) -> Self; } - fn read() -> T where T: Deserialize { + pub fn read() -> T where T: Deserialize { let mut fields: [Field; N] = [0; N]; for i in 0..N { fields[i] = i as Field + 1; @@ -1828,12 +1828,12 @@ fn numeric_generic_used_in_where_clause() { #[test] fn numeric_generic_used_in_turbofish() { let src = r#" - fn double() -> u32 { + pub fn double() -> u32 { // Used as an expression N * 2 } - fn double_numeric_generics_test() { + pub fn double_numeric_generics_test() { // Example usage of a numeric generic arguments. assert(double::<9>() == 18); assert(double::<7 + 8>() == 30); @@ -2736,7 +2736,7 @@ fn trait_constraint_on_tuple_type() { fn foo(self, x: A) -> bool; } - fn bar(x: (T, U), y: V) -> bool where (T, U): Foo { + pub fn bar(x: (T, U), y: V) -> bool where (T, U): Foo { x.foo(y) } @@ -3280,7 +3280,7 @@ fn warns_on_use_of_private_exported_item() { use bar::baz; - fn qux() { + pub fn qux() { baz(); } } From 725f1f71aedc114643a69be66bcdea9454834866 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 3 Sep 2024 14:17:19 -0300 Subject: [PATCH 08/14] Change UnusedItem item_type to be `&'static str` --- compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs | 2 +- compiler/noirc_frontend/src/hir/resolution/errors.rs | 2 +- compiler/noirc_frontend/src/tests.rs | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index bae81fac136..7fc3c53d0d0 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -497,7 +497,7 @@ impl DefCollector { let ident = ident.clone(); let error = CompilationError::ResolverError(ResolverError::UnusedItem { ident, - item_type: unused_item.item_type().to_string(), + item_type: unused_item.item_type(), }); (error, module.location.file) }) diff --git a/compiler/noirc_frontend/src/hir/resolution/errors.rs b/compiler/noirc_frontend/src/hir/resolution/errors.rs index d05b1d9d61b..c0242748250 100644 --- a/compiler/noirc_frontend/src/hir/resolution/errors.rs +++ b/compiler/noirc_frontend/src/hir/resolution/errors.rs @@ -21,7 +21,7 @@ pub enum ResolverError { #[error("Unused variable")] UnusedVariable { ident: Ident }, #[error("Unused {item_type}")] - UnusedItem { ident: Ident, item_type: String }, + UnusedItem { ident: Ident, item_type: &'static str }, #[error("Could not find variable in this scope")] VariableNotDeclared { name: String, span: Span }, #[error("path is not an identifier")] diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index c20ce4f4c60..4f57ead356b 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -3231,7 +3231,7 @@ fn errors_on_unused_private_import() { }; assert_eq!(ident.to_string(), "bar"); - assert_eq!(item_type, "import"); + assert_eq!(*item_type, "import"); } #[test] @@ -3267,7 +3267,7 @@ fn errors_on_unused_pub_crate_import() { }; assert_eq!(ident.to_string(), "bar"); - assert_eq!(item_type, "import"); + assert_eq!(*item_type, "import"); } #[test] @@ -3366,5 +3366,5 @@ fn errors_on_unused_function() { }; assert_eq!(ident.to_string(), "foo"); - assert_eq!(item_type, "function"); + assert_eq!(*item_type, "function"); } From 6651d089a917b026f373f00f6431a40c02d7e795 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 3 Sep 2024 15:03:29 -0300 Subject: [PATCH 09/14] Remove unused functions in the standard library --- noir_stdlib/src/hash/poseidon/bn254/consts.nr | 5 ----- noir_stdlib/src/hash/poseidon/mod.nr | 8 -------- 2 files changed, 13 deletions(-) diff --git a/noir_stdlib/src/hash/poseidon/bn254/consts.nr b/noir_stdlib/src/hash/poseidon/bn254/consts.nr index 81d78377ce8..3b28ced5835 100644 --- a/noir_stdlib/src/hash/poseidon/bn254/consts.nr +++ b/noir_stdlib/src/hash/poseidon/bn254/consts.nr @@ -5,11 +5,6 @@ // Consistent with https://github.com/iden3/circomlib/blob/master/circuits/poseidon.circom and https://github.com/iden3/circomlib/blob/master/circuits/poseidon_constants.circom use crate::hash::poseidon::PoseidonConfig; use crate::hash::poseidon::config; -// Number of full rounds -// Number of partial rounds -fn rp() -> [u8; 16] { - [56, 57, 56, 60, 60, 63, 64, 63, 60, 66, 60, 65, 70, 60, 64, 68] -} // S-box power fn alpha() -> Field { 5 diff --git a/noir_stdlib/src/hash/poseidon/mod.nr b/noir_stdlib/src/hash/poseidon/mod.nr index 963808f6053..cf9b6187c02 100644 --- a/noir_stdlib/src/hash/poseidon/mod.nr +++ b/noir_stdlib/src/hash/poseidon/mod.nr @@ -1,5 +1,4 @@ mod bn254; // Instantiations of Poseidon for prime field of the same order as BN254 -use crate::field::modulus_num_bits; use crate::hash::Hasher; use crate::default::Default; @@ -166,13 +165,6 @@ fn sigma(x: [Field; O]) -> [Field; O] { y } -// Check security of sponge instantiation -fn check_security(rate: Field, width: Field, security: Field) -> bool { - let n = modulus_num_bits(); - - ((n - 1) as Field * (width - rate) / 2) as u8 > security as u8 -} - struct PoseidonHasher{ _state: [Field], } From 099aab1dc44a5fc7ad271402ff6b8cec076e1f84 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 3 Sep 2024 15:04:38 -0300 Subject: [PATCH 10/14] Revert "Remove unused functions in the standard library" This reverts commit 6651d089a917b026f373f00f6431a40c02d7e795. --- noir_stdlib/src/hash/poseidon/bn254/consts.nr | 5 +++++ noir_stdlib/src/hash/poseidon/mod.nr | 8 ++++++++ 2 files changed, 13 insertions(+) diff --git a/noir_stdlib/src/hash/poseidon/bn254/consts.nr b/noir_stdlib/src/hash/poseidon/bn254/consts.nr index 3b28ced5835..81d78377ce8 100644 --- a/noir_stdlib/src/hash/poseidon/bn254/consts.nr +++ b/noir_stdlib/src/hash/poseidon/bn254/consts.nr @@ -5,6 +5,11 @@ // Consistent with https://github.com/iden3/circomlib/blob/master/circuits/poseidon.circom and https://github.com/iden3/circomlib/blob/master/circuits/poseidon_constants.circom use crate::hash::poseidon::PoseidonConfig; use crate::hash::poseidon::config; +// Number of full rounds +// Number of partial rounds +fn rp() -> [u8; 16] { + [56, 57, 56, 60, 60, 63, 64, 63, 60, 66, 60, 65, 70, 60, 64, 68] +} // S-box power fn alpha() -> Field { 5 diff --git a/noir_stdlib/src/hash/poseidon/mod.nr b/noir_stdlib/src/hash/poseidon/mod.nr index cf9b6187c02..963808f6053 100644 --- a/noir_stdlib/src/hash/poseidon/mod.nr +++ b/noir_stdlib/src/hash/poseidon/mod.nr @@ -1,4 +1,5 @@ mod bn254; // Instantiations of Poseidon for prime field of the same order as BN254 +use crate::field::modulus_num_bits; use crate::hash::Hasher; use crate::default::Default; @@ -165,6 +166,13 @@ fn sigma(x: [Field; O]) -> [Field; O] { y } +// Check security of sponge instantiation +fn check_security(rate: Field, width: Field, security: Field) -> bool { + let n = modulus_num_bits(); + + ((n - 1) as Field * (width - rate) / 2) as u8 > security as u8 +} + struct PoseidonHasher{ _state: [Field], } From eb9e92bf23140cc0ad3f9dc7707071ed6fb8fa4d Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 3 Sep 2024 15:49:08 -0300 Subject: [PATCH 11/14] Don't warn on unused contract entrypoints --- .../noirc_frontend/src/hir/def_collector/dc_crate.rs | 7 +------ .../noirc_frontend/src/hir/def_collector/dc_mod.rs | 12 +++++++++--- compiler/noirc_frontend/src/tests.rs | 9 +++++++++ 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 7fc3c53d0d0..90edbf870e4 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -479,15 +479,10 @@ impl DefCollector { } fn check_unused_items( - context: &mut Context, + context: &Context, crate_id: CrateId, errors: &mut Vec<(CompilationError, FileId)>, ) { - // Assume `main` is used so we don't warn on that - let root = ModuleId { krate: crate_id, local_id: context.def_maps[&crate_id].root }; - let main = Ident::new("main".to_string(), Span::default()); - context.def_interner.usage_tracker.mark_as_used(root, &main); - let unused_imports = context.def_interner.usage_tracker.unused_items().iter(); let unused_imports = unused_imports.filter(|(module_id, _)| module_id.krate == crate_id); 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 18bee5579e9..67cb95fc0c6 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -34,7 +34,7 @@ use super::{ }, errors::{DefCollectorErrorKind, DuplicateType}, }; -use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleData, ModuleId}; +use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleData, ModuleId, MAIN_FUNCTION}; use crate::hir::resolution::import::ImportDirective; use crate::hir::Context; @@ -247,7 +247,14 @@ impl<'a> ModCollector<'a> { context.def_interner.register_function(func_id, &function.def); } + let module_data = &mut self.def_collector.def_map.modules[self.module_id.0]; + let is_test = function.def.attributes.is_test_function(); + let is_entry_point_function = if module_data.is_contract { + function.attributes().is_contract_entry_point() + } else { + function.name() == MAIN_FUNCTION + }; // Now link this func_id to a crate level map with the noir function and the module id // Encountering a NoirFunction, we retrieve it's module_data to get the namespace @@ -258,10 +265,9 @@ impl<'a> ModCollector<'a> { unresolved_functions.push_fn(self.module_id, func_id, function); // Add function to scope/ns of the module - let module_data = &mut self.def_collector.def_map.modules[self.module_id.0]; let result = module_data.declare_function(name.clone(), visibility, func_id); - if !is_test { + if !is_test && !is_entry_point_function { let module_id = ModuleId { krate, local_id: self.module_id }; let item = UnusedItem::Function(func_id); context diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 4f57ead356b..9c4027ea3df 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -3349,6 +3349,15 @@ fn warns_on_re_export_of_item_with_less_visibility() { #[test] fn errors_on_unused_function() { let src = r#" + contract some_contract { + // This function is unused, but it's a contract entrypoint + // so it should not produce a warning + fn foo() -> pub Field { + 1 + } + } + + fn foo() { bar(); } From 97c148e88c9a50285941552a4aa7d8be4c21b58e Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 4 Sep 2024 16:50:10 -0300 Subject: [PATCH 12/14] Fix test --- compiler/noirc_frontend/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index a47375b1484..558a4bcd38c 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -3375,7 +3375,7 @@ fn unoquted_integer_as_integer_token() { #[attr] fn foobar() {} - fn attr(_f: FunctionDefinition) -> Quoted { + pub fn attr(_f: FunctionDefinition) -> Quoted { let serialized_len = 1; // We are testing that when we unoqute $serialized_len, it's unquoted // as the token `1` and not as something else that later won't be parsed correctly From 72914f79dbbdf742169a396446cda76895b7affb Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 4 Sep 2024 17:47:26 -0300 Subject: [PATCH 13/14] nargo fmt --- .../compile_success_empty/unquote_struct/src/main.nr | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test_programs/compile_success_empty/unquote_struct/src/main.nr b/test_programs/compile_success_empty/unquote_struct/src/main.nr index e90711dd710..603440b5c76 100644 --- a/test_programs/compile_success_empty/unquote_struct/src/main.nr +++ b/test_programs/compile_success_empty/unquote_struct/src/main.nr @@ -10,11 +10,13 @@ fn foo(x: Field, y: u32) -> u32 { // Given a function, wrap its parameters in a struct definition comptime fn output_struct(f: FunctionDefinition) -> Quoted { - let fields = f.parameters().map(|param: (Quoted, Type)| { + let fields = f.parameters().map( + |param: (Quoted, Type)| { let name = param.0; let typ = param.1; quote { $name: $typ, } - }).join(quote {}); + } + ).join(quote {}); quote { struct Foo { $fields } From b907e2ff5c933ab9c12df91cdf737bb4dcb6605f Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 4 Sep 2024 18:24:14 -0300 Subject: [PATCH 14/14] Fix test --- compiler/noirc_frontend/src/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 558a4bcd38c..04c4e414858 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -3373,9 +3373,9 @@ fn unoquted_integer_as_integer_token() { } #[attr] - fn foobar() {} + pub fn foobar() {} - pub fn attr(_f: FunctionDefinition) -> Quoted { + fn attr(_f: FunctionDefinition) -> Quoted { let serialized_len = 1; // We are testing that when we unoqute $serialized_len, it's unquoted // as the token `1` and not as something else that later won't be parsed correctly