From e349f30b60a473e2068afafb6fae4a4ea50d185b Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 2 Sep 2024 18:07:31 -0300 Subject: [PATCH] feat: use visibility (#5856) # Description ## Problem Part of #4515 ## Summary We recently added a warning for unused imports... but only for bin and contract packages. We didn't enable it for lib packages because a `use` could also be used as `pub use`, something we don't have yet. I thought it would be really nice if we had `pub use`, and warned on unused imports in libs too. I checked the code and we already track visibility for any item, in general, it's just that for things that don't allow a visibility modifier we just consider it's public. So I tried to see how difficult it would be to implement it, and it turned out it wasn't that hard or time-consuming. That said, visibility for `use` involves some more logic, particularly for autocompletion, because now `pub use` should be suggested, but the "parent" module of that item isn't the actual parent (it's the module where the `pub use` is defined) but that was relatively striaght-forward to implement too. ## Additional Context If we decide to go forward with this, any existing `use` that was used as `pub use` will likely start producing a warning for libs (a lot of them in Aztec-Packages), but now that can be silenced by changing them to `pub use`. Where should this new feature be documented? I'm not sure if it should go in `dependencies.md` or `modules.md`. ## Documentation\* Check one: - [ ] No documentation needed. - [x] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- aztec_macros/src/utils/parse_utils.rs | 6 +- compiler/noirc_driver/src/lib.rs | 28 +---- compiler/noirc_frontend/src/ast/mod.rs | 14 ++- compiler/noirc_frontend/src/ast/statement.rs | 10 +- compiler/noirc_frontend/src/ast/visitor.rs | 11 +- .../noirc_frontend/src/elaborator/comptime.rs | 2 +- .../noirc_frontend/src/elaborator/scope.rs | 67 ++++++----- .../src/hir/def_collector/dc_crate.rs | 109 +++++++++++++---- .../src/hir/def_collector/dc_mod.rs | 1 + .../src/hir/def_collector/errors.rs | 10 +- .../src/hir/def_map/module_data.rs | 25 +--- .../src/hir/resolution/import.rs | 79 +++++++++--- .../src/hir/resolution/path_resolver.rs | 20 +++- compiler/noirc_frontend/src/lib.rs | 1 + compiler/noirc_frontend/src/locations.rs | 31 +++-- compiler/noirc_frontend/src/node_interner.rs | 8 +- .../noirc_frontend/src/noir_parser.lalrpop | 2 +- compiler/noirc_frontend/src/parser/mod.rs | 25 ++-- compiler/noirc_frontend/src/parser/parser.rs | 15 ++- .../src/parser/parser/function.rs | 19 +-- .../src/parser/parser/visibility.rs | 27 +++++ compiler/noirc_frontend/src/tests.rs | 112 +++++++++++++++++- compiler/noirc_frontend/src/usage_tracker.rs | 26 ++++ .../noir/modules_packages_crates/modules.md | 28 ++++- noir_stdlib/src/array.nr | 1 - noir_stdlib/src/collections/map.nr | 1 - noir_stdlib/src/collections/umap.nr | 5 +- noir_stdlib/src/ec/consts/te.nr | 1 - noir_stdlib/src/eddsa.nr | 3 +- noir_stdlib/src/field/bn254.nr | 2 +- noir_stdlib/src/hash/mod.nr | 2 +- noir_stdlib/src/hash/poseidon/bn254.nr | 2 +- noir_stdlib/src/hash/poseidon/bn254/perm.nr | 1 - noir_stdlib/src/ops/mod.nr | 4 +- noir_stdlib/src/prelude.nr | 18 +-- noir_stdlib/src/sha256.nr | 2 +- noir_stdlib/src/sha512.nr | 2 +- noir_stdlib/src/uint128.nr | 1 - tooling/lsp/src/notifications/mod.rs | 8 +- tooling/lsp/src/requests/code_action.rs | 38 ++++-- tooling/lsp/src/requests/code_action/tests.rs | 35 ++++++ tooling/lsp/src/requests/completion.rs | 54 ++------- .../src/requests/completion/auto_import.rs | 38 ++++-- .../requests/completion/completion_items.rs | 47 +++----- tooling/lsp/src/requests/completion/kinds.rs | 12 -- tooling/lsp/src/requests/completion/tests.rs | 86 ++++++++++++-- tooling/nargo/src/package.rs | 7 -- tooling/nargo_cli/src/cli/check_cmd.rs | 11 +- tooling/nargo_cli/src/cli/export_cmd.rs | 6 +- tooling/nargo_cli/src/cli/test_cmd.rs | 11 +- tooling/nargo_fmt/src/rewrite/imports.rs | 16 ++- tooling/nargo_fmt/src/visitor/item.rs | 6 +- 52 files changed, 735 insertions(+), 361 deletions(-) create mode 100644 compiler/noirc_frontend/src/parser/parser/visibility.rs create mode 100644 compiler/noirc_frontend/src/usage_tracker.rs diff --git a/aztec_macros/src/utils/parse_utils.rs b/aztec_macros/src/utils/parse_utils.rs index f2998fbaafc..6a2a876e682 100644 --- a/aztec_macros/src/utils/parse_utils.rs +++ b/aztec_macros/src/utils/parse_utils.rs @@ -50,7 +50,7 @@ fn empty_item(item: &mut Item) { empty_parsed_submodule(parsed_submodule); } ItemKind::ModuleDecl(module_declaration) => empty_module_declaration(module_declaration), - ItemKind::Import(use_tree) => empty_use_tree(use_tree), + ItemKind::Import(use_tree, _) => empty_use_tree(use_tree), ItemKind::Struct(noir_struct) => empty_noir_struct(noir_struct), ItemKind::TypeAlias(noir_type_alias) => empty_noir_type_alias(noir_type_alias), } @@ -404,9 +404,9 @@ fn empty_pattern(pattern: &mut Pattern) { } fn empty_unresolved_trait_constraints( - unresolved_trait_constriants: &mut [UnresolvedTraitConstraint], + unresolved_trait_constraints: &mut [UnresolvedTraitConstraint], ) { - for trait_constraint in unresolved_trait_constriants.iter_mut() { + for trait_constraint in unresolved_trait_constraints.iter_mut() { empty_unresolved_trait_constraint(trait_constraint); } } diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index b7bb07ad64a..88918151366 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -131,18 +131,6 @@ pub struct CompileOptions { pub skip_underconstrained_check: bool, } -#[derive(Clone, Debug, Default)] -pub struct CheckOptions { - pub compile_options: CompileOptions, - pub error_on_unused_imports: bool, -} - -impl CheckOptions { - pub fn new(compile_options: &CompileOptions, error_on_unused_imports: bool) -> Self { - Self { compile_options: compile_options.clone(), error_on_unused_imports } - } -} - pub fn parse_expression_width(input: &str) -> Result { use std::io::{Error, ErrorKind}; let width = input @@ -290,20 +278,19 @@ pub fn add_dep( pub fn check_crate( context: &mut Context, crate_id: CrateId, - check_options: &CheckOptions, + options: &CompileOptions, ) -> CompilationResult<()> { - let options = &check_options.compile_options; - let macros: &[&dyn MacroProcessor] = if options.disable_macros { &[] } else { &[&aztec_macros::AztecMacro] }; let mut errors = vec![]; + let error_on_unused_imports = true; let diagnostics = CrateDefMap::collect_defs( crate_id, context, options.debug_comptime_in_file.as_deref(), options.arithmetic_generics, - check_options.error_on_unused_imports, + error_on_unused_imports, macros, ); errors.extend(diagnostics.into_iter().map(|(error, file_id)| { @@ -337,10 +324,7 @@ pub fn compile_main( options: &CompileOptions, cached_program: Option, ) -> CompilationResult { - let error_on_unused_imports = true; - let check_options = CheckOptions::new(options, error_on_unused_imports); - - let (_, mut warnings) = check_crate(context, crate_id, &check_options)?; + let (_, mut warnings) = check_crate(context, crate_id, options)?; let main = context.get_main_function(&crate_id).ok_or_else(|| { // TODO(#2155): This error might be a better to exist in Nargo @@ -375,9 +359,7 @@ pub fn compile_contract( crate_id: CrateId, options: &CompileOptions, ) -> CompilationResult { - let error_on_unused_imports = true; - let check_options = CheckOptions::new(options, error_on_unused_imports); - let (_, warnings) = check_crate(context, crate_id, &check_options)?; + let (_, warnings) = check_crate(context, crate_id, options)?; // TODO: We probably want to error if contracts is empty let contracts = context.get_all_contracts(&crate_id); diff --git a/compiler/noirc_frontend/src/ast/mod.rs b/compiler/noirc_frontend/src/ast/mod.rs index e63222bfc87..3fd63249201 100644 --- a/compiler/noirc_frontend/src/ast/mod.rs +++ b/compiler/noirc_frontend/src/ast/mod.rs @@ -460,12 +460,22 @@ impl UnresolvedTypeExpression { } } -#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)] /// Represents whether the definition can be referenced outside its module/crate pub enum ItemVisibility { - Public, Private, PublicCrate, + Public, +} + +impl std::fmt::Display for ItemVisibility { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + ItemVisibility::Public => write!(f, "pub"), + ItemVisibility::Private => Ok(()), + ItemVisibility::PublicCrate => write!(f, "pub(crate)"), + } + } } #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)] diff --git a/compiler/noirc_frontend/src/ast/statement.rs b/compiler/noirc_frontend/src/ast/statement.rs index 2fc08e1aea1..2e14761a1cc 100644 --- a/compiler/noirc_frontend/src/ast/statement.rs +++ b/compiler/noirc_frontend/src/ast/statement.rs @@ -7,7 +7,7 @@ use iter_extended::vecmap; use noirc_errors::{Span, Spanned}; use super::{ - BlockExpression, Expression, ExpressionKind, GenericTypeArgs, IndexExpression, + BlockExpression, Expression, ExpressionKind, GenericTypeArgs, IndexExpression, ItemVisibility, MemberAccessExpression, MethodCallExpression, UnresolvedType, }; use crate::elaborator::types::SELF_TYPE_NAME; @@ -302,6 +302,7 @@ impl std::fmt::Display for ModuleDeclaration { #[derive(Debug, PartialEq, Eq, Clone)] pub struct ImportStatement { + pub visibility: ItemVisibility, pub path: Path, pub alias: Option, } @@ -350,7 +351,7 @@ pub enum UseTreeKind { } impl UseTree { - pub fn desugar(self, root: Option) -> Vec { + pub fn desugar(self, root: Option, visibility: ItemVisibility) -> Vec { let prefix = if let Some(mut root) = root { root.segments.extend(self.prefix.segments); root @@ -360,10 +361,11 @@ impl UseTree { match self.kind { UseTreeKind::Path(name, alias) => { - vec![ImportStatement { path: prefix.join(name), alias }] + vec![ImportStatement { visibility, path: prefix.join(name), alias }] } UseTreeKind::List(trees) => { - trees.into_iter().flat_map(|tree| tree.desugar(Some(prefix.clone()))).collect() + let trees = trees.into_iter(); + trees.flat_map(|tree| tree.desugar(Some(prefix.clone()), visibility)).collect() } } } diff --git a/compiler/noirc_frontend/src/ast/visitor.rs b/compiler/noirc_frontend/src/ast/visitor.rs index 96183d3322f..3955e50b03e 100644 --- a/compiler/noirc_frontend/src/ast/visitor.rs +++ b/compiler/noirc_frontend/src/ast/visitor.rs @@ -21,8 +21,9 @@ use crate::{ }; use super::{ - FunctionReturnType, GenericTypeArgs, IntegerBitSize, Pattern, Signedness, UnresolvedGenerics, - UnresolvedTraitConstraint, UnresolvedType, UnresolvedTypeData, UnresolvedTypeExpression, + FunctionReturnType, GenericTypeArgs, IntegerBitSize, ItemVisibility, Pattern, Signedness, + UnresolvedGenerics, UnresolvedTraitConstraint, UnresolvedType, UnresolvedTypeData, + UnresolvedTypeExpression, }; /// Implements the [Visitor pattern](https://en.wikipedia.org/wiki/Visitor_pattern) for Noir's AST. @@ -252,7 +253,7 @@ pub trait Visitor { true } - fn visit_import(&mut self, _: &UseTree) -> bool { + fn visit_import(&mut self, _: &UseTree, _visibility: ItemVisibility) -> bool { true } @@ -470,8 +471,8 @@ impl Item { } } ItemKind::Trait(noir_trait) => noir_trait.accept(self.span, visitor), - ItemKind::Import(use_tree) => { - if visitor.visit_import(use_tree) { + ItemKind::Import(use_tree, visibility) => { + if visitor.visit_import(use_tree, *visibility) { use_tree.accept(visitor); } } diff --git a/compiler/noirc_frontend/src/elaborator/comptime.rs b/compiler/noirc_frontend/src/elaborator/comptime.rs index 3e71f167802..baa9c0ab371 100644 --- a/compiler/noirc_frontend/src/elaborator/comptime.rs +++ b/compiler/noirc_frontend/src/elaborator/comptime.rs @@ -333,7 +333,7 @@ impl<'context> Elaborator<'context> { TopLevelStatement::Error => (), TopLevelStatement::Module(_) - | TopLevelStatement::Import(_) + | TopLevelStatement::Import(..) | TopLevelStatement::Struct(_) | TopLevelStatement::Trait(_) | TopLevelStatement::Impl(_) diff --git a/compiler/noirc_frontend/src/elaborator/scope.rs b/compiler/noirc_frontend/src/elaborator/scope.rs index a51fd737f74..7a98e1856b3 100644 --- a/compiler/noirc_frontend/src/elaborator/scope.rs +++ b/compiler/noirc_frontend/src/elaborator/scope.rs @@ -60,12 +60,6 @@ impl<'context> Elaborator<'context> { let mut module_id = self.module_id(); let mut path = path; - if path.kind == PathKind::Plain { - 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 { if let Some(Type::Struct(struct_type, _)) = &self.self_type { let struct_type = struct_type.borrow(); @@ -90,34 +84,47 @@ impl<'context> Elaborator<'context> { fn resolve_path_in_module(&mut self, path: Path, module_id: ModuleId) -> PathResolutionResult { let resolver = StandardPathResolver::new(module_id); - let path_resolution; - - if self.interner.lsp_mode { - let last_segment = path.last_ident(); - let location = Location::new(last_segment.span(), self.file); - let is_self_type_name = last_segment.is_self_type_name(); - - let mut references: Vec<_> = Vec::new(); - path_resolution = - resolver.resolve(self.def_maps, path.clone(), &mut Some(&mut references))?; - - for (referenced, segment) in references.iter().zip(path.segments) { - self.interner.add_reference( - *referenced, - Location::new(segment.ident.span(), self.file), - segment.ident.is_self_type_name(), - ); - } - self.interner.add_module_def_id_reference( - path_resolution.module_def_id, - location, - is_self_type_name, + if !self.interner.lsp_mode { + return resolver.resolve( + self.def_maps, + path, + &mut self.interner.usage_tracker, + &mut None, + ); + } + + let last_segment = path.last_ident(); + let location = Location::new(last_segment.span(), self.file); + let is_self_type_name = last_segment.is_self_type_name(); + + let mut references: Vec<_> = Vec::new(); + let path_resolution = resolver.resolve( + self.def_maps, + path.clone(), + &mut self.interner.usage_tracker, + &mut Some(&mut references), + ); + + for (referenced, segment) in references.iter().zip(path.segments) { + self.interner.add_reference( + *referenced, + Location::new(segment.ident.span(), self.file), + segment.ident.is_self_type_name(), ); - } else { - path_resolution = resolver.resolve(self.def_maps, path, &mut None)?; } + let path_resolution = match path_resolution { + Ok(path_resolution) => path_resolution, + Err(err) => return Err(err), + }; + + self.interner.add_module_def_id_reference( + path_resolution.module_def_id, + location, + is_self_type_name, + ); + Ok(path_resolution) } 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 30c91b42b2e..6a6cabe593d 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -19,8 +19,8 @@ use crate::node_interner::{ }; use crate::ast::{ - ExpressionKind, GenericTypeArgs, Ident, LetStatement, Literal, NoirFunction, NoirStruct, - NoirTrait, NoirTypeAlias, Path, PathKind, PathSegment, UnresolvedGenerics, + ExpressionKind, GenericTypeArgs, Ident, ItemVisibility, LetStatement, Literal, NoirFunction, + NoirStruct, NoirTrait, NoirTypeAlias, Path, PathKind, PathSegment, UnresolvedGenerics, UnresolvedTraitConstraint, UnresolvedType, }; @@ -253,7 +253,7 @@ impl DefCollector { root_file_id: FileId, debug_comptime_in_file: Option<&str>, enable_arithmetic_generics: bool, - error_on_unused_imports: bool, + error_on_usage_tracker: bool, macro_processors: &[&dyn MacroProcessor], ) -> Vec<(CompilationError, FileId)> { let mut errors: Vec<(CompilationError, FileId)> = vec![]; @@ -267,13 +267,13 @@ impl DefCollector { let crate_graph = &context.crate_graph[crate_id]; for dep in crate_graph.dependencies.clone() { - let error_on_unused_imports = false; + let error_on_usage_tracker = false; errors.extend(CrateDefMap::collect_defs( dep.crate_id, context, debug_comptime_in_file, enable_arithmetic_generics, - error_on_unused_imports, + error_on_usage_tracker, macro_processors, )); @@ -286,8 +286,8 @@ impl DefCollector { def_map.extern_prelude.insert(dep.as_name(), module_id); let location = dep_def_map[dep_def_root].location; - let attriutes = ModuleAttributes { name: dep.as_name(), location, parent: None }; - context.def_interner.add_module_attributes(module_id, attriutes); + let attributes = ModuleAttributes { name: dep.as_name(), location, parent: None }; + context.def_interner.add_module_attributes(module_id, attributes); } // At this point, all dependencies are resolved and type checked. @@ -328,6 +328,7 @@ impl DefCollector { crate_id, &collected_import, &context.def_maps, + &mut context.def_interner.usage_tracker, &mut Some(&mut references), ); @@ -345,31 +346,89 @@ impl DefCollector { resolved_import } else { - resolve_import(crate_id, &collected_import, &context.def_maps, &mut None) + resolve_import( + crate_id, + &collected_import, + &context.def_maps, + &mut context.def_interner.usage_tracker, + &mut None, + ) }; match resolved_import { Ok(resolved_import) => { + let current_def_map = context.def_maps.get_mut(&crate_id).unwrap(); + let file_id = current_def_map.file_id(module_id); + if let Some(error) = resolved_import.error { errors.push(( DefCollectorErrorKind::PathResolutionError(error).into(), - root_file_id, + file_id, )); } // Populate module namespaces according to the imports used - let current_def_map = context.def_maps.get_mut(&crate_id).unwrap(); - let name = resolved_import.name; - for ns in resolved_import.resolved_namespace.iter_defs() { + let visibility = collected_import.visibility; + let is_prelude = resolved_import.is_prelude; + for (module_def_id, item_visibility, _) in + resolved_import.resolved_namespace.iter_items() + { + if item_visibility < visibility { + errors.push(( + DefCollectorErrorKind::CannotReexportItemWithLessVisibility { + item_name: name.clone(), + desired_visibility: visibility, + } + .into(), + file_id, + )); + } + let visibility = visibility.min(item_visibility); + let result = current_def_map.modules[resolved_import.module_scope.0] - .import(name.clone(), ns, resolved_import.is_prelude); + .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_import(module_id, name.clone()); + } + + if visibility != ItemVisibility::Private { + let local_id = resolved_import.module_scope; + let defining_module = ModuleId { krate: crate_id, local_id }; + context.def_interner.register_name_for_auto_import( + name.to_string(), + module_def_id, + visibility, + Some(defining_module), + ); + } - let file_id = current_def_map.file_id(module_id); let last_segment = collected_import.path.last_ident(); - add_import_reference(ns, &last_segment, &mut context.def_interner, file_id); + add_import_reference( + module_def_id, + &last_segment, + &mut context.def_interner, + file_id, + ); if let Some(ref alias) = collected_import.alias { - add_import_reference(ns, alias, &mut context.def_interner, file_id); + add_import_reference( + module_def_id, + alias, + &mut context.def_interner, + file_id, + ); } if let Err((first_def, second_def)) = result { @@ -417,20 +476,24 @@ impl DefCollector { ); } - if error_on_unused_imports { - Self::check_unused_imports(context, crate_id, &mut errors); + if error_on_usage_tracker { + Self::check_usage_tracker(context, crate_id, &mut errors); } errors } - fn check_unused_imports( + fn check_usage_tracker( 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 unused_imports = context.def_interner.usage_tracker.unused_imports().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| { let ident = ident.clone(); let error = CompilationError::ResolverError(ResolverError::UnusedImport { ident }); (error, module.location.file) @@ -456,7 +519,7 @@ fn add_import_reference( fn inject_prelude( crate_id: CrateId, - context: &Context, + context: &mut Context, crate_root: LocalModuleId, collected_imports: &mut Vec, ) { @@ -481,6 +544,7 @@ fn inject_prelude( &context.def_maps, ModuleId { krate: crate_id, local_id: crate_root }, path, + &mut context.def_interner.usage_tracker, &mut None, ) { assert!(error.is_none(), "Tried to add private item to prelude"); @@ -494,6 +558,7 @@ fn inject_prelude( collected_imports.insert( 0, ImportDirective { + visibility: ItemVisibility::Private, module_id: crate_root, path: Path { segments, kind: PathKind::Plain, span: Span::default() }, alias: None, 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 459c4869379..590cdc541ce 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -80,6 +80,7 @@ pub fn collect_defs( // Then add the imports to defCollector to resolve once all modules in the hierarchy have been resolved for import in ast.imports { collector.def_collector.imports.push(ImportDirective { + visibility: import.visibility, module_id: collector.module_id, path: import.path, alias: import.alias, diff --git a/compiler/noirc_frontend/src/hir/def_collector/errors.rs b/compiler/noirc_frontend/src/hir/def_collector/errors.rs index e705d7b6fad..f931a7cdf41 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/errors.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/errors.rs @@ -1,4 +1,4 @@ -use crate::ast::{Ident, Path, UnresolvedTypeData}; +use crate::ast::{Ident, ItemVisibility, Path, UnresolvedTypeData}; use crate::hir::resolution::import::PathResolutionError; use crate::hir::type_check::generics::TraitGenerics; @@ -35,6 +35,8 @@ pub enum DefCollectorErrorKind { OverlappingModuleDecls { mod_name: Ident, expected_path: String, alternative_path: String }, #[error("path resolution error")] PathResolutionError(PathResolutionError), + #[error("cannot re-export {item_name} because it has less visibility than this use statement")] + CannotReexportItemWithLessVisibility { item_name: Ident, desired_visibility: ItemVisibility }, #[error("Non-struct type used in impl")] NonStructTypeInImpl { span: Span }, #[error("Cannot implement trait on a mutable reference type")] @@ -173,6 +175,12 @@ impl<'a> From<&'a DefCollectorErrorKind> for Diagnostic { ) } DefCollectorErrorKind::PathResolutionError(error) => error.into(), + DefCollectorErrorKind::CannotReexportItemWithLessVisibility{item_name, desired_visibility} => { + Diagnostic::simple_warning( + format!("cannot re-export {item_name} because it has less visibility than this use statement"), + format!("consider marking {item_name} as {desired_visibility}"), + item_name.span()) + } DefCollectorErrorKind::NonStructTypeInImpl { span } => Diagnostic::simple_error( "Non-struct type used in impl".into(), "Only struct types may have implementation methods".into(), 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 7b14db8be77..f9542094be7 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, HashSet}; +use std::collections::HashMap; use noirc_errors::Location; @@ -24,10 +24,6 @@ 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, } impl ModuleData { @@ -39,7 +35,6 @@ impl ModuleData { definitions: ItemScope::default(), location, is_contract, - unused_imports: HashSet::new(), } } @@ -123,15 +118,11 @@ impl ModuleData { pub fn import( &mut self, name: Ident, + visibility: ItemVisibility, 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) + self.scope.add_item_to_namespace(name, visibility, id, None, is_prelude) } pub fn find_name(&self, name: &Ident) -> PerNs { @@ -147,14 +138,4 @@ impl ModuleData { pub fn value_definitions(&self) -> impl Iterator + '_ { 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 { - &self.unused_imports - } } diff --git a/compiler/noirc_frontend/src/hir/resolution/import.rs b/compiler/noirc_frontend/src/hir/resolution/import.rs index b820e4664e3..938da0a879f 100644 --- a/compiler/noirc_frontend/src/hir/resolution/import.rs +++ b/compiler/noirc_frontend/src/hir/resolution/import.rs @@ -4,6 +4,7 @@ use thiserror::Error; use crate::graph::CrateId; use crate::hir::def_collector::dc_crate::CompilationError; use crate::node_interner::ReferenceId; +use crate::usage_tracker::UsageTracker; use std::collections::BTreeMap; use crate::ast::{Ident, ItemVisibility, Path, PathKind, PathSegment}; @@ -13,6 +14,7 @@ use super::errors::ResolverError; #[derive(Debug, Clone)] pub struct ImportDirective { + pub visibility: ItemVisibility, pub module_id: LocalModuleId, pub path: Path, pub alias: Option, @@ -86,6 +88,7 @@ pub fn resolve_import( crate_id: CrateId, import_directive: &ImportDirective, def_maps: &BTreeMap, + usage_tracker: &mut UsageTracker, path_references: &mut Option<&mut Vec>, ) -> Result { let module_scope = import_directive.module_id; @@ -93,7 +96,14 @@ pub fn resolve_import( module_id: resolved_module, namespace: resolved_namespace, mut error, - } = resolve_path_to_ns(import_directive, crate_id, crate_id, def_maps, path_references)?; + } = resolve_path_to_ns( + import_directive, + crate_id, + crate_id, + def_maps, + usage_tracker, + path_references, + )?; let name = resolve_path_name(import_directive); @@ -131,10 +141,10 @@ fn resolve_path_to_ns( crate_id: CrateId, importing_crate: CrateId, def_maps: &BTreeMap, + usage_tracker: &mut UsageTracker, path_references: &mut Option<&mut Vec>, ) -> NamespaceResolutionResult { let import_path = &import_directive.path.segments; - let def_map = &def_maps[&crate_id]; match import_directive.path.kind { crate::ast::PathKind::Crate => { @@ -144,6 +154,7 @@ fn resolve_path_to_ns( importing_crate, import_path, def_maps, + usage_tracker, path_references, ) } @@ -157,10 +168,13 @@ fn resolve_path_to_ns( import_path, import_directive.module_id, def_maps, + true, + usage_tracker, path_references, ); } + let def_map = &def_maps[&crate_id]; let current_mod_id = ModuleId { krate: crate_id, local_id: import_directive.module_id }; let current_mod = &def_map.modules[current_mod_id.local_id.0]; let first_segment = @@ -168,9 +182,11 @@ fn resolve_path_to_ns( if current_mod.find_name(first_segment).is_none() { // Resolve externally when first segment is unresolved return resolve_external_dep( - def_map, + crate_id, + // def_map, import_directive, def_maps, + usage_tracker, path_references, importing_crate, ); @@ -182,14 +198,17 @@ fn resolve_path_to_ns( import_path, import_directive.module_id, def_maps, + true, + usage_tracker, path_references, ) } crate::ast::PathKind::Dep => resolve_external_dep( - def_map, + crate_id, import_directive, def_maps, + usage_tracker, path_references, importing_crate, ), @@ -204,6 +223,8 @@ fn resolve_path_to_ns( import_path, parent_module_id, def_maps, + false, + usage_tracker, path_references, ) } else { @@ -221,24 +242,31 @@ fn resolve_path_from_crate_root( import_path: &[PathSegment], def_maps: &BTreeMap, + usage_tracker: &mut UsageTracker, path_references: &mut Option<&mut Vec>, ) -> NamespaceResolutionResult { + let starting_mod = def_maps[&crate_id].root; resolve_name_in_module( crate_id, importing_crate, import_path, - def_maps[&crate_id].root, + starting_mod, def_maps, + false, + usage_tracker, path_references, ) } +#[allow(clippy::too_many_arguments)] fn resolve_name_in_module( krate: CrateId, importing_crate: CrateId, import_path: &[PathSegment], starting_mod: LocalModuleId, def_maps: &BTreeMap, + plain: bool, + usage_tracker: &mut UsageTracker, path_references: &mut Option<&mut Vec>, ) -> NamespaceResolutionResult { let def_map = &def_maps[&krate]; @@ -261,8 +289,12 @@ fn resolve_name_in_module( return Err(PathResolutionError::Unresolved(first_segment.clone())); } + usage_tracker.mark_as_used(current_mod_id, first_segment); + let mut warning: Option = None; - for (last_segment, current_segment) in import_path.iter().zip(import_path.iter().skip(1)) { + for (index, (last_segment, current_segment)) in + import_path.iter().zip(import_path.iter().skip(1)).enumerate() + { let last_segment = &last_segment.ident; let current_segment = ¤t_segment.ident; @@ -298,13 +330,17 @@ fn resolve_name_in_module( }; warning = warning.or_else(|| { - if can_reference_module_id( - def_maps, - importing_crate, - starting_mod, - current_mod_id, - visibility, - ) { + // If the path is plain, the first segment will always refer to + // something that's visible from the current module. + if (plain && index == 0) + || can_reference_module_id( + def_maps, + importing_crate, + starting_mod, + current_mod_id, + visibility, + ) + { None } else { Some(PathResolutionError::Private(last_segment.clone())) @@ -320,6 +356,8 @@ fn resolve_name_in_module( return Err(PathResolutionError::Unresolved(current_segment.clone())); } + usage_tracker.mark_as_used(current_mod_id, current_segment); + current_ns = found_ns; } @@ -334,15 +372,18 @@ fn resolve_path_name(import_directive: &ImportDirective) -> Ident { } fn resolve_external_dep( - current_def_map: &CrateDefMap, + crate_id: CrateId, directive: &ImportDirective, def_maps: &BTreeMap, + usage_tracker: &mut UsageTracker, path_references: &mut Option<&mut Vec>, importing_crate: CrateId, ) -> NamespaceResolutionResult { // Use extern_prelude to get the dep let path = &directive.path.segments; + let current_def_map = &def_maps[&crate_id]; + // Fetch the root module from the prelude let crate_name = &path.first().unwrap().ident; let dep_module = current_def_map @@ -365,13 +406,21 @@ fn resolve_external_dep( span: Span::default(), }; let dep_directive = ImportDirective { + visibility: ItemVisibility::Private, module_id: dep_module.local_id, path, alias: directive.alias.clone(), is_prelude: false, }; - resolve_path_to_ns(&dep_directive, dep_module.krate, importing_crate, def_maps, path_references) + resolve_path_to_ns( + &dep_directive, + dep_module.krate, + importing_crate, + def_maps, + usage_tracker, + path_references, + ) } // Returns false if the given private function is being called from a non-child module, or diff --git a/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs b/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs index 712951ad6cb..50089d849ae 100644 --- a/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs +++ b/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs @@ -1,6 +1,7 @@ use super::import::{resolve_import, ImportDirective, PathResolution, PathResolutionResult}; -use crate::ast::Path; +use crate::ast::{ItemVisibility, Path}; use crate::node_interner::ReferenceId; +use crate::usage_tracker::UsageTracker; use std::collections::BTreeMap; use crate::graph::CrateId; @@ -15,6 +16,7 @@ pub trait PathResolver { &self, def_maps: &BTreeMap, path: Path, + usage_tracker: &mut UsageTracker, path_references: &mut Option<&mut Vec>, ) -> PathResolutionResult; @@ -39,9 +41,10 @@ impl PathResolver for StandardPathResolver { &self, def_maps: &BTreeMap, path: Path, + usage_tracker: &mut UsageTracker, path_references: &mut Option<&mut Vec>, ) -> PathResolutionResult { - resolve_path(def_maps, self.module_id, path, path_references) + resolve_path(def_maps, self.module_id, path, usage_tracker, path_references) } fn local_module_id(&self) -> LocalModuleId { @@ -59,12 +62,19 @@ pub fn resolve_path( def_maps: &BTreeMap, module_id: ModuleId, path: Path, + usage_tracker: &mut UsageTracker, path_references: &mut Option<&mut Vec>, ) -> PathResolutionResult { // lets package up the path into an ImportDirective and resolve it using that - let import = - ImportDirective { module_id: module_id.local_id, path, alias: None, is_prelude: false }; - let resolved_import = resolve_import(module_id.krate, &import, def_maps, path_references)?; + let import = ImportDirective { + visibility: ItemVisibility::Private, + module_id: module_id.local_id, + path, + alias: None, + is_prelude: false, + }; + let resolved_import = + resolve_import(module_id.krate, &import, def_maps, usage_tracker, path_references)?; let namespace = resolved_import.resolved_namespace; let id = diff --git a/compiler/noirc_frontend/src/lib.rs b/compiler/noirc_frontend/src/lib.rs index b14f65a3e35..ec09f680bc2 100644 --- a/compiler/noirc_frontend/src/lib.rs +++ b/compiler/noirc_frontend/src/lib.rs @@ -20,6 +20,7 @@ pub mod monomorphization; pub mod node_interner; pub mod parser; pub mod resolve_locations; +pub mod usage_tracker; pub mod hir; pub mod hir_def; diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs index 0ac13a58ecf..58de235455c 100644 --- a/compiler/noirc_frontend/src/locations.rs +++ b/compiler/noirc_frontend/src/locations.rs @@ -278,7 +278,8 @@ impl NodeInterner { } pub(crate) fn register_module(&mut self, id: ModuleId, name: String) { - self.register_name_for_auto_import(name, ModuleDefId::ModuleId(id), ItemVisibility::Public); + let visibility = ItemVisibility::Public; + self.register_name_for_auto_import(name, ModuleDefId::ModuleId(id), visibility, None); } pub(crate) fn register_global( @@ -290,7 +291,7 @@ impl NodeInterner { self.add_definition_location(ReferenceId::Global(id), Some(parent_module_id)); let visibility = ItemVisibility::Public; - self.register_name_for_auto_import(name, ModuleDefId::GlobalId(id), visibility); + self.register_name_for_auto_import(name, ModuleDefId::GlobalId(id), visibility, None); } pub(crate) fn register_struct( @@ -302,13 +303,14 @@ impl NodeInterner { self.add_definition_location(ReferenceId::Struct(id), Some(parent_module_id)); let visibility = ItemVisibility::Public; - self.register_name_for_auto_import(name, ModuleDefId::TypeId(id), visibility); + self.register_name_for_auto_import(name, ModuleDefId::TypeId(id), visibility, None); } pub(crate) fn register_trait(&mut self, id: TraitId, name: String, parent_module_id: ModuleId) { self.add_definition_location(ReferenceId::Trait(id), Some(parent_module_id)); - self.register_name_for_auto_import(name, ModuleDefId::TraitId(id), ItemVisibility::Public); + let visibility = ItemVisibility::Public; + self.register_name_for_auto_import(name, ModuleDefId::TraitId(id), visibility, None); } pub(crate) fn register_type_alias( @@ -320,31 +322,34 @@ impl NodeInterner { self.add_definition_location(ReferenceId::Alias(id), Some(parent_module_id)); let visibility = ItemVisibility::Public; - self.register_name_for_auto_import(name, ModuleDefId::TypeAliasId(id), visibility); + self.register_name_for_auto_import(name, ModuleDefId::TypeAliasId(id), visibility, None); } pub(crate) fn register_function(&mut self, id: FuncId, func_def: &FunctionDefinition) { - self.register_name_for_auto_import( - func_def.name.0.contents.clone(), - ModuleDefId::FunctionId(id), - func_def.visibility, - ); + let name = func_def.name.0.contents.clone(); + let id = ModuleDefId::FunctionId(id); + self.register_name_for_auto_import(name, id, func_def.visibility, None); } - fn register_name_for_auto_import( + pub fn register_name_for_auto_import( &mut self, name: String, module_def_id: ModuleDefId, visibility: ItemVisibility, + defining_module: Option, ) { if !self.lsp_mode { return; } - self.auto_import_names.entry(name).or_default().push((module_def_id, visibility)); + let entry = self.auto_import_names.entry(name).or_default(); + entry.push((module_def_id, visibility, defining_module)); } - pub fn get_auto_import_names(&self) -> &HashMap> { + #[allow(clippy::type_complexity)] + pub fn get_auto_import_names( + &self, + ) -> &HashMap)>> { &self.auto_import_names } } diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 32f25790e12..4a73df6a15f 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -27,6 +27,7 @@ use crate::hir::type_check::generics::TraitGenerics; use crate::hir_def::traits::NamedType; use crate::macros_api::ModuleDefId; use crate::macros_api::UnaryOp; +use crate::usage_tracker::UsageTracker; use crate::QuotedType; use crate::ast::{BinaryOpKind, FunctionDefinition, ItemVisibility}; @@ -253,9 +254,11 @@ pub struct NodeInterner { pub(crate) reference_modules: HashMap, // All names (and their definitions) that can be offered for auto_import. + // The third value in the tuple is the module where the definition is (only for pub use). // These include top-level functions, global variables and types, but excludes // impl and trait-impl methods. - pub(crate) auto_import_names: HashMap>, + pub(crate) auto_import_names: + HashMap)>>, /// Each value currently in scope in the comptime interpreter. /// Each element of the Vec represents a scope with every scope together making @@ -264,6 +267,8 @@ pub struct NodeInterner { /// This is stored in the NodeInterner so that the Elaborator from each crate can /// share the same global values. pub(crate) comptime_scopes: Vec>, + + pub(crate) usage_tracker: UsageTracker, } /// A dependency in the dependency graph may be a type or a definition. @@ -650,6 +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(), } } } diff --git a/compiler/noirc_frontend/src/noir_parser.lalrpop b/compiler/noirc_frontend/src/noir_parser.lalrpop index 1488a53183e..01b8be8f721 100644 --- a/compiler/noirc_frontend/src/noir_parser.lalrpop +++ b/compiler/noirc_frontend/src/noir_parser.lalrpop @@ -103,7 +103,7 @@ extern { pub(crate) TopLevelStatement: TopLevelStatement = { "use" r"[\t\r\n ]+" ";" EOF => { - TopLevelStatement::Import(use_tree) + TopLevelStatement::Import(use_tree, crate::ast::ItemVisibility::Private) } } diff --git a/compiler/noirc_frontend/src/parser/mod.rs b/compiler/noirc_frontend/src/parser/mod.rs index 11944cd3304..c82906b69a2 100644 --- a/compiler/noirc_frontend/src/parser/mod.rs +++ b/compiler/noirc_frontend/src/parser/mod.rs @@ -12,8 +12,9 @@ mod labels; mod parser; use crate::ast::{ - Expression, Ident, ImportStatement, LetStatement, ModuleDeclaration, NoirFunction, NoirStruct, - NoirTrait, NoirTraitImpl, NoirTypeAlias, Recoverable, StatementKind, TypeImpl, UseTree, + Expression, Ident, ImportStatement, ItemVisibility, LetStatement, ModuleDeclaration, + NoirFunction, NoirStruct, NoirTrait, NoirTraitImpl, NoirTypeAlias, Recoverable, StatementKind, + TypeImpl, UseTree, }; use crate::token::{Keyword, Token}; @@ -32,7 +33,7 @@ pub use parser::{ pub enum TopLevelStatement { Function(NoirFunction), Module(ModuleDeclaration), - Import(UseTree), + Import(UseTree, ItemVisibility), Struct(NoirStruct), Trait(NoirTrait), TraitImpl(NoirTraitImpl), @@ -48,7 +49,7 @@ impl TopLevelStatement { match self { TopLevelStatement::Function(f) => Some(ItemKind::Function(f)), TopLevelStatement::Module(m) => Some(ItemKind::ModuleDecl(m)), - TopLevelStatement::Import(i) => Some(ItemKind::Import(i)), + TopLevelStatement::Import(i, visibility) => Some(ItemKind::Import(i, visibility)), TopLevelStatement::Struct(s) => Some(ItemKind::Struct(s)), TopLevelStatement::Trait(t) => Some(ItemKind::Trait(t)), TopLevelStatement::TraitImpl(t) => Some(ItemKind::TraitImpl(t)), @@ -298,7 +299,7 @@ impl ParsedModule { for item in self.items { match item.kind { - ItemKind::Import(import) => module.push_import(import), + ItemKind::Import(import, visibility) => module.push_import(import, visibility), ItemKind::Function(func) => module.push_function(func), ItemKind::Struct(typ) => module.push_type(typ), ItemKind::Trait(noir_trait) => module.push_trait(noir_trait), @@ -323,7 +324,7 @@ pub struct Item { #[derive(Clone, Debug)] pub enum ItemKind { - Import(UseTree), + Import(UseTree, ItemVisibility), Function(NoirFunction), Struct(NoirStruct), Trait(NoirTrait), @@ -398,8 +399,8 @@ impl SortedModule { self.type_aliases.push(type_alias); } - fn push_import(&mut self, import_stmt: UseTree) { - self.imports.extend(import_stmt.desugar(None)); + fn push_import(&mut self, import_stmt: UseTree, visibility: ItemVisibility) { + self.imports.extend(import_stmt.desugar(None, visibility)); } fn push_module_decl(&mut self, mod_decl: ModuleDeclaration) { @@ -497,7 +498,13 @@ impl std::fmt::Display for TopLevelStatement { match self { TopLevelStatement::Function(fun) => fun.fmt(f), TopLevelStatement::Module(m) => m.fmt(f), - TopLevelStatement::Import(tree) => write!(f, "use {tree}"), + TopLevelStatement::Import(tree, visibility) => { + if visibility == &ItemVisibility::Private { + write!(f, "use {tree}") + } else { + write!(f, "{visibility} use {tree}") + } + } TopLevelStatement::Trait(t) => t.fmt(f), TopLevelStatement::TraitImpl(i) => i.fmt(f), TopLevelStatement::Struct(s) => s.fmt(f), diff --git a/compiler/noirc_frontend/src/parser/parser.rs b/compiler/noirc_frontend/src/parser/parser.rs index 5f0ef8909e7..bead1e69006 100644 --- a/compiler/noirc_frontend/src/parser/parser.rs +++ b/compiler/noirc_frontend/src/parser/parser.rs @@ -27,6 +27,7 @@ use self::path::as_trait_path; use self::primitives::{keyword, macro_quote_marker, mutable_reference, variable}; use self::types::{generic_type_args, maybe_comp_time}; pub use types::parse_type; +use visibility::visibility_modifier; use super::{ foldl_with_span, labels::ParsingRuleLabel, parameter_name_recovery, parameter_recovery, @@ -64,6 +65,7 @@ mod primitives; mod structs; pub(super) mod traits; mod types; +mod visibility; // synthesized by LALRPOP lalrpop_mod!(pub noir_parser); @@ -95,7 +97,7 @@ pub fn parse_program(source_program: &str) -> (ParsedModule, Vec) { for parsed_item in &parsed_module.items { if lalrpop_parser_supports_kind(&parsed_item.kind) { match &parsed_item.kind { - ItemKind::Import(parsed_use_tree) => { + ItemKind::Import(parsed_use_tree, _visibility) => { prototype_parse_use_tree(Some(parsed_use_tree), source_program); } // other kinds prevented by lalrpop_parser_supports_kind @@ -139,7 +141,7 @@ fn prototype_parse_use_tree(expected_use_tree_opt: Option<&UseTree>, input: &str ); match calculated.unwrap() { - TopLevelStatement::Import(parsed_use_tree) => { + TopLevelStatement::Import(parsed_use_tree, _visibility) => { assert_eq!(expected_use_tree, &parsed_use_tree); } unexpected_calculated => { @@ -161,7 +163,7 @@ fn prototype_parse_use_tree(expected_use_tree_opt: Option<&UseTree>, input: &str } fn lalrpop_parser_supports_kind(kind: &ItemKind) -> bool { - matches!(kind, ItemKind::Import(_)) + matches!(kind, ItemKind::Import(..)) } /// program: module EOF @@ -438,7 +440,10 @@ fn module_declaration() -> impl NoirParser { } fn use_statement() -> impl NoirParser { - keyword(Keyword::Use).ignore_then(use_tree()).map(TopLevelStatement::Import) + visibility_modifier() + .then_ignore(keyword(Keyword::Use)) + .then(use_tree()) + .map(|(visibility, use_tree)| TopLevelStatement::Import(use_tree, visibility)) } fn rename() -> impl NoirParser> { @@ -1556,7 +1561,7 @@ mod test { parse_recover(&use_statement(), &use_statement_str); use_statement_str.push(';'); match result_opt.unwrap() { - TopLevelStatement::Import(expected_use_statement) => { + TopLevelStatement::Import(expected_use_statement, _visibility) => { Some(expected_use_statement) } _ => unreachable!(), diff --git a/compiler/noirc_frontend/src/parser/parser/function.rs b/compiler/noirc_frontend/src/parser/parser/function.rs index 56760898374..9328c882e54 100644 --- a/compiler/noirc_frontend/src/parser/parser/function.rs +++ b/compiler/noirc_frontend/src/parser/parser/function.rs @@ -3,7 +3,9 @@ use super::{ block, fresh_statement, ident, keyword, maybe_comp_time, nothing, optional_visibility, parameter_name_recovery, parameter_recovery, parenthesized, parse_type, pattern, primitives::token_kind, - self_parameter, where_clause, NoirParser, + self_parameter, + visibility::visibility_modifier, + where_clause, NoirParser, }; use crate::token::{Keyword, Token, TokenKind}; use crate::{ @@ -73,21 +75,6 @@ pub(super) fn function_definition(allow_self: bool) -> impl NoirParser impl NoirParser { - let is_pub_crate = (keyword(Keyword::Pub) - .then_ignore(just(Token::LeftParen)) - .then_ignore(keyword(Keyword::Crate)) - .then_ignore(just(Token::RightParen))) - .map(|_| ItemVisibility::PublicCrate); - - let is_pub = keyword(Keyword::Pub).map(|_| ItemVisibility::Public); - - let is_private = empty().map(|_| ItemVisibility::Private); - - choice((is_pub_crate, is_pub, is_private)) -} - /// function_modifiers: 'unconstrained'? (visibility)? /// /// returns (is_unconstrained, visibility) for whether each keyword was present diff --git a/compiler/noirc_frontend/src/parser/parser/visibility.rs b/compiler/noirc_frontend/src/parser/parser/visibility.rs new file mode 100644 index 00000000000..d9c1abf2123 --- /dev/null +++ b/compiler/noirc_frontend/src/parser/parser/visibility.rs @@ -0,0 +1,27 @@ +use chumsky::{ + prelude::{choice, empty, just}, + Parser, +}; + +use crate::{ + ast::ItemVisibility, + parser::NoirParser, + token::{Keyword, Token}, +}; + +use super::primitives::keyword; + +/// visibility_modifier: 'pub(crate)'? 'pub'? '' +pub(crate) fn visibility_modifier() -> impl NoirParser { + let is_pub_crate = (keyword(Keyword::Pub) + .then_ignore(just(Token::LeftParen)) + .then_ignore(keyword(Keyword::Crate)) + .then_ignore(just(Token::RightParen))) + .map(|_| ItemVisibility::PublicCrate); + + let is_pub = keyword(Keyword::Pub).map(|_| ItemVisibility::Public); + + let is_private = empty().map(|_| ItemVisibility::Private); + + choice((is_pub_crate, is_pub, is_private)) +} diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 870c781b89d..a30907211a3 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -3199,7 +3199,7 @@ fn as_trait_path_syntax_no_impl() { } #[test] -fn errors_on_unused_import() { +fn errors_on_unused_private_import() { let src = r#" mod foo { pub fn bar() {} @@ -3231,3 +3231,113 @@ fn errors_on_unused_import() { assert_eq!(ident.to_string(), "bar"); } + +#[test] +fn errors_on_unused_pub_crate_import() { + let src = r#" + mod foo { + pub fn bar() {} + pub fn baz() {} + + trait Foo { + } + } + + pub(crate) 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"); +} + +#[test] +fn warns_on_use_of_private_exported_item() { + let src = r#" + mod foo { + mod bar { + pub fn baz() {} + } + + use bar::baz; + + fn qux() { + baz(); + } + } + + fn main() { + foo::baz(); + } + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 2); // An existing bug causes this error to be duplicated + + assert!(matches!( + &errors[0].0, + CompilationError::ResolverError(ResolverError::PathResolutionError( + PathResolutionError::Private(..), + )) + )); +} + +#[test] +fn can_use_pub_use_item() { + let src = r#" + mod foo { + mod bar { + pub fn baz() {} + } + + pub use bar::baz; + } + + fn main() { + foo::baz(); + } + "#; + assert_no_errors(src); +} + +#[test] +fn warns_on_re_export_of_item_with_less_visibility() { + let src = r#" + mod foo { + mod bar { + pub(crate) fn baz() {} + } + + pub use bar::baz; + } + + fn main() { + foo::baz(); + } + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + assert!(matches!( + &errors[0].0, + CompilationError::DefinitionError( + DefCollectorErrorKind::CannotReexportItemWithLessVisibility { .. } + ) + )); +} diff --git a/compiler/noirc_frontend/src/usage_tracker.rs b/compiler/noirc_frontend/src/usage_tracker.rs new file mode 100644 index 00000000000..d8b7b271734 --- /dev/null +++ b/compiler/noirc_frontend/src/usage_tracker.rs @@ -0,0 +1,26 @@ +use std::collections::HashSet; + +use rustc_hash::FxHashMap as HashMap; + +use crate::{ast::Ident, hir::def_map::ModuleId}; + +#[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>, +} + +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 mark_as_used(&mut self, current_mod_id: ModuleId, name: &Ident) { + self.unused_imports.entry(current_mod_id).or_default().remove(name); + } + + pub(crate) fn unused_imports(&self) -> &HashMap> { + &self.unused_imports + } +} diff --git a/docs/docs/noir/modules_packages_crates/modules.md b/docs/docs/noir/modules_packages_crates/modules.md index 16b6307d2fd..d21b009be3b 100644 --- a/docs/docs/noir/modules_packages_crates/modules.md +++ b/docs/docs/noir/modules_packages_crates/modules.md @@ -182,4 +182,30 @@ fn from_bar() { from_foo(); // invokes super::from_foo(), which is bar::from_foo() super::from_foo(); // also invokes bar::from_foo() } -``` \ No newline at end of file +``` + +### `use` visibility + +`use` declarations are private to the containing module, by default. However, like functions, +they can be marked as `pub` or `pub(crate)`. Such a use declaration serves to _re-export_ a name. +A public `use` declaration can therefore redirect some public name to a different target definition: +even a definition with a private canonical path, inside a different module. + +An example of re-exporting: + +```rust +mod some_module { + pub use foo::{bar, baz}; + mod foo { + pub fn bar() {} + pub fn baz() {} + } +} + +fn main() { + some_module::bar(); + some_module::baz(); +} +``` + +In this example, the module `some_module` re-exports two public names defined in `foo`. \ No newline at end of file diff --git a/noir_stdlib/src/array.nr b/noir_stdlib/src/array.nr index 23683a54e45..68e134b56fa 100644 --- a/noir_stdlib/src/array.nr +++ b/noir_stdlib/src/array.nr @@ -1,5 +1,4 @@ use crate::cmp::Ord; -use crate::option::Option; use crate::convert::From; impl [T; N] { diff --git a/noir_stdlib/src/collections/map.nr b/noir_stdlib/src/collections/map.nr index 4607b06d667..27a7d0d3550 100644 --- a/noir_stdlib/src/collections/map.nr +++ b/noir_stdlib/src/collections/map.nr @@ -1,5 +1,4 @@ use crate::cmp::Eq; -use crate::collections::vec::Vec; use crate::option::Option; use crate::default::Default; use crate::hash::{Hash, Hasher, BuildHasher}; diff --git a/noir_stdlib/src/collections/umap.nr b/noir_stdlib/src/collections/umap.nr index c552c053a92..c71905e63b3 100644 --- a/noir_stdlib/src/collections/umap.nr +++ b/noir_stdlib/src/collections/umap.nr @@ -1,10 +1,7 @@ use crate::cmp::Eq; -use crate::collections::vec::Vec; use crate::option::Option; use crate::default::Default; -use crate::hash::{Hash, Hasher, BuildHasher, BuildHasherDefault}; -use crate::hash::poseidon2::Poseidon2; -use crate::collections::bounded_vec::BoundedVec; +use crate::hash::{Hash, Hasher, BuildHasher}; // An unconstrained hash table with open addressing and quadratic probing. // Note that "unconstrained" here means that almost all operations on this diff --git a/noir_stdlib/src/ec/consts/te.nr b/noir_stdlib/src/ec/consts/te.nr index e25f373593a..8cea7654e39 100644 --- a/noir_stdlib/src/ec/consts/te.nr +++ b/noir_stdlib/src/ec/consts/te.nr @@ -1,4 +1,3 @@ -use crate::compat; use crate::ec::tecurve::affine::Point as TEPoint; use crate::ec::tecurve::affine::Curve as TECurve; diff --git a/noir_stdlib/src/eddsa.nr b/noir_stdlib/src/eddsa.nr index 337969be90e..cfdbbe9c3d0 100644 --- a/noir_stdlib/src/eddsa.nr +++ b/noir_stdlib/src/eddsa.nr @@ -1,7 +1,6 @@ -use crate::hash::poseidon; use crate::ec::consts::te::baby_jubjub; use crate::ec::tecurve::affine::Point as TEPoint; -use crate::hash::{Hash, Hasher, BuildHasher, BuildHasherDefault}; +use crate::hash::Hasher; use crate::hash::poseidon::PoseidonHasher; use crate::default::Default; diff --git a/noir_stdlib/src/field/bn254.nr b/noir_stdlib/src/field/bn254.nr index 19a49402642..0aa5ca0717b 100644 --- a/noir_stdlib/src/field/bn254.nr +++ b/noir_stdlib/src/field/bn254.nr @@ -142,7 +142,7 @@ pub fn lt(a: Field, b: Field) -> bool { mod tests { // TODO: Allow imports from "super" - use crate::field::bn254::{decompose_hint, decompose, compute_lt, assert_gt, gt, lt, TWO_POW_128, compute_lte, PLO, PHI}; + use crate::field::bn254::{decompose, compute_lt, assert_gt, gt, TWO_POW_128, compute_lte, PLO, PHI}; #[test] fn check_decompose() { diff --git a/noir_stdlib/src/hash/mod.nr b/noir_stdlib/src/hash/mod.nr index 33be56cdc3d..0e15595ff40 100644 --- a/noir_stdlib/src/hash/mod.nr +++ b/noir_stdlib/src/hash/mod.nr @@ -12,7 +12,7 @@ use crate::embedded_curve_ops::{EmbeddedCurvePoint, EmbeddedCurveScalar, multi_s use crate::meta::derive_via; // Kept for backwards compatibility -use sha256::{digest, sha256, sha256_compression, sha256_var}; +pub use sha256::{digest, sha256, sha256_compression, sha256_var}; #[foreign(blake2s)] // docs:start:blake2s diff --git a/noir_stdlib/src/hash/poseidon/bn254.nr b/noir_stdlib/src/hash/poseidon/bn254.nr index 103ab3d166a..848d561f755 100644 --- a/noir_stdlib/src/hash/poseidon/bn254.nr +++ b/noir_stdlib/src/hash/poseidon/bn254.nr @@ -2,7 +2,7 @@ mod perm; mod consts; -use crate::hash::poseidon::{PoseidonConfig, absorb}; +use crate::hash::poseidon::absorb; // Variable-length Poseidon-128 sponge as suggested in second bullet point of ยง3 of https://eprint.iacr.org/2019/458.pdf #[field(bn254)] diff --git a/noir_stdlib/src/hash/poseidon/bn254/perm.nr b/noir_stdlib/src/hash/poseidon/bn254/perm.nr index b7c022c5a00..b7dc05ef9de 100644 --- a/noir_stdlib/src/hash/poseidon/bn254/perm.nr +++ b/noir_stdlib/src/hash/poseidon/bn254/perm.nr @@ -1,7 +1,6 @@ // Instantiations of Poseidon permutation for the prime field of the same order as BN254 use crate::hash::poseidon::bn254::consts; use crate::hash::poseidon::permute; -use crate::hash::poseidon::PoseidonConfig; #[field(bn254)] pub fn x5_2(mut state: [Field; 2]) -> [Field; 2] { diff --git a/noir_stdlib/src/ops/mod.nr b/noir_stdlib/src/ops/mod.nr index 8b1903cff0b..6cf20432468 100644 --- a/noir_stdlib/src/ops/mod.nr +++ b/noir_stdlib/src/ops/mod.nr @@ -1,5 +1,5 @@ mod arith; mod bit; -use arith::{Add, Sub, Mul, Div, Rem, Neg}; -use bit::{Not, BitOr, BitAnd, BitXor, Shl, Shr}; +pub use arith::{Add, Sub, Mul, Div, Rem, Neg}; +pub use bit::{Not, BitOr, BitAnd, BitXor, Shl, Shr}; diff --git a/noir_stdlib/src/prelude.nr b/noir_stdlib/src/prelude.nr index 0d423e3556d..b14f38bdf55 100644 --- a/noir_stdlib/src/prelude.nr +++ b/noir_stdlib/src/prelude.nr @@ -1,9 +1,9 @@ -use crate::collections::vec::Vec; -use crate::collections::bounded_vec::BoundedVec; -use crate::option::Option; -use crate::{print, println, assert_constant}; -use crate::uint128::U128; -use crate::cmp::{Eq, Ord}; -use crate::default::Default; -use crate::convert::{From, Into}; -use crate::meta::{derive, derive_via}; +pub use crate::collections::vec::Vec; +pub use crate::collections::bounded_vec::BoundedVec; +pub use crate::option::Option; +pub use crate::{print, println, assert_constant}; +pub use crate::uint128::U128; +pub use crate::cmp::{Eq, Ord}; +pub use crate::default::Default; +pub use crate::convert::{From, Into}; +pub use crate::meta::{derive, derive_via}; diff --git a/noir_stdlib/src/sha256.nr b/noir_stdlib/src/sha256.nr index c3e18b13e91..ce217f7a689 100644 --- a/noir_stdlib/src/sha256.nr +++ b/noir_stdlib/src/sha256.nr @@ -1,2 +1,2 @@ // This file is kept for backwards compatibility. -use crate::hash::sha256::{digest, sha256_var}; +pub use crate::hash::sha256::{digest, sha256_var}; diff --git a/noir_stdlib/src/sha512.nr b/noir_stdlib/src/sha512.nr index 1ddbf6e0ae1..b474e27b416 100644 --- a/noir_stdlib/src/sha512.nr +++ b/noir_stdlib/src/sha512.nr @@ -1,2 +1,2 @@ // This file is kept for backwards compatibility. -use crate::hash::sha512::digest; +pub use crate::hash::sha512::digest; diff --git a/noir_stdlib/src/uint128.nr b/noir_stdlib/src/uint128.nr index 6b2e78f33d6..ac7f744cb3b 100644 --- a/noir_stdlib/src/uint128.nr +++ b/noir_stdlib/src/uint128.nr @@ -1,6 +1,5 @@ use crate::ops::{Add, Sub, Mul, Div, Rem, Not, BitOr, BitAnd, BitXor, Shl, Shr}; use crate::cmp::{Eq, Ord, Ordering}; -use crate::println; global pow64 : Field = 18446744073709551616; //2^64; global pow63 : Field = 9223372036854775808; // 2^63; diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index f44f8e2e4d5..d1ffdb55066 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -3,7 +3,7 @@ use std::ops::ControlFlow; use crate::insert_all_files_for_workspace_into_file_manager; use async_lsp::{ErrorCode, LanguageClient, ResponseError}; use lsp_types::DiagnosticTag; -use noirc_driver::{check_crate, file_manager_with_stdlib, CheckOptions}; +use noirc_driver::{check_crate, file_manager_with_stdlib}; use noirc_errors::{DiagnosticKind, FileDiagnostic}; use crate::types::{ @@ -133,11 +133,7 @@ pub(crate) fn process_workspace_for_noir_document( let (mut context, crate_id) = crate::prepare_package(&workspace_file_manager, &parsed_files, package); - 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) { + let file_diagnostics = match check_crate(&mut context, crate_id, &Default::default()) { Ok(((), warnings)) => warnings, Err(errors_and_warnings) => errors_and_warnings, }; diff --git a/tooling/lsp/src/requests/code_action.rs b/tooling/lsp/src/requests/code_action.rs index fd8c42a3b87..8e153bb0b46 100644 --- a/tooling/lsp/src/requests/code_action.rs +++ b/tooling/lsp/src/requests/code_action.rs @@ -21,7 +21,7 @@ use noirc_frontend::{ use crate::{ byte_span_to_range, - modules::{get_parent_module_id, module_full_path}, + modules::{get_parent_module_id, module_full_path, module_id_path}, utils, LspState, }; @@ -267,21 +267,33 @@ impl<'a> Visitor for CodeActionFinder<'a> { continue; } - for (module_def_id, visibility) in entries { - let Some(module_full_path) = module_full_path( - *module_def_id, - *visibility, - self.module_id, - current_module_parent_id, - self.interner, - ) else { - continue; + for (module_def_id, visibility, defining_module) in entries { + let module_full_path = if let Some(defining_module) = defining_module { + module_id_path( + *defining_module, + &self.module_id, + current_module_parent_id, + self.interner, + ) + } else { + let Some(module_full_path) = module_full_path( + *module_def_id, + *visibility, + self.module_id, + current_module_parent_id, + self.interner, + ) else { + continue; + }; + module_full_path }; - let full_path = if let ModuleDefId::ModuleId(..) = module_def_id { - module_full_path.clone() - } else { + let full_path = if defining_module.is_some() + || !matches!(module_def_id, ModuleDefId::ModuleId(..)) + { format!("{}::{}", module_full_path, name) + } else { + module_full_path.clone() }; let qualify_prefix = if let ModuleDefId::ModuleId(..) = module_def_id { diff --git a/tooling/lsp/src/requests/code_action/tests.rs b/tooling/lsp/src/requests/code_action/tests.rs index 21b5fab96c6..a5a19049141 100644 --- a/tooling/lsp/src/requests/code_action/tests.rs +++ b/tooling/lsp/src/requests/code_action/tests.rs @@ -199,3 +199,38 @@ fn main() { assert_code_action(title, src, expected).await; } + +#[test] +async fn test_qualify_code_action_for_pub_use_import() { + let title = "Qualify as bar::foobar"; + + let src = r#" + mod bar { + mod baz { + pub fn qux() {} + } + + pub use baz::qux as foobar; + } + + fn main() { + foob>| { lines: Vec<&'a str>, byte_index: usize, byte: Option, - /// The module ID of the current file. - root_module_id: ModuleId, /// The module ID in scope. This might change as we traverse the AST /// if we are analyzing something inside an inline module declaration. module_id: ModuleId, @@ -131,7 +126,6 @@ impl<'a> NodeFinder<'a> { ) -> Self { // Find the module the current file belongs to let def_map = &def_maps[&krate]; - let root_module_id = ModuleId { krate, local_id: def_map.root() }; let local_id = if let Some((module_index, _)) = def_map.modules().iter().find(|(_, module_data)| module_data.location.file == file) { @@ -146,7 +140,6 @@ impl<'a> NodeFinder<'a> { lines: source.lines().collect(), byte_index, byte, - root_module_id, module_id, def_maps, dependencies, @@ -278,12 +271,6 @@ impl<'a> NodeFinder<'a> { let is_single_segment = !after_colons && idents.is_empty() && path.kind == PathKind::Plain; let module_id; - let module_completion_kind = if after_colons || !idents.is_empty() { - ModuleCompletionKind::DirectChildren - } else { - ModuleCompletionKind::AllVisibleItems - }; - // When completing in the middle of an ident, we don't want to complete // with function parameters because there might already be function parameters, // and in the middle of a path it leads to code that won't compile @@ -346,7 +333,6 @@ impl<'a> NodeFinder<'a> { &prefix, path.kind, at_root, - module_completion_kind, function_completion_kind, requested_items, ); @@ -447,7 +433,6 @@ impl<'a> NodeFinder<'a> { } } - let module_completion_kind = ModuleCompletionKind::DirectChildren; let function_completion_kind = FunctionCompletionKind::Name; let requested_items = RequestedItems::AnyItems; @@ -463,7 +448,6 @@ impl<'a> NodeFinder<'a> { prefix, path_kind, at_root, - module_completion_kind, function_completion_kind, requested_items, ); @@ -478,7 +462,6 @@ impl<'a> NodeFinder<'a> { &prefix, path_kind, at_root, - module_completion_kind, function_completion_kind, requested_items, ); @@ -489,7 +472,6 @@ impl<'a> NodeFinder<'a> { &prefix, path_kind, at_root, - module_completion_kind, function_completion_kind, requested_items, ); @@ -604,6 +586,7 @@ impl<'a> NodeFinder<'a> { for func_id in methods.iter() { if name_matches(name, prefix) { if let Some(completion_item) = self.function_completion_item( + name, func_id, function_completion_kind, function_kind, @@ -625,9 +608,12 @@ impl<'a> NodeFinder<'a> { ) { for (name, func_id) in &trait_.method_ids { if name_matches(name, prefix) { - if let Some(completion_item) = - self.function_completion_item(*func_id, function_completion_kind, function_kind) - { + if let Some(completion_item) = self.function_completion_item( + name, + *func_id, + function_completion_kind, + function_kind, + ) { self.completion_items.push(completion_item); self.suggested_module_def_ids.insert(ModuleDefId::FunctionId(*func_id)); } @@ -661,7 +647,6 @@ impl<'a> NodeFinder<'a> { prefix: &str, path_kind: PathKind, at_root: bool, - module_completion_kind: ModuleCompletionKind, function_completion_kind: FunctionCompletionKind, requested_items: RequestedItems, ) { @@ -694,12 +679,7 @@ impl<'a> NodeFinder<'a> { let function_kind = FunctionKind::Any; - let items = match module_completion_kind { - ModuleCompletionKind::DirectChildren => module_data.definitions(), - ModuleCompletionKind::AllVisibleItems => module_data.scope(), - }; - - for ident in items.names() { + for ident in module_data.scope().names() { let name = &ident.0.contents; if name_matches(name, prefix) { @@ -773,14 +753,6 @@ impl<'a> NodeFinder<'a> { fn resolve_path(&self, segments: Vec) -> Option { let last_segment = segments.last().unwrap().clone(); - let path_segments = segments.into_iter().map(PathSegment::from).collect(); - let path = Path { segments: path_segments, kind: PathKind::Plain, span: Span::default() }; - - let path_resolver = StandardPathResolver::new(self.root_module_id); - if let Ok(path_resolution) = path_resolver.resolve(self.def_maps, path, &mut None) { - return Some(path_resolution.module_def_id); - } - // If we can't resolve a path trough lookup, let's see if the last segment is bound to a type let location = Location::new(last_segment.span(), self.file); if let Some(reference_id) = self.interner.find_referenced(location) { @@ -808,7 +780,7 @@ impl<'a> Visitor for NodeFinder<'a> { self.includes_span(item.span) } - fn visit_import(&mut self, use_tree: &UseTree) -> bool { + fn visit_import(&mut self, use_tree: &UseTree, _visibility: ItemVisibility) -> bool { let mut prefixes = Vec::new(); self.find_in_use_tree(use_tree, &mut prefixes); false diff --git a/tooling/lsp/src/requests/completion/auto_import.rs b/tooling/lsp/src/requests/completion/auto_import.rs index bf3ff7f0291..bbd471dfea1 100644 --- a/tooling/lsp/src/requests/completion/auto_import.rs +++ b/tooling/lsp/src/requests/completion/auto_import.rs @@ -1,7 +1,7 @@ use lsp_types::{Position, Range, TextEdit}; use noirc_frontend::macros_api::ModuleDefId; -use crate::modules::{get_parent_module_id, module_full_path}; +use crate::modules::{get_parent_module_id, module_full_path, module_id_path}; use super::{ kinds::{FunctionCompletionKind, FunctionKind, RequestedItems}, @@ -24,7 +24,7 @@ impl<'a> NodeFinder<'a> { continue; } - for (module_def_id, visibility) in entries { + for (module_def_id, visibility, defining_module) in entries { if self.suggested_module_def_ids.contains(module_def_id) { continue; } @@ -39,20 +39,32 @@ impl<'a> NodeFinder<'a> { continue; }; - let Some(module_full_path) = module_full_path( - *module_def_id, - *visibility, - self.module_id, - current_module_parent_id, - self.interner, - ) else { - continue; + let module_full_path = if let Some(defining_module) = defining_module { + module_id_path( + *defining_module, + &self.module_id, + current_module_parent_id, + self.interner, + ) + } else { + let Some(module_full_path) = module_full_path( + *module_def_id, + *visibility, + self.module_id, + current_module_parent_id, + self.interner, + ) else { + continue; + }; + module_full_path }; - let full_path = if let ModuleDefId::ModuleId(..) = module_def_id { - module_full_path - } else { + let full_path = if defining_module.is_some() + || !matches!(module_def_id, ModuleDefId::ModuleId(..)) + { format!("{}::{}", module_full_path, name) + } else { + module_full_path }; let mut label_details = completion_item.label_details.unwrap(); diff --git a/tooling/lsp/src/requests/completion/completion_items.rs b/tooling/lsp/src/requests/completion/completion_items.rs index d4190f5241c..21c3a607b18 100644 --- a/tooling/lsp/src/requests/completion/completion_items.rs +++ b/tooling/lsp/src/requests/completion/completion_items.rs @@ -3,8 +3,8 @@ use lsp_types::{ }; use noirc_frontend::{ hir_def::{function::FuncMeta, stmt::HirPattern}, - macros_api::{ModuleDefId, StructId}, - node_interner::{FuncId, GlobalId, TraitId, TypeAliasId}, + macros_api::ModuleDefId, + node_interner::{FuncId, GlobalId}, Type, }; @@ -38,45 +38,32 @@ impl<'a> NodeFinder<'a> { match module_def_id { ModuleDefId::ModuleId(_) => Some(module_completion_item(name)), - ModuleDefId::FunctionId(func_id) => { - self.function_completion_item(func_id, function_completion_kind, function_kind) - } - ModuleDefId::TypeId(struct_id) => Some(self.struct_completion_item(struct_id)), - ModuleDefId::TypeAliasId(type_alias_id) => { - Some(self.type_alias_completion_item(type_alias_id)) - } - ModuleDefId::TraitId(trait_id) => Some(self.trait_completion_item(trait_id)), - ModuleDefId::GlobalId(global_id) => Some(self.global_completion_item(global_id)), + ModuleDefId::FunctionId(func_id) => self.function_completion_item( + &name, + func_id, + function_completion_kind, + function_kind, + ), + ModuleDefId::TypeId(..) => Some(self.struct_completion_item(name)), + ModuleDefId::TypeAliasId(..) => Some(self.type_alias_completion_item(name)), + ModuleDefId::TraitId(..) => Some(self.trait_completion_item(name)), + ModuleDefId::GlobalId(global_id) => Some(self.global_completion_item(name, global_id)), } } - fn struct_completion_item(&self, struct_id: StructId) -> CompletionItem { - let struct_type = self.interner.get_struct(struct_id); - let struct_type = struct_type.borrow(); - let name = struct_type.name.to_string(); - + fn struct_completion_item(&self, name: String) -> CompletionItem { simple_completion_item(name.clone(), CompletionItemKind::STRUCT, Some(name)) } - fn type_alias_completion_item(&self, type_alias_id: TypeAliasId) -> CompletionItem { - let type_alias = self.interner.get_type_alias(type_alias_id); - let type_alias = type_alias.borrow(); - let name = type_alias.name.to_string(); - + fn type_alias_completion_item(&self, name: String) -> CompletionItem { simple_completion_item(name.clone(), CompletionItemKind::STRUCT, Some(name)) } - fn trait_completion_item(&self, trait_id: TraitId) -> CompletionItem { - let trait_ = self.interner.get_trait(trait_id); - let name = trait_.name.to_string(); - + fn trait_completion_item(&self, name: String) -> CompletionItem { simple_completion_item(name.clone(), CompletionItemKind::INTERFACE, Some(name)) } - fn global_completion_item(&self, global_id: GlobalId) -> CompletionItem { - let global_definition = self.interner.get_global_definition(global_id); - let name = global_definition.name.clone(); - + fn global_completion_item(&self, name: String, global_id: GlobalId) -> CompletionItem { let global = self.interner.get_global(global_id); let typ = self.interner.definition_type(global.definition_id); let description = typ.to_string(); @@ -86,12 +73,12 @@ impl<'a> NodeFinder<'a> { pub(super) fn function_completion_item( &self, + name: &String, func_id: FuncId, function_completion_kind: FunctionCompletionKind, function_kind: FunctionKind, ) -> Option { let func_meta = self.interner.function_meta(&func_id); - let name = &self.interner.function_name(&func_id).to_string(); let func_self_type = if let Some((pattern, typ, _)) = func_meta.parameters.0.first() { if self.hir_pattern_is_self_type(pattern) { diff --git a/tooling/lsp/src/requests/completion/kinds.rs b/tooling/lsp/src/requests/completion/kinds.rs index e01fcfc8c56..2fe039ba331 100644 --- a/tooling/lsp/src/requests/completion/kinds.rs +++ b/tooling/lsp/src/requests/completion/kinds.rs @@ -1,17 +1,5 @@ use noirc_frontend::Type; -/// When finding items in a module, whether to show only direct children or all visible items. -#[derive(Clone, Copy, PartialEq, Eq, Debug)] -pub(super) enum ModuleCompletionKind { - // Only show a module's direct children. This is used when completing a use statement - // or a path after the first segment. - DirectChildren, - // Show all of a module's visible items. This is used when completing a path outside - // of a use statement (in regular code) when the path is just a single segment: - // we want to find items exposed in the current module. - AllVisibleItems, -} - /// When suggest a function as a result of completion, whether to autocomplete its name or its name and parameters. #[derive(Clone, Copy, PartialEq, Eq, Debug)] pub(super) enum FunctionCompletionKind { diff --git a/tooling/lsp/src/requests/completion/tests.rs b/tooling/lsp/src/requests/completion/tests.rs index 2d667ead6bf..d621ca21bb8 100644 --- a/tooling/lsp/src/requests/completion/tests.rs +++ b/tooling/lsp/src/requests/completion/tests.rs @@ -119,14 +119,14 @@ mod completion_tests { #[test] async fn test_use_first_segment() { let src = r#" - mod foo {} + mod foobaz {} mod foobar {} - use f>|< + use foob>|< "#; assert_completion( src, - vec![module_completion_item("foo"), module_completion_item("foobar")], + vec![module_completion_item("foobaz"), module_completion_item("foobar")], ) .await; } @@ -218,7 +218,7 @@ mod completion_tests { #[test] async fn test_use_suggests_hardcoded_crate() { let src = r#" - use c>|< + use cr>|< "#; assert_completion( @@ -291,16 +291,16 @@ mod completion_tests { #[test] async fn test_use_after_super() { let src = r#" - mod foo {} + mod foobar {} mod bar { mod something {} - use super::f>|< + use super::foob>|< } "#; - assert_completion(src, vec![module_completion_item("foo")]).await; + assert_completion(src, vec![module_completion_item("foobar")]).await; } #[test] @@ -1791,4 +1791,76 @@ mod completion_tests { ) .await; } + + #[test] + async fn test_suggests_pub_use() { + let src = r#" + mod bar { + mod baz { + mod coco {} + } + + pub use baz::coco; + } + + fn main() { + bar::c>|< + } + "#; + assert_completion(src, vec![module_completion_item("coco")]).await; + } + + #[test] + async fn test_auto_import_suggests_pub_use_for_module() { + let src = r#" + mod bar { + mod baz { + mod coco {} + } + + pub use baz::coco as foobar; + } + + fn main() { + foob>|< + } + "#; + + let items = get_completions(src).await; + assert_eq!(items.len(), 1); + + let item = &items[0]; + assert_eq!(item.label, "foobar"); + assert_eq!( + item.label_details.as_ref().unwrap().detail, + Some("(use bar::foobar)".to_string()), + ); + } + + #[test] + async fn test_auto_import_suggests_pub_use_for_function() { + let src = r#" + mod bar { + mod baz { + pub fn coco() {} + } + + pub use baz::coco as foobar; + } + + fn main() { + foob>|< + } + "#; + + let items = get_completions(src).await; + assert_eq!(items.len(), 1); + + let item = &items[0]; + assert_eq!(item.label, "foobar()"); + assert_eq!( + item.label_details.as_ref().unwrap().detail, + Some("(use bar::foobar)".to_string()), + ); + } } diff --git a/tooling/nargo/src/package.rs b/tooling/nargo/src/package.rs index cde616a9e32..f55ca5550a3 100644 --- a/tooling/nargo/src/package.rs +++ b/tooling/nargo/src/package.rs @@ -73,11 +73,4 @@ 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 1130a82fdfc..5239070b4d2 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, CheckOptions, CompileOptions, + check_crate, compute_function_abi, file_manager_with_stdlib, CompileOptions, NOIR_ARTIFACT_VERSION_STRING, }; use noirc_frontend::{ @@ -81,9 +81,7 @@ fn check_package( allow_overwrite: bool, ) -> Result { 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::new(compile_options, error_on_unused_imports); - check_crate_and_report_errors(&mut context, crate_id, &check_options)?; + check_crate_and_report_errors(&mut context, crate_id, compile_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 +150,9 @@ fn create_input_toml_template( pub(crate) fn check_crate_and_report_errors( context: &mut Context, crate_id: CrateId, - check_options: &CheckOptions, + options: &CompileOptions, ) -> Result<(), CompileError> { - let options = &check_options.compile_options; - let result = check_crate(context, crate_id, check_options); + 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 5721dd33e27..19add7f30dc 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, CheckOptions, CompileOptions, CompiledProgram, + compile_no_check, file_manager_with_stdlib, CompileOptions, CompiledProgram, NOIR_ARTIFACT_VERSION_STRING, }; @@ -83,9 +83,7 @@ fn compile_exported_functions( 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(); - let check_options = CheckOptions::new(compile_options, error_on_unused_imports); - check_crate_and_report_errors(&mut context, crate_id, &check_options)?; + check_crate_and_report_errors(&mut context, crate_id, compile_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 751d49e6427..2f9390d72e0 100644 --- a/tooling/nargo_cli/src/cli/test_cmd.rs +++ b/tooling/nargo_cli/src/cli/test_cmd.rs @@ -10,8 +10,7 @@ use nargo::{ }; use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; use noirc_driver::{ - check_crate, file_manager_with_stdlib, CheckOptions, CompileOptions, - NOIR_ARTIFACT_VERSION_STRING, + check_crate, file_manager_with_stdlib, CompileOptions, NOIR_ARTIFACT_VERSION_STRING, }; use noirc_frontend::{ graph::CrateName, @@ -186,9 +185,7 @@ fn run_test + Default>( // We then need to construct a separate copy for each test. 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::new(compile_options, error_on_unused_imports); - check_crate(&mut context, crate_id, &check_options) + check_crate(&mut context, crate_id, compile_options) .expect("Any errors should have occurred when collecting test functions"); let test_functions = context @@ -217,9 +214,7 @@ fn get_tests_in_package( 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(); - let check_options = CheckOptions::new(options, error_on_unused_imports); - check_crate_and_report_errors(&mut context, crate_id, &check_options)?; + check_crate_and_report_errors(&mut context, crate_id, options)?; Ok(context .get_all_test_functions_in_crate_matching(&crate_id, fn_name) diff --git a/tooling/nargo_fmt/src/rewrite/imports.rs b/tooling/nargo_fmt/src/rewrite/imports.rs index 025d354259e..6c63c551f7d 100644 --- a/tooling/nargo_fmt/src/rewrite/imports.rs +++ b/tooling/nargo_fmt/src/rewrite/imports.rs @@ -1,4 +1,4 @@ -use noirc_frontend::ast; +use noirc_frontend::ast::{self, ItemVisibility}; use crate::{ items::Item, @@ -96,8 +96,18 @@ impl UseTree { result } - pub(crate) fn rewrite_top_level(&self, visitor: &FmtVisitor, shape: Shape) -> String { - format!("use {};", self.rewrite(visitor, shape)) + pub(crate) fn rewrite_top_level( + &self, + visitor: &FmtVisitor, + shape: Shape, + visibility: ItemVisibility, + ) -> String { + let rewrite = self.rewrite(visitor, shape); + if visibility == ItemVisibility::Private { + format!("use {};", rewrite) + } else { + format!("{} use {};", visibility, rewrite) + } } fn rewrite(&self, visitor: &FmtVisitor, shape: Shape) -> String { diff --git a/tooling/nargo_fmt/src/visitor/item.rs b/tooling/nargo_fmt/src/visitor/item.rs index 94a32449ebe..0e2d07f13d0 100644 --- a/tooling/nargo_fmt/src/visitor/item.rs +++ b/tooling/nargo_fmt/src/visitor/item.rs @@ -216,9 +216,9 @@ impl super::FmtVisitor<'_> { self.last_position = span.end(); } } - ItemKind::Import(use_tree) => { - let use_tree = - UseTree::from_ast(use_tree).rewrite_top_level(self, self.shape()); + ItemKind::Import(use_tree, visibility) => { + let use_tree = UseTree::from_ast(use_tree); + let use_tree = use_tree.rewrite_top_level(self, self.shape(), visibility); self.push_rewrite(use_tree, span); self.last_position = span.end(); }