From f124ff259e0320b641667ab7638794d5ebec583e Mon Sep 17 00:00:00 2001 From: Ary Borenszweig <asterite@gmail.com> Date: Wed, 28 Aug 2024 09:54:54 -0300 Subject: [PATCH 01/10] feat: warning on unused imports --- compiler/noirc_driver/src/lib.rs | 2 + compiler/noirc_frontend/src/elaborator/mod.rs | 14 +++---- .../noirc_frontend/src/elaborator/scope.rs | 42 ++++++++++++------- .../noirc_frontend/src/elaborator/types.rs | 2 +- .../src/hir/def_collector/dc_crate.rs | 2 + .../noirc_frontend/src/hir/def_map/mod.rs | 24 +++++++++++ .../src/hir/def_map/module_data.rs | 22 +++++++++- .../src/hir/resolution/errors.rs | 11 +++++ compiler/noirc_frontend/src/tests.rs | 34 +++++++++++++++ 9 files changed, 128 insertions(+), 25 deletions(-) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index cb3a4d25c9d..4ab66940ab0 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -283,12 +283,14 @@ pub fn check_crate( let macros: &[&dyn MacroProcessor] = if options.disable_macros { &[] } else { &[&aztec_macros::AztecMacro] }; + let error_on_unused_imports = true; let mut errors = vec![]; let diagnostics = CrateDefMap::collect_defs( crate_id, context, options.debug_comptime_in_file.as_deref(), options.arithmetic_generics, + error_on_unused_imports, macros, ); errors.extend(diagnostics.into_iter().map(|(error, file_id)| { diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 53b46536078..e8b38193223 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -11,7 +11,7 @@ use crate::{ UnresolvedTypeAlias, }, def_map::DefMaps, - resolution::{errors::ResolverError, path_resolver::PathResolver}, + resolution::errors::ResolverError, scope::ScopeForest as GenericScopeForest, type_check::{generics::TraitGenerics, TypeCheckError}, }, @@ -36,7 +36,7 @@ use crate::{ hir::{ def_collector::{dc_crate::CollectedItems, errors::DefCollectorErrorKind}, def_map::{LocalModuleId, ModuleDefId, ModuleId, MAIN_FUNCTION}, - resolution::{import::PathResolution, path_resolver::StandardPathResolver}, + resolution::import::PathResolution, Context, }, hir_def::function::{FuncMeta, HirFunction}, @@ -630,10 +630,8 @@ impl<'context> Elaborator<'context> { } } - pub fn resolve_module_by_path(&self, path: Path) -> Option<ModuleId> { - let path_resolver = StandardPathResolver::new(self.module_id()); - - match path_resolver.resolve(self.def_maps, path.clone(), &mut None) { + pub fn resolve_module_by_path(&mut self, path: Path) -> Option<ModuleId> { + match self.resolve_path(path.clone()) { Ok(PathResolution { module_def_id: ModuleDefId::ModuleId(module_id), error }) => { if error.is_some() { None @@ -646,9 +644,7 @@ impl<'context> Elaborator<'context> { } fn resolve_trait_by_path(&mut self, path: Path) -> Option<TraitId> { - let path_resolver = StandardPathResolver::new(self.module_id()); - - let error = match path_resolver.resolve(self.def_maps, path.clone(), &mut None) { + let error = match self.resolve_path(path.clone()) { Ok(PathResolution { module_def_id: ModuleDefId::TraitId(trait_id), error }) => { if let Some(error) = error { self.push_err(error); diff --git a/compiler/noirc_frontend/src/elaborator/scope.rs b/compiler/noirc_frontend/src/elaborator/scope.rs index 3288d10b62e..737b0e0eb6e 100644 --- a/compiler/noirc_frontend/src/elaborator/scope.rs +++ b/compiler/noirc_frontend/src/elaborator/scope.rs @@ -2,6 +2,7 @@ use noirc_errors::{Location, Spanned}; use crate::ast::{PathKind, ERROR_IDENT}; use crate::hir::def_map::{LocalModuleId, ModuleId}; +use crate::hir::resolution::import::{PathResolution, PathResolutionResult}; use crate::hir::resolution::path_resolver::{PathResolver, StandardPathResolver}; use crate::hir::scope::{Scope as GenericScope, ScopeTree as GenericScopeTree}; use crate::macros_api::Ident; @@ -29,7 +30,7 @@ type ScopeTree = GenericScopeTree<String, ResolverMeta>; impl<'context> Elaborator<'context> { pub(super) fn lookup<T: TryFromModuleDefId>(&mut self, path: Path) -> Result<T, ResolverError> { let span = path.span(); - let id = self.resolve_path(path)?; + let id = self.resolve_path_or_error(path)?; T::try_from(id).ok_or_else(|| ResolverError::Expected { expected: T::description(), got: id.as_str().to_owned(), @@ -42,15 +43,36 @@ impl<'context> Elaborator<'context> { ModuleId { krate: self.crate_id, local_id: self.local_module } } - pub(super) fn resolve_path(&mut self, path: Path) -> Result<ModuleDefId, ResolverError> { + pub(super) fn resolve_path_or_error( + &mut self, + path: Path, + ) -> Result<ModuleDefId, ResolverError> { + let path_resolution = self.resolve_path(path)?; + + if let Some(error) = path_resolution.error { + self.push_err(error); + } + + Ok(path_resolution.module_def_id) + } + + pub(super) fn resolve_path(&mut self, path: Path) -> PathResolutionResult { let mut module_id = self.module_id(); let mut path = path; + if path.kind == PathKind::Plain { + self.def_maps.get_mut(&self.crate_id).unwrap().modules[module_id.local_id.0] + .use_import(&path.segments[0].ident); + } + if path.kind == PathKind::Plain && path.first_name() == SELF_TYPE_NAME { if let Some(Type::Struct(struct_type, _)) = &self.self_type { let struct_type = struct_type.borrow(); if path.segments.len() == 1 { - return Ok(ModuleDefId::TypeId(struct_type.id)); + return Ok(PathResolution { + module_def_id: ModuleDefId::TypeId(struct_type.id), + error: None, + }); } module_id = struct_type.id.module_id(); @@ -65,11 +87,7 @@ impl<'context> Elaborator<'context> { self.resolve_path_in_module(path, module_id) } - fn resolve_path_in_module( - &mut self, - path: Path, - module_id: ModuleId, - ) -> Result<ModuleDefId, ResolverError> { + fn resolve_path_in_module(&mut self, path: Path, module_id: ModuleId) -> PathResolutionResult { let resolver = StandardPathResolver::new(module_id); let path_resolution; @@ -99,11 +117,7 @@ impl<'context> Elaborator<'context> { path_resolution = resolver.resolve(self.def_maps, path, &mut None)?; } - if let Some(error) = path_resolution.error { - self.push_err(error); - } - - Ok(path_resolution.module_def_id) + Ok(path_resolution) } pub(super) fn get_struct(&self, type_id: StructId) -> Shared<StructType> { @@ -150,7 +164,7 @@ impl<'context> Elaborator<'context> { pub(super) fn lookup_global(&mut self, path: Path) -> Result<DefinitionId, ResolverError> { let span = path.span(); - let id = self.resolve_path(path)?; + let id = self.resolve_path_or_error(path)?; if let Some(function) = TryFromModuleDefId::try_from(id) { return Ok(self.interner.function_definition_id(function)); diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index 44bded6b92f..3b1ffeb2fc2 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -421,7 +421,7 @@ impl<'context> Elaborator<'context> { } // If we cannot find a local generic of the same name, try to look up a global - match self.resolve_path(path.clone()) { + match self.resolve_path_or_error(path.clone()) { Ok(ModuleDefId::GlobalId(id)) => { if let Some(current_item) = self.current_item { self.interner.add_global_dependency(current_item, id); 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 a961de628a8..7c2eece3054 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -263,6 +263,7 @@ impl DefCollector { // Then added these to the context of DefMaps once they are resolved // let crate_graph = &context.crate_graph[crate_id]; + let error_on_unused_imports = false; for dep in crate_graph.dependencies.clone() { errors.extend(CrateDefMap::collect_defs( @@ -270,6 +271,7 @@ impl DefCollector { context, debug_comptime_in_file, enable_arithmetic_generics, + error_on_unused_imports, macro_processors, )); diff --git a/compiler/noirc_frontend/src/hir/def_map/mod.rs b/compiler/noirc_frontend/src/hir/def_map/mod.rs index e607de52ff1..a7e6e3b5f61 100644 --- a/compiler/noirc_frontend/src/hir/def_map/mod.rs +++ b/compiler/noirc_frontend/src/hir/def_map/mod.rs @@ -19,6 +19,7 @@ mod namespace; pub use namespace::*; use super::def_collector::errors::DefCollectorErrorKind; +use super::resolution::errors::ResolverError; /// The name that is used for a non-contract program's entry-point function. pub const MAIN_FUNCTION: &str = "main"; @@ -77,6 +78,7 @@ impl CrateDefMap { context: &mut Context, debug_comptime_in_file: Option<&str>, enable_arithmetic_generics: bool, + error_on_unused_imports: bool, macro_processors: &[&dyn MacroProcessor], ) -> Vec<(CompilationError, FileId)> { // Check if this Crate has already been compiled @@ -133,6 +135,11 @@ impl CrateDefMap { errors.extend( parsing_errors.iter().map(|e| (e.clone().into(), root_file_id)).collect::<Vec<_>>(), ); + + if error_on_unused_imports { + Self::check_unused_imports(context, crate_id, &mut errors); + } + errors } @@ -298,6 +305,23 @@ impl CrateDefMap { String::new() } } + + fn check_unused_imports( + context: &mut Context, + crate_id: CrateId, + errors: &mut Vec<(CompilationError, FileId)>, + ) { + for module in context.def_maps[&crate_id].modules() { + for ident in module.unused_imports() { + errors.push(( + CompilationError::ResolverError(ResolverError::UnusedImport { + ident: ident.clone(), + }), + module.location.file, + )); + } + } + } } /// Specifies a contract function and extra metadata that diff --git a/compiler/noirc_frontend/src/hir/def_map/module_data.rs b/compiler/noirc_frontend/src/hir/def_map/module_data.rs index 8a0125cfe95..7b14db8be77 100644 --- a/compiler/noirc_frontend/src/hir/def_map/module_data.rs +++ b/compiler/noirc_frontend/src/hir/def_map/module_data.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use noirc_errors::Location; @@ -24,6 +24,10 @@ pub struct ModuleData { /// True if this module is a `contract Foo { ... }` module containing contract functions pub is_contract: bool, + + /// List of all unused imports. Each time something is imported into this module it's added + /// to this set. When it's used, it's removed. At the end of the program only unused imports remain. + unused_imports: HashSet<Ident>, } impl ModuleData { @@ -35,6 +39,7 @@ impl ModuleData { definitions: ItemScope::default(), location, is_contract, + unused_imports: HashSet::new(), } } @@ -121,6 +126,11 @@ impl ModuleData { id: ModuleDefId, is_prelude: bool, ) -> Result<(), (Ident, Ident)> { + // Empty spans could come from implicitly injected imports, and we don't want to track those + if name.span().start() < name.span().end() { + self.unused_imports.insert(name.clone()); + } + self.scope.add_item_to_namespace(name, ItemVisibility::Public, id, None, is_prelude) } @@ -137,4 +147,14 @@ impl ModuleData { pub fn value_definitions(&self) -> impl Iterator<Item = ModuleDefId> + '_ { self.definitions.values().values().flat_map(|a| a.values().map(|(id, _, _)| *id)) } + + /// Marks an ident as being used by an import. + pub fn use_import(&mut self, ident: &Ident) { + self.unused_imports.remove(ident); + } + + /// Returns the list of all unused imports at this moment. + pub fn unused_imports(&self) -> &HashSet<Ident> { + &self.unused_imports + } } diff --git a/compiler/noirc_frontend/src/hir/resolution/errors.rs b/compiler/noirc_frontend/src/hir/resolution/errors.rs index 0aad50d13b2..e5a89e61fc2 100644 --- a/compiler/noirc_frontend/src/hir/resolution/errors.rs +++ b/compiler/noirc_frontend/src/hir/resolution/errors.rs @@ -20,6 +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("Could not find variable in this scope")] VariableNotDeclared { name: String, span: Span }, #[error("path is not an identifier")] @@ -152,6 +154,15 @@ impl<'a> From<&'a ResolverError> for Diagnostic { ident.span(), ) } + ResolverError::UnusedImport { ident } => { + let name = &ident.0.contents; + + Diagnostic::simple_warning( + format!("unused import {name}"), + "unused import ".to_string(), + ident.span(), + ) + } ResolverError::VariableNotDeclared { name, span } => Diagnostic::simple_error( format!("cannot find `{name}` in this scope "), "not found in this scope".to_string(), diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index cc4aae7f447..4757ba216b9 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -3187,3 +3187,37 @@ fn as_trait_path_syntax_no_impl() { use CompilationError::TypeError; assert!(matches!(&errors[0].0, TypeError(TypeCheckError::NoMatchingImplFound { .. }))); } + +#[test] +fn errors_on_unused_import() { + let src = r#" + mod foo { + pub fn bar() {} + pub fn baz() {} + + trait Foo { + } + } + + use foo::bar; + use foo::baz; + use foo::Foo; + + impl Foo for Field { + } + + fn main() { + baz(); + } + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::UnusedImport { ident }) = &errors[0].0 + else { + panic!("Expected an unused import error"); + }; + + assert_eq!(ident.to_string(), "bar"); +} From 0fddfab1655ea432c674c999898a70475b3bd9fd Mon Sep 17 00:00:00 2001 From: Ary Borenszweig <asterite@gmail.com> Date: Wed, 28 Aug 2024 10:25:28 -0300 Subject: [PATCH 02/10] Try to make test pass --- .../noirc_frontend/src/hir/def_collector/dc_crate.rs | 3 ++- compiler/noirc_frontend/src/hir/def_map/mod.rs | 3 +++ compiler/noirc_frontend/src/tests.rs | 12 +++++++++--- 3 files changed, 14 insertions(+), 4 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 7c2eece3054..ff4c0f55223 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -245,6 +245,7 @@ impl DefCollector { /// Collect all of the definitions in a given crate into a CrateDefMap /// Modules which are not a part of the module hierarchy starting with /// the root module, will be ignored. + #[allow(clippy::too_many_arguments)] pub fn collect_crate_and_dependencies( mut def_map: CrateDefMap, context: &mut Context, @@ -252,6 +253,7 @@ impl DefCollector { root_file_id: FileId, debug_comptime_in_file: Option<&str>, enable_arithmetic_generics: bool, + error_on_unused_imports: bool, macro_processors: &[&dyn MacroProcessor], ) -> Vec<(CompilationError, FileId)> { let mut errors: Vec<(CompilationError, FileId)> = vec![]; @@ -263,7 +265,6 @@ impl DefCollector { // Then added these to the context of DefMaps once they are resolved // let crate_graph = &context.crate_graph[crate_id]; - let error_on_unused_imports = false; for dep in crate_graph.dependencies.clone() { errors.extend(CrateDefMap::collect_defs( diff --git a/compiler/noirc_frontend/src/hir/def_map/mod.rs b/compiler/noirc_frontend/src/hir/def_map/mod.rs index a7e6e3b5f61..6a547b8c3a2 100644 --- a/compiler/noirc_frontend/src/hir/def_map/mod.rs +++ b/compiler/noirc_frontend/src/hir/def_map/mod.rs @@ -121,6 +121,8 @@ impl CrateDefMap { extern_prelude: BTreeMap::new(), }; + let error_on_unused_imports_in_dependencies = false; + // Now we want to populate the CrateDefMap using the DefCollector errors.extend(DefCollector::collect_crate_and_dependencies( def_map, @@ -129,6 +131,7 @@ impl CrateDefMap { root_file_id, debug_comptime_in_file, enable_arithmetic_generics, + error_on_unused_imports_in_dependencies, macro_processors, )); diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 4757ba216b9..87ec1cc4953 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -76,15 +76,21 @@ pub(crate) fn get_program(src: &str) -> (ParsedModule, Context, Vec<(Compilation extern_prelude: BTreeMap::new(), }; + let debug_comptime_in_file = None; + let enable_arithmetic_generics = false; + let error_on_unused_imports = true; + let macro_processors = &[]; + // Now we want to populate the CrateDefMap using the DefCollector errors.extend(DefCollector::collect_crate_and_dependencies( def_map, &mut context, program.clone().into_sorted(), root_file_id, - None, // No debug_comptime_in_file - false, // Disallow arithmetic generics - &[], // No macro processors + debug_comptime_in_file, + enable_arithmetic_generics, + error_on_unused_imports, + macro_processors, )); } (program, context, errors) From ad78a455e26643463793e345af7950df4a3c87a9 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig <asterite@gmail.com> Date: Wed, 28 Aug 2024 10:31:25 -0300 Subject: [PATCH 03/10] Use `extend` --- compiler/noirc_frontend/src/hir/def_map/mod.rs | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_map/mod.rs b/compiler/noirc_frontend/src/hir/def_map/mod.rs index 6a547b8c3a2..991bba30ff9 100644 --- a/compiler/noirc_frontend/src/hir/def_map/mod.rs +++ b/compiler/noirc_frontend/src/hir/def_map/mod.rs @@ -314,16 +314,13 @@ impl CrateDefMap { crate_id: CrateId, errors: &mut Vec<(CompilationError, FileId)>, ) { - for module in context.def_maps[&crate_id].modules() { - for ident in module.unused_imports() { - errors.push(( - CompilationError::ResolverError(ResolverError::UnusedImport { - ident: ident.clone(), - }), - module.location.file, - )); - } - } + errors.extend(context.def_maps[&crate_id].modules().iter().flat_map(|(_, module)| { + module.unused_imports().iter().map(|ident| { + let ident = ident.clone(); + let error = CompilationError::ResolverError(ResolverError::UnusedImport { ident }); + (error, module.location.file) + }) + })); } } From 380cd60d55b5ea5cf58452636d0484227895480d Mon Sep 17 00:00:00 2001 From: Ary Borenszweig <asterite@gmail.com> Date: Wed, 28 Aug 2024 10:41:50 -0300 Subject: [PATCH 04/10] Split line --- compiler/noirc_frontend/src/elaborator/scope.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/scope.rs b/compiler/noirc_frontend/src/elaborator/scope.rs index 737b0e0eb6e..a51fd737f74 100644 --- a/compiler/noirc_frontend/src/elaborator/scope.rs +++ b/compiler/noirc_frontend/src/elaborator/scope.rs @@ -61,8 +61,9 @@ impl<'context> Elaborator<'context> { let mut path = path; if path.kind == PathKind::Plain { - self.def_maps.get_mut(&self.crate_id).unwrap().modules[module_id.local_id.0] - .use_import(&path.segments[0].ident); + let def_map = self.def_maps.get_mut(&self.crate_id).unwrap(); + let module_data = &mut def_map.modules[module_id.local_id.0]; + module_data.use_import(&path.segments[0].ident); } if path.kind == PathKind::Plain && path.first_name() == SELF_TYPE_NAME { From b233b2d60d92dc2105396a7ac84ebdba92b6f4f7 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig <asterite@gmail.com> Date: Wed, 28 Aug 2024 11:18:25 -0300 Subject: [PATCH 05/10] Only warn in lib and contract packages --- compiler/noirc_driver/src/lib.rs | 8 +++++--- compiler/noirc_driver/tests/stdlib_warnings.rs | 9 +++++++-- tooling/lsp/src/lib.rs | 8 +++++++- tooling/lsp/src/notifications/mod.rs | 11 +++++++---- tooling/lsp/src/requests/code_lens_request.rs | 3 ++- tooling/lsp/src/requests/mod.rs | 4 +++- tooling/lsp/src/requests/test_run.rs | 4 +++- tooling/lsp/src/requests/tests.rs | 4 +++- tooling/nargo/src/package.rs | 7 +++++++ tooling/nargo_cli/src/cli/check_cmd.rs | 9 ++++++--- tooling/nargo_cli/src/cli/export_cmd.rs | 11 ++++++----- tooling/nargo_cli/src/cli/test_cmd.rs | 8 +++++--- tooling/nargo_cli/tests/stdlib-tests.rs | 4 +++- 13 files changed, 64 insertions(+), 26 deletions(-) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 4ab66940ab0..b7801bf5ba5 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -279,11 +279,11 @@ pub fn check_crate( context: &mut Context, crate_id: CrateId, options: &CompileOptions, + error_on_unused_imports: bool, ) -> CompilationResult<()> { let macros: &[&dyn MacroProcessor] = if options.disable_macros { &[] } else { &[&aztec_macros::AztecMacro] }; - let error_on_unused_imports = true; let mut errors = vec![]; let diagnostics = CrateDefMap::collect_defs( crate_id, @@ -324,7 +324,8 @@ pub fn compile_main( options: &CompileOptions, cached_program: Option<CompiledProgram>, ) -> CompilationResult<CompiledProgram> { - let (_, mut warnings) = check_crate(context, crate_id, options)?; + let error_on_unused_imports = true; + let (_, mut warnings) = check_crate(context, crate_id, options, error_on_unused_imports)?; let main = context.get_main_function(&crate_id).ok_or_else(|| { // TODO(#2155): This error might be a better to exist in Nargo @@ -359,7 +360,8 @@ pub fn compile_contract( crate_id: CrateId, options: &CompileOptions, ) -> CompilationResult<CompiledContract> { - let (_, warnings) = check_crate(context, crate_id, options)?; + let error_on_unused_imports = true; + let (_, warnings) = check_crate(context, crate_id, options, error_on_unused_imports)?; // TODO: We probably want to error if contracts is empty let contracts = context.get_all_contracts(&crate_id); diff --git a/compiler/noirc_driver/tests/stdlib_warnings.rs b/compiler/noirc_driver/tests/stdlib_warnings.rs index e290842480d..330186fd58b 100644 --- a/compiler/noirc_driver/tests/stdlib_warnings.rs +++ b/compiler/noirc_driver/tests/stdlib_warnings.rs @@ -23,9 +23,14 @@ fn stdlib_does_not_produce_constant_warnings() -> Result<(), ErrorsAndWarnings> let mut context = Context::new(file_manager, parsed_files); let root_crate_id = prepare_crate(&mut context, file_name); + let error_on_unused_imports = true; - let ((), warnings) = - noirc_driver::check_crate(&mut context, root_crate_id, &Default::default())?; + let ((), warnings) = noirc_driver::check_crate( + &mut context, + root_crate_id, + &Default::default(), + error_on_unused_imports, + )?; assert_eq!(warnings, Vec::new(), "stdlib is producing {} warnings", warnings.len()); diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index c2885543844..cb432de5e0b 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -415,7 +415,13 @@ fn prepare_package_from_source_string() { let mut state = LspState::new(&client, acvm::blackbox_solver::StubbedBlackBoxSolver); let (mut context, crate_id) = prepare_source(source.to_string(), &mut state); - let _check_result = noirc_driver::check_crate(&mut context, crate_id, &Default::default()); + let error_on_unused_imports = true; + let _check_result = noirc_driver::check_crate( + &mut context, + crate_id, + &Default::default(), + error_on_unused_imports, + ); let main_func_id = context.get_main_function(&crate_id); assert!(main_func_id.is_some()); } diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index 4d2186badc3..0067802e1a7 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -132,10 +132,13 @@ pub(crate) fn process_workspace_for_noir_document( let (mut context, crate_id) = crate::prepare_package(&workspace_file_manager, &parsed_files, package); - let file_diagnostics = match check_crate(&mut context, crate_id, &Default::default()) { - Ok(((), warnings)) => warnings, - Err(errors_and_warnings) => errors_and_warnings, - }; + let error_on_unused_imports = package.error_on_unused_imports(); + let options = &Default::default(); + let file_diagnostics = + match check_crate(&mut context, crate_id, options, error_on_unused_imports) { + Ok(((), warnings)) => warnings, + Err(errors_and_warnings) => errors_and_warnings, + }; // We don't add test headings for a package if it contains no `#[test]` functions if let Some(tests) = get_package_tests_in_crate(&context, &crate_id, &package.name) { diff --git a/tooling/lsp/src/requests/code_lens_request.rs b/tooling/lsp/src/requests/code_lens_request.rs index 9799cf875a9..d23638b37ac 100644 --- a/tooling/lsp/src/requests/code_lens_request.rs +++ b/tooling/lsp/src/requests/code_lens_request.rs @@ -70,9 +70,10 @@ fn on_code_lens_request_inner( })?; let (mut context, crate_id) = prepare_source(source_string, state); + let error_on_unused_imports = false; // We ignore the warnings and errors produced by compilation for producing code lenses // because we can still get the test functions even if compilation fails - let _ = check_crate(&mut context, crate_id, &Default::default()); + let _ = check_crate(&mut context, crate_id, &Default::default(), error_on_unused_imports); let collected_lenses = collect_lenses_for_package(&context, crate_id, &workspace, package, None); diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index e88c7f11465..5da736c36e7 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/tooling/lsp/src/requests/mod.rs @@ -443,7 +443,9 @@ where def_maps = state.cached_def_maps.get(&package_root_path).unwrap(); } else { // We ignore the warnings and errors produced by compilation while resolving the definition - let _ = noirc_driver::check_crate(&mut context, crate_id, &Default::default()); + let error_on_unused_imports = false; + let options = &Default::default(); + let _ = noirc_driver::check_crate(&mut context, crate_id, options, error_on_unused_imports); interner = &context.def_interner; def_maps = &context.def_maps; } diff --git a/tooling/lsp/src/requests/test_run.rs b/tooling/lsp/src/requests/test_run.rs index 5f4f9cd98d0..621e00a9652 100644 --- a/tooling/lsp/src/requests/test_run.rs +++ b/tooling/lsp/src/requests/test_run.rs @@ -61,7 +61,9 @@ fn on_test_run_request_inner( Some(package) => { let (mut context, crate_id) = crate::prepare_package(&workspace_file_manager, &parsed_files, package); - if check_crate(&mut context, crate_id, &Default::default()).is_err() { + let error_on_unused_imports = false; + let options = &Default::default(); + if check_crate(&mut context, crate_id, options, error_on_unused_imports).is_err() { let result = NargoTestRunResult { id: params.id.clone(), result: "error".to_string(), diff --git a/tooling/lsp/src/requests/tests.rs b/tooling/lsp/src/requests/tests.rs index 7203aca7f09..c4fb973491a 100644 --- a/tooling/lsp/src/requests/tests.rs +++ b/tooling/lsp/src/requests/tests.rs @@ -65,7 +65,9 @@ fn on_tests_request_inner( crate::prepare_package(&workspace_file_manager, &parsed_files, package); // We ignore the warnings and errors produced by compilation for producing tests // because we can still get the test functions even if compilation fails - let _ = check_crate(&mut context, crate_id, &Default::default()); + let options = &Default::default(); + let error_on_unused_imports = false; + let _ = check_crate(&mut context, crate_id, options, error_on_unused_imports); // We don't add test headings for a package if it contains no `#[test]` functions get_package_tests_in_crate(&context, &crate_id, &package.name) diff --git a/tooling/nargo/src/package.rs b/tooling/nargo/src/package.rs index f55ca5550a3..cde616a9e32 100644 --- a/tooling/nargo/src/package.rs +++ b/tooling/nargo/src/package.rs @@ -73,4 +73,11 @@ impl Package { pub fn is_library(&self) -> bool { self.package_type == PackageType::Library } + + pub fn error_on_unused_imports(&self) -> bool { + match self.package_type { + PackageType::Library => false, + PackageType::Binary | PackageType::Contract => true, + } + } } diff --git a/tooling/nargo_cli/src/cli/check_cmd.rs b/tooling/nargo_cli/src/cli/check_cmd.rs index 5239070b4d2..63c61dbdc5e 100644 --- a/tooling/nargo_cli/src/cli/check_cmd.rs +++ b/tooling/nargo_cli/src/cli/check_cmd.rs @@ -77,11 +77,13 @@ fn check_package( file_manager: &FileManager, parsed_files: &ParsedFiles, package: &Package, - compile_options: &CompileOptions, + options: &CompileOptions, allow_overwrite: bool, ) -> Result<bool, CompileError> { let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); - check_crate_and_report_errors(&mut context, crate_id, compile_options)?; + let error_on_unused_imports = package.error_on_unused_imports(); + + check_crate_and_report_errors(&mut context, crate_id, options, error_on_unused_imports)?; if package.is_library() || package.is_contract() { // Libraries do not have ABIs while contracts have many, so we cannot generate a `Prover.toml` file. @@ -151,8 +153,9 @@ pub(crate) fn check_crate_and_report_errors( context: &mut Context, crate_id: CrateId, options: &CompileOptions, + error_on_unused_imports: bool, ) -> Result<(), CompileError> { - let result = check_crate(context, crate_id, options); + let result = check_crate(context, crate_id, options, error_on_unused_imports); report_errors(result, &context.file_manager, options.deny_warnings, options.silence_warnings) } diff --git a/tooling/nargo_cli/src/cli/export_cmd.rs b/tooling/nargo_cli/src/cli/export_cmd.rs index 19add7f30dc..e6ad45e7233 100644 --- a/tooling/nargo_cli/src/cli/export_cmd.rs +++ b/tooling/nargo_cli/src/cli/export_cmd.rs @@ -80,10 +80,11 @@ fn compile_exported_functions( parsed_files: &ParsedFiles, workspace: &Workspace, package: &Package, - compile_options: &CompileOptions, + options: &CompileOptions, ) -> Result<(), CliError> { let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); - check_crate_and_report_errors(&mut context, crate_id, compile_options)?; + let error_on_unused_imports = package.error_on_unused_imports(); + check_crate_and_report_errors(&mut context, crate_id, options, error_on_unused_imports)?; let exported_functions = context.get_all_exported_functions_in_crate(&crate_id); @@ -91,14 +92,14 @@ fn compile_exported_functions( exported_functions, |(function_name, function_id)| -> Result<(String, CompiledProgram), CompileError> { // TODO: We should to refactor how to deal with compilation errors to avoid this. - let program = compile_no_check(&mut context, compile_options, function_id, None, false) + let program = compile_no_check(&mut context, options, function_id, None, false) .map_err(|error| vec![FileDiagnostic::from(error)]); let program = report_errors( program.map(|program| (program, Vec::new())), file_manager, - compile_options.deny_warnings, - compile_options.silence_warnings, + options.deny_warnings, + options.silence_warnings, )?; Ok((function_name, program)) diff --git a/tooling/nargo_cli/src/cli/test_cmd.rs b/tooling/nargo_cli/src/cli/test_cmd.rs index 0d7c8fc8bf7..4c5b8e19673 100644 --- a/tooling/nargo_cli/src/cli/test_cmd.rs +++ b/tooling/nargo_cli/src/cli/test_cmd.rs @@ -180,7 +180,8 @@ fn run_test<S: BlackBoxFunctionSolver<FieldElement> + Default>( // We then need to construct a separate copy for each test. let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); - check_crate(&mut context, crate_id, compile_options) + let error_on_unused_imports = package.error_on_unused_imports(); + check_crate(&mut context, crate_id, compile_options, error_on_unused_imports) .expect("Any errors should have occurred when collecting test functions"); let test_functions = context @@ -206,10 +207,11 @@ fn get_tests_in_package( parsed_files: &ParsedFiles, package: &Package, fn_name: FunctionNameMatch, - compile_options: &CompileOptions, + options: &CompileOptions, ) -> Result<Vec<String>, CliError> { let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); - check_crate_and_report_errors(&mut context, crate_id, compile_options)?; + let error_on_unused_imports = package.error_on_unused_imports(); + check_crate_and_report_errors(&mut context, crate_id, options, error_on_unused_imports)?; Ok(context .get_all_test_functions_in_crate_matching(&crate_id, fn_name) diff --git a/tooling/nargo_cli/tests/stdlib-tests.rs b/tooling/nargo_cli/tests/stdlib-tests.rs index 847250c1dc0..4ae3d2d8169 100644 --- a/tooling/nargo_cli/tests/stdlib-tests.rs +++ b/tooling/nargo_cli/tests/stdlib-tests.rs @@ -33,7 +33,9 @@ fn run_stdlib_tests() { let (mut context, dummy_crate_id) = prepare_package(&file_manager, &parsed_files, &dummy_package); - let result = check_crate(&mut context, dummy_crate_id, &Default::default()); + let error_on_unused_imports = false; + let options = &Default::default(); + let result = check_crate(&mut context, dummy_crate_id, options, error_on_unused_imports); report_errors(result, &context.file_manager, true, false) .expect("Error encountered while compiling standard library"); From 6f5d2cc8c70b0b57a3a821be53213723b46e0e30 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig <asterite@gmail.com> Date: Wed, 28 Aug 2024 11:32:21 -0300 Subject: [PATCH 06/10] Introduce `CheckOptions` --- compiler/noirc_driver/src/lib.rs | 62 +++++++++++++++++-- .../noirc_driver/tests/stdlib_warnings.rs | 9 +-- tooling/lsp/src/lib.rs | 8 +-- tooling/lsp/src/notifications/mod.rs | 17 ++--- tooling/lsp/src/requests/code_lens_request.rs | 3 +- tooling/lsp/src/requests/mod.rs | 4 +- tooling/lsp/src/requests/test_run.rs | 4 +- tooling/lsp/src/requests/tests.rs | 4 +- tooling/nargo_cli/src/cli/check_cmd.rs | 14 ++--- tooling/nargo_cli/src/cli/export_cmd.rs | 14 +++-- tooling/nargo_cli/src/cli/test_cmd.rs | 10 ++- tooling/nargo_cli/tests/stdlib-tests.rs | 4 +- 12 files changed, 96 insertions(+), 57 deletions(-) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index b7801bf5ba5..4f45e88f536 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -131,6 +131,56 @@ pub struct CompileOptions { pub skip_underconstrained_check: bool, } +/// Similar to CompileOptions but with some extra fields specific for `check_crate`. +#[derive(Clone, Debug, Default)] +pub struct CheckOptions { + pub expression_width: Option<ExpressionWidth>, + pub bounded_codegen: bool, + pub force_compile: bool, + pub show_ssa: bool, + pub emit_ssa: bool, + pub show_brillig: bool, + pub print_acir: bool, + pub benchmark_codegen: bool, + pub deny_warnings: bool, + pub silence_warnings: bool, + pub disable_macros: bool, + pub show_monomorphized: bool, + pub instrument_debug: bool, + pub force_brillig: bool, + pub debug_comptime_in_file: Option<String>, + pub show_artifact_paths: bool, + pub arithmetic_generics: bool, + pub skip_underconstrained_check: bool, + pub error_on_unused_imports: bool, +} + +impl CheckOptions { + pub fn from_compile_options(options: &CompileOptions, error_on_unused_imports: bool) -> Self { + Self { + expression_width: options.expression_width, + bounded_codegen: options.bounded_codegen, + force_compile: options.force_compile, + show_ssa: options.show_ssa, + emit_ssa: options.emit_ssa, + show_brillig: options.show_brillig, + print_acir: options.print_acir, + benchmark_codegen: options.benchmark_codegen, + deny_warnings: options.deny_warnings, + silence_warnings: options.silence_warnings, + disable_macros: options.disable_macros, + show_monomorphized: options.show_monomorphized, + instrument_debug: options.instrument_debug, + force_brillig: options.force_brillig, + debug_comptime_in_file: options.debug_comptime_in_file.clone(), + show_artifact_paths: options.show_artifact_paths, + arithmetic_generics: options.arithmetic_generics, + skip_underconstrained_check: options.skip_underconstrained_check, + error_on_unused_imports, + } + } +} + pub fn parse_expression_width(input: &str) -> Result<ExpressionWidth, std::io::Error> { use std::io::{Error, ErrorKind}; let width = input @@ -278,8 +328,7 @@ pub fn add_dep( pub fn check_crate( context: &mut Context, crate_id: CrateId, - options: &CompileOptions, - error_on_unused_imports: bool, + options: &CheckOptions, ) -> CompilationResult<()> { let macros: &[&dyn MacroProcessor] = if options.disable_macros { &[] } else { &[&aztec_macros::AztecMacro] }; @@ -290,7 +339,7 @@ pub fn check_crate( context, options.debug_comptime_in_file.as_deref(), options.arithmetic_generics, - error_on_unused_imports, + options.error_on_unused_imports, macros, ); errors.extend(diagnostics.into_iter().map(|(error, file_id)| { @@ -325,7 +374,9 @@ pub fn compile_main( cached_program: Option<CompiledProgram>, ) -> CompilationResult<CompiledProgram> { let error_on_unused_imports = true; - let (_, mut warnings) = check_crate(context, crate_id, options, error_on_unused_imports)?; + let check_options = CheckOptions::from_compile_options(options, error_on_unused_imports); + + let (_, mut warnings) = check_crate(context, crate_id, &check_options)?; let main = context.get_main_function(&crate_id).ok_or_else(|| { // TODO(#2155): This error might be a better to exist in Nargo @@ -361,7 +412,8 @@ pub fn compile_contract( options: &CompileOptions, ) -> CompilationResult<CompiledContract> { let error_on_unused_imports = true; - let (_, warnings) = check_crate(context, crate_id, options, error_on_unused_imports)?; + let check_options = CheckOptions::from_compile_options(options, error_on_unused_imports); + let (_, warnings) = check_crate(context, crate_id, &check_options)?; // TODO: We probably want to error if contracts is empty let contracts = context.get_all_contracts(&crate_id); diff --git a/compiler/noirc_driver/tests/stdlib_warnings.rs b/compiler/noirc_driver/tests/stdlib_warnings.rs index 330186fd58b..e290842480d 100644 --- a/compiler/noirc_driver/tests/stdlib_warnings.rs +++ b/compiler/noirc_driver/tests/stdlib_warnings.rs @@ -23,14 +23,9 @@ fn stdlib_does_not_produce_constant_warnings() -> Result<(), ErrorsAndWarnings> let mut context = Context::new(file_manager, parsed_files); let root_crate_id = prepare_crate(&mut context, file_name); - let error_on_unused_imports = true; - let ((), warnings) = noirc_driver::check_crate( - &mut context, - root_crate_id, - &Default::default(), - error_on_unused_imports, - )?; + let ((), warnings) = + noirc_driver::check_crate(&mut context, root_crate_id, &Default::default())?; assert_eq!(warnings, Vec::new(), "stdlib is producing {} warnings", warnings.len()); diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index cb432de5e0b..c2885543844 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -415,13 +415,7 @@ fn prepare_package_from_source_string() { let mut state = LspState::new(&client, acvm::blackbox_solver::StubbedBlackBoxSolver); let (mut context, crate_id) = prepare_source(source.to_string(), &mut state); - let error_on_unused_imports = true; - let _check_result = noirc_driver::check_crate( - &mut context, - crate_id, - &Default::default(), - error_on_unused_imports, - ); + let _check_result = noirc_driver::check_crate(&mut context, crate_id, &Default::default()); let main_func_id = context.get_main_function(&crate_id); assert!(main_func_id.is_some()); } diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index 0067802e1a7..8b030c9e0aa 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -2,7 +2,7 @@ use std::ops::ControlFlow; use crate::insert_all_files_for_workspace_into_file_manager; use async_lsp::{ErrorCode, LanguageClient, ResponseError}; -use noirc_driver::{check_crate, file_manager_with_stdlib}; +use noirc_driver::{check_crate, file_manager_with_stdlib, CheckOptions}; use noirc_errors::{DiagnosticKind, FileDiagnostic}; use crate::types::{ @@ -132,13 +132,14 @@ pub(crate) fn process_workspace_for_noir_document( let (mut context, crate_id) = crate::prepare_package(&workspace_file_manager, &parsed_files, package); - let error_on_unused_imports = package.error_on_unused_imports(); - let options = &Default::default(); - let file_diagnostics = - match check_crate(&mut context, crate_id, options, error_on_unused_imports) { - Ok(((), warnings)) => warnings, - Err(errors_and_warnings) => errors_and_warnings, - }; + let options = CheckOptions { + error_on_unused_imports: package.error_on_unused_imports(), + ..Default::default() + }; + let file_diagnostics = match check_crate(&mut context, crate_id, &options) { + Ok(((), warnings)) => warnings, + Err(errors_and_warnings) => errors_and_warnings, + }; // We don't add test headings for a package if it contains no `#[test]` functions if let Some(tests) = get_package_tests_in_crate(&context, &crate_id, &package.name) { diff --git a/tooling/lsp/src/requests/code_lens_request.rs b/tooling/lsp/src/requests/code_lens_request.rs index d23638b37ac..9799cf875a9 100644 --- a/tooling/lsp/src/requests/code_lens_request.rs +++ b/tooling/lsp/src/requests/code_lens_request.rs @@ -70,10 +70,9 @@ fn on_code_lens_request_inner( })?; let (mut context, crate_id) = prepare_source(source_string, state); - let error_on_unused_imports = false; // We ignore the warnings and errors produced by compilation for producing code lenses // because we can still get the test functions even if compilation fails - let _ = check_crate(&mut context, crate_id, &Default::default(), error_on_unused_imports); + let _ = check_crate(&mut context, crate_id, &Default::default()); let collected_lenses = collect_lenses_for_package(&context, crate_id, &workspace, package, None); diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index 5da736c36e7..e88c7f11465 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/tooling/lsp/src/requests/mod.rs @@ -443,9 +443,7 @@ where def_maps = state.cached_def_maps.get(&package_root_path).unwrap(); } else { // We ignore the warnings and errors produced by compilation while resolving the definition - let error_on_unused_imports = false; - let options = &Default::default(); - let _ = noirc_driver::check_crate(&mut context, crate_id, options, error_on_unused_imports); + let _ = noirc_driver::check_crate(&mut context, crate_id, &Default::default()); interner = &context.def_interner; def_maps = &context.def_maps; } diff --git a/tooling/lsp/src/requests/test_run.rs b/tooling/lsp/src/requests/test_run.rs index 621e00a9652..5f4f9cd98d0 100644 --- a/tooling/lsp/src/requests/test_run.rs +++ b/tooling/lsp/src/requests/test_run.rs @@ -61,9 +61,7 @@ fn on_test_run_request_inner( Some(package) => { let (mut context, crate_id) = crate::prepare_package(&workspace_file_manager, &parsed_files, package); - let error_on_unused_imports = false; - let options = &Default::default(); - if check_crate(&mut context, crate_id, options, error_on_unused_imports).is_err() { + if check_crate(&mut context, crate_id, &Default::default()).is_err() { let result = NargoTestRunResult { id: params.id.clone(), result: "error".to_string(), diff --git a/tooling/lsp/src/requests/tests.rs b/tooling/lsp/src/requests/tests.rs index c4fb973491a..7203aca7f09 100644 --- a/tooling/lsp/src/requests/tests.rs +++ b/tooling/lsp/src/requests/tests.rs @@ -65,9 +65,7 @@ fn on_tests_request_inner( crate::prepare_package(&workspace_file_manager, &parsed_files, package); // We ignore the warnings and errors produced by compilation for producing tests // because we can still get the test functions even if compilation fails - let options = &Default::default(); - let error_on_unused_imports = false; - let _ = check_crate(&mut context, crate_id, options, error_on_unused_imports); + let _ = check_crate(&mut context, crate_id, &Default::default()); // We don't add test headings for a package if it contains no `#[test]` functions get_package_tests_in_crate(&context, &crate_id, &package.name) diff --git a/tooling/nargo_cli/src/cli/check_cmd.rs b/tooling/nargo_cli/src/cli/check_cmd.rs index 63c61dbdc5e..ed9d8ccd1eb 100644 --- a/tooling/nargo_cli/src/cli/check_cmd.rs +++ b/tooling/nargo_cli/src/cli/check_cmd.rs @@ -10,7 +10,7 @@ use nargo::{ use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; use noirc_abi::{AbiParameter, AbiType, MAIN_RETURN_NAME}; use noirc_driver::{ - check_crate, compute_function_abi, file_manager_with_stdlib, CompileOptions, + check_crate, compute_function_abi, file_manager_with_stdlib, CheckOptions, CompileOptions, NOIR_ARTIFACT_VERSION_STRING, }; use noirc_frontend::{ @@ -77,13 +77,14 @@ fn check_package( file_manager: &FileManager, parsed_files: &ParsedFiles, package: &Package, - options: &CompileOptions, + compile_options: &CompileOptions, allow_overwrite: bool, ) -> Result<bool, CompileError> { let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); let error_on_unused_imports = package.error_on_unused_imports(); - - check_crate_and_report_errors(&mut context, crate_id, options, error_on_unused_imports)?; + let check_options = + CheckOptions::from_compile_options(compile_options, error_on_unused_imports); + check_crate_and_report_errors(&mut context, crate_id, &check_options)?; if package.is_library() || package.is_contract() { // Libraries do not have ABIs while contracts have many, so we cannot generate a `Prover.toml` file. @@ -152,10 +153,9 @@ fn create_input_toml_template( pub(crate) fn check_crate_and_report_errors( context: &mut Context, crate_id: CrateId, - options: &CompileOptions, - error_on_unused_imports: bool, + options: &CheckOptions, ) -> Result<(), CompileError> { - let result = check_crate(context, crate_id, options, error_on_unused_imports); + let result = check_crate(context, crate_id, options); report_errors(result, &context.file_manager, options.deny_warnings, options.silence_warnings) } diff --git a/tooling/nargo_cli/src/cli/export_cmd.rs b/tooling/nargo_cli/src/cli/export_cmd.rs index e6ad45e7233..d2e504b9dfc 100644 --- a/tooling/nargo_cli/src/cli/export_cmd.rs +++ b/tooling/nargo_cli/src/cli/export_cmd.rs @@ -12,7 +12,7 @@ use nargo::workspace::Workspace; use nargo::{insert_all_files_for_workspace_into_file_manager, parse_all}; use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; use noirc_driver::{ - compile_no_check, file_manager_with_stdlib, CompileOptions, CompiledProgram, + compile_no_check, file_manager_with_stdlib, CheckOptions, CompileOptions, CompiledProgram, NOIR_ARTIFACT_VERSION_STRING, }; @@ -80,11 +80,13 @@ fn compile_exported_functions( parsed_files: &ParsedFiles, workspace: &Workspace, package: &Package, - options: &CompileOptions, + compile_options: &CompileOptions, ) -> Result<(), CliError> { let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); let error_on_unused_imports = package.error_on_unused_imports(); - check_crate_and_report_errors(&mut context, crate_id, options, error_on_unused_imports)?; + let check_options = + CheckOptions::from_compile_options(compile_options, error_on_unused_imports); + check_crate_and_report_errors(&mut context, crate_id, &check_options)?; let exported_functions = context.get_all_exported_functions_in_crate(&crate_id); @@ -92,14 +94,14 @@ fn compile_exported_functions( exported_functions, |(function_name, function_id)| -> Result<(String, CompiledProgram), CompileError> { // TODO: We should to refactor how to deal with compilation errors to avoid this. - let program = compile_no_check(&mut context, options, function_id, None, false) + let program = compile_no_check(&mut context, compile_options, function_id, None, false) .map_err(|error| vec![FileDiagnostic::from(error)]); let program = report_errors( program.map(|program| (program, Vec::new())), file_manager, - options.deny_warnings, - options.silence_warnings, + compile_options.deny_warnings, + compile_options.silence_warnings, )?; Ok((function_name, program)) diff --git a/tooling/nargo_cli/src/cli/test_cmd.rs b/tooling/nargo_cli/src/cli/test_cmd.rs index 4c5b8e19673..25f95f7b826 100644 --- a/tooling/nargo_cli/src/cli/test_cmd.rs +++ b/tooling/nargo_cli/src/cli/test_cmd.rs @@ -10,7 +10,8 @@ use nargo::{ }; use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; use noirc_driver::{ - check_crate, file_manager_with_stdlib, CompileOptions, NOIR_ARTIFACT_VERSION_STRING, + check_crate, file_manager_with_stdlib, CheckOptions, CompileOptions, + NOIR_ARTIFACT_VERSION_STRING, }; use noirc_frontend::{ graph::CrateName, @@ -181,7 +182,9 @@ fn run_test<S: BlackBoxFunctionSolver<FieldElement> + Default>( let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); let error_on_unused_imports = package.error_on_unused_imports(); - check_crate(&mut context, crate_id, compile_options, error_on_unused_imports) + let check_options = + CheckOptions::from_compile_options(compile_options, error_on_unused_imports); + check_crate(&mut context, crate_id, &check_options) .expect("Any errors should have occurred when collecting test functions"); let test_functions = context @@ -211,7 +214,8 @@ fn get_tests_in_package( ) -> Result<Vec<String>, CliError> { let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); let error_on_unused_imports = package.error_on_unused_imports(); - check_crate_and_report_errors(&mut context, crate_id, options, error_on_unused_imports)?; + let check_options = CheckOptions::from_compile_options(options, error_on_unused_imports); + check_crate_and_report_errors(&mut context, crate_id, &check_options)?; Ok(context .get_all_test_functions_in_crate_matching(&crate_id, fn_name) diff --git a/tooling/nargo_cli/tests/stdlib-tests.rs b/tooling/nargo_cli/tests/stdlib-tests.rs index 4ae3d2d8169..847250c1dc0 100644 --- a/tooling/nargo_cli/tests/stdlib-tests.rs +++ b/tooling/nargo_cli/tests/stdlib-tests.rs @@ -33,9 +33,7 @@ fn run_stdlib_tests() { let (mut context, dummy_crate_id) = prepare_package(&file_manager, &parsed_files, &dummy_package); - let error_on_unused_imports = false; - let options = &Default::default(); - let result = check_crate(&mut context, dummy_crate_id, options, error_on_unused_imports); + let result = check_crate(&mut context, dummy_crate_id, &Default::default()); report_errors(result, &context.file_manager, true, false) .expect("Error encountered while compiling standard library"); From 1236d2c15b35fae677702cd5a8796d296d0b37b7 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig <asterite@gmail.com> Date: Wed, 28 Aug 2024 11:39:53 -0300 Subject: [PATCH 07/10] Move unused logic to dc_crate --- .../src/hir/def_collector/dc_crate.rs | 19 +++++++++++++++ .../noirc_frontend/src/hir/def_map/mod.rs | 23 +------------------ 2 files changed, 20 insertions(+), 22 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 ff4c0f55223..30c91b42b2e 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -267,6 +267,7 @@ impl DefCollector { let crate_graph = &context.crate_graph[crate_id]; for dep in crate_graph.dependencies.clone() { + let error_on_unused_imports = false; errors.extend(CrateDefMap::collect_defs( dep.crate_id, context, @@ -416,8 +417,26 @@ impl DefCollector { ); } + if error_on_unused_imports { + Self::check_unused_imports(context, crate_id, &mut errors); + } + errors } + + fn check_unused_imports( + context: &Context, + crate_id: CrateId, + errors: &mut Vec<(CompilationError, FileId)>, + ) { + errors.extend(context.def_maps[&crate_id].modules().iter().flat_map(|(_, module)| { + module.unused_imports().iter().map(|ident| { + let ident = ident.clone(); + let error = CompilationError::ResolverError(ResolverError::UnusedImport { ident }); + (error, module.location.file) + }) + })); + } } fn add_import_reference( diff --git a/compiler/noirc_frontend/src/hir/def_map/mod.rs b/compiler/noirc_frontend/src/hir/def_map/mod.rs index 991bba30ff9..758b4cf6e5c 100644 --- a/compiler/noirc_frontend/src/hir/def_map/mod.rs +++ b/compiler/noirc_frontend/src/hir/def_map/mod.rs @@ -19,7 +19,6 @@ mod namespace; pub use namespace::*; use super::def_collector::errors::DefCollectorErrorKind; -use super::resolution::errors::ResolverError; /// The name that is used for a non-contract program's entry-point function. pub const MAIN_FUNCTION: &str = "main"; @@ -121,8 +120,6 @@ impl CrateDefMap { extern_prelude: BTreeMap::new(), }; - let error_on_unused_imports_in_dependencies = false; - // Now we want to populate the CrateDefMap using the DefCollector errors.extend(DefCollector::collect_crate_and_dependencies( def_map, @@ -131,7 +128,7 @@ impl CrateDefMap { root_file_id, debug_comptime_in_file, enable_arithmetic_generics, - error_on_unused_imports_in_dependencies, + error_on_unused_imports, macro_processors, )); @@ -139,10 +136,6 @@ impl CrateDefMap { parsing_errors.iter().map(|e| (e.clone().into(), root_file_id)).collect::<Vec<_>>(), ); - if error_on_unused_imports { - Self::check_unused_imports(context, crate_id, &mut errors); - } - errors } @@ -308,20 +301,6 @@ impl CrateDefMap { String::new() } } - - fn check_unused_imports( - context: &mut Context, - crate_id: CrateId, - errors: &mut Vec<(CompilationError, FileId)>, - ) { - errors.extend(context.def_maps[&crate_id].modules().iter().flat_map(|(_, module)| { - module.unused_imports().iter().map(|ident| { - let ident = ident.clone(); - let error = CompilationError::ResolverError(ResolverError::UnusedImport { ident }); - (error, module.location.file) - }) - })); - } } /// Specifies a contract function and extra metadata that From b50f1daa919f222c72359a8b170a3e17259ebec8 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig <asterite@gmail.com> Date: Wed, 28 Aug 2024 13:38:59 -0300 Subject: [PATCH 08/10] Simplify CheckOptions --- compiler/noirc_driver/src/lib.rs | 54 +++++-------------------- tooling/nargo_cli/src/cli/check_cmd.rs | 8 ++-- tooling/nargo_cli/src/cli/export_cmd.rs | 3 +- tooling/nargo_cli/src/cli/test_cmd.rs | 5 +-- 4 files changed, 16 insertions(+), 54 deletions(-) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 4f45e88f536..f95c9de7c2c 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -131,53 +131,15 @@ pub struct CompileOptions { pub skip_underconstrained_check: bool, } -/// Similar to CompileOptions but with some extra fields specific for `check_crate`. #[derive(Clone, Debug, Default)] pub struct CheckOptions { - pub expression_width: Option<ExpressionWidth>, - pub bounded_codegen: bool, - pub force_compile: bool, - pub show_ssa: bool, - pub emit_ssa: bool, - pub show_brillig: bool, - pub print_acir: bool, - pub benchmark_codegen: bool, - pub deny_warnings: bool, - pub silence_warnings: bool, - pub disable_macros: bool, - pub show_monomorphized: bool, - pub instrument_debug: bool, - pub force_brillig: bool, - pub debug_comptime_in_file: Option<String>, - pub show_artifact_paths: bool, - pub arithmetic_generics: bool, - pub skip_underconstrained_check: bool, + pub compile_options: CompileOptions, pub error_on_unused_imports: bool, } impl CheckOptions { - pub fn from_compile_options(options: &CompileOptions, error_on_unused_imports: bool) -> Self { - Self { - expression_width: options.expression_width, - bounded_codegen: options.bounded_codegen, - force_compile: options.force_compile, - show_ssa: options.show_ssa, - emit_ssa: options.emit_ssa, - show_brillig: options.show_brillig, - print_acir: options.print_acir, - benchmark_codegen: options.benchmark_codegen, - deny_warnings: options.deny_warnings, - silence_warnings: options.silence_warnings, - disable_macros: options.disable_macros, - show_monomorphized: options.show_monomorphized, - instrument_debug: options.instrument_debug, - force_brillig: options.force_brillig, - debug_comptime_in_file: options.debug_comptime_in_file.clone(), - show_artifact_paths: options.show_artifact_paths, - arithmetic_generics: options.arithmetic_generics, - skip_underconstrained_check: options.skip_underconstrained_check, - error_on_unused_imports, - } + pub fn new(compile_options: &CompileOptions, error_on_unused_imports: bool) -> Self { + Self { compile_options: compile_options.clone(), error_on_unused_imports } } } @@ -328,8 +290,10 @@ pub fn add_dep( pub fn check_crate( context: &mut Context, crate_id: CrateId, - options: &CheckOptions, + check_options: &CheckOptions, ) -> CompilationResult<()> { + let options = &check_options.compile_options; + let macros: &[&dyn MacroProcessor] = if options.disable_macros { &[] } else { &[&aztec_macros::AztecMacro] }; @@ -339,7 +303,7 @@ pub fn check_crate( context, options.debug_comptime_in_file.as_deref(), options.arithmetic_generics, - options.error_on_unused_imports, + check_options.error_on_unused_imports, macros, ); errors.extend(diagnostics.into_iter().map(|(error, file_id)| { @@ -374,7 +338,7 @@ pub fn compile_main( cached_program: Option<CompiledProgram>, ) -> CompilationResult<CompiledProgram> { let error_on_unused_imports = true; - let check_options = CheckOptions::from_compile_options(options, error_on_unused_imports); + let check_options = CheckOptions::new(options, error_on_unused_imports); let (_, mut warnings) = check_crate(context, crate_id, &check_options)?; @@ -412,7 +376,7 @@ pub fn compile_contract( options: &CompileOptions, ) -> CompilationResult<CompiledContract> { let error_on_unused_imports = true; - let check_options = CheckOptions::from_compile_options(options, error_on_unused_imports); + let check_options = CheckOptions::new(options, error_on_unused_imports); let (_, warnings) = check_crate(context, crate_id, &check_options)?; // TODO: We probably want to error if contracts is empty diff --git a/tooling/nargo_cli/src/cli/check_cmd.rs b/tooling/nargo_cli/src/cli/check_cmd.rs index ed9d8ccd1eb..1130a82fdfc 100644 --- a/tooling/nargo_cli/src/cli/check_cmd.rs +++ b/tooling/nargo_cli/src/cli/check_cmd.rs @@ -82,8 +82,7 @@ fn check_package( ) -> Result<bool, CompileError> { let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); let error_on_unused_imports = package.error_on_unused_imports(); - let check_options = - CheckOptions::from_compile_options(compile_options, error_on_unused_imports); + let check_options = CheckOptions::new(compile_options, error_on_unused_imports); check_crate_and_report_errors(&mut context, crate_id, &check_options)?; if package.is_library() || package.is_contract() { @@ -153,9 +152,10 @@ fn create_input_toml_template( pub(crate) fn check_crate_and_report_errors( context: &mut Context, crate_id: CrateId, - options: &CheckOptions, + check_options: &CheckOptions, ) -> Result<(), CompileError> { - let result = check_crate(context, crate_id, options); + let options = &check_options.compile_options; + let result = check_crate(context, crate_id, check_options); report_errors(result, &context.file_manager, options.deny_warnings, options.silence_warnings) } diff --git a/tooling/nargo_cli/src/cli/export_cmd.rs b/tooling/nargo_cli/src/cli/export_cmd.rs index d2e504b9dfc..5721dd33e27 100644 --- a/tooling/nargo_cli/src/cli/export_cmd.rs +++ b/tooling/nargo_cli/src/cli/export_cmd.rs @@ -84,8 +84,7 @@ fn compile_exported_functions( ) -> Result<(), CliError> { let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); let error_on_unused_imports = package.error_on_unused_imports(); - let check_options = - CheckOptions::from_compile_options(compile_options, error_on_unused_imports); + let check_options = CheckOptions::new(compile_options, error_on_unused_imports); check_crate_and_report_errors(&mut context, crate_id, &check_options)?; let exported_functions = context.get_all_exported_functions_in_crate(&crate_id); diff --git a/tooling/nargo_cli/src/cli/test_cmd.rs b/tooling/nargo_cli/src/cli/test_cmd.rs index 25f95f7b826..2b0c0fd58db 100644 --- a/tooling/nargo_cli/src/cli/test_cmd.rs +++ b/tooling/nargo_cli/src/cli/test_cmd.rs @@ -182,8 +182,7 @@ fn run_test<S: BlackBoxFunctionSolver<FieldElement> + Default>( let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); let error_on_unused_imports = package.error_on_unused_imports(); - let check_options = - CheckOptions::from_compile_options(compile_options, error_on_unused_imports); + let check_options = CheckOptions::new(compile_options, error_on_unused_imports); check_crate(&mut context, crate_id, &check_options) .expect("Any errors should have occurred when collecting test functions"); @@ -214,7 +213,7 @@ fn get_tests_in_package( ) -> Result<Vec<String>, CliError> { let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); let error_on_unused_imports = package.error_on_unused_imports(); - let check_options = CheckOptions::from_compile_options(options, error_on_unused_imports); + let check_options = CheckOptions::new(options, error_on_unused_imports); check_crate_and_report_errors(&mut context, crate_id, &check_options)?; Ok(context From da7db7b3dcc8b724ac0a757be4e66e3544054794 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig <asterite@gmail.com> Date: Wed, 28 Aug 2024 13:39:27 -0300 Subject: [PATCH 09/10] Fix test --- compiler/noirc_frontend/src/tests.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 87ec1cc4953..254349f8606 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -2431,6 +2431,10 @@ fn use_super() { mod foo { use super::some_func; } + + fn main() { + some_func(); + } "#; assert_no_errors(src); } From add36db6ce4684441b283af08b5c952894798733 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig <asterite@gmail.com> Date: Wed, 28 Aug 2024 13:50:46 -0300 Subject: [PATCH 10/10] Fix test again --- compiler/noirc_frontend/src/tests.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 254349f8606..870c781b89d 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -2430,10 +2430,10 @@ fn use_super() { mod foo { use super::some_func; - } - fn main() { - some_func(); + fn bar() { + some_func(); + } } "#; assert_no_errors(src);